From 7e76d94a225ee53fde0ddbbfd7285890d006db43 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 23 Nov 2022 20:31:35 +0300 Subject: [PATCH] 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 d32c2e990432..fc08d58cc406 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 56959586d117..0f6db93c779c 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, );