resolve: Update NameBindingData::ambiguity in place

instead of creating fresh bindings, except in one case.

Author: petrochenkov

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -86,7 +86,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     ) {
         let binding = self.arenas.alloc_name_binding(NameBindingData {
             kind: NameBindingKind::Res(res),
-            ambiguity,
+            ambiguity: CmCell::new(ambiguity),
             // External ambiguities always report the `AMBIGUOUS_GLOB_IMPORTS` lint at the moment.
             warn_ambiguity: CmCell::new(true),
             vis: CmCell::new(vis),

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -100,7 +100,9 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
                 if let Some(node_id) = import.id() {
                     r.effective_visibilities.update_eff_vis(r.local_def_id(node_id), eff_vis, r.tcx)
                 }
-            } else if binding.ambiguity.is_some() && eff_vis.is_public_at_level(Level::Reexported) {
+            } else if binding.ambiguity.get().is_some()
+                && eff_vis.is_public_at_level(Level::Reexported)
+            {
                 exported_ambiguities.insert(*binding);
             }
         }
@@ -126,7 +128,7 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
             // leading to it into the table. They are used by the `ambiguous_glob_reexports`
             // lint. For all bindings added to the table this way `is_ambiguity` returns true.
             let is_ambiguity =
-                |binding: NameBinding<'ra>, warn: bool| binding.ambiguity.is_some() && !warn;
+                |binding: NameBinding<'ra>, warn: bool| binding.ambiguity.get().is_some() && !warn;
             let mut parent_id = ParentId::Def(module_id);
             let mut warn_ambiguity = binding.warn_ambiguity.get();
             while let NameBindingKind::Import { binding: nested_binding, .. } = binding.kind {

compiler/rustc_resolve/src/imports.rs

@@ -305,10 +305,10 @@ fn remove_same_import<'ra>(
     if let NameBindingKind::Import { import: import1, binding: b1_next } = b1.kind
         && let NameBindingKind::Import { import: import2, binding: b2_next } = b2.kind
         && import1 == import2
-        && b1.ambiguity == b2.ambiguity
+        && b1.warn_ambiguity.get() == b2.warn_ambiguity.get()
     {
-        assert!(b1.ambiguity.is_none());
-        assert_eq!(b1.warn_ambiguity.get(), b2.warn_ambiguity.get());
+        assert_eq!(b1.ambiguity.get(), b2.ambiguity.get());
+        assert!(!b1.warn_ambiguity.get());
         assert_eq!(b1.expansion, b2.expansion);
         assert_eq!(b1.span, b2.span);
         assert_eq!(b1.vis(), b2.vis());
@@ -344,7 +344,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
 
         self.arenas.alloc_name_binding(NameBindingData {
             kind: NameBindingKind::Import { binding, import },
-            ambiguity: None,
+            ambiguity: CmCell::new(None),
             warn_ambiguity: CmCell::new(false),
             span: import.span,
             vis: CmCell::new(vis),
@@ -368,9 +368,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         // all these overwrites will be re-fetched by glob imports importing
         // from that module without generating new ambiguities.
         // - A glob binding is overwritten by a non-glob binding arriving later.
-        // - A glob binding is overwritten by an ambiguous glob binding.
-        //   FIXME: avoid this by putting `NameBindingData::ambiguity` under a
-        //   cell and updating it in place.
+        // - A glob binding is overwritten by its clone after setting ambiguity in it.
+        //   FIXME: avoid this by removing `warn_ambiguity`, or by triggering glob re-fetch
+        //   with the same binding in some way.
         // - A glob binding is overwritten by a glob binding re-fetching an
         //   overwritten binding from other module (the recursive case).
         // Here we are detecting all such re-fetches and overwrite old bindings
@@ -381,14 +381,25 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         if deep_binding != glob_binding {
             // Some import layers have been removed, need to overwrite.
             assert_ne!(old_deep_binding, old_glob_binding);
-            assert_ne!(old_deep_binding, deep_binding);
-            assert!(old_deep_binding.is_glob_import());
+            // FIXME: reenable the asserts when `warn_ambiguity` is removed (#149195).
+            // assert_ne!(old_deep_binding, deep_binding);
+            // assert!(old_deep_binding.is_glob_import());
+            assert!(!deep_binding.is_glob_import());
             if glob_binding.is_ambiguity_recursive() {
                 glob_binding.warn_ambiguity.set_unchecked(true);
             }
             glob_binding
         } else if glob_binding.res() != old_glob_binding.res() {
-            self.new_ambiguity_binding(old_glob_binding, glob_binding, warn_ambiguity)
+            old_glob_binding.ambiguity.set_unchecked(Some(glob_binding));
+            old_glob_binding.warn_ambiguity.set_unchecked(warn_ambiguity);
+            if warn_ambiguity {
+                old_glob_binding
+            } else {
+                // Need a fresh binding so other glob imports importing it could re-fetch it
+                // and set their own `warn_ambiguity` to true.
+                // FIXME: remove this when `warn_ambiguity` is removed (#149195).
+                self.arenas.alloc_name_binding((*old_glob_binding).clone())
+            }
         } else if !old_glob_binding.vis().is_at_least(glob_binding.vis(), self.tcx) {
             // We are glob-importing the same item but with greater visibility.
             old_glob_binding.vis.set_unchecked(glob_binding.vis());
@@ -397,12 +408,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             && !old_glob_binding.is_ambiguity_recursive()
         {
             // Overwriting a non-ambiguous glob import with an ambiguous glob import.
-            self.new_ambiguity_binding(
-                AmbiguityKind::GlobVsGlob,
-                old_glob_binding,
-                glob_binding,
-                true,
-            )
+            old_glob_binding.ambiguity.set_unchecked(Some(glob_binding));
+            old_glob_binding.warn_ambiguity.set_unchecked(true);
+            old_glob_binding
         } else {
             old_glob_binding
         }
@@ -430,6 +438,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         self.update_local_resolution(module, key, warn_ambiguity, |this, resolution| {
             if let Some(old_binding) = resolution.best_binding() {
                 assert_ne!(binding, old_binding);
+                assert!(!binding.warn_ambiguity.get());
                 if res == Res::Err && old_binding.res() != Res::Err {
                     // Do not override real bindings with `Res::Err`s from error recovery.
                     return Ok(());
@@ -471,19 +480,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         })
     }
 
-    fn new_ambiguity_binding(
-        &self,
-        primary_binding: NameBinding<'ra>,
-        secondary_binding: NameBinding<'ra>,
-        warn_ambiguity: bool,
-    ) -> NameBinding<'ra> {
-        let ambiguity = Some(secondary_binding);
-        let warn_ambiguity = CmCell::new(warn_ambiguity);
-        let vis = primary_binding.vis.clone();
-        let data = NameBindingData { ambiguity, warn_ambiguity, vis, ..*primary_binding };
-        self.arenas.alloc_name_binding(data)
-    }
-
     // Use `f` to mutate the resolution of the name in the module.
     // If the resolution becomes a success, define it in the module's glob importers.
     fn update_local_resolution<T, F>(
@@ -690,7 +686,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                 let Some(binding) = resolution.best_binding() else { continue };
 
                 if let NameBindingKind::Import { import, .. } = binding.kind
-                    && let Some(amb_binding) = binding.ambiguity
+                    && let Some(amb_binding) = binding.ambiguity.get()
                     && binding.res() != Res::Err
                     && exported_ambiguities.contains(&binding)
                 {

compiler/rustc_resolve/src/lib.rs

@@ -804,10 +804,10 @@ impl<'ra> fmt::Debug for Module<'ra> {
 }
 
 /// Records a possibly-private value, type, or module definition.
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 struct NameBindingData<'ra> {
     kind: NameBindingKind<'ra>,
-    ambiguity: Option<NameBinding<'ra>>,
+    ambiguity: CmCell<Option<NameBinding<'ra>>>,
     /// Produce a warning instead of an error when reporting ambiguities inside this binding.
     /// May apply to indirect ambiguities under imports, so `ambiguity.is_some()` is not required.
     warn_ambiguity: CmCell<bool>,
@@ -942,7 +942,7 @@ impl<'ra> NameBindingData<'ra> {
     fn descent_to_ambiguity(
         self: NameBinding<'ra>,
     ) -> Option<(NameBinding<'ra>, NameBinding<'ra>)> {
-        match self.ambiguity {
+        match self.ambiguity.get() {
             Some(ambig_binding) => Some((self, ambig_binding)),
             None => match self.kind {
                 NameBindingKind::Import { binding, .. } => binding.descent_to_ambiguity(),
@@ -952,7 +952,7 @@ impl<'ra> NameBindingData<'ra> {
     }
 
     fn is_ambiguity_recursive(&self) -> bool {
-        self.ambiguity.is_some()
+        self.ambiguity.get().is_some()
             || match self.kind {
                 NameBindingKind::Import { binding, .. } => binding.is_ambiguity_recursive(),
                 _ => false,
@@ -1351,7 +1351,7 @@ impl<'ra> ResolverArenas<'ra> {
     ) -> NameBinding<'ra> {
         self.alloc_name_binding(NameBindingData {
             kind: NameBindingKind::Res(res),
-            ambiguity: None,
+            ambiguity: CmCell::new(None),
             warn_ambiguity: CmCell::new(false),
             vis: CmCell::new(vis),
             span,
@@ -2068,7 +2068,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         used: Used,
         warn_ambiguity: bool,
     ) {
-        if let Some(b2) = used_binding.ambiguity {
+        if let Some(b2) = used_binding.ambiguity.get() {
             let ambiguity_error = AmbiguityError {
                 kind: AmbiguityKind::GlobVsGlob,
                 ident,

tests/ui/imports/issue-55884-1.rs

@@ -17,5 +17,5 @@ mod m {
 
 fn main() {
     use m::S; //~ ERROR `S` is ambiguous
-    let s = S {};
+    let s = S {}; //~ ERROR `S` is ambiguous
 }

tests/ui/imports/issue-55884-1.stderr

@@ -18,6 +18,26 @@ LL |     pub use self::m2::*;
    |             ^^^^^^^^^^^
    = help: consider adding an explicit import of `S` to disambiguate
 
-error: aborting due to 1 previous error
+error[E0659]: `S` is ambiguous
+  --> $DIR/issue-55884-1.rs:20:13
+   |
+LL |     let s = S {};
+   |             ^ ambiguous name
+   |
+   = note: ambiguous because of multiple glob imports of a name in the same module
+note: `S` could refer to the struct imported here
+  --> $DIR/issue-55884-1.rs:14:13
+   |
+LL |     pub use self::m1::*;
+   |             ^^^^^^^^^^^
+   = help: consider adding an explicit import of `S` to disambiguate
+note: `S` could also refer to the struct imported here
+  --> $DIR/issue-55884-1.rs:15:13
+   |
+LL |     pub use self::m2::*;
+   |             ^^^^^^^^^^^
+   = help: consider adding an explicit import of `S` to disambiguate
+
+error: aborting due to 2 previous errors
 
 For more information about this error, try `rustc --explain E0659`.