resolving cyclic glob vis-max

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -129,32 +129,31 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
             let Some(decl) = name_resolution.borrow(self.r).best_decl() else {
                 continue;
             };
-            self.update_decl_chain(decl, ParentId::Def(module_id));
+            self.update_decl_chain(decl, ParentId::Def(module_id), &mut FxHashSet::default());
         }
     }
 
     /// Update effective visibilities for the whole reexport chain of a declaration.
     /// Set the given effective visibility level to `Level::Direct` and
     /// sets the rest of the `use` chain to `Level::Reexported` until
     /// we hit the actual exported item.
-    fn update_decl_chain(&mut self, mut decl: Decl<'ra>, mut parent_id: ParentId<'ra>) {
+    fn update_decl_chain(
+        &mut self,
+        mut decl: Decl<'ra>,
+        mut parent_id: ParentId<'ra>,
+        seen_most_visible: &mut FxHashSet<Decl<'ra>>,
+    ) {
         let priv_vis = |this: &Self, parent_id, decl| match parent_id {
             ParentId::Def(_) => this.current_private_vis,
             ParentId::Import(_) => this.r.private_vis_decl(decl),
         };
         while let DeclKind::Import { source_decl, .. } = decl.kind {
             self.update_import(decl, parent_id, priv_vis(self, parent_id, decl));
-            if let Some(max_vis_decl) = decl.ambiguity_vis_max.get() {
-                // The name is exported with the visibility of the most visible declaration
-                // in its ambiguous glob set (see `DeclData::vis`), so everything on that
-                // declaration's reexport chain, including the final item, must get its
-                // effective visibility from that declaration as well. Otherwise the item
-                // would be considered unreachable by dead code analysis and metadata
-                // encoding despite being exported (see the regression test
-                // `ambiguous-import-visibility-globglob-mir.rs`).
-                // This also avoids the most visible import in an ambiguous glob set
-                // being reported as unused.
-                self.update_decl_chain(max_vis_decl, parent_id);
+            // `ambiguity_vis_max` can cycle on mutual globs; follow each once.
+            if let Some(most_visible) = decl.ambiguity_vis_max.get()
+                && seen_most_visible.insert(most_visible)
+            {
+                self.update_decl_chain(most_visible, parent_id, seen_most_visible);
             }
             parent_id = ParentId::Import(decl);
             decl = source_decl;

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

@@ -0,0 +1,30 @@
+// issue: rust-lang/rust#160685
+// Mutual globs of the same item cycle through `ambiguity_vis_max`.
+
+#![feature(rustc_attrs)]
+#![allow(internal_features)]
+#![deny(dead_code)]
+
+pub mod axiomatic {
+    use super::*; // not pub
+    pub use self::own::*;
+
+    pub mod own {
+        pub use super::*;
+        pub use super::orphan::*;
+    }
+
+    pub mod orphan {
+        pub use super::private::CollectionDescriptor;
+    }
+
+    mod private {
+        #[rustc_effective_visibility]
+        pub struct CollectionDescriptor {}
+        //~^ ERROR Direct: pub(in crate::axiomatic), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+    }
+}
+
+pub use axiomatic::orphan::*;
+
+fn main() {}

tests/ui/imports/ambiguous-import-visibility-globglob-cycle.stderr

@@ -0,0 +1,8 @@
+error: Direct: pub(in crate::axiomatic), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+  --> $DIR/ambiguous-import-visibility-globglob-cycle.rs:23:9
+   |
+LL |         pub struct CollectionDescriptor {}
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 1 previous error
+

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

@@ -1,11 +1,6 @@
-// Regression test for the 1.96 -> 1.97 stable-to-stable regression: an item exported
-// only through a public glob, and also glob-imported with restricted visibility through
-// a private facade, lost its exported effective visibility. The defining crate then
-// skipped encoding its optimized MIR (and warned dead_code) while name resolution still
-// exported the item and it remained `cross_crate_inlinable`, so downstream crates failed
-// with "missing optimized MIR". This test pins the missing-MIR half; the dead_code half
-// is checked by the sibling test `ambiguous-import-visibility-globglob-reachable.rs`
-// (via its `#![deny(dead_code)]`).
+// issue: rust-lang/rust#159038
+// Downstream missing optimized MIR when a restricted glob wins the slot.
+// Dead code: `ambiguous-import-visibility-globglob-reachable.rs`.
 
 //@ build-pass
 //@ aux-build:ambiguous-import-visibility-globglob-mir.rs

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

@@ -1,9 +1,5 @@
-// Regression test for the 1.96 -> 1.97 stable-to-stable regression: an item exported
-// only through a public glob, and also glob-imported with restricted visibility through
-// a private facade, lost its exported effective visibility while name resolution still
-// exported it. Downstream: spurious dead_code in this crate, "missing optimized MIR" in
-// dependent crates (see ambiguous-import-visibility-globglob-mir.rs). The public glob
-// declaration must drive the effective visibility of the whole reexport chain.
+// issue: rust-lang/rust#159038
+// A restricted glob in the slot must not hide a more public glob of the same item.
 
 #![feature(rustc_attrs)]
 #![allow(internal_features)]

tests/ui/imports/ambiguous-import-visibility-globglob-reachable.stderr

@@ -1,5 +1,5 @@
 error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
-  --> $DIR/ambiguous-import-visibility-globglob-reachable.rs:14:5
+  --> $DIR/ambiguous-import-visibility-globglob-reachable.rs:10:5
    |
 LL |     pub fn f() {}
    |     ^^^^^^^^^^

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

@@ -1,7 +1,4 @@
-// An item exported only through a public glob, while also glob-imported into the
-// same module through a facade with restricted visibility. The restricted duplicate
-// must not make `f` unreachable: its optimized MIR must still be encoded for
-// downstream crates (it is `cross_crate_inlinable`).
+// Restricted glob must not stop `f` from being encoded.
 
 mod inner {
     pub fn f() -> u32 {