Skip to content

fix: Fix associated item visibility in block-local impls #14176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/hir-def/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
18 changes: 9 additions & 9 deletions crates/hir-def/src/nameres.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand All @@ -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 }
}

Expand Down Expand Up @@ -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),
}
}

Expand All @@ -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;
Expand All @@ -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");
Expand Down
12 changes: 7 additions & 5 deletions crates/hir-def/src/nameres/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
};

Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 3 additions & 1 deletion crates/hir-def/src/nameres/path_resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ impl DefMap {
// pub(path)
// ^^^^ this
visibility: &RawVisibility,
within_impl: bool,
) -> Option<Visibility> {
let mut vis = match visibility {
RawVisibility::Module(path) => {
Expand All @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion crates/hir-def/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
Expand Down
7 changes: 4 additions & 3 deletions crates/hir-def/src/visibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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();
}
Expand Down
33 changes: 24 additions & 9 deletions crates/hir/src/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 ")?;
}
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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}: ")?,
Expand Down
16 changes: 15 additions & 1 deletion crates/hir/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 _,
Expand Down Expand Up @@ -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 { .. }) {
Comment on lines +495 to +497
Copy link
Contributor Author

@lowr lowr Feb 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a simpler way to check whether a hir::Module is a block? I thought I could do better but failed badly...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ModuleId contained in a hir::Module has a block field that's set when its a block

Copy link
Contributor Author

@lowr lowr Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block is also set for a non-block module defined inside a block module, because it's derived from a DefMap for block module. It is rare but I'd like to support as much as possible unless it breaks our incremental model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, then I think this is the appropriate way

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;
Expand Down
38 changes: 38 additions & 0 deletions crates/ide-diagnostics/src/handlers/private_assoc_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
"#,
);
}
Expand Down
Loading