From b1745c3919b57d841c15fe0bfb0dd926ab59bafa Mon Sep 17 00:00:00 2001 From: Lukas Markeffsky <@> Date: Wed, 25 Sep 2024 22:15:23 +0200 Subject: [PATCH 1/3] de-rc external traits Don't keep the `external_traits` as shared mutable data between the `DocContext` and `clean::Crate`. Instead, move the data over when necessary. This allows us to get rid of a borrowck hack in the `DocVisitor`. --- src/librustdoc/clean/inline.rs | 5 ++--- src/librustdoc/clean/types.rs | 4 +--- src/librustdoc/clean/utils.rs | 2 +- src/librustdoc/core.rs | 6 ++---- src/librustdoc/fold.rs | 11 +++++++---- src/librustdoc/formats/cache.rs | 3 ++- src/librustdoc/passes/collect_trait_impls.rs | 2 ++ src/librustdoc/visit.rs | 7 ++----- 8 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/librustdoc/clean/inline.rs b/src/librustdoc/clean/inline.rs index b7d6b3dda72b4..d3c4ef4dc906c 100644 --- a/src/librustdoc/clean/inline.rs +++ b/src/librustdoc/clean/inline.rs @@ -837,8 +837,7 @@ pub(crate) fn record_extern_trait(cx: &mut DocContext<'_>, did: DefId) { } { - if cx.external_traits.borrow().contains_key(&did) || cx.active_extern_traits.contains(&did) - { + if cx.external_traits.contains_key(&did) || cx.active_extern_traits.contains(&did) { return; } } @@ -850,6 +849,6 @@ pub(crate) fn record_extern_trait(cx: &mut DocContext<'_>, did: DefId) { debug!("record_extern_trait: {did:?}"); let trait_ = build_external_trait(cx, did); - cx.external_traits.borrow_mut().insert(did, trait_); + cx.external_traits.insert(did, trait_); cx.active_extern_traits.remove(&did); } diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index bc6b5a7d3e319..69a414ae989ac 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -1,8 +1,6 @@ use std::borrow::Cow; -use std::cell::RefCell; use std::hash::Hash; use std::path::PathBuf; -use std::rc::Rc; use std::sync::{Arc, OnceLock as OnceCell}; use std::{fmt, iter}; @@ -115,7 +113,7 @@ impl From for ItemId { pub(crate) struct Crate { pub(crate) module: Item, /// Only here so that they can be filtered through the rustdoc passes. - pub(crate) external_traits: Rc>>, + pub(crate) external_traits: Box>, } impl Crate { diff --git a/src/librustdoc/clean/utils.rs b/src/librustdoc/clean/utils.rs index b91c1725d0c81..fdf628b50fbe4 100644 --- a/src/librustdoc/clean/utils.rs +++ b/src/librustdoc/clean/utils.rs @@ -74,7 +74,7 @@ pub(crate) fn krate(cx: &mut DocContext<'_>) -> Crate { })); } - Crate { module, external_traits: cx.external_traits.clone() } + Crate { module, external_traits: Box::new(mem::take(&mut cx.external_traits)) } } pub(crate) fn clean_middle_generic_args<'tcx>( diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index 8e46d93c28e02..4c48c075a944d 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -1,5 +1,3 @@ -use std::cell::RefCell; -use std::rc::Rc; use std::sync::atomic::AtomicBool; use std::sync::{Arc, LazyLock}; use std::{io, mem}; @@ -41,7 +39,7 @@ pub(crate) struct DocContext<'tcx> { /// Most of this logic is copied from rustc_lint::late. pub(crate) param_env: ParamEnv<'tcx>, /// Later on moved through `clean::Crate` into `cache` - pub(crate) external_traits: Rc>>, + pub(crate) external_traits: FxHashMap, /// Used while populating `external_traits` to ensure we don't process the same trait twice at /// the same time. pub(crate) active_extern_traits: DefIdSet, @@ -359,7 +357,7 @@ pub(crate) fn run_global_ctxt( // Note that in case of `#![no_core]`, the trait is not available. if let Some(sized_trait_did) = ctxt.tcx.lang_items().sized_trait() { let sized_trait = build_external_trait(&mut ctxt, sized_trait_did); - ctxt.external_traits.borrow_mut().insert(sized_trait_did, sized_trait); + ctxt.external_traits.insert(sized_trait_did, sized_trait); } debug!("crate: {:?}", tcx.hir().krate()); diff --git a/src/librustdoc/fold.rs b/src/librustdoc/fold.rs index c25a4ddb6f369..95e495205aea6 100644 --- a/src/librustdoc/fold.rs +++ b/src/librustdoc/fold.rs @@ -1,3 +1,5 @@ +use std::mem; + use crate::clean::*; pub(crate) fn strip_item(mut item: Item) -> Item { @@ -116,10 +118,11 @@ pub(crate) trait DocFolder: Sized { fn fold_crate(&mut self, mut c: Crate) -> Crate { c.module = self.fold_item(c.module).unwrap(); - let external_traits = { std::mem::take(&mut *c.external_traits.borrow_mut()) }; - for (k, mut v) in external_traits { - v.items = v.items.into_iter().filter_map(|i| self.fold_item(i)).collect(); - c.external_traits.borrow_mut().insert(k, v); + for trait_ in c.external_traits.values_mut() { + trait_.items = mem::take(&mut trait_.items) + .into_iter() + .filter_map(|i| self.fold_item(i)) + .collect(); } c diff --git a/src/librustdoc/formats/cache.rs b/src/librustdoc/formats/cache.rs index 8597d2e2b63eb..db1a0bd0af938 100644 --- a/src/librustdoc/formats/cache.rs +++ b/src/librustdoc/formats/cache.rs @@ -153,7 +153,8 @@ impl Cache { // Crawl the crate to build various caches used for the output debug!(?cx.cache.crate_version); - cx.cache.traits = krate.external_traits.take(); + assert!(cx.external_traits.is_empty()); + cx.cache.traits = mem::take(&mut krate.external_traits); // Cache where all our extern crates are located // FIXME: this part is specific to HTML so it'd be nice to remove it from the common code diff --git a/src/librustdoc/passes/collect_trait_impls.rs b/src/librustdoc/passes/collect_trait_impls.rs index a9edc485d5e7a..949ba548387c9 100644 --- a/src/librustdoc/passes/collect_trait_impls.rs +++ b/src/librustdoc/passes/collect_trait_impls.rs @@ -219,6 +219,8 @@ pub(crate) fn collect_trait_impls(mut krate: Crate, cx: &mut DocContext<'_>) -> panic!("collect-trait-impls can't run"); }; + krate.external_traits.extend(cx.external_traits.drain()); + krate } diff --git a/src/librustdoc/visit.rs b/src/librustdoc/visit.rs index 7fb0a32cc9495..f1de0fe9a3459 100644 --- a/src/librustdoc/visit.rs +++ b/src/librustdoc/visit.rs @@ -61,11 +61,8 @@ pub(crate) trait DocVisitor: Sized { fn visit_crate(&mut self, c: &Crate) { self.visit_item(&c.module); - // FIXME: make this a simple by-ref for loop once external_traits is cleaned up - let external_traits = { std::mem::take(&mut *c.external_traits.borrow_mut()) }; - for (k, v) in external_traits { - v.items.iter().for_each(|i| self.visit_item(i)); - c.external_traits.borrow_mut().insert(k, v); + for trait_ in c.external_traits.values() { + trait_.items.iter().for_each(|i| self.visit_item(i)); } } } From 4facc1ce41f2f98fe75129da8c2f2251e8090e26 Mon Sep 17 00:00:00 2001 From: Lukas Markeffsky <@> Date: Wed, 25 Sep 2024 22:28:28 +0200 Subject: [PATCH 2/3] rm higher-ranked lifetimes from `DocVisitor` This allows the visitor to borrow from the visitees. --- src/librustdoc/html/render/write_shared.rs | 2 +- src/librustdoc/html/sources.rs | 4 ++-- src/librustdoc/passes/calculate_doc_coverage.rs | 2 +- src/librustdoc/passes/check_doc_test_visibility.rs | 2 +- src/librustdoc/passes/collect_intra_doc_links.rs | 2 +- src/librustdoc/passes/collect_trait_impls.rs | 4 ++-- src/librustdoc/passes/lint.rs | 2 +- src/librustdoc/visit.rs | 12 ++++++------ 8 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/librustdoc/html/render/write_shared.rs b/src/librustdoc/html/render/write_shared.rs index 12246b0d416a8..7db70eb9dd80f 100644 --- a/src/librustdoc/html/render/write_shared.rs +++ b/src/librustdoc/html/render/write_shared.rs @@ -865,7 +865,7 @@ struct AliasedTypeImpl<'cache> { type_aliases: Vec<(&'cache [Symbol], Item)>, } -impl<'cx, 'cache> DocVisitor for TypeImplCollector<'cx, 'cache> { +impl<'cx, 'cache> DocVisitor<'_> for TypeImplCollector<'cx, 'cache> { fn visit_item(&mut self, it: &Item) { self.visit_item_recur(it); let cache = self.cache; diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index 9ba0fb7b83c95..7be9cc0b8858e 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -103,7 +103,7 @@ impl LocalSourcesCollector<'_, '_> { } } -impl DocVisitor for LocalSourcesCollector<'_, '_> { +impl DocVisitor<'_> for LocalSourcesCollector<'_, '_> { fn visit_item(&mut self, item: &clean::Item) { self.add_local_source(item); @@ -122,7 +122,7 @@ struct SourceCollector<'a, 'tcx> { crate_name: &'a str, } -impl DocVisitor for SourceCollector<'_, '_> { +impl DocVisitor<'_> for SourceCollector<'_, '_> { fn visit_item(&mut self, item: &clean::Item) { if !self.cx.include_sources { return; diff --git a/src/librustdoc/passes/calculate_doc_coverage.rs b/src/librustdoc/passes/calculate_doc_coverage.rs index 15c0d77e7ba8b..abea5bcbc51ea 100644 --- a/src/librustdoc/passes/calculate_doc_coverage.rs +++ b/src/librustdoc/passes/calculate_doc_coverage.rs @@ -187,7 +187,7 @@ impl<'a, 'b> CoverageCalculator<'a, 'b> { } } -impl<'a, 'b> DocVisitor for CoverageCalculator<'a, 'b> { +impl<'a, 'b> DocVisitor<'_> for CoverageCalculator<'a, 'b> { fn visit_item(&mut self, i: &clean::Item) { if !i.item_id.is_local() { // non-local items are skipped because they can be out of the users control, diff --git a/src/librustdoc/passes/check_doc_test_visibility.rs b/src/librustdoc/passes/check_doc_test_visibility.rs index 52b2cd7774da4..1dc9af7ebe591 100644 --- a/src/librustdoc/passes/check_doc_test_visibility.rs +++ b/src/librustdoc/passes/check_doc_test_visibility.rs @@ -34,7 +34,7 @@ pub(crate) fn check_doc_test_visibility(krate: Crate, cx: &mut DocContext<'_>) - krate } -impl<'a, 'tcx> DocVisitor for DocTestVisibilityLinter<'a, 'tcx> { +impl<'a, 'tcx> DocVisitor<'_> for DocTestVisibilityLinter<'a, 'tcx> { fn visit_item(&mut self, item: &Item) { look_for_tests(self.cx, &item.doc_value(), item); diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 29342dcac592b..e95e8762b1db5 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -816,7 +816,7 @@ fn is_derive_trait_collision(ns: &PerNS, ResolutionFailu } } -impl<'a, 'tcx> DocVisitor for LinkCollector<'a, 'tcx> { +impl<'a, 'tcx> DocVisitor<'_> for LinkCollector<'a, 'tcx> { fn visit_item(&mut self, item: &Item) { self.resolve_links(item); self.visit_item_recur(item) diff --git a/src/librustdoc/passes/collect_trait_impls.rs b/src/librustdoc/passes/collect_trait_impls.rs index 949ba548387c9..b307e84e42ec2 100644 --- a/src/librustdoc/passes/collect_trait_impls.rs +++ b/src/librustdoc/passes/collect_trait_impls.rs @@ -229,7 +229,7 @@ struct SyntheticImplCollector<'a, 'tcx> { impls: Vec, } -impl<'a, 'tcx> DocVisitor for SyntheticImplCollector<'a, 'tcx> { +impl<'a, 'tcx> DocVisitor<'_> for SyntheticImplCollector<'a, 'tcx> { fn visit_item(&mut self, i: &Item) { if i.is_struct() || i.is_enum() || i.is_union() { // FIXME(eddyb) is this `doc(hidden)` check needed? @@ -256,7 +256,7 @@ impl<'cache> ItemAndAliasCollector<'cache> { } } -impl<'cache> DocVisitor for ItemAndAliasCollector<'cache> { +impl<'cache> DocVisitor<'_> for ItemAndAliasCollector<'cache> { fn visit_item(&mut self, i: &Item) { self.items.insert(i.item_id); diff --git a/src/librustdoc/passes/lint.rs b/src/librustdoc/passes/lint.rs index 4da5d8f0e0663..593027ef7d293 100644 --- a/src/librustdoc/passes/lint.rs +++ b/src/librustdoc/passes/lint.rs @@ -25,7 +25,7 @@ pub(crate) fn run_lints(krate: Crate, cx: &mut DocContext<'_>) -> Crate { krate } -impl<'a, 'tcx> DocVisitor for Linter<'a, 'tcx> { +impl<'a, 'tcx> DocVisitor<'_> for Linter<'a, 'tcx> { fn visit_item(&mut self, item: &Item) { let Some(hir_id) = DocContext::as_local_hir_id(self.cx.tcx, item.item_id) else { // If non-local, no need to check anything. diff --git a/src/librustdoc/visit.rs b/src/librustdoc/visit.rs index f1de0fe9a3459..fbc18176ed850 100644 --- a/src/librustdoc/visit.rs +++ b/src/librustdoc/visit.rs @@ -1,12 +1,12 @@ use crate::clean::*; -pub(crate) trait DocVisitor: Sized { - fn visit_item(&mut self, item: &Item) { +pub(crate) trait DocVisitor<'a>: Sized { + fn visit_item(&mut self, item: &'a Item) { self.visit_item_recur(item) } /// don't override! - fn visit_inner_recur(&mut self, kind: &ItemKind) { + fn visit_inner_recur(&mut self, kind: &'a ItemKind) { match kind { StrippedItem(..) => unreachable!(), ModuleItem(i) => { @@ -47,18 +47,18 @@ pub(crate) trait DocVisitor: Sized { } /// don't override! - fn visit_item_recur(&mut self, item: &Item) { + fn visit_item_recur(&mut self, item: &'a Item) { match &item.kind { StrippedItem(i) => self.visit_inner_recur(&*i), _ => self.visit_inner_recur(&item.kind), } } - fn visit_mod(&mut self, m: &Module) { + fn visit_mod(&mut self, m: &'a Module) { m.items.iter().for_each(|i| self.visit_item(i)) } - fn visit_crate(&mut self, c: &Crate) { + fn visit_crate(&mut self, c: &'a Crate) { self.visit_item(&c.module); for trait_ in c.external_traits.values() { From 345077af98bb0efc796536d82e4232aab10671f3 Mon Sep 17 00:00:00 2001 From: Lukas Markeffsky <@> Date: Wed, 25 Sep 2024 22:41:32 +0200 Subject: [PATCH 3/3] don't clone `clean::Item` in `TypeImplCollector` --- src/librustdoc/html/render/write_shared.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/librustdoc/html/render/write_shared.rs b/src/librustdoc/html/render/write_shared.rs index 7db70eb9dd80f..af94b1042c05c 100644 --- a/src/librustdoc/html/render/write_shared.rs +++ b/src/librustdoc/html/render/write_shared.rs @@ -824,9 +824,9 @@ impl Serialize for Implementor { /// this visitor works to reverse that: `aliased_types` is a map /// from target to the aliases that reference it, and each one /// will generate one file. -struct TypeImplCollector<'cx, 'cache> { +struct TypeImplCollector<'cx, 'cache, 'item> { /// Map from DefId-of-aliased-type to its data. - aliased_types: IndexMap>, + aliased_types: IndexMap>, visited_aliases: FxHashSet, cache: &'cache Cache, cx: &'cache mut Context<'cx>, @@ -847,26 +847,26 @@ struct TypeImplCollector<'cx, 'cache> { /// ] /// ) /// ``` -struct AliasedType<'cache> { +struct AliasedType<'cache, 'item> { /// This is used to generate the actual filename of this aliased type. target_fqp: &'cache [Symbol], target_type: ItemType, /// This is the data stored inside the file. /// ItemId is used to deduplicate impls. - impl_: IndexMap>, + impl_: IndexMap>, } /// The `impl_` contains data that's used to figure out if an alias will work, /// and to generate the HTML at the end. /// /// The `type_aliases` list is built up with each type alias that matches. -struct AliasedTypeImpl<'cache> { +struct AliasedTypeImpl<'cache, 'item> { impl_: &'cache Impl, - type_aliases: Vec<(&'cache [Symbol], Item)>, + type_aliases: Vec<(&'cache [Symbol], &'item Item)>, } -impl<'cx, 'cache> DocVisitor<'_> for TypeImplCollector<'cx, 'cache> { - fn visit_item(&mut self, it: &Item) { +impl<'cx, 'cache, 'item> DocVisitor<'item> for TypeImplCollector<'cx, 'cache, 'item> { + fn visit_item(&mut self, it: &'item Item) { self.visit_item_recur(it); let cache = self.cache; let ItemKind::TypeAliasItem(ref t) = it.kind else { return }; @@ -927,7 +927,7 @@ impl<'cx, 'cache> DocVisitor<'_> for TypeImplCollector<'cx, 'cache> { continue; } // This impl was not found in the set of rejected impls - aliased_type_impl.type_aliases.push((&self_fqp[..], it.clone())); + aliased_type_impl.type_aliases.push((&self_fqp[..], it)); } } }