From e1d6981f90ba7852e949141dad0add855b57184a Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 25 Sep 2020 15:19:22 +0200 Subject: Don't unnecessarily unnest imports for import insertion --- .../src/handlers/add_missing_impl_members.rs | 35 +++++ crates/assists/src/handlers/auto_import.rs | 33 +++- crates/hir/src/code_model.rs | 10 ++ crates/hir_def/src/find_path.rs | 170 +++++++++++++++------ 4 files changed, 203 insertions(+), 45 deletions(-) diff --git a/crates/assists/src/handlers/add_missing_impl_members.rs b/crates/assists/src/handlers/add_missing_impl_members.rs index 8df1d786b..1ac5fefd6 100644 --- a/crates/assists/src/handlers/add_missing_impl_members.rs +++ b/crates/assists/src/handlers/add_missing_impl_members.rs @@ -414,6 +414,41 @@ impl foo::Foo for S { ); } + #[test] + fn test_qualify_path_2() { + check_assist( + add_missing_impl_members, + r#" +mod foo { + pub mod bar { + pub struct Bar; + pub trait Foo { fn foo(&self, bar: Bar); } + } +} + +use foo::bar; + +struct S; +impl bar::Foo for S { <|> }"#, + r#" +mod foo { + pub mod bar { + pub struct Bar; + pub trait Foo { fn foo(&self, bar: Bar); } + } +} + +use foo::bar; + +struct S; +impl bar::Foo for S { + fn foo(&self, bar: bar::Bar) { + ${0:todo!()} + } +}"#, + ); + } + #[test] fn test_qualify_path_generic() { check_assist( diff --git a/crates/assists/src/handlers/auto_import.rs b/crates/assists/src/handlers/auto_import.rs index b5eb2c722..ee7277c04 100644 --- a/crates/assists/src/handlers/auto_import.rs +++ b/crates/assists/src/handlers/auto_import.rs @@ -196,10 +196,10 @@ impl AutoImportAssets { }) .filter_map(|candidate| match candidate { Either::Left(module_def) => { - self.module_with_name_to_import.find_use_path(db, module_def) + self.module_with_name_to_import.find_use_path_prefixed(db, module_def) } Either::Right(macro_def) => { - self.module_with_name_to_import.find_use_path(db, macro_def) + self.module_with_name_to_import.find_use_path_prefixed(db, macro_def) } }) .filter(|use_path| !use_path.segments.is_empty()) @@ -290,6 +290,35 @@ mod tests { use super::*; use crate::tests::{check_assist, check_assist_not_applicable, check_assist_target}; + #[test] + fn applicable_when_found_an_import_partial() { + check_assist( + auto_import, + r" + mod std { + pub mod fmt { + pub struct Formatter; + } + } + + use std::fmt; + + <|>Formatter + ", + r" + mod std { + pub mod fmt { + pub struct Formatter; + } + } + + use std::fmt::{self, Formatter}; + + Formatter + ", + ); + } + #[test] fn applicable_when_found_an_import() { check_assist( diff --git a/crates/hir/src/code_model.rs b/crates/hir/src/code_model.rs index a2a166e0a..567fd91af 100644 --- a/crates/hir/src/code_model.rs +++ b/crates/hir/src/code_model.rs @@ -383,6 +383,16 @@ impl Module { pub fn find_use_path(self, db: &dyn DefDatabase, item: impl Into) -> Option { hir_def::find_path::find_path(db, item.into(), self.into()) } + + /// Finds a path that can be used to refer to the given item from within + /// this module, if possible. This is used for returning import paths for use-statements. + pub fn find_use_path_prefixed( + self, + db: &dyn DefDatabase, + item: impl Into, + ) -> Option { + hir_def::find_path::find_path_prefixed(db, item.into(), self.into()) + } } #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] diff --git a/crates/hir_def/src/find_path.rs b/crates/hir_def/src/find_path.rs index ac2c54ac5..baf374144 100644 --- a/crates/hir_def/src/find_path.rs +++ b/crates/hir_def/src/find_path.rs @@ -4,6 +4,7 @@ use hir_expand::name::{known, AsName, Name}; use rustc_hash::FxHashSet; use test_utils::mark; +use crate::nameres::CrateDefMap; use crate::{ db::DefDatabase, item_scope::ItemInNs, @@ -18,7 +19,12 @@ use crate::{ /// *from where* you're referring to the item, hence the `from` parameter. pub fn find_path(db: &dyn DefDatabase, item: ItemInNs, from: ModuleId) -> Option { let _p = profile::span("find_path"); - find_path_inner(db, item, from, MAX_PATH_LEN) + find_path_inner(db, item, from, MAX_PATH_LEN, Prefixed::Not) +} + +pub fn find_path_prefixed(db: &dyn DefDatabase, item: ItemInNs, from: ModuleId) -> Option { + let _p = profile::span("find_path_absolute"); + find_path_inner(db, item, from, MAX_PATH_LEN, Prefixed::Plain) } const MAX_PATH_LEN: usize = 15; @@ -36,11 +42,67 @@ impl ModPath { } } +fn check_crate_self_super( + def_map: &CrateDefMap, + item: ItemInNs, + from: ModuleId, +) -> Option { + // - if the item is the crate root, return `crate` + if item + == ItemInNs::Types(ModuleDefId::ModuleId(ModuleId { + krate: from.krate, + local_id: def_map.root, + })) + { + Some(ModPath::from_segments(PathKind::Crate, Vec::new())) + } else if item == ItemInNs::Types(from.into()) { + // - if the item is the module we're in, use `self` + Some(ModPath::from_segments(PathKind::Super(0), Vec::new())) + } else { + if let Some(parent_id) = def_map.modules[from.local_id].parent { + // - if the item is the parent module, use `super` (this is not used recursively, since `super::super` is ugly) + if item + == ItemInNs::Types(ModuleDefId::ModuleId(ModuleId { + krate: from.krate, + local_id: parent_id, + })) + { + return Some(ModPath::from_segments(PathKind::Super(1), Vec::new())); + } + } + None + } +} + +#[derive(Copy, Clone, PartialEq, Eq)] +pub enum Prefixed { + Not, + BySelf, + Plain, +} + +impl Prefixed { + #[inline] + fn prefix(self) -> Option { + match self { + Prefixed::Not => None, + Prefixed::BySelf => Some(PathKind::Super(0)), + Prefixed::Plain => Some(PathKind::Plain), + } + } + + #[inline] + fn prefixed(self) -> bool { + self != Prefixed::Not + } +} + fn find_path_inner( db: &dyn DefDatabase, item: ItemInNs, from: ModuleId, max_len: usize, + prefixed: Prefixed, ) -> Option { if max_len == 0 { return None; @@ -51,41 +113,22 @@ fn find_path_inner( // - if the item is already in scope, return the name under which it is let def_map = db.crate_def_map(from.krate); let from_scope: &crate::item_scope::ItemScope = &def_map.modules[from.local_id].scope; - if let Some((name, _)) = from_scope.name_of(item) { - return Some(ModPath::from_segments(PathKind::Plain, vec![name.clone()])); + let scope_name = + if let Some((name, _)) = from_scope.name_of(item) { Some(name.clone()) } else { None }; + if !prefixed.prefixed() && scope_name.is_some() { + return scope_name + .map(|scope_name| ModPath::from_segments(PathKind::Plain, vec![scope_name])); } - // - if the item is the crate root, return `crate` - if item - == ItemInNs::Types(ModuleDefId::ModuleId(ModuleId { - krate: from.krate, - local_id: def_map.root, - })) - { - return Some(ModPath::from_segments(PathKind::Crate, Vec::new())); - } - - // - if the item is the module we're in, use `self` - if item == ItemInNs::Types(from.into()) { - return Some(ModPath::from_segments(PathKind::Super(0), Vec::new())); - } - - // - if the item is the parent module, use `super` (this is not used recursively, since `super::super` is ugly) - if let Some(parent_id) = def_map.modules[from.local_id].parent { - if item - == ItemInNs::Types(ModuleDefId::ModuleId(ModuleId { - krate: from.krate, - local_id: parent_id, - })) - { - return Some(ModPath::from_segments(PathKind::Super(1), Vec::new())); - } + if let modpath @ Some(_) = check_crate_self_super(&def_map, item, from) { + return modpath; } // - if the item is the crate root of a dependency crate, return the name from the extern prelude for (name, def_id) in &def_map.extern_prelude { if item == ItemInNs::Types(*def_id) { - return Some(ModPath::from_segments(PathKind::Plain, vec![name.clone()])); + let name = scope_name.unwrap_or_else(|| name.clone()); + return Some(ModPath::from_segments(PathKind::Plain, vec![name])); } } @@ -138,6 +181,7 @@ fn find_path_inner( ItemInNs::Types(ModuleDefId::ModuleId(module_id)), from, best_path_len - 1, + prefixed, ) { path.segments.push(name); @@ -165,6 +209,7 @@ fn find_path_inner( ItemInNs::Types(ModuleDefId::ModuleId(info.container)), from, best_path_len - 1, + prefixed, )?; path.segments.push(info.path.segments.last().unwrap().clone()); Some(path) @@ -181,7 +226,13 @@ fn find_path_inner( } } - best_path + if let Some(prefix) = prefixed.prefix() { + best_path.or_else(|| { + scope_name.map(|scope_name| ModPath::from_segments(prefix, vec![scope_name])) + }) + } else { + best_path + } } fn select_best_path(old_path: ModPath, new_path: ModPath, prefer_no_std: bool) -> ModPath { @@ -304,7 +355,7 @@ 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(ra_fixture: &str, path: &str) { + fn check_found_path_(ra_fixture: &str, path: &str, absolute: bool) { let (db, pos) = TestDB::with_position(ra_fixture); let module = db.module_for_file(pos.file_id); let parsed_path_file = syntax::SourceFile::parse(&format!("use {};", path)); @@ -324,9 +375,20 @@ mod tests { .take_types() .unwrap(); - let found_path = find_path(&db, ItemInNs::Types(resolved), module); + let found_path = if absolute { find_path_prefixed } else { find_path }( + &db, + ItemInNs::Types(resolved), + module, + ); + assert_eq!(found_path, Some(mod_path), "absolute {}", absolute); + } + + fn check_found_path(ra_fixture: &str, path: &str) { + check_found_path_(ra_fixture, path, false); + } - assert_eq!(found_path, Some(mod_path)); + fn check_found_path_abs(ra_fixture: &str, path: &str) { + check_found_path_(ra_fixture, path, true); } #[test] @@ -337,6 +399,7 @@ mod tests { <|> "#; check_found_path(code, "S"); + check_found_path_abs(code, "S"); } #[test] @@ -347,6 +410,7 @@ mod tests { <|> "#; check_found_path(code, "E::A"); + check_found_path_abs(code, "E::A"); } #[test] @@ -359,6 +423,7 @@ mod tests { <|> "#; check_found_path(code, "foo::S"); + check_found_path_abs(code, "foo::S"); } #[test] @@ -373,6 +438,7 @@ mod tests { <|> "#; check_found_path(code, "super::S"); + check_found_path_abs(code, "super::S"); } #[test] @@ -384,6 +450,7 @@ mod tests { <|> "#; check_found_path(code, "self"); + check_found_path_abs(code, "self"); } #[test] @@ -395,6 +462,7 @@ mod tests { <|> "#; check_found_path(code, "crate"); + check_found_path_abs(code, "crate"); } #[test] @@ -407,6 +475,7 @@ mod tests { <|> "#; check_found_path(code, "crate::S"); + check_found_path_abs(code, "crate::S"); } #[test] @@ -418,6 +487,7 @@ mod tests { pub struct S; "#; check_found_path(code, "std::S"); + check_found_path_abs(code, "std::S"); } #[test] @@ -430,14 +500,14 @@ mod tests { pub struct S; "#; check_found_path(code, "std_renamed::S"); + check_found_path_abs(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#" + let code = r#" //- /main.rs crate:main deps:syntax use syntax::ast; @@ -449,12 +519,11 @@ mod tests { A, B, C, } } - "#, - "ast::ModuleItem", - ); + "#; + check_found_path(code, "ast::ModuleItem"); + check_found_path_abs(code, "syntax::ast::ModuleItem"); - check_found_path( - r#" + let code = r#" //- /main.rs crate:main deps:syntax <|> @@ -465,9 +534,9 @@ mod tests { A, B, C, } } - "#, - "syntax::ast::ModuleItem", - ); + "#; + check_found_path(code, "syntax::ast::ModuleItem"); + check_found_path_abs(code, "syntax::ast::ModuleItem"); } #[test] @@ -481,6 +550,7 @@ mod tests { <|> "#; check_found_path(code, "bar::S"); + check_found_path_abs(code, "bar::S"); } #[test] @@ -494,6 +564,7 @@ mod tests { <|> "#; check_found_path(code, "bar::U"); + check_found_path_abs(code, "bar::U"); } #[test] @@ -507,6 +578,7 @@ mod tests { pub struct S; "#; check_found_path(code, "std::S"); + check_found_path_abs(code, "std::S"); } #[test] @@ -520,6 +592,7 @@ mod tests { pub use prelude::*; "#; check_found_path(code, "S"); + check_found_path_abs(code, "S"); } #[test] @@ -537,6 +610,8 @@ mod tests { "#; check_found_path(code, "None"); check_found_path(code, "Some"); + check_found_path_abs(code, "None"); + check_found_path_abs(code, "Some"); } #[test] @@ -553,6 +628,7 @@ mod tests { pub use crate::foo::bar::S; "#; check_found_path(code, "baz::S"); + check_found_path_abs(code, "baz::S"); } #[test] @@ -567,6 +643,7 @@ mod tests { "#; // crate::S would be shorter, but using private imports seems wrong check_found_path(code, "crate::bar::S"); + check_found_path_abs(code, "crate::bar::S"); } #[test] @@ -585,6 +662,7 @@ mod tests { pub use super::foo; "#; check_found_path(code, "crate::foo::S"); + check_found_path_abs(code, "crate::foo::S"); } #[test] @@ -605,6 +683,7 @@ mod tests { } "#; check_found_path(code, "std::sync::Arc"); + check_found_path_abs(code, "std::sync::Arc"); } #[test] @@ -629,6 +708,7 @@ mod tests { } "#; check_found_path(code, "core::fmt::Error"); + check_found_path_abs(code, "core::fmt::Error"); } #[test] @@ -652,6 +732,7 @@ mod tests { } "#; check_found_path(code, "alloc::sync::Arc"); + check_found_path_abs(code, "alloc::sync::Arc"); } #[test] @@ -669,6 +750,7 @@ mod tests { pub struct Arc; "#; check_found_path(code, "megaalloc::Arc"); + check_found_path_abs(code, "megaalloc::Arc"); } #[test] @@ -683,5 +765,7 @@ mod tests { "#; check_found_path(code, "u8"); check_found_path(code, "u16"); + check_found_path_abs(code, "u8"); + check_found_path_abs(code, "u16"); } } -- cgit v1.2.3 From 747f6f64d7f8fae3a40be6ffacc9640bca068bd0 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 25 Sep 2020 15:21:50 +0200 Subject: Remove partial import test in insert_use --- crates/assists/src/utils/insert_use.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 09f4a2224..5719b06af 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -809,16 +809,6 @@ use std::io;", // FIXME: have it emit `use {self, *}`? } - #[test] - #[ignore] // FIXME: Support this - fn merge_partial_path() { - check_full( - "ast::Foo", - r"use syntax::{ast, algo};", - r"use syntax::{ast::{self, Foo}, algo};", - ) - } - #[test] fn merge_glob_nested() { check_full( -- cgit v1.2.3