Auto merge of #104889 - GuillaumeGomez:fix-impl-block-in-const-expr, r=notriddle

bors · bors · commit cce9e72c5599 · 2022-12-22T08:37:58.000Z
Fix impl block in const expr Fixes #83026. The problem was that we didn't visit block expressions. Considering how big the [walk_expr](https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_hir/intravisit.rs.html#678) function is, I decided to instead implement the `hir` visitor on the struct. It also answers the question which was in a comment for `RustdocVisitor`: we should have used a visitor instead of our ad-hoc implementation. Adding this visitor also added some extra checks that weren't present before (check changes in `rustdoc-ui` tests). r? `@notriddle`
diff --git a/src/librustdoc/html/render/write_shared.rs b/src/librustdoc/html/render/write_shared.rs
@@ -137,7 +137,7 @@ pub(super) fn write_shared(
         Ok((ret, krates))
     }
 
-    /// Read a file and return all lines that match the <code>"{crate}":{data},\</code> format,
+    /// Read a file and return all lines that match the <code>"{crate}":{data},\ </code> format,
     /// and return a tuple `(Vec<DataString>, Vec<CrateNameString>)`.
     ///
     /// This forms the payload of files that look like this:
diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs
@@ -5,8 +5,10 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::DefId;
+use rustc_hir::intravisit::{walk_item, Visitor};
 use rustc_hir::Node;
 use rustc_hir::CRATE_HIR_ID;
+use rustc_middle::hir::nested_filter;
 use rustc_middle::ty::TyCtxt;
 use rustc_span::def_id::{CRATE_DEF_ID, LOCAL_CRATE};
 use rustc_span::symbol::{kw, sym, Symbol};
@@ -57,29 +59,34 @@ pub(crate) fn inherits_doc_hidden(tcx: TyCtxt<'_>, mut node: hir::HirId) -> bool
     false
 }
 
-// Also, is there some reason that this doesn't use the 'visit'
-// framework from syntax?.
-
 pub(crate) struct RustdocVisitor<'a, 'tcx> {
     cx: &'a mut core::DocContext<'tcx>,
     view_item_stack: FxHashSet<hir::HirId>,
     inlining: bool,
     /// Are the current module and all of its parents public?
     inside_public_path: bool,
     exact_paths: FxHashMap<DefId, Vec<Symbol>>,
+    modules: Vec<Module<'tcx>>,
 }
 
 impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
     pub(crate) fn new(cx: &'a mut core::DocContext<'tcx>) -> RustdocVisitor<'a, 'tcx> {
         // If the root is re-exported, terminate all recursion.
         let mut stack = FxHashSet::default();
         stack.insert(hir::CRATE_HIR_ID);
+        let om = Module::new(
+            cx.tcx.crate_name(LOCAL_CRATE),
+            hir::CRATE_HIR_ID,
+            cx.tcx.hir().root_module().spans.inner_span,
+        );
+
         RustdocVisitor {
             cx,
             view_item_stack: stack,
             inlining: false,
             inside_public_path: true,
             exact_paths: FxHashMap::default(),
+            modules: vec![om],
         }
     }
 
@@ -89,12 +96,10 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
     }
 
     pub(crate) fn visit(mut self) -> Module<'tcx> {
-        let mut top_level_module = self.visit_mod_contents(
-            hir::CRATE_HIR_ID,
-            self.cx.tcx.hir().root_module(),
-            self.cx.tcx.crate_name(LOCAL_CRATE),
-            None,
-        );
+        let root_module = self.cx.tcx.hir().root_module();
+        self.visit_mod_contents(CRATE_HIR_ID, root_module);
+
+        let mut top_level_module = self.modules.pop().unwrap();
 
         // `#[macro_export] macro_rules!` items are reexported at the top level of the
         // crate, regardless of where they're defined. We want to document the
@@ -109,15 +114,13 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
         // macro in the same module.
         let mut inserted = FxHashSet::default();
         for export in self.cx.tcx.module_reexports(CRATE_DEF_ID).unwrap_or(&[]) {
-            if let Res::Def(DefKind::Macro(_), def_id) = export.res {
-                if let Some(local_def_id) = def_id.as_local() {
-                    if self.cx.tcx.has_attr(def_id, sym::macro_export) {
-                        if inserted.insert(def_id) {
-                            let item = self.cx.tcx.hir().expect_item(local_def_id);
-                            top_level_module.items.push((item, None, None));
-                        }
-                    }
-                }
+            if let Res::Def(DefKind::Macro(_), def_id) = export.res &&
+                let Some(local_def_id) = def_id.as_local() &&
+                self.cx.tcx.has_attr(def_id, sym::macro_export) &&
+                inserted.insert(def_id)
+            {
+                    let item = self.cx.tcx.hir().expect_item(local_def_id);
+                    top_level_module.items.push((item, None, None));
             }
         }
 
@@ -151,36 +154,34 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
         top_level_module
     }
 
-    fn visit_mod_contents(
-        &mut self,
-        id: hir::HirId,
-        m: &'tcx hir::Mod<'tcx>,
-        name: Symbol,
-        parent_id: Option<hir::HirId>,
-    ) -> Module<'tcx> {
-        let mut om = Module::new(name, id, m.spans.inner_span);
+    /// This method will go through the given module items in two passes:
+    /// 1. The items which are not glob imports/reexports.
+    /// 2. The glob imports/reexports.
+    fn visit_mod_contents(&mut self, id: hir::HirId, m: &'tcx hir::Mod<'tcx>) {
+        debug!("Going through module {:?}", m);
         let def_id = self.cx.tcx.hir().local_def_id(id).to_def_id();
         // Keep track of if there were any private modules in the path.
         let orig_inside_public_path = self.inside_public_path;
         self.inside_public_path &= self.cx.tcx.visibility(def_id).is_public();
+
+        // Reimplementation of `walk_mod` because we need to do it in two passes (explanations in
+        // the second loop):
         for &i in m.item_ids {
             let item = self.cx.tcx.hir().item(i);
-            if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) {
-                continue;
+            if !matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) {
+                self.visit_item(item);
             }
-            self.visit_item(item, None, &mut om, parent_id);
         }
         for &i in m.item_ids {
             let item = self.cx.tcx.hir().item(i);
             // To match the way import precedence works, visit glob imports last.
             // Later passes in rustdoc will de-duplicate by name and kind, so if glob-
             // imported items appear last, then they'll be the ones that get discarded.
             if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) {
-                self.visit_item(item, None, &mut om, parent_id);
+                self.visit_item(item);
             }
         }
         self.inside_public_path = orig_inside_public_path;
-        om
     }
 
     /// Tries to resolve the target of a `pub use` statement and inlines the
@@ -198,7 +199,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
         res: Res,
         renamed: Option<Symbol>,
         glob: bool,
-        om: &mut Module<'tcx>,
         please_inline: bool,
     ) -> bool {
         debug!("maybe_inline_local res: {:?}", res);
@@ -249,20 +249,20 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                 let prev = mem::replace(&mut self.inlining, true);
                 for &i in m.item_ids {
                     let i = self.cx.tcx.hir().item(i);
-                    self.visit_item(i, None, om, Some(id));
+                    self.visit_item_inner(i, None, Some(id));
                 }
                 self.inlining = prev;
                 true
             }
             Node::Item(it) if !glob => {
                 let prev = mem::replace(&mut self.inlining, true);
-                self.visit_item(it, renamed, om, Some(id));
+                self.visit_item_inner(it, renamed, Some(id));
                 self.inlining = prev;
                 true
             }
             Node::ForeignItem(it) if !glob => {
                 let prev = mem::replace(&mut self.inlining, true);
-                self.visit_foreign_item(it, renamed, om);
+                self.visit_foreign_item_inner(it, renamed);
                 self.inlining = prev;
                 true
             }
@@ -272,13 +272,22 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
         ret
     }
 
-    fn visit_item(
+    #[inline]
+    fn add_to_current_mod(
         &mut self,
         item: &'tcx hir::Item<'_>,
         renamed: Option<Symbol>,
-        om: &mut Module<'tcx>,
         parent_id: Option<hir::HirId>,
     ) {
+        self.modules.last_mut().unwrap().items.push((item, renamed, parent_id))
+    }
+
+    fn visit_item_inner(
+        &mut self,
+        item: &'tcx hir::Item<'_>,
+        renamed: Option<Symbol>,
+        parent_id: Option<hir::HirId>,
+    ) -> bool {
         debug!("visiting item {:?}", item);
         let name = renamed.unwrap_or(item.ident.name);
 
@@ -293,7 +302,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
             hir::ItemKind::ForeignMod { items, .. } => {
                 for item in items {
                     let item = self.cx.tcx.hir().foreign_item(item.id);
-                    self.visit_foreign_item(item, None, om);
+                    self.visit_foreign_item_inner(item, None);
                 }
             }
             // If we're inlining, skip private items or item reexported as "_".
@@ -326,14 +335,13 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                             res,
                             ident,
                             is_glob,
-                            om,
                             please_inline,
                         ) {
                             continue;
                         }
                     }
 
-                    om.items.push((item, renamed, parent_id))
+                    self.add_to_current_mod(item, renamed, parent_id);
                 }
             }
             hir::ItemKind::Macro(ref macro_def, _) => {
@@ -353,11 +361,11 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                 let nonexported = !self.cx.tcx.has_attr(def_id, sym::macro_export);
 
                 if is_macro_2_0 || nonexported || self.inlining {
-                    om.items.push((item, renamed, None));
+                    self.add_to_current_mod(item, renamed, None);
                 }
             }
             hir::ItemKind::Mod(ref m) => {
-                om.mods.push(self.visit_mod_contents(item.hir_id(), m, name, parent_id));
+                self.enter_mod(item.hir_id(), m, name);
             }
             hir::ItemKind::Fn(..)
             | hir::ItemKind::ExternCrate(..)
@@ -368,33 +376,92 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
             | hir::ItemKind::OpaqueTy(..)
             | hir::ItemKind::Static(..)
             | hir::ItemKind::Trait(..)
-            | hir::ItemKind::TraitAlias(..) => om.items.push((item, renamed, parent_id)),
+            | hir::ItemKind::TraitAlias(..) => {
+                self.add_to_current_mod(item, renamed, parent_id);
+            }
             hir::ItemKind::Const(..) => {
                 // Underscore constants do not correspond to a nameable item and
                 // so are never useful in documentation.
                 if name != kw::Underscore {
-                    om.items.push((item, renamed, parent_id));
+                    self.add_to_current_mod(item, renamed, parent_id);
                 }
             }
             hir::ItemKind::Impl(impl_) => {
                 // Don't duplicate impls when inlining or if it's implementing a trait, we'll pick
                 // them up regardless of where they're located.
                 if !self.inlining && impl_.of_trait.is_none() {
-                    om.items.push((item, None, None));
+                    self.add_to_current_mod(item, None, None);
                 }
             }
         }
+        true
     }
 
-    fn visit_foreign_item(
+    fn visit_foreign_item_inner(
         &mut self,
         item: &'tcx hir::ForeignItem<'_>,
         renamed: Option<Symbol>,
-        om: &mut Module<'tcx>,
     ) {
         // If inlining we only want to include public functions.
         if !self.inlining || self.cx.tcx.visibility(item.owner_id).is_public() {
-            om.foreigns.push((item, renamed));
+            self.modules.last_mut().unwrap().foreigns.push((item, renamed));
         }
     }
+
+    /// This method will create a new module and push it onto the "modules stack" then call
+    /// `visit_mod_contents`. Once done, it'll remove it from the "modules stack" and instead
+    /// add into into the list of modules of the current module.
+    fn enter_mod(&mut self, id: hir::HirId, m: &'tcx hir::Mod<'tcx>, name: Symbol) {
+        self.modules.push(Module::new(name, id, m.spans.inner_span));
+
+        self.visit_mod_contents(id, m);
+
+        let last = self.modules.pop().unwrap();
+        self.modules.last_mut().unwrap().mods.push(last);
+    }
+}
+
+// We need to implement this visitor so it'll go everywhere and retrieve items we're interested in
+// such as impl blocks in const blocks.
+impl<'a, 'tcx> Visitor<'tcx> for RustdocVisitor<'a, 'tcx> {
+    type NestedFilter = nested_filter::All;
+
+    fn nested_visit_map(&mut self) -> Self::Map {
+        self.cx.tcx.hir()
+    }
+
+    fn visit_item(&mut self, i: &'tcx hir::Item<'tcx>) {
+        let parent_id = if self.modules.len() > 1 {
+            Some(self.modules[self.modules.len() - 2].id)
+        } else {
+            None
+        };
+        if self.visit_item_inner(i, None, parent_id) {
+            walk_item(self, i);
+        }
+    }
+
+    fn visit_mod(&mut self, _: &hir::Mod<'tcx>, _: Span, _: hir::HirId) {
+        // Handled in `visit_item_inner`
+    }
+
+    fn visit_use(&mut self, _: &hir::UsePath<'tcx>, _: hir::HirId) {
+        // Handled in `visit_item_inner`
+    }
+
+    fn visit_path(&mut self, _: &hir::Path<'tcx>, _: hir::HirId) {
+        // Handled in `visit_item_inner`
+    }
+
+    fn visit_label(&mut self, _: &rustc_ast::Label) {
+        // Unneeded.
+    }
+
+    fn visit_infer(&mut self, _: &hir::InferArg) {
+        // Unneeded.
+    }
+
+    fn visit_lifetime(&mut self, _: &hir::Lifetime) {
+        // Unneeded.
+    }
 }
diff --git a/src/test/rustdoc-ui/infinite-recursive-type-impl-trait-return.rs b/src/test/rustdoc-ui/infinite-recursive-type-impl-trait-return.rs
@@ -1,12 +1,10 @@
-// check-pass
 // normalize-stderr-test: "`.*`" -> "`DEF_ID`"
 // normalize-stdout-test: "`.*`" -> "`DEF_ID`"
 // edition:2018
 
 pub async fn f() -> impl std::fmt::Debug {
-    // rustdoc doesn't care that this is infinitely sized
     #[derive(Debug)]
-    enum E {
+    enum E { //~ ERROR
         This(E),
         Unit,
     }
diff --git a/src/test/rustdoc-ui/infinite-recursive-type-impl-trait-return.stderr b/src/test/rustdoc-ui/infinite-recursive-type-impl-trait-return.stderr
@@ -0,0 +1,16 @@
+error[E0072]: recursive type `DEF_ID` has infinite size
+  --> $DIR/infinite-recursive-type-impl-trait-return.rs:7:5
+   |
+LL |     enum E {
+   |     ^^^^^^
+LL |         This(E),
+   |              - recursive without indirection
+   |
+help: insert some indirection (e.g., a `DEF_ID`) to break the cycle
+   |
+LL |         This(Box<E>),
+   |              ++++ +
+
+error: aborting due to previous error
+
+For more information about this error, try `DEF_ID`.
diff --git a/src/test/rustdoc-ui/infinite-recursive-type-impl-trait.rs b/src/test/rustdoc-ui/infinite-recursive-type-impl-trait.rs
@@ -1,8 +1,5 @@
-// check-pass
-
 fn f() -> impl Sized {
-    // rustdoc doesn't care that this is infinitely sized
-    enum E {
+    enum E { //~ ERROR
         V(E),
     }
     unimplemented!()
diff --git a/src/test/rustdoc-ui/infinite-recursive-type-impl-trait.stderr b/src/test/rustdoc-ui/infinite-recursive-type-impl-trait.stderr
@@ -0,0 +1,16 @@
+error[E0072]: recursive type `f::E` has infinite size
+  --> $DIR/infinite-recursive-type-impl-trait.rs:2:5
+   |
+LL |     enum E {
+   |     ^^^^^^
+LL |         V(E),
+   |           - recursive without indirection
+   |
+help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to break the cycle
+   |
+LL |         V(Box<E>),
+   |           ++++ +
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0072`.
diff --git a/src/test/rustdoc/impl-in-const-block.rs b/src/test/rustdoc/impl-in-const-block.rs

Original file line number	Diff line number	Diff line change
`@@ -137,7 +137,7 @@ pub(super) fn write_shared(`
`137`	`137`	`Ok((ret, krates))`
`138`	`138`	`}`
`139`	`139`
`140`		`- /// Read a file and return all lines that match the <code>"{crate}":{data},\</code> format,`
	`140`	`+ /// Read a file and return all lines that match the <code>"{crate}":{data},\ </code> format,`
`141`	`141`	/// and return a tuple `(Vec<DataString>, Vec<CrateNameString>)`.
`142`	`142`	`///`
`143`	`143`	`/// This forms the payload of files that look like this:`