Auto merge of #113408 - petrochenkov:bindintern2, r=cjgillot

resolve: Stop creating `NameBinding`s on every use, create them once per definition instead

`NameBinding` values are supposed to be unique, use referential equality, and be created once for every name declaration.

Before this PR many `NameBinding`s were created on name use, rather than on name declaration, because it's sufficiently cheap, and comparisons are not actually used in practice for some binding kinds.
This PR makes `NameBinding`s consistently unique and created on name declaration.

There are two special cases
- for extern prelude names creating `NameBinding` requires loading the corresponding crate, which is expensive, so such bindings are created lazily on first use, but they still keep the uniqueness by being reused on further uses.
- for legacy derive helpers (helper attributes written before derives that introduce them) the declaration and the use is basically the same thing (that's one of the reasons why they are deprecated), so they are still created on use, but we can still maybe do a bit better in a way that I described in FIXME in the last commit.

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -870,10 +870,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
         let imported_binding = self.r.import(binding, import);
         if parent == self.r.graph_root {
             if let Some(entry) = self.r.extern_prelude.get(&ident.normalize_to_macros_2_0()) {
-                if expansion != LocalExpnId::ROOT
-                    && orig_name.is_some()
-                    && entry.extern_crate_item.is_none()
-                {
+                if expansion != LocalExpnId::ROOT && orig_name.is_some() && !entry.is_import() {
                     let msg = "macro-expanded `extern crate` items cannot \
                                        shadow names passed with `--extern`";
                     self.r.tcx.sess.span_err(item.span, msg);
@@ -884,10 +881,14 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
                     return;
                 }
             }
-            let entry = self.r.extern_prelude.entry(ident.normalize_to_macros_2_0()).or_insert(
-                ExternPreludeEntry { extern_crate_item: None, introduced_by_item: true },
-            );
-            entry.extern_crate_item = Some(imported_binding);
+            let entry = self
+                .r
+                .extern_prelude
+                .entry(ident.normalize_to_macros_2_0())
+                .or_insert(ExternPreludeEntry { binding: None, introduced_by_item: true });
+            // Binding from `extern crate` item in source code can replace
+            // a binding from `--extern` on command line here.
+            entry.binding = Some(imported_binding);
             if orig_name.is_some() {
                 entry.introduced_by_item = true;
             }

compiler/rustc_resolve/src/diagnostics.rs

@@ -1032,7 +1032,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                 .get(&expn_id)
                                 .into_iter()
                                 .flatten()
-                                .map(|ident| TypoSuggestion::typo_from_ident(*ident, res)),
+                                .map(|(ident, _)| TypoSuggestion::typo_from_ident(*ident, res)),
                         );
                     }
                 }

compiler/rustc_resolve/src/ident.rs

@@ -1,7 +1,5 @@
 use rustc_ast::{self as ast, NodeId};
-use rustc_feature::is_builtin_attr_name;
 use rustc_hir::def::{DefKind, Namespace, NonMacroAttrKind, PartialRes, PerNS};
-use rustc_hir::PrimTy;
 use rustc_middle::bug;
 use rustc_middle::ty;
 use rustc_session::lint::builtin::PROC_MACRO_DERIVE_RESOLUTION_FALLBACK;
@@ -9,7 +7,7 @@ use rustc_session::lint::BuiltinLintDiagnostics;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::hygiene::{ExpnId, ExpnKind, LocalExpnId, MacroKind, SyntaxContext};
 use rustc_span::symbol::{kw, Ident};
-use rustc_span::{Span, DUMMY_SP};
+use rustc_span::Span;
 
 use crate::errors::{ParamKindInEnumDiscriminant, ParamKindInNonTrivialAnonConst};
 use crate::late::{
@@ -423,32 +421,22 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             orig_ident.span.ctxt(),
             |this, scope, use_prelude, ctxt| {
                 let ident = Ident::new(orig_ident.name, orig_ident.span.with_ctxt(ctxt));
-                let ok = |res, span, arenas| {
-                    Ok((
-                        (res, Visibility::Public, span, LocalExpnId::ROOT).to_name_binding(arenas),
-                        Flags::empty(),
-                    ))
-                };
                 let result = match scope {
                     Scope::DeriveHelpers(expn_id) => {
-                        if let Some(attr) = this
-                            .helper_attrs
-                            .get(&expn_id)
-                            .and_then(|attrs| attrs.iter().rfind(|i| ident == **i))
-                        {
-                            let binding = (
-                                Res::NonMacroAttr(NonMacroAttrKind::DeriveHelper),
-                                Visibility::Public,
-                                attr.span,
-                                expn_id,
-                            )
-                                .to_name_binding(this.arenas);
+                        if let Some(binding) = this.helper_attrs.get(&expn_id).and_then(|attrs| {
+                            attrs.iter().rfind(|(i, _)| ident == *i).map(|(_, binding)| *binding)
+                        }) {
                             Ok((binding, Flags::empty()))
                         } else {
                             Err(Determinacy::Determined)
                         }
                     }
                     Scope::DeriveHelpersCompat => {
+                        // FIXME: Try running this logic eariler, to allocate name bindings for
+                        // legacy derive helpers when creating an attribute invocation with
+                        // following derives. Legacy derive helpers are not common, so it shouldn't
+                        // affect performance. It should also allow to remove the `derives`
+                        // component from `ParentScope`.
                         let mut result = Err(Determinacy::Determined);
                         for derive in parent_scope.derives {
                             let parent_scope = &ParentScope { derives: &[], ..*parent_scope };
@@ -461,11 +449,14 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                             ) {
                                 Ok((Some(ext), _)) => {
                                     if ext.helper_attrs.contains(&ident.name) {
-                                        result = ok(
+                                        let binding = (
                                             Res::NonMacroAttr(NonMacroAttrKind::DeriveHelperCompat),
+                                            Visibility::Public,
                                             derive.span,
-                                            this.arenas,
-                                        );
+                                            LocalExpnId::ROOT,
+                                        )
+                                            .to_name_binding(this.arenas);
+                                        result = Ok((binding, Flags::empty()));
                                         break;
                                     }
                                 }
@@ -562,17 +553,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                             )),
                         }
                     }
-                    Scope::BuiltinAttrs => {
-                        if is_builtin_attr_name(ident.name) {
-                            ok(
-                                Res::NonMacroAttr(NonMacroAttrKind::Builtin(ident.name)),
-                                DUMMY_SP,
-                                this.arenas,
-                            )
-                        } else {
-                            Err(Determinacy::Determined)
-                        }
-                    }
+                    Scope::BuiltinAttrs => match this.builtin_attrs_bindings.get(&ident.name) {
+                        Some(binding) => Ok((*binding, Flags::empty())),
+                        None => Err(Determinacy::Determined),
+                    },
                     Scope::ExternPrelude => {
                         match this.extern_prelude_get(ident, finalize.is_some()) {
                             Some(binding) => Ok((binding, Flags::empty())),
@@ -581,8 +565,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                             )),
                         }
                     }
-                    Scope::ToolPrelude => match this.registered_tools.get(&ident).cloned() {
-                        Some(ident) => ok(Res::ToolMod, ident.span, this.arenas),
+                    Scope::ToolPrelude => match this.registered_tool_bindings.get(&ident) {
+                        Some(binding) => Ok((*binding, Flags::empty())),
                         None => Err(Determinacy::Determined),
                     },
                     Scope::StdLibPrelude => {
@@ -603,8 +587,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                         }
                         result
                     }
-                    Scope::BuiltinTypes => match PrimTy::from_name(ident.name) {
-                        Some(prim_ty) => ok(Res::PrimTy(prim_ty), DUMMY_SP, this.arenas),
+                    Scope::BuiltinTypes => match this.builtin_types_bindings.get(&ident.name) {
+                        Some(binding) => Ok((*binding, Flags::empty())),
                         None => Err(Determinacy::Determined),
                     },
                 };
@@ -842,9 +826,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                 if ns == TypeNS {
                     if ident.name == kw::Crate || ident.name == kw::DollarCrate {
                         let module = self.resolve_crate_root(ident);
-                        let binding = (module, Visibility::Public, module.span, LocalExpnId::ROOT)
-                            .to_name_binding(self.arenas);
-                        return Ok(binding);
+                        return Ok(self.module_self_bindings[&module]);
                     } else if ident.name == kw::Super || ident.name == kw::SelfLower {
                         // FIXME: Implement these with renaming requirements so that e.g.
                         // `use super;` doesn't work, but `use super as name;` does.

compiler/rustc_resolve/src/lib.rs

@@ -39,13 +39,15 @@ use rustc_errors::{
     Applicability, DiagnosticBuilder, DiagnosticMessage, ErrorGuaranteed, SubdiagnosticMessage,
 };
 use rustc_expand::base::{DeriveResolutions, SyntaxExtension, SyntaxExtensionKind};
+use rustc_feature::BUILTIN_ATTRIBUTES;
 use rustc_fluent_macro::fluent_messages;
 use rustc_hir::def::Namespace::{self, *};
+use rustc_hir::def::NonMacroAttrKind;
 use rustc_hir::def::{self, CtorOf, DefKind, DocLinkResMap, LifetimeRes, PartialRes, PerNS};
 use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, LocalDefIdMap, LocalDefIdSet};
 use rustc_hir::def_id::{CRATE_DEF_ID, LOCAL_CRATE};
 use rustc_hir::definitions::DefPathData;
-use rustc_hir::TraitCandidate;
+use rustc_hir::{PrimTy, TraitCandidate};
 use rustc_index::IndexVec;
 use rustc_metadata::creader::{CStore, CrateLoader};
 use rustc_middle::metadata::ModChild;
@@ -517,7 +519,7 @@ struct ModuleData<'a> {
 
 /// All modules are unique and allocated on a same arena,
 /// so we can use referential equality to compare them.
-#[derive(Clone, Copy, PartialEq)]
+#[derive(Clone, Copy, PartialEq, Eq, Hash)]
 #[rustc_pass_by_value]
 struct Module<'a>(Interned<'a, ModuleData<'a>>);
 
@@ -883,10 +885,16 @@ impl<'a> NameBindingData<'a> {
 
 #[derive(Default, Clone)]
 struct ExternPreludeEntry<'a> {
-    extern_crate_item: Option<NameBinding<'a>>,
+    binding: Option<NameBinding<'a>>,
     introduced_by_item: bool,
 }
 
+impl ExternPreludeEntry<'_> {
+    fn is_import(&self) -> bool {
+        self.binding.is_some_and(|binding| binding.is_import())
+    }
+}
+
 /// Used for better errors for E0773
 enum BuiltinMacroState {
     NotYetSeen(SyntaxExtensionKind),
@@ -996,6 +1004,12 @@ pub struct Resolver<'a, 'tcx> {
 
     arenas: &'a ResolverArenas<'a>,
     dummy_binding: NameBinding<'a>,
+    builtin_types_bindings: FxHashMap<Symbol, NameBinding<'a>>,
+    builtin_attrs_bindings: FxHashMap<Symbol, NameBinding<'a>>,
+    registered_tool_bindings: FxHashMap<Ident, NameBinding<'a>>,
+    /// Binding for implicitly declared names that come with a module,
+    /// like `self` (not yet used), or `crate`/`$crate` (for root modules).
+    module_self_bindings: FxHashMap<Module<'a>, NameBinding<'a>>,
 
     used_extern_options: FxHashSet<Symbol>,
     macro_names: FxHashSet<Ident>,
@@ -1033,7 +1047,7 @@ pub struct Resolver<'a, 'tcx> {
     /// `macro_rules` scopes produced by `macro_rules` item definitions.
     macro_rules_scopes: FxHashMap<LocalDefId, MacroRulesScopeRef<'a>>,
     /// Helper attributes that are in scope for the given expansion.
-    helper_attrs: FxHashMap<LocalExpnId, Vec<Ident>>,
+    helper_attrs: FxHashMap<LocalExpnId, Vec<(Ident, NameBinding<'a>)>>,
     /// Ready or in-progress results of resolving paths inside the `#[derive(...)]` attribute
     /// with the given `ExpnId`.
     derive_data: FxHashMap<LocalExpnId, DeriveData>,
@@ -1111,6 +1125,7 @@ impl<'a> ResolverArenas<'a> {
         span: Span,
         no_implicit_prelude: bool,
         module_map: &mut FxHashMap<DefId, Module<'a>>,
+        module_self_bindings: &mut FxHashMap<Module<'a>, NameBinding<'a>>,
     ) -> Module<'a> {
         let module = Module(Interned::new_unchecked(self.modules.alloc(ModuleData::new(
             parent,
@@ -1125,6 +1140,9 @@ impl<'a> ResolverArenas<'a> {
         }
         if let Some(def_id) = def_id {
             module_map.insert(def_id, module);
+            let vis = ty::Visibility::<DefId>::Public;
+            let binding = (module, vis, module.span, LocalExpnId::ROOT).to_name_binding(self);
+            module_self_bindings.insert(module, binding);
         }
         module
     }
@@ -1236,6 +1254,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
     ) -> Resolver<'a, 'tcx> {
         let root_def_id = CRATE_DEF_ID.to_def_id();
         let mut module_map = FxHashMap::default();
+        let mut module_self_bindings = FxHashMap::default();
         let graph_root = arenas.new_module(
             None,
             ModuleKind::Def(DefKind::Mod, root_def_id, kw::Empty),
@@ -1243,6 +1262,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             crate_span,
             attr::contains_name(attrs, sym::no_implicit_prelude),
             &mut module_map,
+            &mut module_self_bindings,
         );
         let empty_module = arenas.new_module(
             None,
@@ -1251,6 +1271,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             DUMMY_SP,
             true,
             &mut FxHashMap::default(),
+            &mut FxHashMap::default(),
         );
 
         let mut visibilities = FxHashMap::default();
@@ -1283,6 +1304,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         let registered_tools = tcx.registered_tools(());
 
         let features = tcx.features();
+        let pub_vis = ty::Visibility::<DefId>::Public;
 
         let mut resolver = Resolver {
             tcx,
@@ -1330,14 +1352,33 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             macro_expanded_macro_export_errors: BTreeSet::new(),
 
             arenas,
-            dummy_binding: arenas.alloc_name_binding(NameBindingData {
-                kind: NameBindingKind::Res(Res::Err),
-                ambiguity: None,
-                warn_ambiguity: false,
-                expansion: LocalExpnId::ROOT,
-                span: DUMMY_SP,
-                vis: ty::Visibility::Public,
-            }),
+            dummy_binding: (Res::Err, pub_vis, DUMMY_SP, LocalExpnId::ROOT).to_name_binding(arenas),
+            builtin_types_bindings: PrimTy::ALL
+                .iter()
+                .map(|prim_ty| {
+                    let binding = (Res::PrimTy(*prim_ty), pub_vis, DUMMY_SP, LocalExpnId::ROOT)
+                        .to_name_binding(arenas);
+                    (prim_ty.name(), binding)
+                })
+                .collect(),
+            builtin_attrs_bindings: BUILTIN_ATTRIBUTES
+                .iter()
+                .map(|builtin_attr| {
+                    let res = Res::NonMacroAttr(NonMacroAttrKind::Builtin(builtin_attr.name));
+                    let binding =
+                        (res, pub_vis, DUMMY_SP, LocalExpnId::ROOT).to_name_binding(arenas);
+                    (builtin_attr.name, binding)
+                })
+                .collect(),
+            registered_tool_bindings: registered_tools
+                .iter()
+                .map(|ident| {
+                    let binding = (Res::ToolMod, pub_vis, ident.span, LocalExpnId::ROOT)
+                        .to_name_binding(arenas);
+                    (*ident, binding)
+                })
+                .collect(),
+            module_self_bindings,
 
             used_extern_options: Default::default(),
             macro_names: FxHashSet::default(),
@@ -1407,7 +1448,16 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         no_implicit_prelude: bool,
     ) -> Module<'a> {
         let module_map = &mut self.module_map;
-        self.arenas.new_module(parent, kind, expn_id, span, no_implicit_prelude, module_map)
+        let module_self_bindings = &mut self.module_self_bindings;
+        self.arenas.new_module(
+            parent,
+            kind,
+            expn_id,
+            span,
+            no_implicit_prelude,
+            module_map,
+            module_self_bindings,
+        )
     }
 
     fn next_node_id(&mut self) -> NodeId {
@@ -1727,7 +1777,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             // but not introduce it, as used if they are accessed from lexical scope.
             if is_lexical_scope {
                 if let Some(entry) = self.extern_prelude.get(&ident.normalize_to_macros_2_0()) {
-                    if !entry.introduced_by_item && entry.extern_crate_item == Some(used_binding) {
+                    if !entry.introduced_by_item && entry.binding == Some(used_binding) {
                         return;
                     }
                 }
@@ -1885,12 +1935,18 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             // Make sure `self`, `super` etc produce an error when passed to here.
             return None;
         }
-        self.extern_prelude.get(&ident.normalize_to_macros_2_0()).cloned().and_then(|entry| {
-            if let Some(binding) = entry.extern_crate_item {
-                if finalize && entry.introduced_by_item {
-                    self.record_use(ident, binding, false);
+
+        let norm_ident = ident.normalize_to_macros_2_0();
+        let binding = self.extern_prelude.get(&norm_ident).cloned().and_then(|entry| {
+            Some(if let Some(binding) = entry.binding {
+                if finalize {
+                    if !entry.is_import() {
+                        self.crate_loader(|c| c.process_path_extern(ident.name, ident.span));
+                    } else if entry.introduced_by_item {
+                        self.record_use(ident, binding, false);
+                    }
                 }
-                Some(binding)
+                binding
             } else {
                 let crate_id = if finalize {
                     let Some(crate_id) =
@@ -1903,10 +1959,16 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                     self.crate_loader(|c| c.maybe_process_path_extern(ident.name))?
                 };
                 let crate_root = self.expect_module(crate_id.as_def_id());
-                let vis = ty::Visibility::<LocalDefId>::Public;
-                Some((crate_root, vis, DUMMY_SP, LocalExpnId::ROOT).to_name_binding(self.arenas))
-            }
-        })
+                let vis = ty::Visibility::<DefId>::Public;
+                (crate_root, vis, DUMMY_SP, LocalExpnId::ROOT).to_name_binding(self.arenas)
+            })
+        });
+
+        if let Some(entry) = self.extern_prelude.get_mut(&norm_ident) {
+            entry.binding = binding;
+        }
+
+        binding
     }
 
     /// Rustdoc uses this to resolve doc link paths in a recoverable way. `PathResult<'a>`

compiler/rustc_resolve/src/macros.rs

@@ -7,7 +7,7 @@ use crate::errors::{
 use crate::Namespace::*;
 use crate::{BuiltinMacroState, Determinacy};
 use crate::{DeriveData, Finalize, ParentScope, ResolutionError, Resolver, ScopeSet};
-use crate::{ModuleKind, ModuleOrUniformRoot, NameBinding, PathResult, Segment};
+use crate::{ModuleKind, ModuleOrUniformRoot, NameBinding, PathResult, Segment, ToNameBinding};
 use rustc_ast::expand::StrippedCfgItem;
 use rustc_ast::{self as ast, attr, Crate, Inline, ItemKind, ModKind, NodeId};
 use rustc_ast_pretty::pprust;
@@ -20,10 +20,10 @@ use rustc_expand::base::{SyntaxExtension, SyntaxExtensionKind};
 use rustc_expand::compile_declarative_macro;
 use rustc_expand::expand::{AstFragment, Invocation, InvocationKind, SupportsMacroExpansion};
 use rustc_hir::def::{self, DefKind, NonMacroAttrKind};
-use rustc_hir::def_id::{CrateNum, LocalDefId};
+use rustc_hir::def_id::{CrateNum, DefId, LocalDefId};
 use rustc_middle::middle::stability;
 use rustc_middle::ty::RegisteredTools;
-use rustc_middle::ty::TyCtxt;
+use rustc_middle::ty::{TyCtxt, Visibility};
 use rustc_session::lint::builtin::{
     LEGACY_DERIVE_HELPERS, SOFT_UNSTABLE, UNKNOWN_DIAGNOSTIC_ATTRIBUTES,
 };
@@ -401,8 +401,17 @@ impl<'a, 'tcx> ResolverExpand for Resolver<'a, 'tcx> {
         }
         // Sort helpers in a stable way independent from the derive resolution order.
         entry.helper_attrs.sort_by_key(|(i, _)| *i);
-        self.helper_attrs
-            .insert(expn_id, entry.helper_attrs.iter().map(|(_, ident)| *ident).collect());
+        let helper_attrs = entry
+            .helper_attrs
+            .iter()
+            .map(|(_, ident)| {
+                let res = Res::NonMacroAttr(NonMacroAttrKind::DeriveHelper);
+                let binding = (res, Visibility::<DefId>::Public, ident.span, expn_id)
+                    .to_name_binding(self.arenas);
+                (*ident, binding)
+            })
+            .collect();
+        self.helper_attrs.insert(expn_id, helper_attrs);
         // Mark this derive as having `Copy` either if it has `Copy` itself or if its parent derive
         // has `Copy`, to support cases like `#[derive(Clone, Copy)] #[derive(Debug)]`.
         if entry.has_derive_copy || self.has_derive_copy(parent_scope.expansion) {