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

Author: petrochenkov

compiler/rustc_middle/src/middle/privacy.rs

@@ -8,6 +8,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};
 
@@ -185,13 +186,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 ev.reachable.greater_than(nominal_vis, 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 increased 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

@@ -93,7 +93,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.to_module()),

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

@@ -490,6 +490,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                             }
                             Some(Finalize { import, .. }) => import,
                         };
+                        this.get_mut().maybe_push_glob_vs_glob_vis_ambiguity(
+                            ident,
+                            orig_ident_span,
+                            decl,
+                            import,
+                        );
 
                         if let Some(&(innermost_decl, _)) = innermost_results.first() {
                             // Found another solution, if the first one was "weak", report an error.
@@ -779,6 +785,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: Option<ImportSummary>,
+    ) {
+        let Some(import) = import else { return };
+        let vis1 = self.import_decl_vis(decl, import);
+        let vis2 = self.import_decl_vis_ext(decl, import, true);
+        if vis1 != vis2 {
+            self.ambiguity_errors.push(AmbiguityError {
+                kind: AmbiguityKind::GlobVsGlob,
+                ambig_vis: Some((vis1, vis2)),
+                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,

compiler/rustc_resolve/src/imports.rs

@@ -370,8 +370,17 @@ fn remove_same_import<'ra>(d1: Decl<'ra>, d2: Decl<'ra>) -> (Decl<'ra>, Decl<'ra
 
 impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     pub(crate) fn import_decl_vis(&self, decl: Decl<'ra>, import: ImportSummary) -> Visibility {
+        self.import_decl_vis_ext(decl, import, false)
+    }
+
+    pub(crate) fn import_decl_vis_ext(
+        &self,
+        decl: Decl<'ra>,
+        import: ImportSummary,
+        min: bool,
+    ) -> Visibility {
         assert!(import.vis.is_accessible_from(import.nearest_parent_mod, self.tcx));
-        let decl_vis = decl.vis();
+        let decl_vis = if min { decl.min_vis() } else { decl.vis() };
         if decl_vis.partial_cmp(import.vis, self.tcx) == Some(Ordering::Less)
             && decl_vis.is_accessible_from(import.nearest_parent_mod, self.tcx)
             && pub_use_of_private_extern_crate_hack(import, decl).is_none()
@@ -409,7 +418,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             ambiguity: CmCell::new(None),
             warn_ambiguity: CmCell::new(false),
             span: import.span,
-            vis: CmCell::new(vis.to_def_id()),
+            initial_vis: vis.to_def_id(),
+            ambiguity_vis_max: CmCell::new(None),
+            ambiguity_vis_min: CmCell::new(None),
             expansion: import.parent_scope.expansion,
             parent_module: Some(import.parent_scope.module),
         })
@@ -434,8 +445,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
@@ -449,8 +458,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());
@@ -470,12 +478,21 @@ 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 glob_decl.vis().greater_than(old_glob_decl.vis(), self.tcx) {
-            // We are glob-importing the same item but with greater visibility.
+        } 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.
             // All visibilities here are ordered because all of them are ancestors of `module`.
-            // FIXME: Update visibility in place, but without regressions
-            // (#152004, #151124, #152347).
-            glob_decl
+            if vis.greater_than(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.greater_than(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));
@@ -498,6 +515,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     ) -> Result<(), Decl<'ra>> {
         assert!(!decl.warn_ambiguity.get());
         assert!(decl.ambiguity.get().is_none());
+        assert!(decl.ambiguity_vis_max.get().is_none());
+        assert!(decl.ambiguity_vis_min.get().is_none());
         let module = decl.parent_module.unwrap().expect_local();
         assert!(self.is_accessible_from(decl.vis(), module.to_module()));
         let res = decl.res();
@@ -556,11 +575,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                 .resolution_or_default(module.to_module(), key, orig_ident_span)
                 .borrow_mut_unchecked();
             let old_decl = resolution.determined_decl();
+            let old_vis = old_decl.map(|d| d.vis());
 
             let t = f(self, resolution);
 
             if let Some(binding) = resolution.determined_decl()
-                && 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

@@ -929,7 +929,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
+    /// declaration 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
+    /// declaration from the set, if its visibility is different from `initial_vis`.
+    ambiguity_vis_min: CmCell<Option<Decl<'ra>>>,
     parent_module: Option<Module<'ra>>,
 }
 
@@ -1049,7 +1055,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 {
@@ -1505,7 +1517,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-priv.stderr

@@ -5,10 +5,10 @@ LL | use crate::both::private::S;
    |                           ^ private struct import
    |
 note: the struct import `S` is defined here...
-  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:8:24
+  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:7:13
    |
-LL |         pub(super) use crate::m::*;
-   |                        ^^^^^^^^^^^
+LL |         use crate::m::*;
+   |             ^^^^^^^^^^^
 note: ...and refers to the struct `S` which is defined here
   --> $DIR/ambiguous-import-visibility-globglob-priv.rs:2:5
    |
@@ -27,10 +27,10 @@ LL |     use crate::both::private::S;
    |                               ^ private struct import
    |
 note: the struct import `S` is defined here...
-  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:8:24
+  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:7:13
    |
-LL |         pub(super) use crate::m::*;
-   |                        ^^^^^^^^^^^
+LL |         use crate::m::*;
+   |             ^^^^^^^^^^^
 note: ...and refers to the struct `S` which is defined here
   --> $DIR/ambiguous-import-visibility-globglob-priv.rs:2:5
    |

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

@@ -1,7 +1,5 @@
 //@ check-pass
 
-// FIXME: should report "ambiguous import visibility" in all the cases below.
-
 mod m {
     pub struct S {}
 }
@@ -12,7 +10,11 @@ mod min_vis_first {
     pub use crate::m::*;
 
     pub use self::S as S1;
+    //~^ WARN ambiguous import visibility
+    //~| WARN this was previously accepted
     pub(crate) use self::S as S2;
+    //~^ WARN ambiguous import visibility
+    //~| WARN this was previously accepted
     use self::S as S3; // OK
 }
 
@@ -22,7 +24,11 @@ mod mid_vis_first {
     pub use crate::m::*;
 
     pub use self::S as S1;
+    //~^ WARN ambiguous import visibility
+    //~| WARN this was previously accepted
     pub(crate) use self::S as S2;
+    //~^ WARN ambiguous import visibility
+    //~| WARN this was previously accepted
     use self::S as S3; // OK
 }
 
@@ -32,7 +38,11 @@ mod max_vis_first {
     pub(crate) use crate::m::*;
 
     pub use self::S as S1;
+    //~^ WARN ambiguous import visibility
+    //~| WARN this was previously accepted
     pub(crate) use self::S as S2;
+    //~^ WARN ambiguous import visibility
+    //~| WARN this was previously accepted
     use self::S as S3; // OK
 }