Unnamed repository; edit this file 'description' to name the repository.
Optimize private visibility resolution
Lukas Wirth 10 months ago
parent e129cdc · commit b3768cd
-rw-r--r--crates/hir-def/src/expr_store/pretty.rs3
-rw-r--r--crates/hir-def/src/expr_store/tests/body/block.rs1
-rw-r--r--crates/hir-def/src/find_path.rs1
-rw-r--r--crates/hir-def/src/item_tree.rs22
-rw-r--r--crates/hir-def/src/item_tree/pretty.rs3
-rw-r--r--crates/hir-def/src/item_tree/tests.rs2
-rw-r--r--crates/hir-def/src/lib.rs4
-rw-r--r--crates/hir-def/src/nameres/path_resolution.rs36
-rw-r--r--crates/hir-def/src/resolver.rs3
-rw-r--r--crates/hir-def/src/visibility.rs47
10 files changed, 83 insertions, 39 deletions
diff --git a/crates/hir-def/src/expr_store/pretty.rs b/crates/hir-def/src/expr_store/pretty.rs
index ae8a959164..56c7655f9e 100644
--- a/crates/hir-def/src/expr_store/pretty.rs
+++ b/crates/hir-def/src/expr_store/pretty.rs
@@ -141,10 +141,11 @@ pub fn print_variant_body_hir(db: &dyn DefDatabase, owner: VariantId, edition: E
let FieldData { name, type_ref, visibility, is_unsafe } = data;
match visibility {
crate::item_tree::RawVisibility::Module(interned, _visibility_explicitness) => {
- w!(p, "{}", interned.display(db, p.edition))
+ w!(p, "pub(in {})", interned.display(db, p.edition))
}
crate::item_tree::RawVisibility::Public => w!(p, "pub "),
crate::item_tree::RawVisibility::PubCrate => w!(p, "pub(crate) "),
+ crate::item_tree::RawVisibility::PubSelf(_) => w!(p, "pub(self) "),
}
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 5a694c2092..bb0b70bc5b 100644
--- a/crates/hir-def/src/expr_store/tests/body/block.rs
+++ b/crates/hir-def/src/expr_store/tests/body/block.rs
@@ -397,7 +397,6 @@ fn main() {
fn underscore_import() {
// This used to panic, because the default (private) visibility inside block expressions would
// point into the containing `DefMap`, which visibilities should never be able to do.
- cov_mark::check!(adjust_vis_in_block_def_map);
check_at(
r#"
mod m {
diff --git a/crates/hir-def/src/find_path.rs b/crates/hir-def/src/find_path.rs
index 2820729d60..dccfff002f 100644
--- a/crates/hir-def/src/find_path.rs
+++ b/crates/hir-def/src/find_path.rs
@@ -1287,7 +1287,6 @@ $0
#[test]
fn explicit_private_imports_crate() {
- cov_mark::check!(explicit_private_imports);
check_found_path(
r#"
//- /main.rs
diff --git a/crates/hir-def/src/item_tree.rs b/crates/hir-def/src/item_tree.rs
index bf14c0ecee..c633339857 100644
--- a/crates/hir-def/src/item_tree.rs
+++ b/crates/hir-def/src/item_tree.rs
@@ -414,23 +414,15 @@ impl Index<RawVisibilityId> for ItemTree {
type Output = RawVisibility;
fn index(&self, index: RawVisibilityId) -> &Self::Output {
static VIS_PUB: RawVisibility = RawVisibility::Public;
- static VIS_PRIV_IMPLICIT: OnceLock<RawVisibility> = OnceLock::new();
- static VIS_PRIV_EXPLICIT: OnceLock<RawVisibility> = OnceLock::new();
+ static VIS_PRIV_IMPLICIT: RawVisibility =
+ RawVisibility::PubSelf(VisibilityExplicitness::Implicit);
+ static VIS_PRIV_EXPLICIT: RawVisibility =
+ RawVisibility::PubSelf(VisibilityExplicitness::Explicit);
static VIS_PUB_CRATE: RawVisibility = RawVisibility::PubCrate;
match index {
- RawVisibilityId::PRIV_IMPLICIT => VIS_PRIV_IMPLICIT.get_or_init(|| {
- RawVisibility::Module(
- Interned::new(ModPath::from_kind(PathKind::SELF)),
- VisibilityExplicitness::Implicit,
- )
- }),
- RawVisibilityId::PRIV_EXPLICIT => VIS_PRIV_EXPLICIT.get_or_init(|| {
- RawVisibility::Module(
- Interned::new(ModPath::from_kind(PathKind::SELF)),
- VisibilityExplicitness::Explicit,
- )
- }),
+ RawVisibilityId::PRIV_IMPLICIT => &VIS_PRIV_IMPLICIT,
+ RawVisibilityId::PRIV_EXPLICIT => &VIS_PRIV_EXPLICIT,
RawVisibilityId::PUB => &VIS_PUB,
RawVisibilityId::PUB_CRATE => &VIS_PUB_CRATE,
_ => &self.vis.arena[index.0 as usize],
@@ -550,6 +542,8 @@ pub enum RawVisibility {
/// `pub(in module)`, `pub(crate)` or `pub(super)`. Also private, which is
/// equivalent to `pub(self)`.
Module(Interned<ModPath>, VisibilityExplicitness),
+ /// `pub(self)`.
+ PubSelf(VisibilityExplicitness),
/// `pub(crate)`.
PubCrate,
/// `pub`.
diff --git a/crates/hir-def/src/item_tree/pretty.rs b/crates/hir-def/src/item_tree/pretty.rs
index a8e816bd3a..696174cb07 100644
--- a/crates/hir-def/src/item_tree/pretty.rs
+++ b/crates/hir-def/src/item_tree/pretty.rs
@@ -108,10 +108,11 @@ impl Printer<'_> {
fn print_visibility(&mut self, vis: RawVisibilityId) {
match &self.tree[vis] {
RawVisibility::Module(path, _expl) => {
- w!(self, "pub({}) ", path.display(self.db, self.edition))
+ w!(self, "pub(in {}) ", path.display(self.db, self.edition))
}
RawVisibility::Public => w!(self, "pub "),
RawVisibility::PubCrate => w!(self, "pub(crate) "),
+ RawVisibility::PubSelf(_) => w!(self, "pub(self) "),
};
}
diff --git a/crates/hir-def/src/item_tree/tests.rs b/crates/hir-def/src/item_tree/tests.rs
index e39efd31c6..5923b3ea49 100644
--- a/crates/hir-def/src/item_tree/tests.rs
+++ b/crates/hir-def/src/item_tree/tests.rs
@@ -39,7 +39,7 @@ use a::{c, d::{e}};
pub(self) extern crate self as renamed;
// AstId: ExternCrate[7E1C, 0]
- pub(super) extern crate bli;
+ pub(in super) extern crate bli;
// AstId: Use[0000, 0]
pub use crate::path::{nested, items as renamed, Trait as _};
diff --git a/crates/hir-def/src/lib.rs b/crates/hir-def/src/lib.rs
index 0cd7e89432..a542214d30 100644
--- a/crates/hir-def/src/lib.rs
+++ b/crates/hir-def/src/lib.rs
@@ -149,6 +149,7 @@ impl<N: AstIdNode> HasModule for ItemLoc<N> {
#[derive(Debug)]
pub struct AssocItemLoc<N: AstIdNode> {
+ // FIXME: Store this as an erased `salsa::Id` to save space
pub container: ItemContainerId,
pub id: AstId<N>,
}
@@ -577,6 +578,7 @@ pub type LocalModuleId = Idx<nameres::ModuleData>;
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct FieldId {
+ // FIXME: Store this as an erased `salsa::Id` to save space
pub parent: VariantId,
pub local_id: LocalFieldId,
}
@@ -592,6 +594,7 @@ pub struct TupleFieldId {
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct TypeOrConstParamId {
+ // FIXME: Store this as an erased `salsa::Id` to save space
pub parent: GenericDefId,
pub local_id: LocalTypeOrConstParamId,
}
@@ -650,6 +653,7 @@ impl From<ConstParamId> for TypeOrConstParamId {
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct LifetimeParamId {
+ // FIXME: Store this as an erased `salsa::Id` to save space
pub parent: GenericDefId,
pub local_id: LocalLifetimeParamId,
}
diff --git a/crates/hir-def/src/nameres/path_resolution.rs b/crates/hir-def/src/nameres/path_resolution.rs
index 307acaf1fe..e8235b1c96 100644
--- a/crates/hir-def/src/nameres/path_resolution.rs
+++ b/crates/hir-def/src/nameres/path_resolution.rs
@@ -106,7 +106,7 @@ impl DefMap {
visibility: &RawVisibility,
within_impl: bool,
) -> Option<Visibility> {
- let mut vis = match visibility {
+ let vis = match visibility {
RawVisibility::Module(path, explicitness) => {
let (result, remaining) = self.resolve_path(
local_def_map,
@@ -120,30 +120,36 @@ impl DefMap {
return None;
}
let types = result.take_types()?;
- match types {
+ let mut vis = match types {
ModuleDefId::ModuleId(m) => Visibility::Module(m, *explicitness),
// error: visibility needs to refer to module
_ => {
return None;
}
+ };
+
+ // In block expressions, `self` normally refers to the containing non-block module, and
+ // `super` to its parent (etc.). However, visibilities must only refer to a module in the
+ // DefMap they're written in, so we restrict them when that happens.
+ if let Visibility::Module(m, mv) = vis {
+ // ...unless we're resolving visibility for an associated item in an impl.
+ if self.block_id() != m.block && !within_impl {
+ vis = Visibility::Module(self.module_id(Self::ROOT), mv);
+ tracing::debug!(
+ "visibility {:?} points outside DefMap, adjusting to {:?}",
+ m,
+ vis
+ );
+ }
}
+ vis
+ }
+ RawVisibility::PubSelf(explicitness) => {
+ Visibility::Module(self.module_id(original_module), *explicitness)
}
RawVisibility::Public => Visibility::Public,
RawVisibility::PubCrate => Visibility::PubCrate(self.krate),
};
-
- // In block expressions, `self` normally refers to the containing non-block module, and
- // `super` to its parent (etc.). However, visibilities must only refer to a module in the
- // DefMap they're written in, so we restrict them when that happens.
- if let Visibility::Module(m, mv) = vis {
- // ...unless we're resolving visibility for an associated item in an impl.
- if self.block_id() != m.block && !within_impl {
- cov_mark::hit!(adjust_vis_in_block_def_map);
- vis = Visibility::Module(self.module_id(Self::ROOT), mv);
- tracing::debug!("visibility {:?} points outside DefMap, adjusting to {:?}", m, vis);
- }
- }
-
Some(vis)
}
diff --git a/crates/hir-def/src/resolver.rs b/crates/hir-def/src/resolver.rs
index 7b24f6cc74..6f321980af 100644
--- a/crates/hir-def/src/resolver.rs
+++ b/crates/hir-def/src/resolver.rs
@@ -305,6 +305,9 @@ impl<'db> Resolver<'db> {
}),
)
}
+ RawVisibility::PubSelf(explicitness) => {
+ Some(Visibility::Module(self.module(), *explicitness))
+ }
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 11efeed17b..2514e88864 100644
--- a/crates/hir-def/src/visibility.rs
+++ b/crates/hir-def/src/visibility.rs
@@ -47,6 +47,10 @@ impl Visibility {
Visibility::PubCrate(krate) => return from_module.krate == krate,
Visibility::Public => return true,
};
+ if from_module == to_module {
+ // if the modules are the same, visibility is trivially satisfied
+ return true;
+ }
// if they're not in the same crate, it can't be visible
if from_module.krate != to_module.krate {
return false;
@@ -70,6 +74,11 @@ impl Visibility {
if def_map.krate() != to_module.krate {
return false;
}
+
+ if from_module == to_module.local_id && def_map.block_id() == to_module.block {
+ // if the modules are the same, visibility is trivially satisfied
+ return true;
+ }
Self::is_visible_from_def_map_(db, def_map, to_module, from_module)
}
@@ -93,9 +102,7 @@ impl Visibility {
// `to_module` is not a block, so there is no parent def map to use.
(None, _) => (),
// `to_module` is at `def_map`'s block, no need to move further.
- (Some(a), Some(b)) if a == b => {
- cov_mark::hit!(is_visible_from_same_block_def_map);
- }
+ (Some(a), Some(b)) if a == b => {}
_ => {
if let Some(parent) = to_module.def_map(db).parent() {
to_module = parent;
@@ -153,7 +160,22 @@ impl Visibility {
}
}
(Visibility::Module(mod_a, expl_a), Visibility::Module(mod_b, expl_b)) => {
- if mod_a.krate != def_map.krate() || mod_b.krate != def_map.krate() {
+ if mod_a == mod_b {
+ // Most module visibilities are `pub(self)`, and assuming no errors
+ // this will be the common and thus fast path.
+ return Some(Visibility::Module(
+ mod_a,
+ match (expl_a, expl_b) {
+ (VisibilityExplicitness::Explicit, _)
+ | (_, VisibilityExplicitness::Explicit) => {
+ VisibilityExplicitness::Explicit
+ }
+ _ => VisibilityExplicitness::Implicit,
+ },
+ ));
+ }
+
+ if mod_a.krate() != def_map.krate() || mod_b.krate() != def_map.krate() {
return None;
}
@@ -201,7 +223,22 @@ impl Visibility {
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 != def_map.krate() || mod_b.krate != def_map.krate() {
+ if mod_a == mod_b {
+ // Most module visibilities are `pub(self)`, and assuming no errors
+ // this will be the common and thus fast path.
+ return Some(Visibility::Module(
+ mod_a,
+ match (expl_a, expl_b) {
+ (VisibilityExplicitness::Explicit, _)
+ | (_, VisibilityExplicitness::Explicit) => {
+ VisibilityExplicitness::Explicit
+ }
+ _ => VisibilityExplicitness::Implicit,
+ },
+ ));
+ }
+
+ if mod_a.krate() != def_map.krate() || mod_b.krate() != def_map.krate() {
return None;
}