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]>

Author: bors[bot]

Cargo.lock

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

crates/ra_assists/src/add_derive.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>> {}",
+        );
+    }
 }

crates/ra_assists/src/add_impl.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,
+        }
     }
 }

crates/ra_assists/src/assist_ctx.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");
+    }
 }

crates/ra_assists/src/change_visibility.rs

@@ -65,14 +65,15 @@ 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);
     })
 }
 
 #[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 {}",
+        );
+    }
 }

crates/ra_assists/src/fill_match_arms.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<(), ()>) {}", ",")
+    }
 }

crates/ra_assists/src/flip_comma.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",
+        );
+    }
 }