From 3f20f4ac42be55f309224ef365dfa2ca64359e07 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 19 Nov 2022 01:52:49 +0300 Subject: [PATCH 1/5] effective visibility: Satisfy borrow checker to use resolver lazily from a closure --- compiler/rustc_middle/src/middle/privacy.rs | 30 +++++++++----- .../src/effective_visibilities.rs | 39 ++++++++++++------- 2 files changed, 46 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs index 3a91522d362..decd213763d 100644 --- a/compiler/rustc_middle/src/middle/privacy.rs +++ b/compiler/rustc_middle/src/middle/privacy.rs @@ -142,13 +142,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; @@ -206,6 +206,11 @@ impl EffectiveVisibilities { } } +pub trait IntoDefIdTree { + type Tree: DefIdTree; + fn tree(self) -> Self::Tree; +} + impl EffectiveVisibilities { pub fn iter(&self) -> impl Iterator { self.map.iter() @@ -217,21 +222,26 @@ impl EffectiveVisibilities { // `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( + pub fn update( &mut self, id: Id, nominal_vis: Visibility, - default_vis: Visibility, + lazy_private_vis: impl FnOnce(T) -> (Visibility, T), inherited_eff_vis: Option, 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)); + 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(); + if let Some(inherited_effective_vis) = inherited_eff_vis { let mut inherited_effective_vis_at_prev_level = *inherited_effective_vis.at_level(level); diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 82dcc7efb1b..57fb1fa9389 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -7,7 +7,8 @@ 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::middle::privacy::{EffectiveVisibilities, EffectiveVisibility}; +use rustc_middle::middle::privacy::{IntoDefIdTree, Level}; use rustc_middle::ty::Visibility; type ImportId<'a> = Interned<'a, NameBinding<'a>>; @@ -29,6 +30,7 @@ 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. @@ -36,6 +38,19 @@ pub struct EffectiveVisibilitiesVisitor<'r, 'a> { 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() + } +} + +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 @@ -43,6 +58,7 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { 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(), changed: false, }; @@ -54,6 +70,7 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { 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 +107,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) { @@ -124,7 +137,7 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { fn effective_vis(&self, parent_id: ParentId<'a>) -> Option { match parent_id { - ParentId::Def(def_id) => self.r.effective_visibilities.effective_vis(def_id), + ParentId::Def(def_id) => self.def_effective_visibilities.effective_vis(def_id), ParentId::Import(binding) => self.import_effective_visibilities.effective_vis(binding), } .copied() @@ -150,7 +163,7 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { let default_vis = Visibility::Restricted( import .id() - .map(|id| self.nearest_normal_mod(self.r.local_def_id(id))) + .map(|id| self.r.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) { @@ -159,25 +172,25 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { self.changed |= self.import_effective_visibilities.update( binding, nominal_vis, - default_vis, + |r| (default_vis, r), self.effective_vis(parent_id), 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)); + let default_vis = Visibility::Restricted(self.r.nearest_normal_mod(def_id)); if self.is_noop_update(parent_id, nominal_vis, default_vis) { return; } - self.changed |= self.r.effective_visibilities.update( + self.changed |= self.def_effective_visibilities.update( def_id, nominal_vis, - if def_id == CRATE_DEF_ID { Visibility::Public } else { default_vis }, + |r| (if def_id == CRATE_DEF_ID { Visibility::Public } else { default_vis }, r), self.effective_vis(parent_id), parent_id.level(), - ResolverTree(&self.r.definitions, &self.r.crate_loader), + &mut *self.r, ); } From f0843b89d1336962e9cb0572a40a790cd60ef4d9 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 23 Nov 2022 19:19:06 +0300 Subject: [PATCH 2/5] effective visibility: Remove questionable optimizations First, they require eagerly calculating private visibility (current normal module), which is somewhat expensive. Private visibilities are also lost once calculated, instead of being cached in the table. Second, I cannot prove that the optimizations are correct. Maybe they can be partially reinstated in the future in cases when it's cheap and provably correct to do them. They will also probably be merged into `fn update` in that case. Partially fixes https://github.com/rust-lang/rust/issues/104249 Fixes https://github.com/rust-lang/rust/issues/104539 --- compiler/rustc_middle/src/middle/privacy.rs | 7 --- .../src/effective_visibilities.rs | 50 ++++++++----------- src/test/ui/privacy/effective_visibilities.rs | 6 +-- .../ui/privacy/effective_visibilities.stderr | 6 +-- .../effective_visibilities_invariants.rs | 12 +++++ .../effective_visibilities_invariants.stderr | 32 ++++++++++++ 6 files changed, 70 insertions(+), 43 deletions(-) create mode 100644 src/test/ui/privacy/effective_visibilities_invariants.rs create mode 100644 src/test/ui/privacy/effective_visibilities_invariants.stderr diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs index decd213763d..d32c2e99043 100644 --- a/compiler/rustc_middle/src/middle/privacy.rs +++ b/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; @@ -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,11 +195,6 @@ 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); - } } } } diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 57fb1fa9389..32ab58b459a 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -42,6 +42,24 @@ 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 { + if def_id == CRATE_DEF_ID { + Visibility::Public + } else { + Visibility::Restricted(self.nearest_normal_mod(def_id)) + } + } } impl<'a, 'b> IntoDefIdTree for &'b mut Resolver<'a> { @@ -143,36 +161,12 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { .copied() } - /// 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 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.r.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; - } self.changed |= self.import_effective_visibilities.update( binding, nominal_vis, - |r| (default_vis, r), + |r| (r.private_vis_import(binding), r), self.effective_vis(parent_id), parent_id.level(), &mut *self.r, @@ -180,14 +174,10 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { } fn update_def(&mut self, def_id: LocalDefId, nominal_vis: Visibility, parent_id: ParentId<'a>) { - let default_vis = Visibility::Restricted(self.r.nearest_normal_mod(def_id)); - if self.is_noop_update(parent_id, nominal_vis, default_vis) { - return; - } self.changed |= self.def_effective_visibilities.update( def_id, nominal_vis, - |r| (if def_id == CRATE_DEF_ID { Visibility::Public } else { default_vis }, r), + |r| (r.private_vis_def(def_id), r), self.effective_vis(parent_id), parent_id.level(), &mut *self.r, diff --git a/src/test/ui/privacy/effective_visibilities.rs b/src/test/ui/privacy/effective_visibilities.rs index 4479b0d8f61..8d0602fa79f 100644 --- a/src/test/ui/privacy/effective_visibilities.rs +++ b/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 } diff --git a/src/test/ui/privacy/effective_visibilities.stderr b/src/test/ui/privacy/effective_visibilities.stderr index 019aaf8086a..6a99afe64fe 100644 --- a/src/test/ui/privacy/effective_visibilities.stderr +++ b/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, diff --git a/src/test/ui/privacy/effective_visibilities_invariants.rs b/src/test/ui/privacy/effective_visibilities_invariants.rs new file mode 100644 index 00000000000..8c524d32815 --- /dev/null +++ b/src/test/ui/privacy/effective_visibilities_invariants.rs @@ -0,0 +1,12 @@ +// Invariant checking doesn't ICE in some cases with errors (issue #104249). + +#![feature(staged_api)] //~ ERROR module has missing stability attribute + +pub mod m {} //~ ERROR module has missing stability attribute + +pub mod m { //~ ERROR the name `m` is defined multiple times + // mod inner {} - ICE + type Inner = u8; +} + +fn main() {} diff --git a/src/test/ui/privacy/effective_visibilities_invariants.stderr b/src/test/ui/privacy/effective_visibilities_invariants.stderr new file mode 100644 index 00000000000..fd205f4058a --- /dev/null +++ b/src/test/ui/privacy/effective_visibilities_invariants.stderr @@ -0,0 +1,32 @@ +error[E0428]: the name `m` is defined multiple times + --> $DIR/effective_visibilities_invariants.rs:7:1 + | +LL | pub mod m {} + | --------- previous definition of the module `m` here +LL | +LL | pub mod m { + | ^^^^^^^^^ `m` redefined here + | + = note: `m` must be defined only once in the type namespace of this module + +error: module has missing stability attribute + --> $DIR/effective_visibilities_invariants.rs:3:1 + | +LL | / #![feature(staged_api)] +LL | | +LL | | pub mod m {} +LL | | +... | +LL | | +LL | | fn main() {} + | |____________^ + +error: module has missing stability attribute + --> $DIR/effective_visibilities_invariants.rs:5:1 + | +LL | pub mod m {} + | ^^^^^^^^^^^^ + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0428`. From a45a302be536126accf29764b6a3dca39d57d0fe Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 23 Nov 2022 20:13:44 +0300 Subject: [PATCH 3/5] effective visibility: Fix private visibility calculation for modules Optimizations removed in the previous commit required this function to behave incorrectly, but now those optimizations are gone so we can fix the bug. Fixes https://github.com/rust-lang/rust/issues/104249 --- compiler/rustc_resolve/src/effective_visibilities.rs | 10 ++++++---- .../ui/privacy/effective_visibilities_invariants.rs | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 32ab58b459a..56959586d11 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -9,7 +9,7 @@ use rustc_hir::def_id::LocalDefId; use rustc_hir::def_id::CRATE_DEF_ID; use rustc_middle::middle::privacy::{EffectiveVisibilities, EffectiveVisibility}; use rustc_middle::middle::privacy::{IntoDefIdTree, Level}; -use rustc_middle::ty::Visibility; +use rustc_middle::ty::{DefIdTree, Visibility}; type ImportId<'a> = Interned<'a, NameBinding<'a>>; @@ -54,10 +54,12 @@ impl Resolver<'_> { } fn private_vis_def(&mut self, def_id: LocalDefId) -> Visibility { - if def_id == CRATE_DEF_ID { - Visibility::Public + // 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(self.nearest_normal_mod(def_id)) + Visibility::Restricted(normal_mod_id) } } } diff --git a/src/test/ui/privacy/effective_visibilities_invariants.rs b/src/test/ui/privacy/effective_visibilities_invariants.rs index 8c524d32815..af5a2bed6ab 100644 --- a/src/test/ui/privacy/effective_visibilities_invariants.rs +++ b/src/test/ui/privacy/effective_visibilities_invariants.rs @@ -5,7 +5,7 @@ pub mod m {} //~ ERROR module has missing stability attribute pub mod m { //~ ERROR the name `m` is defined multiple times - // mod inner {} - ICE + mod inner {} type Inner = u8; } From 7e76d94a225ee53fde0ddbbfd7285890d006db43 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 23 Nov 2022 20:31:35 +0300 Subject: [PATCH 4/5] effective visibility: Always add table entries for nodes used as parents Previously if the parent was not in the table, and there was nothing to inherit from, the child's private visibility was used, but that's not correct - the parent may have a larger visibility so we should set it to at least the parent's private visibility. That parent's private visibility is also inserted into the table for caching, so it's not recalculated later if used again. --- compiler/rustc_middle/src/middle/privacy.rs | 70 ++++++++++--------- .../src/effective_visibilities.rs | 21 ++++-- 2 files changed, 51 insertions(+), 40 deletions(-) diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs index d32c2e99043..fc08d58cc40 100644 --- a/compiler/rustc_middle/src/middle/privacy.rs +++ b/compiler/rustc_middle/src/middle/privacy.rs @@ -213,14 +213,21 @@ impl EffectiveVisibilities { 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. + // 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( &mut self, id: Id, nominal_vis: Visibility, lazy_private_vis: impl FnOnce(T) -> (Visibility, T), - inherited_eff_vis: Option, + inherited_effective_vis: EffectiveVisibility, level: Level, mut into_tree: T, ) -> bool { @@ -235,39 +242,36 @@ impl EffectiveVisibilities { }; let tree = into_tree.tree(); - 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 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 } diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 56959586d11..0f6db93c779 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -155,32 +155,39 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { } } - fn effective_vis(&self, parent_id: ParentId<'a>) -> Option { - match parent_id { - ParentId::Def(def_id) => self.def_effective_visibilities.effective_vis(def_id), - ParentId::Import(binding) => self.import_effective_visibilities.effective_vis(binding), + 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)), } - .copied() } fn update_import(&mut self, binding: ImportId<'a>, parent_id: ParentId<'a>) { let nominal_vis = binding.vis.expect_local(); + let inherited_eff_vis = self.effective_vis_or_private(parent_id); self.changed |= self.import_effective_visibilities.update( binding, nominal_vis, |r| (r.private_vis_import(binding), r), - self.effective_vis(parent_id), + inherited_eff_vis, parent_id.level(), &mut *self.r, ); } fn update_def(&mut self, def_id: LocalDefId, nominal_vis: Visibility, parent_id: ParentId<'a>) { + let inherited_eff_vis = self.effective_vis_or_private(parent_id); self.changed |= self.def_effective_visibilities.update( def_id, nominal_vis, |r| (r.private_vis_def(def_id), r), - self.effective_vis(parent_id), + inherited_eff_vis, parent_id.level(), &mut *self.r, ); From 47cd844468a45ea3dde60b2d19c282639e2e44fc Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Thu, 24 Nov 2022 01:30:58 +0300 Subject: [PATCH 5/5] effective visibility: Stop recalculating current private visibility It becomes relatively expensive if done often and shows up during perf profiling. --- .../src/effective_visibilities.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 0f6db93c779..3aa8d52db03 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -10,6 +10,7 @@ use rustc_hir::def_id::CRATE_DEF_ID; 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>>; @@ -35,6 +36,8 @@ pub struct EffectiveVisibilitiesVisitor<'r, 'a> { /// 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>, + // It's possible to recalculate this at any point, but it's relatively expensive. + current_private_vis: Visibility, changed: bool, } @@ -80,10 +83,12 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { 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 { @@ -155,6 +160,10 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { } } + 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. @@ -170,11 +179,12 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { 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 inherited_eff_vis = self.effective_vis_or_private(parent_id); self.changed |= self.import_effective_visibilities.update( binding, nominal_vis, - |r| (r.private_vis_import(binding), r), + |r| (private_vis.unwrap_or_else(|| r.private_vis_import(binding)), r), inherited_eff_vis, parent_id.level(), &mut *self.r, @@ -182,11 +192,12 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { } 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 inherited_eff_vis = self.effective_vis_or_private(parent_id); self.changed |= self.def_effective_visibilities.update( def_id, nominal_vis, - |r| (r.private_vis_def(def_id), r), + |r| (private_vis.unwrap_or_else(|| r.private_vis_def(def_id)), r), inherited_eff_vis, parent_id.level(), &mut *self.r, @@ -213,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 }, _) => {