From 24093fc6bd442be45a3ddb935b7056ab77cf96f5 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 28 Oct 2022 14:58:21 +0400 Subject: [PATCH 1/4] resolve: More detailed effective visibility tracking for imports Also drop `extern` blocks from the effective visibility table, they are nominally private and it doesn't make sense to keep them there. --- compiler/rustc_middle/src/middle/privacy.rs | 43 ++-- .../src/effective_visibilities.rs | 205 +++++++++++------- compiler/rustc_resolve/src/lib.rs | 19 +- src/test/ui/privacy/effective_visibilities.rs | 2 +- .../ui/privacy/effective_visibilities.stderr | 2 +- .../ui/privacy/effective_visibilities_glob.rs | 21 ++ .../effective_visibilities_glob.stderr | 26 +++ 7 files changed, 212 insertions(+), 106 deletions(-) create mode 100644 src/test/ui/privacy/effective_visibilities_glob.rs create mode 100644 src/test/ui/privacy/effective_visibilities_glob.stderr diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs index ffbd6d10da6b2..3d7a379c56cd1 100644 --- a/compiler/rustc_middle/src/middle/privacy.rs +++ b/compiler/rustc_middle/src/middle/privacy.rs @@ -7,6 +7,7 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_macros::HashStable; use rustc_query_system::ich::StableHashingContext; use rustc_span::def_id::LocalDefId; +use std::hash::Hash; /// Represents the levels of effective visibility an item can have. /// @@ -74,9 +75,9 @@ impl EffectiveVisibility { } /// Holds a map of effective visibilities for reachable HIR nodes. -#[derive(Default, Clone, Debug)] -pub struct EffectiveVisibilities { - map: FxHashMap, +#[derive(Clone, Debug)] +pub struct EffectiveVisibilities { + map: FxHashMap, } impl EffectiveVisibilities { @@ -111,10 +112,6 @@ impl EffectiveVisibilities { }) } - pub fn effective_vis(&self, id: LocalDefId) -> Option<&EffectiveVisibility> { - self.map.get(&id) - } - pub fn iter(&self) -> impl Iterator { self.map.iter() } @@ -136,27 +133,31 @@ impl EffectiveVisibilities { } self.map.insert(id, effective_vis); } +} + +impl EffectiveVisibilities { + pub fn effective_vis(&self, id: Id) -> Option<&EffectiveVisibility> { + 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( &mut self, - id: LocalDefId, + id: Id, nominal_vis: Visibility, - default_vis: impl FnOnce() -> Visibility, - parent_id: LocalDefId, + default_vis: Visibility, + inherited_eff_vis: Option, level: Level, tree: impl DefIdTree, ) -> bool { let mut changed = false; - let mut current_effective_vis = self.effective_vis(id).copied().unwrap_or_else(|| { - if id.is_top_level_module() { - EffectiveVisibility::from_vis(Visibility::Public) - } else { - EffectiveVisibility::from_vis(default_vis()) - } - }); - if let Some(inherited_effective_vis) = self.effective_vis(parent_id) { + 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; @@ -194,6 +195,12 @@ impl EffectiveVisibilities { } } +impl Default for EffectiveVisibilities { + fn default() -> Self { + EffectiveVisibilities { map: Default::default() } + } +} + impl<'a> HashStable> for EffectiveVisibilities { fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) { let EffectiveVisibilities { ref map } = *self; diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 17ce854cb4388..56dde6f8ca74e 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -1,16 +1,38 @@ -use crate::{ImportKind, NameBindingKind, Resolver}; +use crate::{ImportKind, NameBinding, NameBindingKind, Resolver, ResolverTree}; use rustc_ast::ast; use rustc_ast::visit; use rustc_ast::visit::Visitor; use rustc_ast::Crate; 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::Level; -use rustc_middle::ty::{DefIdTree, Visibility}; +use rustc_middle::middle::privacy::{EffectiveVisibilities, EffectiveVisibility, Level}; +use rustc_middle::ty::Visibility; + +type ImportId<'a> = Interned<'a, NameBinding<'a>>; + +#[derive(Clone, Copy)] +enum ParentId<'a> { + Def(LocalDefId), + Import(ImportId<'a>), +} + +impl ParentId<'_> { + fn level(self) -> Level { + match self { + ParentId::Def(_) => Level::Direct, + ParentId::Import(_) => Level::Reexported, + } + } +} pub struct EffectiveVisibilitiesVisitor<'r, 'a> { r: &'r mut Resolver<'a>, + /// 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>, changed: bool, } @@ -19,21 +41,25 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { /// For now, this doesn't resolve macros (FIXME) and cannot resolve Impl, as we /// need access to a TyCtxt for that. pub fn compute_effective_visibilities<'c>(r: &'r mut Resolver<'a>, krate: &'c Crate) { - let mut visitor = EffectiveVisibilitiesVisitor { r, changed: false }; + let mut visitor = EffectiveVisibilitiesVisitor { + r, + import_effective_visibilities: Default::default(), + changed: false, + }; - visitor.update(CRATE_DEF_ID, Visibility::Public, CRATE_DEF_ID, Level::Direct); + visitor.update(CRATE_DEF_ID, CRATE_DEF_ID); visitor.set_bindings_effective_visibilities(CRATE_DEF_ID); while visitor.changed { - visitor.reset(); + visitor.changed = false; visit::walk_crate(&mut visitor, krate); } info!("resolve::effective_visibilities: {:#?}", r.effective_visibilities); } - fn reset(&mut self) { - self.changed = false; + 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, @@ -48,92 +74,114 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { // 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. - - // FIXME: tag and is_public() condition should be removed, but assertions occur. - let tag = if binding.is_import() { Level::Reexported } else { Level::Direct }; - if binding.vis.is_public() { - let mut prev_parent_id = module_id; - let mut level = Level::Direct; - while let NameBindingKind::Import { binding: nested_binding, import, .. } = - binding.kind - { + let mut parent_id = ParentId::Def(module_id); + while let NameBindingKind::Import { binding: nested_binding, import, .. } = + binding.kind + { + let binding_id = ImportId::new_unchecked(binding); + self.update_import(binding_id, parent_id); + + // Update visibilities for import ids. These are not used during this pass, + // because we have more detailed binding-based information, but are used by + // later passes. Effective visibility of an import def id is the maximum value + // among visibilities of bindings corresponding to that def id. + if let Some(node_id) = import.id() { let mut update = |node_id| { - self.update( + self.update_def( self.r.local_def_id(node_id), binding.vis.expect_local(), - prev_parent_id, - level, + parent_id, ) }; - match import.kind { - ImportKind::Single { id, additional_ids, .. } => { - // In theory all the import IDs have individual visibilities and - // effective visibilities, but in practice these IDs go straigth to - // HIR where all their few uses assume that their (effective) - // visibility applies to the whole syntactic `use` item. So we - // update them all to the maximum value among the potential - // individual effective visibilities. Maybe HIR for imports - // shouldn't use three IDs at all. - update(id); - update(additional_ids.0); - update(additional_ids.1); - prev_parent_id = self.r.local_def_id(id); - } - ImportKind::Glob { id, .. } | ImportKind::ExternCrate { id, .. } => { - update(id); - prev_parent_id = self.r.local_def_id(id); + update(node_id); + if let ImportKind::Single { additional_ids: (id1, id2), .. } = import.kind { + // In theory all the single import IDs have individual visibilities and + // effective visibilities, but in practice these IDs go straigth to HIR + // where all their few uses assume that their (effective) visibility + // applies to the whole syntactic `use` item. So they all get the same + // value which is the maximum of all bindings. Maybe HIR for imports + // shouldn't use three IDs at all. + if id1 != ast::DUMMY_NODE_ID { + update(id1); } - ImportKind::MacroUse => { - // In theory we should reset the parent id to something private - // here, but `macro_use` imports always refer to external items, - // so it doesn't matter and we can just do nothing. - } - ImportKind::MacroExport => { - // In theory we should reset the parent id to something public - // here, but it has the same effect as leaving the previous parent, - // so we can just do nothing. + if id2 != ast::DUMMY_NODE_ID { + update(id2); } } - - level = Level::Reexported; - binding = nested_binding; } + + parent_id = ParentId::Import(binding_id); + binding = nested_binding; } if let Some(def_id) = binding.res().opt_def_id().and_then(|id| id.as_local()) { - self.update(def_id, binding.vis.expect_local(), module_id, tag); + self.update_def(def_id, binding.vis.expect_local(), parent_id); } } } } - fn update( - &mut self, - def_id: LocalDefId, + 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() + } + + /// 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, - parent_id: LocalDefId, - tag: Level, - ) { - let module_id = self - .r - .get_nearest_non_block_module(def_id.to_def_id()) - .nearest_parent_mod() - .expect_local(); - if nominal_vis == Visibility::Restricted(module_id) - || self.r.visibilities[&parent_id] == Visibility::Restricted(module_id) - { + 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.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, + default_vis, + self.effective_vis(parent_id), + parent_id.level(), + ResolverTree(&self.r.definitions, &self.r.crate_loader), + ); + } + + 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; } - let mut effective_visibilities = std::mem::take(&mut self.r.effective_visibilities); - self.changed |= effective_visibilities.update( + self.changed |= self.r.effective_visibilities.update( def_id, nominal_vis, - || Visibility::Restricted(module_id), - parent_id, - tag, - &*self.r, + if def_id == CRATE_DEF_ID { Visibility::Public } else { default_vis }, + self.effective_vis(parent_id), + parent_id.level(), + ResolverTree(&self.r.definitions, &self.r.crate_loader), ); - self.r.effective_visibilities = effective_visibilities; + } + + fn update(&mut self, def_id: LocalDefId, parent_id: LocalDefId) { + self.update_def(def_id, self.r.visibilities[&def_id], ParentId::Def(parent_id)); } } @@ -151,12 +199,6 @@ impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> { "ast::ItemKind::MacCall encountered, this should not anymore appear at this stage" ), - // Foreign modules inherit level from parents. - ast::ItemKind::ForeignMod(..) => { - let parent_id = self.r.local_parent(def_id); - self.update(def_id, Visibility::Public, parent_id, Level::Direct); - } - ast::ItemKind::Mod(..) => { self.set_bindings_effective_visibilities(def_id); visit::walk_item(self, item); @@ -167,18 +209,14 @@ impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> { for variant in variants { let variant_def_id = self.r.local_def_id(variant.id); for field in variant.data.fields() { - let field_def_id = self.r.local_def_id(field.id); - let vis = self.r.visibilities[&field_def_id]; - self.update(field_def_id, vis, variant_def_id, Level::Direct); + self.update(self.r.local_def_id(field.id), variant_def_id); } } } ast::ItemKind::Struct(ref def, _) | ast::ItemKind::Union(ref def, _) => { for field in def.fields() { - let field_def_id = self.r.local_def_id(field.id); - let vis = self.r.visibilities[&field_def_id]; - self.update(field_def_id, vis, def_id, Level::Direct); + self.update(self.r.local_def_id(field.id), def_id); } } @@ -194,6 +232,7 @@ impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> { | ast::ItemKind::TyAlias(..) | ast::ItemKind::TraitAlias(..) | ast::ItemKind::MacroDef(..) + | ast::ItemKind::ForeignMod(..) | ast::ItemKind::Fn(..) => return, } } diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index ee1c97d5ad2b7..a1ff477c6fefb 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -1106,17 +1106,30 @@ impl<'a> AsMut> for Resolver<'a> { } } -impl<'a, 'b> DefIdTree for &'a Resolver<'b> { +/// A minimal subset of resolver that can implemenent `DefIdTree`, sometimes +/// required to satisfy borrow checker by avoiding borrowing the whole resolver. +#[derive(Clone, Copy)] +struct ResolverTree<'a, 'b>(&'a Definitions, &'a CrateLoader<'b>); + +impl DefIdTree for ResolverTree<'_, '_> { #[inline] fn opt_parent(self, id: DefId) -> Option { + let ResolverTree(definitions, crate_loader) = self; match id.as_local() { - Some(id) => self.definitions.def_key(id).parent, - None => self.cstore().def_key(id).parent, + Some(id) => definitions.def_key(id).parent, + None => crate_loader.cstore().def_key(id).parent, } .map(|index| DefId { index, ..id }) } } +impl<'a, 'b> DefIdTree for &'a Resolver<'b> { + #[inline] + fn opt_parent(self, id: DefId) -> Option { + ResolverTree(&self.definitions, &self.crate_loader).opt_parent(id) + } +} + impl Resolver<'_> { fn opt_local_def_id(&self, node: NodeId) -> Option { self.node_id_to_def_id.get(&node).copied() diff --git a/src/test/ui/privacy/effective_visibilities.rs b/src/test/ui/privacy/effective_visibilities.rs index c1f9ee8dfdf73..187bd75933048 100644 --- a/src/test/ui/privacy/effective_visibilities.rs +++ b/src/test/ui/privacy/effective_visibilities.rs @@ -6,7 +6,7 @@ mod outer { //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub pub mod inner1 { //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub #[rustc_effective_visibility] - extern "C" {} //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub + extern "C" {} //~ ERROR not in the table #[rustc_effective_visibility] pub trait PubTrait { //~ 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 5a8f7db38fc8a..10ed14aa15035 100644 --- a/src/test/ui/privacy/effective_visibilities.stderr +++ b/src/test/ui/privacy/effective_visibilities.stderr @@ -10,7 +10,7 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl LL | pub mod inner1 { | ^^^^^^^^^^^^^^ -error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub +error: not in the table --> $DIR/effective_visibilities.rs:9:9 | LL | extern "C" {} diff --git a/src/test/ui/privacy/effective_visibilities_glob.rs b/src/test/ui/privacy/effective_visibilities_glob.rs new file mode 100644 index 0000000000000..eb9dcd6cd1fa4 --- /dev/null +++ b/src/test/ui/privacy/effective_visibilities_glob.rs @@ -0,0 +1,21 @@ +// Effective visibility tracking for imports is fine-grained, so `S2` is not fully exported +// even if its parent import (`m::*`) is fully exported as a `use` item. + +#![feature(rustc_attrs)] + +mod m { + #[rustc_effective_visibility] + pub struct S1 {} //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub + #[rustc_effective_visibility] + pub struct S2 {} //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub(crate), ReachableThroughImplTrait: pub(crate) +} + +mod glob { + #[rustc_effective_visibility] + pub use crate::m::*; //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub +} + +#[rustc_effective_visibility] +pub use glob::S1; //~ ERROR Direct: pub, Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub + +fn main() {} diff --git a/src/test/ui/privacy/effective_visibilities_glob.stderr b/src/test/ui/privacy/effective_visibilities_glob.stderr new file mode 100644 index 0000000000000..0496cd5df8db0 --- /dev/null +++ b/src/test/ui/privacy/effective_visibilities_glob.stderr @@ -0,0 +1,26 @@ +error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub + --> $DIR/effective_visibilities_glob.rs:8:5 + | +LL | pub struct S1 {} + | ^^^^^^^^^^^^^ + +error: Direct: pub(crate), Reexported: pub(crate), Reachable: pub(crate), ReachableThroughImplTrait: pub(crate) + --> $DIR/effective_visibilities_glob.rs:10:5 + | +LL | pub struct S2 {} + | ^^^^^^^^^^^^^ + +error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub + --> $DIR/effective_visibilities_glob.rs:15:13 + | +LL | pub use crate::m::*; + | ^^^^^^^^ + +error: Direct: pub, Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub + --> $DIR/effective_visibilities_glob.rs:19:9 + | +LL | pub use glob::S1; + | ^^^^^^^^ + +error: aborting due to 4 previous errors + From bb401bd04d6239932c37a8f741fefb278f009a1b Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 4 Nov 2022 16:28:03 +0400 Subject: [PATCH 2/4] privacy: Print effective visibilities of constructors --- compiler/rustc_privacy/src/lib.rs | 8 ++++ src/test/ui/privacy/effective_visibilities.rs | 2 + .../ui/privacy/effective_visibilities.stderr | 44 ++++++++++++------- 3 files changed, 38 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_privacy/src/lib.rs b/compiler/rustc_privacy/src/lib.rs index 865d6306bd349..bda3b55cfc33f 100644 --- a/compiler/rustc_privacy/src/lib.rs +++ b/compiler/rustc_privacy/src/lib.rs @@ -959,6 +959,10 @@ impl<'tcx, 'a> Visitor<'tcx> for TestReachabilityVisitor<'tcx, 'a> { for variant in def.variants.iter() { let variant_id = self.tcx.hir().local_def_id(variant.id); self.effective_visibility_diagnostic(variant_id); + if let Some(ctor_hir_id) = variant.data.ctor_hir_id() { + let ctor_def_id = self.tcx.hir().local_def_id(ctor_hir_id); + self.effective_visibility_diagnostic(ctor_def_id); + } for field in variant.data.fields() { let def_id = self.tcx.hir().local_def_id(field.hir_id); self.effective_visibility_diagnostic(def_id); @@ -966,6 +970,10 @@ impl<'tcx, 'a> Visitor<'tcx> for TestReachabilityVisitor<'tcx, 'a> { } } hir::ItemKind::Struct(ref def, _) | hir::ItemKind::Union(ref def, _) => { + if let Some(ctor_hir_id) = def.ctor_hir_id() { + let ctor_def_id = self.tcx.hir().local_def_id(ctor_hir_id); + self.effective_visibility_diagnostic(ctor_def_id); + } for field in def.fields() { let def_id = self.tcx.hir().local_def_id(field.hir_id); self.effective_visibility_diagnostic(def_id); diff --git a/src/test/ui/privacy/effective_visibilities.rs b/src/test/ui/privacy/effective_visibilities.rs index 187bd75933048..4479b0d8f61ba 100644 --- a/src/test/ui/privacy/effective_visibilities.rs +++ b/src/test/ui/privacy/effective_visibilities.rs @@ -18,6 +18,7 @@ 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 #[rustc_effective_visibility] pub union PubUnion { //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub @@ -31,6 +32,7 @@ mod outer { //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub pub enum Enum { //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub #[rustc_effective_visibility] A( //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub + //~| ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub #[rustc_effective_visibility] PubUnion, //~ 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 10ed14aa15035..019aaf8086a6a 100644 --- a/src/test/ui/privacy/effective_visibilities.stderr +++ b/src/test/ui/privacy/effective_visibilities.stderr @@ -28,92 +28,104 @@ error: not in the table LL | struct PrivStruct; | ^^^^^^^^^^^^^^^^^ +error: not in the table + --> $DIR/effective_visibilities.rs:20:9 + | +LL | struct PrivStruct; + | ^^^^^^^^^^^^^^^^^ + error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:23:9 + --> $DIR/effective_visibilities.rs:24:9 | LL | pub union PubUnion { | ^^^^^^^^^^^^^^^^^^ error: not in the table - --> $DIR/effective_visibilities.rs:25:13 + --> $DIR/effective_visibilities.rs:26:13 | LL | a: u8, | ^^^^^ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:27:13 + --> $DIR/effective_visibilities.rs:28:13 | LL | pub b: u8, | ^^^^^^^^^ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:31:9 + --> $DIR/effective_visibilities.rs:32:9 | LL | pub enum Enum { | ^^^^^^^^^^^^^ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:33:13 + --> $DIR/effective_visibilities.rs:34:13 + | +LL | A( + | ^ + +error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub + --> $DIR/effective_visibilities.rs:34:13 | LL | A( | ^ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:35:17 + --> $DIR/effective_visibilities.rs:37:17 | LL | PubUnion, | ^^^^^^^^ error: not in the table - --> $DIR/effective_visibilities.rs:41:5 + --> $DIR/effective_visibilities.rs:43:5 | LL | macro_rules! none_macro { | ^^^^^^^^^^^^^^^^^^^^^^^ error: Direct: pub(self), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:47:5 + --> $DIR/effective_visibilities.rs:49:5 | LL | macro_rules! public_macro { | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: Direct: pub(crate), Reexported: pub(crate), Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:52:5 + --> $DIR/effective_visibilities.rs:54:5 | LL | pub struct ReachableStruct { | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Direct: pub(crate), Reexported: pub(crate), Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:54:9 + --> $DIR/effective_visibilities.rs:56:9 | LL | pub a: u8, | ^^^^^^^^^ error: Direct: pub, Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:59:9 + --> $DIR/effective_visibilities.rs:61:9 | LL | pub use outer::inner1; | ^^^^^^^^^^^^^ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:65:5 + --> $DIR/effective_visibilities.rs:67:5 | LL | pub type HalfPublicImport = u8; | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: Direct: pub(crate), Reexported: pub(crate), Reachable: pub(crate), ReachableThroughImplTrait: pub(crate) - --> $DIR/effective_visibilities.rs:68:5 + --> $DIR/effective_visibilities.rs:70:5 | LL | pub(crate) const HalfPublicImport: u8 = 0; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Direct: pub, Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:72:9 + --> $DIR/effective_visibilities.rs:74:9 | LL | pub use half_public_import::HalfPublicImport; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Direct: pub, Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub - --> $DIR/effective_visibilities.rs:72:9 + --> $DIR/effective_visibilities.rs:74:9 | LL | pub use half_public_import::HalfPublicImport; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -130,5 +142,5 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl LL | type B; | ^^^^^^ -error: aborting due to 22 previous errors +error: aborting due to 24 previous errors From 448261a78a35026b3f5e855b705f35c916ecb19b Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 4 Nov 2022 17:44:40 +0400 Subject: [PATCH 3/4] privacy: Check effective visibility invariants --- compiler/rustc_middle/src/middle/privacy.rs | 51 ++++++++++++++++++++- compiler/rustc_privacy/src/lib.rs | 2 + 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs index 3d7a379c56cd1..11fbefefcc9ef 100644 --- a/compiler/rustc_middle/src/middle/privacy.rs +++ b/compiler/rustc_middle/src/middle/privacy.rs @@ -1,9 +1,10 @@ //! A pass that checks to make sure private fields and methods aren't used //! outside their scopes. This pass will also generate a set of exported items //! which are available for use externally when compiled as a library. -use crate::ty::{DefIdTree, Visibility}; +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; @@ -133,6 +134,54 @@ impl EffectiveVisibilities { } self.map.insert(id, effective_vis); } + + pub fn check_invariants(&self, tcx: TyCtxt<'_>, early: bool) { + if !cfg!(debug_assertions) { + return; + } + for (&def_id, ev) in &self.map { + // More direct visibility levels can never go farther than less direct ones, + // neither of effective visibilities can go farther than nominal visibility, + // and all effective visibilities are larger or equal than private visibility. + let private_vis = Visibility::Restricted(tcx.parent_module_from_def_id(def_id)); + let span = tcx.def_span(def_id.to_def_id()); + if !ev.direct.is_at_least(private_vis, tcx) { + span_bug!(span, "private {:?} > direct {:?}", private_vis, ev.direct); + } + if !ev.reexported.is_at_least(ev.direct, tcx) { + span_bug!(span, "direct {:?} > reexported {:?}", ev.direct, ev.reexported); + } + if !ev.reachable.is_at_least(ev.reexported, tcx) { + span_bug!(span, "reexported {:?} > reachable {:?}", ev.reexported, ev.reachable); + } + if !ev.reachable_through_impl_trait.is_at_least(ev.reachable, tcx) { + span_bug!( + span, + "reachable {:?} > reachable_through_impl_trait {:?}", + ev.reachable, + ev.reachable_through_impl_trait + ); + } + 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 { + span_bug!( + span, + "{:?}: reachable_through_impl_trait {:?} > nominal {:?}", + def_id, + ev.reachable_through_impl_trait, + 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); + } + } + } } impl EffectiveVisibilities { diff --git a/compiler/rustc_privacy/src/lib.rs b/compiler/rustc_privacy/src/lib.rs index bda3b55cfc33f..e17f85c1aae0f 100644 --- a/compiler/rustc_privacy/src/lib.rs +++ b/compiler/rustc_privacy/src/lib.rs @@ -2139,6 +2139,7 @@ fn effective_visibilities(tcx: TyCtxt<'_>, (): ()) -> &EffectiveVisibilities { changed: false, }; + visitor.effective_visibilities.check_invariants(tcx, true); loop { tcx.hir().walk_toplevel_module(&mut visitor); if visitor.changed { @@ -2147,6 +2148,7 @@ fn effective_visibilities(tcx: TyCtxt<'_>, (): ()) -> &EffectiveVisibilities { break; } } + visitor.effective_visibilities.check_invariants(tcx, false); let mut check_visitor = TestReachabilityVisitor { tcx, effective_visibilities: &visitor.effective_visibilities }; From 43bea6cf69501dee3d71e4440d947182a67137a0 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 5 Nov 2022 16:54:45 +0400 Subject: [PATCH 4/4] resolve: Fill effective visibilities for import def ids in a separate pass This should result in less update calls than doing it repeatedly during the fix point iteration. --- compiler/rustc_middle/src/middle/privacy.rs | 30 ++++++++- .../src/effective_visibilities.rs | 65 ++++++++++--------- 2 files changed, 61 insertions(+), 34 deletions(-) diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs index 11fbefefcc9ef..3a91522d36229 100644 --- a/compiler/rustc_middle/src/middle/privacy.rs +++ b/compiler/rustc_middle/src/middle/privacy.rs @@ -113,8 +113,30 @@ impl EffectiveVisibilities { }) } - pub fn iter(&self) -> impl Iterator { - self.map.iter() + // FIXME: Share code with `fn update`. + pub fn update_eff_vis( + &mut self, + def_id: LocalDefId, + eff_vis: &EffectiveVisibility, + tree: impl DefIdTree, + ) { + use std::collections::hash_map::Entry; + match self.map.entry(def_id) { + Entry::Occupied(mut occupied) => { + let old_eff_vis = occupied.get_mut(); + for l in Level::all_levels() { + let vis_at_level = eff_vis.at_level(l); + let old_vis_at_level = old_eff_vis.at_level_mut(l); + if vis_at_level != old_vis_at_level + && vis_at_level.is_at_least(*old_vis_at_level, tree) + { + *old_vis_at_level = *vis_at_level + } + } + old_eff_vis + } + Entry::Vacant(vacant) => vacant.insert(*eff_vis), + }; } pub fn set_public_at_level( @@ -185,6 +207,10 @@ impl EffectiveVisibilities { } impl EffectiveVisibilities { + pub fn iter(&self) -> impl Iterator { + self.map.iter() + } + pub fn effective_vis(&self, id: Id) -> Option<&EffectiveVisibility> { self.map.get(&id) } diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 56dde6f8ca74e..fa6d34be0cc37 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -55,6 +55,38 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { visit::walk_crate(&mut visitor, krate); } + // Update visibilities for import def ids. These are not used during the + // `EffectiveVisibilitiesVisitor` pass, because we have more detailed binding-based + // information, but are used by later passes. Effective visibility of an import def id + // is the maximum value among visibilities of bindings corresponding to that def id. + for (binding, eff_vis) in visitor.import_effective_visibilities.iter() { + let NameBindingKind::Import { import, .. } = binding.kind else { unreachable!() }; + if let Some(node_id) = import.id() { + let mut update = |node_id| { + r.effective_visibilities.update_eff_vis( + r.local_def_id(node_id), + eff_vis, + ResolverTree(&r.definitions, &r.crate_loader), + ) + }; + update(node_id); + if let ImportKind::Single { additional_ids: (id1, id2), .. } = import.kind { + // In theory all the single import IDs have individual visibilities and + // effective visibilities, but in practice these IDs go straigth to HIR + // where all their few uses assume that their (effective) visibility + // applies to the whole syntactic `use` item. So they all get the same + // value which is the maximum of all bindings. Maybe HIR for imports + // shouldn't use three IDs at all. + if id1 != ast::DUMMY_NODE_ID { + update(id1); + } + if id2 != ast::DUMMY_NODE_ID { + update(id2); + } + } + } + } + info!("resolve::effective_visibilities: {:#?}", r.effective_visibilities); } @@ -75,41 +107,10 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { // sets the rest of the `use` chain to `Level::Reexported` until // we hit the actual exported item. let mut parent_id = ParentId::Def(module_id); - while let NameBindingKind::Import { binding: nested_binding, import, .. } = - binding.kind - { + while let NameBindingKind::Import { binding: nested_binding, .. } = binding.kind { let binding_id = ImportId::new_unchecked(binding); self.update_import(binding_id, parent_id); - // Update visibilities for import ids. These are not used during this pass, - // because we have more detailed binding-based information, but are used by - // later passes. Effective visibility of an import def id is the maximum value - // among visibilities of bindings corresponding to that def id. - if let Some(node_id) = import.id() { - let mut update = |node_id| { - self.update_def( - self.r.local_def_id(node_id), - binding.vis.expect_local(), - parent_id, - ) - }; - update(node_id); - if let ImportKind::Single { additional_ids: (id1, id2), .. } = import.kind { - // In theory all the single import IDs have individual visibilities and - // effective visibilities, but in practice these IDs go straigth to HIR - // where all their few uses assume that their (effective) visibility - // applies to the whole syntactic `use` item. So they all get the same - // value which is the maximum of all bindings. Maybe HIR for imports - // shouldn't use three IDs at all. - if id1 != ast::DUMMY_NODE_ID { - update(id1); - } - if id2 != ast::DUMMY_NODE_ID { - update(id2); - } - } - } - parent_id = ParentId::Import(binding_id); binding = nested_binding; }