Rollup merge of rust-lang#154149 - petrochenkov:globvisglob, r=mu001999

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

Continuation of rust-lang#149596, implementation of this comment rust-lang#149596 (comment) in particular.
FCP for the lint in general - rust-lang#149596 (comment).
rust-lang#152498 is reverted as a part of the change, but fixes are applied to keep the tests added in that PR working.

To implement this we have to have to track the most and the least visible declarations in an ambiguous glob set.

Part of rust-lang#153961.
r? @yaahc maybe

Author: JonathanBrouwer

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
 }