From af30e3767e196aa63f813f02910f1d085de89146 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 20 Jan 2023 16:02:31 +0100 Subject: [PATCH 01/10] Fix missing const expression items visit --- src/librustdoc/visit_ast.rs | 299 ++++++++++++++++++++---------------- 1 file changed, 170 insertions(+), 129 deletions(-) diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index a89d6fa83983d..2fbcda35e4417 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -5,7 +5,10 @@ use rustc_data_structures::fx::FxHashSet; use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::{DefId, DefIdMap, LocalDefId, LocalDefIdSet}; +use rustc_hir::intravisit::{walk_item, Visitor}; use rustc_hir::{Node, CRATE_HIR_ID}; +use rustc_middle::hir::map::Map; +use rustc_middle::hir::nested_filter; use rustc_middle::ty::{DefIdTree, TyCtxt}; use rustc_span::def_id::{CRATE_DEF_ID, LOCAL_CRATE}; use rustc_span::symbol::{kw, sym, Symbol}; @@ -63,9 +66,6 @@ pub(crate) fn inherits_doc_hidden(tcx: TyCtxt<'_>, mut node: LocalDefId) -> bool false } -// Also, is there some reason that this doesn't use the 'visit' -// framework from syntax?. - pub(crate) struct RustdocVisitor<'a, 'tcx> { cx: &'a mut core::DocContext<'tcx>, view_item_stack: LocalDefIdSet, @@ -73,6 +73,8 @@ pub(crate) struct RustdocVisitor<'a, 'tcx> { /// Are the current module and all of its parents public? inside_public_path: bool, exact_paths: DefIdMap>, + modules: Vec>, + map: Map<'tcx>, } impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { @@ -80,12 +82,21 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { // If the root is re-exported, terminate all recursion. let mut stack = LocalDefIdSet::default(); stack.insert(CRATE_DEF_ID); + let om = Module::new( + cx.tcx.crate_name(LOCAL_CRATE), + CRATE_DEF_ID, + cx.tcx.hir().root_module().spans.inner_span, + ); + let map = cx.tcx.hir(); + RustdocVisitor { cx, view_item_stack: stack, inlining: false, inside_public_path: true, exact_paths: Default::default(), + modules: vec![om], + map, } } @@ -94,100 +105,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { self.exact_paths.entry(did).or_insert_with(|| def_id_to_path(tcx, did)); } - pub(crate) fn visit(mut self) -> Module<'tcx> { - let mut top_level_module = self.visit_mod_contents( - CRATE_DEF_ID, - self.cx.tcx.hir().root_module(), - self.cx.tcx.crate_name(LOCAL_CRATE), - None, - ); - - // `#[macro_export] macro_rules!` items are reexported at the top level of the - // crate, regardless of where they're defined. We want to document the - // top level rexport of the macro, not its original definition, since - // the rexport defines the path that a user will actually see. Accordingly, - // we add the rexport as an item here, and then skip over the original - // definition in `visit_item()` below. - // - // We also skip `#[macro_export] macro_rules!` that have already been inserted, - // it can happen if within the same module a `#[macro_export] macro_rules!` - // is declared but also a reexport of itself producing two exports of the same - // macro in the same module. - let mut inserted = FxHashSet::default(); - for export in self.cx.tcx.module_reexports(CRATE_DEF_ID).unwrap_or(&[]) { - if let Res::Def(DefKind::Macro(_), def_id) = export.res { - if let Some(local_def_id) = def_id.as_local() { - if self.cx.tcx.has_attr(def_id, sym::macro_export) { - if inserted.insert(def_id) { - let item = self.cx.tcx.hir().expect_item(local_def_id); - top_level_module.items.push((item, None, None)); - } - } - } - } - } - - self.cx.cache.hidden_cfg = self - .cx - .tcx - .hir() - .attrs(CRATE_HIR_ID) - .iter() - .filter(|attr| attr.has_name(sym::doc)) - .flat_map(|attr| attr.meta_item_list().into_iter().flatten()) - .filter(|attr| attr.has_name(sym::cfg_hide)) - .flat_map(|attr| { - attr.meta_item_list() - .unwrap_or(&[]) - .iter() - .filter_map(|attr| { - Cfg::parse(attr.meta_item()?) - .map_err(|e| self.cx.sess().diagnostic().span_err(e.span, e.msg)) - .ok() - }) - .collect::>() - }) - .chain( - [Cfg::Cfg(sym::test, None), Cfg::Cfg(sym::doc, None), Cfg::Cfg(sym::doctest, None)] - .into_iter(), - ) - .collect(); - - self.cx.cache.exact_paths = self.exact_paths; - top_level_module - } - - fn visit_mod_contents( - &mut self, - def_id: LocalDefId, - m: &'tcx hir::Mod<'tcx>, - name: Symbol, - parent_id: Option, - ) -> Module<'tcx> { - let mut om = Module::new(name, def_id, m.spans.inner_span); - // Keep track of if there were any private modules in the path. - let orig_inside_public_path = self.inside_public_path; - self.inside_public_path &= self.cx.tcx.local_visibility(def_id).is_public(); - for &i in m.item_ids { - let item = self.cx.tcx.hir().item(i); - if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) { - continue; - } - self.visit_item(item, None, &mut om, parent_id); - } - for &i in m.item_ids { - let item = self.cx.tcx.hir().item(i); - // To match the way import precedence works, visit glob imports last. - // Later passes in rustdoc will de-duplicate by name and kind, so if glob- - // imported items appear last, then they'll be the ones that get discarded. - if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) { - self.visit_item(item, None, &mut om, parent_id); - } - } - self.inside_public_path = orig_inside_public_path; - om - } - /// Tries to resolve the target of a `pub use` statement and inlines the /// target if it is defined locally and would not be documented otherwise, /// or when it is specifically requested with `please_inline`. @@ -203,7 +120,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { res: Res, renamed: Option, glob: bool, - om: &mut Module<'tcx>, please_inline: bool, ) -> bool { debug!("maybe_inline_local res: {:?}", res); @@ -213,33 +129,30 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { } let tcx = self.cx.tcx; - let Some(res_did) = res.opt_def_id() else { + let Some(ori_res_did) = res.opt_def_id() else { return false; }; let use_attrs = tcx.hir().attrs(tcx.hir().local_def_id_to_hir_id(def_id)); // Don't inline `doc(hidden)` imports so they can be stripped at a later stage. let is_no_inline = use_attrs.lists(sym::doc).has_word(sym::no_inline) - || tcx.is_doc_hidden(def_id.to_def_id()); + || use_attrs.lists(sym::doc).has_word(sym::hidden); // For cross-crate impl inlining we need to know whether items are // reachable in documentation -- a previously unreachable item can be // made reachable by cross-crate inlining which we're checking here. // (this is done here because we need to know this upfront). - if !res_did.is_local() && !is_no_inline { - crate::visit_lib::lib_embargo_visit_item(self.cx, res_did); + if !ori_res_did.is_local() && !is_no_inline { + crate::visit_lib::lib_embargo_visit_item(self.cx, ori_res_did); return false; } - let Some(res_did) = res_did.as_local() else { + let Some(res_did) = ori_res_did.as_local() else { return false; }; - let is_private = !self - .cx - .cache - .effective_visibilities - .is_directly_public(self.cx.tcx, res_did.to_def_id()); + 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); // Only inline if requested or if the item would otherwise be stripped. @@ -256,20 +169,20 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { let prev = mem::replace(&mut self.inlining, true); for &i in m.item_ids { let i = self.cx.tcx.hir().item(i); - self.visit_item(i, None, om, Some(def_id)); + self.visit_item_inner(i, None, Some(def_id)); } self.inlining = prev; true } Node::Item(it) if !glob => { let prev = mem::replace(&mut self.inlining, true); - self.visit_item(it, renamed, om, Some(def_id)); + self.visit_item_inner(it, renamed, Some(def_id)); self.inlining = prev; true } Node::ForeignItem(it) if !glob => { let prev = mem::replace(&mut self.inlining, true); - self.visit_foreign_item(it, renamed, om); + self.visit_foreign_item_inner(it, renamed); self.inlining = prev; true } @@ -279,18 +192,18 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { ret } - fn visit_item( + fn visit_item_inner( &mut self, item: &'tcx hir::Item<'_>, renamed: Option, - om: &mut Module<'tcx>, parent_id: Option, - ) { + ) -> bool { debug!("visiting item {:?}", item); let name = renamed.unwrap_or(item.ident.name); + let tcx = self.cx.tcx; let def_id = item.owner_id.to_def_id(); - let is_pub = self.cx.tcx.visibility(def_id).is_public(); + let is_pub = tcx.visibility(def_id).is_public(); if is_pub { self.store_path(item.owner_id.to_def_id()); @@ -299,8 +212,8 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { match item.kind { hir::ItemKind::ForeignMod { items, .. } => { for item in items { - let item = self.cx.tcx.hir().foreign_item(item.id); - self.visit_foreign_item(item, None, om); + let item = tcx.hir().foreign_item(item.id); + self.visit_foreign_item_inner(item, None); } } // If we're inlining, skip private items or item reexported as "_". @@ -315,7 +228,8 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { continue; } - let attrs = self.cx.tcx.hir().attrs(item.hir_id()); + let attrs = + tcx.hir().attrs(tcx.hir().local_def_id_to_hir_id(item.owner_id.def_id)); // If there was a private module in the current path then don't bother inlining // anything as it will probably be stripped anyway. @@ -333,14 +247,13 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { res, ident, is_glob, - om, please_inline, ) { continue; } } - om.items.push((item, renamed, parent_id)) + self.modules.last_mut().unwrap().items.push((item, renamed, parent_id)); } } hir::ItemKind::Macro(ref macro_def, _) => { @@ -357,14 +270,14 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { let def_id = item.owner_id.to_def_id(); let is_macro_2_0 = !macro_def.macro_rules; - let nonexported = !self.cx.tcx.has_attr(def_id, sym::macro_export); + let nonexported = !tcx.has_attr(def_id, sym::macro_export); if is_macro_2_0 || nonexported || self.inlining { - om.items.push((item, renamed, None)); + self.modules.last_mut().unwrap().items.push((item, renamed, None)); } } hir::ItemKind::Mod(ref m) => { - om.mods.push(self.visit_mod_contents(item.owner_id.def_id, m, name, parent_id)); + self.enter_mod(item.owner_id.def_id, m, name); } hir::ItemKind::Fn(..) | hir::ItemKind::ExternCrate(..) @@ -375,33 +288,161 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { | hir::ItemKind::OpaqueTy(..) | hir::ItemKind::Static(..) | hir::ItemKind::Trait(..) - | hir::ItemKind::TraitAlias(..) => om.items.push((item, renamed, parent_id)), + | hir::ItemKind::TraitAlias(..) => { + self.modules.last_mut().unwrap().items.push((item, renamed, parent_id)) + } hir::ItemKind::Const(..) => { // Underscore constants do not correspond to a nameable item and // so are never useful in documentation. if name != kw::Underscore { - om.items.push((item, renamed, parent_id)); + self.modules.last_mut().unwrap().items.push((item, renamed, parent_id)); } } hir::ItemKind::Impl(impl_) => { // Don't duplicate impls when inlining or if it's implementing a trait, we'll pick // them up regardless of where they're located. if !self.inlining && impl_.of_trait.is_none() { - om.items.push((item, None, None)); + self.modules.last_mut().unwrap().items.push((item, None, None)); } } } + true } - fn visit_foreign_item( + fn visit_foreign_item_inner( &mut self, item: &'tcx hir::ForeignItem<'_>, renamed: Option, - om: &mut Module<'tcx>, ) { // If inlining we only want to include public functions. if !self.inlining || self.cx.tcx.visibility(item.owner_id).is_public() { - om.foreigns.push((item, renamed)); + self.modules.last_mut().unwrap().foreigns.push((item, renamed)); } } + + pub(crate) fn visit(mut self) -> Module<'tcx> { + let root_module = self.cx.tcx.hir().root_module(); + self.visit_mod_contents(CRATE_DEF_ID, root_module); + + let mut top_level_module = self.modules.pop().unwrap(); + + // `#[macro_export] macro_rules!` items are reexported at the top level of the + // crate, regardless of where they're defined. We want to document the + // top level rexport of the macro, not its original definition, since + // the rexport defines the path that a user will actually see. Accordingly, + // we add the rexport as an item here, and then skip over the original + // definition in `visit_item()` below. + // + // We also skip `#[macro_export] macro_rules!` that have already been inserted, + // it can happen if within the same module a `#[macro_export] macro_rules!` + // is declared but also a reexport of itself producing two exports of the same + // macro in the same module. + let mut inserted = FxHashSet::default(); + for export in self.cx.tcx.module_reexports(CRATE_DEF_ID).unwrap_or(&[]) { + if let Res::Def(DefKind::Macro(_), def_id) = export.res { + if let Some(local_def_id) = def_id.as_local() { + if self.cx.tcx.has_attr(def_id, sym::macro_export) { + if inserted.insert(def_id) { + let item = self.cx.tcx.hir().expect_item(local_def_id); + top_level_module.items.push((item, None, None)); + } + } + } + } + } + + self.cx.cache.hidden_cfg = self + .cx + .tcx + .hir() + .attrs(CRATE_HIR_ID) + .iter() + .filter(|attr| attr.has_name(sym::doc)) + .flat_map(|attr| attr.meta_item_list().into_iter().flatten()) + .filter(|attr| attr.has_name(sym::cfg_hide)) + .flat_map(|attr| { + attr.meta_item_list() + .unwrap_or(&[]) + .iter() + .filter_map(|attr| { + Cfg::parse(attr.meta_item()?) + .map_err(|e| self.cx.sess().diagnostic().span_err(e.span, e.msg)) + .ok() + }) + .collect::>() + }) + .chain( + [Cfg::Cfg(sym::test, None), Cfg::Cfg(sym::doc, None), Cfg::Cfg(sym::doctest, None)] + .into_iter(), + ) + .collect(); + + self.cx.cache.exact_paths = self.exact_paths; + top_level_module + } + + /// 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)); + + self.visit_mod_contents(id, m); + + let last = self.modules.pop().unwrap(); + self.modules.last_mut().unwrap().mods.push(last); + } + + /// This method will go through the given module items in two passes: + /// 1. The items which are not glob imports/reexports. + /// 2. The glob imports/reexports. + fn visit_mod_contents(&mut self, def_id: LocalDefId, m: &'tcx hir::Mod<'tcx>) { + debug!("Going through module {:?}", m); + // Keep track of if there were any private modules in the path. + let orig_inside_public_path = self.inside_public_path; + self.inside_public_path &= self.cx.tcx.local_visibility(def_id).is_public(); + + // Reimplementation of `walk_mod`: + for &i in m.item_ids { + let item = self.cx.tcx.hir().item(i); + if !matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) { + self.visit_item(item); + } + } + for &i in m.item_ids { + let item = self.cx.tcx.hir().item(i); + // To match the way import precedence works, visit glob imports last. + // Later passes in rustdoc will de-duplicate by name and kind, so if glob- + // imported items appear last, then they'll be the ones that get discarded. + if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) { + self.visit_item(item); + } + } + self.inside_public_path = orig_inside_public_path; + } +} + +// We need to implement this visitor so it'll go everywhere and retrieve items we're interested in +// such as impl blocks in const blocks. +impl<'a, 'tcx> Visitor<'tcx> for RustdocVisitor<'a, 'tcx> { + type NestedFilter = nested_filter::All; + + fn nested_visit_map(&mut self) -> Self::Map { + self.map + } + + fn visit_item(&mut self, i: &'tcx hir::Item<'tcx>) { + let parent_id = if self.modules.len() > 1 { + Some(self.modules[self.modules.len() - 2].def_id) + } else { + None + }; + if self.visit_item_inner(i, None, parent_id) { + walk_item(self, i); + } + } + + fn visit_mod(&mut self, _: &hir::Mod<'tcx>, _: Span, _: hir::HirId) { + // handled in `visit_item_inner` + } } From 3f057dd600d1db142c3bd50c7bf4ec220a35e45f Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 20 Jan 2023 16:02:39 +0100 Subject: [PATCH 02/10] Add regression test for impl blocks in const expr --- tests/rustdoc/impl-in-const-block.rs | 43 ++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 tests/rustdoc/impl-in-const-block.rs diff --git a/tests/rustdoc/impl-in-const-block.rs b/tests/rustdoc/impl-in-const-block.rs new file mode 100644 index 0000000000000..b44e713524668 --- /dev/null +++ b/tests/rustdoc/impl-in-const-block.rs @@ -0,0 +1,43 @@ +// Regression test for #83026. +// The goal of this test is to ensure that impl blocks inside +// const expressions are documented as well. + +#![crate_name = "foo"] + +// @has 'foo/struct.A.html' +// @has - '//*[@id="method.new"]/*[@class="code-header"]' 'pub fn new() -> A' +// @has - '//*[@id="method.bar"]/*[@class="code-header"]' 'pub fn bar(&self)' +// @has - '//*[@id="method.woo"]/*[@class="code-header"]' 'pub fn woo(&self)' +// @has - '//*[@id="method.yoo"]/*[@class="code-header"]' 'pub fn yoo()' +// @has - '//*[@id="method.yuu"]/*[@class="code-header"]' 'pub fn yuu()' +pub struct A; + +const _: () = { + impl A { + const FOO: () = { + impl A { + pub fn woo(&self) {} + } + }; + + pub fn new() -> A { + A + } + } +}; +pub const X: () = { + impl A { + pub fn bar(&self) {} + } +}; + +fn foo() { + impl A { + pub fn yoo() {} + } + const _: () = { + impl A { + pub fn yuu() {} + } + }; +} From 3623613dc7384c7956b19f25be94538e310b9a77 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 20 Jan 2023 16:02:46 +0100 Subject: [PATCH 03/10] Update newly failing UI tests --- .../infinite-recursive-type-impl-trait-return.rs | 4 +--- ...inite-recursive-type-impl-trait-return.stderr | 16 ++++++++++++++++ .../infinite-recursive-type-impl-trait.rs | 5 +---- .../infinite-recursive-type-impl-trait.stderr | 16 ++++++++++++++++ 4 files changed, 34 insertions(+), 7 deletions(-) create mode 100644 tests/rustdoc-ui/infinite-recursive-type-impl-trait-return.stderr create mode 100644 tests/rustdoc-ui/infinite-recursive-type-impl-trait.stderr diff --git a/tests/rustdoc-ui/infinite-recursive-type-impl-trait-return.rs b/tests/rustdoc-ui/infinite-recursive-type-impl-trait-return.rs index 4b1e04234c870..939da186fbcdb 100644 --- a/tests/rustdoc-ui/infinite-recursive-type-impl-trait-return.rs +++ b/tests/rustdoc-ui/infinite-recursive-type-impl-trait-return.rs @@ -1,12 +1,10 @@ -// check-pass // normalize-stderr-test: "`.*`" -> "`DEF_ID`" // normalize-stdout-test: "`.*`" -> "`DEF_ID`" // edition:2018 pub async fn f() -> impl std::fmt::Debug { - // rustdoc doesn't care that this is infinitely sized #[derive(Debug)] - enum E { + enum E { //~ ERROR This(E), Unit, } diff --git a/tests/rustdoc-ui/infinite-recursive-type-impl-trait-return.stderr b/tests/rustdoc-ui/infinite-recursive-type-impl-trait-return.stderr new file mode 100644 index 0000000000000..aff7402bc91c6 --- /dev/null +++ b/tests/rustdoc-ui/infinite-recursive-type-impl-trait-return.stderr @@ -0,0 +1,16 @@ +error[E0072]: recursive type `DEF_ID` has infinite size + --> $DIR/infinite-recursive-type-impl-trait-return.rs:7:5 + | +LL | enum E { + | ^^^^^^ +LL | This(E), + | - recursive without indirection + | +help: insert some indirection (e.g., a `DEF_ID`) to break the cycle + | +LL | This(Box), + | ++++ + + +error: aborting due to previous error + +For more information about this error, try `DEF_ID`. diff --git a/tests/rustdoc-ui/infinite-recursive-type-impl-trait.rs b/tests/rustdoc-ui/infinite-recursive-type-impl-trait.rs index ac79582fb3f0d..ac51725749867 100644 --- a/tests/rustdoc-ui/infinite-recursive-type-impl-trait.rs +++ b/tests/rustdoc-ui/infinite-recursive-type-impl-trait.rs @@ -1,8 +1,5 @@ -// check-pass - fn f() -> impl Sized { - // rustdoc doesn't care that this is infinitely sized - enum E { + enum E { //~ ERROR V(E), } unimplemented!() diff --git a/tests/rustdoc-ui/infinite-recursive-type-impl-trait.stderr b/tests/rustdoc-ui/infinite-recursive-type-impl-trait.stderr new file mode 100644 index 0000000000000..a61577bd14afc --- /dev/null +++ b/tests/rustdoc-ui/infinite-recursive-type-impl-trait.stderr @@ -0,0 +1,16 @@ +error[E0072]: recursive type `f::E` has infinite size + --> $DIR/infinite-recursive-type-impl-trait.rs:2:5 + | +LL | enum E { + | ^^^^^^ +LL | V(E), + | - recursive without indirection + | +help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to break the cycle + | +LL | V(Box), + | ++++ + + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0072`. From 9f70bdcbc88a51f6065dba37e033c2422dd263dc Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 20 Jan 2023 16:02:52 +0100 Subject: [PATCH 04/10] Improve code readability --- src/librustdoc/html/render/write_shared.rs | 2 +- src/librustdoc/visit_ast.rs | 36 +++++++++++++--------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/librustdoc/html/render/write_shared.rs b/src/librustdoc/html/render/write_shared.rs index bc8badad38eb0..ca3e9916487aa 100644 --- a/src/librustdoc/html/render/write_shared.rs +++ b/src/librustdoc/html/render/write_shared.rs @@ -138,7 +138,7 @@ pub(super) fn write_shared( Ok((ret, krates)) } - /// Read a file and return all lines that match the "{crate}":{data},\ format, + /// Read a file and return all lines that match the "{crate}":{data},\ format, /// and return a tuple `(Vec, Vec)`. /// /// This forms the payload of files that look like this: diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index 2fbcda35e4417..952304d40eccf 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -192,6 +192,16 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { ret } + #[inline] + fn add_to_current_mod( + &mut self, + item: &'tcx hir::Item<'_>, + renamed: Option, + parent_id: Option, + ) { + self.modules.last_mut().unwrap().items.push((item, renamed, parent_id)) + } + fn visit_item_inner( &mut self, item: &'tcx hir::Item<'_>, @@ -253,7 +263,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { } } - self.modules.last_mut().unwrap().items.push((item, renamed, parent_id)); + self.add_to_current_mod(item, renamed, parent_id); } } hir::ItemKind::Macro(ref macro_def, _) => { @@ -273,7 +283,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { let nonexported = !tcx.has_attr(def_id, sym::macro_export); if is_macro_2_0 || nonexported || self.inlining { - self.modules.last_mut().unwrap().items.push((item, renamed, None)); + self.add_to_current_mod(item, renamed, None); } } hir::ItemKind::Mod(ref m) => { @@ -289,20 +299,20 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { | hir::ItemKind::Static(..) | hir::ItemKind::Trait(..) | hir::ItemKind::TraitAlias(..) => { - self.modules.last_mut().unwrap().items.push((item, renamed, parent_id)) + self.add_to_current_mod(item, renamed, parent_id); } hir::ItemKind::Const(..) => { // Underscore constants do not correspond to a nameable item and // so are never useful in documentation. if name != kw::Underscore { - self.modules.last_mut().unwrap().items.push((item, renamed, parent_id)); + self.add_to_current_mod(item, renamed, parent_id); } } hir::ItemKind::Impl(impl_) => { // Don't duplicate impls when inlining or if it's implementing a trait, we'll pick // them up regardless of where they're located. if !self.inlining && impl_.of_trait.is_none() { - self.modules.last_mut().unwrap().items.push((item, None, None)); + self.add_to_current_mod(item, None, None); } } } @@ -339,15 +349,13 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { // macro in the same module. let mut inserted = FxHashSet::default(); for export in self.cx.tcx.module_reexports(CRATE_DEF_ID).unwrap_or(&[]) { - if let Res::Def(DefKind::Macro(_), def_id) = export.res { - if let Some(local_def_id) = def_id.as_local() { - if self.cx.tcx.has_attr(def_id, sym::macro_export) { - if inserted.insert(def_id) { - let item = self.cx.tcx.hir().expect_item(local_def_id); - top_level_module.items.push((item, None, None)); - } - } - } + if let Res::Def(DefKind::Macro(_), def_id) = export.res && + let Some(local_def_id) = def_id.as_local() && + self.cx.tcx.has_attr(def_id, sym::macro_export) && + inserted.insert(def_id) + { + let item = self.cx.tcx.hir().expect_item(local_def_id); + top_level_module.items.push((item, None, None)); } } From 9b80a6ddf887dcbb2a8932741c6d711b7c475d9c Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 20 Jan 2023 16:02:57 +0100 Subject: [PATCH 05/10] Speed up execution a bit by removing some walks --- src/librustdoc/visit_ast.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index 952304d40eccf..eccde86ff2469 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -451,6 +451,26 @@ impl<'a, 'tcx> Visitor<'tcx> for RustdocVisitor<'a, 'tcx> { } fn visit_mod(&mut self, _: &hir::Mod<'tcx>, _: Span, _: hir::HirId) { - // handled in `visit_item_inner` + // Handled in `visit_item_inner` + } + + fn visit_use(&mut self, _: &hir::UsePath<'tcx>, _: hir::HirId) { + // Handled in `visit_item_inner` + } + + fn visit_path(&mut self, _: &hir::Path<'tcx>, _: hir::HirId) { + // Handled in `visit_item_inner` + } + + fn visit_label(&mut self, _: &rustc_ast::Label) { + // Unneeded. + } + + fn visit_infer(&mut self, _: &hir::InferArg) { + // Unneeded. + } + + fn visit_lifetime(&mut self, _: &hir::Lifetime) { + // Unneeded. } } From 34685485c7c3dc001dd25ff1ba1db76694173a21 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 20 Jan 2023 16:03:04 +0100 Subject: [PATCH 06/10] Improve code --- src/librustdoc/visit_ast.rs | 183 ++++++++++++++++++------------------ 1 file changed, 90 insertions(+), 93 deletions(-) diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index eccde86ff2469..c95ca6727b52f 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -7,7 +7,6 @@ use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::{DefId, DefIdMap, LocalDefId, LocalDefIdSet}; use rustc_hir::intravisit::{walk_item, Visitor}; use rustc_hir::{Node, CRATE_HIR_ID}; -use rustc_middle::hir::map::Map; use rustc_middle::hir::nested_filter; use rustc_middle::ty::{DefIdTree, TyCtxt}; use rustc_span::def_id::{CRATE_DEF_ID, LOCAL_CRATE}; @@ -74,7 +73,6 @@ pub(crate) struct RustdocVisitor<'a, 'tcx> { inside_public_path: bool, exact_paths: DefIdMap>, modules: Vec>, - map: Map<'tcx>, } impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { @@ -87,7 +85,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { CRATE_DEF_ID, cx.tcx.hir().root_module().spans.inner_span, ); - let map = cx.tcx.hir(); RustdocVisitor { cx, @@ -96,7 +93,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { inside_public_path: true, exact_paths: Default::default(), modules: vec![om], - map, } } @@ -105,6 +101,94 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { self.exact_paths.entry(did).or_insert_with(|| def_id_to_path(tcx, did)); } + pub(crate) fn visit(mut self) -> Module<'tcx> { + let root_module = self.cx.tcx.hir().root_module(); + self.visit_mod_contents(CRATE_DEF_ID, root_module); + + let mut top_level_module = self.modules.pop().unwrap(); + + // `#[macro_export] macro_rules!` items are reexported at the top level of the + // crate, regardless of where they're defined. We want to document the + // top level rexport of the macro, not its original definition, since + // the rexport defines the path that a user will actually see. Accordingly, + // we add the rexport as an item here, and then skip over the original + // definition in `visit_item()` below. + // + // We also skip `#[macro_export] macro_rules!` that have already been inserted, + // it can happen if within the same module a `#[macro_export] macro_rules!` + // is declared but also a reexport of itself producing two exports of the same + // macro in the same module. + let mut inserted = FxHashSet::default(); + for export in self.cx.tcx.module_reexports(CRATE_DEF_ID).unwrap_or(&[]) { + if let Res::Def(DefKind::Macro(_), def_id) = export.res && + let Some(local_def_id) = def_id.as_local() && + self.cx.tcx.has_attr(def_id, sym::macro_export) && + inserted.insert(def_id) + { + let item = self.cx.tcx.hir().expect_item(local_def_id); + top_level_module.items.push((item, None, None)); + } + } + + self.cx.cache.hidden_cfg = self + .cx + .tcx + .hir() + .attrs(CRATE_HIR_ID) + .iter() + .filter(|attr| attr.has_name(sym::doc)) + .flat_map(|attr| attr.meta_item_list().into_iter().flatten()) + .filter(|attr| attr.has_name(sym::cfg_hide)) + .flat_map(|attr| { + attr.meta_item_list() + .unwrap_or(&[]) + .iter() + .filter_map(|attr| { + Cfg::parse(attr.meta_item()?) + .map_err(|e| self.cx.sess().diagnostic().span_err(e.span, e.msg)) + .ok() + }) + .collect::>() + }) + .chain( + [Cfg::Cfg(sym::test, None), Cfg::Cfg(sym::doc, None), Cfg::Cfg(sym::doctest, None)] + .into_iter(), + ) + .collect(); + + self.cx.cache.exact_paths = self.exact_paths; + top_level_module + } + + /// This method will go through the given module items in two passes: + /// 1. The items which are not glob imports/reexports. + /// 2. The glob imports/reexports. + fn visit_mod_contents(&mut self, def_id: LocalDefId, m: &'tcx hir::Mod<'tcx>) { + debug!("Going through module {:?}", m); + // Keep track of if there were any private modules in the path. + let orig_inside_public_path = self.inside_public_path; + self.inside_public_path &= self.cx.tcx.local_visibility(def_id).is_public(); + + // Reimplementation of `walk_mod` because we need to do it in two passes (explanations in + // the second loop): + for &i in m.item_ids { + let item = self.cx.tcx.hir().item(i); + if !matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) { + self.visit_item(item); + } + } + for &i in m.item_ids { + let item = self.cx.tcx.hir().item(i); + // To match the way import precedence works, visit glob imports last. + // Later passes in rustdoc will de-duplicate by name and kind, so if glob- + // imported items appear last, then they'll be the ones that get discarded. + if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) { + self.visit_item(item); + } + } + self.inside_public_path = orig_inside_public_path; + } + /// Tries to resolve the target of a `pub use` statement and inlines the /// target if it is defined locally and would not be documented otherwise, /// or when it is specifically requested with `please_inline`. @@ -197,7 +281,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { &mut self, item: &'tcx hir::Item<'_>, renamed: Option, - parent_id: Option, + parent_id: Option, ) { self.modules.last_mut().unwrap().items.push((item, renamed, parent_id)) } @@ -330,65 +414,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { } } - pub(crate) fn visit(mut self) -> Module<'tcx> { - let root_module = self.cx.tcx.hir().root_module(); - self.visit_mod_contents(CRATE_DEF_ID, root_module); - - let mut top_level_module = self.modules.pop().unwrap(); - - // `#[macro_export] macro_rules!` items are reexported at the top level of the - // crate, regardless of where they're defined. We want to document the - // top level rexport of the macro, not its original definition, since - // the rexport defines the path that a user will actually see. Accordingly, - // we add the rexport as an item here, and then skip over the original - // definition in `visit_item()` below. - // - // We also skip `#[macro_export] macro_rules!` that have already been inserted, - // it can happen if within the same module a `#[macro_export] macro_rules!` - // is declared but also a reexport of itself producing two exports of the same - // macro in the same module. - let mut inserted = FxHashSet::default(); - for export in self.cx.tcx.module_reexports(CRATE_DEF_ID).unwrap_or(&[]) { - if let Res::Def(DefKind::Macro(_), def_id) = export.res && - let Some(local_def_id) = def_id.as_local() && - self.cx.tcx.has_attr(def_id, sym::macro_export) && - inserted.insert(def_id) - { - let item = self.cx.tcx.hir().expect_item(local_def_id); - top_level_module.items.push((item, None, None)); - } - } - - self.cx.cache.hidden_cfg = self - .cx - .tcx - .hir() - .attrs(CRATE_HIR_ID) - .iter() - .filter(|attr| attr.has_name(sym::doc)) - .flat_map(|attr| attr.meta_item_list().into_iter().flatten()) - .filter(|attr| attr.has_name(sym::cfg_hide)) - .flat_map(|attr| { - attr.meta_item_list() - .unwrap_or(&[]) - .iter() - .filter_map(|attr| { - Cfg::parse(attr.meta_item()?) - .map_err(|e| self.cx.sess().diagnostic().span_err(e.span, e.msg)) - .ok() - }) - .collect::>() - }) - .chain( - [Cfg::Cfg(sym::test, None), Cfg::Cfg(sym::doc, None), Cfg::Cfg(sym::doctest, None)] - .into_iter(), - ) - .collect(); - - self.cx.cache.exact_paths = self.exact_paths; - top_level_module - } - /// 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. @@ -400,34 +425,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { let last = self.modules.pop().unwrap(); self.modules.last_mut().unwrap().mods.push(last); } - - /// This method will go through the given module items in two passes: - /// 1. The items which are not glob imports/reexports. - /// 2. The glob imports/reexports. - fn visit_mod_contents(&mut self, def_id: LocalDefId, m: &'tcx hir::Mod<'tcx>) { - debug!("Going through module {:?}", m); - // Keep track of if there were any private modules in the path. - let orig_inside_public_path = self.inside_public_path; - self.inside_public_path &= self.cx.tcx.local_visibility(def_id).is_public(); - - // Reimplementation of `walk_mod`: - for &i in m.item_ids { - let item = self.cx.tcx.hir().item(i); - if !matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) { - self.visit_item(item); - } - } - for &i in m.item_ids { - let item = self.cx.tcx.hir().item(i); - // To match the way import precedence works, visit glob imports last. - // Later passes in rustdoc will de-duplicate by name and kind, so if glob- - // imported items appear last, then they'll be the ones that get discarded. - if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) { - self.visit_item(item); - } - } - self.inside_public_path = orig_inside_public_path; - } } // We need to implement this visitor so it'll go everywhere and retrieve items we're interested in @@ -436,7 +433,7 @@ impl<'a, 'tcx> Visitor<'tcx> for RustdocVisitor<'a, 'tcx> { type NestedFilter = nested_filter::All; fn nested_visit_map(&mut self) -> Self::Map { - self.map + self.cx.tcx.hir() } fn visit_item(&mut self, i: &'tcx hir::Item<'tcx>) { From 5b654a7e5eaa377bb75fee8dd20b8daba6d408dd Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 17 Jan 2023 20:40:25 +0100 Subject: [PATCH 07/10] Fix handling of items inside a `doc(hidden)` block --- src/librustdoc/clean/mod.rs | 16 +++---- src/librustdoc/passes/strip_hidden.rs | 68 +++++++++++++++++++++------ src/librustdoc/visit_ast.rs | 16 +++---- 3 files changed, 66 insertions(+), 34 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 3cb6ad10e72b8..ee9d0e829f0f2 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -2225,21 +2225,17 @@ fn clean_maybe_renamed_item<'tcx>( get_all_import_attributes(use_node, cx.tcx, item.owner_id.def_id, &mut extra_attrs); } - if !extra_attrs.is_empty() { + let mut item = if !extra_attrs.is_empty() { extra_attrs.extend_from_slice(inline::load_attrs(cx, def_id)); let attrs = Attributes::from_ast(&extra_attrs); let cfg = extra_attrs.cfg(cx.tcx, &cx.cache.hidden_cfg); - vec![Item::from_def_id_and_attrs_and_parts( - def_id, - Some(name), - kind, - Box::new(attrs), - cfg, - )] + Item::from_def_id_and_attrs_and_parts(def_id, Some(name), kind, Box::new(attrs), cfg) } else { - vec![Item::from_def_id_and_parts(def_id, Some(name), kind, cx)] - } + Item::from_def_id_and_parts(def_id, Some(name), kind, cx) + }; + item.inline_stmt_id = import_id.map(|def_id| def_id.to_def_id()); + vec![item] }) } diff --git a/src/librustdoc/passes/strip_hidden.rs b/src/librustdoc/passes/strip_hidden.rs index e07a788a72a41..cfd2171395ceb 100644 --- a/src/librustdoc/passes/strip_hidden.rs +++ b/src/librustdoc/passes/strip_hidden.rs @@ -1,4 +1,6 @@ //! Strip all doc(hidden) items from the output. + +use rustc_middle::ty::TyCtxt; use rustc_span::symbol::sym; use std::mem; @@ -7,6 +9,7 @@ use crate::clean::{Item, ItemIdSet, NestedAttributesExt}; use crate::core::DocContext; use crate::fold::{strip_item, DocFolder}; use crate::passes::{ImplStripper, Pass}; +use crate::visit_ast::inherits_doc_hidden; pub(crate) const STRIP_HIDDEN: Pass = Pass { name: "strip-hidden", @@ -21,7 +24,12 @@ pub(crate) fn strip_hidden(krate: clean::Crate, cx: &mut DocContext<'_>) -> clea // strip all #[doc(hidden)] items let krate = { - let mut stripper = Stripper { retained: &mut retained, update_retained: true }; + let mut stripper = Stripper { + retained: &mut retained, + update_retained: true, + tcx: cx.tcx, + is_in_hidden_item: false, + }; stripper.fold_crate(krate) }; @@ -36,14 +44,38 @@ pub(crate) fn strip_hidden(krate: clean::Crate, cx: &mut DocContext<'_>) -> clea stripper.fold_crate(krate) } -struct Stripper<'a> { +struct Stripper<'a, 'tcx> { retained: &'a mut ItemIdSet, update_retained: bool, + tcx: TyCtxt<'tcx>, + is_in_hidden_item: bool, +} + +impl<'a, 'tcx> Stripper<'a, 'tcx> { + 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); + self.is_in_hidden_item = prev; + ret + } } -impl<'a> DocFolder for Stripper<'a> { +impl<'a, 'tcx> DocFolder for Stripper<'a, 'tcx> { fn fold_item(&mut self, i: Item) -> Option { - if i.attrs.lists(sym::doc).has_word(sym::hidden) { + let has_doc_hidden = i.attrs.lists(sym::doc).has_word(sym::hidden); + let mut 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. + 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)) + .unwrap_or(false); + } + if is_hidden { debug!("strip_hidden: stripping {:?} {:?}", i.type_(), i.name); // Use a dedicated hidden item for fields, variants, and modules. // We need to keep private fields and variants, so that the docs @@ -53,23 +85,31 @@ impl<'a> DocFolder for Stripper<'a> { // module it's defined in. Both of these are marked "stripped," and // not included in the final docs, but since they still have an effect // on the final doc, cannot be completely removed from the Clean IR. - match *i.kind { + return match *i.kind { clean::StructFieldItem(..) | clean::ModuleItem(..) | clean::VariantItem(..) => { // We need to recurse into stripped modules to // strip things like impl methods but when doing so // we must not add any items to the `retained` set. let old = mem::replace(&mut self.update_retained, false); - let ret = strip_item(self.fold_item_recur(i)); + let ret = strip_item(self.set_is_in_hidden_item_and_fold(true, i)); self.update_retained = old; - return Some(ret); + Some(ret) + } + _ => { + let ret = self.set_is_in_hidden_item_and_fold(true, i); + if has_doc_hidden { + // If the item itself has `#[doc(hidden)]`, then we simply remove it. + None + } else { + // However if it's a "descendant" of a `#[doc(hidden)]` item, then we strip it. + Some(strip_item(ret)) + } } - _ => return None, - } - } else { - if self.update_retained { - self.retained.insert(i.item_id); - } + }; + } + if self.update_retained { + self.retained.insert(i.item_id); } - Some(self.fold_item_recur(i)) + Some(self.set_is_in_hidden_item_and_fold(is_hidden, i)) } } diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index c95ca6727b52f..2d2afb83f9dd8 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -187,6 +187,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { } } self.inside_public_path = orig_inside_public_path; + debug!("Leaving module {:?}", m); } /// Tries to resolve the target of a `pub use` statement and inlines the @@ -290,7 +291,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { &mut self, item: &'tcx hir::Item<'_>, renamed: Option, - parent_id: Option, + import_id: Option, ) -> bool { debug!("visiting item {:?}", item); let name = renamed.unwrap_or(item.ident.name); @@ -347,7 +348,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { } } - self.add_to_current_mod(item, renamed, parent_id); + self.add_to_current_mod(item, renamed, import_id); } } hir::ItemKind::Macro(ref macro_def, _) => { @@ -383,13 +384,13 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { | hir::ItemKind::Static(..) | hir::ItemKind::Trait(..) | hir::ItemKind::TraitAlias(..) => { - self.add_to_current_mod(item, renamed, parent_id); + self.add_to_current_mod(item, renamed, import_id); } hir::ItemKind::Const(..) => { // Underscore constants do not correspond to a nameable item and // so are never useful in documentation. if name != kw::Underscore { - self.add_to_current_mod(item, renamed, parent_id); + self.add_to_current_mod(item, renamed, import_id); } } hir::ItemKind::Impl(impl_) => { @@ -437,12 +438,7 @@ impl<'a, 'tcx> Visitor<'tcx> for RustdocVisitor<'a, 'tcx> { } fn visit_item(&mut self, i: &'tcx hir::Item<'tcx>) { - let parent_id = if self.modules.len() > 1 { - Some(self.modules[self.modules.len() - 2].def_id) - } else { - None - }; - if self.visit_item_inner(i, None, parent_id) { + if self.visit_item_inner(i, None, None) { walk_item(self, i); } } From c918efa6642c4ca6611068d09e7341731fd00a80 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 17 Jan 2023 20:40:47 +0100 Subject: [PATCH 08/10] Update rustdoc/redirect test --- tests/rustdoc/redirect.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/rustdoc/redirect.rs b/tests/rustdoc/redirect.rs index e3a14c7a74a00..5b7a76e1a7739 100644 --- a/tests/rustdoc/redirect.rs +++ b/tests/rustdoc/redirect.rs @@ -9,9 +9,10 @@ pub trait Foo {} // @has redirect/index.html // @has - '//code' 'pub use reexp_stripped::Bar' // @has - '//code/a' 'Bar' +// @has - '//a[@href="../reexp_stripped/hidden/struct.Bar.html"]' 'Bar' // @has reexp_stripped/hidden/struct.Bar.html -// @has - '//p/a' '../../reexp_stripped/struct.Bar.html' // @has 'reexp_stripped/struct.Bar.html' +// @has - '//a[@href="struct.Bar.html"]' 'Bar' #[doc(no_inline)] pub use reexp_stripped::Bar; impl Foo for Bar {} From 4b3eef873407ab49918f941810e69c47c79a2b3c Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 17 Jan 2023 20:41:16 +0100 Subject: [PATCH 09/10] Add rustdoc test to ensure that items into a `doc(hidden)` block are handled as expected --- tests/rustdoc/hidden-private.rs | 50 +++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 tests/rustdoc/hidden-private.rs diff --git a/tests/rustdoc/hidden-private.rs b/tests/rustdoc/hidden-private.rs new file mode 100644 index 0000000000000..834ba5231a1e1 --- /dev/null +++ b/tests/rustdoc/hidden-private.rs @@ -0,0 +1,50 @@ +// This is a regression test for . +// It ensures that the items in the `doc(hidden)` const block don't show up in the +// generated docs. + +// compile-flags: --document-private-items + +#![crate_name = "foo"] + +// @has 'foo/index.html' +// @count - '//*[@class="item-table"]//a[@class="struct"]' 2 +// @count - '//*[@class="item-table"]//a[@class="trait"]' 1 +// @count - '//*[@class="item-table"]//a[@class="macro"]' 0 +#[doc(hidden)] +const _: () = { + macro_rules! stry { + () => {}; + } + + struct ShouldBeHidden; + + // @has 'foo/struct.Foo.html' + // @!has - '//*[@class="code-header"]' 'impl Bar for Foo' + #[doc(hidden)] + impl Bar for Foo { + fn bar(&self) { + struct SHouldAlsoBeHidden; + } + } + + // @has 'foo/struct.Private.html' + // @has - '//*[@id="impl-Bar-for-Private"]/*[@class="code-header"]' 'impl Bar for Private' + // @has - '//*[@id="method.bar"]/*[@class="code-header"]' 'fn bar(&self)' + impl Bar for Private { + fn bar(&self) {} + } + + // @has - '//*[@id="impl-Private"]/*[@class="code-header"]' 'impl Private' + // @has - '//*[@id="method.tralala"]/*[@class="code-header"]' 'fn tralala()' + impl Private { + fn tralala() {} + } +}; + + +struct Private; +pub struct Foo; + +pub trait Bar { + fn bar(&self); +} From ea844187b27fbcff521bcbcbe6615d51d0196fa2 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 27 Jan 2023 20:32:50 +0100 Subject: [PATCH 10/10] Special-case handling of impl blocks --- src/librustdoc/passes/strip_hidden.rs | 105 +++++++++++++++----------- src/librustdoc/visit_ast.rs | 18 ++++- 2 files changed, 75 insertions(+), 48 deletions(-) diff --git a/src/librustdoc/passes/strip_hidden.rs b/src/librustdoc/passes/strip_hidden.rs index cfd2171395ceb..8c733ddefc0a5 100644 --- a/src/librustdoc/passes/strip_hidden.rs +++ b/src/librustdoc/passes/strip_hidden.rs @@ -59,57 +59,74 @@ impl<'a, 'tcx> Stripper<'a, 'tcx> { self.is_in_hidden_item = prev; ret } + + /// In case `i` is a non-hidden impl block, then we special-case it by changing the value + /// of `is_in_hidden_item` to `true` because the impl children inherit its visibility. + fn recurse_in_impl(&mut self, i: Item) -> Item { + let prev = mem::replace(&mut self.is_in_hidden_item, false); + let ret = self.fold_item_recur(i); + self.is_in_hidden_item = prev; + ret + } } impl<'a, 'tcx> DocFolder for Stripper<'a, 'tcx> { fn fold_item(&mut self, i: Item) -> Option { let has_doc_hidden = i.attrs.lists(sym::doc).has_word(sym::hidden); - let mut 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. - 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)) - .unwrap_or(false); + let is_impl = matches!(*i.kind, clean::ImplItem(..)); + let mut is_hidden = has_doc_hidden; + if !is_impl { + 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. + 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)) + .unwrap_or(false); + } } - if is_hidden { - debug!("strip_hidden: stripping {:?} {:?}", i.type_(), i.name); - // Use a dedicated hidden item for fields, variants, and modules. - // We need to keep private fields and variants, so that the docs - // can show a placeholder "// some variants omitted". We need to keep - // private modules, because they can contain impl blocks, and impl - // block privacy is inherited from the type and trait, not from the - // module it's defined in. Both of these are marked "stripped," and - // not included in the final docs, but since they still have an effect - // on the final doc, cannot be completely removed from the Clean IR. - return match *i.kind { - clean::StructFieldItem(..) | clean::ModuleItem(..) | clean::VariantItem(..) => { - // We need to recurse into stripped modules to - // strip things like impl methods but when doing so - // we must not add any items to the `retained` set. - let old = mem::replace(&mut self.update_retained, false); - let ret = strip_item(self.set_is_in_hidden_item_and_fold(true, i)); - self.update_retained = old; - Some(ret) - } - _ => { - let ret = self.set_is_in_hidden_item_and_fold(true, i); - if has_doc_hidden { - // If the item itself has `#[doc(hidden)]`, then we simply remove it. - None - } else { - // However if it's a "descendant" of a `#[doc(hidden)]` item, then we strip it. - Some(strip_item(ret)) - } - } - }; + if !is_hidden { + if self.update_retained { + self.retained.insert(i.item_id); + } + return Some(if is_impl { + self.recurse_in_impl(i) + } else { + self.set_is_in_hidden_item_and_fold(false, i) + }); } - if self.update_retained { - self.retained.insert(i.item_id); + debug!("strip_hidden: stripping {:?} {:?}", i.type_(), i.name); + // Use a dedicated hidden item for fields, variants, and modules. + // We need to keep private fields and variants, so that the docs + // can show a placeholder "// some variants omitted". We need to keep + // private modules, because they can contain impl blocks, and impl + // block privacy is inherited from the type and trait, not from the + // module it's defined in. Both of these are marked "stripped," and + // not included in the final docs, but since they still have an effect + // on the final doc, cannot be completely removed from the Clean IR. + match *i.kind { + clean::StructFieldItem(..) | clean::ModuleItem(..) | clean::VariantItem(..) => { + // We need to recurse into stripped modules to + // strip things like impl methods but when doing so + // we must not add any items to the `retained` set. + let old = mem::replace(&mut self.update_retained, false); + let ret = strip_item(self.set_is_in_hidden_item_and_fold(true, i)); + self.update_retained = old; + Some(ret) + } + _ => { + let ret = self.set_is_in_hidden_item_and_fold(true, i); + if has_doc_hidden { + // If the item itself has `#[doc(hidden)]`, then we simply remove it. + None + } else { + // However if it's a "descendant" of a `#[doc(hidden)]` item, then we strip it. + Some(strip_item(ret)) + } + } } - Some(self.set_is_in_hidden_item_and_fold(is_hidden, i)) } } diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index 2d2afb83f9dd8..088cb3f339492 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -55,11 +55,21 @@ 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 node: LocalDefId) -> bool { - while let Some(id) = tcx.opt_local_parent(node) { - node = id; - if tcx.is_doc_hidden(node.to_def_id()) { +pub(crate) fn inherits_doc_hidden(tcx: TyCtxt<'_>, mut def_id: LocalDefId) -> bool { + let hir = tcx.hir(); + while let Some(id) = tcx.opt_local_parent(def_id) { + def_id = id; + if tcx.is_doc_hidden(def_id.to_def_id()) { return true; + } else if let Some(node) = hir.find_by_def_id(def_id) && + matches!( + node, + hir::Node::Item(hir::Item { kind: hir::ItemKind::Impl(_), .. }), + ) + { + // `impl` blocks stand a bit on their own: unless they have `#[doc(hidden)]` directly + // on them, they don't inherit it from the parent context. + return false; } } false