privacy: Fix more (potential) issues with effective visibilities

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,