From 316795e074dff8f627f8c70c85d236420ecfb3a6 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 24 Dec 2019 02:19:09 +0200 Subject: Initial auto import action implementation --- crates/ra_assists/src/assist_ctx.rs | 2 - crates/ra_assists/src/assists/auto_import.rs | 181 +++++++++++++++++++++++++++ crates/ra_assists/src/doc_tests.rs | 4 + crates/ra_assists/src/doc_tests/generated.rs | 19 +++ crates/ra_assists/src/lib.rs | 126 +++++++++++++++++-- crates/ra_hir/src/lib.rs | 2 +- crates/ra_ide/src/assists.rs | 7 +- crates/ra_ide/src/imports_locator.rs | 97 ++++++++++++++ crates/ra_ide/src/lib.rs | 1 + 9 files changed, 420 insertions(+), 19 deletions(-) create mode 100644 crates/ra_assists/src/assists/auto_import.rs create mode 100644 crates/ra_ide/src/imports_locator.rs (limited to 'crates') diff --git a/crates/ra_assists/src/assist_ctx.rs b/crates/ra_assists/src/assist_ctx.rs index 43f0d664b..2ab65ab99 100644 --- a/crates/ra_assists/src/assist_ctx.rs +++ b/crates/ra_assists/src/assist_ctx.rs @@ -101,7 +101,6 @@ impl<'a, DB: HirDatabase> AssistCtx<'a, DB> { Some(assist) } - #[allow(dead_code)] // will be used for auto import assist with multiple actions pub(crate) fn add_assist_group( self, id: AssistId, @@ -168,7 +167,6 @@ pub(crate) struct ActionBuilder { } impl ActionBuilder { - #[allow(dead_code)] /// Adds a custom label to the action, if it needs to be different from the assist label pub(crate) fn label(&mut self, label: impl Into) { self.label = Some(label.into()) diff --git a/crates/ra_assists/src/assists/auto_import.rs b/crates/ra_assists/src/assists/auto_import.rs new file mode 100644 index 000000000..fe226521e --- /dev/null +++ b/crates/ra_assists/src/assists/auto_import.rs @@ -0,0 +1,181 @@ +use hir::db::HirDatabase; +use ra_syntax::{ + ast::{self, AstNode}, + SmolStr, SyntaxElement, + SyntaxKind::{NAME_REF, USE_ITEM}, + SyntaxNode, +}; + +use crate::{ + assist_ctx::{ActionBuilder, Assist, AssistCtx}, + auto_import_text_edit, AssistId, ImportsLocator, +}; + +// Assist: auto_import +// +// If the name is unresolved, provides all possible imports for it. +// +// ``` +// fn main() { +// let map = HashMap<|>::new(); +// } +// ``` +// -> +// ``` +// use std::collections::HashMap; +// +// fn main() { +// let map = HashMap<|>::new(); +// } +// ``` +pub(crate) fn auto_import<'a, F: ImportsLocator<'a>>( + ctx: AssistCtx, + imports_locator: &mut F, +) -> Option { + let path: ast::Path = ctx.find_node_at_offset()?; + let module = path.syntax().ancestors().find_map(ast::Module::cast); + let position = match module.and_then(|it| it.item_list()) { + Some(item_list) => item_list.syntax().clone(), + None => { + let current_file = path.syntax().ancestors().find_map(ast::SourceFile::cast)?; + current_file.syntax().clone() + } + }; + + let module_with_name_to_import = ctx.source_analyzer(&position, None).module()?; + let name_to_import = hir::InFile { + file_id: ctx.frange.file_id.into(), + value: &find_applicable_name_ref(ctx.covering_element())?, + }; + + let proposed_imports = + imports_locator.find_imports(name_to_import, module_with_name_to_import)?; + if proposed_imports.is_empty() { + return None; + } + + ctx.add_assist_group(AssistId("auto_import"), "auto import", || { + proposed_imports + .into_iter() + .map(|import| import_to_action(import.to_string(), &position, &path)) + .collect() + }) +} + +fn find_applicable_name_ref(element: SyntaxElement) -> Option { + if element.ancestors().find(|ancestor| ancestor.kind() == USE_ITEM).is_some() { + None + } else if element.kind() == NAME_REF { + Some(element.as_node().cloned().and_then(ast::NameRef::cast)?) + } else { + let parent = element.parent()?; + if parent.kind() == NAME_REF { + Some(ast::NameRef::cast(parent)?) + } else { + None + } + } +} + +fn import_to_action(import: String, position: &SyntaxNode, path: &ast::Path) -> ActionBuilder { + let mut action_builder = ActionBuilder::default(); + action_builder.label(format!("Import `{}`", &import)); + auto_import_text_edit( + position, + &path.syntax().clone(), + &[SmolStr::new(import)], + action_builder.text_edit_builder(), + ); + action_builder +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::helpers::{ + check_assist_with_imports_locator, check_assist_with_imports_locator_not_applicable, + }; + use hir::Name; + + #[derive(Clone)] + struct TestImportsLocator<'a> { + import_path: &'a [Name], + } + + impl<'a> TestImportsLocator<'a> { + fn new(import_path: &'a [Name]) -> Self { + TestImportsLocator { import_path } + } + } + + impl<'a> ImportsLocator<'_> for TestImportsLocator<'_> { + fn find_imports( + &mut self, + _: hir::InFile<&ast::NameRef>, + _: hir::Module, + ) -> Option> { + if self.import_path.is_empty() { + None + } else { + Some(vec![hir::ModPath { + kind: hir::PathKind::Plain, + segments: self.import_path.to_owned(), + }]) + } + } + } + + #[test] + fn applicable_when_found_an_import() { + let import_path = &[hir::name::known::std, hir::name::known::ops, hir::name::known::Debug]; + let mut imports_locator = TestImportsLocator::new(import_path); + check_assist_with_imports_locator( + auto_import, + &mut imports_locator, + " + fn main() { + } + + Debug<|>", + &format!( + " + use {}; + + fn main() {{ + }} + + Debug<|>", + import_path + .into_iter() + .map(|name| name.to_string()) + .collect::>() + .join("::") + ), + ); + } + + #[test] + fn not_applicable_when_no_imports_found() { + let mut imports_locator = TestImportsLocator::new(&[]); + check_assist_with_imports_locator_not_applicable( + auto_import, + &mut imports_locator, + " + fn main() { + } + + Debug<|>", + ); + } + + #[test] + fn not_applicable_in_import_statements() { + let import_path = &[hir::name::known::std, hir::name::known::ops, hir::name::known::Debug]; + let mut imports_locator = TestImportsLocator::new(import_path); + check_assist_with_imports_locator_not_applicable( + auto_import, + &mut imports_locator, + "use Debug<|>;", + ); + } +} diff --git a/crates/ra_assists/src/doc_tests.rs b/crates/ra_assists/src/doc_tests.rs index 5dc1ee233..65d51428b 100644 --- a/crates/ra_assists/src/doc_tests.rs +++ b/crates/ra_assists/src/doc_tests.rs @@ -11,6 +11,10 @@ use test_utils::{assert_eq_text, extract_range_or_offset}; use crate::test_db::TestDB; fn check(assist_id: &str, before: &str, after: &str) { + // FIXME we cannot get the imports search functionality here yet, but still need to generate a test and a doc for an assist + if assist_id == "auto_import" { + return; + } let (selection, before) = extract_range_or_offset(before); let (db, file_id) = TestDB::with_single_file(&before); let frange = FileRange { file_id, range: selection.into() }; diff --git a/crates/ra_assists/src/doc_tests/generated.rs b/crates/ra_assists/src/doc_tests/generated.rs index 7d84dc8fb..ec4587ce7 100644 --- a/crates/ra_assists/src/doc_tests/generated.rs +++ b/crates/ra_assists/src/doc_tests/generated.rs @@ -214,6 +214,25 @@ fn main() { ) } +#[test] +fn doctest_auto_import() { + check( + "auto_import", + r#####" +fn main() { + let map = HashMap<|>::new(); +} +"#####, + r#####" +use std::collections::HashMap; + +fn main() { + let map = HashMap<|>::new(); +} +"#####, + ) +} + #[test] fn doctest_change_visibility() { check( diff --git a/crates/ra_assists/src/lib.rs b/crates/ra_assists/src/lib.rs index 3337805a5..4029962f7 100644 --- a/crates/ra_assists/src/lib.rs +++ b/crates/ra_assists/src/lib.rs @@ -14,9 +14,9 @@ mod test_db; pub mod ast_transform; use either::Either; -use hir::db::HirDatabase; +use hir::{db::HirDatabase, InFile, ModPath, Module}; use ra_db::FileRange; -use ra_syntax::{TextRange, TextUnit}; +use ra_syntax::{ast::NameRef, TextRange, TextUnit}; use ra_text_edit::TextEdit; pub(crate) use crate::assist_ctx::{Assist, AssistCtx}; @@ -77,6 +77,55 @@ where }) } +/// A functionality for locating imports for the given name. +/// +/// Currently has to be a trait with the real implementation provided by the ra_ide_api crate, +/// due to the search functionality located there. +/// Later, this trait should be removed completely and the search functionality moved to a separate crate, +/// accessible from the ra_assists crate. +pub trait ImportsLocator<'a> { + /// Finds all imports for the given name and the module that contains this name. + fn find_imports( + &mut self, + name_to_import: InFile<&NameRef>, + module_with_name_to_import: Module, + ) -> Option>; +} + +/// Return all the assists applicable at the given position +/// and additional assists that need the imports locator functionality to work. +/// +/// Assists are returned in the "resolved" state, that is with edit fully +/// computed. +pub fn assists_with_imports_locator<'a, H, F: 'a>( + db: &H, + range: FileRange, + mut imports_locator: F, +) -> Vec +where + H: HirDatabase + 'static, + F: ImportsLocator<'a>, +{ + AssistCtx::with_ctx(db, range, true, |ctx| { + let mut assists = assists::all() + .iter() + .map(|f| f(ctx.clone())) + .chain( + assists::all_with_imports_locator() + .iter() + .map(|f| f(ctx.clone(), &mut imports_locator)), + ) + .filter_map(std::convert::identity) + .map(|a| match a { + Assist::Resolved { assist } => assist, + Assist::Unresolved { .. } => unreachable!(), + }) + .collect(); + sort_assists(&mut assists); + assists + }) +} + /// Return all the assists applicable at the given position. /// /// Assists are returned in the "resolved" state, that is with edit fully @@ -85,8 +134,6 @@ pub fn assists(db: &H, range: FileRange) -> Vec where H: HirDatabase + 'static, { - use std::cmp::Ordering; - AssistCtx::with_ctx(db, range, true, |ctx| { let mut a = assists::all() .iter() @@ -95,19 +142,24 @@ where Assist::Resolved { assist } => assist, Assist::Unresolved { .. } => unreachable!(), }) - .collect::>(); - a.sort_by(|a, b| match (a.get_first_action().target, b.get_first_action().target) { - (Some(a), Some(b)) => a.len().cmp(&b.len()), - (Some(_), None) => Ordering::Less, - (None, Some(_)) => Ordering::Greater, - (None, None) => Ordering::Equal, - }); + .collect(); + sort_assists(&mut a); a }) } +fn sort_assists(assists: &mut Vec) { + use std::cmp::Ordering; + assists.sort_by(|a, b| match (a.get_first_action().target, b.get_first_action().target) { + (Some(a), Some(b)) => a.len().cmp(&b.len()), + (Some(_), None) => Ordering::Less, + (None, Some(_)) => Ordering::Greater, + (None, None) => Ordering::Equal, + }); +} + mod assists { - use crate::{Assist, AssistCtx}; + use crate::{Assist, AssistCtx, ImportsLocator}; use hir::db::HirDatabase; mod add_derive; @@ -116,6 +168,7 @@ mod assists { mod add_custom_impl; mod add_new; mod apply_demorgan; + mod auto_import; mod invert_if; mod flip_comma; mod flip_binexpr; @@ -168,6 +221,11 @@ mod assists { early_return::convert_to_guarded_return, ] } + + pub(crate) fn all_with_imports_locator<'a, DB: HirDatabase, F: ImportsLocator<'a>>( + ) -> &'a [fn(AssistCtx, &mut F) -> Option] { + &[auto_import::auto_import] + } } #[cfg(test)] @@ -176,7 +234,7 @@ mod helpers { use ra_syntax::TextRange; use test_utils::{add_cursor, assert_eq_text, extract_offset, extract_range}; - use crate::{test_db::TestDB, Assist, AssistCtx}; + use crate::{test_db::TestDB, Assist, AssistCtx, ImportsLocator}; pub(crate) fn check_assist( assist: fn(AssistCtx) -> Option, @@ -206,6 +264,35 @@ mod helpers { assert_eq_text!(after, &actual); } + pub(crate) fn check_assist_with_imports_locator<'a, F: ImportsLocator<'a>>( + assist: fn(AssistCtx, &mut F) -> Option, + imports_locator: &mut F, + before: &str, + after: &str, + ) { + let (before_cursor_pos, before) = extract_offset(before); + let (db, file_id) = TestDB::with_single_file(&before); + let frange = + FileRange { file_id, range: TextRange::offset_len(before_cursor_pos, 0.into()) }; + let assist = AssistCtx::with_ctx(&db, frange, true, |ctx| assist(ctx, imports_locator)) + .expect("code action is not applicable"); + let action = match assist { + Assist::Unresolved { .. } => unreachable!(), + Assist::Resolved { assist } => assist.get_first_action(), + }; + + let actual = action.edit.apply(&before); + let actual_cursor_pos = match action.cursor_position { + None => action + .edit + .apply_to_offset(before_cursor_pos) + .expect("cursor position is affected by the edit"), + Some(off) => off, + }; + let actual = add_cursor(&actual, actual_cursor_pos); + assert_eq_text!(after, &actual); + } + pub(crate) fn check_assist_range( assist: fn(AssistCtx) -> Option, before: &str, @@ -279,6 +366,19 @@ mod helpers { assert!(assist.is_none()); } + pub(crate) fn check_assist_with_imports_locator_not_applicable<'a, F: ImportsLocator<'a>>( + assist: fn(AssistCtx, &mut F) -> Option, + imports_locator: &mut F, + before: &str, + ) { + let (before_cursor_pos, before) = extract_offset(before); + let (db, file_id) = TestDB::with_single_file(&before); + let frange = + FileRange { file_id, range: TextRange::offset_len(before_cursor_pos, 0.into()) }; + let assist = AssistCtx::with_ctx(&db, frange, true, |ctx| assist(ctx, imports_locator)); + assert!(assist.is_none()); + } + pub(crate) fn check_assist_range_not_applicable( assist: fn(AssistCtx) -> Option, before: &str, diff --git a/crates/ra_hir/src/lib.rs b/crates/ra_hir/src/lib.rs index e1c7b7a20..be6dced53 100644 --- a/crates/ra_hir/src/lib.rs +++ b/crates/ra_hir/src/lib.rs @@ -58,6 +58,6 @@ pub use hir_def::{ type_ref::Mutability, }; pub use hir_expand::{ - name::Name, HirFileId, InFile, MacroCallId, MacroCallLoc, MacroDefId, MacroFile, Origin, + name, name::Name, HirFileId, InFile, MacroCallId, MacroCallLoc, MacroDefId, MacroFile, Origin, }; pub use hir_ty::{display::HirDisplay, CallableDef}; diff --git a/crates/ra_ide/src/assists.rs b/crates/ra_ide/src/assists.rs index a936900da..c43c45c65 100644 --- a/crates/ra_ide/src/assists.rs +++ b/crates/ra_ide/src/assists.rs @@ -2,8 +2,9 @@ use ra_db::{FilePosition, FileRange}; -use crate::{db::RootDatabase, FileId, SourceChange, SourceFileEdit}; - +use crate::{ + db::RootDatabase, imports_locator::ImportsLocatorIde, FileId, SourceChange, SourceFileEdit, +}; use either::Either; pub use ra_assists::AssistId; use ra_assists::{AssistAction, AssistLabel}; @@ -16,7 +17,7 @@ pub struct Assist { } pub(crate) fn assists(db: &RootDatabase, frange: FileRange) -> Vec { - ra_assists::assists(db, frange) + ra_assists::assists_with_imports_locator(db, frange, ImportsLocatorIde::new(db)) .into_iter() .map(|assist| { let file_id = frange.file_id; diff --git a/crates/ra_ide/src/imports_locator.rs b/crates/ra_ide/src/imports_locator.rs new file mode 100644 index 000000000..23391ac3b --- /dev/null +++ b/crates/ra_ide/src/imports_locator.rs @@ -0,0 +1,97 @@ +//! This module contains an import search funcionality that is provided to the ra_assists module. +//! Later, this should be moved away to a separate crate that is accessible from the ra_assists module. + +use crate::{ + db::RootDatabase, + references::{classify_name, classify_name_ref, NameDefinition, NameKind}, + symbol_index::{self, FileSymbol}, + Query, +}; +use ast::NameRef; +use hir::{db::HirDatabase, InFile, ModPath, Module, SourceBinder}; +use itertools::Itertools; +use ra_assists::ImportsLocator; +use ra_prof::profile; +use ra_syntax::{ast, AstNode, SyntaxKind::NAME}; + +pub(crate) struct ImportsLocatorIde<'a> { + source_binder: SourceBinder<'a, RootDatabase>, +} + +impl<'a> ImportsLocatorIde<'a> { + pub(crate) fn new(db: &'a RootDatabase) -> Self { + Self { source_binder: SourceBinder::new(db) } + } + + fn search_for_imports( + &mut self, + name_to_import: &ast::NameRef, + module_with_name_to_import: Module, + ) -> Vec { + let _p = profile("search_for_imports"); + let db = self.source_binder.db; + let name_to_import = name_to_import.text(); + + let project_results = { + let mut query = Query::new(name_to_import.to_string()); + query.exact(); + query.limit(10); + symbol_index::world_symbols(db, query) + }; + let lib_results = { + let mut query = Query::new(name_to_import.to_string()); + query.libs(); + query.exact(); + query.limit(10); + symbol_index::world_symbols(db, query) + }; + + project_results + .into_iter() + .chain(lib_results.into_iter()) + .filter_map(|import_candidate| self.get_name_definition(db, &import_candidate)) + .filter_map(|name_definition_to_import| { + if let NameKind::Def(module_def) = name_definition_to_import.kind { + module_with_name_to_import.find_use_path(db, module_def) + } else { + None + } + }) + .filter(|use_path| !use_path.segments.is_empty()) + .unique() + .collect() + } + + fn get_name_definition( + &mut self, + db: &impl HirDatabase, + import_candidate: &FileSymbol, + ) -> Option { + let _p = profile("get_name_definition"); + let file_id = import_candidate.file_id.into(); + let candidate_node = import_candidate.ptr.to_node(&db.parse_or_expand(file_id)?); + let candidate_name_node = if candidate_node.kind() != NAME { + candidate_node.children().find(|it| it.kind() == NAME)? + } else { + candidate_node + }; + classify_name( + &mut self.source_binder, + hir::InFile { file_id, value: &ast::Name::cast(candidate_name_node)? }, + ) + } +} + +impl<'a> ImportsLocator<'a> for ImportsLocatorIde<'a> { + fn find_imports( + &mut self, + name_to_import: InFile<&NameRef>, + module_with_name_to_import: Module, + ) -> Option> { + if classify_name_ref(&mut self.source_binder, name_to_import).is_none() { + Some(self.search_for_imports(name_to_import.value, module_with_name_to_import)) + } else { + None + } + } +} diff --git a/crates/ra_ide/src/lib.rs b/crates/ra_ide/src/lib.rs index 62fe6d2a9..03ad6b2c1 100644 --- a/crates/ra_ide/src/lib.rs +++ b/crates/ra_ide/src/lib.rs @@ -30,6 +30,7 @@ mod syntax_highlighting; mod parent_module; mod references; mod impls; +mod imports_locator; mod assists; mod diagnostics; mod syntax_tree; -- cgit v1.2.3 From f57239729cb9d6f60eb09d8cfd8244066b5e182c Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 23 Jan 2020 21:15:16 +0200 Subject: Remove unnecessary lifetime parameter --- crates/ra_assists/src/assists/auto_import.rs | 4 ++-- crates/ra_assists/src/lib.rs | 12 ++++++------ crates/ra_ide/src/imports_locator.rs | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) (limited to 'crates') diff --git a/crates/ra_assists/src/assists/auto_import.rs b/crates/ra_assists/src/assists/auto_import.rs index fe226521e..ae216944b 100644 --- a/crates/ra_assists/src/assists/auto_import.rs +++ b/crates/ra_assists/src/assists/auto_import.rs @@ -28,7 +28,7 @@ use crate::{ // let map = HashMap<|>::new(); // } // ``` -pub(crate) fn auto_import<'a, F: ImportsLocator<'a>>( +pub(crate) fn auto_import( ctx: AssistCtx, imports_locator: &mut F, ) -> Option { @@ -108,7 +108,7 @@ mod tests { } } - impl<'a> ImportsLocator<'_> for TestImportsLocator<'_> { + impl<'a> ImportsLocator for TestImportsLocator<'a> { fn find_imports( &mut self, _: hir::InFile<&ast::NameRef>, diff --git a/crates/ra_assists/src/lib.rs b/crates/ra_assists/src/lib.rs index 4029962f7..381a51df6 100644 --- a/crates/ra_assists/src/lib.rs +++ b/crates/ra_assists/src/lib.rs @@ -83,7 +83,7 @@ where /// due to the search functionality located there. /// Later, this trait should be removed completely and the search functionality moved to a separate crate, /// accessible from the ra_assists crate. -pub trait ImportsLocator<'a> { +pub trait ImportsLocator { /// Finds all imports for the given name and the module that contains this name. fn find_imports( &mut self, @@ -97,14 +97,14 @@ pub trait ImportsLocator<'a> { /// /// Assists are returned in the "resolved" state, that is with edit fully /// computed. -pub fn assists_with_imports_locator<'a, H, F: 'a>( +pub fn assists_with_imports_locator( db: &H, range: FileRange, mut imports_locator: F, ) -> Vec where H: HirDatabase + 'static, - F: ImportsLocator<'a>, + F: ImportsLocator, { AssistCtx::with_ctx(db, range, true, |ctx| { let mut assists = assists::all() @@ -222,7 +222,7 @@ mod assists { ] } - pub(crate) fn all_with_imports_locator<'a, DB: HirDatabase, F: ImportsLocator<'a>>( + pub(crate) fn all_with_imports_locator<'a, DB: HirDatabase, F: ImportsLocator>( ) -> &'a [fn(AssistCtx, &mut F) -> Option] { &[auto_import::auto_import] } @@ -264,7 +264,7 @@ mod helpers { assert_eq_text!(after, &actual); } - pub(crate) fn check_assist_with_imports_locator<'a, F: ImportsLocator<'a>>( + pub(crate) fn check_assist_with_imports_locator( assist: fn(AssistCtx, &mut F) -> Option, imports_locator: &mut F, before: &str, @@ -366,7 +366,7 @@ mod helpers { assert!(assist.is_none()); } - pub(crate) fn check_assist_with_imports_locator_not_applicable<'a, F: ImportsLocator<'a>>( + pub(crate) fn check_assist_with_imports_locator_not_applicable( assist: fn(AssistCtx, &mut F) -> Option, imports_locator: &mut F, before: &str, diff --git a/crates/ra_ide/src/imports_locator.rs b/crates/ra_ide/src/imports_locator.rs index 23391ac3b..ab9cd7990 100644 --- a/crates/ra_ide/src/imports_locator.rs +++ b/crates/ra_ide/src/imports_locator.rs @@ -82,7 +82,7 @@ impl<'a> ImportsLocatorIde<'a> { } } -impl<'a> ImportsLocator<'a> for ImportsLocatorIde<'a> { +impl<'a> ImportsLocator for ImportsLocatorIde<'a> { fn find_imports( &mut self, name_to_import: InFile<&NameRef>, -- cgit v1.2.3 From bef5cf0b9929539e8d9fece006bfd3db1b68bec4 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 23 Jan 2020 21:30:59 +0200 Subject: Raise the import search query cap --- crates/ra_ide/src/imports_locator.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'crates') diff --git a/crates/ra_ide/src/imports_locator.rs b/crates/ra_ide/src/imports_locator.rs index ab9cd7990..e69fb4070 100644 --- a/crates/ra_ide/src/imports_locator.rs +++ b/crates/ra_ide/src/imports_locator.rs @@ -35,14 +35,14 @@ impl<'a> ImportsLocatorIde<'a> { let project_results = { let mut query = Query::new(name_to_import.to_string()); query.exact(); - query.limit(10); + query.limit(40); symbol_index::world_symbols(db, query) }; let lib_results = { let mut query = Query::new(name_to_import.to_string()); query.libs(); query.exact(); - query.limit(10); + query.limit(40); symbol_index::world_symbols(db, query) }; @@ -59,6 +59,7 @@ impl<'a> ImportsLocatorIde<'a> { }) .filter(|use_path| !use_path.segments.is_empty()) .unique() + .take(20) .collect() } -- cgit v1.2.3 From d0a782ef1c53ef5c5d3b49b02c498f7a688c3a4d Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Fri, 24 Jan 2020 09:33:18 +0200 Subject: Have a better trait interface --- crates/ra_assists/src/assists/auto_import.rs | 26 +++++++---- crates/ra_assists/src/lib.rs | 10 ++-- crates/ra_hir/src/lib.rs | 3 +- crates/ra_ide/src/imports_locator.rs | 70 ++++++++++------------------ 4 files changed, 45 insertions(+), 64 deletions(-) (limited to 'crates') diff --git a/crates/ra_assists/src/assists/auto_import.rs b/crates/ra_assists/src/assists/auto_import.rs index ae216944b..d196f6c5c 100644 --- a/crates/ra_assists/src/assists/auto_import.rs +++ b/crates/ra_assists/src/assists/auto_import.rs @@ -1,4 +1,4 @@ -use hir::db::HirDatabase; +use hir::{db::HirDatabase, AsName}; use ra_syntax::{ ast::{self, AstNode}, SmolStr, SyntaxElement, @@ -41,15 +41,21 @@ pub(crate) fn auto_import( current_file.syntax().clone() } }; + let source_analyzer = ctx.source_analyzer(&position, None); + let module_with_name_to_import = source_analyzer.module()?; + let path_to_import = ctx.covering_element().ancestors().find_map(ast::Path::cast)?; + if source_analyzer.resolve_path(ctx.db, &path_to_import).is_some() { + return None; + } - let module_with_name_to_import = ctx.source_analyzer(&position, None).module()?; - let name_to_import = hir::InFile { - file_id: ctx.frange.file_id.into(), - value: &find_applicable_name_ref(ctx.covering_element())?, - }; - - let proposed_imports = - imports_locator.find_imports(name_to_import, module_with_name_to_import)?; + let name_to_import = &find_applicable_name_ref(ctx.covering_element())?.as_name(); + let proposed_imports = imports_locator + .find_imports(&name_to_import.to_string()) + .into_iter() + .filter_map(|module_def| module_with_name_to_import.find_use_path(ctx.db, module_def)) + .filter(|use_path| !use_path.segments.is_empty()) + .take(20) + .collect::>(); if proposed_imports.is_empty() { return None; } @@ -57,7 +63,7 @@ pub(crate) fn auto_import( ctx.add_assist_group(AssistId("auto_import"), "auto import", || { proposed_imports .into_iter() - .map(|import| import_to_action(import.to_string(), &position, &path)) + .map(|import| import_to_action(import.to_string(), &position, &path_to_import)) .collect() }) } diff --git a/crates/ra_assists/src/lib.rs b/crates/ra_assists/src/lib.rs index 381a51df6..9e4ebec47 100644 --- a/crates/ra_assists/src/lib.rs +++ b/crates/ra_assists/src/lib.rs @@ -14,9 +14,9 @@ mod test_db; pub mod ast_transform; use either::Either; -use hir::{db::HirDatabase, InFile, ModPath, Module}; +use hir::{db::HirDatabase, ModuleDef}; use ra_db::FileRange; -use ra_syntax::{ast::NameRef, TextRange, TextUnit}; +use ra_syntax::{TextRange, TextUnit}; use ra_text_edit::TextEdit; pub(crate) use crate::assist_ctx::{Assist, AssistCtx}; @@ -85,11 +85,7 @@ where /// accessible from the ra_assists crate. pub trait ImportsLocator { /// Finds all imports for the given name and the module that contains this name. - fn find_imports( - &mut self, - name_to_import: InFile<&NameRef>, - module_with_name_to_import: Module, - ) -> Option>; + fn find_imports(&mut self, name_to_import: &str) -> Vec; } /// Return all the assists applicable at the given position diff --git a/crates/ra_hir/src/lib.rs b/crates/ra_hir/src/lib.rs index be6dced53..21b0553be 100644 --- a/crates/ra_hir/src/lib.rs +++ b/crates/ra_hir/src/lib.rs @@ -58,6 +58,7 @@ pub use hir_def::{ type_ref::Mutability, }; pub use hir_expand::{ - name, name::Name, HirFileId, InFile, MacroCallId, MacroCallLoc, MacroDefId, MacroFile, Origin, + name::{AsName, Name}, + HirFileId, InFile, MacroCallId, MacroCallLoc, MacroDefId, MacroFile, Origin, }; pub use hir_ty::{display::HirDisplay, CallableDef}; diff --git a/crates/ra_ide/src/imports_locator.rs b/crates/ra_ide/src/imports_locator.rs index e69fb4070..b2fc48159 100644 --- a/crates/ra_ide/src/imports_locator.rs +++ b/crates/ra_ide/src/imports_locator.rs @@ -3,13 +3,11 @@ use crate::{ db::RootDatabase, - references::{classify_name, classify_name_ref, NameDefinition, NameKind}, + references::{classify_name, NameDefinition, NameKind}, symbol_index::{self, FileSymbol}, Query, }; -use ast::NameRef; -use hir::{db::HirDatabase, InFile, ModPath, Module, SourceBinder}; -use itertools::Itertools; +use hir::{db::HirDatabase, ModuleDef, SourceBinder}; use ra_assists::ImportsLocator; use ra_prof::profile; use ra_syntax::{ast, AstNode, SyntaxKind::NAME}; @@ -23,14 +21,30 @@ impl<'a> ImportsLocatorIde<'a> { Self { source_binder: SourceBinder::new(db) } } - fn search_for_imports( + fn get_name_definition( &mut self, - name_to_import: &ast::NameRef, - module_with_name_to_import: Module, - ) -> Vec { + db: &impl HirDatabase, + import_candidate: &FileSymbol, + ) -> Option { + let _p = profile("get_name_definition"); + let file_id = import_candidate.file_id.into(); + let candidate_node = import_candidate.ptr.to_node(&db.parse_or_expand(file_id)?); + let candidate_name_node = if candidate_node.kind() != NAME { + candidate_node.children().find(|it| it.kind() == NAME)? + } else { + candidate_node + }; + classify_name( + &mut self.source_binder, + hir::InFile { file_id, value: &ast::Name::cast(candidate_name_node)? }, + ) + } +} + +impl<'a> ImportsLocator for ImportsLocatorIde<'a> { + fn find_imports(&mut self, name_to_import: &str) -> Vec { let _p = profile("search_for_imports"); let db = self.source_binder.db; - let name_to_import = name_to_import.text(); let project_results = { let mut query = Query::new(name_to_import.to_string()); @@ -52,47 +66,11 @@ impl<'a> ImportsLocatorIde<'a> { .filter_map(|import_candidate| self.get_name_definition(db, &import_candidate)) .filter_map(|name_definition_to_import| { if let NameKind::Def(module_def) = name_definition_to_import.kind { - module_with_name_to_import.find_use_path(db, module_def) + Some(module_def) } else { None } }) - .filter(|use_path| !use_path.segments.is_empty()) - .unique() - .take(20) .collect() } - - fn get_name_definition( - &mut self, - db: &impl HirDatabase, - import_candidate: &FileSymbol, - ) -> Option { - let _p = profile("get_name_definition"); - let file_id = import_candidate.file_id.into(); - let candidate_node = import_candidate.ptr.to_node(&db.parse_or_expand(file_id)?); - let candidate_name_node = if candidate_node.kind() != NAME { - candidate_node.children().find(|it| it.kind() == NAME)? - } else { - candidate_node - }; - classify_name( - &mut self.source_binder, - hir::InFile { file_id, value: &ast::Name::cast(candidate_name_node)? }, - ) - } -} - -impl<'a> ImportsLocator for ImportsLocatorIde<'a> { - fn find_imports( - &mut self, - name_to_import: InFile<&NameRef>, - module_with_name_to_import: Module, - ) -> Option> { - if classify_name_ref(&mut self.source_binder, name_to_import).is_none() { - Some(self.search_for_imports(name_to_import.value, module_with_name_to_import)) - } else { - None - } - } } -- cgit v1.2.3 From 1a78991df69630b581b4210083c9e94157bab0e1 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 27 Jan 2020 00:16:18 +0200 Subject: Adjust the tests --- crates/ra_assists/src/assists/auto_import.rs | 144 +++++++++++++++++---------- crates/ra_assists/src/lib.rs | 66 ++++++++++-- crates/ra_hir/src/lib.rs | 1 + 3 files changed, 150 insertions(+), 61 deletions(-) (limited to 'crates') diff --git a/crates/ra_assists/src/assists/auto_import.rs b/crates/ra_assists/src/assists/auto_import.rs index d196f6c5c..295fdf2e2 100644 --- a/crates/ra_assists/src/assists/auto_import.rs +++ b/crates/ra_assists/src/assists/auto_import.rs @@ -100,88 +100,122 @@ mod tests { use super::*; use crate::helpers::{ check_assist_with_imports_locator, check_assist_with_imports_locator_not_applicable, + TestImportsLocator, }; - use hir::Name; - #[derive(Clone)] - struct TestImportsLocator<'a> { - import_path: &'a [Name], - } + #[test] + fn applicable_when_found_an_import() { + check_assist_with_imports_locator( + auto_import, + TestImportsLocator::new, + r" + PubStruct<|> - impl<'a> TestImportsLocator<'a> { - fn new(import_path: &'a [Name]) -> Self { - TestImportsLocator { import_path } - } - } + pub mod PubMod { + pub struct PubStruct; + } + ", + r" + use PubMod::PubStruct; - impl<'a> ImportsLocator for TestImportsLocator<'a> { - fn find_imports( - &mut self, - _: hir::InFile<&ast::NameRef>, - _: hir::Module, - ) -> Option> { - if self.import_path.is_empty() { - None - } else { - Some(vec![hir::ModPath { - kind: hir::PathKind::Plain, - segments: self.import_path.to_owned(), - }]) + PubStruct<|> + + pub mod PubMod { + pub struct PubStruct; } - } + ", + ); } #[test] - fn applicable_when_found_an_import() { - let import_path = &[hir::name::known::std, hir::name::known::ops, hir::name::known::Debug]; - let mut imports_locator = TestImportsLocator::new(import_path); + fn applicable_when_found_multiple_imports() { check_assist_with_imports_locator( auto_import, - &mut imports_locator, - " - fn main() { + TestImportsLocator::new, + r" + PubStruct<|> + + pub mod PubMod1 { + pub struct PubStruct; + } + pub mod PubMod2 { + pub struct PubStruct; + } + pub mod PubMod3 { + pub struct PubStruct; + } + ", + r" + use PubMod1::PubStruct; + + PubStruct<|> + + pub mod PubMod1 { + pub struct PubStruct; + } + pub mod PubMod2 { + pub struct PubStruct; } + pub mod PubMod3 { + pub struct PubStruct; + } + ", + ); + } + + #[test] + fn not_applicable_for_already_imported_types() { + check_assist_with_imports_locator_not_applicable( + auto_import, + TestImportsLocator::new, + r" + use PubMod::PubStruct; + + PubStruct<|> - Debug<|>", - &format!( - " - use {}; - - fn main() {{ - }} - - Debug<|>", - import_path - .into_iter() - .map(|name| name.to_string()) - .collect::>() - .join("::") - ), + pub mod PubMod { + pub struct PubStruct; + } + ", ); } #[test] - fn not_applicable_when_no_imports_found() { - let mut imports_locator = TestImportsLocator::new(&[]); + fn not_applicable_for_types_with_private_paths() { check_assist_with_imports_locator_not_applicable( auto_import, - &mut imports_locator, - " - fn main() { + TestImportsLocator::new, + r" + PrivateStruct<|> + + pub mod PubMod { + struct PrivateStruct; } + ", + ); + } - Debug<|>", + #[test] + fn not_applicable_when_no_imports_found() { + check_assist_with_imports_locator_not_applicable( + auto_import, + TestImportsLocator::new, + " + PubStruct<|>", ); } #[test] fn not_applicable_in_import_statements() { - let import_path = &[hir::name::known::std, hir::name::known::ops, hir::name::known::Debug]; - let mut imports_locator = TestImportsLocator::new(import_path); check_assist_with_imports_locator_not_applicable( auto_import, - &mut imports_locator, - "use Debug<|>;", + TestImportsLocator::new, + r" + use PubStruct<|>; + + pub mod PubMod { + pub struct PubStruct; + }", ); } } diff --git a/crates/ra_assists/src/lib.rs b/crates/ra_assists/src/lib.rs index 9e4ebec47..724bce191 100644 --- a/crates/ra_assists/src/lib.rs +++ b/crates/ra_assists/src/lib.rs @@ -226,11 +226,59 @@ mod assists { #[cfg(test)] mod helpers { - use ra_db::{fixture::WithFixture, FileRange}; + use hir::db::DefDatabase; + use ra_db::{fixture::WithFixture, FileId, FileRange}; use ra_syntax::TextRange; use test_utils::{add_cursor, assert_eq_text, extract_offset, extract_range}; use crate::{test_db::TestDB, Assist, AssistCtx, ImportsLocator}; + use std::sync::Arc; + + pub(crate) struct TestImportsLocator { + db: Arc, + test_file_id: FileId, + } + + impl TestImportsLocator { + pub(crate) fn new(db: Arc, test_file_id: FileId) -> Self { + TestImportsLocator { db, test_file_id } + } + } + + impl ImportsLocator for TestImportsLocator { + fn find_imports(&mut self, name_to_import: &str) -> Vec { + let crate_def_map = self.db.crate_def_map(self.db.test_crate()); + let mut findings = vec![]; + + let mut module_ids_to_process = + crate_def_map.modules_for_file(self.test_file_id).collect::>(); + + while !module_ids_to_process.is_empty() { + let mut more_ids_to_process = vec![]; + for local_module_id in module_ids_to_process.drain(..) { + for (name, namespace_data) in + crate_def_map[local_module_id].scope.entries_without_primitives() + { + let found_a_match = &name.to_string() == name_to_import; + vec![namespace_data.types, namespace_data.values] + .into_iter() + .filter_map(std::convert::identity) + .for_each(|(module_def_id, _)| { + if found_a_match { + findings.push(module_def_id.into()); + } + if let hir::ModuleDefId::ModuleId(module_id) = module_def_id { + more_ids_to_process.push(module_id.local_id); + } + }); + } + } + module_ids_to_process = more_ids_to_process; + } + + findings + } + } pub(crate) fn check_assist( assist: fn(AssistCtx) -> Option, @@ -262,16 +310,19 @@ mod helpers { pub(crate) fn check_assist_with_imports_locator( assist: fn(AssistCtx, &mut F) -> Option, - imports_locator: &mut F, + imports_locator_provider: fn(db: Arc, file_id: FileId) -> F, before: &str, after: &str, ) { let (before_cursor_pos, before) = extract_offset(before); let (db, file_id) = TestDB::with_single_file(&before); + let db = Arc::new(db); + let mut imports_locator = imports_locator_provider(Arc::clone(&db), file_id); let frange = FileRange { file_id, range: TextRange::offset_len(before_cursor_pos, 0.into()) }; - let assist = AssistCtx::with_ctx(&db, frange, true, |ctx| assist(ctx, imports_locator)) - .expect("code action is not applicable"); + let assist = + AssistCtx::with_ctx(db.as_ref(), frange, true, |ctx| assist(ctx, &mut imports_locator)) + .expect("code action is not applicable"); let action = match assist { Assist::Unresolved { .. } => unreachable!(), Assist::Resolved { assist } => assist.get_first_action(), @@ -364,14 +415,17 @@ mod helpers { pub(crate) fn check_assist_with_imports_locator_not_applicable( assist: fn(AssistCtx, &mut F) -> Option, - imports_locator: &mut F, + imports_locator_provider: fn(db: Arc, file_id: FileId) -> F, before: &str, ) { let (before_cursor_pos, before) = extract_offset(before); let (db, file_id) = TestDB::with_single_file(&before); + let db = Arc::new(db); + let mut imports_locator = imports_locator_provider(Arc::clone(&db), file_id); let frange = FileRange { file_id, range: TextRange::offset_len(before_cursor_pos, 0.into()) }; - let assist = AssistCtx::with_ctx(&db, frange, true, |ctx| assist(ctx, imports_locator)); + let assist = + AssistCtx::with_ctx(db.as_ref(), frange, true, |ctx| assist(ctx, &mut imports_locator)); assert!(assist.is_none()); } diff --git a/crates/ra_hir/src/lib.rs b/crates/ra_hir/src/lib.rs index 21b0553be..2cfeaba72 100644 --- a/crates/ra_hir/src/lib.rs +++ b/crates/ra_hir/src/lib.rs @@ -56,6 +56,7 @@ pub use hir_def::{ nameres::ModuleSource, path::{ModPath, Path, PathKind}, type_ref::Mutability, + ModuleDefId, }; pub use hir_expand::{ name::{AsName, Name}, -- cgit v1.2.3 From 9a6b5c6183b2d7aa3d577c3fb12d519721f4a4d0 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 27 Jan 2020 01:53:59 +0200 Subject: Enforce alphabetical import sorting --- crates/ra_assists/src/assists/auto_import.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'crates') diff --git a/crates/ra_assists/src/assists/auto_import.rs b/crates/ra_assists/src/assists/auto_import.rs index 295fdf2e2..4c3793ac7 100644 --- a/crates/ra_assists/src/assists/auto_import.rs +++ b/crates/ra_assists/src/assists/auto_import.rs @@ -55,7 +55,8 @@ pub(crate) fn auto_import( .filter_map(|module_def| module_with_name_to_import.find_use_path(ctx.db, module_def)) .filter(|use_path| !use_path.segments.is_empty()) .take(20) - .collect::>(); + .map(|import| import.to_string()) + .collect::>(); if proposed_imports.is_empty() { return None; } @@ -63,7 +64,7 @@ pub(crate) fn auto_import( ctx.add_assist_group(AssistId("auto_import"), "auto import", || { proposed_imports .into_iter() - .map(|import| import_to_action(import.to_string(), &position, &path_to_import)) + .map(|import| import_to_action(import, &position, &path_to_import)) .collect() }) } -- cgit v1.2.3 From 9be1ab7ff948d89334a8acbc309c8235d4ab374f Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 27 Jan 2020 14:42:45 +0200 Subject: Code review fixes --- crates/ra_assists/src/assists/auto_import.rs | 10 +++++----- crates/ra_assists/src/lib.rs | 5 +++-- crates/ra_hir/src/lib.rs | 5 ++--- crates/ra_ide/src/imports_locator.rs | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) (limited to 'crates') diff --git a/crates/ra_assists/src/assists/auto_import.rs b/crates/ra_assists/src/assists/auto_import.rs index 4c3793ac7..9163cc662 100644 --- a/crates/ra_assists/src/assists/auto_import.rs +++ b/crates/ra_assists/src/assists/auto_import.rs @@ -1,4 +1,4 @@ -use hir::{db::HirDatabase, AsName}; +use hir::db::HirDatabase; use ra_syntax::{ ast::{self, AstNode}, SmolStr, SyntaxElement, @@ -48,7 +48,7 @@ pub(crate) fn auto_import( return None; } - let name_to_import = &find_applicable_name_ref(ctx.covering_element())?.as_name(); + let name_to_import = &find_applicable_name_ref(ctx.covering_element())?.syntax().to_string(); let proposed_imports = imports_locator .find_imports(&name_to_import.to_string()) .into_iter() @@ -64,7 +64,7 @@ pub(crate) fn auto_import( ctx.add_assist_group(AssistId("auto_import"), "auto import", || { proposed_imports .into_iter() - .map(|import| import_to_action(import, &position, &path_to_import)) + .map(|import| import_to_action(import, &position, &path_to_import.syntax())) .collect() }) } @@ -84,12 +84,12 @@ fn find_applicable_name_ref(element: SyntaxElement) -> Option { } } -fn import_to_action(import: String, position: &SyntaxNode, path: &ast::Path) -> ActionBuilder { +fn import_to_action(import: String, position: &SyntaxNode, anchor: &SyntaxNode) -> ActionBuilder { let mut action_builder = ActionBuilder::default(); action_builder.label(format!("Import `{}`", &import)); auto_import_text_edit( position, - &path.syntax().clone(), + anchor, &[SmolStr::new(import)], action_builder.text_edit_builder(), ); diff --git a/crates/ra_assists/src/lib.rs b/crates/ra_assists/src/lib.rs index 724bce191..625ebc4a2 100644 --- a/crates/ra_assists/src/lib.rs +++ b/crates/ra_assists/src/lib.rs @@ -234,6 +234,7 @@ mod helpers { use crate::{test_db::TestDB, Assist, AssistCtx, ImportsLocator}; use std::sync::Arc; + // FIXME remove the `ModuleDefId` reexport from `ra_hir` when this gets removed. pub(crate) struct TestImportsLocator { db: Arc, test_file_id: FileId, @@ -248,13 +249,13 @@ mod helpers { impl ImportsLocator for TestImportsLocator { fn find_imports(&mut self, name_to_import: &str) -> Vec { let crate_def_map = self.db.crate_def_map(self.db.test_crate()); - let mut findings = vec![]; + let mut findings = Vec::new(); let mut module_ids_to_process = crate_def_map.modules_for_file(self.test_file_id).collect::>(); while !module_ids_to_process.is_empty() { - let mut more_ids_to_process = vec![]; + let mut more_ids_to_process = Vec::new(); for local_module_id in module_ids_to_process.drain(..) { for (name, namespace_data) in crate_def_map[local_module_id].scope.entries_without_primitives() diff --git a/crates/ra_hir/src/lib.rs b/crates/ra_hir/src/lib.rs index 2cfeaba72..9e2673d13 100644 --- a/crates/ra_hir/src/lib.rs +++ b/crates/ra_hir/src/lib.rs @@ -56,10 +56,9 @@ pub use hir_def::{ nameres::ModuleSource, path::{ModPath, Path, PathKind}, type_ref::Mutability, - ModuleDefId, + ModuleDefId, // FIXME this is exposed and should be used for implementing the `TestImportsLocator` in `ra_assists` only, should be removed later along with the trait and the implementation. }; pub use hir_expand::{ - name::{AsName, Name}, - HirFileId, InFile, MacroCallId, MacroCallLoc, MacroDefId, MacroFile, Origin, + name::Name, HirFileId, InFile, MacroCallId, MacroCallLoc, MacroDefId, MacroFile, Origin, }; pub use hir_ty::{display::HirDisplay, CallableDef}; diff --git a/crates/ra_ide/src/imports_locator.rs b/crates/ra_ide/src/imports_locator.rs index b2fc48159..48b014c7d 100644 --- a/crates/ra_ide/src/imports_locator.rs +++ b/crates/ra_ide/src/imports_locator.rs @@ -41,7 +41,7 @@ impl<'a> ImportsLocatorIde<'a> { } } -impl<'a> ImportsLocator for ImportsLocatorIde<'a> { +impl ImportsLocator for ImportsLocatorIde<'_> { fn find_imports(&mut self, name_to_import: &str) -> Vec { let _p = profile("search_for_imports"); let db = self.source_binder.db; -- cgit v1.2.3