Add tests for action target ranges

10 files changed, 210 insertions, 16 deletions

diff --git a/crates/ra_assists/src/add_derive.rs b/crates/ra_assists/src/add_derive.rs
index de33b356c..ea9707631 100644
--- a/crates/ra_assists/src/add_derive.rs
+++ b/crates/ra_assists/src/add_derive.rs
@@ -39,7 +39,7 @@ fn derive_insertion_offset(nominal: &ast::NominalDef) -> Option<TextUnit> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::helpers::check_assist;
+    use crate::helpers::{check_assist, check_assist_target};
 
     #[test]
     fn add_derive_new() {
@@ -81,4 +81,21 @@ struct Foo { a: i32, }
             ",
         );
     }
+
+    #[test]
+    fn add_derive_target() {
+        check_assist_target(
+            add_derive,
+            "
+struct SomeThingIrrelevant;
+/// `Foo` is a pretty important struct.
+/// It does stuff.
+struct Foo { a: i32<|>, }
+struct EvenMoreIrrelevant;
+            ",
+            "/// `Foo` is a pretty important struct.
+/// It does stuff.
+struct Foo { a: i32, }",
+        );
+    }
 }
diff --git a/crates/ra_assists/src/add_impl.rs b/crates/ra_assists/src/add_impl.rs
index f2360bc89..32fc074a6 100644
--- a/crates/ra_assists/src/add_impl.rs
+++ b/crates/ra_assists/src/add_impl.rs
@@ -11,6 +11,7 @@ pub(crate) fn add_impl(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
     let nominal = ctx.node_at_offset::<ast::NominalDef>()?;
     let name = nominal.name()?;
     ctx.build("add impl", |edit| {
+        edit.target(nominal.syntax().range());
         let type_params = nominal.type_param_list();
         let start_offset = nominal.syntax().range().end();
         let mut buf = String::new();
@@ -37,7 +38,7 @@ pub(crate) fn add_impl(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::helpers::check_assist;
+    use crate::helpers::{check_assist, check_assist_target};
 
     #[test]
     fn test_add_impl() {
@@ -54,4 +55,18 @@ mod tests {
         );
     }
 
+    #[test]
+    fn add_impl_target() {
+        check_assist_target(
+            add_impl,
+            "
+struct SomeThingIrrelevant;
+/// Has a lifetime parameter
+struct Foo<'a, T: Foo<'a>> {<|>}
+struct EvenMoreIrrelevant;
+",
+            "/// Has a lifetime parameter
+struct Foo<'a, T: Foo<'a>> {}",
+        );
+    }
 }
diff --git a/crates/ra_assists/src/change_visibility.rs b/crates/ra_assists/src/change_visibility.rs
index 73dd8319f..6d9a4eec2 100644
--- a/crates/ra_assists/src/change_visibility.rs
+++ b/crates/ra_assists/src/change_visibility.rs
@@ -31,14 +31,14 @@ fn add_vis(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
         if parent.children().any(|child| child.kind() == VISIBILITY) {
             return None;
         }
-        (vis_offset(parent), parent.range())
+        (vis_offset(parent), keyword.range())
     } else {
         let ident = ctx.leaf_at_offset().find(|leaf| leaf.kind() == IDENT)?;
         let field = ident.ancestors().find_map(ast::NamedFieldDef::cast)?;
         if field.name()?.syntax().range() != ident.range() && field.visibility().is_some() {
             return None;
         }
-        (vis_offset(field.syntax()), field.syntax().range())
+        (vis_offset(field.syntax()), ident.range())
     };
 
     ctx.build("make pub(crate)", |edit| {
@@ -80,7 +80,7 @@ fn change_vis(ctx: AssistCtx<impl HirDatabase>, vis: &ast::Visibility) -> Option
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::helpers::check_assist;
+    use crate::helpers::{check_assist, check_assist_target};
 
     #[test]
     fn change_visibility_adds_pub_crate_to_items() {
@@ -138,4 +138,11 @@ mod tests {
             ",
         )
     }
+
+    #[test]
+    fn change_visibility_target() {
+        check_assist_target(change_visibility, "<|>fn foo() {}", "fn");
+        check_assist_target(change_visibility, "pub(crate)<|> fn foo() {}", "pub(crate)");
+        check_assist_target(change_visibility, "struct S { <|>field: u32 }", "field");
+    }
 }
diff --git a/crates/ra_assists/src/fill_match_arms.rs b/crates/ra_assists/src/fill_match_arms.rs
index 741f75e2a..69b535a27 100644
--- a/crates/ra_assists/src/fill_match_arms.rs
+++ b/crates/ra_assists/src/fill_match_arms.rs
@@ -65,6 +65,7 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist
             buf.push_str(" => (),\n");
         }
         buf.push_str("}");
+        edit.target(match_expr.syntax().range());
         edit.set_cursor(expr.syntax().range().start());
         edit.replace_node_and_indent(match_expr.syntax(), buf);
     })
@@ -72,7 +73,7 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist
 
 #[cfg(test)]
 mod tests {
-    use crate::helpers::check_assist;
+    use crate::helpers::{check_assist, check_assist_target};
 
     use super::fill_match_arms;
 
@@ -139,4 +140,19 @@ mod tests {
             "#,
         );
     }
+
+    #[test]
+    fn fill_match_arms_target() {
+        check_assist_target(
+            fill_match_arms,
+            r#"
+            enum E { X, Y}
+
+            fn main() {
+                match E::X<|> {}
+            }
+            "#,
+            "match E::X {}",
+        );
+    }
 }
diff --git a/crates/ra_assists/src/flip_comma.rs b/crates/ra_assists/src/flip_comma.rs
index a49820c29..33da58f17 100644
--- a/crates/ra_assists/src/flip_comma.rs
+++ b/crates/ra_assists/src/flip_comma.rs
@@ -11,6 +11,7 @@ pub(crate) fn flip_comma(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
     let prev = non_trivia_sibling(comma, Direction::Prev)?;
     let next = non_trivia_sibling(comma, Direction::Next)?;
     ctx.build("flip comma", |edit| {
+        edit.target(comma.range());
         edit.replace(prev.range(), next.text());
         edit.replace(next.range(), prev.text());
     })
@@ -20,7 +21,7 @@ pub(crate) fn flip_comma(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
 mod tests {
     use super::*;
 
-    use crate::helpers::check_assist;
+    use crate::helpers::{check_assist, check_assist_target};
 
     #[test]
     fn flip_comma_works_for_function_parameters() {
@@ -30,4 +31,9 @@ mod tests {
             "fn foo(y: Result<(), ()>,<|> x: i32) {}",
         )
     }
+
+    #[test]
+    fn flip_comma_target() {
+        check_assist_target(flip_comma, "fn foo(x: i32,<|> y: Result<(), ()>) {}", ",")
+    }
 }
diff --git a/crates/ra_assists/src/introduce_variable.rs b/crates/ra_assists/src/introduce_variable.rs
index 4f7c9f3c2..934d1d6b3 100644
--- a/crates/ra_assists/src/introduce_variable.rs
+++ b/crates/ra_assists/src/introduce_variable.rs
@@ -45,6 +45,7 @@ pub(crate) fn introduce_variable(ctx: AssistCtx<impl HirDatabase>) -> Option<Ass
         } else {
             buf.push_str(";");
             indent.text().push_to(&mut buf);
+            edit.target(expr.syntax().range());
             edit.replace(expr.syntax().range(), "var_name".to_string());
             edit.insert(anchor_stmt.range().start(), buf);
             if wrap_in_block {
@@ -58,7 +59,7 @@ pub(crate) fn introduce_variable(ctx: AssistCtx<impl HirDatabase>) -> Option<Ass
 fn valid_covering_node(node: &SyntaxNode) -> bool {
     node.kind() != COMMENT
 }
-/// Check wether the node is a valid expression which can be extracted to a variable.
+/// Check whether the node is a valid expression which can be extracted to a variable.
 /// In general that's true for any expression, but in some cases that would produce invalid code.
 fn valid_target_expr(node: &SyntaxNode) -> Option<&ast::Expr> {
     match node.kind() {
@@ -74,7 +75,7 @@ fn valid_target_expr(node: &SyntaxNode) -> Option<&ast::Expr> {
 /// and a boolean indicating whether we have to wrap it within a { } block
 /// to produce correct code.
 /// It can be a statement, the last in a block expression or a wanna be block
-/// expression like a lamba or match arm.
+/// expression like a lambda or match arm.
 fn anchor_stmt(expr: &ast::Expr) -> Option<(&SyntaxNode, bool)> {
     expr.syntax().ancestors().find_map(|node| {
         if ast::Stmt::cast(node).is_some() {
@@ -100,7 +101,7 @@ fn anchor_stmt(expr: &ast::Expr) -> Option<(&SyntaxNode, bool)> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::helpers::{check_assist, check_assist_not_applicable, check_assist_range};
+    use crate::helpers::{check_assist, check_assist_not_applicable, check_assist_range, check_assist_target, check_assist_range_target};
 
     #[test]
     fn test_introduce_var_simple() {
@@ -425,4 +426,32 @@ fn main() {
 ",
         );
     }
+
+    // FIXME: This is not quite correct, but good enough(tm) for the sorting heuristic
+    #[test]
+    fn introduce_var_target() {
+        check_assist_target(
+            introduce_variable,
+            "
+fn foo() -> u32 {
+    r<|>eturn 2 + 2;
+}
+",
+            "2 + 2",
+        );
+
+        check_assist_range_target(
+            introduce_variable,
+            "
+fn main() {
+    let x = true;
+    let tuple = match x {
+        true => (<|>2 + 2<|>, true)
+        _ => (0, false)
+    };
+}
+",
+            "2 + 2",
+        );
+    }
 }
diff --git a/crates/ra_assists/src/lib.rs b/crates/ra_assists/src/lib.rs
index fc4e95303..2590faca9 100644
--- a/crates/ra_assists/src/lib.rs
+++ b/crates/ra_assists/src/lib.rs
@@ -65,7 +65,7 @@ where
                 Assist::Unresolved(..) => unreachable!(),
             })
             .collect::<Vec<(AssistLabel, AssistAction)>>();
-        a.sort_unstable_by(|a, b| match a {
+        a.sort_by(|a, b| match a {
             // Some(y) < Some(x) < None for y < x
             (_, AssistAction { target: Some(a), .. }) => match b {
                 (_, AssistAction { target: Some(b), .. }) => a.len().cmp(&b.len()),
@@ -163,6 +163,45 @@ mod helpers {
         assert_eq_text!(after, &actual);
     }
 
+    pub(crate) fn check_assist_target(
+        assist: fn(AssistCtx<MockDatabase>) -> Option<Assist>,
+        before: &str,
+        target: &str,
+    ) {
+        let (before_cursor_pos, before) = extract_offset(before);
+        let (db, _source_root, file_id) = MockDatabase::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, assist).expect("code action is not applicable");
+        let action = match assist {
+            Assist::Unresolved(_) => unreachable!(),
+            Assist::Resolved(_, it) => it,
+        };
+
+        let range = action.target.expect("expected target on action");
+        assert_eq_text!(&before[range.start().to_usize()..range.end().to_usize()], target);
+    }
+
+    pub(crate) fn check_assist_range_target(
+        assist: fn(AssistCtx<MockDatabase>) -> Option<Assist>,
+        before: &str,
+        target: &str,
+    ) {
+        let (range, before) = extract_range(before);
+        let (db, _source_root, file_id) = MockDatabase::with_single_file(&before);
+        let frange = FileRange { file_id, range };
+        let assist =
+            AssistCtx::with_ctx(&db, frange, true, assist).expect("code action is not applicable");
+        let action = match assist {
+            Assist::Unresolved(_) => unreachable!(),
+            Assist::Resolved(_, it) => it,
+        };
+
+        let range = action.target.expect("expected target on action");
+        assert_eq_text!(&before[range.start().to_usize()..range.end().to_usize()], target);
+    }
+
     pub(crate) fn check_assist_not_applicable(
         assist: fn(AssistCtx<MockDatabase>) -> Option<Assist>,
         before: &str,
@@ -181,10 +220,10 @@ mod tests {
     use hir::mock::MockDatabase;
     use ra_syntax::TextRange;
     use ra_db::FileRange;
-    use test_utils::extract_offset;
+    use test_utils::{extract_offset, extract_range};
 
     #[test]
-    fn assist_order() {
+    fn assist_order_field_struct() {
         let before = "struct Foo { <|>bar: u32 }";
         let (before_cursor_pos, before) = extract_offset(before);
         let (db, _source_root, file_id) = MockDatabase::with_single_file(&before);
@@ -197,4 +236,25 @@ mod tests {
         assert_eq!(assists.next().expect("expected assist").0.label, "add `#[derive]`");
     }
 
+    #[test]
+    fn assist_order_if_expr() {
+        let before = "
+        pub fn test_some_range(a: int) -> bool {
+            if let 2..6 = 5<|> {
+                true
+            } else {
+                false
+            }
+        }";
+        let (before_cursor_pos, before) = extract_offset(before);
+        let (db, _source_root, file_id) = MockDatabase::with_single_file(&before);
+        let frange =
+            FileRange { file_id, range: TextRange::offset_len(before_cursor_pos, 0.into()) };
+        let assists = super::assists(&db, frange);
+        let mut assists = assists.iter();
+
+        assert_eq!(assists.next().expect("expected assist").0.label, "introduce variable");
+        assert_eq!(assists.next().expect("expected assist").0.label, "replace with match");
+    }
+
 }
diff --git a/crates/ra_assists/src/remove_dbg.rs b/crates/ra_assists/src/remove_dbg.rs
index 40f97a849..e9d0a635b 100644
--- a/crates/ra_assists/src/remove_dbg.rs
+++ b/crates/ra_assists/src/remove_dbg.rs
@@ -47,6 +47,7 @@ pub(crate) fn remove_dbg(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
     };
 
     ctx.build("remove dbg!()", |edit| {
+        edit.target(macro_call.syntax().range());
         edit.replace(macro_range, macro_content);
         edit.set_cursor(cursor_pos);
     })
@@ -78,7 +79,7 @@ fn is_valid_macrocall(macro_call: &ast::MacroCall, macro_name: &str) -> Option<b
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::helpers::{check_assist, check_assist_not_applicable};
+    use crate::helpers::{check_assist, check_assist_not_applicable, check_assist_target};
 
     #[test]
     fn test_remove_dbg() {
@@ -120,4 +121,19 @@ fn foo(n: usize) {
         check_assist_not_applicable(remove_dbg, "<|>dbg(5, 6, 7)");
         check_assist_not_applicable(remove_dbg, "<|>dbg!(5, 6, 7");
     }
+
+    #[test]
+    fn remove_dbg_target() {
+        check_assist_target(
+            remove_dbg,
+            "
+fn foo(n: usize) {
+    if let Some(_) = dbg!(n.<|>checked_sub(4)) {
+        // ...
+    }
+}
+",
+            "dbg!(n.checked_sub(4))",
+        );
+    }
 }
diff --git a/crates/ra_assists/src/replace_if_let_with_match.rs b/crates/ra_assists/src/replace_if_let_with_match.rs
index 683f0d119..a22ec5584 100644
--- a/crates/ra_assists/src/replace_if_let_with_match.rs
+++ b/crates/ra_assists/src/replace_if_let_with_match.rs
@@ -17,6 +17,7 @@ pub(crate) fn replace_if_let_with_match(ctx: AssistCtx<impl HirDatabase>) -> Opt
 
     ctx.build("replace with match", |edit| {
         let match_expr = build_match_expr(expr, pat, then_block, else_block);
+        edit.target(if_expr.syntax().range());
         edit.replace_node_and_indent(if_expr.syntax(), match_expr);
         edit.set_cursor(if_expr.syntax().range().start())
     })
@@ -46,7 +47,7 @@ fn format_arm(block: &ast::Block) -> String {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::helpers::check_assist;
+    use crate::helpers::{check_assist, check_assist_target};
 
     #[test]
     fn test_replace_if_let_with_match_unwraps_simple_expressions() {
@@ -73,4 +74,26 @@ impl VariantData {
 }           ",
         )
     }
+
+    #[test]
+    fn replace_if_let_with_match_target() {
+        check_assist_target(
+            replace_if_let_with_match,
+            "
+impl VariantData {
+    pub fn is_struct(&self) -> bool {
+        if <|>let VariantData::Struct(..) = *self {
+            true
+        } else {
+            false
+        }
+    }
+}           ",
+            "if let VariantData::Struct(..) = *self {
+            true
+        } else {
+            false
+        }",
+        );
+    }
 }
diff --git a/crates/ra_assists/src/split_import.rs b/crates/ra_assists/src/split_import.rs
index 287c05830..051bc6fec 100644
--- a/crates/ra_assists/src/split_import.rs
+++ b/crates/ra_assists/src/split_import.rs
@@ -34,7 +34,7 @@ pub(crate) fn split_import(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::helpers::check_assist;
+    use crate::helpers::{check_assist, check_assist_target};
 
     #[test]
     fn test_split_import() {
@@ -53,4 +53,9 @@ mod tests {
             "use algo::{<|>visitor::{Visitor, visit}}",
         )
     }
+
+    #[test]
+    fn split_import_target() {
+        check_assist_target(split_import, "use algo::<|>visitor::{Visitor, visit}", "::");
+    }
 }