Merge #2716

2716: Allow assists with multiple selectable actions r=SomeoneToIgnore a=SomeoneToIgnore

This PR prepares an infra for https://github.com/rust-analyzer/rust-analyzer/issues/2180 task by adding a possibility to specify multiple actions in one assist as multiple edit parameters to the `applySourceChange` command.

When this is done, the command opens a selection dialog, allowing the user to pick the edit to be applied.

I have no working example to test in this PR, but here's a demo of an auto import feature (a separate PR coming later for that one) using this functionality:

![out](https://user-images.githubusercontent.com/2690773/71633614-f8ea4d80-2c1d-11ea-9b15-0e13611a7aa4.gif)

The PR is not that massive as it may seem: all the assist files' changes are very generic and similar.

Co-authored-by: Kirill Bulatov <mail4score@gmail.com>

Cargo.lock

@@ -869,8 +869,8 @@ version = "0.1.0"
 name = "ra_assists"
 version = "0.1.0"
 dependencies = [
+ "either 1.5.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "format-buf 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
- "itertools 0.8.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "join_to_string 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "ra_db 0.1.0",
  "ra_fmt 0.1.0",
@@ -1066,6 +1066,7 @@ name = "ra_lsp_server"
 version = "0.1.0"
 dependencies = [
  "crossbeam-channel 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "either 1.5.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "env_logger 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "jod-thread 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)",

crates/ra_assists/Cargo.toml

@@ -11,7 +11,7 @@ doctest = false
 format-buf = "1.0.0"
 join_to_string = "0.1.3"
 rustc-hash = "1.0"
-itertools = "0.8.0"
+either = "1.5"
 
 ra_syntax = { path = "../ra_syntax" }
 ra_text_edit = { path = "../ra_text_edit" }

crates/ra_assists/src/assist_ctx.rs

@@ -1,4 +1,5 @@
 //! This module defines `AssistCtx` -- the API surface that is exposed to assists.
+use either::Either;
 use hir::{db::HirDatabase, InFile, SourceAnalyzer};
 use ra_db::FileRange;
 use ra_fmt::{leading_indent, reindent};
@@ -9,12 +10,12 @@ use ra_syntax::{
 };
 use ra_text_edit::TextEditBuilder;
 
-use crate::{AssistAction, AssistId, AssistLabel};
+use crate::{AssistAction, AssistId, AssistLabel, ResolvedAssist};
 
 #[derive(Clone, Debug)]
 pub(crate) enum Assist {
     Unresolved { label: AssistLabel },
-    Resolved { label: AssistLabel, action: AssistAction },
+    Resolved { assist: ResolvedAssist },
 }
 
 /// `AssistCtx` allows to apply an assist or check if it could be applied.
@@ -81,18 +82,45 @@ impl<'a, DB: HirDatabase> AssistCtx<'a, DB> {
         self,
         id: AssistId,
         label: impl Into<String>,
-        f: impl FnOnce(&mut AssistBuilder),
+        f: impl FnOnce(&mut ActionBuilder),
     ) -> Option<Assist> {
         let label = AssistLabel { label: label.into(), id };
         assert!(label.label.chars().nth(0).unwrap().is_uppercase());
 
         let assist = if self.should_compute_edit {
             let action = {
-                let mut edit = AssistBuilder::default();
+                let mut edit = ActionBuilder::default();
                 f(&mut edit);
                 edit.build()
             };
-            Assist::Resolved { label, action }
+            Assist::Resolved { assist: ResolvedAssist { label, action_data: Either::Left(action) } }
+        } else {
+            Assist::Unresolved { label }
+        };
+
+        Some(assist)
+    }
+
+    #[allow(dead_code)] // will be used for auto import assist with multiple actions
+    pub(crate) fn add_assist_group(
+        self,
+        id: AssistId,
+        label: impl Into<String>,
+        f: impl FnOnce() -> Vec<ActionBuilder>,
+    ) -> Option<Assist> {
+        let label = AssistLabel { label: label.into(), id };
+        let assist = if self.should_compute_edit {
+            let actions = f();
+            assert!(!actions.is_empty(), "Assist cannot have no");
+
+            Assist::Resolved {
+                assist: ResolvedAssist {
+                    label,
+                    action_data: Either::Right(
+                        actions.into_iter().map(ActionBuilder::build).collect(),
+                    ),
+                },
+            }
         } else {
             Assist::Unresolved { label }
         };
@@ -128,13 +156,20 @@ impl<'a, DB: HirDatabase> AssistCtx<'a, DB> {
 }
 
 #[derive(Default)]
-pub(crate) struct AssistBuilder {
+pub(crate) struct ActionBuilder {
     edit: TextEditBuilder,
     cursor_position: Option<TextUnit>,
     target: Option<TextRange>,
+    label: Option<String>,
 }
 
-impl AssistBuilder {
+impl ActionBuilder {
+    #[allow(dead_code)]
+    /// Adds a custom label to the action, if it needs to be different from the assist label
+    pub(crate) fn label(&mut self, label: impl Into<String>) {
+        self.label = Some(label.into())
+    }
+
     /// Replaces specified `range` of text with a given string.
     pub(crate) fn replace(&mut self, range: TextRange, replace_with: impl Into<String>) {
         self.edit.replace(range, replace_with.into())
@@ -193,6 +228,7 @@ impl AssistBuilder {
             edit: self.edit.finish(),
             cursor_position: self.cursor_position,
             target: self.target,
+            label: self.label,
         }
     }
 }

crates/ra_assists/src/assists/inline_local_variable.rs

@@ -4,7 +4,7 @@ use ra_syntax::{
     TextRange,
 };
 
-use crate::assist_ctx::AssistBuilder;
+use crate::assist_ctx::ActionBuilder;
 use crate::{Assist, AssistCtx, AssistId};
 
 // Assist: inline_local_variable
@@ -94,7 +94,7 @@ pub(crate) fn inline_local_varialbe(ctx: AssistCtx<impl HirDatabase>) -> Option<
     ctx.add_assist(
         AssistId("inline_local_variable"),
         "Inline variable",
-        move |edit: &mut AssistBuilder| {
+        move |edit: &mut ActionBuilder| {
             edit.delete(delete_range);
             for (desc, should_wrap) in refs.iter().zip(wrap_in_parens) {
                 if should_wrap {

crates/ra_assists/src/doc_tests.rs

@@ -15,21 +15,21 @@ fn check(assist_id: &str, before: &str, after: &str) {
     let (db, file_id) = TestDB::with_single_file(&before);
     let frange = FileRange { file_id, range: selection.into() };
 
-    let (_assist_id, action) = crate::assists(&db, frange)
+    let assist = crate::assists(&db, frange)
         .into_iter()
-        .find(|(id, _)| id.id.0 == assist_id)
+        .find(|assist| assist.label.id.0 == assist_id)
         .unwrap_or_else(|| {
             panic!(
                 "\n\nAssist is not applicable: {}\nAvailable assists: {}",
                 assist_id,
                 crate::assists(&db, frange)
                     .into_iter()
-                    .map(|(id, _)| id.id.0)
+                    .map(|assist| assist.label.id.0)
                     .collect::<Vec<_>>()
                     .join(", ")
             )
         });
 
-    let actual = action.edit.apply(&before);
+    let actual = assist.get_first_action().edit.apply(&before);
     assert_eq_text!(after, &actual);
 }

crates/ra_assists/src/lib.rs

@@ -13,6 +13,7 @@ mod doc_tests;
 mod test_db;
 pub mod ast_transform;
 
+use either::Either;
 use hir::db::HirDatabase;
 use ra_db::FileRange;
 use ra_syntax::{TextRange, TextUnit};
@@ -35,11 +36,27 @@ pub struct AssistLabel {
 
 #[derive(Debug, Clone)]
 pub struct AssistAction {
+    pub label: Option<String>,
     pub edit: TextEdit,
     pub cursor_position: Option<TextUnit>,
     pub target: Option<TextRange>,
 }
 
+#[derive(Debug, Clone)]
+pub struct ResolvedAssist {
+    pub label: AssistLabel,
+    pub action_data: Either<AssistAction, Vec<AssistAction>>,
+}
+
+impl ResolvedAssist {
+    pub fn get_first_action(&self) -> AssistAction {
+        match &self.action_data {
+            Either::Left(action) => action.clone(),
+            Either::Right(actions) => actions[0].clone(),
+        }
+    }
+}
+
 /// Return all the assists applicable at the given position.
 ///
 /// Assists are returned in the "unresolved" state, that is only labels are
@@ -64,7 +81,7 @@ where
 ///
 /// Assists are returned in the "resolved" state, that is with edit fully
 /// computed.
-pub fn assists<H>(db: &H, range: FileRange) -> Vec<(AssistLabel, AssistAction)>
+pub fn assists<H>(db: &H, range: FileRange) -> Vec<ResolvedAssist>
 where
     H: HirDatabase + 'static,
 {
@@ -75,11 +92,11 @@ where
             .iter()
             .filter_map(|f| f(ctx.clone()))
             .map(|a| match a {
-                Assist::Resolved { label, action } => (label, action),
+                Assist::Resolved { assist } => assist,
                 Assist::Unresolved { .. } => unreachable!(),
             })
             .collect::<Vec<_>>();
-        a.sort_by(|a, b| match (a.1.target, b.1.target) {
+        a.sort_by(|a, b| match (a.get_first_action().target, b.get_first_action().target) {
             (Some(a), Some(b)) => a.len().cmp(&b.len()),
             (Some(_), None) => Ordering::Less,
             (None, Some(_)) => Ordering::Greater,
@@ -174,7 +191,7 @@ mod helpers {
             AssistCtx::with_ctx(&db, frange, true, assist).expect("code action is not applicable");
         let action = match assist {
             Assist::Unresolved { .. } => unreachable!(),
-            Assist::Resolved { action, .. } => action,
+            Assist::Resolved { assist } => assist.get_first_action(),
         };
 
         let actual = action.edit.apply(&before);
@@ -201,7 +218,7 @@ mod helpers {
             AssistCtx::with_ctx(&db, frange, true, assist).expect("code action is not applicable");
         let action = match assist {
             Assist::Unresolved { .. } => unreachable!(),
-            Assist::Resolved { action, .. } => action,
+            Assist::Resolved { assist } => assist.get_first_action(),
         };
 
         let mut actual = action.edit.apply(&before);
@@ -224,7 +241,7 @@ mod helpers {
             AssistCtx::with_ctx(&db, frange, true, assist).expect("code action is not applicable");
         let action = match assist {
             Assist::Unresolved { .. } => unreachable!(),
-            Assist::Resolved { action, .. } => action,
+            Assist::Resolved { assist } => assist.get_first_action(),
         };
 
         let range = action.target.expect("expected target on action");
@@ -243,7 +260,7 @@ mod helpers {
             AssistCtx::with_ctx(&db, frange, true, assist).expect("code action is not applicable");
         let action = match assist {
             Assist::Unresolved { .. } => unreachable!(),
-            Assist::Resolved { action, .. } => action,
+            Assist::Resolved { assist } => assist.get_first_action(),
         };
 
         let range = action.target.expect("expected target on action");
@@ -293,10 +310,10 @@ mod tests {
         let mut assists = assists.iter();
 
         assert_eq!(
-            assists.next().expect("expected assist").0.label,
+            assists.next().expect("expected assist").label.label,
             "Change visibility to pub(crate)"
         );
-        assert_eq!(assists.next().expect("expected assist").0.label, "Add `#[derive]`");
+        assert_eq!(assists.next().expect("expected assist").label.label, "Add `#[derive]`");
     }
 
     #[test]
@@ -315,7 +332,7 @@ mod tests {
         let assists = super::assists(&db, frange);
         let mut assists = assists.iter();
 
-        assert_eq!(assists.next().expect("expected assist").0.label, "Extract into variable");
+        assert_eq!(assists.next().expect("expected assist").label.label, "Extract into variable");
-        assert_eq!(assists.next().expect("expected assist").0.label, "Replace with match");
+        assert_eq!(assists.next().expect("expected assist").label.label, "Replace with match");
     }
 }

crates/ra_ide/src/assists.rs

@@ -2,27 +2,53 @@
 
 use ra_db::{FilePosition, FileRange};
 
-use crate::{db::RootDatabase, SourceChange, SourceFileEdit};
+use crate::{db::RootDatabase, FileId, SourceChange, SourceFileEdit};
 
+use either::Either;
 pub use ra_assists::AssistId;
+use ra_assists::{AssistAction, AssistLabel};
 
 #[derive(Debug)]
 pub struct Assist {
     pub id: AssistId,
-    pub change: SourceChange,
+    pub label: String,
+    pub change_data: Either<SourceChange, Vec<SourceChange>>,
 }
 
 pub(crate) fn assists(db: &RootDatabase, frange: FileRange) -> Vec<Assist> {
     ra_assists::assists(db, frange)
         .into_iter()
-        .map(|(label, action)| {
+        .map(|assist| {
             let file_id = frange.file_id;
-            let file_edit = SourceFileEdit { file_id, edit: action.edit };
+            let assist_label = &assist.label;
-            let id = label.id;
+            Assist {
-            let change = SourceChange::source_file_edit(label.label, file_edit).with_cursor_opt(
+                id: assist_label.id,
-                action.cursor_position.map(|offset| FilePosition { offset, file_id }),
+                label: assist_label.label.clone(),
-            );
+                change_data: match assist.action_data {
-            Assist { id, change }
+                    Either::Left(action) => {
+                        Either::Left(action_to_edit(action, file_id, assist_label))
+                    }
+                    Either::Right(actions) => Either::Right(
+                        actions
+                            .into_iter()
+                            .map(|action| action_to_edit(action, file_id, assist_label))
+                            .collect(),
+                    ),
+                },
+            }
         })
         .collect()
 }
+
+fn action_to_edit(
+    action: AssistAction,
+    file_id: FileId,
+    assist_label: &AssistLabel,
+) -> SourceChange {
+    let file_edit = SourceFileEdit { file_id, edit: action.edit };
+    SourceChange::source_file_edit(
+        action.label.unwrap_or_else(|| assist_label.label.clone()),
+        file_edit,
+    )
+    .with_cursor_opt(action.cursor_position.map(|offset| FilePosition { offset, file_id }))
+}

crates/ra_lsp_server/Cargo.toml

@@ -28,6 +28,7 @@ ra_prof = { path = "../ra_prof" }
 ra_vfs_glob = { path = "../ra_vfs_glob" }
 env_logger = { version = "0.7.1", default-features = false, features = ["humantime"] }
 ra_cargo_watch = { path = "../ra_cargo_watch" }
+either = "1.5"
 
 [dev-dependencies]
 tempfile = "3"

crates/ra_lsp_server/src/main_loop/handlers.rs

@@ -3,6 +3,7 @@
 
 use std::{fmt::Write as _, io::Write as _};
 
+use either::Either;
 use lsp_server::ErrorCode;
 use lsp_types::{
     CallHierarchyIncomingCall, CallHierarchyIncomingCallsParams, CallHierarchyItem,
@@ -644,7 +645,6 @@ pub fn handle_code_action(
     let line_index = world.analysis().file_line_index(file_id)?;
     let range = params.range.conv_with(&line_index);
 
-    let assists = world.analysis().assists(FileRange { file_id, range })?.into_iter();
     let diagnostics = world.analysis().diagnostics(file_id)?;
     let mut res = CodeActionResponse::default();
 
@@ -697,15 +697,27 @@ pub fn handle_code_action(
         res.push(action.into());
     }
 
-    for assist in assists {
+    for assist in world.analysis().assists(FileRange { file_id, range })?.into_iter() {
-        let title = assist.change.label.clone();
+        let title = assist.label.clone();
-        let edit = assist.change.try_conv_with(&world)?;
 
-        let command = Command {
+        let command = match assist.change_data {
-            title,
+            Either::Left(change) => Command {
-            command: "rust-analyzer.applySourceChange".to_string(),
+                title,
-            arguments: Some(vec![to_value(edit).unwrap()]),
+                command: "rust-analyzer.applySourceChange".to_string(),
+                arguments: Some(vec![to_value(change.try_conv_with(&world)?)?]),
+            },
+            Either::Right(changes) => Command {
+                title,
+                command: "rust-analyzer.selectAndApplySourceChange".to_string(),
+                arguments: Some(vec![to_value(
+                    changes
+                        .into_iter()
+                        .map(|change| change.try_conv_with(&world))
+                        .collect::<Result<Vec<_>>>()?,
+                )?]),
+            },
         };
+
         let action = CodeAction {
             title: command.title.clone(),
             kind: match assist.id {

editors/code/src/commands/index.ts

@@ -39,6 +39,18 @@ function applySourceChange(ctx: Ctx): Cmd {
     };
 }
 
+function selectAndApplySourceChange(ctx: Ctx): Cmd {
+    return async (changes: sourceChange.SourceChange[]) => {
+        if (changes.length === 1) {
+            await sourceChange.applySourceChange(ctx, changes[0]);
+        } else if (changes.length > 0) {
+            const selectedChange = await vscode.window.showQuickPick(changes);
+            if (!selectedChange) return;
+            await sourceChange.applySourceChange(ctx, selectedChange);
+        }
+    };
+}
+
 function reload(ctx: Ctx): Cmd {
     return async () => {
         vscode.window.showInformationMessage('Reloading rust-analyzer...');
@@ -59,5 +71,6 @@ export {
     runSingle,
     showReferences,
     applySourceChange,
+    selectAndApplySourceChange,
     reload
 };

editors/code/src/main.ts

@@ -26,6 +26,7 @@ export async function activate(context: vscode.ExtensionContext) {
     ctx.registerCommand('runSingle', commands.runSingle);
     ctx.registerCommand('showReferences', commands.showReferences);
     ctx.registerCommand('applySourceChange', commands.applySourceChange);
+    ctx.registerCommand('selectAndApplySourceChange', commands.selectAndApplySourceChange);
 
     if (ctx.config.enableEnhancedTyping) {
         ctx.overrideCommand('type', commands.onEnter);