Auto merge of rust-lang#104602 - petrochenkov:effvisperf5, r=oli-obk

privacy: Fix more (potential) issues with effective visibilities

Continuation of rust-lang#103965.
See individual commits for more detailed description of the changes.

The shortcuts removed in rust-lang@4eb63f6 and rust-lang@c7c7d16 could actually be correct (or correct after some tweaks), but they used global reasoning like "we can skip this update because if the code compiles then some other update should do the same thing eventually".
I have some expertise in this area, but I still have doubt whether such global reasoning was correct or not, especially in presence of all possible exotic cases with imports.
After this PR all table changes should be "locally correct" after every update, even if it may be overcautious.
If similar optimizations are introduced again they will need detailed comments explaining why it's legal to do what they do and providing proofs.

Fixes rust-lang#104249.
Fixes rust-lang#104539.

Author: bors

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;
@@ -142,13 +141,13 @@ impl EffectiveVisibilities {
     pub fn set_public_at_level(
         &mut self,
         id: LocalDefId,
-        default_vis: impl FnOnce() -> Visibility,
+        lazy_private_vis: impl FnOnce() -> Visibility,
         level: Level,
     ) {
         let mut effective_vis = self
             .effective_vis(id)
             .copied()
-            .unwrap_or_else(|| EffectiveVisibility::from_vis(default_vis()));
+            .unwrap_or_else(|| EffectiveVisibility::from_vis(lazy_private_vis()));
         for l in Level::all_levels() {
             if l <= level {
                 *effective_vis.at_level_mut(l) = Visibility::Public;
@@ -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,15 +195,15 @@ 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);
-            }
         }
     }
 }
 
+pub trait IntoDefIdTree {
+    type Tree: DefIdTree;
+    fn tree(self) -> Self::Tree;
+}
+
 impl<Id: Eq + Hash> EffectiveVisibilities<Id> {
     pub fn iter(&self) -> impl Iterator<Item = (&Id, &EffectiveVisibility)> {
         self.map.iter()
@@ -215,56 +213,65 @@ impl<Id: Eq + Hash> EffectiveVisibilities<Id> {
         self.map.get(&id)
     }
 
-    // `parent_id` is not necessarily a parent in source code tree,
-    // it is the node from which the maximum effective visibility is inherited.
-    pub fn update(
+    // FIXME: Share code with `fn update`.
+    pub fn effective_vis_or_private(
+        &mut self,
+        id: Id,
+        lazy_private_vis: impl FnOnce() -> Visibility,
+    ) -> &EffectiveVisibility {
+        self.map.entry(id).or_insert_with(|| EffectiveVisibility::from_vis(lazy_private_vis()))
+    }
+
+    pub fn update<T: IntoDefIdTree>(
         &mut self,
         id: Id,
         nominal_vis: Visibility,
-        default_vis: Visibility,
-        inherited_eff_vis: Option<EffectiveVisibility>,
+        lazy_private_vis: impl FnOnce(T) -> (Visibility, T),
+        inherited_effective_vis: EffectiveVisibility,
         level: Level,
-        tree: impl DefIdTree,
+        mut into_tree: T,
     ) -> bool {
         let mut changed = false;
-        let mut current_effective_vis = self
-            .map
-            .get(&id)
-            .copied()
-            .unwrap_or_else(|| EffectiveVisibility::from_vis(default_vis));
-        if let Some(inherited_effective_vis) = inherited_eff_vis {
-            let mut inherited_effective_vis_at_prev_level =
-                *inherited_effective_vis.at_level(level);
-            let mut calculated_effective_vis = inherited_effective_vis_at_prev_level;
-            for l in Level::all_levels() {
-                if level >= l {
-                    let inherited_effective_vis_at_level = *inherited_effective_vis.at_level(l);
-                    let current_effective_vis_at_level = current_effective_vis.at_level_mut(l);
-                    // effective visibility for id shouldn't be recalculated if
-                    // inherited from parent_id effective visibility isn't changed at next level
-                    if !(inherited_effective_vis_at_prev_level == inherited_effective_vis_at_level
-                        && level != l)
-                    {
-                        calculated_effective_vis =
-                            if nominal_vis.is_at_least(inherited_effective_vis_at_level, tree) {
-                                inherited_effective_vis_at_level
-                            } else {
-                                nominal_vis
-                            };
-                    }
-                    // effective visibility can't be decreased at next update call for the
-                    // same id
-                    if *current_effective_vis_at_level != calculated_effective_vis
-                        && calculated_effective_vis
-                            .is_at_least(*current_effective_vis_at_level, tree)
-                    {
-                        changed = true;
-                        *current_effective_vis_at_level = calculated_effective_vis;
-                    }
-                    inherited_effective_vis_at_prev_level = inherited_effective_vis_at_level;
+        let mut current_effective_vis = match self.map.get(&id).copied() {
+            Some(eff_vis) => eff_vis,
+            None => {
+                let private_vis;
+                (private_vis, into_tree) = lazy_private_vis(into_tree);
+                EffectiveVisibility::from_vis(private_vis)
+            }
+        };
+        let tree = into_tree.tree();
+
+        let mut inherited_effective_vis_at_prev_level = *inherited_effective_vis.at_level(level);
+        let mut calculated_effective_vis = inherited_effective_vis_at_prev_level;
+        for l in Level::all_levels() {
+            if level >= l {
+                let inherited_effective_vis_at_level = *inherited_effective_vis.at_level(l);
+                let current_effective_vis_at_level = current_effective_vis.at_level_mut(l);
+                // effective visibility for id shouldn't be recalculated if
+                // inherited from parent_id effective visibility isn't changed at next level
+                if !(inherited_effective_vis_at_prev_level == inherited_effective_vis_at_level
+                    && level != l)
+                {
+                    calculated_effective_vis =
+                        if nominal_vis.is_at_least(inherited_effective_vis_at_level, tree) {
+                            inherited_effective_vis_at_level
+                        } else {
+                            nominal_vis
+                        };
                 }
+                // effective visibility can't be decreased at next update call for the
+                // same id
+                if *current_effective_vis_at_level != calculated_effective_vis
+                    && calculated_effective_vis.is_at_least(*current_effective_vis_at_level, tree)
+                {
+                    changed = true;
+                    *current_effective_vis_at_level = calculated_effective_vis;
+                }
+                inherited_effective_vis_at_prev_level = inherited_effective_vis_at_level;
             }
         }
+
         self.map.insert(id, current_effective_vis);
         changed
     }

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -7,8 +7,10 @@ use rustc_ast::EnumDef;
 use rustc_data_structures::intern::Interned;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::def_id::CRATE_DEF_ID;
-use rustc_middle::middle::privacy::{EffectiveVisibilities, EffectiveVisibility, Level};
-use rustc_middle::ty::Visibility;
+use rustc_middle::middle::privacy::{EffectiveVisibilities, EffectiveVisibility};
+use rustc_middle::middle::privacy::{IntoDefIdTree, Level};
+use rustc_middle::ty::{DefIdTree, Visibility};
+use std::mem;
 
 type ImportId<'a> = Interned<'a, NameBinding<'a>>;
 
@@ -29,31 +31,71 @@ impl ParentId<'_> {
 
 pub struct EffectiveVisibilitiesVisitor<'r, 'a> {
     r: &'r mut Resolver<'a>,
+    def_effective_visibilities: EffectiveVisibilities,
     /// While walking import chains we need to track effective visibilities per-binding, and def id
     /// keys in `Resolver::effective_visibilities` are not enough for that, because multiple
     /// bindings can correspond to a single def id in imports. So we keep a separate table.
     import_effective_visibilities: EffectiveVisibilities<ImportId<'a>>,
+    // It's possible to recalculate this at any point, but it's relatively expensive.
+    current_private_vis: Visibility,
     changed: bool,
 }
 
+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 {
+        // For mod items `nearest_normal_mod` returns its argument, but we actually need its parent.
+        let normal_mod_id = self.nearest_normal_mod(def_id);
+        if normal_mod_id == def_id {
+            self.opt_local_parent(def_id).map_or(Visibility::Public, Visibility::Restricted)
+        } else {
+            Visibility::Restricted(normal_mod_id)
+        }
+    }
+}
+
+impl<'a, 'b> IntoDefIdTree for &'b mut Resolver<'a> {
+    type Tree = &'b Resolver<'a>;
+    fn tree(self) -> Self::Tree {
+        self
+    }
+}
+
 impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> {
     /// Fills the `Resolver::effective_visibilities` table with public & exported items
     /// For now, this doesn't resolve macros (FIXME) and cannot resolve Impl, as we
     /// need access to a TyCtxt for that.
     pub fn compute_effective_visibilities<'c>(r: &'r mut Resolver<'a>, krate: &'c Crate) {
         let mut visitor = EffectiveVisibilitiesVisitor {
             r,
+            def_effective_visibilities: Default::default(),
             import_effective_visibilities: Default::default(),
+            current_private_vis: Visibility::Public,
             changed: false,
         };
 
         visitor.update(CRATE_DEF_ID, CRATE_DEF_ID);
+        visitor.current_private_vis = Visibility::Restricted(CRATE_DEF_ID);
         visitor.set_bindings_effective_visibilities(CRATE_DEF_ID);
 
         while visitor.changed {
             visitor.changed = false;
             visit::walk_crate(&mut visitor, krate);
         }
+        visitor.r.effective_visibilities = visitor.def_effective_visibilities;
 
         // Update visibilities for import def ids. These are not used during the
         // `EffectiveVisibilitiesVisitor` pass, because we have more detailed binding-based
@@ -90,10 +132,6 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> {
         info!("resolve::effective_visibilities: {:#?}", r.effective_visibilities);
     }
 
-    fn nearest_normal_mod(&mut self, def_id: LocalDefId) -> LocalDefId {
-        self.r.get_nearest_non_block_module(def_id.to_def_id()).nearest_parent_mod().expect_local()
-    }
-
     /// Update effective visibilities of bindings in the given module,
     /// including their whole reexport chains.
     fn set_bindings_effective_visibilities(&mut self, module_id: LocalDefId) {
@@ -122,62 +160,47 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> {
         }
     }
 
-    fn effective_vis(&self, parent_id: ParentId<'a>) -> Option<EffectiveVisibility> {
-        match parent_id {
-            ParentId::Def(def_id) => self.r.effective_visibilities.effective_vis(def_id),
-            ParentId::Import(binding) => self.import_effective_visibilities.effective_vis(binding),
-        }
-        .copied()
+    fn cheap_private_vis(&self, parent_id: ParentId<'_>) -> Option<Visibility> {
+        matches!(parent_id, ParentId::Def(_)).then_some(self.current_private_vis)
     }
 
-    /// 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 effective_vis_or_private(&mut self, parent_id: ParentId<'a>) -> EffectiveVisibility {
+        // Private nodes are only added to the table for caching, they could be added or removed at
+        // any moment without consequences, so we don't set `changed` to true when adding them.
+        *match parent_id {
+            ParentId::Def(def_id) => self
+                .def_effective_visibilities
+                .effective_vis_or_private(def_id, || self.r.private_vis_def(def_id)),
+            ParentId::Import(binding) => self
+                .import_effective_visibilities
+                .effective_vis_or_private(binding, || self.r.private_vis_import(binding)),
+        }
     }
 
     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.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;
-        }
+        let private_vis = self.cheap_private_vis(parent_id);
+        let inherited_eff_vis = self.effective_vis_or_private(parent_id);
         self.changed |= self.import_effective_visibilities.update(
             binding,
             nominal_vis,
-            default_vis,
-            self.effective_vis(parent_id),
+            |r| (private_vis.unwrap_or_else(|| r.private_vis_import(binding)), r),
+            inherited_eff_vis,
             parent_id.level(),
-            ResolverTree(&self.r.definitions, &self.r.crate_loader),
+            &mut *self.r,
         );
     }
 
     fn update_def(&mut self, def_id: LocalDefId, nominal_vis: Visibility, parent_id: ParentId<'a>) {
-        let default_vis = Visibility::Restricted(self.nearest_normal_mod(def_id));
-        if self.is_noop_update(parent_id, nominal_vis, default_vis) {
-            return;
-        }
-        self.changed |= self.r.effective_visibilities.update(
+        let private_vis = self.cheap_private_vis(parent_id);
+        let inherited_eff_vis = self.effective_vis_or_private(parent_id);
+        self.changed |= self.def_effective_visibilities.update(
             def_id,
             nominal_vis,
-            if def_id == CRATE_DEF_ID { Visibility::Public } else { default_vis },
-            self.effective_vis(parent_id),
+            |r| (private_vis.unwrap_or_else(|| r.private_vis_def(def_id)), r),
+            inherited_eff_vis,
             parent_id.level(),
-            ResolverTree(&self.r.definitions, &self.r.crate_loader),
+            &mut *self.r,
         );
     }
 
@@ -201,8 +224,11 @@ impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> {
             ),
 
             ast::ItemKind::Mod(..) => {
+                let prev_private_vis =
+                    mem::replace(&mut self.current_private_vis, Visibility::Restricted(def_id));
                 self.set_bindings_effective_visibilities(def_id);
                 visit::walk_item(self, item);
+                self.current_private_vis = prev_private_vis;
             }
 
             ast::ItemKind::Enum(EnumDef { ref variants }, _) => {

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
         }

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,