Merge #7749

7749: Make more common assist easier to ues r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

Cargo.lock

@@ -58,6 +58,7 @@ name = "assists"
 version = "0.0.0"
 dependencies = [
  "either",
+ "expect-test",
  "hir",
  "ide_db",
  "itertools 0.10.0",

crates/assists/Cargo.toml

@@ -21,3 +21,6 @@ profile = { path = "../profile", version = "0.0.0" }
 ide_db = { path = "../ide_db", version = "0.0.0" }
 hir = { path = "../hir", version = "0.0.0" }
 test_utils = { path = "../test_utils", version = "0.0.0" }
+
+[dev-dependencies]
+expect-test = "1.1"

crates/assists/src/lib.rs

@@ -179,9 +179,7 @@ pub(crate) fn all() -> &'static [Handler] {
             early_return::convert_to_guarded_return,
             expand_glob_import::expand_glob_import,
             move_module_to_file::move_module_to_file,
-            extract_function::extract_function,
             extract_struct_from_enum_variant::extract_struct_from_enum_variant,
-            extract_variable::extract_variable,
             fill_match_arms::fill_match_arms,
             fix_visibility::fix_visibility,
             flip_binexpr::flip_binexpr,
@@ -229,12 +227,18 @@ pub(crate) fn all() -> &'static [Handler] {
             unmerge_use::unmerge_use,
             unwrap_block::unwrap_block,
             wrap_return_type_in_result::wrap_return_type_in_result,
-            // These are manually sorted for better priorities
+            // These are manually sorted for better priorities. By default,
+            // priority is determined by the size of the target range (smaller
+            // target wins). If the ranges are equal, position in this list is
+            // used as a tie-breaker.
             add_missing_impl_members::add_missing_impl_members,
             add_missing_impl_members::add_missing_default_members,
             //
             replace_string_with_char::replace_string_with_char,
             raw_string::make_raw_string,
+            //
+            extract_variable::extract_variable,
+            extract_function::extract_function,
             // Are you sure you want to add new assist here, and not to the
             // sorted list above?
         ]

crates/assists/src/tests.rs

@@ -1,5 +1,6 @@
 mod generated;
 
+use expect_test::expect;
 use hir::Semantics;
 use ide_db::{
     base_db::{fixture::WithFixture, FileId, FileRange, SourceDatabaseExt},
@@ -10,11 +11,11 @@
     source_change::FileSystemEdit,
     RootDatabase,
 };
+use stdx::{format_to, trim_indent};
 use syntax::TextRange;
-use test_utils::{assert_eq_text, extract_offset, extract_range};
+use test_utils::{assert_eq_text, extract_offset};
 
 use crate::{handlers::Handler, Assist, AssistConfig, AssistContext, AssistKind, Assists};
-use stdx::{format_to, trim_indent};
 
 pub(crate) const TEST_CONFIG: AssistConfig = AssistConfig {
     snippet_cap: SnippetCap::new(true),
@@ -163,6 +164,22 @@ fn check(handler: Handler, before: &str, expected: ExpectedResult, assist_label:
     };
 }
 
+fn labels(assists: &[Assist]) -> String {
+    let mut labels = assists
+        .iter()
+        .map(|assist| {
+            let mut label = match &assist.group {
+                Some(g) => g.0.clone(),
+                None => assist.label.to_string(),
+            };
+            label.push('\n');
+            label
+        })
+        .collect::<Vec<_>>();
+    labels.dedup();
+    labels.into_iter().collect::<String>()
+}
+
 #[test]
 fn assist_order_field_struct() {
     let before = "struct Foo { $0bar: u32 }";
@@ -181,66 +198,78 @@ fn assist_order_field_struct() {
 
 #[test]
 fn assist_order_if_expr() {
-    let before = "
-    pub fn test_some_range(a: int) -> bool {
-        if let 2..6 = $05$0 {
-            true
-        } else {
-            false
-        }
-    }";
-    let (range, before) = extract_range(before);
-    let (db, file_id) = with_single_file(&before);
-    let frange = FileRange { file_id, range };
-    let assists = Assist::get(&db, &TEST_CONFIG, false, frange);
-    let mut assists = assists.iter();
+    let (db, frange) = RootDatabase::with_range(
+        r#"
+pub fn test_some_range(a: int) -> bool {
+    if let 2..6 = $05$0 {
+        true
+    } else {
+        false
+    }
+}
+"#,
+    );
 
-    assert_eq!(assists.next().expect("expected assist").label, "Extract into function");
-    assert_eq!(assists.next().expect("expected assist").label, "Extract into variable");
-    assert_eq!(assists.next().expect("expected assist").label, "Replace with match");
+    let assists = Assist::get(&db, &TEST_CONFIG, false, frange);
+    let expected = labels(&assists);
+
+    expect![[r#"
+        Convert integer base
+        Extract into variable
+        Extract into function
+        Replace with match
+    "#]]
+    .assert_eq(&expected);
 }
 
 #[test]
 fn assist_filter_works() {
-    let before = "
-    pub fn test_some_range(a: int) -> bool {
-        if let 2..6 = $05$0 {
-            true
-        } else {
-            false
-        }
-    }";
-    let (range, before) = extract_range(before);
-    let (db, file_id) = with_single_file(&before);
-    let frange = FileRange { file_id, range };
-
+    let (db, frange) = RootDatabase::with_range(
+        r#"
+pub fn test_some_range(a: int) -> bool {
+    if let 2..6 = $05$0 {
+        true
+    } else {
+        false
+    }
+}
+"#,
+    );
     {
         let mut cfg = TEST_CONFIG;
         cfg.allowed = Some(vec![AssistKind::Refactor]);
 
         let assists = Assist::get(&db, &cfg, false, frange);
-        let mut assists = assists.iter();
+        let expected = labels(&assists);
 
-        assert_eq!(assists.next().expect("expected assist").label, "Extract into function");
-        assert_eq!(assists.next().expect("expected assist").label, "Extract into variable");
-        assert_eq!(assists.next().expect("expected assist").label, "Replace with match");
+        expect![[r#"
+            Convert integer base
+            Extract into variable
+            Extract into function
+            Replace with match
+        "#]]
+        .assert_eq(&expected);
     }
 
     {
         let mut cfg = TEST_CONFIG;
         cfg.allowed = Some(vec![AssistKind::RefactorExtract]);
         let assists = Assist::get(&db, &cfg, false, frange);
-        assert_eq!(assists.len(), 2);
+        let expected = labels(&assists);
 
-        let mut assists = assists.iter();
-        assert_eq!(assists.next().expect("expected assist").label, "Extract into function");
-        assert_eq!(assists.next().expect("expected assist").label, "Extract into variable");
+        expect![[r#"
+            Extract into variable
+            Extract into function
+        "#]]
+        .assert_eq(&expected);
     }
 
     {
         let mut cfg = TEST_CONFIG;
         cfg.allowed = Some(vec![AssistKind::QuickFix]);
         let assists = Assist::get(&db, &cfg, false, frange);
-        assert!(assists.is_empty(), "All asserts but quickfixes should be filtered out");
+        let expected = labels(&assists);
+
+        expect![[r#""#]].assert_eq(&expected);
     }
 }

crates/base_db/src/fixture.rs

@@ -67,7 +67,7 @@
 use vfs::{file_set::FileSet, VfsPath};
 
 use crate::{
-    input::CrateName, Change, CrateGraph, CrateId, Edition, Env, FileId, FilePosition,
+    input::CrateName, Change, CrateGraph, CrateId, Edition, Env, FileId, FilePosition, FileRange,
     SourceDatabaseExt, SourceRoot, SourceRootId,
 };
 
@@ -99,6 +99,15 @@ fn with_position(ra_fixture: &str) -> (Self, FilePosition) {
         (db, FilePosition { file_id, offset })
     }
 
+    fn with_range(ra_fixture: &str) -> (Self, FileRange) {
+        let (db, file_id, range_or_offset) = Self::with_range_or_offset(ra_fixture);
+        let range = match range_or_offset {
+            RangeOrOffset::Range(it) => it,
+            RangeOrOffset::Offset(_) => panic!(),
+        };
+        (db, FileRange { file_id, range })
+    }
+
     fn with_range_or_offset(ra_fixture: &str) -> (Self, FileId, RangeOrOffset) {
         let fixture = ChangeFixture::parse(ra_fixture);
         let mut db = Self::default();