From 0dcec31553f0fe42d7c18e6471655e8fa2258bfc Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Mon, 1 Mar 2021 19:36:34 +0100 Subject: Fix `find_path` when inner items are present --- crates/hir_def/src/find_path.rs | 59 ++++++++++++++++++++++++++++++++-------- crates/hir_def/src/item_scope.rs | 22 ++++----------- crates/hir_def/src/lib.rs | 23 ++++++++++++++++ crates/hir_def/src/nameres.rs | 12 ++++++++ 4 files changed, 88 insertions(+), 28 deletions(-) diff --git a/crates/hir_def/src/find_path.rs b/crates/hir_def/src/find_path.rs index 5e2a711b8..3e19a7702 100644 --- a/crates/hir_def/src/find_path.rs +++ b/crates/hir_def/src/find_path.rs @@ -1,5 +1,7 @@ //! An algorithm to find a path to refer to a certain item. +use std::iter; + use hir_expand::name::{known, AsName, Name}; use rustc_hash::FxHashSet; use test_utils::mark; @@ -95,7 +97,7 @@ fn find_path_inner( item: ItemInNs, from: ModuleId, max_len: usize, - prefixed: Option, + mut prefixed: Option, ) -> Option { if max_len == 0 { return None; @@ -114,8 +116,9 @@ fn find_path_inner( } // - if the item is the crate root, return `crate` - let root = def_map.module_id(def_map.root()); + let root = def_map.crate_root(db); if item == ItemInNs::Types(ModuleDefId::ModuleId(root)) && def_map.block_id().is_none() { + // FIXME: the `block_id()` check should be unnecessary, but affects the result return Some(ModPath::from_segments(PathKind::Crate, Vec::new())); } @@ -165,7 +168,7 @@ fn find_path_inner( // - otherwise, look for modules containing (reexporting) it and import it from one of those - let crate_root = def_map.module_id(def_map.root()); + let crate_root = def_map.crate_root(db); let crate_attrs = db.attrs(crate_root.into()); let prefer_no_std = crate_attrs.by_key("no_std").exists(); let mut best_path = None; @@ -228,12 +231,16 @@ fn find_path_inner( } } - if let Some(mut prefix) = prefixed.map(PrefixKind::prefix) { - if matches!(prefix, PathKind::Crate | PathKind::Super(0)) && def_map.block_id().is_some() { - // Inner items cannot be referred to via `crate::` or `self::` paths. - prefix = PathKind::Plain; + // If the item is declared inside a block expression, don't use a prefix, as we don't handle + // that correctly (FIXME). + if let Some(item_module) = item.as_module_def_id().and_then(|did| did.module(db)) { + if item_module.def_map(db).block_id().is_some() && prefixed.is_some() { + mark::hit!(prefixed_in_block_expression); + prefixed = Some(PrefixKind::Plain); } + } + if let Some(prefix) = prefixed.map(PrefixKind::prefix) { best_path.or_else(|| { scope_name.map(|scope_name| ModPath::from_segments(prefix, vec![scope_name])) }) @@ -285,12 +292,12 @@ fn find_local_import_locations( let data = &def_map[from.local_id]; let mut worklist = data.children.values().map(|child| def_map.module_id(*child)).collect::>(); - let mut parent = data.parent; - while let Some(p) = parent { - worklist.push(def_map.module_id(p)); - parent = def_map[p].parent; + for ancestor in iter::successors(from.containing_module(db), |m| m.containing_module(db)) { + worklist.push(ancestor); } + let def_map = def_map.crate_root(db).def_map(db); + let mut seen: FxHashSet<_> = FxHashSet::default(); let mut locations = Vec::new(); @@ -301,7 +308,14 @@ fn find_local_import_locations( let ext_def_map; let data = if module.krate == from.krate { - &def_map[module.local_id] + if module.block.is_some() { + // Re-query the block's DefMap + ext_def_map = module.def_map(db); + &ext_def_map[module.local_id] + } else { + // Reuse the root DefMap + &def_map[module.local_id] + } } else { // The crate might reexport a module defined in another crate. ext_def_map = module.def_map(db); @@ -828,6 +842,7 @@ mod tests { #[test] fn inner_items_from_inner_module() { + mark::check!(prefixed_in_block_expression); check_found_path( r#" fn main() { @@ -869,4 +884,24 @@ mod tests { "super::Struct", ); } + + #[test] + fn outer_items_with_inner_items_present() { + check_found_path( + r#" + mod module { + pub struct CompleteMe; + } + + fn main() { + fn inner() {} + $0 + } + "#, + "module::CompleteMe", + "module::CompleteMe", + "crate::module::CompleteMe", + "self::module::CompleteMe", + ) + } } diff --git a/crates/hir_def/src/item_scope.rs b/crates/hir_def/src/item_scope.rs index ee46c3330..4e5daa2ff 100644 --- a/crates/hir_def/src/item_scope.rs +++ b/crates/hir_def/src/item_scope.rs @@ -12,8 +12,8 @@ use stdx::format_to; use test_utils::mark; use crate::{ - db::DefDatabase, per_ns::PerNs, visibility::Visibility, AdtId, BuiltinType, HasModule, ImplId, - LocalModuleId, Lookup, MacroDefId, ModuleDefId, ModuleId, TraitId, + db::DefDatabase, per_ns::PerNs, visibility::Visibility, AdtId, BuiltinType, ImplId, + LocalModuleId, MacroDefId, ModuleDefId, ModuleId, TraitId, }; #[derive(Copy, Clone)] @@ -375,19 +375,9 @@ impl ItemInNs { /// Returns the crate defining this item (or `None` if `self` is built-in). pub fn krate(&self, db: &dyn DefDatabase) -> Option { - Some(match self { - ItemInNs::Types(did) | ItemInNs::Values(did) => match did { - ModuleDefId::ModuleId(id) => id.krate, - ModuleDefId::FunctionId(id) => id.lookup(db).module(db).krate, - ModuleDefId::AdtId(id) => id.module(db).krate, - ModuleDefId::EnumVariantId(id) => id.parent.lookup(db).container.module(db).krate, - ModuleDefId::ConstId(id) => id.lookup(db).container.module(db).krate, - ModuleDefId::StaticId(id) => id.lookup(db).container.module(db).krate, - ModuleDefId::TraitId(id) => id.lookup(db).container.module(db).krate, - ModuleDefId::TypeAliasId(id) => id.lookup(db).module(db).krate, - ModuleDefId::BuiltinType(_) => return None, - }, - ItemInNs::Macros(id) => return Some(id.krate), - }) + match self { + ItemInNs::Types(did) | ItemInNs::Values(did) => did.module(db).map(|m| m.krate), + ItemInNs::Macros(id) => Some(id.krate), + } } } diff --git a/crates/hir_def/src/lib.rs b/crates/hir_def/src/lib.rs index 6802bc250..4498d94bb 100644 --- a/crates/hir_def/src/lib.rs +++ b/crates/hir_def/src/lib.rs @@ -97,6 +97,10 @@ impl ModuleId { pub fn krate(&self) -> CrateId { self.krate } + + pub fn containing_module(&self, db: &dyn db::DefDatabase) -> Option { + self.def_map(db).containing_module(self.local_id) + } } /// An ID of a module, **local** to a specific crate @@ -529,6 +533,25 @@ impl HasModule for StaticLoc { } } +impl ModuleDefId { + /// Returns the module containing `self` (or `self`, if `self` is itself a module). + /// + /// Returns `None` if `self` refers to a primitive type. + pub fn module(&self, db: &dyn db::DefDatabase) -> Option { + Some(match self { + ModuleDefId::ModuleId(id) => *id, + ModuleDefId::FunctionId(id) => id.lookup(db).module(db), + ModuleDefId::AdtId(id) => id.module(db), + ModuleDefId::EnumVariantId(id) => id.parent.lookup(db).container.module(db), + ModuleDefId::ConstId(id) => id.lookup(db).container.module(db), + ModuleDefId::StaticId(id) => id.lookup(db).container.module(db), + ModuleDefId::TraitId(id) => id.lookup(db).container.module(db), + ModuleDefId::TypeAliasId(id) => id.lookup(db).module(db), + ModuleDefId::BuiltinType(_) => return None, + }) + } +} + impl AttrDefId { pub fn krate(&self, db: &dyn db::DefDatabase) -> CrateId { match self { diff --git a/crates/hir_def/src/nameres.rs b/crates/hir_def/src/nameres.rs index 6a3456f2e..003d668ca 100644 --- a/crates/hir_def/src/nameres.rs +++ b/crates/hir_def/src/nameres.rs @@ -343,6 +343,18 @@ impl DefMap { Some(self.block?.parent) } + /// 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 { + match &self[local_mod].parent { + Some(parent) => Some(self.module_id(*parent)), + None => match &self.block { + Some(block) => Some(block.parent), + None => None, + }, + } + } + // FIXME: this can use some more human-readable format (ideally, an IR // even), as this should be a great debugging aid. pub fn dump(&self, db: &dyn DefDatabase) -> String { -- cgit v1.2.3