Auto merge of #142074 - oli-obk:its-finally-gone, r=petrochenkov

Remove CollectItemTypesVisitor

I always felt like we were very unnecessarily walking the HIR, let's see if perf agrees

There is lots to ~~improve~~ consolidate further here, as we still have 3 item wfchecks:

* check_item (matching on the hir::ItemKind)
    * actually doing trait solver based checks (by using HIR spans)
* lower_item (matching on the hir::ItemKind after loading it again??)
    * just ensure_ok-ing a bunch of queries
* check_item_type (matching on DefKind)
    * some type based checks, mostly ensure_ok-ing a bunch of queries

fixes #121429

Author: bors

compiler/rustc_hir_analysis/src/check/check.rs

@@ -702,6 +702,29 @@ fn check_static_linkage(tcx: TyCtxt<'_>, def_id: LocalDefId) {
 }
 
 pub(crate) fn check_item_type(tcx: TyCtxt<'_>, def_id: LocalDefId) {
+    let generics = tcx.generics_of(def_id);
+
+    for param in &generics.own_params {
+        match param.kind {
+            ty::GenericParamDefKind::Lifetime { .. } => {}
+            ty::GenericParamDefKind::Type { has_default, .. } => {
+                if has_default {
+                    tcx.ensure_ok().type_of(param.def_id);
+                }
+            }
+            ty::GenericParamDefKind::Const { has_default, .. } => {
+                tcx.ensure_ok().type_of(param.def_id);
+                if has_default {
+                    // need to store default and type of default
+                    let ct = tcx.const_param_default(param.def_id).skip_binder();
+                    if let ty::ConstKind::Unevaluated(uv) = ct.kind() {
+                        tcx.ensure_ok().type_of(uv.def);
+                    }
+                }
+            }
+        }
+    }
+
     match tcx.def_kind(def_id) {
         DefKind::Static { .. } => {
             check_static_inhabited(tcx, def_id);
@@ -770,6 +793,16 @@ pub(crate) fn check_item_type(tcx: TyCtxt<'_>, def_id: LocalDefId) {
             } else {
                 check_opaque(tcx, def_id);
             }
+
+            tcx.ensure_ok().predicates_of(def_id);
+            tcx.ensure_ok().explicit_item_bounds(def_id);
+            tcx.ensure_ok().explicit_item_self_bounds(def_id);
+            tcx.ensure_ok().item_bounds(def_id);
+            tcx.ensure_ok().item_self_bounds(def_id);
+            if tcx.is_conditionally_const(def_id) {
+                tcx.ensure_ok().explicit_implied_const_bounds(def_id);
+                tcx.ensure_ok().const_conditions(def_id);
+            }
         }
         DefKind::TyAlias => {
             check_type_alias_type_params_are_used(tcx, def_id);
@@ -827,6 +860,15 @@ pub(crate) fn check_item_type(tcx: TyCtxt<'_>, def_id: LocalDefId) {
                 }
             }
         }
+        DefKind::Closure => {
+            // This is guaranteed to be called by metadata encoding,
+            // we still call it in wfcheck eagerly to ensure errors in codegen
+            // attrs prevent lints from spamming the output.
+            tcx.ensure_ok().codegen_fn_attrs(def_id);
+            // We do not call `type_of` for closures here as that
+            // depends on typecheck and would therefore hide
+            // any further errors in case one typeck fails.
+        }
         _ => {}
     }
 }

compiler/rustc_hir_analysis/src/check/wfcheck.rs

@@ -40,7 +40,6 @@ use tracing::{debug, instrument};
 use {rustc_ast as ast, rustc_hir as hir};
 
 use crate::autoderef::Autoderef;
-use crate::collect::CollectItemTypesVisitor;
 use crate::constrained_generic_params::{Parameter, identify_constrained_generic_params};
 use crate::errors::InvalidReceiverTyHint;
 use crate::{errors, fluent_generated as fluent};
@@ -195,7 +194,9 @@ fn check_well_formed(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Result<(), ErrorGua
         hir::Node::TraitItem(item) => check_trait_item(tcx, item),
         hir::Node::ImplItem(item) => check_impl_item(tcx, item),
         hir::Node::ForeignItem(item) => check_foreign_item(tcx, item),
-        hir::Node::OpaqueTy(_) => Ok(crate::check::check::check_item_type(tcx, def_id)),
+        hir::Node::ConstBlock(_) | hir::Node::Expr(_) | hir::Node::OpaqueTy(_) => {
+            Ok(crate::check::check::check_item_type(tcx, def_id))
+        }
         _ => unreachable!("{node:?}"),
     };
 
@@ -229,7 +230,8 @@ fn check_item<'tcx>(tcx: TyCtxt<'tcx>, item: &'tcx hir::Item<'tcx>) -> Result<()
         ?item.owner_id,
         item.name = ? tcx.def_path_str(def_id)
     );
-    CollectItemTypesVisitor { tcx }.visit_item(item);
+    crate::collect::lower_item(tcx, item.item_id());
+    crate::collect::reject_placeholder_type_signatures_in_item(tcx, item);
 
     let res = match item.kind {
         // Right now we check that every default trait implementation
@@ -350,8 +352,6 @@ fn check_foreign_item<'tcx>(
 ) -> Result<(), ErrorGuaranteed> {
     let def_id = item.owner_id.def_id;
 
-    CollectItemTypesVisitor { tcx }.visit_foreign_item(item);
-
     debug!(
         ?item.owner_id,
         item.name = ? tcx.def_path_str(def_id)
@@ -374,7 +374,7 @@ fn check_trait_item<'tcx>(
 ) -> Result<(), ErrorGuaranteed> {
     let def_id = trait_item.owner_id.def_id;
 
-    CollectItemTypesVisitor { tcx }.visit_trait_item(trait_item);
+    crate::collect::lower_trait_item(tcx, trait_item.trait_item_id());
 
     let (method_sig, span) = match trait_item.kind {
         hir::TraitItemKind::Fn(ref sig, _) => (Some(sig), trait_item.span),
@@ -939,7 +939,7 @@ fn check_impl_item<'tcx>(
     tcx: TyCtxt<'tcx>,
     impl_item: &'tcx hir::ImplItem<'tcx>,
 ) -> Result<(), ErrorGuaranteed> {
-    CollectItemTypesVisitor { tcx }.visit_impl_item(impl_item);
+    crate::collect::lower_impl_item(tcx, impl_item.impl_item_id());
 
     let (method_sig, span) = match impl_item.kind {
         hir::ImplItemKind::Fn(ref sig, _) => (Some(sig), impl_item.span),
@@ -2411,6 +2411,7 @@ fn check_type_wf(tcx: TyCtxt<'_>, (): ()) -> Result<(), ErrorGuaranteed> {
         .and(
             items.par_foreign_items(|item| tcx.ensure_ok().check_well_formed(item.owner_id.def_id)),
         )
+        .and(items.par_nested_bodies(|item| tcx.ensure_ok().check_well_formed(item)))
         .and(items.par_opaques(|item| tcx.ensure_ok().check_well_formed(item)));
     super::entry::check_for_entry_fn(tcx);
 

compiler/rustc_hir_analysis/src/collect.rs

@@ -28,11 +28,10 @@ use rustc_errors::{
 };
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::{DefId, LocalDefId};
-use rustc_hir::intravisit::{self, InferKind, Visitor, VisitorExt, walk_generics};
+use rustc_hir::intravisit::{InferKind, Visitor, VisitorExt, walk_generics};
 use rustc_hir::{self as hir, GenericParamKind, HirId, Node, PreciseCapturingArgKind};
 use rustc_infer::infer::{InferCtxt, TyCtxtInferExt};
 use rustc_infer::traits::{DynCompatibilityViolation, ObligationCause};
-use rustc_middle::hir::nested_filter;
 use rustc_middle::query::Providers;
 use rustc_middle::ty::util::{Discr, IntTypeExt};
 use rustc_middle::ty::{self, AdtKind, Const, IsSuggestable, Ty, TyCtxt, TypingMode, fold_regions};
@@ -148,10 +147,6 @@ impl<'v> Visitor<'v> for HirPlaceholderCollector {
     }
 }
 
-pub(crate) struct CollectItemTypesVisitor<'tcx> {
-    pub tcx: TyCtxt<'tcx>,
-}
-
 /// If there are any placeholder types (`_`), emit an error explaining that this is not allowed
 /// and suggest adding type parameters in the appropriate place, taking into consideration any and
 /// all already existing generic type parameters to avoid suggesting a name that is already in use.
@@ -243,7 +238,7 @@ pub(crate) fn placeholder_type_error_diag<'cx, 'tcx>(
     err
 }
 
-fn reject_placeholder_type_signatures_in_item<'tcx>(
+pub(super) fn reject_placeholder_type_signatures_in_item<'tcx>(
     tcx: TyCtxt<'tcx>,
     item: &'tcx hir::Item<'tcx>,
 ) {
@@ -274,81 +269,6 @@ fn reject_placeholder_type_signatures_in_item<'tcx>(
     );
 }
 
-impl<'tcx> Visitor<'tcx> for CollectItemTypesVisitor<'tcx> {
-    type NestedFilter = nested_filter::OnlyBodies;
-
-    fn maybe_tcx(&mut self) -> Self::MaybeTyCtxt {
-        self.tcx
-    }
-
-    fn visit_item(&mut self, item: &'tcx hir::Item<'tcx>) {
-        lower_item(self.tcx, item.item_id());
-        reject_placeholder_type_signatures_in_item(self.tcx, item);
-        intravisit::walk_item(self, item);
-    }
-
-    fn visit_generics(&mut self, generics: &'tcx hir::Generics<'tcx>) {
-        for param in generics.params {
-            match param.kind {
-                hir::GenericParamKind::Lifetime { .. } => {}
-                hir::GenericParamKind::Type { default: Some(_), .. } => {
-                    self.tcx.ensure_ok().type_of(param.def_id);
-                }
-                hir::GenericParamKind::Type { .. } => {}
-                hir::GenericParamKind::Const { default, .. } => {
-                    self.tcx.ensure_ok().type_of(param.def_id);
-                    if let Some(default) = default {
-                        // need to store default and type of default
-                        self.tcx.ensure_ok().const_param_default(param.def_id);
-                        if let hir::ConstArgKind::Anon(ac) = default.kind {
-                            self.tcx.ensure_ok().type_of(ac.def_id);
-                        }
-                    }
-                }
-            }
-        }
-        intravisit::walk_generics(self, generics);
-    }
-
-    fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) {
-        if let hir::ExprKind::Closure(closure) = expr.kind {
-            self.tcx.ensure_ok().generics_of(closure.def_id);
-            self.tcx.ensure_ok().codegen_fn_attrs(closure.def_id);
-            // We do not call `type_of` for closures here as that
-            // depends on typecheck and would therefore hide
-            // any further errors in case one typeck fails.
-        }
-        intravisit::walk_expr(self, expr);
-    }
-
-    /// Don't call `type_of` on opaque types, since that depends on type checking function bodies.
-    /// `check_item_type` ensures that it's called instead.
-    fn visit_opaque_ty(&mut self, opaque: &'tcx hir::OpaqueTy<'tcx>) {
-        let def_id = opaque.def_id;
-        self.tcx.ensure_ok().generics_of(def_id);
-        self.tcx.ensure_ok().predicates_of(def_id);
-        self.tcx.ensure_ok().explicit_item_bounds(def_id);
-        self.tcx.ensure_ok().explicit_item_self_bounds(def_id);
-        self.tcx.ensure_ok().item_bounds(def_id);
-        self.tcx.ensure_ok().item_self_bounds(def_id);
-        if self.tcx.is_conditionally_const(def_id) {
-            self.tcx.ensure_ok().explicit_implied_const_bounds(def_id);
-            self.tcx.ensure_ok().const_conditions(def_id);
-        }
-        intravisit::walk_opaque_ty(self, opaque);
-    }
-
-    fn visit_trait_item(&mut self, trait_item: &'tcx hir::TraitItem<'tcx>) {
-        lower_trait_item(self.tcx, trait_item.trait_item_id());
-        intravisit::walk_trait_item(self, trait_item);
-    }
-
-    fn visit_impl_item(&mut self, impl_item: &'tcx hir::ImplItem<'tcx>) {
-        lower_impl_item(self.tcx, impl_item.impl_item_id());
-        intravisit::walk_impl_item(self, impl_item);
-    }
-}
-
 ///////////////////////////////////////////////////////////////////////////
 // Utility types and common code for the above passes.
 
@@ -669,7 +589,7 @@ fn get_new_lifetime_name<'tcx>(
 }
 
 #[instrument(level = "debug", skip_all)]
-fn lower_item(tcx: TyCtxt<'_>, item_id: hir::ItemId) {
+pub(super) fn lower_item(tcx: TyCtxt<'_>, item_id: hir::ItemId) {
     let it = tcx.hir_item(item_id);
     debug!(item = ?it.kind.ident(), id = %it.hir_id());
     let def_id = item_id.owner_id.def_id;
@@ -790,7 +710,7 @@ fn lower_item(tcx: TyCtxt<'_>, item_id: hir::ItemId) {
     }
 }
 
-fn lower_trait_item(tcx: TyCtxt<'_>, trait_item_id: hir::TraitItemId) {
+pub(crate) fn lower_trait_item(tcx: TyCtxt<'_>, trait_item_id: hir::TraitItemId) {
     let trait_item = tcx.hir_trait_item(trait_item_id);
     let def_id = trait_item_id.owner_id;
     tcx.ensure_ok().generics_of(def_id);
@@ -861,7 +781,7 @@ fn lower_trait_item(tcx: TyCtxt<'_>, trait_item_id: hir::TraitItemId) {
     tcx.ensure_ok().predicates_of(def_id);
 }
 
-fn lower_impl_item(tcx: TyCtxt<'_>, impl_item_id: hir::ImplItemId) {
+pub(super) fn lower_impl_item(tcx: TyCtxt<'_>, impl_item_id: hir::ImplItemId) {
     let def_id = impl_item_id.owner_id;
     tcx.ensure_ok().generics_of(def_id);
     tcx.ensure_ok().type_of(def_id);

compiler/rustc_middle/src/hir/mod.rs

@@ -71,6 +71,7 @@ impl ModuleItems {
         self.opaques.iter().copied()
     }
 
+    /// Closures and inline consts
     pub fn nested_bodies(&self) -> impl Iterator<Item = LocalDefId> {
         self.nested_bodies.iter().copied()
     }
@@ -79,6 +80,14 @@ impl ModuleItems {
         self.owners().map(|id| id.def_id)
     }
 
+    /// Closures and inline consts
+    pub fn par_nested_bodies(
+        &self,
+        f: impl Fn(LocalDefId) -> Result<(), ErrorGuaranteed> + DynSend + DynSync,
+    ) -> Result<(), ErrorGuaranteed> {
+        try_par_for_each_in(&self.nested_bodies[..], |&&id| f(id))
+    }
+
     pub fn par_items(
         &self,
         f: impl Fn(ItemId) -> Result<(), ErrorGuaranteed> + DynSend + DynSync,

tests/crashes/121429.rs

@@ -1,3 +1,11 @@
+error: unconstrained opaque type
+  --> $DIR/issue-109299-1.rs:10:10
+   |
+LL | type X = impl for<T> Fn() -> Lexer<T>::Cursor;
+   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `X` must be used in combination with a concrete type within the same crate
+
 error[E0220]: associated type `Cursor` not found for `Lexer<T>` in the current scope
   --> $DIR/issue-109299-1.rs:10:40
    |
@@ -23,14 +31,6 @@ LL | type X = impl for<T> Fn() -> Lexer<T>::Cursor;
            - `Lexer<i32>`
    = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
 
-error: unconstrained opaque type
-  --> $DIR/issue-109299-1.rs:10:10
-   |
-LL | type X = impl for<T> Fn() -> Lexer<T>::Cursor;
-   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = note: `X` must be used in combination with a concrete type within the same crate
-
 error: aborting due to 3 previous errors
 
 For more information about this error, try `rustc --explain E0220`.