Auto merge of rust-lang#90475 - camelid:docvisitor, r=notriddle

rustdoc: Add `DocVisitor` and use it where possible

`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<RefCell<...>>`). 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.

r? `@GuillaumeGomez`
cc `@jyn514`

Author: bors

src/librustdoc/fold.rs

@@ -46,24 +46,40 @@ crate trait DocFolder: Sized {
                 i.items = i.items.into_iter().filter_map(|x| self.fold_item(x)).collect();
                 ImplItem(i)
             }
-            VariantItem(i) => {
-                let i2 = i.clone(); // this clone is small
-                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))
-                    }
-                    _ => VariantItem(i2),
+            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))
                 }
-            }
-            x => x,
+                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),
+            },
+            ExternCrateItem { src: _ }
+            | ImportItem(_)
+            | FunctionItem(_)
+            | TypedefItem(_, _)
+            | OpaqueTyItem(_)
+            | StaticItem(_)
+            | ConstantItem(_)
+            | TraitAliasItem(_)
+            | TyMethodItem(_)
+            | MethodItem(_, _)
+            | StructFieldItem(_)
+            | ForeignFunctionItem(_)
+            | ForeignStaticItem(_)
+            | ForeignTypeItem
+            | MacroItem(_)
+            | ProcMacroItem(_)
+            | PrimitiveItem(_)
+            | AssocConstItem(_, _)
+            | AssocTypeItem(_, _)
+            | KeywordItem(_) => kind,
         }
     }
 
@@ -86,14 +102,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
     }
 }

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

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<PathBuf, String>, FxHashMap<Span, LinkFromSrc>) {
+) -> (FxHashMap<PathBuf, String>, FxHashMap<Span, LinkFromSrc>) {
     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())
     }
 }
 

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;
@@ -16,23 +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<clean::Crate, Error> {
+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 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(())
 }
 
 crate fn collect_local_sources<'tcx>(
     tcx: TyCtxt<'tcx>,
     src_root: &Path,
-    krate: clean::Crate,
-) -> (clean::Crate, FxHashMap<PathBuf, String>) {
+    krate: &clean::Crate,
+) -> FxHashMap<PathBuf, String> {
     let mut lsc = LocalSourcesCollector { tcx, local_sources: FxHashMap::default(), src_root };
-
-    let krate = lsc.fold_crate(krate);
-    (krate, lsc.local_sources)
+    lsc.visit_crate(krate);
+    lsc.local_sources
 }
 
 struct LocalSourcesCollector<'a, 'tcx> {
@@ -42,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<'_, '_> {
@@ -54,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
@@ -79,13 +82,11 @@ impl LocalSourcesCollector<'_, '_> {
     }
 }
 
-impl DocFolder for LocalSourcesCollector<'_, '_> {
-    fn fold_item(&mut self, item: clean::Item) -> Option<clean::Item> {
-        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,16 +99,20 @@ struct SourceCollector<'a, 'tcx> {
     emitted_local_sources: FxHashSet<PathBuf>,
 }
 
-impl DocFolder for SourceCollector<'_, '_> {
-    fn fold_item(&mut self, item: clean::Item) -> Option<clean::Item> {
+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;
 
         // 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());
@@ -132,9 +137,8 @@ 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)
     }
 }
 

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;
 

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<Item> {
+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)
     }
 }

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<clean::Item> {
+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)
     }
 }

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<clean::Item> {
+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)
     }
 }