Auto merge of rust-lang#14176 - lowr:fix/assoc-func-vis-in-local-impl, r=Veykril

Fix associated item visibility in block-local impls

Fixes rust-lang#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

Author: bors

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)]

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) = &current_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) = &current_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");

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 {

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);

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),
         }

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();
         }

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}: ")?,

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;

crates/ide-diagnostics/src/handlers/private_assoc_item.rs

@@ -115,6 +115,44 @@ mod module {
 fn main(s: module::Struct) {
     s.method();
 }
+"#,
+        );
+    }
+
+    #[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
+}
 "#,
         );
     }