Merge #768

768: Sort assists by the range of the affected element r=matklad a=robojumper

Closes #763.

https://github.com/rust-analyzer/rust-analyzer/blob/3be98f2ac93b278828e76eb813bdd8033f647b12/crates/ra_assists/src/lib.rs#L233-L236

This could be made more robust by a) adding a way to identify actions by things other than their label and b) allowing arbitrary actions to appear in the list as long as the tested actions are there in the correct order. Let me know if I should do any of that.


Co-authored-by: robojumper <[email protected]>

11 files changed, 262 insertions, 22 deletions

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}", "::");
+    }
 }