From c70b7aafae283b69762ee809d6b2b5adaf0f3c43 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 21 Jan 2023 23:49:23 +0400 Subject: [PATCH] rustc_metadata: Fix `encode_attrs` This function didn't do what the authors intended it to do. - Due to `move` in the closure `is_public` wasn't captured by mutalbe reference and wasn't used as a cache. - Due to iterator cloning all the `should_encode_attr` logic run for the second time to calculate `may_have_doc_links` This PR fixes these issues, and calculates all the needed attribute flags in one go. --- compiler/rustc_metadata/src/rmeta/encoder.rs | 77 ++++++++++++-------- 1 file changed, 48 insertions(+), 29 deletions(-) diff --git a/compiler/rustc_metadata/src/rmeta/encoder.rs b/compiler/rustc_metadata/src/rmeta/encoder.rs index 2ecaa33d4d315..e6430d327879f 100644 --- a/compiler/rustc_metadata/src/rmeta/encoder.rs +++ b/compiler/rustc_metadata/src/rmeta/encoder.rs @@ -3,6 +3,7 @@ use crate::rmeta::def_path_hash_map::DefPathHashMapRef; use crate::rmeta::table::TableBuilder; use crate::rmeta::*; +use rustc_ast::util::comments; use rustc_ast::Attribute; use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::fx::{FxHashMap, FxIndexSet}; @@ -760,36 +761,54 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> { } } +struct AnalyzeAttrState { + is_exported: bool, + may_have_doc_links: bool, + is_doc_hidden: bool, +} + /// Returns whether an attribute needs to be recorded in metadata, that is, if it's usable and /// useful in downstream crates. Local-only attributes are an obvious example, but some /// rustdoc-specific attributes can equally be of use while documenting the current crate only. /// /// Removing these superfluous attributes speeds up compilation by making the metadata smaller. /// -/// Note: the `is_def_id_public` parameter is used to cache whether the given `DefId` has a public +/// Note: the `is_exported` parameter is used to cache whether the given `DefId` has a public /// visibility: this is a piece of data that can be computed once per defid, and not once per /// attribute. Some attributes would only be usable downstream if they are public. #[inline] -fn should_encode_attr( - tcx: TyCtxt<'_>, - attr: &Attribute, - def_id: LocalDefId, - is_def_id_public: &mut Option, -) -> bool { +fn analyze_attr(attr: &Attribute, state: &mut AnalyzeAttrState) -> bool { + let mut should_encode = false; if rustc_feature::is_builtin_only_local(attr.name_or_empty()) { // Attributes marked local-only don't need to be encoded for downstream crates. - false - } else if attr.doc_str().is_some() { - // We keep all public doc comments because they might be "imported" into downstream crates - // if they use `#[doc(inline)]` to copy an item's documentation into their own. - *is_def_id_public.get_or_insert_with(|| tcx.effective_visibilities(()).is_exported(def_id)) + } else if let Some(s) = attr.doc_str() { + // We keep all doc comments reachable to rustdoc because they might be "imported" into + // downstream crates if they use `#[doc(inline)]` to copy an item's documentation into + // their own. + if state.is_exported { + should_encode = true; + if comments::may_have_doc_links(s.as_str()) { + state.may_have_doc_links = true; + } + } } else if attr.has_name(sym::doc) { - // If this is a `doc` attribute, and it's marked `inline` (as in `#[doc(inline)]`), we can - // remove it. It won't be inlinable in downstream crates. - attr.meta_item_list().map(|l| l.iter().any(|l| !l.has_name(sym::inline))).unwrap_or(false) + // If this is a `doc` attribute that doesn't have anything except maybe `inline` (as in + // `#[doc(inline)]`), then we can remove it. It won't be inlinable in downstream crates. + if let Some(item_list) = attr.meta_item_list() { + for item in item_list { + if !item.has_name(sym::inline) { + should_encode = true; + if item.has_name(sym::hidden) { + state.is_doc_hidden = true; + break; + } + } + } + } } else { - true + should_encode = true; } + should_encode } fn should_encode_visibility(def_kind: DefKind) -> bool { @@ -1109,24 +1128,24 @@ fn should_encode_trait_impl_trait_tys(tcx: TyCtxt<'_>, def_id: DefId) -> bool { impl<'a, 'tcx> EncodeContext<'a, 'tcx> { fn encode_attrs(&mut self, def_id: LocalDefId) { let tcx = self.tcx; - let mut is_public: Option = None; - - let hir_attrs = tcx.hir().attrs(tcx.hir().local_def_id_to_hir_id(def_id)); - let mut attrs = hir_attrs + let mut state = AnalyzeAttrState { + is_exported: tcx.effective_visibilities(()).is_exported(def_id), + may_have_doc_links: false, + is_doc_hidden: false, + }; + let attr_iter = tcx + .hir() + .attrs(tcx.hir().local_def_id_to_hir_id(def_id)) .iter() - .filter(move |attr| should_encode_attr(tcx, attr, def_id, &mut is_public)); + .filter(|attr| analyze_attr(attr, &mut state)); + + record_array!(self.tables.attributes[def_id.to_def_id()] <- attr_iter); - record_array!(self.tables.attributes[def_id.to_def_id()] <- attrs.clone()); let mut attr_flags = AttrFlags::empty(); - if attrs.any(|attr| attr.may_have_doc_links()) { + if state.may_have_doc_links { attr_flags |= AttrFlags::MAY_HAVE_DOC_LINKS; } - if hir_attrs - .iter() - .filter(|attr| attr.has_name(sym::doc)) - .filter_map(|attr| attr.meta_item_list()) - .any(|items| items.iter().any(|item| item.has_name(sym::hidden))) - { + if state.is_doc_hidden { attr_flags |= AttrFlags::IS_DOC_HIDDEN; } if !attr_flags.is_empty() {