Auto merge of #154149 - petrochenkov:globvisglob, r=<try>

resolve: Extend `ambiguous_import_visibilities` deprecation lint to glob-vs-glob ambiguities

Author: bors

compiler/rustc_lint_defs/src/builtin.rs

@@ -4745,7 +4745,7 @@ declare_lint! {
     ///
     /// [future-incompatible]: ../index.md#future-incompatible-lints
     pub AMBIGUOUS_IMPORT_VISIBILITIES,
-    Warn,
+    Deny,
     "detects certain glob imports that require reporting an ambiguity error",
     @future_incompatible = FutureIncompatibleInfo {
         reason: fcw!(FutureReleaseError #149145),

compiler/rustc_middle/src/middle/privacy.rs

@@ -7,6 +7,7 @@ use std::hash::Hash;
 use rustc_data_structures::fx::{FxIndexMap, IndexEntry};
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_hir::def::DefKind;
+use rustc_hir::{ItemKind, Node, UseKind};
 use rustc_macros::HashStable;
 use rustc_span::def_id::{CRATE_DEF_ID, LocalDefId};
 
@@ -184,13 +185,20 @@ impl EffectiveVisibilities {
             if !is_impl && tcx.trait_impl_of_assoc(def_id.to_def_id()).is_none() {
                 let nominal_vis = tcx.visibility(def_id);
                 if !nominal_vis.is_at_least(ev.reachable, tcx) {
-                    span_bug!(
-                        span,
-                        "{:?}: reachable {:?} > nominal {:?}",
-                        def_id,
-                        ev.reachable,
-                        nominal_vis,
-                    );
+                    if let Node::Item(item) = tcx.hir_node_by_def_id(def_id)
+                        && let ItemKind::Use(_, UseKind::Glob) = item.kind
+                    {
+                        // Glob import visibilities can be increasee by other
+                        // more public glob imports in cases of ambiguity.
+                    } else {
+                        span_bug!(
+                            span,
+                            "{:?}: reachable {:?} > nominal {:?}",
+                            def_id,
+                            ev.reachable,
+                            nominal_vis,
+                        );
+                    }
                 }
             }
         }

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -95,7 +95,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             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),
+            initial_vis: vis,
+            ambiguity_vis_max: CmCell::new(None),
+            ambiguity_vis_min: CmCell::new(None),
             span,
             expansion,
             parent_module: Some(parent),

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -182,6 +182,10 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
             parent_id.level(),
             tcx,
         );
+        if let Some(max_vis_decl) = decl.ambiguity_vis_max.get() {
+            // Avoid the most visible import in an ambiguous glob set being reported as unused.
+            self.update_import(max_vis_decl, parent_id);
+        }
     }
 
     fn update_def(

compiler/rustc_resolve/src/ident.rs

@@ -5,6 +5,7 @@ use Namespace::*;
 use rustc_ast::{self as ast, NodeId};
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir::def::{DefKind, MacroKinds, Namespace, NonMacroAttrKind, PartialRes, PerNS};
+use rustc_hir::def_id::DefId;
 use rustc_middle::ty::Visibility;
 use rustc_middle::{bug, span_bug};
 use rustc_session::lint::builtin::PROC_MACRO_DERIVE_RESOLUTION_FALLBACK;
@@ -492,6 +493,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                             }
                             Some(Finalize { import_vis, .. }) => import_vis,
                         };
+                        this.get_mut().maybe_push_glob_vs_glob_vis_ambiguity(
+                            ident,
+                            orig_ident_span,
+                            decl,
+                            import_vis,
+                        );
 
                         if let Some(&(innermost_decl, _)) = innermost_results.first() {
                             // Found another solution, if the first one was "weak", report an error.
@@ -774,6 +781,30 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         ret.map_err(ControlFlow::Continue)
     }
 
+    fn maybe_push_glob_vs_glob_vis_ambiguity(
+        &mut self,
+        ident: IdentKey,
+        orig_ident_span: Span,
+        decl: Decl<'ra>,
+        import_vis: Option<Visibility>,
+    ) {
+        let Some(import_vis) = import_vis else { return };
+        let min = |vis: Visibility<DefId>| vis.min(import_vis.to_def_id(), self.tcx).expect_local();
+        let (min1, min2) = (min(decl.vis()), min(decl.min_vis()));
+        if min1 != min2 {
+            self.ambiguity_errors.push(AmbiguityError {
+                kind: AmbiguityKind::GlobVsGlob,
+                ambig_vis: Some((min1, min2)),
+                ident: ident.orig(orig_ident_span),
+                b1: decl.ambiguity_vis_max.get().unwrap_or(decl),
+                b2: decl.ambiguity_vis_min.get().unwrap_or(decl),
+                scope1: Scope::ModuleGlobs(decl.parent_module.unwrap(), None),
+                scope2: Scope::ModuleGlobs(decl.parent_module.unwrap(), None),
+                warning: Some(AmbiguityWarning::GlobImport),
+            });
+        }
+    }
+
     fn maybe_push_ambiguity(
         &mut self,
         ident: IdentKey,
@@ -894,16 +925,18 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     None
                 };
 
-                self.ambiguity_errors.push(AmbiguityError {
-                    kind,
-                    ambig_vis,
-                    ident: ident.orig(orig_ident_span),
-                    b1: innermost_decl,
-                    b2: decl,
-                    scope1: innermost_scope,
-                    scope2: scope,
-                    warning,
-                });
+                if ambig_vis.is_none() {
+                    self.ambiguity_errors.push(AmbiguityError {
+                        kind,
+                        ambig_vis,
+                        ident: ident.orig(orig_ident_span),
+                        b1: innermost_decl,
+                        b2: decl,
+                        scope1: innermost_scope,
+                        scope2: scope,
+                        warning,
+                    });
+                }
                 return true;
             }
         }

compiler/rustc_resolve/src/imports.rs

@@ -346,7 +346,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             ambiguity: CmCell::new(None),
             warn_ambiguity: CmCell::new(false),
             span: import.span,
-            vis: CmCell::new(vis),
+            initial_vis: vis,
+            ambiguity_vis_max: CmCell::new(None),
+            ambiguity_vis_min: CmCell::new(None),
             expansion: import.parent_scope.expansion,
             parent_module: Some(import.parent_scope.module),
         })
@@ -371,8 +373,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         // - A glob decl 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 decl in some way.
-        // - A glob decl is overwritten by a glob decl with larger visibility.
-        //   FIXME: avoid this by updating this visibility in place.
         // - A glob decl is overwritten by a glob decl re-fetching an
         //   overwritten decl from other module (the recursive case).
         // Here we are detecting all such re-fetches and overwrite old decls
@@ -386,8 +386,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             // FIXME: reenable the asserts when `warn_ambiguity` is removed (#149195).
             // assert_ne!(old_deep_decl, deep_decl);
             // assert!(old_deep_decl.is_glob_import());
-            // FIXME: reenable the assert when visibility is updated in place.
-            // assert!(!deep_decl.is_glob_import());
+            assert!(!deep_decl.is_glob_import());
             if old_glob_decl.ambiguity.get().is_some() && glob_decl.ambiguity.get().is_none() {
                 // Do not lose glob ambiguities when re-fetching the glob.
                 glob_decl.ambiguity.set_unchecked(old_glob_decl.ambiguity.get());
@@ -407,11 +406,20 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                 // FIXME: remove this when `warn_ambiguity` is removed (#149195).
                 self.arenas.alloc_decl((*old_glob_decl).clone())
             }
-        } else if !old_glob_decl.vis().is_at_least(glob_decl.vis(), self.tcx) {
-            // We are glob-importing the same item but with greater visibility.
-            // FIXME: Update visibility in place, but without regressions
-            // (#152004, #151124, #152347).
-            glob_decl
+        } else if let old_vis = old_glob_decl.vis()
+            && let vis = glob_decl.vis()
+            && old_vis != vis
+        {
+            // We are glob-importing the same item but with a different visibility.
+            if vis.is_at_least(old_vis, self.tcx) {
+                old_glob_decl.ambiguity_vis_max.set_unchecked(Some(glob_decl));
+            } else if let old_min_vis = old_glob_decl.min_vis()
+                && old_min_vis != vis
+                && old_min_vis.is_at_least(vis, self.tcx)
+            {
+                old_glob_decl.ambiguity_vis_min.set_unchecked(Some(glob_decl));
+            }
+            old_glob_decl
         } else if glob_decl.is_ambiguity_recursive() && !old_glob_decl.is_ambiguity_recursive() {
             // Overwriting a non-ambiguous glob import with an ambiguous glob import.
             old_glob_decl.ambiguity.set_unchecked(Some(glob_decl));
@@ -510,11 +518,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                 .resolution_or_default(module, key, orig_ident_span)
                 .borrow_mut_unchecked();
             let old_decl = resolution.binding();
+            let old_vis = old_decl.map(|d| d.vis());
 
             let t = f(self, resolution);
 
             if let Some(binding) = resolution.binding()
-                && old_decl != Some(binding)
+                && (old_decl != Some(binding) || old_vis != Some(binding.vis()))
             {
                 (binding, t, warn_ambiguity || old_decl.is_some())
             } else {

compiler/rustc_resolve/src/lib.rs

@@ -850,7 +850,13 @@ struct DeclData<'ra> {
     warn_ambiguity: CmCell<bool>,
     expansion: LocalExpnId,
     span: Span,
-    vis: CmCell<Visibility<DefId>>,
+    initial_vis: Visibility<DefId>,
+    /// If the declaration refers to an ambiguous glob set, then this is the most visible binding
+    /// from the set, if its visibility is different from `initial_vis`.
+    ambiguity_vis_max: CmCell<Option<Decl<'ra>>>,
+    /// If the declaration refers to an ambiguous glob set, then this is the least visible binding
+    /// from the set, if its visibility is different from `initial_vis`.
+    ambiguity_vis_min: CmCell<Option<Decl<'ra>>>,
     parent_module: Option<Module<'ra>>,
 }
 
@@ -970,7 +976,13 @@ struct AmbiguityError<'ra> {
 
 impl<'ra> DeclData<'ra> {
     fn vis(&self) -> Visibility<DefId> {
-        self.vis.get()
+        // Select the maximum visibility if there are multiple ambiguous glob imports.
+        self.ambiguity_vis_max.get().map(|d| d.vis()).unwrap_or_else(|| self.initial_vis)
+    }
+
+    fn min_vis(&self) -> Visibility<DefId> {
+        // Select the minimum visibility if there are multiple ambiguous glob imports.
+        self.ambiguity_vis_min.get().map(|d| d.vis()).unwrap_or_else(|| self.initial_vis)
     }
 
     fn res(&self) -> Res {
@@ -1415,7 +1427,9 @@ impl<'ra> ResolverArenas<'ra> {
             kind: DeclKind::Def(res),
             ambiguity: CmCell::new(None),
             warn_ambiguity: CmCell::new(false),
-            vis: CmCell::new(vis),
+            initial_vis: vis,
+            ambiguity_vis_max: CmCell::new(None),
+            ambiguity_vis_min: CmCell::new(None),
             span,
             expansion,
             parent_module,

tests/ui/imports/ambiguous-import-visibility-globglob.rs

@@ -0,0 +1,47 @@
+mod m {
+    pub struct S {}
+}
+
+mod min_vis_first {
+    use crate::m::*;
+    pub(crate) use crate::m::*;
+    pub use crate::m::*;
+
+    pub use self::S as S1;
+    //~^ ERROR ambiguous import visibility
+    //~| WARN this was previously accepted
+    pub(crate) use self::S as S2;
+    //~^ ERROR ambiguous import visibility
+    //~| WARN this was previously accepted
+    use self::S as S3; // OK
+}
+
+mod mid_vis_first {
+    pub(crate) use crate::m::*;
+    use crate::m::*;
+    pub use crate::m::*;
+
+    pub use self::S as S1;
+    //~^ ERROR ambiguous import visibility
+    //~| WARN this was previously accepted
+    pub(crate) use self::S as S2;
+    //~^ ERROR ambiguous import visibility
+    //~| WARN this was previously accepted
+    use self::S as S3; // OK
+}
+
+mod max_vis_first {
+    pub use crate::m::*;
+    use crate::m::*;
+    pub(crate) use crate::m::*;
+
+    pub use self::S as S1;
+    //~^ ERROR ambiguous import visibility
+    //~| WARN this was previously accepted
+    pub(crate) use self::S as S2;
+    //~^ ERROR ambiguous import visibility
+    //~| WARN this was previously accepted
+    use self::S as S3; // OK
+}
+
+fn main() {}