Unnamed repository; edit this file 'description' to name the repository.
Auto merge of #14176 - lowr:fix/assoc-func-vis-in-local-impl, r=Veykril
Fix associated item visibility in block-local impls
Fixes #14046
When we're resolving visibility of block-local items...
> `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. ([link])
...unless we're resolving visibility of associated items in block-local impls, because that impl is semantically "hoisted" to the nearest (non-block) module. With this PR, we skip the adjustment for such items.
Since visibility representation of those items is modified, this PR also adjusts visibility rendering in `HirDisplay`.
[link]: https://github.com/rust-lang/rust-analyzer/blob/a6603fc21d50b3386a488c96225b2d1fd492e533/crates/hir-def/src/nameres/path_resolution.rs#L101-L103
| -rw-r--r-- | crates/hir-def/src/lib.rs | 2 | ||||
| -rw-r--r-- | crates/hir-def/src/nameres.rs | 18 | ||||
| -rw-r--r-- | crates/hir-def/src/nameres/collector.rs | 12 | ||||
| -rw-r--r-- | crates/hir-def/src/nameres/path_resolution.rs | 4 | ||||
| -rw-r--r-- | crates/hir-def/src/resolver.rs | 4 | ||||
| -rw-r--r-- | crates/hir-def/src/visibility.rs | 7 | ||||
| -rw-r--r-- | crates/hir/src/display.rs | 33 | ||||
| -rw-r--r-- | crates/hir/src/lib.rs | 16 | ||||
| -rw-r--r-- | crates/ide-diagnostics/src/handlers/private_assoc_item.rs | 38 | ||||
| -rw-r--r-- | crates/ide/src/hover/tests.rs | 178 |
10 files changed, 282 insertions, 30 deletions
diff --git a/crates/hir-def/src/lib.rs b/crates/hir-def/src/lib.rs index d07c5fb67c..2aab1ccd91 100644 --- a/crates/hir-def/src/lib.rs +++ b/crates/hir-def/src/lib.rs @@ -128,7 +128,7 @@ impl ModuleId { } } -/// An ID of a module, **local** to a specific crate +/// An ID of a module, **local** to a `DefMap`. pub type LocalModuleId = Idx<nameres::ModuleData>; #[derive(Debug)] diff --git a/crates/hir-def/src/nameres.rs b/crates/hir-def/src/nameres.rs index 393747d304..a7ce036051 100644 --- a/crates/hir-def/src/nameres.rs +++ b/crates/hir-def/src/nameres.rs @@ -342,7 +342,7 @@ impl DefMap { } pub(crate) fn block_id(&self) -> Option<BlockId> { - self.block.as_ref().map(|block| block.block) + self.block.map(|block| block.block) } pub(crate) fn prelude(&self) -> Option<ModuleId> { @@ -354,7 +354,7 @@ impl DefMap { } pub fn module_id(&self, local_id: LocalModuleId) -> ModuleId { - let block = self.block.as_ref().map(|b| b.block); + let block = self.block.map(|b| b.block); ModuleId { krate: self.krate, local_id, block } } @@ -432,9 +432,9 @@ impl DefMap { /// Returns the module containing `local_mod`, either the parent `mod`, or the module containing /// the block, if `self` corresponds to a block expression. pub fn containing_module(&self, local_mod: LocalModuleId) -> Option<ModuleId> { - match &self[local_mod].parent { - Some(parent) => Some(self.module_id(*parent)), - None => self.block.as_ref().map(|block| block.parent), + match self[local_mod].parent { + Some(parent) => Some(self.module_id(parent)), + None => self.block.map(|block| block.parent), } } @@ -444,11 +444,11 @@ impl DefMap { let mut buf = String::new(); let mut arc; let mut current_map = self; - while let Some(block) = ¤t_map.block { + while let Some(block) = current_map.block { go(&mut buf, current_map, "block scope", current_map.root); buf.push('\n'); arc = block.parent.def_map(db); - current_map = &*arc; + current_map = &arc; } go(&mut buf, current_map, "crate", current_map.root); return buf; @@ -472,10 +472,10 @@ impl DefMap { let mut buf = String::new(); let mut arc; let mut current_map = self; - while let Some(block) = ¤t_map.block { + while let Some(block) = current_map.block { format_to!(buf, "{:?} in {:?}\n", block.block, block.parent); arc = block.parent.def_map(db); - current_map = &*arc; + current_map = &arc; } format_to!(buf, "crate scope\n"); diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs index 4b39a20d86..e3704bf216 100644 --- a/crates/hir-def/src/nameres/collector.rs +++ b/crates/hir-def/src/nameres/collector.rs @@ -666,8 +666,10 @@ impl DefCollector<'_> { macro_: Macro2Id, vis: &RawVisibility, ) { - let vis = - self.def_map.resolve_visibility(self.db, module_id, vis).unwrap_or(Visibility::Public); + let vis = self + .def_map + .resolve_visibility(self.db, module_id, vis, false) + .unwrap_or(Visibility::Public); self.def_map.modules[module_id].scope.declare(macro_.into()); self.update( module_id, @@ -831,7 +833,7 @@ impl DefCollector<'_> { let mut def = directive.status.namespaces(); let vis = self .def_map - .resolve_visibility(self.db, module_id, &directive.import.visibility) + .resolve_visibility(self.db, module_id, &directive.import.visibility, false) .unwrap_or(Visibility::Public); match import.kind { @@ -1547,7 +1549,7 @@ impl ModCollector<'_, '_> { }; let resolve_vis = |def_map: &DefMap, visibility| { def_map - .resolve_visibility(db, self.module_id, visibility) + .resolve_visibility(db, self.module_id, visibility, false) .unwrap_or(Visibility::Public) }; @@ -1823,7 +1825,7 @@ impl ModCollector<'_, '_> { ) -> LocalModuleId { let def_map = &mut self.def_collector.def_map; let vis = def_map - .resolve_visibility(self.def_collector.db, self.module_id, visibility) + .resolve_visibility(self.def_collector.db, self.module_id, visibility, false) .unwrap_or(Visibility::Public); let modules = &mut def_map.modules; let origin = match definition { diff --git a/crates/hir-def/src/nameres/path_resolution.rs b/crates/hir-def/src/nameres/path_resolution.rs index 1d9d5cccde..25478481dd 100644 --- a/crates/hir-def/src/nameres/path_resolution.rs +++ b/crates/hir-def/src/nameres/path_resolution.rs @@ -78,6 +78,7 @@ impl DefMap { // pub(path) // ^^^^ this visibility: &RawVisibility, + within_impl: bool, ) -> Option<Visibility> { let mut vis = match visibility { RawVisibility::Module(path) => { @@ -102,7 +103,8 @@ impl DefMap { // `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) = vis { - if self.block_id() != m.block { + // ...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())); tracing::debug!("visibility {:?} points outside DefMap, adjusting to {:?}", m, vis); diff --git a/crates/hir-def/src/resolver.rs b/crates/hir-def/src/resolver.rs index b2323915c1..36d8b24e9c 100644 --- a/crates/hir-def/src/resolver.rs +++ b/crates/hir-def/src/resolver.rs @@ -214,10 +214,12 @@ impl Resolver { db: &dyn DefDatabase, visibility: &RawVisibility, ) -> Option<Visibility> { + let within_impl = + self.scopes().find(|scope| matches!(scope, Scope::ImplDefScope(_))).is_some(); match visibility { RawVisibility::Module(_) => { let (item_map, module) = self.item_scope(); - item_map.resolve_visibility(db, module, visibility) + item_map.resolve_visibility(db, module, visibility, within_impl) } RawVisibility::Public => Some(Visibility::Public), } diff --git a/crates/hir-def/src/visibility.rs b/crates/hir-def/src/visibility.rs index 087268a9ec..c9fcaae56c 100644 --- a/crates/hir-def/src/visibility.rs +++ b/crates/hir-def/src/visibility.rs @@ -11,7 +11,7 @@ use crate::{ nameres::DefMap, path::{ModPath, PathKind}, resolver::HasResolver, - ConstId, FunctionId, HasModule, LocalFieldId, ModuleId, VariantId, + ConstId, FunctionId, HasModule, LocalFieldId, LocalModuleId, ModuleId, VariantId, }; /// Visibility of an item, not yet resolved. @@ -120,7 +120,7 @@ impl Visibility { self, db: &dyn DefDatabase, def_map: &DefMap, - mut from_module: crate::LocalModuleId, + mut from_module: LocalModuleId, ) -> bool { let mut to_module = match self { Visibility::Module(m) => m, @@ -142,7 +142,8 @@ impl Visibility { arc = to_module.def_map(db); &arc }; - let is_block_root = matches!(to_module.block, Some(_) if to_module_def_map[to_module.local_id].parent.is_none()); + let is_block_root = + to_module.block.is_some() && to_module_def_map[to_module.local_id].parent.is_none(); if is_block_root { to_module = to_module_def_map.containing_module(to_module.local_id).unwrap(); } diff --git a/crates/hir/src/display.rs b/crates/hir/src/display.rs index 0d19420127..66bf2a2900 100644 --- a/crates/hir/src/display.rs +++ b/crates/hir/src/display.rs @@ -17,15 +17,23 @@ use hir_ty::{ }; use crate::{ - Adt, Const, ConstParam, Enum, Field, Function, GenericParam, HasCrate, HasVisibility, - LifetimeParam, Macro, Module, Static, Struct, Trait, TyBuilder, Type, TypeAlias, - TypeOrConstParam, TypeParam, Union, Variant, + Adt, AsAssocItem, AssocItemContainer, Const, ConstParam, Enum, Field, Function, GenericParam, + HasCrate, HasVisibility, LifetimeParam, Macro, Module, Static, Struct, Trait, TyBuilder, Type, + TypeAlias, TypeOrConstParam, TypeParam, Union, Variant, }; impl HirDisplay for Function { fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> { - let data = f.db.function_data(self.id); - write_visibility(self.module(f.db).id, self.visibility(f.db), f)?; + let db = f.db; + let data = db.function_data(self.id); + let container = self.as_assoc_item(db).map(|it| it.container(db)); + let mut module = self.module(db); + if let Some(AssocItemContainer::Impl(_)) = container { + // Block-local impls are "hoisted" to the nearest (non-block) module. + module = module.nearest_non_block_module(db); + } + let module_id = module.id; + write_visibility(module_id, self.visibility(db), f)?; if data.has_default_kw() { f.write_str("default ")?; } @@ -35,7 +43,7 @@ impl HirDisplay for Function { if data.has_async_kw() { f.write_str("async ")?; } - if self.is_unsafe_to_call(f.db) { + if self.is_unsafe_to_call(db) { f.write_str("unsafe ")?; } if let Some(abi) = &data.abi { @@ -50,7 +58,7 @@ impl HirDisplay for Function { let write_self_param = |ty: &TypeRef, f: &mut HirFormatter<'_>| match ty { TypeRef::Path(p) if p.is_self_type() => f.write_str("self"), - TypeRef::Reference(inner, lifetime, mut_) if matches!(&**inner,TypeRef::Path(p) if p.is_self_type()) => + TypeRef::Reference(inner, lifetime, mut_) if matches!(&**inner, TypeRef::Path(p) if p.is_self_type()) => { f.write_char('&')?; if let Some(lifetime) = lifetime { @@ -442,8 +450,15 @@ fn write_where_clause(def: GenericDefId, f: &mut HirFormatter<'_>) -> Result<(), impl HirDisplay for Const { fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> { - write_visibility(self.module(f.db).id, self.visibility(f.db), f)?; - let data = f.db.const_data(self.id); + let db = f.db; + let container = self.as_assoc_item(db).map(|it| it.container(db)); + let mut module = self.module(db); + if let Some(AssocItemContainer::Impl(_)) = container { + // Block-local impls are "hoisted" to the nearest (non-block) module. + module = module.nearest_non_block_module(db); + } + write_visibility(module.id, self.visibility(db), f)?; + let data = db.const_data(self.id); f.write_str("const ")?; match &data.name { Some(name) => write!(f, "{name}: ")?, diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 921e64ad93..c64106d3af 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -46,7 +46,7 @@ use hir_def::{ item_tree::ItemTreeNode, lang_item::{LangItem, LangItemTarget}, layout::{Layout, LayoutError, ReprOptions}, - nameres::{self, diagnostics::DefDiagnostic}, + nameres::{self, diagnostics::DefDiagnostic, ModuleOrigin}, per_ns::PerNs, resolver::{HasResolver, Resolver}, src::HasSource as _, @@ -488,6 +488,20 @@ impl Module { Some(Module { id: def_map.module_id(parent_id) }) } + /// Finds nearest non-block ancestor `Module` (`self` included). + fn nearest_non_block_module(self, db: &dyn HirDatabase) -> Module { + let mut id = self.id; + loop { + let def_map = id.def_map(db.upcast()); + let origin = def_map[id.local_id].origin; + if matches!(origin, ModuleOrigin::BlockExpr { .. }) { + id = id.containing_module(db.upcast()).expect("block without parent module") + } else { + return Module { id }; + } + } + } + pub fn path_to_root(self, db: &dyn HirDatabase) -> Vec<Module> { let mut res = vec![self]; let mut curr = self; diff --git a/crates/ide-diagnostics/src/handlers/private_assoc_item.rs b/crates/ide-diagnostics/src/handlers/private_assoc_item.rs index 0b3121c765..67da5c7f27 100644 --- a/crates/ide-diagnostics/src/handlers/private_assoc_item.rs +++ b/crates/ide-diagnostics/src/handlers/private_assoc_item.rs @@ -118,4 +118,42 @@ fn main(s: module::Struct) { "#, ); } + + #[test] + fn can_see_through_top_level_anonymous_const() { + // regression test for #14046. + check_diagnostics( + r#" +struct S; +mod m { + const _: () = { + impl crate::S { + pub(crate) fn method(self) {} + pub(crate) const A: usize = 42; + } + }; + mod inner { + const _: () = { + impl crate::S { + pub(crate) fn method2(self) {} + pub(crate) const B: usize = 42; + pub(super) fn private(self) {} + pub(super) const PRIVATE: usize = 42; + } + }; + } +} +fn main() { + S.method(); + S::A; + S.method2(); + S::B; + S.private(); + //^^^^^^^^^^^ error: function `private` is private + S::PRIVATE; + //^^^^^^^^^^ error: const `PRIVATE` is private +} +"#, + ); + } } diff --git a/crates/ide/src/hover/tests.rs b/crates/ide/src/hover/tests.rs index b29b57d134..42f7c4339c 100644 --- a/crates/ide/src/hover/tests.rs +++ b/crates/ide/src/hover/tests.rs @@ -5619,3 +5619,181 @@ fn main() { "#]], ); } + +#[test] +fn assoc_fn_in_block_local_impl() { + check( + r#" +struct S; +mod m { + const _: () = { + impl crate::S { + pub(crate) fn foo() {} + } + }; +} +fn test() { + S::foo$0(); +} +"#, + expect![[r#" + *foo* + + ```rust + test::S + ``` + + ```rust + pub(crate) fn foo() + ``` + "#]], + ); + + check( + r#" +struct S; +mod m { + const _: () = { + const _: () = { + impl crate::S { + pub(crate) fn foo() {} + } + }; + }; +} +fn test() { + S::foo$0(); +} +"#, + expect![[r#" + *foo* + + ```rust + test::S + ``` + + ```rust + pub(crate) fn foo() + ``` + "#]], + ); + + check( + r#" +struct S; +mod m { + mod inner { + const _: () = { + impl crate::S { + pub(super) fn foo() {} + } + }; + } + + fn test() { + crate::S::foo$0(); + } +} +"#, + expect![[r#" + *foo* + + ```rust + test::S + ``` + + ```rust + pub(super) fn foo() + ``` + "#]], + ); +} + +#[test] +fn assoc_const_in_block_local_impl() { + check( + r#" +struct S; +mod m { + const _: () = { + impl crate::S { + pub(crate) const A: () = (); + } + }; +} +fn test() { + S::A$0; +} +"#, + expect![[r#" + *A* + + ```rust + test + ``` + + ```rust + pub(crate) const A: () = () + ``` + "#]], + ); + + check( + r#" +struct S; +mod m { + const _: () = { + const _: () = { + impl crate::S { + pub(crate) const A: () = (); + } + }; + }; +} +fn test() { + S::A$0; +} +"#, + expect![[r#" + *A* + + ```rust + test + ``` + + ```rust + pub(crate) const A: () = () + ``` + "#]], + ); + + check( + r#" +struct S; +mod m { + mod inner { + const _: () = { + impl crate::S { + pub(super) const A: () = (); + } + }; + } + + fn test() { + crate::S::A$0; + } +} +"#, + expect![[r#" + *A* + + ```rust + test + ``` + + ```rust + pub(super) const A: () = () + ``` + "#]], + ); +} |