Merge #2887

2887: Initial auto import action implementation r=matklad a=SomeoneToIgnore

Closes https://github.com/rust-analyzer/rust-analyzer/issues/2180

Adds an auto import action implementation.

This implementation is not ideal and has a few limitations:

* The import search functionality should be moved into a separate crate accessible from ra_assists.
This requires a lot of changes and a preliminary design. 
Currently the functionality is provided as a trait impl, more on that here: https://github.com/rust-analyzer/rust-analyzer/issues/2180#issuecomment-575690942

* Due to the design desicion from the previous item, no doctests are run for the new aciton (look for a new FIXME in the PR)

* For the same reason, I have to create the mock trait implementaion to test the assist

* Ideally, I think we should have this feature as a diagnostics (that detects an absense of an import) that has a corresponding quickfix action that gets evaluated on demand.
Curretly we perform the import search every time we resolve the import which looks suboptimal.
This requires `classify_name_ref` to be moved from ra_ide, so not done currently.

A few improvements to the imports mechanism to be considered later:

* Constants like `ra_syntax::SyntaxKind::NAME` are not imported, because they are not present in the database

* Method usages are not imported, they are found in the database, but `find_use_path` does not return any import paths for them

* Some import paths returned by the `find_use_path` method end up in `core::` or `alloc::` instead of `std:`, for example: `core::fmt::Debug` instead of `std::fmt::Debug`.
This is not an error techically, but still looks weird.

* No detection of cases where a trait should be imported in order to be able to call a method

* Improve `auto_import_text_edit` functionality: refactor it and move away from the place it is now, add better logic for merging the new import with already existing imports

Co-authored-by: Kirill Bulatov <mail4score@gmail.com>

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<String>) {
         self.label = Some(label.into())

crates/ra_assists/src/assists/auto_import.rs

@@ -0,0 +1,222 @@
+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<F: ImportsLocator>(
+    ctx: AssistCtx<impl HirDatabase>,
+    imports_locator: &mut F,
+) -> Option<Assist> {
+    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 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 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()
+        .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)
+        .map(|import| import.to_string())
+        .collect::<std::collections::BTreeSet<_>>();
+    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, &position, &path_to_import.syntax()))
+            .collect()
+    })
+}
+
+fn find_applicable_name_ref(element: SyntaxElement) -> Option<ast::NameRef> {
+    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, anchor: &SyntaxNode) -> ActionBuilder {
+    let mut action_builder = ActionBuilder::default();
+    action_builder.label(format!("Import `{}`", &import));
+    auto_import_text_edit(
+        position,
+        anchor,
+        &[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,
+        TestImportsLocator,
+    };
+
+    #[test]
+    fn applicable_when_found_an_import() {
+        check_assist_with_imports_locator(
+            auto_import,
+            TestImportsLocator::new,
+            r"
+            PubStruct<|>
+
+            pub mod PubMod {
+                pub struct PubStruct;
+            }
+            ",
+            r"
+            use PubMod::PubStruct;
+
+            PubStruct<|>
+
+            pub mod PubMod {
+                pub struct PubStruct;
+            }
+            ",
+        );
+    }
+
+    #[test]
+    fn applicable_when_found_multiple_imports() {
+        check_assist_with_imports_locator(
+            auto_import,
+            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<|>
+
+            pub mod PubMod {
+                pub struct PubStruct;
+            }
+            ",
+        );
+    }
+
+    #[test]
+    fn not_applicable_for_types_with_private_paths() {
+        check_assist_with_imports_locator_not_applicable(
+            auto_import,
+            TestImportsLocator::new,
+            r"
+            PrivateStruct<|>
+
+            pub mod PubMod {
+                struct PrivateStruct;
+            }
+            ",
+        );
+    }
+
+    #[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() {
+        check_assist_with_imports_locator_not_applicable(
+            auto_import,
+            TestImportsLocator::new,
+            r"
+            use PubStruct<|>;
+
+            pub mod PubMod {
+                pub struct PubStruct;
+            }",
+        );
+    }
+}

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() };

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(

crates/ra_assists/src/lib.rs

@@ -14,7 +14,7 @@ mod test_db;
 pub mod ast_transform;
 
 use either::Either;
-use hir::db::HirDatabase;
+use hir::{db::HirDatabase, ModuleDef};
 use ra_db::FileRange;
 use ra_syntax::{TextRange, TextUnit};
 use ra_text_edit::TextEdit;
@@ -77,6 +77,51 @@ 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 {
+    /// Finds all imports for the given name and the module that contains this name.
+    fn find_imports(&mut self, name_to_import: &str) -> Vec<ModuleDef>;
+}
+
+/// 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<H, F>(
+    db: &H,
+    range: FileRange,
+    mut imports_locator: F,
+) -> Vec<ResolvedAssist>
+where
+    H: HirDatabase + 'static,
+    F: ImportsLocator,
+{
+    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 +130,6 @@ pub fn assists<H>(db: &H, range: FileRange) -> Vec<ResolvedAssist>
 where
     H: HirDatabase + 'static,
 {
-    use std::cmp::Ordering;
-
     AssistCtx::with_ctx(db, range, true, |ctx| {
         let mut a = assists::all()
             .iter()
@@ -95,19 +138,24 @@ where
                 Assist::Resolved { assist } => assist,
                 Assist::Unresolved { .. } => unreachable!(),
             })
-            .collect::<Vec<_>>();
+            .collect();
-        a.sort_by(|a, b| match (a.get_first_action().target, b.get_first_action().target) {
+        sort_assists(&mut a);
-            (Some(a), Some(b)) => a.len().cmp(&b.len()),
-            (Some(_), None) => Ordering::Less,
-            (None, Some(_)) => Ordering::Greater,
-            (None, None) => Ordering::Equal,
-        });
         a
     })
 }
 
+fn sort_assists(assists: &mut Vec<ResolvedAssist>) {
+    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 +164,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,15 +217,69 @@ mod assists {
             early_return::convert_to_guarded_return,
         ]
     }
+
+    pub(crate) fn all_with_imports_locator<'a, DB: HirDatabase, F: ImportsLocator>(
+    ) -> &'a [fn(AssistCtx<DB>, &mut F) -> Option<Assist>] {
+        &[auto_import::auto_import]
+    }
 }
 
 #[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};
+    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<TestDB>,
+        test_file_id: FileId,
+    }
+
+    impl TestImportsLocator {
+        pub(crate) fn new(db: Arc<TestDB>, test_file_id: FileId) -> Self {
+            TestImportsLocator { db, test_file_id }
+        }
+    }
+
+    impl ImportsLocator for TestImportsLocator {
+        fn find_imports(&mut self, name_to_import: &str) -> Vec<hir::ModuleDef> {
+            let crate_def_map = self.db.crate_def_map(self.db.test_crate());
+            let mut findings = Vec::new();
+
+            let mut module_ids_to_process =
+                crate_def_map.modules_for_file(self.test_file_id).collect::<Vec<_>>();
+
+            while !module_ids_to_process.is_empty() {
+                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()
+                    {
+                        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<TestDB>) -> Option<Assist>,
@@ -206,6 +309,38 @@ mod helpers {
         assert_eq_text!(after, &actual);
     }
 
+    pub(crate) fn check_assist_with_imports_locator<F: ImportsLocator>(
+        assist: fn(AssistCtx<TestDB>, &mut F) -> Option<Assist>,
+        imports_locator_provider: fn(db: Arc<TestDB>, 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.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(),
+        };
+
+        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<TestDB>) -> Option<Assist>,
         before: &str,
@@ -279,6 +414,22 @@ mod helpers {
         assert!(assist.is_none());
     }
 
+    pub(crate) fn check_assist_with_imports_locator_not_applicable<F: ImportsLocator>(
+        assist: fn(AssistCtx<TestDB>, &mut F) -> Option<Assist>,
+        imports_locator_provider: fn(db: Arc<TestDB>, 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.as_ref(), frange, true, |ctx| assist(ctx, &mut imports_locator));
+        assert!(assist.is_none());
+    }
+
     pub(crate) fn check_assist_range_not_applicable(
         assist: fn(AssistCtx<TestDB>) -> Option<Assist>,
         before: &str,

crates/ra_hir/src/lib.rs

@@ -56,6 +56,7 @@ pub use hir_def::{
     nameres::ModuleSource,
     path::{ModPath, Path, PathKind},
     type_ref::Mutability,
+    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::Name, HirFileId, InFile, MacroCallId, MacroCallLoc, MacroDefId, MacroFile, Origin,

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<Assist> {
-    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;

crates/ra_ide/src/imports_locator.rs

@@ -0,0 +1,76 @@
+//! 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, NameDefinition, NameKind},
+    symbol_index::{self, FileSymbol},
+    Query,
+};
+use hir::{db::HirDatabase, ModuleDef, SourceBinder};
+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 get_name_definition(
+        &mut self,
+        db: &impl HirDatabase,
+        import_candidate: &FileSymbol,
+    ) -> Option<NameDefinition> {
+        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 ImportsLocator for ImportsLocatorIde<'_> {
+    fn find_imports(&mut self, name_to_import: &str) -> Vec<ModuleDef> {
+        let _p = profile("search_for_imports");
+        let db = self.source_binder.db;
+
+        let project_results = {
+            let mut query = Query::new(name_to_import.to_string());
+            query.exact();
+            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(40);
+            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 {
+                    Some(module_def)
+                } else {
+                    None
+                }
+            })
+            .collect()
+    }
+}

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;

docs/user/assists.md

@@ -209,6 +209,24 @@ fn main() {
 }
 ```
 
+## `auto_import`
+
+If the name is unresolved, provides all possible imports for it.
+
+```rust
+// BEFORE
+fn main() {
+    let map = HashMap┃::new();
+}
+
+// AFTER
+use std::collections::HashMap;
+
+fn main() {
+    let map = HashMap┃::new();
+}
+```
+
 ## `change_visibility`
 
 Adds or changes existing visibility specifier.