From 0231e4ac77dacf6ca30f6b68c6081415f2da54ba Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 12 Jun 2020 13:01:20 +0200 Subject: find_path: return shorter paths for external items If a containing module is already in scope, there's no need to use the full path to the item. --- crates/ra_hir_def/src/find_path.rs | 59 ++++++++++++++++++++++++++++++++----- crates/ra_hir_def/src/import_map.rs | 39 ++++++++++++++++-------- 2 files changed, 77 insertions(+), 21 deletions(-) diff --git a/crates/ra_hir_def/src/find_path.rs b/crates/ra_hir_def/src/find_path.rs index a7f59e028..06701a830 100644 --- a/crates/ra_hir_def/src/find_path.rs +++ b/crates/ra_hir_def/src/find_path.rs @@ -159,10 +159,16 @@ fn find_path_inner( let crate_graph = db.crate_graph(); let extern_paths = crate_graph[from.krate].dependencies.iter().filter_map(|dep| { let import_map = db.import_map(dep.crate_id); - import_map.path_of(item).map(|modpath| { - let mut modpath = modpath.clone(); - modpath.segments.insert(0, dep.as_name()); - modpath + import_map.import_info_for(item).and_then(|info| { + // Determine best path for containing module and append last segment from `info`. + let mut path = find_path_inner( + db, + ItemInNs::Types(ModuleDefId::ModuleId(info.container)), + from, + best_path_len - 1, + )?; + path.segments.push(info.path.segments.last().unwrap().clone()); + Some(path) }) }); @@ -299,8 +305,8 @@ mod tests { /// `code` needs to contain a cursor marker; checks that `find_path` for the /// item the `path` refers to returns that same path when called from the /// module the cursor is in. - fn check_found_path(code: &str, path: &str) { - let (db, pos) = TestDB::with_position(code); + fn check_found_path(ra_fixture: &str, path: &str) { + let (db, pos) = TestDB::with_position(ra_fixture); let module = db.module_for_file(pos.file_id); let parsed_path_file = ra_syntax::SourceFile::parse(&format!("use {};", path)); let ast_path = parsed_path_file @@ -420,7 +426,6 @@ mod tests { #[test] fn different_crate_renamed() { - // Even if a local path exists, if the item is defined externally, prefer an external path. let code = r#" //- /main.rs crate:main deps:std extern crate std as std_renamed; @@ -428,7 +433,45 @@ mod tests { //- /std.rs crate:std pub struct S; "#; - check_found_path(code, "std::S"); + check_found_path(code, "std_renamed::S"); + } + + #[test] + fn partially_imported() { + // Tests that short paths are used even for external items, when parts of the path are + // already in scope. + check_found_path( + r#" + //- /main.rs crate:main deps:ra_syntax + + use ra_syntax::ast; + <|> + + //- /lib.rs crate:ra_syntax + pub mod ast { + pub enum ModuleItem { + A, B, C, + } + } + "#, + "ast::ModuleItem", + ); + + check_found_path( + r#" + //- /main.rs crate:main deps:ra_syntax + + <|> + + //- /lib.rs crate:ra_syntax + pub mod ast { + pub enum ModuleItem { + A, B, C, + } + } + "#, + "ra_syntax::ast::ModuleItem", + ); } #[test] diff --git a/crates/ra_hir_def/src/import_map.rs b/crates/ra_hir_def/src/import_map.rs index 36b4fdd81..68e20d06b 100644 --- a/crates/ra_hir_def/src/import_map.rs +++ b/crates/ra_hir_def/src/import_map.rs @@ -17,6 +17,15 @@ use crate::{ type FxIndexMap = IndexMap>; +/// Item import details stored in the `ImportMap`. +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct ImportInfo { + /// A path that can be used to import the item, relative to the crate's root. + pub path: ModPath, + /// The module containing this item. + pub container: ModuleId, +} + /// A map from publicly exported items to the path needed to import/name them from a downstream /// crate. /// @@ -26,7 +35,7 @@ type FxIndexMap = IndexMap>; /// Note that all paths are relative to the containing crate's root, so the crate name still needs /// to be prepended to the `ModPath` before the path is valid. pub struct ImportMap { - map: FxIndexMap, + map: FxIndexMap, /// List of keys stored in `map`, sorted lexicographically by their `ModPath`. Indexed by the /// values returned by running `fst`. @@ -78,12 +87,12 @@ impl ImportMap { let path = mk_path(); match import_map.entry(item) { Entry::Vacant(entry) => { - entry.insert(path); + entry.insert(ImportInfo { path, container: module }); } Entry::Occupied(mut entry) => { // If the new path is shorter, prefer that one. - if path.len() < entry.get().len() { - *entry.get_mut() = path; + if path.len() < entry.get().path.len() { + *entry.get_mut() = ImportInfo { path, container: module }; } else { continue; } @@ -119,7 +128,7 @@ impl ImportMap { let start = last_batch_start; last_batch_start = idx + 1; - let key = fst_path(&importables[start].1); + let key = fst_path(&importables[start].1.path); builder.insert(key, start as u64).unwrap(); } @@ -132,6 +141,10 @@ impl ImportMap { /// Returns the `ModPath` needed to import/mention `item`, relative to this crate's root. pub fn path_of(&self, item: ItemInNs) -> Option<&ModPath> { + Some(&self.map.get(&item)?.path) + } + + pub fn import_info_for(&self, item: ItemInNs) -> Option<&ImportInfo> { self.map.get(&item) } } @@ -150,13 +163,13 @@ impl fmt::Debug for ImportMap { let mut importable_paths: Vec<_> = self .map .iter() - .map(|(item, modpath)| { + .map(|(item, info)| { let ns = match item { ItemInNs::Types(_) => "t", ItemInNs::Values(_) => "v", ItemInNs::Macros(_) => "m", }; - format!("- {} ({})", modpath, ns) + format!("- {} ({})", info.path, ns) }) .collect(); @@ -171,9 +184,9 @@ fn fst_path(path: &ModPath) -> String { s } -fn cmp((_, lhs): &(&ItemInNs, &ModPath), (_, rhs): &(&ItemInNs, &ModPath)) -> Ordering { - let lhs_str = fst_path(lhs); - let rhs_str = fst_path(rhs); +fn cmp((_, lhs): &(&ItemInNs, &ImportInfo), (_, rhs): &(&ItemInNs, &ImportInfo)) -> Ordering { + let lhs_str = fst_path(&lhs.path); + let rhs_str = fst_path(&rhs.path); lhs_str.cmp(&rhs_str) } @@ -243,7 +256,7 @@ pub fn search_dependencies<'a>( let importables = &import_map.importables[indexed_value.value as usize..]; // Path shared by the importable items in this group. - let path = &import_map.map[&importables[0]]; + let path = &import_map.map[&importables[0]].path; if query.anchor_end { // Last segment must match query. @@ -256,14 +269,14 @@ pub fn search_dependencies<'a>( // Add the items from this `ModPath` group. Those are all subsequent items in // `importables` whose paths match `path`. let iter = importables.iter().copied().take_while(|item| { - let item_path = &import_map.map[item]; + let item_path = &import_map.map[item].path; fst_path(item_path) == fst_path(path) }); if query.case_sensitive { // FIXME: This does not do a subsequence match. res.extend(iter.filter(|item| { - let item_path = &import_map.map[item]; + let item_path = &import_map.map[item].path; item_path.to_string().contains(&query.query) })); } else { -- cgit v1.2.3