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

@@ -4437,7 +4437,7 @@ declare_lint! {
     ///
     /// ### Example
     ///
-    /// ```rust,compile_fail
+    /// ```rust,ignore (needs extern crate)
     /// #![deny(ambiguous_glob_imports)]
     /// pub fn foo() -> u32 {
     ///     use sub::*;
@@ -4453,8 +4453,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/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,7 +325,6 @@ 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,
@@ -339,7 +338,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         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,7 +349,7 @@ 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() {
                 if res == Res::Err && old_binding.res() != Res::Err {
                     // Do not override real bindings with `Res::Err`s from error recovery.
@@ -360,30 +358,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                 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() {
+                        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) {
+                        } else if !old_glob_binding.vis.is_at_least(glob_binding.vis, this.tcx)
+                            || glob_binding.is_ambiguity_recursive()
+                        {
                             // 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));
                         }
                     }
                     (old_glob @ true, false) | (old_glob @ false, true) => {
@@ -397,7 +382,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                                 AmbiguityKind::GlobVsExpanded,
                                 non_glob_binding,
                                 glob_binding,
-                                false,
                             ));
                         } else {
                             resolution.non_glob_binding = Some(non_glob_binding);
@@ -410,9 +394,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                                     AmbiguityKind::GlobVsGlob,
                                     old_glob_binding,
                                     glob_binding,
-                                    false,
                                 ));
-                            } else if !old_glob_binding.vis.is_at_least(binding.vis, this.tcx) {
+                            } else if !old_glob_binding.vis.is_at_least(glob_binding.vis, this.tcx)
+                                || glob_binding.is_ambiguity_recursive()
+                            {
+                                // We are glob-importing the same item but with greater visibility.
                                 resolution.glob_binding = Some(glob_binding);
                             }
                         } else {
@@ -440,33 +426,21 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         ambiguity_kind: AmbiguityKind,
         primary_binding: NameBinding<'ra>,
         secondary_binding: NameBinding<'ra>,
-        warn_ambiguity: 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, false));
+        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 +449,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 +474,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     ident.0,
                     key.ns,
                     imported_binding,
-                    warn_ambiguity,
                 );
             }
         }
@@ -521,11 +494,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 +631,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 +890,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 +1495,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,
                 );
             }
         }

compiler/rustc_resolve/src/lib.rs

@@ -792,10 +792,7 @@ impl<'ra> fmt::Debug for Module<'ra> {
 #[derive(Clone, Copy, Debug)]
 struct NameBindingData<'ra> {
     kind: NameBindingKind<'ra>,
-    ambiguity: Option<(NameBinding<'ra>, AmbiguityKind)>,
-    /// 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: bool,
+    ambiguity: Option<(NameBinding<'ra>, AmbiguityKind, bool /*warning*/)>,
     expansion: LocalExpnId,
     span: Span,
     vis: Visibility<DefId>,
@@ -932,7 +929,7 @@ impl<'ra> NameBindingData<'ra> {
         self: NameBinding<'ra>,
     ) -> Option<(NameBinding<'ra>, NameBinding<'ra>, AmbiguityKind)> {
         match self.ambiguity {
-            Some((ambig_binding, ambig_kind)) => Some((self, ambig_binding, ambig_kind)),
+            Some((ambig_binding, ambig_kind, _)) => Some((self, ambig_binding, ambig_kind)),
             None => match self.kind {
                 NameBindingKind::Import { binding, .. } => binding.descent_to_ambiguity(),
                 _ => None,
@@ -948,14 +945,6 @@ impl<'ra> NameBindingData<'ra> {
             }
     }
 
-    fn warn_ambiguity_recursive(&self) -> bool {
-        self.warn_ambiguity
-            || match self.kind {
-                NameBindingKind::Import { binding, .. } => binding.warn_ambiguity_recursive(),
-                _ => false,
-            }
-    }
-
     fn is_possibly_imported_variant(&self) -> bool {
         match self.kind {
             NameBindingKind::Import { binding, .. } => binding.is_possibly_imported_variant(),
@@ -1340,7 +1329,6 @@ impl<'ra> ResolverArenas<'ra> {
         self.alloc_name_binding(NameBindingData {
             kind: NameBindingKind::Res(res),
             ambiguity: None,
-            warn_ambiguity: false,
             vis,
             span,
             expansion,
@@ -2045,25 +2033,15 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     }
 
     fn record_use(&mut self, ident: Ident, used_binding: NameBinding<'ra>, used: Used) {
-        self.record_use_inner(ident, used_binding, used, used_binding.warn_ambiguity);
-    }
-
-    fn record_use_inner(
-        &mut self,
-        ident: Ident,
-        used_binding: NameBinding<'ra>,
-        used: Used,
-        warn_ambiguity: bool,
-    ) {
-        if let Some((b2, kind)) = used_binding.ambiguity {
+        if let Some((b2, kind, warning)) = used_binding.ambiguity {
             let ambiguity_error = AmbiguityError {
                 kind,
                 ident,
                 b1: used_binding,
                 b2,
                 misc1: AmbiguityErrorMisc::None,
                 misc2: AmbiguityErrorMisc::None,
-                warning: warn_ambiguity,
+                warning,
             };
             if !self.matches_previous_ambiguity_error(&ambiguity_error) {
                 // avoid duplicated span information to be emit out
@@ -2112,12 +2090,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                 self.used_imports.insert(id);
             }
             self.add_to_glob_map(import, ident);
-            self.record_use_inner(
-                ident,
-                binding,
-                Used::Other,
-                warn_ambiguity || binding.warn_ambiguity,
-            );
+            self.record_use(ident, binding, Used::Other);
         }
     }
 

tests/ui/imports/ambiguous-1.rs

@@ -1,8 +1,5 @@
-//@ check-pass
 // https://github.com/rust-lang/rust/pull/112743#issuecomment-1601986883
 
-#![warn(ambiguous_glob_imports)]
-
 macro_rules! m {
     () => {
       pub fn id() {}
@@ -27,6 +24,5 @@ pub use openssl::*;
 
 fn main() {
     id();
-    //~^ WARNING `id` is ambiguous
-    //~| WARNING this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+    //~^ ERROR `id` is ambiguous
 }