12 files changed, 264 insertions, 22 deletions

diff --git a/Cargo.lock b/Cargo.lock
index c330115b8..5ce4a8084 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1,3 +1,5 @@
+# This file is automatically @generated by Cargo.
+# It is not intended for manual editing.
 [[package]]
 name = "aho-corasick"
 version = "0.6.9"
diff --git a/crates/ra_assists/src/add_derive.rs b/crates/ra_assists/src/add_derive.rs
index caf21e079..ea9707631 100644
--- a/crates/ra_assists/src/add_derive.rs
+++ b/crates/ra_assists/src/add_derive.rs
@@ -24,6 +24,7 @@ pub(crate) fn add_derive(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
             }
             Some(tt) => tt.syntax().range().end() - TextUnit::of_char(')'),
         };
+        edit.target(nominal.syntax().range());
         edit.set_cursor(offset)
     })
 }
@@ -38,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() {
@@ -80,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/assist_ctx.rs b/crates/ra_assists/src/assist_ctx.rs
index 0bf640241..41c8ac2f6 100644
--- a/crates/ra_assists/src/assist_ctx.rs
+++ b/crates/ra_assists/src/assist_ctx.rs
@@ -16,7 +16,7 @@ pub(crate) enum Assist {
 
 /// `AssistCtx` allows to apply an assist or check if it could be applied.
 ///
-/// Assists use a somewhat overengineered approach, given the current needs. The
+/// Assists use a somewhat over-engineered approach, given the current needs. The
 /// assists workflow consists of two phases. In the first phase, a user asks for
 /// the list of available assists. In the second phase, the user picks a
 /// particular assist and it gets applied.
@@ -106,6 +106,7 @@ impl<'a, DB: HirDatabase> AssistCtx<'a, DB> {
 pub(crate) struct AssistBuilder {
     edit: TextEditBuilder,
     cursor_position: Option<TextUnit>,
+    target: Option<TextRange>,
 }
 
 impl AssistBuilder {
@@ -138,7 +139,15 @@ impl AssistBuilder {
         self.cursor_position = Some(offset)
     }
 
+    pub(crate) fn target(&mut self, target: TextRange) {
+        self.target = Some(target)
+    }
+
     fn build(self) -> AssistAction {
-        AssistAction { edit: self.edit.finish(), cursor_position: self.cursor_position }
+        AssistAction {
+            edit: self.edit.finish(),
+            cursor_position: self.cursor_position,
+            target: self.target,
+        }
     }
 }
diff --git a/crates/ra_assists/src/change_visibility.rs b/crates/ra_assists/src/change_visibility.rs
index fa5f231c8..6d9a4eec2 100644
--- a/crates/ra_assists/src/change_visibility.rs
+++ b/crates/ra_assists/src/change_visibility.rs
@@ -20,7 +20,7 @@ fn add_vis(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
         _ => false,
     });
 
-    let offset = if let Some(keyword) = item_keyword {
+    let (offset, target) = if let Some(keyword) = item_keyword {
         let parent = keyword.parent()?;
         let def_kws = vec![FN_DEF, MODULE, STRUCT_DEF, ENUM_DEF, TRAIT_DEF];
         // Parent is not a definition, can't add visibility
@@ -31,17 +31,18 @@ fn add_vis(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
         if parent.children().any(|child| child.kind() == VISIBILITY) {
             return None;
         }
-        vis_offset(parent)
+        (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())
+        (vis_offset(field.syntax()), ident.range())
     };
 
     ctx.build("make pub(crate)", |edit| {
+        edit.target(target);
         edit.insert(offset, "pub(crate) ");
         edit.set_cursor(offset);
     })
@@ -60,13 +61,15 @@ fn vis_offset(node: &SyntaxNode) -> TextUnit {
 
 fn change_vis(ctx: AssistCtx<impl HirDatabase>, vis: &ast::Visibility) -> Option<Assist> {
     if vis.syntax().text() == "pub" {
-        return ctx.build("chage to pub(crate)", |edit| {
+        return ctx.build("change to pub(crate)", |edit| {
+            edit.target(vis.syntax().range());
             edit.replace(vis.syntax().range(), "pub(crate)");
             edit.set_cursor(vis.syntax().range().start());
         });
     }
     if vis.syntax().text() == "pub(crate)" {
-        return ctx.build("chage to pub", |edit| {
+        return ctx.build("change to pub", |edit| {
+            edit.target(vis.syntax().range());
             edit.replace(vis.syntax().range(), "pub");
             edit.set_cursor(vis.syntax().range().start());
         });
@@ -77,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() {
@@ -135,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 881db6347..7928b4983 100644
--- a/crates/ra_assists/src/lib.rs
+++ b/crates/ra_assists/src/lib.rs
@@ -8,7 +8,7 @@
 mod assist_ctx;
 
 use ra_text_edit::TextEdit;
-use ra_syntax::{TextUnit, SyntaxNode, Direction};
+use ra_syntax::{TextRange, TextUnit, SyntaxNode, Direction};
 use ra_db::FileRange;
 use hir::db::HirDatabase;
 
@@ -23,6 +23,7 @@ pub struct AssistLabel {
 pub struct AssistAction {
     pub edit: TextEdit,
     pub cursor_position: Option<TextUnit>,
+    pub target: Option<TextRange>,
 }
 
 /// Return all the assists applicable at the given position.
@@ -53,15 +54,24 @@ pub fn assists<H>(db: &H, range: FileRange) -> Vec<(AssistLabel, AssistAction)>
 where
     H: HirDatabase + 'static,
 {
+    use std::cmp::Ordering;
+
     AssistCtx::with_ctx(db, range, true, |ctx| {
-        all_assists()
+        let mut a = all_assists()
             .iter()
             .filter_map(|f| f(ctx.clone()))
             .map(|a| match a {
                 Assist::Resolved(label, action) => (label, action),
                 Assist::Unresolved(..) => unreachable!(),
             })
-            .collect()
+            .collect::<Vec<(AssistLabel, AssistAction)>>();
+        a.sort_by(|a, b| match (a.1.target, b.1.target) {
+            (Some(a), Some(b)) => a.len().cmp(&b.len()),
+            (Some(_), None) => Ordering::Less,
+            (None, Some(_)) => Ordering::Greater,
+            (None, None) => Ordering::Equal,
+        });
+        a
     })
 }
 
@@ -97,7 +107,7 @@ mod helpers {
     use hir::mock::MockDatabase;
     use ra_syntax::TextRange;
     use ra_db::FileRange;
-    use test_utils::{extract_offset, assert_eq_text, add_cursor, extract_range};
+    use test_utils::{extract_offset, extract_range, assert_eq_text, add_cursor};
 
     use crate::{AssistCtx, Assist};
 
@@ -151,6 +161,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,
@@ -162,5 +211,48 @@ mod helpers {
         let assist = AssistCtx::with_ctx(&db, frange, true, assist);
         assert!(assist.is_none());
     }
+}
+
+#[cfg(test)]
+mod tests {
+    use hir::mock::MockDatabase;
+    use ra_syntax::TextRange;
+    use ra_db::FileRange;
+    use test_utils::{extract_offset};
+
+    #[test]
+    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);
+        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, "make pub(crate)");
+        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 fb69cef9c..051bc6fec 100644
--- a/crates/ra_assists/src/split_import.rs
+++ b/crates/ra_assists/src/split_import.rs
@@ -24,6 +24,7 @@ pub(crate) fn split_import(ctx: AssistCtx<impl HirDatabase>) -> Option<Assist> {
     };
 
     ctx.build("split import", |edit| {
+        edit.target(colon_colon.range());
         edit.insert(l_curly, "{");
         edit.insert(r_curly, "}");
         edit.set_cursor(l_curly + TextUnit::of_str("{"));
@@ -33,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() {
@@ -52,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}", "::");
+    }
 }