effective visibility: Remove questionable optimizations

First, they require eagerly calculating private visibility (current normal module), which is somewhat expensive.
Private visibilities are also lost once calculated, instead of being cached in the table.

Second, I cannot prove that the optimizations are correct.
Maybe they can be partially reinstated in the future in cases when it's cheap and provably correct to do them.
They will also probably be merged into `fn update` in that case.

Partially fixes https://github.com/rust-lang/rust/issues/104249
Fixes https://github.com/rust-lang/rust/issues/104539

6 files changed, 70 insertions, 43 deletions

diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs
index decd213763d..d32c2e99043 100644
--- a/compiler/rustc_middle/src/middle/privacy.rs
+++ b/compiler/rustc_middle/src/middle/privacy.rs
@@ -4,7 +4,6 @@
 use crate::ty::{DefIdTree, TyCtxt, Visibility};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
-use rustc_hir::def::DefKind;
 use rustc_macros::HashStable;
 use rustc_query_system::ich::StableHashingContext;
 use rustc_span::def_id::LocalDefId;
@@ -185,7 +184,6 @@ impl EffectiveVisibilities {
                 );
             }
             let nominal_vis = tcx.visibility(def_id);
-            let def_kind = tcx.opt_def_kind(def_id);
             // FIXME: `rustc_privacy` is not yet updated for the new logic and can set
             // effective visibilities that are larger than the nominal one.
             if !nominal_vis.is_at_least(ev.reachable_through_impl_trait, tcx) && early {
@@ -197,11 +195,6 @@ impl EffectiveVisibilities {
                     nominal_vis
                 );
             }
-            // Fully private items are never put into the table, this is important for performance.
-            // FIXME: Fully private `mod` items are currently put into the table.
-            if ev.reachable_through_impl_trait == private_vis && def_kind != Some(DefKind::Mod) {
-                span_bug!(span, "fully private item in the table {:?}: {:?}", def_id, ev.direct);
-            }
         }
     }
 }
diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs
index 57fb1fa9389..32ab58b459a 100644
--- a/compiler/rustc_resolve/src/effective_visibilities.rs
+++ b/compiler/rustc_resolve/src/effective_visibilities.rs
@@ -42,6 +42,24 @@ impl Resolver<'_> {
     fn nearest_normal_mod(&mut self, def_id: LocalDefId) -> LocalDefId {
         self.get_nearest_non_block_module(def_id.to_def_id()).nearest_parent_mod().expect_local()
     }
+
+    fn private_vis_import(&mut self, binding: ImportId<'_>) -> Visibility {
+        let NameBindingKind::Import { import, .. } = binding.kind else { unreachable!() };
+        Visibility::Restricted(
+            import
+                .id()
+                .map(|id| self.nearest_normal_mod(self.local_def_id(id)))
+                .unwrap_or(CRATE_DEF_ID),
+        )
+    }
+
+    fn private_vis_def(&mut self, def_id: LocalDefId) -> Visibility {
+        if def_id == CRATE_DEF_ID {
+            Visibility::Public
+        } else {
+            Visibility::Restricted(self.nearest_normal_mod(def_id))
+        }
+    }
 }
 
 impl<'a, 'b> IntoDefIdTree for &'b mut Resolver<'a> {
@@ -143,36 +161,12 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> {
         .copied()
     }
 
-    /// The update is guaranteed to not change the table and we can skip it.
-    fn is_noop_update(
-        &self,
-        parent_id: ParentId<'a>,
-        nominal_vis: Visibility,
-        default_vis: Visibility,
-    ) -> bool {
-        nominal_vis == default_vis
-            || match parent_id {
-                ParentId::Def(def_id) => self.r.visibilities[&def_id],
-                ParentId::Import(binding) => binding.vis.expect_local(),
-            } == default_vis
-    }
-
     fn update_import(&mut self, binding: ImportId<'a>, parent_id: ParentId<'a>) {
-        let NameBindingKind::Import { import, .. } = binding.kind else { unreachable!() };
         let nominal_vis = binding.vis.expect_local();
-        let default_vis = Visibility::Restricted(
-            import
-                .id()
-                .map(|id| self.r.nearest_normal_mod(self.r.local_def_id(id)))
-                .unwrap_or(CRATE_DEF_ID),
-        );
-        if self.is_noop_update(parent_id, nominal_vis, default_vis) {
-            return;
-        }
         self.changed |= self.import_effective_visibilities.update(
             binding,
             nominal_vis,
-            |r| (default_vis, r),
+            |r| (r.private_vis_import(binding), r),
             self.effective_vis(parent_id),
             parent_id.level(),
             &mut *self.r,
@@ -180,14 +174,10 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> {
     }
 
     fn update_def(&mut self, def_id: LocalDefId, nominal_vis: Visibility, parent_id: ParentId<'a>) {
-        let default_vis = Visibility::Restricted(self.r.nearest_normal_mod(def_id));
-        if self.is_noop_update(parent_id, nominal_vis, default_vis) {
-            return;
-        }
         self.changed |= self.def_effective_visibilities.update(
             def_id,
             nominal_vis,
-            |r| (if def_id == CRATE_DEF_ID { Visibility::Public } else { default_vis }, r),
+            |r| (r.private_vis_def(def_id), r),
             self.effective_vis(parent_id),
             parent_id.level(),
             &mut *self.r,
diff --git a/src/test/ui/privacy/effective_visibilities.rs b/src/test/ui/privacy/effective_visibilities.rs
index 4479b0d8f61..8d0602fa79f 100644
--- a/src/test/ui/privacy/effective_visibilities.rs
+++ b/src/test/ui/privacy/effective_visibilities.rs
@@ -17,13 +17,13 @@ mod outer { //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub
         }
 
         #[rustc_effective_visibility]
-        struct PrivStruct; //~ ERROR not in the table
-                           //~| ERROR not in the table
+        struct PrivStruct; //~ ERROR Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
+                           //~| ERROR Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
 
         #[rustc_effective_visibility]
         pub union PubUnion { //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
             #[rustc_effective_visibility]
-            a: u8, //~ ERROR not in the table
+            a: u8, //~ ERROR Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
             #[rustc_effective_visibility]
             pub b: u8, //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
         }
diff --git a/src/test/ui/privacy/effective_visibilities.stderr b/src/test/ui/privacy/effective_visibilities.stderr
index 019aaf8086a..6a99afe64fe 100644
--- a/src/test/ui/privacy/effective_visibilities.stderr
+++ b/src/test/ui/privacy/effective_visibilities.stderr
@@ -22,13 +22,13 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl
 LL |         pub trait PubTrait {
    |         ^^^^^^^^^^^^^^^^^^
 
-error: not in the table
+error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
   --> $DIR/effective_visibilities.rs:20:9
    |
 LL |         struct PrivStruct;
    |         ^^^^^^^^^^^^^^^^^
 
-error: not in the table
+error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
   --> $DIR/effective_visibilities.rs:20:9
    |
 LL |         struct PrivStruct;
@@ -40,7 +40,7 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl
 LL |         pub union PubUnion {
    |         ^^^^^^^^^^^^^^^^^^
 
-error: not in the table
+error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
   --> $DIR/effective_visibilities.rs:26:13
    |
 LL |             a: u8,
diff --git a/src/test/ui/privacy/effective_visibilities_invariants.rs b/src/test/ui/privacy/effective_visibilities_invariants.rs
new file mode 100644
index 00000000000..8c524d32815
--- /dev/null
+++ b/src/test/ui/privacy/effective_visibilities_invariants.rs
@@ -0,0 +1,12 @@
+// Invariant checking doesn't ICE in some cases with errors (issue #104249).
+
+#![feature(staged_api)] //~ ERROR module has missing stability attribute
+
+pub mod m {} //~ ERROR module has missing stability attribute
+
+pub mod m { //~ ERROR the name `m` is defined multiple times
+    // mod inner {} - ICE
+    type Inner = u8;
+}
+
+fn main() {}
diff --git a/src/test/ui/privacy/effective_visibilities_invariants.stderr b/src/test/ui/privacy/effective_visibilities_invariants.stderr
new file mode 100644
index 00000000000..fd205f4058a
--- /dev/null
+++ b/src/test/ui/privacy/effective_visibilities_invariants.stderr
@@ -0,0 +1,32 @@
+error[E0428]: the name `m` is defined multiple times
+  --> $DIR/effective_visibilities_invariants.rs:7:1
+   |
+LL | pub mod m {}
+   | --------- previous definition of the module `m` here
+LL |
+LL | pub mod m {
+   | ^^^^^^^^^ `m` redefined here
+   |
+   = note: `m` must be defined only once in the type namespace of this module
+
+error: module has missing stability attribute
+  --> $DIR/effective_visibilities_invariants.rs:3:1
+   |
+LL | / #![feature(staged_api)]
+LL | |
+LL | | pub mod m {}
+LL | |
+...  |
+LL | |
+LL | | fn main() {}
+   | |____________^
+
+error: module has missing stability attribute
+  --> $DIR/effective_visibilities_invariants.rs:5:1
+   |
+LL | pub mod m {}
+   | ^^^^^^^^^^^^
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0428`.