Rollup merge of #111095 - GuillaumeGomez:fix-assoc-item-trait-inside-hidden, r=notriddle

matthiaskrgr · web-flow · commit d117b4140974 · 2023-05-10T06:12:14.000+02:00
Correctly handle associated items of a trait inside a `#[doc(hidden)]` item Fixes #111064. cc `@compiler-errors` r? `@notriddle`
diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs
@@ -119,7 +119,39 @@ pub(crate) fn clean_doc_module<'tcx>(doc: &DocModule<'tcx>, cx: &mut DocContext<
     });
 
     let kind = ModuleItem(Module { items, span });
-    Item::from_def_id_and_parts(doc.def_id.to_def_id(), Some(doc.name), kind, cx)
+    generate_item_with_correct_attrs(cx, kind, doc.def_id, doc.name, doc.import_id, doc.renamed)
+}
+
+fn generate_item_with_correct_attrs(
+    cx: &mut DocContext<'_>,
+    kind: ItemKind,
+    local_def_id: LocalDefId,
+    name: Symbol,
+    import_id: Option<LocalDefId>,
+    renamed: Option<Symbol>,
+) -> Item {
+    let def_id = local_def_id.to_def_id();
+    let target_attrs = inline::load_attrs(cx, def_id);
+    let attrs = if let Some(import_id) = import_id {
+        let is_inline = inline::load_attrs(cx, import_id.to_def_id())
+            .lists(sym::doc)
+            .get_word_attr(sym::inline)
+            .is_some();
+        let mut attrs = get_all_import_attributes(cx, import_id, local_def_id, is_inline);
+        add_without_unwanted_attributes(&mut attrs, target_attrs, is_inline, None);
+        attrs
+    } else {
+        // We only keep the item's attributes.
+        target_attrs.iter().map(|attr| (Cow::Borrowed(attr), None)).collect()
+    };
+
+    let cfg = attrs.cfg(cx.tcx, &cx.cache.hidden_cfg);
+    let attrs = Attributes::from_ast_iter(attrs.iter().map(|(attr, did)| (&**attr, *did)), false);
+
+    let name = renamed.or(Some(name));
+    let mut item = Item::from_def_id_and_attrs_and_parts(def_id, name, kind, Box::new(attrs), cfg);
+    item.inline_stmt_id = import_id.map(|local| local.to_def_id());
+    item
 }
 
 fn clean_generic_bound<'tcx>(
@@ -2345,29 +2377,14 @@ fn clean_maybe_renamed_item<'tcx>(
             _ => unreachable!("not yet converted"),
         };
 
-        let target_attrs = inline::load_attrs(cx, def_id);
-        let attrs = if let Some(import_id) = import_id {
-            let is_inline = inline::load_attrs(cx, import_id.to_def_id())
-                .lists(sym::doc)
-                .get_word_attr(sym::inline)
-                .is_some();
-            let mut attrs =
-                get_all_import_attributes(cx, import_id, item.owner_id.def_id, is_inline);
-            add_without_unwanted_attributes(&mut attrs, target_attrs, is_inline, None);
-            attrs
-        } else {
-            // We only keep the item's attributes.
-            target_attrs.iter().map(|attr| (Cow::Borrowed(attr), None)).collect()
-        };
-
-        let cfg = attrs.cfg(cx.tcx, &cx.cache.hidden_cfg);
-        let attrs =
-            Attributes::from_ast_iter(attrs.iter().map(|(attr, did)| (&**attr, *did)), false);
-
-        let mut item =
-            Item::from_def_id_and_attrs_and_parts(def_id, Some(name), kind, Box::new(attrs), cfg);
-        item.inline_stmt_id = import_id.map(|local| local.to_def_id());
-        vec![item]
+        vec![generate_item_with_correct_attrs(
+            cx,
+            kind,
+            item.owner_id.def_id,
+            name,
+            import_id,
+            renamed,
+        )]
     })
 }
 
diff --git a/src/librustdoc/formats/cache.rs b/src/librustdoc/formats/cache.rs
@@ -195,7 +195,13 @@ impl Cache {
 impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> {
     fn fold_item(&mut self, item: clean::Item) -> Option<clean::Item> {
         if item.item_id.is_local() {
-            debug!("folding {} \"{:?}\", id {:?}", item.type_(), item.name, item.item_id);
+            let is_stripped = matches!(*item.kind, clean::ItemKind::StrippedItem(..));
+            debug!(
+                "folding {} (stripped: {is_stripped:?}) \"{:?}\", id {:?}",
+                item.type_(),
+                item.name,
+                item.item_id
+            );
         }
 
         // If this is a stripped module,
diff --git a/src/librustdoc/passes/check_doc_test_visibility.rs b/src/librustdoc/passes/check_doc_test_visibility.rs
@@ -95,7 +95,7 @@ pub(crate) fn should_have_doc_example(cx: &DocContext<'_>, item: &clean::Item) -
     }
 
     if cx.tcx.is_doc_hidden(def_id.to_def_id())
-        || inherits_doc_hidden(cx.tcx, def_id)
+        || inherits_doc_hidden(cx.tcx, def_id, None)
         || cx.tcx.def_span(def_id.to_def_id()).in_derive_expansion()
     {
         return false;
diff --git a/src/librustdoc/passes/strip_hidden.rs b/src/librustdoc/passes/strip_hidden.rs
@@ -1,5 +1,6 @@
 //! Strip all doc(hidden) items from the output.
 
+use rustc_hir::def_id::LocalDefId;
 use rustc_middle::ty::TyCtxt;
 use rustc_span::symbol::sym;
 use std::mem;
@@ -29,6 +30,7 @@ pub(crate) fn strip_hidden(krate: clean::Crate, cx: &mut DocContext<'_>) -> clea
             update_retained: true,
             tcx: cx.tcx,
             is_in_hidden_item: false,
+            last_reexport: None,
         };
         stripper.fold_crate(krate)
     };
@@ -49,13 +51,24 @@ struct Stripper<'a, 'tcx> {
     update_retained: bool,
     tcx: TyCtxt<'tcx>,
     is_in_hidden_item: bool,
+    last_reexport: Option<LocalDefId>,
 }
 
 impl<'a, 'tcx> Stripper<'a, 'tcx> {
+    fn set_last_reexport_then_fold_item(&mut self, i: Item) -> Item {
+        let prev_from_reexport = self.last_reexport;
+        if i.inline_stmt_id.is_some() {
+            self.last_reexport = i.item_id.as_def_id().and_then(|def_id| def_id.as_local());
+        }
+        let ret = self.fold_item_recur(i);
+        self.last_reexport = prev_from_reexport;
+        ret
+    }
+
     fn set_is_in_hidden_item_and_fold(&mut self, is_in_hidden_item: bool, i: Item) -> Item {
         let prev = self.is_in_hidden_item;
         self.is_in_hidden_item |= is_in_hidden_item;
-        let ret = self.fold_item_recur(i);
+        let ret = self.set_last_reexport_then_fold_item(i);
         self.is_in_hidden_item = prev;
         ret
     }
@@ -64,7 +77,7 @@ impl<'a, 'tcx> Stripper<'a, 'tcx> {
     /// of `is_in_hidden_item` to `true` because the impl children inherit its visibility.
     fn recurse_in_impl_or_exported_macro(&mut self, i: Item) -> Item {
         let prev = mem::replace(&mut self.is_in_hidden_item, false);
-        let ret = self.fold_item_recur(i);
+        let ret = self.set_last_reexport_then_fold_item(i);
         self.is_in_hidden_item = prev;
         ret
     }
@@ -86,13 +99,20 @@ impl<'a, 'tcx> DocFolder for Stripper<'a, 'tcx> {
         if !is_impl_or_exported_macro {
             is_hidden = self.is_in_hidden_item || has_doc_hidden;
             if !is_hidden && i.inline_stmt_id.is_none() {
-                // We don't need to check if it's coming from a reexport since the reexport itself was
-                // already checked.
+                // `i.inline_stmt_id` is `Some` if the item is directly reexported. If it is, we
+                // don't need to check it, because the reexport itself was already checked.
+                //
+                // If this item is the child of a reexported module, `self.last_reexport` will be
+                // `Some` even though `i.inline_stmt_id` is `None`. Hiddenness inheritance needs to
+                // account for the possibility that an item's true parent module is hidden, but it's
+                // inlined into a visible module true. This code shouldn't be reachable if the
+                // module's reexport is itself hidden, for the same reason it doesn't need to be
+                // checked if `i.inline_stmt_id` is Some: hidden reexports are never inlined.
                 is_hidden = i
                     .item_id
                     .as_def_id()
                     .and_then(|def_id| def_id.as_local())
-                    .map(|def_id| inherits_doc_hidden(self.tcx, def_id))
+                    .map(|def_id| inherits_doc_hidden(self.tcx, def_id, self.last_reexport))
                     .unwrap_or(false);
             }
         }
diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs
@@ -27,6 +27,8 @@ pub(crate) struct Module<'hir> {
     pub(crate) where_inner: Span,
     pub(crate) mods: Vec<Module<'hir>>,
     pub(crate) def_id: LocalDefId,
+    pub(crate) renamed: Option<Symbol>,
+    pub(crate) import_id: Option<LocalDefId>,
     /// The key is the item `ItemId` and the value is: (item, renamed, import_id).
     /// We use `FxIndexMap` to keep the insert order.
     pub(crate) items: FxIndexMap<
@@ -37,11 +39,19 @@ pub(crate) struct Module<'hir> {
 }
 
 impl Module<'_> {
-    pub(crate) fn new(name: Symbol, def_id: LocalDefId, where_inner: Span) -> Self {
+    pub(crate) fn new(
+        name: Symbol,
+        def_id: LocalDefId,
+        where_inner: Span,
+        renamed: Option<Symbol>,
+        import_id: Option<LocalDefId>,
+    ) -> Self {
         Module {
             name,
             def_id,
             where_inner,
+            renamed,
+            import_id,
             mods: Vec::new(),
             items: FxIndexMap::default(),
             foreigns: Vec::new(),
@@ -60,9 +70,16 @@ fn def_id_to_path(tcx: TyCtxt<'_>, did: DefId) -> Vec<Symbol> {
     std::iter::once(crate_name).chain(relative).collect()
 }
 
-pub(crate) fn inherits_doc_hidden(tcx: TyCtxt<'_>, mut def_id: LocalDefId) -> bool {
+pub(crate) fn inherits_doc_hidden(
+    tcx: TyCtxt<'_>,
+    mut def_id: LocalDefId,
+    stop_at: Option<LocalDefId>,
+) -> bool {
     let hir = tcx.hir();
     while let Some(id) = tcx.opt_local_parent(def_id) {
+        if let Some(stop_at) = stop_at && id == stop_at {
+            return false;
+        }
         def_id = id;
         if tcx.is_doc_hidden(def_id.to_def_id()) {
             return true;
@@ -100,6 +117,8 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
             cx.tcx.crate_name(LOCAL_CRATE),
             CRATE_DEF_ID,
             cx.tcx.hir().root_module().spans.inner_span,
+            None,
+            None,
         );
 
         RustdocVisitor {
@@ -261,7 +280,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
 
         let is_private =
             !self.cx.cache.effective_visibilities.is_directly_public(self.cx.tcx, ori_res_did);
-        let is_hidden = inherits_doc_hidden(self.cx.tcx, res_did);
+        let is_hidden = inherits_doc_hidden(self.cx.tcx, res_did, None);
 
         // Only inline if requested or if the item would otherwise be stripped.
         if (!please_inline && !is_private && !is_hidden) || is_no_inline {
@@ -278,7 +297,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                 .cache
                 .effective_visibilities
                 .is_directly_public(self.cx.tcx, item_def_id.to_def_id()) &&
-            !inherits_doc_hidden(self.cx.tcx, item_def_id)
+            !inherits_doc_hidden(self.cx.tcx, item_def_id, None)
         {
             // The imported item is public and not `doc(hidden)` so no need to inline it.
             return false;
@@ -427,7 +446,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                 }
             }
             hir::ItemKind::Mod(ref m) => {
-                self.enter_mod(item.owner_id.def_id, m, name);
+                self.enter_mod(item.owner_id.def_id, m, name, renamed, import_id);
             }
             hir::ItemKind::Fn(..)
             | hir::ItemKind::ExternCrate(..)
@@ -480,8 +499,15 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
     /// This method will create a new module and push it onto the "modules stack" then call
     /// `visit_mod_contents`. Once done, it'll remove it from the "modules stack" and instead
     /// add into the list of modules of the current module.
-    fn enter_mod(&mut self, id: LocalDefId, m: &'tcx hir::Mod<'tcx>, name: Symbol) {
-        self.modules.push(Module::new(name, id, m.spans.inner_span));
+    fn enter_mod(
+        &mut self,
+        id: LocalDefId,
+        m: &'tcx hir::Mod<'tcx>,
+        name: Symbol,
+        renamed: Option<Symbol>,
+        import_id: Option<LocalDefId>,
+    ) {
+        self.modules.push(Module::new(name, id, m.spans.inner_span, renamed, import_id));
 
         self.visit_mod_contents(id, m);
 
@@ -501,19 +527,14 @@ impl<'a, 'tcx> Visitor<'tcx> for RustdocVisitor<'a, 'tcx> {
 
     fn visit_item(&mut self, i: &'tcx hir::Item<'tcx>) {
         self.visit_item_inner(i, None, None);
-        let new_value = if self.is_importable_from_parent {
-            matches!(
+        let new_value = self.is_importable_from_parent
+            && matches!(
                 i.kind,
                 hir::ItemKind::Mod(..)
                     | hir::ItemKind::ForeignMod { .. }
                     | hir::ItemKind::Impl(..)
                     | hir::ItemKind::Trait(..)
-            )
-        } else {
-            // Whatever the context, if it's an impl block, the items inside it can be used so they
-            // should be visible.
-            matches!(i.kind, hir::ItemKind::Impl(..))
-        };
+            );
         let prev = mem::replace(&mut self.is_importable_from_parent, new_value);
         walk_item(self, i);
         self.is_importable_from_parent = prev;
diff --git a/tests/rustdoc/issue-111064-reexport-trait-from-hidden-2.rs b/tests/rustdoc/issue-111064-reexport-trait-from-hidden-2.rs
@@ -0,0 +1,31 @@
+#![feature(no_core)]
+#![no_core]
+#![crate_name = "foo"]
+
+// @!has 'foo/hidden/index.html'
+// FIXME: add missing `@` for the two next tests once issue is fixed!
+// To be done in <https://github.com/rust-lang/rust/issues/111249>.
+// !has 'foo/hidden/inner/index.html'
+// !has 'foo/hidden/inner/trait.Foo.html'
+#[doc(hidden)]
+pub mod hidden {
+    pub mod inner {
+        pub trait Foo {
+            /// Hello, world!
+            fn test();
+        }
+    }
+}
+
+// @has 'foo/visible/index.html'
+// @has 'foo/visible/trait.Foo.html'
+#[doc(inline)]
+pub use hidden::inner as visible;
+
+// @has 'foo/struct.Bar.html'
+// @count - '//*[@id="impl-Foo-for-Bar"]' 1
+pub struct Bar;
+
+impl visible::Foo for Bar {
+    fn test() {}
+}
diff --git a/tests/rustdoc/issue-111064-reexport-trait-from-hidden.rs b/tests/rustdoc/issue-111064-reexport-trait-from-hidden.rs
@@ -0,0 +1,21 @@
+// Regression test for <https://github.com/rust-lang/rust/issues/111064>.
+// Methods from a re-exported trait inside a `#[doc(hidden)]` item should
+// be visible.
+
+#![crate_name = "foo"]
+
+// @has 'foo/index.html'
+// @has - '//*[@id="main-content"]//*[@class="item-name"]/a[@href="trait.Foo.html"]' 'Foo'
+
+// @has 'foo/trait.Foo.html'
+// @has - '//*[@id="main-content"]//*[@class="code-header"]' 'fn test()'
+
+#[doc(hidden)]
+mod hidden {
+    pub trait Foo {
+        /// Hello, world!
+        fn test();
+    }
+}
+
+pub use hidden::Foo;

Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ pub(crate) fn should_have_doc_example(cx: &DocContext<'_>, item: &clean::Item) -`
`95`	`95`	`}`
`96`	`96`
`97`	`97`	`if cx.tcx.is_doc_hidden(def_id.to_def_id())`
`98`		`- \|\| inherits_doc_hidden(cx.tcx, def_id)`
	`98`	`+ \|\| inherits_doc_hidden(cx.tcx, def_id, None)`
`99`	`99`	`\|\| cx.tcx.def_span(def_id.to_def_id()).in_derive_expansion()`
`100`	`100`	`{`
`101`	`101`	`return false;`