Skip to content

rustdoc: Remove AttributesExt trait magic that added needless complexity #135428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 15, 2025
Merged

rustdoc: Remove AttributesExt trait magic that added needless complexity #135428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e2fcdb8
rustdoc: Extract `AttributesExt::cfg` trait method as function
camelid Jan 13, 2025
5ccd6c0
rustdoc: Extract `AttributesExt::lists` trait method as function
camelid Jan 13, 2025
f92b32c
rustdoc: Eliminate `AttributesExt`
camelid Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/librustdoc/clean/inline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ use rustc_span::symbol::{Symbol, sym};
use thin_vec::{ThinVec, thin_vec};
use tracing::{debug, trace};

use super::Item;
use super::{Item, extract_cfg_from_attrs};
use crate::clean::{
self, Attributes, AttributesExt, ImplKind, ItemId, Type, clean_bound_vars, clean_generics,
clean_impl_item, clean_middle_assoc_item, clean_middle_field, clean_middle_ty,
clean_poly_fn_sig, clean_trait_ref_with_constraints, clean_ty, clean_ty_alias_inner_type,
clean_ty_generics, clean_variant_def, utils,
self, Attributes, ImplKind, ItemId, Type, clean_bound_vars, clean_generics, clean_impl_item,
clean_middle_assoc_item, clean_middle_field, clean_middle_ty, clean_poly_fn_sig,
clean_trait_ref_with_constraints, clean_ty, clean_ty_alias_inner_type, clean_ty_generics,
clean_variant_def, utils,
};
use crate::core::DocContext;
use crate::formats::item_type::ItemType;
Expand Down Expand Up @@ -408,10 +408,13 @@ pub(crate) fn merge_attrs(
} else {
Attributes::from_hir(&both)
},
both.cfg(cx.tcx, &cx.cache.hidden_cfg),
extract_cfg_from_attrs(both.iter(), cx.tcx, &cx.cache.hidden_cfg),
)
} else {
(Attributes::from_hir(old_attrs), old_attrs.cfg(cx.tcx, &cx.cache.hidden_cfg))
(
Attributes::from_hir(old_attrs),
extract_cfg_from_attrs(old_attrs.iter(), cx.tcx, &cx.cache.hidden_cfg),
)
}
}

Expand Down
20 changes: 13 additions & 7 deletions src/librustdoc/clean/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,7 @@ fn generate_item_with_correct_attrs(
// For glob re-exports the item may or may not exist to be re-exported (potentially the cfgs
// on the path up until the glob can be removed, and only cfgs on the globbed item itself
// matter), for non-inlined re-exports see #85043.
let is_inline = inline::load_attrs(cx, import_id.to_def_id())
.lists(sym::doc)
let is_inline = hir_attr_lists(inline::load_attrs(cx, import_id.to_def_id()), sym::doc)
.get_word_attr(sym::inline)
.is_some()
|| (is_glob_import(cx.tcx, import_id)
Expand All @@ -199,8 +198,14 @@ fn generate_item_with_correct_attrs(
// 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 cfg = extract_cfg_from_attrs(
attrs.iter().map(move |(attr, _)| match attr {
Cow::Borrowed(attr) => *attr,
Cow::Owned(attr) => attr,
}),
cx.tcx,
&cx.cache.hidden_cfg,
);
let attrs = Attributes::from_hir_iter(attrs.iter().map(|(attr, did)| (&**attr, *did)), false);

let name = renamed.or(Some(name));
Expand Down Expand Up @@ -979,13 +984,14 @@ fn clean_proc_macro<'tcx>(
) -> ItemKind {
let attrs = cx.tcx.hir().attrs(item.hir_id());
if kind == MacroKind::Derive
&& let Some(derive_name) = attrs.lists(sym::proc_macro_derive).find_map(|mi| mi.ident())
&& let Some(derive_name) =
hir_attr_lists(attrs, sym::proc_macro_derive).find_map(|mi| mi.ident())
{
*name = derive_name.name;
}

let mut helpers = Vec::new();
for mi in attrs.lists(sym::proc_macro_derive) {
for mi in hir_attr_lists(attrs, sym::proc_macro_derive) {
if !mi.has_name(sym::attributes) {
continue;
}
Expand Down Expand Up @@ -2985,7 +2991,7 @@ fn clean_use_statement_inner<'tcx>(

let visibility = cx.tcx.visibility(import.owner_id);
let attrs = cx.tcx.hir().attrs(import.hir_id());
let inline_attr = attrs.lists(sym::doc).get_word_attr(sym::inline);
let inline_attr = hir_attr_lists(attrs, sym::doc).get_word_attr(sym::inline);
let pub_underscore = visibility.is_public() && name == kw::Underscore;
let current_mod = cx.tcx.parent_module_from_def_id(import.owner_id.def_id);
let import_def_id = import.owner_id.def_id;
Expand Down
217 changes: 89 additions & 128 deletions src/librustdoc/clean/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use std::borrow::Cow;
use std::hash::Hash;
use std::path::PathBuf;
use std::sync::{Arc, OnceLock as OnceCell};
Expand Down Expand Up @@ -479,7 +478,7 @@ impl Item {
name,
kind,
Attributes::from_hir(hir_attrs),
hir_attrs.cfg(cx.tcx, &cx.cache.hidden_cfg),
extract_cfg_from_attrs(hir_attrs.iter(), cx.tcx, &cx.cache.hidden_cfg),
)
}

Expand Down Expand Up @@ -979,147 +978,107 @@ pub(crate) struct Module {
pub(crate) span: Span,
}

pub(crate) trait AttributesExt {
type AttributeIterator<'a>: Iterator<Item = ast::MetaItemInner>
where
Self: 'a;
type Attributes<'a>: Iterator<Item = &'a hir::Attribute>
where
Self: 'a;

fn lists(&self, name: Symbol) -> Self::AttributeIterator<'_>;

fn iter(&self) -> Self::Attributes<'_>;

fn cfg(&self, tcx: TyCtxt<'_>, hidden_cfg: &FxHashSet<Cfg>) -> Option<Arc<Cfg>> {
let sess = tcx.sess;
let doc_cfg_active = tcx.features().doc_cfg();
let doc_auto_cfg_active = tcx.features().doc_auto_cfg();

fn single<T: IntoIterator>(it: T) -> Option<T::Item> {
let mut iter = it.into_iter();
let item = iter.next()?;
if iter.next().is_some() {
return None;
}
Some(item)
pub(crate) fn hir_attr_lists<'a, I: IntoIterator<Item = &'a hir::Attribute>>(
attrs: I,
name: Symbol,
) -> impl Iterator<Item = ast::MetaItemInner> + use<'a, I> {
attrs
.into_iter()
.filter(move |attr| attr.has_name(name))
.filter_map(ast::attr::AttributeExt::meta_item_list)
.flatten()
}

pub(crate) fn extract_cfg_from_attrs<'a, I: Iterator<Item = &'a hir::Attribute> + Clone>(
attrs: I,
tcx: TyCtxt<'_>,
hidden_cfg: &FxHashSet<Cfg>,
) -> Option<Arc<Cfg>> {
let sess = tcx.sess;
let doc_cfg_active = tcx.features().doc_cfg();
let doc_auto_cfg_active = tcx.features().doc_auto_cfg();

fn single<T: IntoIterator>(it: T) -> Option<T::Item> {
let mut iter = it.into_iter();
let item = iter.next()?;
if iter.next().is_some() {
return None;
}
Some(item)
}

let mut cfg = if doc_cfg_active || doc_auto_cfg_active {
let mut doc_cfg = self
.iter()
.filter(|attr| attr.has_name(sym::doc))
.flat_map(|attr| attr.meta_item_list().unwrap_or_default())
let mut cfg = if doc_cfg_active || doc_auto_cfg_active {
let mut doc_cfg = attrs
.clone()
.filter(|attr| attr.has_name(sym::doc))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before it didn't need to clone, is there a reason it needs to now?

Copy link
Member Author

@camelid camelid Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really cloning the attributes. Before attrs was the attributes themselves, and we used the extension trait to make an iterator from them. Now, the iterator is created by the caller of this function and attrs is the iterator. So this is just cloning the iterator, which is a very cheap operation and equivalent to creating a new one using .iter() like before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, thanks for the explanation!

.flat_map(|attr| attr.meta_item_list().unwrap_or_default())
.filter(|attr| attr.has_name(sym::cfg))
.peekable();
if doc_cfg.peek().is_some() && doc_cfg_active {
doc_cfg
.filter_map(|attr| Cfg::parse(&attr).ok())
.fold(Cfg::True, |cfg, new_cfg| cfg & new_cfg)
} else if doc_auto_cfg_active {
// If there is no `doc(cfg())`, then we retrieve the `cfg()` attributes (because
// `doc(cfg())` overrides `cfg()`).
attrs
.clone()
.filter(|attr| attr.has_name(sym::cfg))
.peekable();
if doc_cfg.peek().is_some() && doc_cfg_active {
doc_cfg
.filter_map(|attr| Cfg::parse(&attr).ok())
.fold(Cfg::True, |cfg, new_cfg| cfg & new_cfg)
} else if doc_auto_cfg_active {
// If there is no `doc(cfg())`, then we retrieve the `cfg()` attributes (because
// `doc(cfg())` overrides `cfg()`).
self.iter()
.filter(|attr| attr.has_name(sym::cfg))
.filter_map(|attr| single(attr.meta_item_list()?))
.filter_map(|attr| {
Cfg::parse_without(attr.meta_item()?, hidden_cfg).ok().flatten()
})
.fold(Cfg::True, |cfg, new_cfg| cfg & new_cfg)
} else {
Cfg::True
}
.filter_map(|attr| single(attr.meta_item_list()?))
.filter_map(|attr| Cfg::parse_without(attr.meta_item()?, hidden_cfg).ok().flatten())
.fold(Cfg::True, |cfg, new_cfg| cfg & new_cfg)
} else {
Cfg::True
};

for attr in self.iter() {
// #[doc]
if attr.doc_str().is_none() && attr.has_name(sym::doc) {
// #[doc(...)]
if let Some(list) = attr.meta_item_list() {
for item in list {
// #[doc(hidden)]
if !item.has_name(sym::cfg) {
continue;
}
// #[doc(cfg(...))]
if let Some(cfg_mi) = item
.meta_item()
.and_then(|item| rustc_expand::config::parse_cfg(item, sess))
{
match Cfg::parse(cfg_mi) {
Ok(new_cfg) => cfg &= new_cfg,
Err(e) => {
sess.dcx().span_err(e.span, e.msg);
}
}
} else {
Cfg::True
};

for attr in attrs.clone() {
// #[doc]
if attr.doc_str().is_none() && attr.has_name(sym::doc) {
// #[doc(...)]
if let Some(list) = attr.meta_item_list() {
for item in list {
// #[doc(hidden)]
if !item.has_name(sym::cfg) {
continue;
}
// #[doc(cfg(...))]
if let Some(cfg_mi) = item
.meta_item()
.and_then(|item| rustc_expand::config::parse_cfg(item, sess))
{
match Cfg::parse(cfg_mi) {
Ok(new_cfg) => cfg &= new_cfg,
Err(e) => {
sess.dcx().span_err(e.span, e.msg);
}
}
}
}
}
}
}

// treat #[target_feature(enable = "feat")] attributes as if they were
// #[doc(cfg(target_feature = "feat"))] attributes as well
for attr in self.lists(sym::target_feature) {
if attr.has_name(sym::enable) {
if attr.value_str().is_some() {
// Clone `enable = "feat"`, change to `target_feature = "feat"`.
// Unwrap is safe because `value_str` succeeded above.
let mut meta = attr.meta_item().unwrap().clone();
meta.path = ast::Path::from_ident(Ident::with_dummy_span(sym::target_feature));

if let Ok(feat_cfg) = Cfg::parse(&ast::MetaItemInner::MetaItem(meta)) {
cfg &= feat_cfg;
}
// treat #[target_feature(enable = "feat")] attributes as if they were
// #[doc(cfg(target_feature = "feat"))] attributes as well
for attr in hir_attr_lists(attrs, sym::target_feature) {
if attr.has_name(sym::enable) {
if attr.value_str().is_some() {
// Clone `enable = "feat"`, change to `target_feature = "feat"`.
// Unwrap is safe because `value_str` succeeded above.
let mut meta = attr.meta_item().unwrap().clone();
meta.path = ast::Path::from_ident(Ident::with_dummy_span(sym::target_feature));

if let Ok(feat_cfg) = Cfg::parse(&ast::MetaItemInner::MetaItem(meta)) {
cfg &= feat_cfg;
}
}
}

if cfg == Cfg::True { None } else { Some(Arc::new(cfg)) }
}
}

impl AttributesExt for [hir::Attribute] {
type AttributeIterator<'a> = impl Iterator<Item = ast::MetaItemInner> + 'a;
type Attributes<'a> = impl Iterator<Item = &'a hir::Attribute> + 'a;

fn lists(&self, name: Symbol) -> Self::AttributeIterator<'_> {
self.iter()
.filter(move |attr| attr.has_name(name))
.filter_map(ast::attr::AttributeExt::meta_item_list)
.flatten()
}

fn iter(&self) -> Self::Attributes<'_> {
self.iter()
}
}

impl AttributesExt for [(Cow<'_, hir::Attribute>, Option<DefId>)] {
type AttributeIterator<'a>
= impl Iterator<Item = ast::MetaItemInner> + 'a
where
Self: 'a;
type Attributes<'a>
= impl Iterator<Item = &'a hir::Attribute> + 'a
where
Self: 'a;

fn lists(&self, name: Symbol) -> Self::AttributeIterator<'_> {
AttributesExt::iter(self)
.filter(move |attr| attr.has_name(name))
.filter_map(hir::Attribute::meta_item_list)
.flatten()
}

fn iter(&self) -> Self::Attributes<'_> {
self.iter().map(move |(attr, _)| match attr {
Cow::Borrowed(attr) => *attr,
Cow::Owned(attr) => attr,
})
}
if cfg == Cfg::True { None } else { Some(Arc::new(cfg)) }
}

pub(crate) trait NestedAttributesExt {
Expand Down Expand Up @@ -1185,7 +1144,7 @@ pub(crate) struct Attributes {

impl Attributes {
pub(crate) fn lists(&self, name: Symbol) -> impl Iterator<Item = ast::MetaItemInner> + '_ {
self.other_attrs.lists(name)
hir_attr_lists(&self.other_attrs[..], name)
}

pub(crate) fn has_doc_flag(&self, flag: Symbol) -> bool {
Expand Down Expand Up @@ -1252,7 +1211,9 @@ impl Attributes {
pub(crate) fn get_doc_aliases(&self) -> Box<[Symbol]> {
let mut aliases = FxIndexSet::default();

for attr in self.other_attrs.lists(sym::doc).filter(|a| a.has_name(sym::alias)) {
for attr in
hir_attr_lists(&self.other_attrs[..], sym::doc).filter(|a| a.has_name(sym::alias))
{
if let Some(values) = attr.meta_item_list() {
for l in values {
if let Some(lit) = l.lit()
Expand Down
7 changes: 4 additions & 3 deletions src/librustdoc/doctest/rust.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ use rustc_span::source_map::SourceMap;
use rustc_span::{BytePos, DUMMY_SP, FileName, Pos, Span};

use super::{DocTestVisitor, ScrapedDocTest};
use crate::clean::Attributes;
use crate::clean::types::AttributesExt;
use crate::clean::{Attributes, extract_cfg_from_attrs};
use crate::html::markdown::{self, ErrorCodes, LangString, MdRelLine};

struct RustCollector {
Expand Down Expand Up @@ -97,7 +96,9 @@ impl HirCollector<'_> {
nested: F,
) {
let ast_attrs = self.tcx.hir().attrs(self.tcx.local_def_id_to_hir_id(def_id));
if let Some(ref cfg) = ast_attrs.cfg(self.tcx, &FxHashSet::default()) {
if let Some(ref cfg) =
extract_cfg_from_attrs(ast_attrs.iter(), self.tcx, &FxHashSet::default())
{
if !cfg.matches(&self.tcx.sess.psess, Some(self.tcx.features())) {
return;
}
Expand Down
6 changes: 3 additions & 3 deletions src/librustdoc/visit_ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use tracing::debug;

use crate::clean::cfg::Cfg;
use crate::clean::utils::{inherits_doc_hidden, should_ignore_res};
use crate::clean::{AttributesExt, NestedAttributesExt, reexport_chain};
use crate::clean::{NestedAttributesExt, hir_attr_lists, reexport_chain};
use crate::core;

/// This module is used to store stuff from Rust's AST in a more convenient
Expand Down Expand Up @@ -247,8 +247,8 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
let document_hidden = self.cx.render_options.document_hidden;
let use_attrs = tcx.hir().attrs(tcx.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)
|| (document_hidden && use_attrs.lists(sym::doc).has_word(sym::hidden));
let is_no_inline = hir_attr_lists(use_attrs, sym::doc).has_word(sym::no_inline)
|| (document_hidden && hir_attr_lists(use_attrs, sym::doc).has_word(sym::hidden));

if is_no_inline {
return false;
Expand Down
Loading