From 537b82cfad55dda93592aad6538790ddbffa3dfe Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 19:58:28 -0700 Subject: [PATCH 01/11] rustdoc: Remove unnecessary clone in `DocFolder` Also, contrary to the comment, the clone is not that small, since `Variant` contains `Item`s, which are quite large when you factor in both stack- and heap-allocated memory. --- src/librustdoc/fold.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/librustdoc/fold.rs b/src/librustdoc/fold.rs index f84850c0fe1f1..23a9d085b3270 100644 --- a/src/librustdoc/fold.rs +++ b/src/librustdoc/fold.rs @@ -47,7 +47,6 @@ crate trait DocFolder: Sized { ImplItem(i) } VariantItem(i) => { - let i2 = i.clone(); // this clone is small match i { Variant::Struct(mut j) => { let num_fields = j.fields.len(); @@ -60,7 +59,7 @@ crate trait DocFolder: Sized { let fields = fields.into_iter().filter_map(|x| self.fold_item(x)).collect(); VariantItem(Variant::Tuple(fields)) } - _ => VariantItem(i2), + i => VariantItem(i), } } x => x, From 705dec11946f130faca91453886eba2108443339 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 20:00:48 -0700 Subject: [PATCH 02/11] rustdoc: Replace wildcard with explicit pattern This simplifies the code and future-proofs it against changes to `Variant`. --- src/librustdoc/fold.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustdoc/fold.rs b/src/librustdoc/fold.rs index 23a9d085b3270..4984d9171244e 100644 --- a/src/librustdoc/fold.rs +++ b/src/librustdoc/fold.rs @@ -59,7 +59,7 @@ crate trait DocFolder: Sized { let fields = fields.into_iter().filter_map(|x| self.fold_item(x)).collect(); VariantItem(Variant::Tuple(fields)) } - i => VariantItem(i), + Variant::CLike => VariantItem(Variant::CLike), } } x => x, From 5a9bbba2809e1a7fcf381f1e04cb81e5074e1f55 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 20:18:52 -0700 Subject: [PATCH 03/11] rustdoc: Add `DocVisitor` `DocFolder` allows transforming the docs, accomplished by making its methods take and return types by-value. However, several of the rustdoc `DocFolder` impls only *visit* the docs; they don't change anything. Passing around types by-value is thus unnecessary, confusing, and potentially inefficient for those impls. `DocVisitor` is very similar to `DocFolder`, except that its methods take shared references and return nothing (i.e., the unit type). This should both be more efficient and make the code clearer. There is an additional reason to add `DocVisitor`, too. As part of my cleanup of `external_traits`, I'm planning to add a `fn cache(&mut self) -> &mut Cache` method to `DocFolder` so that `external_traits` can be retrieved explicitly from the `Cache`, rather than implicitly via `Crate.external_traits` (which is an `Rc>`). However, some of the `DocFolder` impls that could be turned into `DocVisitor` impls only have a shared reference to the `Cache`, because they are used during rendering. (They have to access the `Cache` via `html::render::Context.shared.cache`, which involves an `Rc`.) Since `DocVisitor` does not mutate any of the types it's visiting, its equivalent `cache()` method will only need a shared reference to the `Cache`, avoiding the problem described above. --- src/librustdoc/fold.rs | 29 +++++++++++------------ src/librustdoc/lib.rs | 1 + src/librustdoc/visit.rs | 51 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 15 deletions(-) create mode 100644 src/librustdoc/visit.rs diff --git a/src/librustdoc/fold.rs b/src/librustdoc/fold.rs index 4984d9171244e..9009a9bd41b63 100644 --- a/src/librustdoc/fold.rs +++ b/src/librustdoc/fold.rs @@ -46,22 +46,21 @@ crate trait DocFolder: Sized { i.items = i.items.into_iter().filter_map(|x| self.fold_item(x)).collect(); ImplItem(i) } - VariantItem(i) => { - match i { - Variant::Struct(mut j) => { - let num_fields = j.fields.len(); - j.fields = j.fields.into_iter().filter_map(|x| self.fold_item(x)).collect(); - j.fields_stripped |= num_fields != j.fields.len() - || j.fields.iter().any(|f| f.is_stripped()); - VariantItem(Variant::Struct(j)) - } - Variant::Tuple(fields) => { - let fields = fields.into_iter().filter_map(|x| self.fold_item(x)).collect(); - VariantItem(Variant::Tuple(fields)) - } - Variant::CLike => VariantItem(Variant::CLike), + VariantItem(i) => match i { + Variant::Struct(mut j) => { + let num_fields = j.fields.len(); + j.fields = j.fields.into_iter().filter_map(|x| self.fold_item(x)).collect(); + j.fields_stripped |= + num_fields != j.fields.len() || j.fields.iter().any(|f| f.is_stripped()); + VariantItem(Variant::Struct(j)) } - } + Variant::Tuple(fields) => { + let fields = fields.into_iter().filter_map(|x| self.fold_item(x)).collect(); + VariantItem(Variant::Tuple(fields)) + } + Variant::CLike => VariantItem(Variant::CLike), + }, + // FIXME: list all cases explicitly x => x, } } diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index b50fbf58bae29..7eeb9d1fcaa55 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -121,6 +121,7 @@ mod markdown; mod passes; mod scrape_examples; mod theme; +mod visit; mod visit_ast; mod visit_lib; diff --git a/src/librustdoc/visit.rs b/src/librustdoc/visit.rs new file mode 100644 index 0000000000000..9edf6e11bec90 --- /dev/null +++ b/src/librustdoc/visit.rs @@ -0,0 +1,51 @@ +use crate::clean::*; + +crate trait DocVisitor: Sized { + fn visit_item(&mut self, item: &Item) { + self.visit_item_recur(item) + } + + /// don't override! + fn visit_inner_recur(&mut self, kind: &ItemKind) { + match kind { + StrippedItem(..) => unreachable!(), + ModuleItem(i) => { + self.visit_mod(i); + return; + } + StructItem(i) => i.fields.iter().for_each(|x| self.visit_item(x)), + UnionItem(i) => i.fields.iter().for_each(|x| self.visit_item(x)), + EnumItem(i) => i.variants.iter().for_each(|x| self.visit_item(x)), + TraitItem(i) => i.items.iter().for_each(|x| self.visit_item(x)), + ImplItem(i) => i.items.iter().for_each(|x| self.visit_item(x)), + VariantItem(i) => match i { + Variant::Struct(j) => j.fields.iter().for_each(|x| self.visit_item(x)), + Variant::Tuple(fields) => fields.iter().for_each(|x| self.visit_item(x)), + Variant::CLike => {} + }, + // FIXME: list all cases explicitly + _ => return, + } + } + + /// don't override! + fn visit_item_recur(&mut self, item: &Item) { + match &*item.kind { + StrippedItem(i) => self.visit_inner_recur(i), + _ => self.visit_inner_recur(&item.kind), + } + } + + fn visit_mod(&mut self, m: &Module) { + m.items.iter().for_each(|i| self.visit_item(i)) + } + + fn visit_crate(&mut self, c: &Crate) { + self.visit_item(&c.module); + + let external_traits = c.external_traits.borrow(); + for v in external_traits.values() { + v.trait_.items.iter().for_each(|i| self.visit_item(i)) + } + } +} From 28bdf892d6c2fc3a38d8fc40fab013d810fb3269 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 20:54:37 -0700 Subject: [PATCH 04/11] rustdoc: Use `DocVisitor` for sources collection --- src/librustdoc/html/sources.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index 9422f84f99775..74fcd22adf854 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -1,11 +1,11 @@ use crate::clean; use crate::docfs::PathError; use crate::error::Error; -use crate::fold::DocFolder; use crate::html::format::Buffer; use crate::html::highlight; use crate::html::layout; use crate::html::render::{Context, BASIC_KEYWORDS}; +use crate::visit::DocVisitor; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_hir::def_id::LOCAL_CRATE; use rustc_middle::ty::TyCtxt; @@ -18,10 +18,13 @@ use std::path::{Component, Path, PathBuf}; crate fn render(cx: &mut Context<'_>, krate: clean::Crate) -> Result { info!("emitting source files"); + let dst = cx.dst.join("src").join(&*krate.name(cx.tcx()).as_str()); cx.shared.ensure_dir(&dst)?; - let mut folder = SourceCollector { dst, cx, emitted_local_sources: FxHashSet::default() }; - Ok(folder.fold_crate(krate)) + + let mut collector = SourceCollector { dst, cx, emitted_local_sources: FxHashSet::default() }; + collector.visit_crate(&krate); + Ok(krate) } crate fn collect_local_sources<'tcx>( @@ -30,8 +33,7 @@ crate fn collect_local_sources<'tcx>( krate: clean::Crate, ) -> (clean::Crate, FxHashMap) { let mut lsc = LocalSourcesCollector { tcx, local_sources: FxHashMap::default(), src_root }; - - let krate = lsc.fold_crate(krate); + lsc.visit_crate(&krate); (krate, lsc.local_sources) } @@ -79,13 +81,13 @@ impl LocalSourcesCollector<'_, '_> { } } -impl DocFolder for LocalSourcesCollector<'_, '_> { - fn fold_item(&mut self, item: clean::Item) -> Option { - self.add_local_source(&item); +impl DocVisitor for LocalSourcesCollector<'_, '_> { + fn visit_item(&mut self, item: &clean::Item) { + self.add_local_source(item); // FIXME: if `include_sources` isn't set and DocFolder didn't require consuming the crate by value, // we could return None here without having to walk the rest of the crate. - Some(self.fold_item_recur(item)) + self.visit_item_recur(item) } } @@ -98,8 +100,8 @@ struct SourceCollector<'a, 'tcx> { emitted_local_sources: FxHashSet, } -impl DocFolder for SourceCollector<'_, '_> { - fn fold_item(&mut self, item: clean::Item) -> Option { +impl DocVisitor for SourceCollector<'_, '_> { + fn visit_item(&mut self, item: &clean::Item) { let tcx = self.cx.tcx(); let span = item.span(tcx); let sess = tcx.sess; @@ -134,7 +136,7 @@ impl DocFolder for SourceCollector<'_, '_> { } // FIXME: if `include_sources` isn't set and DocFolder didn't require consuming the crate by value, // we could return None here without having to walk the rest of the crate. - Some(self.fold_item_recur(item)) + self.visit_item_recur(item) } } From 6215f7c85fe7dae4240d9bdb35c46fcaf5b5903d Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 20:59:22 -0700 Subject: [PATCH 05/11] Clean up now that visitors only need `&clean::Crate` --- src/librustdoc/html/render/context.rs | 6 +++--- src/librustdoc/html/render/span_map.rs | 10 +++++----- src/librustdoc/html/sources.rs | 14 +++++++------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index 76e46fa0aa354..069862efde640 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -461,9 +461,9 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { } } - let (mut krate, local_sources, matches) = collect_spans_and_sources( + let (local_sources, matches) = collect_spans_and_sources( tcx, - krate, + &krate, &src_root, include_sources, generate_link_to_definition, @@ -522,7 +522,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> { }; if emit_crate { - krate = sources::render(&mut cx, krate)?; + sources::render(&mut cx, &krate)?; } // Build our search index diff --git a/src/librustdoc/html/render/span_map.rs b/src/librustdoc/html/render/span_map.rs index 1a8562d05eab7..7803a779727c5 100644 --- a/src/librustdoc/html/render/span_map.rs +++ b/src/librustdoc/html/render/span_map.rs @@ -37,21 +37,21 @@ crate enum LinkFromSrc { /// only keep the `lo` and `hi`. crate fn collect_spans_and_sources( tcx: TyCtxt<'_>, - krate: clean::Crate, + krate: &clean::Crate, src_root: &Path, include_sources: bool, generate_link_to_definition: bool, -) -> (clean::Crate, FxHashMap, FxHashMap) { +) -> (FxHashMap, FxHashMap) { let mut visitor = SpanMapVisitor { tcx, matches: FxHashMap::default() }; if include_sources { if generate_link_to_definition { tcx.hir().walk_toplevel_module(&mut visitor); } - let (krate, sources) = sources::collect_local_sources(tcx, src_root, krate); - (krate, sources, visitor.matches) + let sources = sources::collect_local_sources(tcx, src_root, &krate); + (sources, visitor.matches) } else { - (krate, Default::default(), Default::default()) + (Default::default(), Default::default()) } } diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index 74fcd22adf854..0a9f9741d7bd6 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -16,25 +16,25 @@ use std::ffi::OsStr; use std::fs; use std::path::{Component, Path, PathBuf}; -crate fn render(cx: &mut Context<'_>, krate: clean::Crate) -> Result { +crate fn render(cx: &mut Context<'_>, krate: &clean::Crate) -> Result<(), Error> { info!("emitting source files"); let dst = cx.dst.join("src").join(&*krate.name(cx.tcx()).as_str()); cx.shared.ensure_dir(&dst)?; let mut collector = SourceCollector { dst, cx, emitted_local_sources: FxHashSet::default() }; - collector.visit_crate(&krate); - Ok(krate) + collector.visit_crate(krate); + Ok(()) } crate fn collect_local_sources<'tcx>( tcx: TyCtxt<'tcx>, src_root: &Path, - krate: clean::Crate, -) -> (clean::Crate, FxHashMap) { + krate: &clean::Crate, +) -> FxHashMap { let mut lsc = LocalSourcesCollector { tcx, local_sources: FxHashMap::default(), src_root }; - lsc.visit_crate(&krate); - (krate, lsc.local_sources) + lsc.visit_crate(krate); + lsc.local_sources } struct LocalSourcesCollector<'a, 'tcx> { From 4ee2d0351a67e382f56a85eaeada0b7c773f0691 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 21:05:09 -0700 Subject: [PATCH 06/11] Fix FIXMEs in `rustdoc::html::sources` One of the FIXMEs is irrelevant since that code is only run if `include_sources` is set. I fixed the other FIXME. --- src/librustdoc/html/sources.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index 0a9f9741d7bd6..30ec592ba23d8 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -85,8 +85,6 @@ impl DocVisitor for LocalSourcesCollector<'_, '_> { fn visit_item(&mut self, item: &clean::Item) { self.add_local_source(item); - // FIXME: if `include_sources` isn't set and DocFolder didn't require consuming the crate by value, - // we could return None here without having to walk the rest of the crate. self.visit_item_recur(item) } } @@ -102,6 +100,10 @@ struct SourceCollector<'a, 'tcx> { impl DocVisitor for SourceCollector<'_, '_> { fn visit_item(&mut self, item: &clean::Item) { + if !self.cx.include_sources { + return; + } + let tcx = self.cx.tcx(); let span = item.span(tcx); let sess = tcx.sess; @@ -109,7 +111,7 @@ impl DocVisitor for SourceCollector<'_, '_> { // If we're not rendering sources, there's nothing to do. // If we're including source files, and we haven't seen this file yet, // then we need to render it out to the filesystem. - if self.cx.include_sources && is_real_and_local(span, sess) { + if is_real_and_local(span, sess) { let filename = span.filename(sess); let span = span.inner(); let pos = sess.source_map().lookup_source_file(span.lo()); @@ -134,8 +136,7 @@ impl DocVisitor for SourceCollector<'_, '_> { } }; } - // FIXME: if `include_sources` isn't set and DocFolder didn't require consuming the crate by value, - // we could return None here without having to walk the rest of the crate. + self.visit_item_recur(item) } } From 10606c3caf42124bf5b8069647badc0cfcc31a89 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 21:07:32 -0700 Subject: [PATCH 07/11] rustdoc: Small micro-optimizations and cleanups * Flip conjuncts of `&&` in rustdoc The `CrateNum` comparison should be very cheap, while `span.filename()` fetches and clones a `FileName`. * Use `into_local_path()` instead of `local_path().clone()` --- src/librustdoc/html/sources.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index 30ec592ba23d8..c8e93374e63cc 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -44,7 +44,7 @@ struct LocalSourcesCollector<'a, 'tcx> { } fn is_real_and_local(span: clean::Span, sess: &Session) -> bool { - span.filename(sess).is_real() && span.cnum(sess) == LOCAL_CRATE + span.cnum(sess) == LOCAL_CRATE && span.filename(sess).is_real() } impl LocalSourcesCollector<'_, '_> { @@ -56,12 +56,13 @@ impl LocalSourcesCollector<'_, '_> { return; } let filename = span.filename(sess); - let p = match filename { - FileName::Real(ref file) => match file.local_path() { - Some(p) => p.to_path_buf(), - _ => return, - }, - _ => return, + let p = if let FileName::Real(file) = filename { + match file.into_local_path() { + Some(p) => p, + None => return, + } + } else { + return; }; if self.local_sources.contains_key(&*p) { // We've already emitted this source From b74287076cd31ee538f178220e00de60bfcf9655 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 21:24:11 -0700 Subject: [PATCH 08/11] Convert many impls of `DocFolder` to `DocVisitor` Many of `DocFolder`'s impls didn't actually transform the syntax tree, so they can be visitors instead. --- src/librustdoc/passes/bare_urls.rs | 13 ++++++----- .../passes/calculate_doc_coverage.rs | 23 ++++++++++--------- .../passes/check_code_block_syntax.rs | 11 +++++---- .../passes/check_doc_test_visibility.rs | 12 +++++----- src/librustdoc/passes/html_tags.rs | 18 +++++++-------- 5 files changed, 39 insertions(+), 38 deletions(-) diff --git a/src/librustdoc/passes/bare_urls.rs b/src/librustdoc/passes/bare_urls.rs index 4501914fe0c07..4e146a07d154a 100644 --- a/src/librustdoc/passes/bare_urls.rs +++ b/src/librustdoc/passes/bare_urls.rs @@ -1,8 +1,8 @@ use super::Pass; use crate::clean::*; use crate::core::DocContext; -use crate::fold::DocFolder; use crate::html::markdown::main_body_opts; +use crate::visit::DocVisitor; use core::ops::Range; use pulldown_cmark::{Event, Parser, Tag}; use regex::Regex; @@ -53,16 +53,17 @@ impl<'a, 'tcx> BareUrlsLinter<'a, 'tcx> { } crate fn check_bare_urls(krate: Crate, cx: &mut DocContext<'_>) -> Crate { - BareUrlsLinter { cx }.fold_crate(krate) + BareUrlsLinter { cx }.visit_crate(&krate); + krate } -impl<'a, 'tcx> DocFolder for BareUrlsLinter<'a, 'tcx> { - fn fold_item(&mut self, item: Item) -> Option { +impl<'a, 'tcx> DocVisitor for BareUrlsLinter<'a, 'tcx> { + fn visit_item(&mut self, item: &Item) { let hir_id = match DocContext::as_local_hir_id(self.cx.tcx, item.def_id) { Some(hir_id) => hir_id, None => { // If non-local, no need to check anything. - return Some(self.fold_item_recur(item)); + return; } }; let dox = item.attrs.collapsed_doc_value().unwrap_or_default(); @@ -106,6 +107,6 @@ impl<'a, 'tcx> DocFolder for BareUrlsLinter<'a, 'tcx> { } } - Some(self.fold_item_recur(item)) + self.visit_item_recur(item) } } diff --git a/src/librustdoc/passes/calculate_doc_coverage.rs b/src/librustdoc/passes/calculate_doc_coverage.rs index 5e3bd41b85c7e..85542ebd9ac55 100644 --- a/src/librustdoc/passes/calculate_doc_coverage.rs +++ b/src/librustdoc/passes/calculate_doc_coverage.rs @@ -1,9 +1,9 @@ use crate::clean; use crate::core::DocContext; -use crate::fold::{self, DocFolder}; use crate::html::markdown::{find_testable_code, ErrorCodes}; use crate::passes::check_doc_test_visibility::{should_have_doc_example, Tests}; use crate::passes::Pass; +use crate::visit::DocVisitor; use rustc_hir as hir; use rustc_lint::builtin::MISSING_DOCS; use rustc_middle::lint::LintLevelSource; @@ -23,7 +23,7 @@ crate const CALCULATE_DOC_COVERAGE: Pass = Pass { fn calculate_doc_coverage(krate: clean::Crate, ctx: &mut DocContext<'_>) -> clean::Crate { let mut calc = CoverageCalculator { items: Default::default(), ctx }; - let krate = calc.fold_crate(krate); + calc.visit_crate(&krate); calc.print_results(); @@ -182,17 +182,18 @@ impl<'a, 'b> CoverageCalculator<'a, 'b> { } } -impl<'a, 'b> fold::DocFolder for CoverageCalculator<'a, 'b> { - fn fold_item(&mut self, i: clean::Item) -> Option { +impl<'a, 'b> DocVisitor for CoverageCalculator<'a, 'b> { + fn visit_item(&mut self, i: &clean::Item) { + if !i.def_id.is_local() { + // non-local items are skipped because they can be out of the users control, + // especially in the case of trait impls, which rustdoc eagerly inlines + return; + } + match *i.kind { - _ if !i.def_id.is_local() => { - // non-local items are skipped because they can be out of the users control, - // especially in the case of trait impls, which rustdoc eagerly inlines - return Some(i); - } clean::StrippedItem(..) => { // don't count items in stripped modules - return Some(i); + return; } // docs on `use` and `extern crate` statements are not displayed, so they're not // worth counting @@ -269,6 +270,6 @@ impl<'a, 'b> fold::DocFolder for CoverageCalculator<'a, 'b> { } } - Some(self.fold_item_recur(i)) + self.visit_item_recur(i) } } diff --git a/src/librustdoc/passes/check_code_block_syntax.rs b/src/librustdoc/passes/check_code_block_syntax.rs index b18208d26e2c4..fd2ab0dc97cb2 100644 --- a/src/librustdoc/passes/check_code_block_syntax.rs +++ b/src/librustdoc/passes/check_code_block_syntax.rs @@ -8,9 +8,9 @@ use rustc_span::{hygiene::AstPass, ExpnData, ExpnKind, FileName, InnerSpan, DUMM use crate::clean; use crate::core::DocContext; -use crate::fold::DocFolder; use crate::html::markdown::{self, RustCodeBlock}; use crate::passes::Pass; +use crate::visit::DocVisitor; crate const CHECK_CODE_BLOCK_SYNTAX: Pass = Pass { name: "check-code-block-syntax", @@ -19,7 +19,8 @@ crate const CHECK_CODE_BLOCK_SYNTAX: Pass = Pass { }; crate fn check_code_block_syntax(krate: clean::Crate, cx: &mut DocContext<'_>) -> clean::Crate { - SyntaxChecker { cx }.fold_crate(krate) + SyntaxChecker { cx }.visit_crate(&krate); + krate } struct SyntaxChecker<'a, 'tcx> { @@ -141,8 +142,8 @@ impl<'a, 'tcx> SyntaxChecker<'a, 'tcx> { } } -impl<'a, 'tcx> DocFolder for SyntaxChecker<'a, 'tcx> { - fn fold_item(&mut self, item: clean::Item) -> Option { +impl<'a, 'tcx> DocVisitor for SyntaxChecker<'a, 'tcx> { + fn visit_item(&mut self, item: &clean::Item) { if let Some(dox) = &item.attrs.collapsed_doc_value() { let sp = item.attr_span(self.cx.tcx); let extra = crate::html::markdown::ExtraInfo::new_did( @@ -155,7 +156,7 @@ impl<'a, 'tcx> DocFolder for SyntaxChecker<'a, 'tcx> { } } - Some(self.fold_item_recur(item)) + self.visit_item_recur(item) } } diff --git a/src/librustdoc/passes/check_doc_test_visibility.rs b/src/librustdoc/passes/check_doc_test_visibility.rs index 69a526d461810..7d3010cf3325b 100644 --- a/src/librustdoc/passes/check_doc_test_visibility.rs +++ b/src/librustdoc/passes/check_doc_test_visibility.rs @@ -7,8 +7,8 @@ use super::Pass; use crate::clean; use crate::clean::*; use crate::core::DocContext; -use crate::fold::DocFolder; use crate::html::markdown::{find_testable_code, ErrorCodes, Ignore, LangString}; +use crate::visit::DocVisitor; use crate::visit_ast::inherits_doc_hidden; use rustc_hir as hir; use rustc_middle::lint::LintLevelSource; @@ -27,17 +27,17 @@ struct DocTestVisibilityLinter<'a, 'tcx> { crate fn check_doc_test_visibility(krate: Crate, cx: &mut DocContext<'_>) -> Crate { let mut coll = DocTestVisibilityLinter { cx }; - - coll.fold_crate(krate) + coll.visit_crate(&krate); + krate } -impl<'a, 'tcx> DocFolder for DocTestVisibilityLinter<'a, 'tcx> { - fn fold_item(&mut self, item: Item) -> Option { +impl<'a, 'tcx> DocVisitor for DocTestVisibilityLinter<'a, 'tcx> { + fn visit_item(&mut self, item: &Item) { let dox = item.attrs.collapsed_doc_value().unwrap_or_else(String::new); look_for_tests(self.cx, &dox, &item); - Some(self.fold_item_recur(item)) + self.visit_item_recur(item) } } diff --git a/src/librustdoc/passes/html_tags.rs b/src/librustdoc/passes/html_tags.rs index a3fde92d7655d..da9d1305b9394 100644 --- a/src/librustdoc/passes/html_tags.rs +++ b/src/librustdoc/passes/html_tags.rs @@ -1,8 +1,8 @@ use super::Pass; use crate::clean::*; use crate::core::DocContext; -use crate::fold::DocFolder; use crate::html::markdown::main_body_opts; +use crate::visit::DocVisitor; use core::ops::Range; use pulldown_cmark::{Event, Parser, Tag}; use std::iter::Peekable; @@ -19,13 +19,11 @@ struct InvalidHtmlTagsLinter<'a, 'tcx> { } crate fn check_invalid_html_tags(krate: Crate, cx: &mut DocContext<'_>) -> Crate { - if !cx.tcx.sess.is_nightly_build() { - krate - } else { + if cx.tcx.sess.is_nightly_build() { let mut coll = InvalidHtmlTagsLinter { cx }; - - coll.fold_crate(krate) + coll.visit_crate(&krate); } + krate } const ALLOWED_UNCLOSED: &[&str] = &[ @@ -165,14 +163,14 @@ fn extract_tags( } } -impl<'a, 'tcx> DocFolder for InvalidHtmlTagsLinter<'a, 'tcx> { - fn fold_item(&mut self, item: Item) -> Option { +impl<'a, 'tcx> DocVisitor for InvalidHtmlTagsLinter<'a, 'tcx> { + fn visit_item(&mut self, item: &Item) { let tcx = self.cx.tcx; let hir_id = match DocContext::as_local_hir_id(tcx, item.def_id) { Some(hir_id) => hir_id, None => { // If non-local, no need to check anything. - return Some(self.fold_item_recur(item)); + return; } }; let dox = item.attrs.collapsed_doc_value().unwrap_or_default(); @@ -217,6 +215,6 @@ impl<'a, 'tcx> DocFolder for InvalidHtmlTagsLinter<'a, 'tcx> { } } - Some(self.fold_item_recur(item)) + self.visit_item_recur(item) } } From 3f0f51017c2febcf1709d1d7ff2bf6957c46c963 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 21:33:50 -0700 Subject: [PATCH 09/11] Convert more impls of `DocFolder` to `DocVisitor` I think these are the last of the impls that can be easily converted to visitors. --- .../passes/collect_intra_doc_links.rs | 22 ++++++------- src/librustdoc/passes/collect_trait_impls.rs | 31 +++++++++---------- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 9b2fe0c77e6cf..8541e6e18816f 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -32,10 +32,10 @@ use std::ops::Range; use crate::clean::{self, utils::find_nearest_parent_module, Crate, Item, ItemLink, PrimitiveType}; use crate::core::DocContext; -use crate::fold::DocFolder; use crate::html::markdown::{markdown_links, MarkdownLink}; use crate::lint::{BROKEN_INTRA_DOC_LINKS, PRIVATE_INTRA_DOC_LINKS}; use crate::passes::Pass; +use crate::visit::DocVisitor; mod early; crate use early::load_intra_link_crates; @@ -47,13 +47,14 @@ crate const COLLECT_INTRA_DOC_LINKS: Pass = Pass { }; fn collect_intra_doc_links(krate: Crate, cx: &mut DocContext<'_>) -> Crate { - LinkCollector { + let mut collector = LinkCollector { cx, mod_ids: Vec::new(), kind_side_channel: Cell::new(None), visited_links: FxHashMap::default(), - } - .fold_crate(krate) + }; + collector.visit_crate(&krate); + krate } /// Top-level errors emitted by this pass. @@ -816,8 +817,8 @@ fn is_derive_trait_collision(ns: &PerNS DocFolder for LinkCollector<'a, 'tcx> { - fn fold_item(&mut self, item: Item) -> Option { +impl<'a, 'tcx> DocVisitor for LinkCollector<'a, 'tcx> { + fn visit_item(&mut self, item: &Item) { use rustc_middle::ty::DefIdTree; let parent_node = @@ -911,17 +912,16 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { } } - Some(if item.is_mod() { + if item.is_mod() { if !inner_docs { self.mod_ids.push(item.def_id.expect_def_id()); } - let ret = self.fold_item_recur(item); + self.visit_item_recur(item); self.mod_ids.pop(); - ret } else { - self.fold_item_recur(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 7a4198198fa69..000fe01f5adb9 100644 --- a/src/librustdoc/passes/collect_trait_impls.rs +++ b/src/librustdoc/passes/collect_trait_impls.rs @@ -1,7 +1,6 @@ use super::Pass; -use crate::clean::*; use crate::core::DocContext; -use crate::fold::DocFolder; +use crate::{clean::*, visit::DocVisitor}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_hir::def_id::DefId; @@ -14,17 +13,18 @@ crate const COLLECT_TRAIT_IMPLS: Pass = Pass { description: "retrieves trait impls for items in the crate", }; -crate fn collect_trait_impls(krate: Crate, cx: &mut DocContext<'_>) -> Crate { - let (mut krate, synth_impls) = cx.sess().time("collect_synthetic_impls", || { +crate fn collect_trait_impls(mut krate: Crate, cx: &mut DocContext<'_>) -> Crate { + let synth_impls = cx.sess().time("collect_synthetic_impls", || { let mut synth = SyntheticImplCollector { cx, impls: Vec::new() }; - (synth.fold_crate(krate), synth.impls) + synth.visit_crate(&krate); + synth.impls }); let prims: FxHashSet = krate.primitives.iter().map(|p| p.1).collect(); let crate_items = { let mut coll = ItemCollector::new(); - krate = cx.sess().time("collect_items_for_trait_impls", || coll.fold_crate(krate)); + cx.sess().time("collect_items_for_trait_impls", || coll.visit_crate(&krate)); coll.items }; @@ -152,14 +152,13 @@ crate fn collect_trait_impls(krate: Crate, cx: &mut DocContext<'_>) -> Crate { } } - let items = if let ModuleItem(Module { ref mut items, .. }) = *krate.module.kind { - items + if let ModuleItem(Module { items, .. }) = &mut *krate.module.kind { + items.extend(synth_impls); + items.extend(new_items); } else { panic!("collect-trait-impls can't run"); }; - items.extend(synth_impls); - items.extend(new_items); krate } @@ -168,8 +167,8 @@ struct SyntheticImplCollector<'a, 'tcx> { impls: Vec, } -impl<'a, 'tcx> DocFolder for SyntheticImplCollector<'a, 'tcx> { - fn fold_item(&mut self, i: Item) -> Option { +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? if !self @@ -184,7 +183,7 @@ impl<'a, 'tcx> DocFolder for SyntheticImplCollector<'a, 'tcx> { } } - Some(self.fold_item_recur(i)) + self.visit_item_recur(i) } } @@ -199,11 +198,11 @@ impl ItemCollector { } } -impl DocFolder for ItemCollector { - fn fold_item(&mut self, i: Item) -> Option { +impl DocVisitor for ItemCollector { + fn visit_item(&mut self, i: &Item) { self.items.insert(i.def_id); - Some(self.fold_item_recur(i)) + self.visit_item_recur(i) } } From c03cab3fd07ab070d6ec0fa77655ad8d3a85a372 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Sun, 31 Oct 2021 21:53:31 -0700 Subject: [PATCH 10/11] Fix `RefCell` `BorrowMut` error in `DocVisitor` Until `external_traits` is cleaned up (i.e., no longer behind a `RefCell`), `DocVisitor` will have to `take` `external_traits` -- just like `DocFolder` -- to prevent `RefCell` runtime errors. --- src/librustdoc/fold.rs | 12 +++++------- src/librustdoc/visit.rs | 8 +++++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/librustdoc/fold.rs b/src/librustdoc/fold.rs index 9009a9bd41b63..e8069f39f45dc 100644 --- a/src/librustdoc/fold.rs +++ b/src/librustdoc/fold.rs @@ -84,14 +84,12 @@ 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.trait_.items = - v.trait_.items.into_iter().filter_map(|i| self.fold_item(i)).collect(); - c.external_traits.borrow_mut().insert(k, v); - } + let external_traits = { std::mem::take(&mut *c.external_traits.borrow_mut()) }; + for (k, mut v) in external_traits { + v.trait_.items = v.trait_.items.into_iter().filter_map(|i| self.fold_item(i)).collect(); + c.external_traits.borrow_mut().insert(k, v); } + c } } diff --git a/src/librustdoc/visit.rs b/src/librustdoc/visit.rs index 9edf6e11bec90..44129548d1c99 100644 --- a/src/librustdoc/visit.rs +++ b/src/librustdoc/visit.rs @@ -43,9 +43,11 @@ crate trait DocVisitor: Sized { fn visit_crate(&mut self, c: &Crate) { self.visit_item(&c.module); - let external_traits = c.external_traits.borrow(); - for v in external_traits.values() { - v.trait_.items.iter().for_each(|i| self.visit_item(i)) + // 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.trait_.items.iter().for_each(|i| self.visit_item(i)); + c.external_traits.borrow_mut().insert(k, v); } } } From 8e4bcdf84b2f07d0a3e0aac21b85b54b14031f33 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Mon, 1 Nov 2021 15:46:08 -0700 Subject: [PATCH 11/11] List all cases explicitly in `Doc{Folder,Visitor}` --- src/librustdoc/fold.rs | 22 ++++++++++++++++++-- src/librustdoc/passes/collect_trait_impls.rs | 3 ++- src/librustdoc/passes/html_tags.rs | 4 +++- src/librustdoc/visit.rs | 22 ++++++++++++++++++-- 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/src/librustdoc/fold.rs b/src/librustdoc/fold.rs index e8069f39f45dc..65cd71307aa3f 100644 --- a/src/librustdoc/fold.rs +++ b/src/librustdoc/fold.rs @@ -60,8 +60,26 @@ crate trait DocFolder: Sized { } Variant::CLike => VariantItem(Variant::CLike), }, - // FIXME: list all cases explicitly - x => x, + ExternCrateItem { src: _ } + | ImportItem(_) + | FunctionItem(_) + | TypedefItem(_, _) + | OpaqueTyItem(_) + | StaticItem(_) + | ConstantItem(_) + | TraitAliasItem(_) + | TyMethodItem(_) + | MethodItem(_, _) + | StructFieldItem(_) + | ForeignFunctionItem(_) + | ForeignStaticItem(_) + | ForeignTypeItem + | MacroItem(_) + | ProcMacroItem(_) + | PrimitiveItem(_) + | AssocConstItem(_, _) + | AssocTypeItem(_, _) + | KeywordItem(_) => kind, } } diff --git a/src/librustdoc/passes/collect_trait_impls.rs b/src/librustdoc/passes/collect_trait_impls.rs index 000fe01f5adb9..77513b05ff2f9 100644 --- a/src/librustdoc/passes/collect_trait_impls.rs +++ b/src/librustdoc/passes/collect_trait_impls.rs @@ -1,6 +1,7 @@ use super::Pass; +use crate::clean::*; use crate::core::DocContext; -use crate::{clean::*, visit::DocVisitor}; +use crate::visit::DocVisitor; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_hir::def_id::DefId; diff --git a/src/librustdoc/passes/html_tags.rs b/src/librustdoc/passes/html_tags.rs index da9d1305b9394..56b222d893262 100644 --- a/src/librustdoc/passes/html_tags.rs +++ b/src/librustdoc/passes/html_tags.rs @@ -3,9 +3,11 @@ use crate::clean::*; use crate::core::DocContext; use crate::html::markdown::main_body_opts; use crate::visit::DocVisitor; -use core::ops::Range; + use pulldown_cmark::{Event, Parser, Tag}; + use std::iter::Peekable; +use std::ops::Range; use std::str::CharIndices; crate const CHECK_INVALID_HTML_TAGS: Pass = Pass { diff --git a/src/librustdoc/visit.rs b/src/librustdoc/visit.rs index 44129548d1c99..df4d1558ebdf4 100644 --- a/src/librustdoc/visit.rs +++ b/src/librustdoc/visit.rs @@ -23,8 +23,26 @@ crate trait DocVisitor: Sized { Variant::Tuple(fields) => fields.iter().for_each(|x| self.visit_item(x)), Variant::CLike => {} }, - // FIXME: list all cases explicitly - _ => return, + ExternCrateItem { src: _ } + | ImportItem(_) + | FunctionItem(_) + | TypedefItem(_, _) + | OpaqueTyItem(_) + | StaticItem(_) + | ConstantItem(_) + | TraitAliasItem(_) + | TyMethodItem(_) + | MethodItem(_, _) + | StructFieldItem(_) + | ForeignFunctionItem(_) + | ForeignStaticItem(_) + | ForeignTypeItem + | MacroItem(_) + | ProcMacroItem(_) + | PrimitiveItem(_) + | AssocConstItem(_, _) + | AssocTypeItem(_, _) + | KeywordItem(_) => {} } }