From 60c6dc07dedc1c85ebfff54a0d81f3328923a2a8 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 22 Mar 2023 16:58:49 +0400 Subject: [PATCH] resolve: Restore some effective visibility optimizations Something similar was previously removed as a part of #104602, but after this PR all table changes should also be "locally correct" after every update. --- .../src/effective_visibilities.rs | 36 ++++++++++++++----- tests/ui/privacy/effective_visibilities.rs | 6 ++-- .../ui/privacy/effective_visibilities.stderr | 6 ++-- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 3673f603d1676..bed579f6b9258 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -155,10 +155,6 @@ impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> { } } - fn cheap_private_vis(&self, parent_id: ParentId<'_>) -> Option { - matches!(parent_id, ParentId::Def(_)).then_some(self.current_private_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. @@ -172,15 +168,39 @@ impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> { } } + /// 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 + /// 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 does'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.visibilities[&def_id] != self.current_private_vis) + .then_some(Some(self.current_private_vis)), + ParentId::Import(_) => Some(None), + } + } + fn update_import(&mut self, binding: ImportId<'a>, parent_id: ParentId<'a>) { let nominal_vis = binding.vis.expect_local(); - let private_vis = self.cheap_private_vis(parent_id); + let Some(cheap_private_vis) = self.may_update(nominal_vis, parent_id) else { return }; let inherited_eff_vis = self.effective_vis_or_private(parent_id); let tcx = self.r.tcx; self.changed |= self.import_effective_visibilities.update( binding, nominal_vis, - || private_vis.unwrap_or_else(|| self.r.private_vis_import(binding)), + || cheap_private_vis.unwrap_or_else(|| self.r.private_vis_import(binding)), inherited_eff_vis, parent_id.level(), tcx, @@ -188,13 +208,13 @@ impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> { } fn update_def(&mut self, def_id: LocalDefId, nominal_vis: Visibility, parent_id: ParentId<'a>) { - let private_vis = self.cheap_private_vis(parent_id); + let Some(cheap_private_vis) = self.may_update(nominal_vis, parent_id) else { 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, nominal_vis, - || private_vis.unwrap_or_else(|| self.r.private_vis_def(def_id)), + || cheap_private_vis.unwrap_or_else(|| self.r.private_vis_def(def_id)), inherited_eff_vis, parent_id.level(), tcx, diff --git a/tests/ui/privacy/effective_visibilities.rs b/tests/ui/privacy/effective_visibilities.rs index 3e9eef462306c..e9ac931608703 100644 --- a/tests/ui/privacy/effective_visibilities.rs +++ b/tests/ui/privacy/effective_visibilities.rs @@ -18,13 +18,13 @@ mod outer { //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub } #[rustc_effective_visibility] - 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) + struct PrivStruct; //~ ERROR not in the table + //~| ERROR not in the table #[rustc_effective_visibility] pub union PubUnion { //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub #[rustc_effective_visibility] - a: u8, //~ ERROR Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self) + a: u8, //~ ERROR not in the table #[rustc_effective_visibility] pub b: u8, //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub } diff --git a/tests/ui/privacy/effective_visibilities.stderr b/tests/ui/privacy/effective_visibilities.stderr index 2618fc427e917..f74f812e1a0ed 100644 --- a/tests/ui/privacy/effective_visibilities.stderr +++ b/tests/ui/privacy/effective_visibilities.stderr @@ -34,13 +34,13 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl LL | pub trait PubTrait { | ^^^^^^^^^^^^^^^^^^ -error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self) +error: not in the table --> $DIR/effective_visibilities.rs:21:9 | LL | struct PrivStruct; | ^^^^^^^^^^^^^^^^^ -error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self) +error: not in the table --> $DIR/effective_visibilities.rs:21:9 | LL | struct PrivStruct; @@ -52,7 +52,7 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl LL | pub union PubUnion { | ^^^^^^^^^^^^^^^^^^ -error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self) +error: not in the table --> $DIR/effective_visibilities.rs:27:13 | LL | a: u8,