Auto merge of #144272 - petrochenkov:disambunder2, r=oli-obk

resolve: Make disambiguators for underscore bindings module-local (take 2)

The difference with #144013 can be seen in the second commit.

Now we just keep a separate disambiguator counter in every `Module`, instead of a global counter in `Resolver`.
This will be ok for parallel import resolution because we'll need to lock the module anyway when updating `resolutions` and other fields in it.
And for external modules the disabmiguator could be just passed as an argument to `define_extern`, without using any cells or locks, once #143884 lands.

Unblocks #143884.

Author: bors

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -53,8 +53,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         ns: Namespace,
         binding: NameBinding<'ra>,
     ) {
-        let key = self.new_disambiguated_key(ident, ns);
-        if let Err(old_binding) = self.try_define(parent, key, binding, false) {
+        if let Err(old_binding) = self.try_define(parent, ident, ns, binding, false) {
             self.report_conflict(parent, ident, ns, old_binding, binding);
         }
     }
@@ -446,16 +445,18 @@ impl<'a, 'ra, 'tcx> BuildReducedGraphVisitor<'a, 'ra, 'tcx> {
 
         self.r.indeterminate_imports.push(import);
         match import.kind {
-            // Don't add unresolved underscore imports to modules
-            ImportKind::Single { target: Ident { name: kw::Underscore, .. }, .. } => {}
             ImportKind::Single { target, type_ns_only, .. } => {
-                self.r.per_ns(|this, ns| {
-                    if !type_ns_only || ns == TypeNS {
-                        let key = BindingKey::new(target, ns);
-                        let mut resolution = this.resolution(current_module, key).borrow_mut();
-                        resolution.single_imports.insert(import);
-                    }
-                });
+                // Don't add underscore imports to `single_imports`
+                // because they cannot define any usable names.
+                if target.name != kw::Underscore {
+                    self.r.per_ns(|this, ns| {
+                        if !type_ns_only || ns == TypeNS {
+                            let key = BindingKey::new(target, ns);
+                            let mut resolution = this.resolution(current_module, key).borrow_mut();
+                            resolution.single_imports.insert(import);
+                        }
+                    });
+                }
             }
             // We don't add prelude imports to the globs since they only affect lexical scopes,
             // which are not relevant to import resolution.
@@ -1401,9 +1402,12 @@ impl<'a, 'ra, 'tcx> Visitor<'a> for BuildReducedGraphVisitor<'a, 'ra, 'tcx> {
             let parent = self.parent_scope.module;
             let expansion = self.parent_scope.expansion;
             self.r.define(parent, ident, ns, self.res(def_id), vis, item.span, expansion);
-        } else if !matches!(&item.kind, AssocItemKind::Delegation(deleg) if deleg.from_glob) {
+        } else if !matches!(&item.kind, AssocItemKind::Delegation(deleg) if deleg.from_glob)
+            && ident.name != kw::Underscore
+        {
+            // Don't add underscore names, they cannot be looked up anyway.
             let impl_def_id = self.r.tcx.local_parent(local_def_id);
-            let key = BindingKey::new(ident.normalize_to_macros_2_0(), ns);
+            let key = BindingKey::new(ident, ns);
             self.r.impl_binding_keys.entry(impl_def_id).or_default().insert(key);
         }
 

compiler/rustc_resolve/src/imports.rs

@@ -25,7 +25,7 @@ use rustc_span::{Ident, Span, Symbol, kw, sym};
 use smallvec::SmallVec;
 use tracing::debug;
 
-use crate::Namespace::*;
+use crate::Namespace::{self, *};
 use crate::diagnostics::{DiagMode, Suggestion, import_candidates};
 use crate::errors::{
     CannotBeReexportedCratePublic, CannotBeReexportedCratePublicNS, CannotBeReexportedPrivate,
@@ -338,13 +338,21 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     pub(crate) fn try_define(
         &mut self,
         module: Module<'ra>,
-        key: BindingKey,
+        ident: Ident,
+        ns: Namespace,
         binding: NameBinding<'ra>,
         warn_ambiguity: bool,
     ) -> Result<(), NameBinding<'ra>> {
         let res = binding.res();
-        self.check_reserved_macro_name(key.ident, res);
+        self.check_reserved_macro_name(ident, res);
         self.set_binding_parent_module(binding, module);
+        // Even if underscore names cannot be looked up, we still need to add them to modules,
+        // because they can be fetched by glob imports from those modules, and bring traits
+        // into scope both directly and through glob imports.
+        let key = BindingKey::new_disambiguated(ident, ns, || {
+            module.underscore_disambiguator.update(|d| d + 1);
+            module.underscore_disambiguator.get()
+        });
         self.update_resolution(module, key, warn_ambiguity, |this, resolution| {
             if let Some(old_binding) = resolution.best_binding() {
                 if res == Res::Err && old_binding.res() != Res::Err {
@@ -383,7 +391,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     (old_glob @ true, false) | (old_glob @ false, true) => {
                         let (glob_binding, non_glob_binding) =
                             if old_glob { (old_binding, binding) } else { (binding, old_binding) };
-                        if key.ns == MacroNS
+                        if ns == MacroNS
                             && non_glob_binding.expansion != LocalExpnId::ROOT
                             && glob_binding.res() != non_glob_binding.res()
                         {
@@ -489,10 +497,10 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             };
             if self.is_accessible_from(binding.vis, scope) {
                 let imported_binding = self.import(binding, *import);
-                let key = BindingKey { ident, ..key };
                 let _ = self.try_define(
                     import.parent_scope.module,
-                    key,
+                    ident,
+                    key.ns,
                     imported_binding,
                     warn_ambiguity,
                 );
@@ -514,11 +522,15 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             let dummy_binding = self.dummy_binding;
             let dummy_binding = self.import(dummy_binding, import);
             self.per_ns(|this, ns| {
-                let key = BindingKey::new(target, ns);
-                let _ = this.try_define(import.parent_scope.module, key, dummy_binding, false);
-                this.update_resolution(import.parent_scope.module, key, false, |_, resolution| {
-                    resolution.single_imports.swap_remove(&import);
-                })
+                let module = import.parent_scope.module;
+                let _ = this.try_define(module, target, ns, dummy_binding, false);
+                // Don't remove underscores from `single_imports`, they were never added.
+                if target.name != kw::Underscore {
+                    let key = BindingKey::new(target, ns);
+                    this.update_resolution(module, key, false, |_, resolution| {
+                        resolution.single_imports.swap_remove(&import);
+                    })
+                }
             });
             self.record_use(target, dummy_binding, Used::Other);
         } else if import.imported_module.get().is_none() {
@@ -895,7 +907,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                         PendingBinding::Ready(Some(imported_binding))
                     }
                     Err(Determinacy::Determined) => {
-                        // Don't update the resolution for underscores, because it was never added.
+                        // Don't remove underscores from `single_imports`, they were never added.
                         if target.name != kw::Underscore {
                             let key = BindingKey::new(target, ns);
                             this.update_resolution(parent, key, false, |_, resolution| {
@@ -1510,7 +1522,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     .is_some_and(|binding| binding.warn_ambiguity_recursive());
                 let _ = self.try_define(
                     import.parent_scope.module,
-                    key,
+                    key.ident,
+                    key.ns,
                     imported_binding,
                     warn_ambiguity,
                 );

compiler/rustc_resolve/src/lib.rs

@@ -532,15 +532,26 @@ struct BindingKey {
     /// identifier.
     ident: Ident,
     ns: Namespace,
-    /// 0 if ident is not `_`, otherwise a value that's unique to the specific
-    /// `_` in the expanded AST that introduced this binding.
+    /// When we add an underscore binding (with ident `_`) to some module, this field has
+    /// a non-zero value that uniquely identifies this binding in that module.
+    /// For non-underscore bindings this field is zero.
+    /// When a key is constructed for name lookup (as opposed to name definition), this field is
+    /// also zero, even for underscore names, so for underscores the lookup will never succeed.
     disambiguator: u32,
 }
 
 impl BindingKey {
     fn new(ident: Ident, ns: Namespace) -> Self {
-        let ident = ident.normalize_to_macros_2_0();
-        BindingKey { ident, ns, disambiguator: 0 }
+        BindingKey { ident: ident.normalize_to_macros_2_0(), ns, disambiguator: 0 }
+    }
+
+    fn new_disambiguated(
+        ident: Ident,
+        ns: Namespace,
+        disambiguator: impl FnOnce() -> u32,
+    ) -> BindingKey {
+        let disambiguator = if ident.name == kw::Underscore { disambiguator() } else { 0 };
+        BindingKey { ident: ident.normalize_to_macros_2_0(), ns, disambiguator }
     }
 }
 
@@ -568,6 +579,8 @@ struct ModuleData<'ra> {
     lazy_resolutions: Resolutions<'ra>,
     /// True if this is a module from other crate that needs to be populated on access.
     populate_on_access: Cell<bool>,
+    /// Used to disambiguate underscore items (`const _: T = ...`) in the module.
+    underscore_disambiguator: Cell<u32>,
 
     /// Macro invocations that can expand into items in this module.
     unexpanded_invocations: RefCell<FxHashSet<LocalExpnId>>,
@@ -628,6 +641,7 @@ impl<'ra> ModuleData<'ra> {
             kind,
             lazy_resolutions: Default::default(),
             populate_on_access: Cell::new(is_foreign),
+            underscore_disambiguator: Cell::new(0),
             unexpanded_invocations: Default::default(),
             no_implicit_prelude,
             glob_importers: RefCell::new(Vec::new()),
@@ -1087,8 +1101,6 @@ pub struct Resolver<'ra, 'tcx> {
     extern_module_map: RefCell<FxIndexMap<DefId, Module<'ra>>>,
     binding_parent_modules: FxHashMap<NameBinding<'ra>, Module<'ra>>,
 
-    underscore_disambiguator: u32,
-
     /// Maps glob imports to the names of items actually imported.
     glob_map: FxIndexMap<LocalDefId, FxIndexSet<Symbol>>,
     glob_error: Option<ErrorGuaranteed>,
@@ -1500,7 +1512,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             extern_crate_map: Default::default(),
             module_children: Default::default(),
             trait_map: NodeMap::default(),
-            underscore_disambiguator: 0,
             empty_module,
             local_module_map,
             extern_module_map: Default::default(),
@@ -1881,17 +1892,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         import_ids
     }
 
-    fn new_disambiguated_key(&mut self, ident: Ident, ns: Namespace) -> BindingKey {
-        let ident = ident.normalize_to_macros_2_0();
-        let disambiguator = if ident.name == kw::Underscore {
-            self.underscore_disambiguator += 1;
-            self.underscore_disambiguator
-        } else {
-            0
-        };
-        BindingKey { ident, ns, disambiguator }
-    }
-
     fn resolutions(&mut self, module: Module<'ra>) -> &'ra Resolutions<'ra> {
         if module.populate_on_access.get() {
             module.populate_on_access.set(false);

compiler/rustc_resolve/src/macros.rs

@@ -535,7 +535,7 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
         target_trait.for_each_child(self, |this, ident, ns, _binding| {
             // FIXME: Adjust hygiene for idents from globs, like for glob imports.
             if let Some(overriding_keys) = this.impl_binding_keys.get(&impl_def_id)
-                && overriding_keys.contains(&BindingKey::new(ident.normalize_to_macros_2_0(), ns))
+                && overriding_keys.contains(&BindingKey::new(ident, ns))
             {
                 // The name is overridden, do not produce it from the glob delegation.
             } else {