resolve: Partially convert ambiguous_glob_imports lint into a hard error

Author: petrochenkov

compiler/rustc_codegen_gcc/build_system/src/test.rs

@@ -826,8 +826,6 @@ fn valid_ui_error_pattern_test(file: &str) -> bool {
         "type-alias-impl-trait/auxiliary/cross_crate_ice.rs",
         "type-alias-impl-trait/auxiliary/cross_crate_ice2.rs",
         "macros/rfc-2011-nicer-assert-messages/auxiliary/common.rs",
-        "imports/ambiguous-1.rs",
-        "imports/ambiguous-4-extern.rs",
         "entry-point/auxiliary/bad_main_functions.rs",
     ]
     .iter()

compiler/rustc_lint_defs/src/builtin.rs

@@ -4436,7 +4436,7 @@ declare_lint! {
     ///
     /// ### Example
     ///
-    /// ```rust,compile_fail
+    /// ```rust,ignore (needs extern crate)
     /// #![deny(ambiguous_glob_imports)]
     /// pub fn foo() -> u32 {
     ///     use sub::*;
@@ -4452,8 +4452,6 @@ declare_lint! {
     /// }
     /// ```
     ///
-    /// {{produces}}
-    ///
     /// ### Explanation
     ///
     /// Previous versions of Rust compile it successfully because it

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -53,7 +53,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         ns: Namespace,
         binding: NameBinding<'ra>,
     ) {
-        if let Err(old_binding) = self.try_define_local(parent, ident, ns, binding, false) {
+        if let Err(old_binding) = self.try_define_local(parent, ident, ns, binding) {
             self.report_conflict(parent, ident, ns, old_binding, binding);
         }
     }
@@ -82,13 +82,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         vis: Visibility<DefId>,
         span: Span,
         expansion: LocalExpnId,
-        ambiguity: Option<(NameBinding<'ra>, AmbiguityKind)>,
+        ambiguity: Option<(NameBinding<'ra>, AmbiguityKind, bool)>,
     ) {
         let binding = self.arenas.alloc_name_binding(NameBindingData {
             kind: NameBindingKind::Res(res),
             ambiguity,
-            // External ambiguities always report the `AMBIGUOUS_GLOB_IMPORTS` lint at the moment.
-            warn_ambiguity: true,
             vis,
             span,
             expansion,
@@ -288,7 +286,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                 AmbigModChildKind::GlobVsGlob => AmbiguityKind::GlobVsGlob,
                 AmbigModChildKind::GlobVsExpanded => AmbiguityKind::GlobVsExpanded,
             };
-            (self.arenas.new_res_binding(res, vis, span, expansion), ambig_kind)
+            // External ambiguities always report the `AMBIGUOUS_GLOB_IMPORTS` lint at the moment.
+            (self.arenas.new_res_binding(res, vis, span, expansion), ambig_kind, true)
         });
 
         // Record primary definitions.

compiler/rustc_resolve/src/diagnostics.rs

@@ -1995,6 +1995,25 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         }
     }
 
+    pub(crate) fn remove_same_import(
+        b1: NameBinding<'ra>,
+        b2: NameBinding<'ra>,
+    ) -> (NameBinding<'ra>, NameBinding<'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
+        {
+            assert!(b1.ambiguity.is_none());
+            assert_eq!(b1.expansion, b2.expansion);
+            assert_eq!(b1.span, b2.span);
+            assert_eq!(b1.vis, b2.vis);
+            Self::remove_same_import(b1_next, b2_next)
+        } else {
+            (b1, b2)
+        }
+    }
+
     fn ambiguity_diagnostic(&self, ambiguity_error: &AmbiguityError<'ra>) -> errors::Ambiguity {
         let AmbiguityError { kind, ident, b1, b2, misc1, misc2, .. } = *ambiguity_error;
         let extern_prelude_ambiguity = || {
@@ -2003,6 +2022,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     && entry.flag_binding.as_ref().and_then(|pb| pb.get().0.binding()) == Some(b2)
             })
         };
+        let (b1, b2) = Self::remove_same_import(b1, b2);
         let (b1, b2, misc1, misc2, swapped) = if b2.span.is_dummy() && !b1.span.is_dummy() {
             // We have to print the span-less alternative first, otherwise formatting looks bad.
             (b2, b1, misc2, misc1, true)

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -125,27 +125,13 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
             // If the binding is ambiguous, put the root ambiguity binding and all reexports
             // 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;
             let mut parent_id = ParentId::Def(module_id);
-            let mut warn_ambiguity = binding.warn_ambiguity;
             while let NameBindingKind::Import { binding: nested_binding, .. } = binding.kind {
                 self.update_import(binding, parent_id);
-
-                if is_ambiguity(binding, warn_ambiguity) {
-                    // Stop at the root ambiguity, further bindings in the chain should not
-                    // be reexported because the root ambiguity blocks any access to them.
-                    // (Those further bindings are most likely not ambiguities themselves.)
-                    break;
-                }
-
                 parent_id = ParentId::Import(binding);
                 binding = nested_binding;
-                warn_ambiguity |= nested_binding.warn_ambiguity;
             }
-            if !is_ambiguity(binding, warn_ambiguity)
-                && let Some(def_id) = binding.res().opt_def_id().and_then(|id| id.as_local())
-            {
+            if let Some(def_id) = binding.res().opt_def_id().and_then(|id| id.as_local()) {
                 self.update_def(def_id, binding.vis.expect_local(), parent_id);
             }
         }

compiler/rustc_resolve/src/imports.rs

@@ -325,21 +325,68 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         self.arenas.alloc_name_binding(NameBindingData {
             kind: NameBindingKind::Import { binding, import },
             ambiguity: None,
-            warn_ambiguity: false,
             span: import.span,
             vis,
             expansion: import.parent_scope.expansion,
         })
     }
 
+    fn select_glob_binding(
+        &self,
+        glob_binding: NameBinding<'ra>,
+        old_glob_binding: NameBinding<'ra>,
+    ) -> NameBinding<'ra> {
+        assert!(glob_binding.is_glob_import());
+        assert!(old_glob_binding.is_glob_import());
+        assert_ne!(glob_binding, old_glob_binding);
+        // `best_binding` with a given key in a module may be overwritten in a
+        // number of cases (all of them can be seen below in this `match`),
+        // 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 a glob binding with larger visibility.
+        //   FIXME: avoid this by putting `NameBindingData::vis` under a cell
+        //   and updating it in place.
+        // - 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
+        // with the re-fetched bindings.
+        let (deep_binding, old_deep_binding) =
+            Self::remove_same_import(glob_binding, old_glob_binding);
+        if deep_binding != glob_binding {
+            assert_ne!(old_deep_binding, old_glob_binding);
+            assert_ne!(old_deep_binding, deep_binding);
+            assert!(old_deep_binding.is_glob_import());
+            assert!(old_deep_binding.ambiguity.is_none());
+            glob_binding
+        } else if glob_binding.res() != old_glob_binding.res() {
+            self.new_ambiguity_binding(
+                AmbiguityKind::GlobVsGlob,
+                old_glob_binding,
+                glob_binding,
+                false,
+            )
+        } else if !old_glob_binding.vis.is_at_least(glob_binding.vis, self.tcx)
+            || glob_binding.is_ambiguity_recursive()
+        {
+            // We are glob-importing the same item but with greater visibility,
+            // or overwriting with an ambiguous glob import.
+            glob_binding
+        } else {
+            old_glob_binding
+        }
+    }
+
     /// Define the name or return the existing binding if there is a collision.
     pub(crate) fn try_define_local(
         &mut self,
         module: Module<'ra>,
         ident: Ident,
         ns: Namespace,
         binding: NameBinding<'ra>,
-        warn_ambiguity: bool,
     ) -> Result<(), NameBinding<'ra>> {
         let res = binding.res();
         self.check_reserved_macro_name(ident, res);
@@ -351,40 +398,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             module.underscore_disambiguator.update_unchecked(|d| d + 1);
             module.underscore_disambiguator.get()
         });
-        self.update_local_resolution(module, key, warn_ambiguity, |this, resolution| {
+        self.update_local_resolution(module, key, |this, resolution| {
             if let Some(old_binding) = resolution.best_binding() {
+                assert_ne!(binding, old_binding);
                 if res == Res::Err && old_binding.res() != Res::Err {
                     // Do not override real bindings with `Res::Err`s from error recovery.
                     return Ok(());
                 }
                 match (old_binding.is_glob_import(), binding.is_glob_import()) {
                     (true, true) => {
-                        let (glob_binding, old_glob_binding) = (binding, old_binding);
-                        // FIXME: remove `!binding.is_ambiguity_recursive()` after delete the warning ambiguity.
-                        if !binding.is_ambiguity_recursive()
-                            && let NameBindingKind::Import { import: old_import, .. } =
-                                old_glob_binding.kind
-                            && let NameBindingKind::Import { import, .. } = glob_binding.kind
-                            && old_import == import
-                        {
-                            // When imported from the same glob-import statement, we should replace
-                            // `old_glob_binding` with `glob_binding`, regardless of whether
-                            // they have the same resolution or not.
-                            resolution.glob_binding = Some(glob_binding);
-                        } else if res != old_glob_binding.res() {
-                            resolution.glob_binding = Some(this.new_ambiguity_binding(
-                                AmbiguityKind::GlobVsGlob,
-                                old_glob_binding,
-                                glob_binding,
-                                warn_ambiguity,
-                            ));
-                        } else if !old_binding.vis.is_at_least(binding.vis, this.tcx) {
-                            // We are glob-importing the same item but with greater visibility.
-                            resolution.glob_binding = Some(glob_binding);
-                        } else if binding.is_ambiguity_recursive() {
-                            resolution.glob_binding =
-                                Some(this.new_warn_ambiguity_binding(glob_binding));
-                        }
+                        resolution.glob_binding =
+                            Some(this.select_glob_binding(binding, old_binding))
                     }
                     (old_glob @ true, false) | (old_glob @ false, true) => {
                         let (glob_binding, non_glob_binding) =
@@ -403,18 +427,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                             resolution.non_glob_binding = Some(non_glob_binding);
                         }
 
-                        if let Some(old_glob_binding) = resolution.glob_binding {
-                            assert!(old_glob_binding.is_glob_import());
-                            if glob_binding.res() != old_glob_binding.res() {
-                                resolution.glob_binding = Some(this.new_ambiguity_binding(
-                                    AmbiguityKind::GlobVsGlob,
-                                    old_glob_binding,
-                                    glob_binding,
-                                    false,
-                                ));
-                            } else if !old_glob_binding.vis.is_at_least(binding.vis, this.tcx) {
-                                resolution.glob_binding = Some(glob_binding);
-                            }
+                        if let Some(old_glob_binding) = resolution.glob_binding
+                            && old_glob_binding != glob_binding
+                        {
+                            resolution.glob_binding =
+                                Some(this.select_glob_binding(glob_binding, old_glob_binding));
                         } else {
                             resolution.glob_binding = Some(glob_binding);
                         }
@@ -440,33 +457,22 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         ambiguity_kind: AmbiguityKind,
         primary_binding: NameBinding<'ra>,
         secondary_binding: NameBinding<'ra>,
-        warn_ambiguity: bool,
+        warning: bool,
     ) -> NameBinding<'ra> {
-        let ambiguity = Some((secondary_binding, ambiguity_kind));
-        let data = NameBindingData { ambiguity, warn_ambiguity, ..*primary_binding };
+        let ambiguity = Some((secondary_binding, ambiguity_kind, warning));
+        let data = NameBindingData { ambiguity, ..*primary_binding };
         self.arenas.alloc_name_binding(data)
     }
 
-    fn new_warn_ambiguity_binding(&self, binding: NameBinding<'ra>) -> NameBinding<'ra> {
-        assert!(binding.is_ambiguity_recursive());
-        self.arenas.alloc_name_binding(NameBindingData { warn_ambiguity: true, ..*binding })
-    }
-
     // 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>(
-        &mut self,
-        module: Module<'ra>,
-        key: BindingKey,
-        warn_ambiguity: bool,
-        f: F,
-    ) -> T
+    fn update_local_resolution<T, F>(&mut self, module: Module<'ra>, key: BindingKey, f: F) -> T
     where
         F: FnOnce(&Resolver<'ra, 'tcx>, &mut NameResolution<'ra>) -> T,
     {
         // Ensure that `resolution` isn't borrowed when defining in the module's glob importers,
         // during which the resolution might end up getting re-defined via a glob cycle.
-        let (binding, t, warn_ambiguity) = {
+        let (binding, t) = {
             let resolution = &mut *self.resolution_or_default(module, key).borrow_mut_unchecked();
             let old_binding = resolution.binding();
 
@@ -475,7 +481,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             if let Some(binding) = resolution.binding()
                 && old_binding != Some(binding)
             {
-                (binding, t, warn_ambiguity || old_binding.is_some())
+                (binding, t)
             } else {
                 return t;
             }
@@ -500,7 +506,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     ident.0,
                     key.ns,
                     imported_binding,
-                    warn_ambiguity,
                 );
             }
         }
@@ -521,11 +526,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             let dummy_binding = self.import(dummy_binding, import);
             self.per_ns(|this, ns| {
                 let module = import.parent_scope.module;
-                let _ = this.try_define_local(module, target, ns, dummy_binding, false);
+                let _ = this.try_define_local(module, target, ns, dummy_binding);
                 // Don't remove underscores from `single_imports`, they were never added.
                 if target.name != kw::Underscore {
                     let key = BindingKey::new(target, ns);
-                    this.update_local_resolution(module, key, false, |_, resolution| {
+                    this.update_local_resolution(module, key, |_, resolution| {
                         resolution.single_imports.swap_remove(&import);
                     })
                 }
@@ -658,7 +663,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
                     && binding.res() != Res::Err
                     && exported_ambiguities.contains(&binding)
                 {
@@ -917,7 +922,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                             this.get_mut_unchecked().update_local_resolution(
                                 parent,
                                 key,
-                                false,
                                 |_, resolution| {
                                     resolution.single_imports.swap_remove(&import);
                                 },
@@ -1523,16 +1527,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             };
             if self.is_accessible_from(binding.vis, scope) {
                 let imported_binding = self.import(binding, import);
-                let warn_ambiguity = self
-                    .resolution(import.parent_scope.module, key)
-                    .and_then(|r| r.binding())
-                    .is_some_and(|binding| binding.warn_ambiguity_recursive());
                 let _ = self.try_define_local(
                     import.parent_scope.module,
                     key.ident.0,
                     key.ns,
                     imported_binding,
-                    warn_ambiguity,
                 );
             }
         }