From cc630bc47048c4bb00a2b9478f156ed9113b81f5 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Thu, 7 May 2026 20:40:29 +0300 Subject: [PATCH] resolve: Evaluate private visibilities eagerly in eff vis computation --- compiler/rustc_middle/src/middle/privacy.rs | 4 +- compiler/rustc_privacy/src/lib.rs | 2 +- .../src/effective_visibilities.rs | 69 ++++++++++--------- 3 files changed, 39 insertions(+), 36 deletions(-) diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs index 96588fc640f44..0359264d3f787 100644 --- a/compiler/rustc_middle/src/middle/privacy.rs +++ b/compiler/rustc_middle/src/middle/privacy.rs @@ -226,13 +226,13 @@ impl EffectiveVisibilities { &mut self, id: Id, max_vis: Option, - 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; diff --git a/compiler/rustc_privacy/src/lib.rs b/compiler/rustc_privacy/src/lib.rs index ba30dfccc33cc..d92944d1b31fa 100644 --- a/compiler/rustc_privacy/src/lib.rs +++ b/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, diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 693e49995c1cc..f3b47f04c90a6 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/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,14 +115,19 @@ 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 priv_vis = |this: &Self, parent_id, decl| match parent_id { + ParentId::Def(_) => this.current_private_vis, + ParentId::Import(_) => this.r.private_vis_decl(decl), + }; let mut parent_id = ParentId::Def(module_id); while let DeclKind::Import { source_decl, .. } = decl.kind { - self.update_import(decl, parent_id); + self.update_import(decl, parent_id, priv_vis(self, parent_id, decl)); parent_id = ParentId::Import(decl); decl = source_decl; } if let Some(def_id) = decl.res().opt_def_id().and_then(|id| id.as_local()) { - self.update_def(def_id, decl.vis().expect_local(), parent_id); + let priv_vis = priv_vis(self, parent_id, decl); + self.update_def(def_id, decl.vis().expect_local(), parent_id, priv_vis); } } } @@ -138,50 +141,46 @@ 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)), } } /// All effective visibilities for a node are larger or equal than private visibility /// for that node (see `check_invariants` in middle/privacy.rs). /// 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 + /// `min(parent_vis, nominal_vis) <= priv_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> { - 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), - } + priv_vis: Visibility, + ) -> bool { + nominal_vis != priv_vis + && match parent_id { + ParentId::Def(def_id) => self.r.tcx.local_visibility(def_id), + ParentId::Import(decl) => decl.vis().expect_local(), + } != priv_vis } - fn update_import(&mut self, decl: Decl<'ra>, parent_id: ParentId<'ra>) { + fn update_import(&mut self, decl: Decl<'ra>, parent_id: ParentId<'ra>, priv_vis: Visibility) { 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, priv_vis) { + 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)), + priv_vis, inherited_eff_vis, parent_id.level(), tcx, ); if let Some(max_vis_decl) = decl.ambiguity_vis_max.get() { // Avoid the most visible import in an ambiguous glob set being reported as unused. - self.update_import(max_vis_decl, parent_id); + self.update_import(max_vis_decl, parent_id, priv_vis); } } @@ -190,14 +189,17 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> { def_id: LocalDefId, nominal_vis: Visibility, parent_id: ParentId<'ra>, + priv_vis: Visibility, ) { - let Some(cheap_private_vis) = self.may_update(nominal_vis, parent_id) else { return }; + if !self.may_update(nominal_vis, parent_id, priv_vis) { + 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)), + priv_vis, inherited_eff_vis, parent_id.level(), tcx, @@ -205,7 +207,8 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> { } fn update_field(&mut self, def_id: LocalDefId, parent_id: LocalDefId) { - self.update_def(def_id, self.r.tcx.local_visibility(def_id), ParentId::Def(parent_id)); + let nominal_vis = self.r.tcx.local_visibility(def_id); + self.update_def(def_id, nominal_vis, ParentId::Def(parent_id), self.current_private_vis); } }