Optimize private visibility resolution

Veykril · Veykril · commit b3768cdc0e69 · 2025-06-15T10:25:45.000+02:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
                 }
 

Original file line number	Diff line number	Diff line change
`@@ -141,10 +141,11 @@ pub fn print_variant_body_hir(db: &dyn DefDatabase, owner: VariantId, edition: E`
`141`	`141`	`let FieldData { name, type_ref, visibility, is_unsafe } = data;`
`142`	`142`	`match visibility {`
`143`	`143`	`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))`
`145`	`145`	`}`
`146`	`146`	`crate::item_tree::RawVisibility::Public => w!(p, "pub "),`
`147`	`147`	`crate::item_tree::RawVisibility::PubCrate => w!(p, "pub(crate) "),`
	`148`	`+ crate::item_tree::RawVisibility::PubSelf(_) => w!(p, "pub(self) "),`
`148`	`149`	`}`
`149`	`150`	`if *is_unsafe {`
`150`	`151`	`w!(p, "unsafe ");`
Original file line number	Diff line number	Diff line change
`@@ -108,10 +108,11 @@ impl Printer<'_> {`
`108`	`108`	`fn print_visibility(&mut self, vis: RawVisibilityId) {`
`109`	`109`	`match &self.tree[vis] {`
`110`	`110`	`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))`
`112`	`112`	`}`
`113`	`113`	`RawVisibility::Public => w!(self, "pub "),`
`114`	`114`	`RawVisibility::PubCrate => w!(self, "pub(crate) "),`
	`115`	`+ RawVisibility::PubSelf(_) => w!(self, "pub(self) "),`
`115`	`116`	`};`
`116`	`117`	`}`
`117`	`118`
Original file line number	Diff line number	Diff line change
`@@ -305,6 +305,9 @@ impl<'db> Resolver<'db> {`
`305`	`305`	`}),`
`306`	`306`	`)`
`307`	`307`	`}`
	`308`	`+ RawVisibility::PubSelf(explicitness) => {`
	`309`	`+ Some(Visibility::Module(self.module(), *explicitness))`
	`310`	`+ }`
`308`	`311`	`RawVisibility::PubCrate => Some(Visibility::PubCrate(self.krate())),`
`309`	`312`	`RawVisibility::Public => Some(Visibility::Public),`
`310`	`313`	`}`