From 39888c4fa4a0c61586aee51e9cafd105aedc42d3 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 28 Oct 2022 14:58:21 +0400 Subject: [PATCH] resolve/effective-visibility: Stop recalculating current normal module It becomes relatively expensive if done often and shows up during perf profiling Also drop `extern` blocks from the effective visibility table, they are nominally private and it doesn't make sense to keep them there. --- .../src/effective_visibilities.rs | 113 +++++++++++------- src/test/ui/privacy/effective_visibilities.rs | 2 +- .../ui/privacy/effective_visibilities.stderr | 2 +- 3 files changed, 73 insertions(+), 44 deletions(-) diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index c40669ac95bee..91fe677e77629 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -4,13 +4,17 @@ use rustc_ast::visit; use rustc_ast::visit::Visitor; use rustc_ast::Crate; use rustc_ast::EnumDef; +use rustc_hir::def::{DefKind, Res}; 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 std::mem; pub struct EffectiveVisibilitiesVisitor<'r, 'a> { r: &'r mut Resolver<'a>, + // It's possible to recalculate this at any point, but it's relatively expensive. + current_normal_mod: LocalDefId, changed: bool, } @@ -19,21 +23,22 @@ 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, current_normal_mod: CRATE_DEF_ID, 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, @@ -49,20 +54,30 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { // 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 }; + // FIXME: level and is_public() condition should be removed, but assertions occur. + let level = if binding.is_import() { Level::Reexported } else { Level::Direct }; + let mut normal_mod_id = self.current_normal_mod; 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 update = |node_id| self.update( - self.r.local_def_id(node_id), - binding.vis.expect_local(), - prev_parent_id, - level, - ); + let def_id = self.r.local_def_id(import.id); + if level != Level::Direct { + // Not a first binding in the chain, recalculate parent module. + normal_mod_id = self.nearest_normal_mod(def_id); + } + + let mut update = |node_id| { + self.update_ext( + self.r.local_def_id(node_id), + binding.vis.expect_local(), + prev_parent_id, + normal_mod_id, + level, + ) + }; // 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 @@ -76,32 +91,45 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { } level = Level::Reexported; - prev_parent_id = self.r.local_def_id(import.id); + prev_parent_id = def_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); + let res = binding.res(); + if let Some(def_id) = res.opt_def_id().and_then(|id| id.as_local()) { + if level != Level::Direct + || matches!(binding.kind, NameBindingKind::Res(_, _is_macro_export @ true)) + // FIXME: This condition is incorrect, but it preserves pre-existing logic. + // Rewrite update logic to support parent nodes that are fully + // private and not in the table (which makes sense for `mod` items). + || matches!(res, Res::Def(DefKind::Mod, _)) + { + // Not a first binding in the chain, recalculate parent module. + normal_mod_id = self.nearest_normal_mod(def_id); + } + self.update_ext( + def_id, + binding.vis.expect_local(), + module_id, + normal_mod_id, + level, + ); } } } } - fn update( + fn update_ext( &mut self, def_id: LocalDefId, nominal_vis: Visibility, parent_id: LocalDefId, - tag: Level, + normal_mod_id: LocalDefId, + level: 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) + if nominal_vis == Visibility::Restricted(normal_mod_id) + || self.r.visibilities[&parent_id] == Visibility::Restricted(normal_mod_id) { return; } @@ -109,13 +137,23 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> { self.changed |= effective_visibilities.update( def_id, nominal_vis, - || Visibility::Restricted(module_id), + || Visibility::Restricted(normal_mod_id), parent_id, - tag, + level, &*self.r, ); self.r.effective_visibilities = effective_visibilities; } + + fn update(&mut self, def_id: LocalDefId, parent_id: LocalDefId) { + self.update_ext( + def_id, + self.r.visibilities[&def_id], + parent_id, + self.current_normal_mod, + Level::Direct, + ); + } } impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> { @@ -132,22 +170,16 @@ 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); - } - // Only exported `macro_rules!` items are public, but they always are ast::ItemKind::MacroDef(ref macro_def) if macro_def.macro_rules => { - let parent_id = self.r.local_parent(def_id); - let vis = self.r.visibilities[&def_id]; - self.update(def_id, vis, parent_id, Level::Direct); + self.update(def_id, self.r.local_parent(def_id)); } ast::ItemKind::Mod(..) => { + let prev_normal_mod = mem::replace(&mut self.current_normal_mod, def_id); self.set_bindings_effective_visibilities(def_id); visit::walk_item(self, item); + self.current_normal_mod = prev_normal_mod; } ast::ItemKind::Enum(EnumDef { ref variants }, _) => { @@ -155,18 +187,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); } } @@ -182,6 +210,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/src/test/ui/privacy/effective_visibilities.rs b/src/test/ui/privacy/effective_visibilities.rs index 1d806a1d1d167..567792df38572 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 1c6201600b630..acc8c574814b9 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" {}