resolve: Evaluate private visibilities eagerly in eff vis computation

Author: petrochenkov

compiler/rustc_middle/src/middle/privacy.rs

@@ -226,13 +226,13 @@ impl<Id: Eq + Hash> EffectiveVisibilities<Id> {
         &mut self,
         id: Id,
         max_vis: Option<Visibility>,
-        lazy_private_vis: impl FnOnce() -> Visibility,
+        private_vis: Visibility,
         inherited_effective_vis: EffectiveVisibility,
         level: Level,
         tcx: TyCtxt<'_>,
     ) -> bool {
         let mut changed = false;
-        let current_effective_vis = self.effective_vis_or_private(id, lazy_private_vis);
+        let current_effective_vis = self.effective_vis_or_private(id, || private_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;

compiler/rustc_privacy/src/lib.rs

@@ -476,7 +476,7 @@ impl<'tcx> EmbargoVisitor<'tcx> {
             self.changed |= self.effective_visibilities.update(
                 def_id,
                 max_vis,
-                || private_vis,
+                private_vis,
                 inherited_effective_vis,
                 level,
                 self.tcx,

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -38,20 +38,18 @@ pub(crate) struct EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
 }
 
 impl Resolver<'_, '_> {
-    fn nearest_normal_mod(&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(&self, decl: Decl<'_>) -> Visibility {
-        let DeclKind::Import { import, .. } = decl.kind else { unreachable!() };
+    fn private_vis_decl(&self, decl: Decl<'_>) -> Visibility {
         Visibility::Restricted(
-            import.def_id().map(|id| self.nearest_normal_mod(id)).unwrap_or(CRATE_DEF_ID),
+            decl.parent_module.map_or(CRATE_DEF_ID, |m| m.nearest_parent_mod().expect_local()),
         )
     }
 
     fn private_vis_def(&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);
+        // For mod items `normal_mod_id` will be equal to `def_id`, but we actually need its parent.
+        let normal_mod_id = self
+            .get_nearest_non_block_module(def_id.to_def_id())
+            .nearest_parent_mod()
+            .expect_local();
         if normal_mod_id == def_id {
             Visibility::Restricted(self.tcx.local_parent(def_id))
         } else {
@@ -117,15 +115,23 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
             // 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.
+            let orig_private_vis = self.current_private_vis;
             let mut parent_id = ParentId::Def(module_id);
             while let DeclKind::Import { source_decl, .. } = decl.kind {
+                if matches!(parent_id, ParentId::Import(_)) {
+                    self.current_private_vis = self.r.private_vis_decl(decl);
+                }
                 self.update_import(decl, parent_id);
                 parent_id = ParentId::Import(decl);
                 decl = source_decl;
             }
             if let Some(def_id) = decl.res().opt_def_id().and_then(|id| id.as_local()) {
+                if matches!(parent_id, ParentId::Import(_)) {
+                    self.current_private_vis = self.r.private_vis_decl(decl);
+                }
                 self.update_def(def_id, decl.vis().expect_local(), parent_id);
             }
+            self.current_private_vis = orig_private_vis;
         }
     }
 
@@ -138,7 +144,7 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
                 .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)),
+                .effective_vis_or_private(binding, || self.r.private_vis_decl(binding)),
         }
     }
 
@@ -147,34 +153,25 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
     /// So if either parent or nominal visibility is the same as private visibility, then
     /// `min(parent_vis, nominal_vis) <= private_vis`, and the update logic is guaranteed
     /// to not update anything and we can skip it.
-    ///
-    /// We are checking this condition only if the correct value of private visibility is
-    /// cheaply available, otherwise it doesn't make sense performance-wise.
-    ///
-    /// `None` is returned if the update can be skipped,
-    /// and cheap private visibility is returned otherwise.
-    fn may_update(
-        &self,
-        nominal_vis: Visibility,
-        parent_id: ParentId<'_>,
-    ) -> Option<Option<Visibility>> {
-        match parent_id {
-            ParentId::Def(def_id) => (nominal_vis != self.current_private_vis
-                && self.r.tcx.local_visibility(def_id) != self.current_private_vis)
-                .then_some(Some(self.current_private_vis)),
-            ParentId::Import(_) => Some(None),
-        }
+    fn may_update(&self, nominal_vis: Visibility, parent_id: ParentId<'_>) -> bool {
+        nominal_vis != self.current_private_vis
+            && match parent_id {
+                ParentId::Def(def_id) => self.r.tcx.local_visibility(def_id),
+                ParentId::Import(decl) => decl.vis().expect_local(),
+            } != self.current_private_vis
     }
 
     fn update_import(&mut self, decl: Decl<'ra>, parent_id: ParentId<'ra>) {
         let nominal_vis = decl.vis().expect_local();
-        let Some(cheap_private_vis) = self.may_update(nominal_vis, parent_id) else { return };
+        if !self.may_update(nominal_vis, parent_id) {
+            return;
+        };
         let inherited_eff_vis = self.effective_vis_or_private(parent_id);
         let tcx = self.r.tcx;
         self.changed |= self.import_effective_visibilities.update(
             decl,
             Some(nominal_vis),
-            || cheap_private_vis.unwrap_or_else(|| self.r.private_vis_import(decl)),
+            self.current_private_vis,
             inherited_eff_vis,
             parent_id.level(),
             tcx,
@@ -191,13 +188,15 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> {
         nominal_vis: Visibility,
         parent_id: ParentId<'ra>,
     ) {
-        let Some(cheap_private_vis) = self.may_update(nominal_vis, parent_id) else { return };
+        if !self.may_update(nominal_vis, parent_id) {
+            return;
+        };
         let inherited_eff_vis = self.effective_vis_or_private(parent_id);
         let tcx = self.r.tcx;
         self.changed |= self.def_effective_visibilities.update(
             def_id,
             Some(nominal_vis),
-            || cheap_private_vis.unwrap_or_else(|| self.r.private_vis_def(def_id)),
+            self.current_private_vis,
             inherited_eff_vis,
             parent_id.level(),
             tcx,