From 879f8de4096b2db4769e64e4c1af5ffb10b53a22 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 2 May 2023 14:35:34 +0200 Subject: [PATCH 1/4] Correctly handle associated items of a trait inside a `#[doc(hidden)]` item --- src/librustdoc/formats/cache.rs | 8 ++++++- src/librustdoc/passes/strip_hidden.rs | 30 ++++++++++++++++++++++----- src/librustdoc/visit_ast.rs | 11 +++------- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/src/librustdoc/formats/cache.rs b/src/librustdoc/formats/cache.rs index 841abfab666..c0730e90740 100644 --- a/src/librustdoc/formats/cache.rs +++ b/src/librustdoc/formats/cache.rs @@ -195,7 +195,13 @@ pub(crate) fn populate(cx: &mut DocContext<'_>, mut krate: clean::Crate) -> clea impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> { fn fold_item(&mut self, item: clean::Item) -> Option { 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/strip_hidden.rs b/src/librustdoc/passes/strip_hidden.rs index a688aa14863..972b0c5ec19 100644 --- 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, } 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 @@ fn set_is_in_hidden_item_and_fold(&mut self, is_in_hidden_item: bool, i: Item) - /// 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 @@ fn fold_item(&mut self, i: Item) -> Option { 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 index a6089680fae..a6c24041380 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -500,19 +500,14 @@ fn nested_visit_map(&mut self) -> Self::Map { 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; From fb160d5d3b30ad4a522149d309002fd76137b048 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 5 May 2023 15:32:56 +0200 Subject: [PATCH 2/4] Modules can be reexported and it should be handled by rustdoc --- src/librustdoc/clean/mod.rs | 23 ++++++++++- .../passes/check_doc_test_visibility.rs | 2 +- src/librustdoc/visit_ast.rs | 40 +++++++++++++++---- 3 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 8ccdb16b784..d21a8a5477f 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -119,7 +119,28 @@ 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) + let def_id = doc.def_id.to_def_id(); + let target_attrs = inline::load_attrs(cx, def_id); + let attrs = if let Some(import_id) = doc.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, doc.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 = doc.renamed.or_else(|| Some(doc.name)); + let mut item = Item::from_def_id_and_attrs_and_parts(def_id, name, kind, Box::new(attrs), cfg); + item.inline_stmt_id = doc.import_id.map(|local| local.to_def_id()); + item } fn clean_generic_bound<'tcx>( diff --git a/src/librustdoc/passes/check_doc_test_visibility.rs b/src/librustdoc/passes/check_doc_test_visibility.rs index 6b13e6c9581..10295cbd189 100644 --- 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/visit_ast.rs b/src/librustdoc/visit_ast.rs index a6c24041380..f7c525042c2 100644 --- 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>, pub(crate) def_id: LocalDefId, + pub(crate) renamed: Option, + pub(crate) import_id: Option, /// 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, + import_id: Option, + ) -> 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 { 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, +) -> 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 @@ pub(crate) fn new(cx: &'a mut core::DocContext<'tcx>) -> RustdocVisitor<'a, 'tcx cx.tcx.crate_name(LOCAL_CRATE), CRATE_DEF_ID, cx.tcx.hir().root_module().spans.inner_span, + None, + None, ); RustdocVisitor { @@ -260,7 +279,7 @@ fn maybe_inline_local( 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 { @@ -277,7 +296,7 @@ fn maybe_inline_local( .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; @@ -426,7 +445,7 @@ fn visit_item_inner( } } 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(..) @@ -479,8 +498,15 @@ fn visit_foreign_item_inner( /// 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, + import_id: Option, + ) { + self.modules.push(Module::new(name, id, m.spans.inner_span, renamed, import_id)); self.visit_mod_contents(id, m); From cbc6daa5597916c12ee24a6870f7611adf989878 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 5 May 2023 15:59:41 +0200 Subject: [PATCH 3/4] Improve code to remove duplication --- src/librustdoc/clean/mod.rs | 52 +++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index d21a8a5477f..53b631b986d 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -119,14 +119,25 @@ pub(crate) fn clean_doc_module<'tcx>(doc: &DocModule<'tcx>, cx: &mut DocContext< }); let kind = ModuleItem(Module { items, span }); - let def_id = doc.def_id.to_def_id(); + 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, + renamed: Option, +) -> 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) = doc.import_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, doc.def_id, is_inline); + 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 { @@ -137,9 +148,9 @@ pub(crate) fn clean_doc_module<'tcx>(doc: &DocModule<'tcx>, cx: &mut DocContext< 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 = doc.renamed.or_else(|| Some(doc.name)); + 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 = doc.import_id.map(|local| local.to_def_id()); + item.inline_stmt_id = import_id.map(|local| local.to_def_id()); item } @@ -2309,29 +2320,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, + )] }) } From 8de4308b43e9694384d1292f22aef384dc44a5df Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 2 May 2023 14:35:53 +0200 Subject: [PATCH 4/4] Add regression test for #111064 --- ...sue-111064-reexport-trait-from-hidden-2.rs | 31 +++++++++++++++++++ ...issue-111064-reexport-trait-from-hidden.rs | 21 +++++++++++++ 2 files changed, 52 insertions(+) create mode 100644 tests/rustdoc/issue-111064-reexport-trait-from-hidden-2.rs create mode 100644 tests/rustdoc/issue-111064-reexport-trait-from-hidden.rs diff --git a/tests/rustdoc/issue-111064-reexport-trait-from-hidden-2.rs b/tests/rustdoc/issue-111064-reexport-trait-from-hidden-2.rs new file mode 100644 index 00000000000..8e1029a1ca3 --- /dev/null +++ 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 . +// !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 new file mode 100644 index 00000000000..a9ce4a34507 --- /dev/null +++ b/tests/rustdoc/issue-111064-reexport-trait-from-hidden.rs @@ -0,0 +1,21 @@ +// Regression test for . +// 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;