From 5f93edd4b8b03a4b16e0efa6868166a6bc7ed645 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 14 Feb 2023 18:27:59 +0100 Subject: [PATCH 1/4] Prevent some attributes from being merged with others on reexports --- src/librustdoc/clean/mod.rs | 97 ++++++++++++++++++++++++++++++++++--- 1 file changed, 89 insertions(+), 8 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index bf3bbeb2dd133..bdb559af037c1 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -2126,6 +2126,87 @@ fn get_all_import_attributes<'hir>( } } +/// When inlining items, we merge its attributes (and all the reexports attributes too) with the +/// final reexport. For example: +/// +/// ``` +/// #[doc(hidden, cfg(feature = "foo"))] +/// pub struct Foo; +/// +/// #[doc(cfg(feature = "bar"))] +/// #[doc(hidden, no_inline)] +/// pub use Foo as Foo1; +/// +/// #[doc(inline)] +/// pub use Foo2 as Bar; +/// ``` +/// +/// So `Bar` at the end will have both `cfg(feature = "...")`. However, we don't want to merge all +/// attributes so we filter out the following ones: +/// * `doc(inline)` +/// * `doc(no_inline)` +/// * `doc(hidden)` +fn add_without_unwanted_attributes(attrs: &mut Vec, new_attrs: &[ast::Attribute]) { + use rustc_ast::token::{Token, TokenKind}; + use rustc_ast::tokenstream::{TokenStream, TokenTree}; + + for attr in new_attrs { + let mut attr = attr.clone(); + match attr.kind { + ast::AttrKind::Normal(ref mut normal) => { + if let [ident] = &*normal.item.path.segments { + let ident = ident.ident.name; + if ident == sym::doc { + match normal.item.args { + ast::AttrArgs::Delimited(ref mut args) => { + let mut tokens = Vec::with_capacity(args.tokens.len()); + let mut skip_next_comma = false; + for token in args.tokens.clone().into_trees() { + match token { + TokenTree::Token( + Token { + kind: + TokenKind::Ident( + sym::hidden | sym::inline | sym::no_inline, + _, + ), + .. + }, + _, + ) => { + skip_next_comma = true; + continue; + } + TokenTree::Token( + Token { kind: TokenKind::Comma, .. }, + _, + ) if skip_next_comma => { + skip_next_comma = false; + continue; + } + _ => {} + } + skip_next_comma = false; + tokens.push(token); + } + args.tokens = TokenStream::new(tokens); + attrs.push(attr); + } + ast::AttrArgs::Empty | ast::AttrArgs::Eq(..) => { + attrs.push(attr); + continue; + } + } + } + } + } + ast::AttrKind::DocComment(..) => { + attrs.push(attr); + } + } + } +} + fn clean_maybe_renamed_item<'tcx>( cx: &mut DocContext<'tcx>, item: &hir::Item<'tcx>, @@ -2216,17 +2297,17 @@ fn clean_maybe_renamed_item<'tcx>( extra_attrs.extend_from_slice(inline::load_attrs(cx, import_id.to_def_id())); // Then we get all the various imports' attributes. get_all_import_attributes(use_node, cx.tcx, item.owner_id.def_id, &mut extra_attrs); + add_without_unwanted_attributes(&mut extra_attrs, inline::load_attrs(cx, def_id)); + } else { + // We only keep the item's attributes. + extra_attrs.extend_from_slice(inline::load_attrs(cx, def_id)); } - 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); + let attrs = Attributes::from_ast(&extra_attrs); + let cfg = extra_attrs.cfg(cx.tcx, &cx.cache.hidden_cfg); - Item::from_def_id_and_attrs_and_parts(def_id, Some(name), kind, Box::new(attrs), cfg) - } else { - Item::from_def_id_and_parts(def_id, Some(name), kind, cx) - }; + 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(|def_id| def_id.to_def_id()); vec![item] }) From 02a845a8268e6c6ed45f8bf2e7748ec8208af650 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 14 Feb 2023 18:29:56 +0100 Subject: [PATCH 2/4] Correctly handle reexport traversal by fixing multiple bugs, especially for items with a path of 1 element --- src/librustdoc/clean/mod.rs | 144 +++++++++++++++++++++--------------- 1 file changed, 85 insertions(+), 59 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index bdb559af037c1..a46e6a2ca6c15 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -11,6 +11,8 @@ pub(crate) mod types; pub(crate) mod utils; use rustc_ast as ast; +use rustc_ast::token::{Token, TokenKind}; +use rustc_ast::tokenstream::{TokenStream, TokenTree}; use rustc_attr as attr; use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap, FxIndexSet, IndexEntry}; use rustc_hir as hir; @@ -2081,8 +2083,8 @@ impl<'hir> hir::intravisit::Visitor<'hir> for OneLevelVisitor<'hir> { fn visit_item(&mut self, item: &'hir hir::Item<'hir>) { if self.item.is_none() && item.ident == self.looking_for - && matches!(item.kind, hir::ItemKind::Use(_, _)) - || item.owner_id.def_id == self.target_def_id + && (matches!(item.kind, hir::ItemKind::Use(_, _)) + || item.owner_id.def_id == self.target_def_id) { self.item = Some(item); } @@ -2103,33 +2105,74 @@ fn get_all_import_attributes<'hir>( let mut visitor = OneLevelVisitor::new(hir_map, target_def_id); let mut visited = FxHashSet::default(); // If the item is an import and has at least a path with two parts, we go into it. - while let hir::ItemKind::Use(path, _) = item.kind && - path.segments.len() > 1 && - let hir::def::Res::Def(_, def_id) = path.segments[path.segments.len() - 2].res && - visited.insert(def_id) - { - if let Some(hir::Node::Item(parent_item)) = hir_map.get_if_local(def_id) { - // We add the attributes from this import into the list. - attributes.extend_from_slice(hir_map.attrs(item.hir_id())); - // We get the `Ident` we will be looking for into `item`. - let looking_for = path.segments[path.segments.len() - 1].ident; - visitor.reset(looking_for); - hir::intravisit::walk_item(&mut visitor, parent_item); - if let Some(i) = visitor.item { - item = i; - } else { - break; + while let hir::ItemKind::Use(path, _) = item.kind && visited.insert(item.hir_id()) { + // We add the attributes from this import into the list. + add_without_unwanted_attributes(attributes, hir_map.attrs(item.hir_id())); + + let def_id = if path.segments.len() > 1 { + match path.segments[path.segments.len() - 2].res { + hir::def::Res::Def(_, def_id) => def_id, + _ => break, + } + } else { + // If the path doesn't have a parent, then the parent is the current module. + tcx.parent(item.owner_id.def_id.to_def_id()) + }; + + let Some(parent) = hir_map.get_if_local(def_id) else { break }; + + // We get the `Ident` we will be looking for into `item`. + let looking_for = path.segments[path.segments.len() - 1].ident; + visitor.reset(looking_for); + + match parent { + hir::Node::Item(parent_item) => { + hir::intravisit::walk_item(&mut visitor, parent_item); + } + hir::Node::Crate(m) => { + hir::intravisit::walk_mod( + &mut visitor, + m, + tcx.local_def_id_to_hir_id(def_id.as_local().unwrap()), + ); } + _ => break, + } + if let Some(i) = visitor.item { + item = i; } else { break; } } } +fn filter_tokens_from_list( + args_tokens: TokenStream, + should_retain: impl Fn(&TokenTree) -> bool, +) -> Vec { + let mut tokens = Vec::with_capacity(args_tokens.len()); + let mut skip_next_comma = false; + for token in args_tokens.into_trees() { + match token { + TokenTree::Token(Token { kind: TokenKind::Comma, .. }, _) if skip_next_comma => { + skip_next_comma = false; + } + token if should_retain(&token) => { + skip_next_comma = false; + tokens.push(token); + } + _ => { + skip_next_comma = true; + } + } + } + tokens +} + /// When inlining items, we merge its attributes (and all the reexports attributes too) with the /// final reexport. For example: /// -/// ``` +/// ```ignore (just an example) /// #[doc(hidden, cfg(feature = "foo"))] /// pub struct Foo; /// @@ -2147,55 +2190,38 @@ fn get_all_import_attributes<'hir>( /// * `doc(no_inline)` /// * `doc(hidden)` fn add_without_unwanted_attributes(attrs: &mut Vec, new_attrs: &[ast::Attribute]) { - use rustc_ast::token::{Token, TokenKind}; - use rustc_ast::tokenstream::{TokenStream, TokenTree}; - for attr in new_attrs { let mut attr = attr.clone(); match attr.kind { ast::AttrKind::Normal(ref mut normal) => { - if let [ident] = &*normal.item.path.segments { - let ident = ident.ident.name; - if ident == sym::doc { - match normal.item.args { - ast::AttrArgs::Delimited(ref mut args) => { - let mut tokens = Vec::with_capacity(args.tokens.len()); - let mut skip_next_comma = false; - for token in args.tokens.clone().into_trees() { - match token { + if let [ident] = &*normal.item.path.segments && + let ident = ident.ident.name && + ident == sym::doc + { + match normal.item.args { + ast::AttrArgs::Delimited(ref mut args) => { + let tokens = + filter_tokens_from_list(args.tokens.clone(), |token| { + !matches!( + token, TokenTree::Token( Token { - kind: - TokenKind::Ident( - sym::hidden | sym::inline | sym::no_inline, - _, - ), + kind: TokenKind::Ident( + sym::hidden | sym::inline | sym::no_inline, + _, + ), .. }, _, - ) => { - skip_next_comma = true; - continue; - } - TokenTree::Token( - Token { kind: TokenKind::Comma, .. }, - _, - ) if skip_next_comma => { - skip_next_comma = false; - continue; - } - _ => {} - } - skip_next_comma = false; - tokens.push(token); - } - args.tokens = TokenStream::new(tokens); - attrs.push(attr); - } - ast::AttrArgs::Empty | ast::AttrArgs::Eq(..) => { - attrs.push(attr); - continue; - } + ), + ) + }); + args.tokens = TokenStream::new(tokens); + attrs.push(attr); + } + ast::AttrArgs::Empty | ast::AttrArgs::Eq(..) => { + attrs.push(attr); + continue; } } } From 1ec1d9481253ad828a648a2ca9a2556b6efd69cc Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 14 Feb 2023 18:30:21 +0100 Subject: [PATCH 3/4] Add test for reexports attr merge --- tests/rustdoc/reexport-attr-merge.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 tests/rustdoc/reexport-attr-merge.rs diff --git a/tests/rustdoc/reexport-attr-merge.rs b/tests/rustdoc/reexport-attr-merge.rs new file mode 100644 index 0000000000000..f1aee08c9c06b --- /dev/null +++ b/tests/rustdoc/reexport-attr-merge.rs @@ -0,0 +1,26 @@ +// Regression test for . +// The goal is to ensure that `doc(hidden)`, `doc(inline)` and `doc(no_inline`) + +#![crate_name = "foo"] +#![feature(doc_cfg)] + +// @has 'foo/index.html' + +#[doc(hidden, cfg(feature = "foo"))] +pub struct Foo; + +#[doc(hidden, no_inline, cfg(feature = "bar"))] +pub use Foo as Foo1; + +#[doc(hidden, inline)] +pub use Foo1 as Foo2; + +// First we ensure that none of the other items are generated. +// @count - '//a[@class="struct"]' 1 +// Then we check that both `cfg` are displayed. +// @has - '//*[@class="stab portability"]' 'foo' +// @has - '//*[@class="stab portability"]' 'bar' +// And finally we check that the only element displayed is `Bar`. +// @has - '//a[@class="struct"]' 'Bar' +#[doc(inline)] +pub use Foo2 as Bar; From 374f798ad2f10280f75a3561f2dc9449ccb5e5fe Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 15 Feb 2023 00:00:51 +0100 Subject: [PATCH 4/4] Correctly handle reexports of `#[doc(hidden)]` is reexport does not use `#[doc(inline)]` --- src/librustdoc/clean/mod.rs | 19 +++++++++++++++---- tests/rustdoc/reexport-attr-merge.rs | 13 ++++++++++--- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index a46e6a2ca6c15..80f05863d0ec1 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -2100,6 +2100,7 @@ fn get_all_import_attributes<'hir>( tcx: TyCtxt<'hir>, target_def_id: LocalDefId, attributes: &mut Vec, + is_inline: bool, ) { let hir_map = tcx.hir(); let mut visitor = OneLevelVisitor::new(hir_map, target_def_id); @@ -2107,7 +2108,7 @@ fn get_all_import_attributes<'hir>( // If the item is an import and has at least a path with two parts, we go into it. while let hir::ItemKind::Use(path, _) = item.kind && visited.insert(item.hir_id()) { // We add the attributes from this import into the list. - add_without_unwanted_attributes(attributes, hir_map.attrs(item.hir_id())); + add_without_unwanted_attributes(attributes, hir_map.attrs(item.hir_id()), is_inline); let def_id = if path.segments.len() > 1 { match path.segments[path.segments.len() - 2].res { @@ -2189,7 +2190,16 @@ fn filter_tokens_from_list( /// * `doc(inline)` /// * `doc(no_inline)` /// * `doc(hidden)` -fn add_without_unwanted_attributes(attrs: &mut Vec, new_attrs: &[ast::Attribute]) { +fn add_without_unwanted_attributes( + attrs: &mut Vec, + new_attrs: &[ast::Attribute], + is_inline: bool, +) { + // If it's `#[doc(inline)]`, we don't want all attributes, otherwise we keep everything. + if !is_inline { + attrs.extend_from_slice(new_attrs); + return; + } for attr in new_attrs { let mut attr = attr.clone(); match attr.kind { @@ -2321,9 +2331,10 @@ fn clean_maybe_renamed_item<'tcx>( { // First, we add the attributes from the current import. extra_attrs.extend_from_slice(inline::load_attrs(cx, import_id.to_def_id())); + let is_inline = extra_attrs.lists(sym::doc).get_word_attr(sym::inline).is_some(); // Then we get all the various imports' attributes. - get_all_import_attributes(use_node, cx.tcx, item.owner_id.def_id, &mut extra_attrs); - add_without_unwanted_attributes(&mut extra_attrs, inline::load_attrs(cx, def_id)); + get_all_import_attributes(use_node, cx.tcx, item.owner_id.def_id, &mut extra_attrs, is_inline); + add_without_unwanted_attributes(&mut extra_attrs, inline::load_attrs(cx, def_id), is_inline); } else { // We only keep the item's attributes. extra_attrs.extend_from_slice(inline::load_attrs(cx, def_id)); diff --git a/tests/rustdoc/reexport-attr-merge.rs b/tests/rustdoc/reexport-attr-merge.rs index f1aee08c9c06b..f6c23a1365f46 100644 --- a/tests/rustdoc/reexport-attr-merge.rs +++ b/tests/rustdoc/reexport-attr-merge.rs @@ -1,5 +1,6 @@ // Regression test for . -// The goal is to ensure that `doc(hidden)`, `doc(inline)` and `doc(no_inline`) +// The goal is to ensure that `doc(hidden)`, `doc(inline)` and `doc(no_inline)` +// are not copied from an item when inlined. #![crate_name = "foo"] #![feature(doc_cfg)] @@ -15,8 +16,9 @@ pub use Foo as Foo1; #[doc(hidden, inline)] pub use Foo1 as Foo2; -// First we ensure that none of the other items are generated. -// @count - '//a[@class="struct"]' 1 +// First we ensure that only the reexport `Bar2` and the inlined struct `Bar` +// are inlined. +// @count - '//a[@class="struct"]' 2 // Then we check that both `cfg` are displayed. // @has - '//*[@class="stab portability"]' 'foo' // @has - '//*[@class="stab portability"]' 'bar' @@ -24,3 +26,8 @@ pub use Foo1 as Foo2; // @has - '//a[@class="struct"]' 'Bar' #[doc(inline)] pub use Foo2 as Bar; + +// This one should appear but `Bar2` won't be linked because there is no +// `#[doc(inline)]`. +// @has - '//*[@id="reexport.Bar2"]' 'pub use Foo2 as Bar2;' +pub use Foo2 as Bar2;