diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs index 3a91522d36229..fc08d58cc4068 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; @@ -142,13 +141,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; @@ -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,15 +195,15 @@ 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); - } } } } +pub trait IntoDefIdTree { + type Tree: DefIdTree; + fn tree(self) -> Self::Tree; +} + impl EffectiveVisibilities { pub fn iter(&self) -> impl Iterator { self.map.iter() @@ -215,56 +213,65 @@ 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. - pub fn update( + // 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, - default_vis: Visibility, - inherited_eff_vis: Option, + lazy_private_vis: impl FnOnce(T) -> (Visibility, T), + inherited_effective_vis: EffectiveVisibility, 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)); - 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 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(); + + 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 82dcc7efb1baf..3aa8d52db0381 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -7,8 +7,10 @@ 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::ty::Visibility; +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>>; @@ -29,13 +31,49 @@ 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. import_effective_visibilities: EffectiveVisibilities>, + // It's possible to recalculate this at any point, but it's relatively expensive. + current_private_vis: Visibility, 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() + } + + 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 { + // 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(normal_mod_id) + } + } +} + +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,17 +81,21 @@ 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(), + 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 { 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 +132,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) { @@ -122,62 +160,47 @@ 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::Import(binding) => self.import_effective_visibilities.effective_vis(binding), - } - .copied() + fn cheap_private_vis(&self, parent_id: ParentId<'_>) -> Option { + matches!(parent_id, ParentId::Def(_)).then_some(self.current_private_vis) } - /// 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 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)), + } } 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.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; - } + 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, - default_vis, - self.effective_vis(parent_id), + |r| (private_vis.unwrap_or_else(|| r.private_vis_import(binding)), r), + inherited_eff_vis, 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)); - if self.is_noop_update(parent_id, nominal_vis, default_vis) { - return; - } - self.changed |= self.r.effective_visibilities.update( + 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, - if def_id == CRATE_DEF_ID { Visibility::Public } else { default_vis }, - self.effective_vis(parent_id), + |r| (private_vis.unwrap_or_else(|| r.private_vis_def(def_id)), r), + inherited_eff_vis, parent_id.level(), - ResolverTree(&self.r.definitions, &self.r.crate_loader), + &mut *self.r, ); } @@ -201,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 }, _) => { diff --git a/src/test/ui/privacy/effective_visibilities.rs b/src/test/ui/privacy/effective_visibilities.rs index 4479b0d8f61ba..8d0602fa79f02 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 019aaf8086a6a..6a99afe64fee3 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 0000000000000..af5a2bed6ab24 --- /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 {} + 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 0000000000000..fd205f4058ae0 --- /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`.