5 files changed, 188 insertions, 100 deletions

diff --git a/crates/ra_assists/src/handlers/add_missing_impl_members.rs b/crates/ra_assists/src/handlers/add_missing_impl_members.rs
index e5920b6f6..722f207e2 100644
--- a/crates/ra_assists/src/handlers/add_missing_impl_members.rs
+++ b/crates/ra_assists/src/handlers/add_missing_impl_members.rs
@@ -151,7 +151,7 @@ fn add_missing_impl_members_inner(
                 ast::ImplItem::FnDef(def) => ast::ImplItem::FnDef(add_body(def)),
                 _ => it,
             })
-            .map(|it| edit::strip_attrs_and_docs(&it));
+            .map(|it| edit::remove_attrs_and_docs(&it));
         let new_impl_item_list = impl_item_list.append_items(items);
         let cursor_position = {
             let first_new_item = new_impl_item_list.impl_items().nth(n_existing_items).unwrap();
diff --git a/crates/ra_assists/src/handlers/auto_import.rs b/crates/ra_assists/src/handlers/auto_import.rs
index bb280f633..99682e023 100644
--- a/crates/ra_assists/src/handlers/auto_import.rs
+++ b/crates/ra_assists/src/handlers/auto_import.rs
@@ -17,6 +17,7 @@ use crate::{
     utils::insert_use_statement,
     AssistId,
 };
+use either::Either;
 
 // Assist: auto_import
 //
@@ -58,6 +59,7 @@ pub(crate) fn auto_import(ctx: AssistCtx) -> Option<Assist> {
     group.finish()
 }
 
+#[derive(Debug)]
 struct AutoImportAssets {
     import_candidate: ImportCandidate,
     module_with_name_to_import: Module,
@@ -127,14 +129,14 @@ impl AutoImportAssets {
         ImportsLocator::new(db)
             .find_imports(&self.get_search_query())
             .into_iter()
-            .filter_map(|module_def| match &self.import_candidate {
+            .filter_map(|candidate| match &self.import_candidate {
                 ImportCandidate::TraitAssocItem(assoc_item_type, _) => {
-                    let located_assoc_item = match module_def {
-                        ModuleDef::Function(located_function) => located_function
+                    let located_assoc_item = match candidate {
+                        Either::Left(ModuleDef::Function(located_function)) => located_function
                             .as_assoc_item(db)
                             .map(|assoc| assoc.container(db))
                             .and_then(Self::assoc_to_trait),
-                        ModuleDef::Const(located_const) => located_const
+                        Either::Left(ModuleDef::Const(located_const)) => located_const
                             .as_assoc_item(db)
                             .map(|assoc| assoc.container(db))
                             .and_then(Self::assoc_to_trait),
@@ -153,10 +155,11 @@ impl AutoImportAssets {
                             |_, assoc| Self::assoc_to_trait(assoc.container(db)),
                         )
                         .map(ModuleDef::from)
+                        .map(Either::Left)
                 }
                 ImportCandidate::TraitMethod(function_callee, _) => {
                     let located_assoc_item =
-                        if let ModuleDef::Function(located_function) = module_def {
+                        if let Either::Left(ModuleDef::Function(located_function)) = candidate {
                             located_function
                                 .as_assoc_item(db)
                                 .map(|assoc| assoc.container(db))
@@ -179,10 +182,18 @@ impl AutoImportAssets {
                             },
                         )
                         .map(ModuleDef::from)
+                        .map(Either::Left)
+                }
+                _ => Some(candidate),
+            })
+            .filter_map(|candidate| match candidate {
+                Either::Left(module_def) => {
+                    self.module_with_name_to_import.find_use_path(db, module_def)
+                }
+                Either::Right(macro_def) => {
+                    self.module_with_name_to_import.find_use_path(db, macro_def)
                 }
-                _ => Some(module_def),
             })
-            .filter_map(|module_def| self.module_with_name_to_import.find_use_path(db, module_def))
             .filter(|use_path| !use_path.segments.is_empty())
             .take(20)
             .collect::<BTreeSet<_>>()
@@ -440,6 +451,30 @@ mod tests {
     }
 
     #[test]
+    fn macro_import() {
+        check_assist(
+            auto_import,
+            r"
+                    //- /lib.rs crate:crate_with_macro
+                    #[macro_export]
+                    macro_rules! foo {
+                        () => ()
+                    }
+
+                    //- /main.rs crate:main deps:crate_with_macro
+                    fn main() {
+                        foo<|>
+                    }",
+            r"use crate_with_macro::foo;
+
+fn main() {
+    foo<|>
+}
+",
+        );
+    }
+
+    #[test]
     fn auto_import_target() {
         check_assist_target(
             auto_import,
diff --git a/crates/ra_assists/src/handlers/change_visibility.rs b/crates/ra_assists/src/handlers/change_visibility.rs
index 54e0a6c84..cd6d1ee6c 100644
--- a/crates/ra_assists/src/handlers/change_visibility.rs
+++ b/crates/ra_assists/src/handlers/change_visibility.rs
@@ -2,13 +2,14 @@ use ra_syntax::{
     ast::{self, NameOwner, VisibilityOwner},
     AstNode,
     SyntaxKind::{
-        ATTR, COMMENT, CONST_DEF, ENUM_DEF, FN_DEF, IDENT, MODULE, STRUCT_DEF, TRAIT_DEF,
-        VISIBILITY, WHITESPACE,
+        ATTR, COMMENT, CONST_DEF, ENUM_DEF, FN_DEF, MODULE, STRUCT_DEF, TRAIT_DEF, VISIBILITY,
+        WHITESPACE,
     },
     SyntaxNode, TextUnit, T,
 };
 
 use crate::{Assist, AssistCtx, AssistId};
+use test_utils::tested_by;
 
 // Assist: change_visibility
 //
@@ -47,13 +48,16 @@ fn add_vis(ctx: AssistCtx) -> Option<Assist> {
         }
         (vis_offset(&parent), keyword.text_range())
     } else {
-        let ident = ctx.token_at_offset().find(|leaf| leaf.kind() == IDENT)?;
-        let field = ident.parent().ancestors().find_map(ast::RecordFieldDef::cast)?;
-        if field.name()?.syntax().text_range() != ident.text_range() && field.visibility().is_some()
-        {
+        let field_name: ast::Name = ctx.find_node_at_offset()?;
+        let field = field_name.syntax().ancestors().find_map(ast::RecordFieldDef::cast)?;
+        if field.name()? != field_name {
+            tested_by!(change_visibility_field_false_positive);
             return None;
         }
-        (vis_offset(field.syntax()), ident.text_range())
+        if field.visibility().is_some() {
+            return None;
+        }
+        (vis_offset(field.syntax()), field_name.syntax().text_range())
     };
 
     ctx.add_assist(AssistId("change_visibility"), "Change visibility to pub(crate)", |edit| {
@@ -98,8 +102,11 @@ fn change_vis(ctx: AssistCtx, vis: ast::Visibility) -> Option<Assist> {
 
 #[cfg(test)]
 mod tests {
+    use test_utils::covers;
+
+    use crate::helpers::{check_assist, check_assist_not_applicable, check_assist_target};
+
     use super::*;
-    use crate::helpers::{check_assist, check_assist_target};
 
     #[test]
     fn change_visibility_adds_pub_crate_to_items() {
@@ -120,8 +127,17 @@ mod tests {
     fn change_visibility_works_with_struct_fields() {
         check_assist(
             change_visibility,
-            "struct S { <|>field: u32 }",
-            "struct S { <|>pub(crate) field: u32 }",
+            r"struct S { <|>field: u32 }",
+            r"struct S { <|>pub(crate) field: u32 }",
+        )
+    }
+
+    #[test]
+    fn change_visibility_field_false_positive() {
+        covers!(change_visibility_field_false_positive);
+        check_assist_not_applicable(
+            change_visibility,
+            r"struct S { field: [(); { let <|>x = ();}] }",
         )
     }
 
@@ -144,7 +160,7 @@ mod tests {
     fn change_visibility_handles_comment_attrs() {
         check_assist(
             change_visibility,
-            "
+            r"
             /// docs
 
             // comments
@@ -152,7 +168,7 @@ mod tests {
             #[derive(Debug)]
             <|>struct Foo;
             ",
-            "
+            r"
             /// docs
 
             // comments
diff --git a/crates/ra_assists/src/handlers/fill_match_arms.rs b/crates/ra_assists/src/handlers/fill_match_arms.rs
index 7463b2af7..add82e5b1 100644
--- a/crates/ra_assists/src/handlers/fill_match_arms.rs
+++ b/crates/ra_assists/src/handlers/fill_match_arms.rs
@@ -2,7 +2,7 @@
 
 use std::iter;
 
-use hir::{Adt, HasSource, Semantics};
+use hir::{Adt, HasSource, ModuleDef, Semantics};
 use itertools::Itertools;
 use ra_ide_db::RootDatabase;
 
@@ -30,8 +30,8 @@ use ast::{MatchArm, Pat};
 //
 // fn handle(action: Action) {
 //     match action {
-//         Action::Move { distance } => (),
-//         Action::Stop => (),
+//         Action::Move { distance } => {}
+//         Action::Stop => {}
 //     }
 // }
 // ```
@@ -57,7 +57,7 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx) -> Option<Assist> {
             .into_iter()
             .filter_map(|variant| build_pat(ctx.db, module, variant))
             .filter(|variant_pat| is_variant_missing(&mut arms, variant_pat))
-            .map(|pat| make::match_arm(iter::once(pat), make::expr_unit()))
+            .map(|pat| make::match_arm(iter::once(pat), make::expr_empty_block()))
             .collect()
     } else if let Some(enum_defs) = resolve_tuple_of_enum_def(&ctx.sema, &expr) {
         // Partial fill not currently supported for tuple of enums.
@@ -86,7 +86,7 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx) -> Option<Assist> {
                 ast::Pat::from(make::tuple_pat(patterns))
             })
             .filter(|variant_pat| is_variant_missing(&mut arms, variant_pat))
-            .map(|pat| make::match_arm(iter::once(pat), make::expr_unit()))
+            .map(|pat| make::match_arm(iter::once(pat), make::expr_empty_block()))
             .collect()
     } else {
         return None;
@@ -154,7 +154,7 @@ fn resolve_tuple_of_enum_def(
 }
 
 fn build_pat(db: &RootDatabase, module: hir::Module, var: hir::EnumVariant) -> Option<ast::Pat> {
-    let path = crate::ast_transform::path_to_ast(module.find_use_path(db, var.into())?);
+    let path = crate::ast_transform::path_to_ast(module.find_use_path(db, ModuleDef::from(var))?);
 
     // FIXME: use HIR for this; it doesn't currently expose struct vs. tuple vs. unit variants though
     let pat: ast::Pat = match var.source(db).value.kind() {
@@ -192,8 +192,8 @@ mod tests {
             fn main() {
                 match A::As<|> {
                     A::As,
-                    A::Bs{x,y:Some(_)} => (),
-                    A::Cs(_, Some(_)) => (),
+                    A::Bs{x,y:Some(_)} => {}
+                    A::Cs(_, Some(_)) => {}
                 }
             }
             "#,
@@ -227,8 +227,8 @@ mod tests {
             }
             fn main() {
                 match A::As<|> {
-                    A::Bs{x,y:Some(_)} => (),
-                    A::Cs(_, Some(_)) => (),
+                    A::Bs{x,y:Some(_)} => {}
+                    A::Cs(_, Some(_)) => {}
                 }
             }
             "#,
@@ -240,9 +240,9 @@ mod tests {
             }
             fn main() {
                 match <|>A::As {
-                    A::Bs{x,y:Some(_)} => (),
-                    A::Cs(_, Some(_)) => (),
-                    A::As => (),
+                    A::Bs{x,y:Some(_)} => {}
+                    A::Cs(_, Some(_)) => {}
+                    A::As => {}
                 }
             }
             "#,
@@ -261,7 +261,7 @@ mod tests {
             }
             fn main() {
                 match A::As<|> {
-                    A::Cs(_) | A::Bs => (),
+                    A::Cs(_) | A::Bs => {}
                 }
             }
             "#,
@@ -273,8 +273,8 @@ mod tests {
             }
             fn main() {
                 match <|>A::As {
-                    A::Cs(_) | A::Bs => (),
-                    A::As => (),
+                    A::Cs(_) | A::Bs => {}
+                    A::As => {}
                 }
             }
             "#,
@@ -299,8 +299,8 @@ mod tests {
             }
             fn main() {
                 match A::As<|> {
-                    A::Bs if 0 < 1 => (),
-                    A::Ds(_value) => (),
+                    A::Bs if 0 < 1 => {}
+                    A::Ds(_value) => { let x = 1; }
                     A::Es(B::Xs) => (),
                 }
             }
@@ -319,11 +319,11 @@ mod tests {
             }
             fn main() {
                 match <|>A::As {
-                    A::Bs if 0 < 1 => (),
-                    A::Ds(_value) => (),
+                    A::Bs if 0 < 1 => {}
+                    A::Ds(_value) => { let x = 1; }
                     A::Es(B::Xs) => (),
-                    A::As => (),
-                    A::Cs => (),
+                    A::As => {}
+                    A::Cs => {}
                 }
             }
             "#,
@@ -360,11 +360,11 @@ mod tests {
             fn main() {
                 let a = A::As;
                 match <|>a {
-                    A::As => (),
-                    A::Bs => (),
-                    A::Cs(_) => (),
-                    A::Ds(_, _) => (),
-                    A::Es { x, y } => (),
+                    A::As => {}
+                    A::Bs => {}
+                    A::Cs(_) => {}
+                    A::Ds(_, _) => {}
+                    A::Es { x, y } => {}
                 }
             }
             "#,
@@ -405,10 +405,10 @@ mod tests {
                 let a = A::One;
                 let b = B::One;
                 match <|>(a, b) {
-                    (A::One, B::One) => (),
-                    (A::One, B::Two) => (),
-                    (A::Two, B::One) => (),
-                    (A::Two, B::Two) => (),
+                    (A::One, B::One) => {}
+                    (A::One, B::Two) => {}
+                    (A::Two, B::One) => {}
+                    (A::Two, B::Two) => {}
                 }
             }
             "#,
@@ -449,10 +449,10 @@ mod tests {
                 let a = A::One;
                 let b = B::One;
                 match <|>(&a, &b) {
-                    (A::One, B::One) => (),
-                    (A::One, B::Two) => (),
-                    (A::Two, B::One) => (),
-                    (A::Two, B::Two) => (),
+                    (A::One, B::One) => {}
+                    (A::One, B::Two) => {}
+                    (A::Two, B::One) => {}
+                    (A::Two, B::Two) => {}
                 }
             }
             "#,
@@ -477,7 +477,7 @@ mod tests {
                 let a = A::One;
                 let b = B::One;
                 match (a<|>, b) {
-                    (A::Two, B::One) => (),
+                    (A::Two, B::One) => {}
                 }
             }
             "#,
@@ -502,10 +502,10 @@ mod tests {
                 let a = A::One;
                 let b = B::One;
                 match (a<|>, b) {
-                    (A::Two, B::One) => (),
-                    (A::One, B::One) => (),
-                    (A::One, B::Two) => (),
-                    (A::Two, B::Two) => (),
+                    (A::Two, B::One) => {}
+                    (A::One, B::One) => {}
+                    (A::One, B::Two) => {}
+                    (A::Two, B::Two) => {}
                 }
             }
             "#,
@@ -555,7 +555,7 @@ mod tests {
 
             fn foo(a: &A) {
                 match <|>a {
-                    A::As => (),
+                    A::As => {}
                 }
             }
             "#,
@@ -580,7 +580,7 @@ mod tests {
 
             fn foo(a: &mut A) {
                 match <|>a {
-                    A::Es { x, y } => (),
+                    A::Es { x, y } => {}
                 }
             }
             "#,
@@ -611,7 +611,7 @@ mod tests {
 
             fn main() {
                 match E::X {
-                    <|>_ => {},
+                    <|>_ => {}
                 }
             }
             "#,
@@ -620,8 +620,8 @@ mod tests {
 
             fn main() {
                 match <|>E::X {
-                    E::X => (),
-                    E::Y => (),
+                    E::X => {}
+                    E::Y => {}
                 }
             }
             "#,
@@ -648,8 +648,8 @@ mod tests {
 
             fn main() {
                 match <|>X {
-                    X => (),
-                    foo::E::Y => (),
+                    X => {}
+                    foo::E::Y => {}
                 }
             }
             "#,
diff --git a/crates/ra_assists/src/handlers/merge_imports.rs b/crates/ra_assists/src/handlers/merge_imports.rs
index 89bc975bd..9c57d1e30 100644
--- a/crates/ra_assists/src/handlers/merge_imports.rs
+++ b/crates/ra_assists/src/handlers/merge_imports.rs
@@ -1,9 +1,9 @@
 use std::iter::successors;
 
 use ra_syntax::{
-    algo::neighbor,
+    algo::{neighbor, SyntaxRewriter},
     ast::{self, edit::AstNodeEdit, make},
-    AstNode, AstToken, Direction, InsertPosition, SyntaxElement, TextRange, T,
+    AstNode, Direction, InsertPosition, SyntaxElement, T,
 };
 
 use crate::{Assist, AssistCtx, AssistId};
@@ -22,9 +22,10 @@ use crate::{Assist, AssistCtx, AssistId};
 // ```
 pub(crate) fn merge_imports(ctx: AssistCtx) -> Option<Assist> {
     let tree: ast::UseTree = ctx.find_node_at_offset()?;
-    let (new_tree, to_delete) = if let Some(use_item) =
-        tree.syntax().parent().and_then(ast::UseItem::cast)
-    {
+    let mut rewriter = SyntaxRewriter::default();
+    let mut offset = ctx.frange.range.start();
+
+    if let Some(use_item) = tree.syntax().parent().and_then(ast::UseItem::cast) {
         let (merged, to_delete) = next_prev()
             .filter_map(|dir| neighbor(&use_item, dir))
             .filter_map(|it| Some((it.clone(), it.use_tree()?)))
@@ -32,42 +33,28 @@ pub(crate) fn merge_imports(ctx: AssistCtx) -> Option<Assist> {
                 Some((try_merge_trees(&tree, &use_tree)?, use_item.clone()))
             })?;
 
-        let mut range = to_delete.syntax().text_range();
-        let next_ws = to_delete
-            .syntax()
-            .next_sibling_or_token()
-            .and_then(|it| it.into_token())
-            .and_then(ast::Whitespace::cast);
-        if let Some(ws) = next_ws {
-            range = range.extend_to(&ws.syntax().text_range())
+        rewriter.replace_ast(&tree, &merged);
+        rewriter += to_delete.remove();
+
+        if to_delete.syntax().text_range().end() < offset {
+            offset -= to_delete.syntax().text_range().len();
         }
-        (merged, range)
     } else {
         let (merged, to_delete) = next_prev()
             .filter_map(|dir| neighbor(&tree, dir))
             .find_map(|use_tree| Some((try_merge_trees(&tree, &use_tree)?, use_tree.clone())))?;
 
-        let mut range = to_delete.syntax().text_range();
-        if let Some((dir, nb)) = next_prev().find_map(|dir| Some((dir, neighbor(&to_delete, dir)?)))
-        {
-            let nb_range = nb.syntax().text_range();
-            if dir == Direction::Prev {
-                range = TextRange::from_to(nb_range.end(), range.end());
-            } else {
-                range = TextRange::from_to(range.start(), nb_range.start());
-            }
+        rewriter.replace_ast(&tree, &merged);
+        rewriter += to_delete.remove();
+
+        if to_delete.syntax().text_range().end() < offset {
+            offset -= to_delete.syntax().text_range().len();
         }
-        (merged, range)
     };
 
-    let mut offset = ctx.frange.range.start();
     ctx.add_assist(AssistId("merge_imports"), "Merge imports", |edit| {
-        edit.replace_ast(tree, new_tree);
-        edit.delete(to_delete);
-
-        if to_delete.end() <= offset {
-            offset -= to_delete.len();
-        }
+        edit.rewrite(rewriter);
+        // FIXME: we only need because our diff is imprecise
         edit.set_cursor(offset);
     })
 }
@@ -156,7 +143,7 @@ use std::fmt::Debug;
 use std::fmt<|>::Display;
 ",
             r"
-use std::fmt<|>::{Display, Debug};
+use std::fmt:<|>:{Display, Debug};
 ",
         );
     }
@@ -178,7 +165,57 @@ use std::{fmt<|>::{Debug, Display}};
 use std::{fmt::Debug, fmt<|>::Display};
 ",
             r"
-use std::{fmt<|>::{Display, Debug}};
+use std::{fmt::<|>{Display, Debug}};
+",
+        );
+    }
+
+    #[test]
+    fn removes_just_enough_whitespace() {
+        check_assist(
+            merge_imports,
+            r"
+use foo<|>::bar;
+use foo::baz;
+
+/// Doc comment
+",
+            r"
+use foo<|>::{bar, baz};
+
+/// Doc comment
+",
+        );
+    }
+
+    #[test]
+    fn works_with_trailing_comma() {
+        check_assist(
+            merge_imports,
+            r"
+use {
+    foo<|>::bar,
+    foo::baz,
+};
+",
+            r"
+use {
+    foo<|>::{bar, baz},
+};
+",
+        );
+        check_assist(
+            merge_imports,
+            r"
+use {
+    foo::baz,
+    foo<|>::bar,
+};
+",
+            r"
+use {
+    foo::{bar<|>, baz},
+};
 ",
         );
     }