Skip to content

Commit 6ff82fb

Browse files
VeykrilVeykril
committed
Optimize private visibility resolution
1 parent ece523c commit 6ff82fb

File tree

10 files changed

+83
-39
lines changed

10 files changed

+83
-39
lines changed

‎src/tools/rust-analyzer/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 ");

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,6 @@ fn main() {
397397
fn underscore_import() {
398398
// This used to panic, because the default (private) visibility inside block expressions would
399399
// point into the containing `DefMap`, which visibilities should never be able to do.
400-
cov_mark::check!(adjust_vis_in_block_def_map);
401400
check_at(
402401
r#"
403402
mod m {

‎src/tools/rust-analyzer/crates/hir-def/src/find_path.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1287,7 +1287,6 @@ $0
12871287

12881288
#[test]
12891289
fn explicit_private_imports_crate() {
1290-
cov_mark::check!(explicit_private_imports);
12911290
check_found_path(
12921291
r#"
12931292
//- /main.rs

‎src/tools/rust-analyzer/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`.

‎src/tools/rust-analyzer/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

‎src/tools/rust-analyzer/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 _};

‎src/tools/rust-analyzer/crates/hir-def/src/lib.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ impl<N: AstIdNode> HasModule for ItemLoc<N> {
149149

150150
#[derive(Debug)]
151151
pub struct AssocItemLoc<N: AstIdNode> {
152+
// FIXME: Store this as an erased `salsa::Id` to save space
152153
pub container: ItemContainerId,
153154
pub id: AstId<N>,
154155
}
@@ -577,6 +578,7 @@ pub type LocalModuleId = Idx<nameres::ModuleData>;
577578

578579
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
579580
pub struct FieldId {
581+
// FIXME: Store this as an erased `salsa::Id` to save space
580582
pub parent: VariantId,
581583
pub local_id: LocalFieldId,
582584
}
@@ -592,6 +594,7 @@ pub struct TupleFieldId {
592594

593595
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
594596
pub struct TypeOrConstParamId {
597+
// FIXME: Store this as an erased `salsa::Id` to save space
595598
pub parent: GenericDefId,
596599
pub local_id: LocalTypeOrConstParamId,
597600
}
@@ -650,6 +653,7 @@ impl From<ConstParamId> for TypeOrConstParamId {
650653

651654
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
652655
pub struct LifetimeParamId {
656+
// FIXME: Store this as an erased `salsa::Id` to save space
653657
pub parent: GenericDefId,
654658
pub local_id: LocalLifetimeParamId,
655659
}

‎src/tools/rust-analyzer/crates/hir-def/src/nameres/path_resolution.rs

Lines changed: 21 additions & 15 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,36 @@ 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 && !within_impl {
137+
vis = Visibility::Module(self.module_id(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(self.module_id(original_module), *explicitness)
130149
}
131150
RawVisibility::Public => Visibility::Public,
132151
RawVisibility::PubCrate => Visibility::PubCrate(self.krate),
133152
};
134-
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 && !within_impl {
141-
cov_mark::hit!(adjust_vis_in_block_def_map);
142-
vis = Visibility::Module(self.module_id(Self::ROOT), mv);
143-
tracing::debug!("visibility {:?} points outside DefMap, adjusting to {:?}", m, vis);
144-
}
145-
}
146-
147153
Some(vis)
148154
}
149155

‎src/tools/rust-analyzer/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
}

‎src/tools/rust-analyzer/crates/hir-def/src/visibility.rs

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ impl Visibility {
4747
Visibility::PubCrate(krate) => return from_module.krate == krate,
4848
Visibility::Public => return true,
4949
};
50+
if from_module == to_module {
51+
// if the modules are the same, visibility is trivially satisfied
52+
return true;
53+
}
5054
// if they're not in the same crate, it can't be visible
5155
if from_module.krate != to_module.krate {
5256
return false;
@@ -70,6 +74,11 @@ impl Visibility {
7074
if def_map.krate() != to_module.krate {
7175
return false;
7276
}
77+
78+
if from_module == to_module.local_id && def_map.block_id() == to_module.block {
79+
// if the modules are the same, visibility is trivially satisfied
80+
return true;
81+
}
7382
Self::is_visible_from_def_map_(db, def_map, to_module, from_module)
7483
}
7584

@@ -93,9 +102,7 @@ impl Visibility {
93102
// `to_module` is not a block, so there is no parent def map to use.
94103
(None, _) => (),
95104
// `to_module` is at `def_map`'s block, no need to move further.
96-
(Some(a), Some(b)) if a == b => {
97-
cov_mark::hit!(is_visible_from_same_block_def_map);
98-
}
105+
(Some(a), Some(b)) if a == b => {}
99106
_ => {
100107
if let Some(parent) = to_module.def_map(db).parent() {
101108
to_module = parent;
@@ -153,7 +160,22 @@ impl Visibility {
153160
}
154161
}
155162
(Visibility::Module(mod_a, expl_a), Visibility::Module(mod_b, expl_b)) => {
156-
if mod_a.krate != def_map.krate() || mod_b.krate != def_map.krate() {
163+
if mod_a == mod_b {
164+
// Most module visibilities are `pub(self)`, and assuming no errors
165+
// this will be the common and thus fast path.
166+
return Some(Visibility::Module(
167+
mod_a,
168+
match (expl_a, expl_b) {
169+
(VisibilityExplicitness::Explicit, _)
170+
| (_, VisibilityExplicitness::Explicit) => {
171+
VisibilityExplicitness::Explicit
172+
}
173+
_ => VisibilityExplicitness::Implicit,
174+
},
175+
));
176+
}
177+
178+
if mod_a.krate() != def_map.krate() || mod_b.krate() != def_map.krate() {
157179
return None;
158180
}
159181

@@ -201,7 +223,22 @@ impl Visibility {
201223
if mod_.krate == krate { Some(Visibility::Module(mod_, exp)) } else { None }
202224
}
203225
(Visibility::Module(mod_a, expl_a), Visibility::Module(mod_b, expl_b)) => {
204-
if mod_a.krate != def_map.krate() || mod_b.krate != def_map.krate() {
226+
if mod_a == mod_b {
227+
// Most module visibilities are `pub(self)`, and assuming no errors
228+
// this will be the common and thus fast path.
229+
return Some(Visibility::Module(
230+
mod_a,
231+
match (expl_a, expl_b) {
232+
(VisibilityExplicitness::Explicit, _)
233+
| (_, VisibilityExplicitness::Explicit) => {
234+
VisibilityExplicitness::Explicit
235+
}
236+
_ => VisibilityExplicitness::Implicit,
237+
},
238+
));
239+
}
240+
241+
if mod_a.krate() != def_map.krate() || mod_b.krate() != def_map.krate() {
205242
return None;
206243
}
207244

0 commit comments

Comments
 (0)