From e129cdc202345024be35c8f10f1ed00cc4e9538e Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sat, 14 Jun 2025 14:33:52 +0200 Subject: [PATCH] Optimize `pub(crate)` visibility resolution --- crates/hir-def/src/expr_store/pretty.rs | 1 + .../src/expr_store/tests/body/block.rs | 1 - crates/hir-def/src/find_path.rs | 1 + crates/hir-def/src/item_scope.rs | 22 ++------- crates/hir-def/src/item_tree.rs | 11 ++--- crates/hir-def/src/item_tree/pretty.rs | 1 + crates/hir-def/src/nameres/collector.rs | 3 +- crates/hir-def/src/nameres/path_resolution.rs | 1 + crates/hir-def/src/resolver.rs | 1 + crates/hir-def/src/visibility.rs | 47 +++++++++++++++---- crates/hir-ty/src/display.rs | 1 + crates/hir/src/symbols.rs | 1 + 12 files changed, 55 insertions(+), 36 deletions(-) diff --git a/crates/hir-def/src/expr_store/pretty.rs b/crates/hir-def/src/expr_store/pretty.rs index 7b452721df..ae8a959164 100644 --- a/crates/hir-def/src/expr_store/pretty.rs +++ b/crates/hir-def/src/expr_store/pretty.rs @@ -144,6 +144,7 @@ pub fn print_variant_body_hir(db: &dyn DefDatabase, owner: VariantId, edition: E w!(p, "{}", interned.display(db, p.edition)) } crate::item_tree::RawVisibility::Public => w!(p, "pub "), + crate::item_tree::RawVisibility::PubCrate => w!(p, "pub(crate) "), } if *is_unsafe { w!(p, "unsafe "); diff --git a/crates/hir-def/src/expr_store/tests/body/block.rs b/crates/hir-def/src/expr_store/tests/body/block.rs index 5fe7277817..5a694c2092 100644 --- a/crates/hir-def/src/expr_store/tests/body/block.rs +++ b/crates/hir-def/src/expr_store/tests/body/block.rs @@ -457,7 +457,6 @@ fn foo() { #[test] fn is_visible_from_same_def_map() { // Regression test for https://github.com/rust-lang/rust-analyzer/issues/9481 - cov_mark::check!(is_visible_from_same_block_def_map); check_at( r#" fn outer() { diff --git a/crates/hir-def/src/find_path.rs b/crates/hir-def/src/find_path.rs index 6ebc0d8dfd..2820729d60 100644 --- a/crates/hir-def/src/find_path.rs +++ b/crates/hir-def/src/find_path.rs @@ -615,6 +615,7 @@ fn find_local_import_locations( cov_mark::hit!(discount_private_imports); false } + Visibility::PubCrate(_) => true, Visibility::Public => true, }; diff --git a/crates/hir-def/src/item_scope.rs b/crates/hir-def/src/item_scope.rs index 8591874eb9..efa4399468 100644 --- a/crates/hir-def/src/item_scope.rs +++ b/crates/hir-def/src/item_scope.rs @@ -20,7 +20,7 @@ use crate::{ LocalModuleId, Lookup, MacroId, ModuleDefId, ModuleId, TraitId, UseId, db::DefDatabase, per_ns::{Item, MacrosItem, PerNs, TypesItem, ValuesItem}, - visibility::{Visibility, VisibilityExplicitness}, + visibility::Visibility, }; #[derive(Debug, Default)] @@ -720,33 +720,19 @@ impl ItemScope { } /// Marks everything that is not a procedural macro as private to `this_module`. - pub(crate) fn censor_non_proc_macros(&mut self, this_module: ModuleId) { + pub(crate) fn censor_non_proc_macros(&mut self, krate: Crate) { self.types .values_mut() .map(|def| &mut def.vis) .chain(self.values.values_mut().map(|def| &mut def.vis)) .chain(self.unnamed_trait_imports.iter_mut().map(|(_, def)| &mut def.vis)) - .for_each(|vis| match vis { - &mut Visibility::Module(_, visibility_explicitness) => { - *vis = Visibility::Module(this_module, visibility_explicitness) - } - Visibility::Public => { - *vis = Visibility::Module(this_module, VisibilityExplicitness::Implicit) - } - }); + .for_each(|vis| *vis = Visibility::PubCrate(krate)); for mac in self.macros.values_mut() { if matches!(mac.def, MacroId::ProcMacroId(_) if mac.import.is_none()) { continue; } - match mac.vis { - Visibility::Module(_, visibility_explicitness) => { - mac.vis = Visibility::Module(this_module, visibility_explicitness) - } - Visibility::Public => { - mac.vis = Visibility::Module(this_module, VisibilityExplicitness::Implicit) - } - } + mac.vis = Visibility::PubCrate(krate) } } diff --git a/crates/hir-def/src/item_tree.rs b/crates/hir-def/src/item_tree.rs index f61b403b84..bf14c0ecee 100644 --- a/crates/hir-def/src/item_tree.rs +++ b/crates/hir-def/src/item_tree.rs @@ -416,7 +416,7 @@ impl Index for ItemTree { static VIS_PUB: RawVisibility = RawVisibility::Public; static VIS_PRIV_IMPLICIT: OnceLock = OnceLock::new(); static VIS_PRIV_EXPLICIT: OnceLock = OnceLock::new(); - static VIS_PUB_CRATE: OnceLock = OnceLock::new(); + static VIS_PUB_CRATE: RawVisibility = RawVisibility::PubCrate; match index { RawVisibilityId::PRIV_IMPLICIT => VIS_PRIV_IMPLICIT.get_or_init(|| { @@ -432,12 +432,7 @@ impl Index for ItemTree { ) }), RawVisibilityId::PUB => &VIS_PUB, - RawVisibilityId::PUB_CRATE => VIS_PUB_CRATE.get_or_init(|| { - RawVisibility::Module( - Interned::new(ModPath::from_kind(PathKind::Crate)), - VisibilityExplicitness::Explicit, - ) - }), + RawVisibilityId::PUB_CRATE => &VIS_PUB_CRATE, _ => &self.vis.arena[index.0 as usize], } } @@ -555,6 +550,8 @@ pub enum RawVisibility { /// `pub(in module)`, `pub(crate)` or `pub(super)`. Also private, which is /// equivalent to `pub(self)`. Module(Interned, VisibilityExplicitness), + /// `pub(crate)`. + PubCrate, /// `pub`. Public, } diff --git a/crates/hir-def/src/item_tree/pretty.rs b/crates/hir-def/src/item_tree/pretty.rs index eb97081128..a8e816bd3a 100644 --- a/crates/hir-def/src/item_tree/pretty.rs +++ b/crates/hir-def/src/item_tree/pretty.rs @@ -111,6 +111,7 @@ impl Printer<'_> { w!(self, "pub({}) ", path.display(self.db, self.edition)) } RawVisibility::Public => w!(self, "pub "), + RawVisibility::PubCrate => w!(self, "pub(crate) "), }; } diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs index 508fcc2fa5..78fdc27560 100644 --- a/crates/hir-def/src/nameres/collector.rs +++ b/crates/hir-def/src/nameres/collector.rs @@ -437,9 +437,8 @@ impl<'db> DefCollector<'db> { // Additionally, while the proc macro entry points must be `pub`, they are not publicly // exported in type/value namespace. This function reduces the visibility of all items // in the crate root that aren't proc macros. - let module_id = self.def_map.module_id(DefMap::ROOT); let root = &mut self.def_map.modules[DefMap::ROOT]; - root.scope.censor_non_proc_macros(module_id); + root.scope.censor_non_proc_macros(self.def_map.krate); } } diff --git a/crates/hir-def/src/nameres/path_resolution.rs b/crates/hir-def/src/nameres/path_resolution.rs index ca39173d47..307acaf1fe 100644 --- a/crates/hir-def/src/nameres/path_resolution.rs +++ b/crates/hir-def/src/nameres/path_resolution.rs @@ -129,6 +129,7 @@ impl DefMap { } } RawVisibility::Public => Visibility::Public, + RawVisibility::PubCrate => Visibility::PubCrate(self.krate), }; // In block expressions, `self` normally refers to the containing non-block module, and diff --git a/crates/hir-def/src/resolver.rs b/crates/hir-def/src/resolver.rs index 491b6204bc..7b24f6cc74 100644 --- a/crates/hir-def/src/resolver.rs +++ b/crates/hir-def/src/resolver.rs @@ -305,6 +305,7 @@ impl<'db> Resolver<'db> { }), ) } + RawVisibility::PubCrate => Some(Visibility::PubCrate(self.krate())), RawVisibility::Public => Some(Visibility::Public), } } diff --git a/crates/hir-def/src/visibility.rs b/crates/hir-def/src/visibility.rs index aea2ac550e..11efeed17b 100644 --- a/crates/hir-def/src/visibility.rs +++ b/crates/hir-def/src/visibility.rs @@ -2,6 +2,7 @@ use std::iter; +use base_db::Crate; use hir_expand::{InFile, Lookup}; use la_arena::ArenaMap; use syntax::ast::{self, HasVisibility}; @@ -19,6 +20,8 @@ pub use crate::item_tree::{RawVisibility, VisibilityExplicitness}; pub enum Visibility { /// Visibility is restricted to a certain module. Module(ModuleId, VisibilityExplicitness), + /// Visibility is restricted to the crate. + PubCrate(Crate), /// Visibility is unrestricted. Public, } @@ -41,6 +44,7 @@ impl Visibility { pub fn is_visible_from(self, db: &dyn DefDatabase, from_module: ModuleId) -> bool { let to_module = match self { Visibility::Module(m, _) => m, + Visibility::PubCrate(krate) => return from_module.krate == krate, Visibility::Public => return true, }; // if they're not in the same crate, it can't be visible @@ -59,6 +63,7 @@ impl Visibility { ) -> bool { let to_module = match self { Visibility::Module(m, _) => m, + Visibility::PubCrate(krate) => return def_map.krate() == krate, Visibility::Public => return true, }; // if they're not in the same crate, it can't be visible @@ -132,26 +137,41 @@ impl Visibility { pub(crate) fn max(self, other: Visibility, def_map: &DefMap) -> Option { match (self, other) { (_, Visibility::Public) | (Visibility::Public, _) => Some(Visibility::Public), + (Visibility::PubCrate(krate), Visibility::PubCrate(krateb)) => { + if krate == krateb { + Some(Visibility::PubCrate(krate)) + } else { + None + } + } + (Visibility::Module(mod_, _), Visibility::PubCrate(krate)) + | (Visibility::PubCrate(krate), Visibility::Module(mod_, _)) => { + if mod_.krate == krate { + Some(Visibility::PubCrate(krate)) + } else { + None + } + } (Visibility::Module(mod_a, expl_a), Visibility::Module(mod_b, expl_b)) => { - if mod_a.krate != mod_b.krate { + if mod_a.krate != def_map.krate() || mod_b.krate != def_map.krate() { return None; } let def_block = def_map.block_id(); - if (mod_a.containing_block(), mod_b.containing_block()) != (def_block, def_block) { + if mod_a.containing_block() != def_block || mod_b.containing_block() != def_block { return None; } let mut a_ancestors = iter::successors(Some(mod_a.local_id), |&m| def_map[m].parent); - let mut b_ancestors = - iter::successors(Some(mod_b.local_id), |&m| def_map[m].parent); if a_ancestors.any(|m| m == mod_b.local_id) { // B is above A return Some(Visibility::Module(mod_b, expl_b)); } + let mut b_ancestors = + iter::successors(Some(mod_b.local_id), |&m| def_map[m].parent); if b_ancestors.any(|m| m == mod_a.local_id) { // A is above B return Some(Visibility::Module(mod_a, expl_a)); @@ -169,26 +189,37 @@ impl Visibility { pub(crate) fn min(self, other: Visibility, def_map: &DefMap) -> Option { match (self, other) { (vis, Visibility::Public) | (Visibility::Public, vis) => Some(vis), + (Visibility::PubCrate(krate), Visibility::PubCrate(krateb)) => { + if krate == krateb { + Some(Visibility::PubCrate(krate)) + } else { + None + } + } + (Visibility::Module(mod_, exp), Visibility::PubCrate(krate)) + | (Visibility::PubCrate(krate), Visibility::Module(mod_, exp)) => { + if mod_.krate == krate { Some(Visibility::Module(mod_, exp)) } else { None } + } (Visibility::Module(mod_a, expl_a), Visibility::Module(mod_b, expl_b)) => { - if mod_a.krate != mod_b.krate { + if mod_a.krate != def_map.krate() || mod_b.krate != def_map.krate() { return None; } let def_block = def_map.block_id(); - if (mod_a.containing_block(), mod_b.containing_block()) != (def_block, def_block) { + if mod_a.containing_block() != def_block || mod_b.containing_block() != def_block { return None; } let mut a_ancestors = iter::successors(Some(mod_a.local_id), |&m| def_map[m].parent); - let mut b_ancestors = - iter::successors(Some(mod_b.local_id), |&m| def_map[m].parent); if a_ancestors.any(|m| m == mod_b.local_id) { // B is above A return Some(Visibility::Module(mod_a, expl_a)); } + let mut b_ancestors = + iter::successors(Some(mod_b.local_id), |&m| def_map[m].parent); if b_ancestors.any(|m| m == mod_a.local_id) { // A is above B return Some(Visibility::Module(mod_b, expl_b)); diff --git a/crates/hir-ty/src/display.rs b/crates/hir-ty/src/display.rs index da21f08a1e..1aa7e0fcf8 100644 --- a/crates/hir-ty/src/display.rs +++ b/crates/hir-ty/src/display.rs @@ -2082,6 +2082,7 @@ pub fn write_visibility( ) -> Result<(), HirDisplayError> { match vis { Visibility::Public => write!(f, "pub "), + Visibility::PubCrate(_) => write!(f, "pub(crate) "), Visibility::Module(vis_id, _) => { let def_map = module_id.def_map(f.db); let root_module_id = def_map.module_id(DefMap::ROOT); diff --git a/crates/hir/src/symbols.rs b/crates/hir/src/symbols.rs index 05f4a830da..d80b704f53 100644 --- a/crates/hir/src/symbols.rs +++ b/crates/hir/src/symbols.rs @@ -164,6 +164,7 @@ impl<'a> SymbolCollector<'a> { let is_explicit_import = |vis| match vis { Visibility::Public => true, + Visibility::PubCrate(_) => true, Visibility::Module(_, VisibilityExplicitness::Explicit) => true, Visibility::Module(_, VisibilityExplicitness::Implicit) => false, };