Skip to content

Commit eae79c3

Browse files
VeykrilVeykril
committed
Optimize private visibility resolution
1 parent 5bbf8aa commit eae79c3

File tree

10 files changed

+82
-39
lines changed

10 files changed

+82
-39
lines changed

‎crates/hir-def/src/expr_store/pretty.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,10 +141,11 @@ pub fn print_variant_body_hir(db: &dyn DefDatabase, owner: VariantId, edition: E
141141
let FieldData { name, type_ref, visibility, is_unsafe } = data;
142142
match visibility {
143143
crate::item_tree::RawVisibility::Module(interned, _visibility_explicitness) => {
144-
w!(p, "{}", interned.display(db, p.edition))
144+
w!(p, "pub(in {})", interned.display(db, p.edition))
145145
}
146146
crate::item_tree::RawVisibility::Public => w!(p, "pub "),
147147
crate::item_tree::RawVisibility::PubCrate => w!(p, "pub(crate) "),
148+
crate::item_tree::RawVisibility::PubSelf(_) => w!(p, "pub(self) "),
148149
}
149150
if *is_unsafe {
150151
w!(p, "unsafe ");

‎crates/hir-def/src/expr_store/tests/body/block.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,6 @@ fn outer() {
176176

177177
#[test]
178178
fn nested_module_scoping() {
179-
cov_mark::check!(nested_module_scoping);
180179
check_block_scopes_at(
181180
r#"
182181
fn f() {
@@ -445,7 +444,6 @@ fn main() {
445444
fn underscore_import() {
446445
// This used to panic, because the default (private) visibility inside block expressions would
447446
// point into the containing `DefMap`, which visibilities should never be able to do.
448-
cov_mark::check!(adjust_vis_in_block_def_map);
449447
check_at(
450448
r#"
451449
mod m {

‎crates/hir-def/src/find_path.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,7 @@ fn find_local_import_locations(
604604

605605
if let Some((name, vis, declared)) = data.scope.name_of(item) {
606606
if vis.is_visible_from(db, ctx.from) {
607-
let is_pub_or_explicit = match vis {
607+
let is_pub_or_explicit = match dbg!(vis) {
608608
Visibility::Module(_, VisibilityExplicitness::Explicit) => {
609609
cov_mark::hit!(explicit_private_imports);
610610
true
@@ -1285,7 +1285,6 @@ $0
12851285

12861286
#[test]
12871287
fn explicit_private_imports_crate() {
1288-
cov_mark::check!(explicit_private_imports);
12891288
check_found_path(
12901289
r#"
12911290
//- /main.rs

‎crates/hir-def/src/item_tree.rs

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -414,23 +414,15 @@ impl Index<RawVisibilityId> for ItemTree {
414414
type Output = RawVisibility;
415415
fn index(&self, index: RawVisibilityId) -> &Self::Output {
416416
static VIS_PUB: RawVisibility = RawVisibility::Public;
417-
static VIS_PRIV_IMPLICIT: OnceLock<RawVisibility> = OnceLock::new();
418-
static VIS_PRIV_EXPLICIT: OnceLock<RawVisibility> = OnceLock::new();
417+
static VIS_PRIV_IMPLICIT: RawVisibility =
418+
RawVisibility::PubSelf(VisibilityExplicitness::Implicit);
419+
static VIS_PRIV_EXPLICIT: RawVisibility =
420+
RawVisibility::PubSelf(VisibilityExplicitness::Explicit);
419421
static VIS_PUB_CRATE: RawVisibility = RawVisibility::PubCrate;
420422

421423
match index {
422-
RawVisibilityId::PRIV_IMPLICIT => VIS_PRIV_IMPLICIT.get_or_init(|| {
423-
RawVisibility::Module(
424-
Interned::new(ModPath::from_kind(PathKind::SELF)),
425-
VisibilityExplicitness::Implicit,
426-
)
427-
}),
428-
RawVisibilityId::PRIV_EXPLICIT => VIS_PRIV_EXPLICIT.get_or_init(|| {
429-
RawVisibility::Module(
430-
Interned::new(ModPath::from_kind(PathKind::SELF)),
431-
VisibilityExplicitness::Explicit,
432-
)
433-
}),
424+
RawVisibilityId::PRIV_IMPLICIT => &VIS_PRIV_IMPLICIT,
425+
RawVisibilityId::PRIV_EXPLICIT => &VIS_PRIV_EXPLICIT,
434426
RawVisibilityId::PUB => &VIS_PUB,
435427
RawVisibilityId::PUB_CRATE => &VIS_PUB_CRATE,
436428
_ => &self.vis.arena[index.0 as usize],
@@ -550,6 +542,8 @@ pub enum RawVisibility {
550542
/// `pub(in module)`, `pub(crate)` or `pub(super)`. Also private, which is
551543
/// equivalent to `pub(self)`.
552544
Module(Interned<ModPath>, VisibilityExplicitness),
545+
/// `pub(self)`.
546+
PubSelf(VisibilityExplicitness),
553547
/// `pub(crate)`.
554548
PubCrate,
555549
/// `pub`.

‎crates/hir-def/src/item_tree/pretty.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,10 +108,11 @@ impl Printer<'_> {
108108
fn print_visibility(&mut self, vis: RawVisibilityId) {
109109
match &self.tree[vis] {
110110
RawVisibility::Module(path, _expl) => {
111-
w!(self, "pub({}) ", path.display(self.db, self.edition))
111+
w!(self, "pub(in {}) ", path.display(self.db, self.edition))
112112
}
113113
RawVisibility::Public => w!(self, "pub "),
114114
RawVisibility::PubCrate => w!(self, "pub(crate) "),
115+
RawVisibility::PubSelf(_) => w!(self, "pub(self) "),
115116
};
116117
}
117118

‎crates/hir-def/src/item_tree/tests.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ use a::{c, d::{e}};
3939
pub(self) extern crate self as renamed;
4040
4141
// AstId: ExternCrate[7E1C, 0]
42-
pub(super) extern crate bli;
42+
pub(in super) extern crate bli;
4343
4444
// AstId: Use[0000, 0]
4545
pub use crate::path::{nested, items as renamed, Trait as _};

‎crates/hir-def/src/lib.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ impl<N: AstIdNode> HasModule for ItemLoc<N> {
144144

145145
#[derive(Debug)]
146146
pub struct AssocItemLoc<N: AstIdNode> {
147+
// FIXME: Store this as an erased `salsa::Id` to save space
147148
pub container: ItemContainerId,
148149
pub id: AstId<N>,
149150
}
@@ -445,6 +446,7 @@ impl HasModule for ModuleId {
445446

446447
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
447448
pub struct FieldId {
449+
// FIXME: Store this as an erased `salsa::Id` to save space
448450
pub parent: VariantId,
449451
pub local_id: LocalFieldId,
450452
}
@@ -460,6 +462,7 @@ pub struct TupleFieldId {
460462

461463
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
462464
pub struct TypeOrConstParamId {
465+
// FIXME: Store this as an erased `salsa::Id` to save space
463466
pub parent: GenericDefId,
464467
pub local_id: LocalTypeOrConstParamId,
465468
}
@@ -518,6 +521,7 @@ impl From<ConstParamId> for TypeOrConstParamId {
518521

519522
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
520523
pub struct LifetimeParamId {
524+
// FIXME: Store this as an erased `salsa::Id` to save space
521525
pub parent: GenericDefId,
522526
pub local_id: LocalLifetimeParamId,
523527
}
@@ -661,7 +665,7 @@ impl DefWithBodyId {
661665
}
662666
}
663667

664-
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
668+
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, salsa_macros::Supertype)]
665669
pub enum AssocItemId {
666670
FunctionId(FunctionId),
667671
ConstId(ConstId),

‎crates/hir-def/src/nameres/path_resolution.rs

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ impl DefMap {
106106
visibility: &RawVisibility,
107107
within_impl: bool,
108108
) -> Option<Visibility> {
109-
let mut vis = match visibility {
109+
let vis = match visibility {
110110
RawVisibility::Module(path, explicitness) => {
111111
let (result, remaining) = self.resolve_path(
112112
local_def_map,
@@ -120,30 +120,37 @@ impl DefMap {
120120
return None;
121121
}
122122
let types = result.take_types()?;
123-
match types {
123+
let mut vis = match types {
124124
ModuleDefId::ModuleId(m) => Visibility::Module(m, *explicitness),
125125
// error: visibility needs to refer to module
126126
_ => {
127127
return None;
128128
}
129+
};
130+
131+
// In block expressions, `self` normally refers to the containing non-block module, and
132+
// `super` to its parent (etc.). However, visibilities must only refer to a module in the
133+
// DefMap they're written in, so we restrict them when that happens.
134+
if let Visibility::Module(m, mv) = vis {
135+
// ...unless we're resolving visibility for an associated item in an impl.
136+
if self.block_id() != m.block(db) && !within_impl {
137+
vis = Visibility::Module(self.root, mv);
138+
tracing::debug!(
139+
"visibility {:?} points outside DefMap, adjusting to {:?}",
140+
m,
141+
vis
142+
);
143+
}
129144
}
145+
vis
146+
}
147+
RawVisibility::PubSelf(explicitness) => {
148+
Visibility::Module(original_module, *explicitness)
130149
}
131150
RawVisibility::Public => Visibility::Public,
132151
RawVisibility::PubCrate => Visibility::PubCrate(self.krate),
133152
};
134153

135-
// In block expressions, `self` normally refers to the containing non-block module, and
136-
// `super` to its parent (etc.). However, visibilities must only refer to a module in the
137-
// DefMap they're written in, so we restrict them when that happens.
138-
if let Visibility::Module(m, mv) = vis {
139-
// ...unless we're resolving visibility for an associated item in an impl.
140-
if self.block_id() != m.block(db) && !within_impl {
141-
cov_mark::hit!(adjust_vis_in_block_def_map);
142-
vis = Visibility::Module(self.root, mv);
143-
tracing::debug!("visibility {:?} points outside DefMap, adjusting to {:?}", m, vis);
144-
}
145-
}
146-
147154
Some(vis)
148155
}
149156

‎crates/hir-def/src/resolver.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,9 @@ impl<'db> Resolver<'db> {
305305
}),
306306
)
307307
}
308+
RawVisibility::PubSelf(explicitness) => {
309+
Some(Visibility::Module(self.module(), *explicitness))
310+
}
308311
RawVisibility::PubCrate => Some(Visibility::PubCrate(self.krate())),
309312
RawVisibility::Public => Some(Visibility::Public),
310313
}

‎crates/hir-def/src/visibility.rs

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ impl Visibility {
5151
Visibility::PubCrate(krate) => return from_module.krate(db) == krate,
5252
Visibility::Public => return true,
5353
};
54+
if from_module == to_module {
55+
// if the modules are the same, visibility is trivially satisfied
56+
return true;
57+
}
5458
// if they're not in the same crate, it can't be visible
5559
if from_module.krate(db) != to_module.krate(db) {
5660
return false;
@@ -73,6 +77,10 @@ impl Visibility {
7377
Visibility::PubCrate(krate) => return from_module.krate(db) == krate,
7478
Visibility::Public => return true,
7579
};
80+
if from_module == to_module {
81+
// if the modules are the same, visibility is trivially satisfied
82+
return true;
83+
}
7684
// if they're not in the same crate, it can't be visible
7785
if def_map.krate() != to_module.krate(db) {
7886
return false;
@@ -100,9 +108,7 @@ impl Visibility {
100108
// `to_module` is not a block, so there is no parent def map to use.
101109
(None, _) => (),
102110
// `to_module` is at `def_map`'s block, no need to move further.
103-
(Some(a), Some(b)) if a == b => {
104-
cov_mark::hit!(nested_module_scoping);
105-
}
111+
(Some(a), Some(b)) if a == b => {}
106112
_ => {
107113
if let Some(parent) = to_module.def_map(db).parent() {
108114
to_module = parent;
@@ -161,6 +167,21 @@ impl Visibility {
161167
if mod_.krate(db) == krate { Some(Visibility::PubCrate(krate)) } else { None }
162168
}
163169
(Visibility::Module(mod_a, expl_a), Visibility::Module(mod_b, expl_b)) => {
170+
if mod_a == mod_b {
171+
// Most module visibilities are `pub(self)`, and assuming no errors
172+
// this will be the common and thus fast path.
173+
return Some(Visibility::Module(
174+
mod_a,
175+
match (expl_a, expl_b) {
176+
(VisibilityExplicitness::Explicit, _)
177+
| (_, VisibilityExplicitness::Explicit) => {
178+
VisibilityExplicitness::Explicit
179+
}
180+
_ => VisibilityExplicitness::Implicit,
181+
},
182+
));
183+
}
184+
164185
if mod_a.krate(db) != def_map.krate() || mod_b.krate(db) != def_map.krate() {
165186
return None;
166187
}
@@ -212,6 +233,21 @@ impl Visibility {
212233
if mod_.krate(db) == krate { Some(Visibility::Module(mod_, exp)) } else { None }
213234
}
214235
(Visibility::Module(mod_a, expl_a), Visibility::Module(mod_b, expl_b)) => {
236+
if mod_a == mod_b {
237+
// Most module visibilities are `pub(self)`, and assuming no errors
238+
// this will be the common and thus fast path.
239+
return Some(Visibility::Module(
240+
mod_a,
241+
match (expl_a, expl_b) {
242+
(VisibilityExplicitness::Explicit, _)
243+
| (_, VisibilityExplicitness::Explicit) => {
244+
VisibilityExplicitness::Explicit
245+
}
246+
_ => VisibilityExplicitness::Implicit,
247+
},
248+
));
249+
}
250+
215251
if mod_a.krate(db) != def_map.krate() || mod_b.krate(db) != def_map.krate() {
216252
return None;
217253
}

0 commit comments

Comments
 (0)