rustdoc: Correctly handle reexports when --document-private-items is used

[GuillaumeGomez]

Fixes #159109.

This PR makes the (private) reexport work like the public ones when the --document-private-items option is used. I still differentiate between imports (use) and reexports (pub(...) use) in this PR as I don't think we should display imports.

r? @Urgau

[GuillaumeGomez]

Fun fact: this test was added in 2016 in #37773. :)

View changes since the review

[Urgau]

I don't like this hack. I think we would be better of checking if the visibility of the use item is different from where the use item is defined.

I'm thinking of something like this:

let parent_module = tcx.parent_module_from_def_id(def_id);
let is_exported = tcx.visibility(def_id) != Visibility::Restricted(parent_module.to_mod_id());
// or maybe call Visibility::greater_than

View changes since the review

[GuillaumeGomez]

That's... strange. With this code:

pub(crate) mod foo {
    pub(crate) use self::x::Y;

    #[doc(hidden)]
    mod x {
        struct Y;
    }
}

It means we would not see Y. I think the only thing we want to care about is whether it's a reexport or not. And the only way to see that is if there is a pub(), which we know with vis_span. Not a trick imo.

[Urgau]

It means we would not see Y

I think we would, parent_module should be mod foo and tcx.visibility(def_id) should be CRATE_DEF_ID and those do not match, implying that there is a pub(...) token. It's possible my logic is wrong and should be adjusted, but I think something like that should work.

And the only way to see that is if there is a pub(), which we know with vis_span. Not a trick imo.

Spans can be arbitrarily manipulated by proc-macros, a proc-macro could assign a empty span to the pub(...) tokens and your check would be bypassed. We should never rely on spans for correctness.

[GuillaumeGomez]

Fair point. I'll test locally and we'll see if use without visibility are correctly handled.

[GuillaumeGomez]

Outcome:

11: count check failed
	Expected 4 occurrences but found 1
	//@ count - '//*[@class="item-table"]/dt' 4
13: has check failed
	`XPATH PATTERN` did not match
	//@ has - '//*[@class="item-table"]/dt/a[@href="struct.Top.html"]' 'Top'
14: has check failed
	`XPATH PATTERN` did not match
	//@ has - '//*[@class="item-table"]/dt/a[@href="struct.NotHidden.html"]' 'NotHidden'
15: has check failed
	`XPATH PATTERN` did not match
	//@ has - '//*[@class="item-table"]/dt/a[@href="struct.Hidden2.html"]' 'Hidden2'

For this diff:

--- a/src/librustdoc/visit_ast.rs
+++ b/src/librustdoc/visit_ast.rs
@@ -476,7 +476,12 @@ fn visit_item_inner(
         // a `pub(...)` part with its `vis_span` field.
         let is_pub = tcx.visibility(def_id).is_public()
             || (self.cx.document_private()
-                && (!matches!(item.kind, hir::ItemKind::Use(..)) || !item.vis_span.is_empty()));
+                && (!matches!(item.kind, hir::ItemKind::Use(..))
+                    || def_id.as_local().is_some_and(|def_id| {
+                        let parent_module = tcx.parent_module_from_def_id(def_id);
+                        tcx.visibility(def_id)
+                            != rustc_middle::ty::Visibility::Restricted(parent_module.to_mod_id())
+                    })));
 
         if is_pub {
             self.store_path(item.owner_id.to_def_id());

Seems like this approach is not working.

[Urgau]

I didn't think of the interaction of pub(crate) use ... at crate-root, both the visibility and parent module are equal is that case.

My solution do still seems to works when inside a module (and technically also discards pub(self), which is a no-op anyway).

Not sure how to handle use statements at crate-root.

[Urgau]

I wonder if we couldn't somehow use our friend reexport_chain.

[Urgau]

Actually, do we even care about pub(...) use at crate-root? Having it or not doesn't seems to change the semantics.

mod foo {
    pub struct Bar;
}

// any of the three make the use statement in lol works, but remove it and it doesn't anymore

use foo::Bar;
//pub(crate) use foo::Bar;
//pub(self) use foo::Bar;

mod lol {
    use crate::Bar;
}

EDIT: I never actually realized that every use statement is technically a re-export, and that even without any pub(...) it's always accessible to children modules.

This then begs the question of which reexport we are looking for? I think we want use statement that are also available to the parents (so pub(crate) and pub, but not pub(self) or nothing), right?

That would mean that we ignore reexport at crate-root since except for pub use (which is already handled) every other (pub(crate), pub(self), nothing) are semantically equal at crate-root.

[Urgau]

I was wondering if this was a breaking change, but our documentation clearly states:

--document-private-items includes all non-public items in the generated documentation except for #[doc(hidden)] items.

as such, this seems clearly like a bug fix.

[rust-log-analyzer]

The job aarch64-gnu-llvm-21-1 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)