Rustdoc is inlining use ... imports when --document-private-items is passed

[jyn514]

I tried this code:

use std::env;

I expected to see this happen: The documentation is empty.

Instead, this happened:

Meta

rustdoc --version: rustdoc 1.58.0-nightly (e90c5fb 2021-11-12)

cc @camelid @inquisitivecrystal, I suspect this is related to one of your recent changes to visibility.

[inquisitivecrystal]

This does appear to have been #88447.

I'm not entirely sure how to fix this. Basically, what's happening here is that the is_accessible_from query is checking if the use statement is accessible from its parent module. It is, because the parent module of the top level of the module is itself. Before this, it would have been documented if you did a pub(self) use std::env;, but not with just use std::env;. The new behavior is more consistent, and better in that sense, but perhaps not ideal. Do you have an idea on how you'd like this to work?

[inquisitivecrystal]

Some additional thoughts:

If we want to stick to the "visible from parent module" thing, the simplest way to do that would be to add a check for whether current_module is the top level module. Of course, I'd also have to add a check for the visibility being actually public... sighs. Might need to break this into a seperate thing.

I guess the problem I'm struggling with after thinking about all of this is that I don't fully understand the justification for all of this logic. Why is it that we want to show some imports but not others? If these were structs, we'd just show all of them, regardless of their visibility. That said, if this really is the behavior we want, it shouldn't be hard to implement.

[jyn514]

Basically, what's happening here is that the is_accessible_from query is checking if the use statement is accessible from its parent module. It is, because the parent module of the top level of the module is itself. Before this, it would have been documented if you did a pub(self) use std::env;, but not with just use std::env;.

I think we should inline neither pub(self) nor bare use (pretty sure I said so on the PR, but I guess I forgot to check for a test case).

Why is it that we want to show some imports but not others? If these were structs, we'd just show all of them, regardless of their visibility.

Because the structs are only visible from the module where they're defined. The use statements aren't intended to be used from elsewhere in the crate.

[inquisitivecrystal]

I think we should inline neither pub(self) nor bare use (pretty sure I said so on the PR, but I guess I forgot to check for a test case).

I think I explained this poorly above. We generally don't inline either pub(self) or bare use. Not doing so is the intended behavior from the PR. Because of how I implemented it, the top of the crate ended up being an inadvertent exception, and we didn't have any tests that covered the top of the crate as opposed to lower level modules.

It's an easy fix if we want it not to be an exception, I was just trying to make sure we had a clear philosophical reason for wanting things to be the way we were trying to have them be. (In other words, I was double-checking that the behavior the PR was supposed to create is really what we want before submitting a PR to do that.) It sounds like it is, so I'll get working on a bugfix.

I feel like I'm expressing myself rather unclearly at the moment. If there's anything I need to clear up about this explanation, please feel free to let me know.

[jyn514]

Because of how I implemented it, the top of the crate ended up being an inadvertent exception, and we didn't have any tests that covered the top of the crate as opposed to lower level modules.

Oh oops, I missed this bit. Yes, I think this should be consistent with the submodules of the crate.

[camelid]

Because of how I implemented it, the top of the crate ended up being an inadvertent exception, and we didn't have any tests that covered the top of the crate as opposed to lower level modules.

Oh oops, I missed this bit. Yes, I think this should be consistent with the submodules of the crate.

I agree, that sounds like the right behavior to me.

[inquisitivecrystal]

Quick update: I've got a substantive patch, but it's taking a while to get the tests to a point where I'm satisfied with them.

[inquisitivecrystal]

My PR is up!

Also, I'm going to assign this as a P-medium, per the discussion in the Zulip thread of the prioritization working group.