6 files changed, 143 insertions, 104 deletions

diff --git a/crates/ra_assists/src/handlers/add_new.rs b/crates/ra_assists/src/handlers/add_new.rs
index a08639311..2701eddb8 100644
--- a/crates/ra_assists/src/handlers/add_new.rs
+++ b/crates/ra_assists/src/handlers/add_new.rs
@@ -1,5 +1,5 @@
 use format_buf::format;
-use hir::InFile;
+use hir::{Adt, InFile};
 use join_to_string::join;
 use ra_syntax::{
     ast::{
@@ -135,16 +135,22 @@ fn find_struct_impl(ctx: &AssistCtx, strukt: &ast::StructDef) -> Option<Option<a
     })?;
     let mut sb = ctx.source_binder();
 
-    let struct_ty = {
+    let struct_def = {
         let src = InFile { file_id: ctx.frange.file_id.into(), value: strukt.clone() };
-        sb.to_def(src)?.ty(db)
+        sb.to_def(src)?
     };
 
     let block = module.descendants().filter_map(ast::ImplBlock::cast).find_map(|impl_blk| {
         let src = InFile { file_id: ctx.frange.file_id.into(), value: impl_blk.clone() };
         let blk = sb.to_def(src)?;
 
-        let same_ty = blk.target_ty(db) == struct_ty;
+        // FIXME: handle e.g. `struct S<T>; impl<U> S<U> {}`
+        // (we currently use the wrong type parameter)
+        // also we wouldn't want to use e.g. `impl S<u32>`
+        let same_ty = match blk.target_ty(db).as_adt() {
+            Some(def) => def == Adt::Struct(struct_def),
+            None => false,
+        };
         let not_trait_impl = blk.target_trait(db).is_none();
 
         if !(same_ty && not_trait_impl) {
diff --git a/crates/ra_assists/src/handlers/auto_import.rs b/crates/ra_assists/src/handlers/auto_import.rs
index 84b5474f9..1fb701da5 100644
--- a/crates/ra_assists/src/handlers/auto_import.rs
+++ b/crates/ra_assists/src/handlers/auto_import.rs
@@ -1,13 +1,9 @@
-use hir::ModPath;
 use ra_ide_db::imports_locator::ImportsLocator;
-use ra_syntax::{
-    ast::{self, AstNode},
-    SyntaxNode,
-};
+use ra_syntax::ast::{self, AstNode};
 
 use crate::{
-    assist_ctx::{ActionBuilder, Assist, AssistCtx},
-    auto_import_text_edit, AssistId,
+    assist_ctx::{Assist, AssistCtx},
+    insert_use_statement, AssistId,
 };
 use std::collections::BTreeSet;
 
@@ -31,31 +27,34 @@ use std::collections::BTreeSet;
 // # pub mod std { pub mod collections { pub struct HashMap { } } }
 // ```
 pub(crate) fn auto_import(ctx: AssistCtx) -> Option<Assist> {
-    let path_to_import: ast::Path = ctx.find_node_at_offset()?;
-    let path_to_import_syntax = path_to_import.syntax();
-    if path_to_import_syntax.ancestors().find_map(ast::UseItem::cast).is_some() {
+    let path_under_caret: ast::Path = ctx.find_node_at_offset()?;
+    if path_under_caret.syntax().ancestors().find_map(ast::UseItem::cast).is_some() {
         return None;
     }
-    let name_to_import =
-        path_to_import_syntax.descendants().find_map(ast::NameRef::cast)?.syntax().to_string();
 
-    let module = path_to_import_syntax.ancestors().find_map(ast::Module::cast);
+    let module = path_under_caret.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_to_import_syntax.ancestors().find_map(ast::SourceFile::cast)?;
+            let current_file =
+                path_under_caret.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()?;
-    if source_analyzer.resolve_path(ctx.db, &path_to_import).is_some() {
+
+    let name_ref_to_import =
+        path_under_caret.syntax().descendants().find_map(ast::NameRef::cast)?;
+    if source_analyzer
+        .resolve_path(ctx.db, &name_ref_to_import.syntax().ancestors().find_map(ast::Path::cast)?)
+        .is_some()
+    {
         return None;
     }
 
-    let mut imports_locator = ImportsLocator::new(ctx.db);
-
-    let proposed_imports = imports_locator
+    let name_to_import = name_ref_to_import.syntax().to_string();
+    let proposed_imports = ImportsLocator::new(ctx.db)
         .find_imports(&name_to_import)
         .into_iter()
         .filter_map(|module_def| module_with_name_to_import.find_use_path(ctx.db, module_def))
@@ -67,24 +66,24 @@ pub(crate) fn auto_import(ctx: AssistCtx) -> Option<Assist> {
         return None;
     }
 
-    ctx.add_assist_group(AssistId("auto_import"), format!("Import {}", name_to_import), || {
-        proposed_imports
-            .into_iter()
-            .map(|import| import_to_action(import, &position, &path_to_import_syntax))
-            .collect()
-    })
-}
-
-fn import_to_action(import: ModPath, position: &SyntaxNode, anchor: &SyntaxNode) -> ActionBuilder {
-    let mut action_builder = ActionBuilder::default();
-    action_builder.label(format!("Import `{}`", &import));
-    auto_import_text_edit(position, anchor, &import, action_builder.text_edit_builder());
-    action_builder
+    let mut group = ctx.add_assist_group(format!("Import {}", name_to_import));
+    for import in proposed_imports {
+        group.add_assist(AssistId("auto_import"), format!("Import `{}`", &import), |edit| {
+            edit.target(path_under_caret.syntax().text_range());
+            insert_use_statement(
+                &position,
+                path_under_caret.syntax(),
+                &import,
+                edit.text_edit_builder(),
+            );
+        });
+    }
+    group.finish()
 }
 
 #[cfg(test)]
 mod tests {
-    use crate::helpers::{check_assist, check_assist_not_applicable};
+    use crate::helpers::{check_assist, check_assist_not_applicable, check_assist_target};
 
     use super::*;
 
@@ -255,4 +254,40 @@ mod tests {
             ",
         );
     }
+
+    #[test]
+    fn auto_import_target() {
+        check_assist_target(
+            auto_import,
+            r"
+            struct AssistInfo {
+                group_label: Option<<|>GroupLabel>,
+            }
+
+            mod m { pub struct GroupLabel; }
+            ",
+            "GroupLabel",
+        )
+    }
+
+    #[test]
+    fn not_applicable_when_path_start_is_imported() {
+        check_assist_not_applicable(
+            auto_import,
+            r"
+            pub mod mod1 {
+                pub mod mod2 {
+                    pub mod mod3 {
+                        pub struct TestStruct;
+                    }
+                }
+            }
+
+            use mod1::mod2;
+            fn main() {
+                mod2::mod3::TestStruct<|>
+            }
+            ",
+        );
+    }
 }
diff --git a/crates/ra_assists/src/handlers/fill_match_arms.rs b/crates/ra_assists/src/handlers/fill_match_arms.rs
index 0908fc246..ae2437ed3 100644
--- a/crates/ra_assists/src/handlers/fill_match_arms.rs
+++ b/crates/ra_assists/src/handlers/fill_match_arms.rs
@@ -75,10 +75,10 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx) -> Option<Assist> {
 }
 
 fn is_trivial(arm: &ast::MatchArm) -> bool {
-    arm.pats().any(|pat| match pat {
-        ast::Pat::PlaceholderPat(..) => true,
+    match arm.pat() {
+        Some(ast::Pat::PlaceholderPat(..)) => true,
         _ => false,
-    })
+    }
 }
 
 fn resolve_enum_def(
diff --git a/crates/ra_assists/src/handlers/merge_match_arms.rs b/crates/ra_assists/src/handlers/merge_match_arms.rs
index 670614dd8..b2a194cb5 100644
--- a/crates/ra_assists/src/handlers/merge_match_arms.rs
+++ b/crates/ra_assists/src/handlers/merge_match_arms.rs
@@ -75,7 +75,7 @@ pub(crate) fn merge_match_arms(ctx: AssistCtx) -> Option<Assist> {
         } else {
             arms_to_merge
                 .iter()
-                .flat_map(ast::MatchArm::pats)
+                .filter_map(ast::MatchArm::pat)
                 .map(|x| x.syntax().to_string())
                 .collect::<Vec<String>>()
                 .join(" | ")
@@ -96,10 +96,10 @@ pub(crate) fn merge_match_arms(ctx: AssistCtx) -> Option<Assist> {
 }
 
 fn contains_placeholder(a: &ast::MatchArm) -> bool {
-    a.pats().any(|x| match x {
-        ra_syntax::ast::Pat::PlaceholderPat(..) => true,
+    match a.pat() {
+        Some(ra_syntax::ast::Pat::PlaceholderPat(..)) => true,
         _ => false,
-    })
+    }
 }
 
 fn next_arm(arm: &ast::MatchArm) -> Option<ast::MatchArm> {
diff --git a/crates/ra_assists/src/handlers/move_guard.rs b/crates/ra_assists/src/handlers/move_guard.rs
index 2b91ce7c4..a61a2ba3e 100644
--- a/crates/ra_assists/src/handlers/move_guard.rs
+++ b/crates/ra_assists/src/handlers/move_guard.rs
@@ -90,7 +90,7 @@ pub(crate) fn move_guard_to_arm_body(ctx: AssistCtx) -> Option<Assist> {
 // ```
 pub(crate) fn move_arm_cond_to_match_guard(ctx: AssistCtx) -> Option<Assist> {
     let match_arm: MatchArm = ctx.find_node_at_offset::<MatchArm>()?;
-    let last_match_pat = match_arm.pats().last()?;
+    let match_pat = match_arm.pat()?;
 
     let arm_body = match_arm.expr()?;
     let if_expr: IfExpr = IfExpr::cast(arm_body.syntax().clone())?;
@@ -122,8 +122,8 @@ pub(crate) fn move_arm_cond_to_match_guard(ctx: AssistCtx) -> Option<Assist> {
                 _ => edit.replace(if_expr.syntax().text_range(), then_block.syntax().text()),
             }
 
-            edit.insert(last_match_pat.syntax().text_range().end(), buf);
-            edit.set_cursor(last_match_pat.syntax().text_range().end() + TextUnit::from(1));
+            edit.insert(match_pat.syntax().text_range().end(), buf);
+            edit.set_cursor(match_pat.syntax().text_range().end() + TextUnit::from(1));
         },
     )
 }
diff --git a/crates/ra_assists/src/handlers/add_import.rs b/crates/ra_assists/src/handlers/replace_qualified_name_with_use.rs
index f03dddac8..b70c88ec2 100644
--- a/crates/ra_assists/src/handlers/add_import.rs
+++ b/crates/ra_assists/src/handlers/replace_qualified_name_with_use.rs
@@ -12,10 +12,10 @@ use crate::{
     AssistId,
 };
 
-/// This function produces sequence of text edits into edit
-/// to import the target path in the most appropriate scope given
-/// the cursor position
-pub fn auto_import_text_edit(
+/// Creates and inserts a use statement for the given path to import.
+/// The use statement is inserted in the scope most appropriate to the
+/// the cursor position given, additionally merged with the existing use imports.
+pub fn insert_use_statement(
     // Ideally the position of the cursor, used to
     position: &SyntaxNode,
     // The statement to use as anchor (last resort)
@@ -37,9 +37,9 @@ pub fn auto_import_text_edit(
     }
 }
 
-// Assist: add_import
+// Assist: replace_qualified_name_with_use
 //
-// Adds a use statement for a given fully-qualified path.
+// Adds a use statement for a given fully-qualified name.
 //
 // ```
 // fn process(map: std::collections::<|>HashMap<String, String>) {}
@@ -50,7 +50,7 @@ pub fn auto_import_text_edit(
 //
 // fn process(map: HashMap<String, String>) {}
 // ```
-pub(crate) fn add_import(ctx: AssistCtx) -> Option<Assist> {
+pub(crate) fn replace_qualified_name_with_use(ctx: AssistCtx) -> Option<Assist> {
     let path: ast::Path = ctx.find_node_at_offset()?;
     // We don't want to mess with use statements
     if path.syntax().ancestors().find_map(ast::UseItem::cast).is_some() {
@@ -72,9 +72,13 @@ pub(crate) fn add_import(ctx: AssistCtx) -> Option<Assist> {
         }
     };
 
-    ctx.add_assist(AssistId("add_import"), format!("Import {}", fmt_segments(&segments)), |edit| {
-        apply_auto_import(&position, &path, &segments, edit.text_edit_builder());
-    })
+    ctx.add_assist(
+        AssistId("replace_qualified_name_with_use"),
+        "Replace qualified path with use",
+        |edit| {
+            replace_with_use(&position, &path, &segments, edit.text_edit_builder());
+        },
+    )
 }
 
 fn collect_path_segments_raw(
@@ -107,12 +111,6 @@ fn collect_path_segments_raw(
     Some(segments.len() - oldlen)
 }
 
-fn fmt_segments(segments: &[SmolStr]) -> String {
-    let mut buf = String::new();
-    fmt_segments_raw(segments, &mut buf);
-    buf
-}
-
 fn fmt_segments_raw(segments: &[SmolStr], buf: &mut String) {
     let mut iter = segments.iter();
     if let Some(s) = iter.next() {
@@ -558,7 +556,7 @@ fn make_assist_add_nested_import(
     }
 }
 
-fn apply_auto_import(
+fn replace_with_use(
     container: &SyntaxNode,
     path: &ast::Path,
     target: &[SmolStr],
@@ -567,7 +565,7 @@ fn apply_auto_import(
     let action = best_action_for_target(container.clone(), path.syntax().clone(), target);
     make_assist(&action, target, edit);
     if let Some(last) = path.segment() {
-        // Here we are assuming the assist will provide a  correct use statement
+        // Here we are assuming the assist will provide a correct use statement
         // so we can delete the path qualifier
         edit.delete(TextRange::from_to(
             path.syntax().text_range().start(),
@@ -603,9 +601,9 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_auto_import_add_use_no_anchor() {
+    fn test_replace_add_use_no_anchor() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 std::fmt::Debug<|>
     ",
@@ -617,9 +615,9 @@ Debug<|>
         );
     }
     #[test]
-    fn test_auto_import_add_use_no_anchor_with_item_below() {
+    fn test_replace_add_use_no_anchor_with_item_below() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 std::fmt::Debug<|>
 
@@ -638,9 +636,9 @@ fn main() {
     }
 
     #[test]
-    fn test_auto_import_add_use_no_anchor_with_item_above() {
+    fn test_replace_add_use_no_anchor_with_item_above() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 fn main() {
 }
@@ -659,9 +657,9 @@ Debug<|>
     }
 
     #[test]
-    fn test_auto_import_add_use_no_anchor_2seg() {
+    fn test_replace_add_use_no_anchor_2seg() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 std::fmt<|>::Debug
     ",
@@ -674,9 +672,9 @@ fmt<|>::Debug
     }
 
     #[test]
-    fn test_auto_import_add_use() {
+    fn test_replace_add_use() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use stdx;
 
@@ -694,9 +692,9 @@ impl Debug<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_file_use_other_anchor() {
+    fn test_replace_file_use_other_anchor() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 impl std::fmt::Debug<|> for Foo {
 }
@@ -711,9 +709,9 @@ impl Debug<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_add_use_other_anchor_indent() {
+    fn test_replace_add_use_other_anchor_indent() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
     impl std::fmt::Debug<|> for Foo {
     }
@@ -728,9 +726,9 @@ impl Debug<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_split_different() {
+    fn test_replace_split_different() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt;
 
@@ -747,9 +745,9 @@ impl io<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_split_self_for_use() {
+    fn test_replace_split_self_for_use() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt;
 
@@ -766,9 +764,9 @@ impl Debug<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_split_self_for_target() {
+    fn test_replace_split_self_for_target() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt::Debug;
 
@@ -785,9 +783,9 @@ impl fmt<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_add_to_nested_self_nested() {
+    fn test_replace_add_to_nested_self_nested() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt::{Debug, nested::{Display}};
 
@@ -804,9 +802,9 @@ impl nested<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_add_to_nested_self_already_included() {
+    fn test_replace_add_to_nested_self_already_included() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt::{Debug, nested::{self, Display}};
 
@@ -823,9 +821,9 @@ impl nested<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_add_to_nested_nested() {
+    fn test_replace_add_to_nested_nested() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt::{Debug, nested::{Display}};
 
@@ -842,9 +840,9 @@ impl Debug<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_split_common_target_longer() {
+    fn test_replace_split_common_target_longer() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt::Debug;
 
@@ -861,9 +859,9 @@ impl Display<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_split_common_use_longer() {
+    fn test_replace_split_common_use_longer() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt::nested::Debug;
 
@@ -880,9 +878,9 @@ impl Display<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_use_nested_import() {
+    fn test_replace_use_nested_import() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use crate::{
     ty::{Substs, Ty},
@@ -903,9 +901,9 @@ fn foo() { lower<|>::trait_env() }
     }
 
     #[test]
-    fn test_auto_import_alias() {
+    fn test_replace_alias() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt as foo;
 
@@ -922,9 +920,9 @@ impl Debug<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_not_applicable_one_segment() {
+    fn test_replace_not_applicable_one_segment() {
         check_assist_not_applicable(
-            add_import,
+            replace_qualified_name_with_use,
             "
 impl foo<|> for Foo {
 }
@@ -933,9 +931,9 @@ impl foo<|> for Foo {
     }
 
     #[test]
-    fn test_auto_import_not_applicable_in_use() {
+    fn test_replace_not_applicable_in_use() {
         check_assist_not_applicable(
-            add_import,
+            replace_qualified_name_with_use,
             "
 use std::fmt<|>;
 ",
@@ -943,9 +941,9 @@ use std::fmt<|>;
     }
 
     #[test]
-    fn test_auto_import_add_use_no_anchor_in_mod_mod() {
+    fn test_replace_add_use_no_anchor_in_mod_mod() {
         check_assist(
-            add_import,
+            replace_qualified_name_with_use,
             "
 mod foo {
     mod bar {