Apply the api design suggestions

10 files changed, 97 insertions, 59 deletions

diff --git a/Cargo.lock b/Cargo.lock
index a0444e97a..2e7698b59 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1067,6 +1067,7 @@ version = "0.1.0"
 dependencies = [
  "crossbeam-channel 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "env_logger 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "itertools 0.8.2 (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)",
  "lsp-server 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)",
diff --git a/crates/ra_assists/src/assist_ctx.rs b/crates/ra_assists/src/assist_ctx.rs
index 879216a36..e13f969c7 100644
--- a/crates/ra_assists/src/assist_ctx.rs
+++ b/crates/ra_assists/src/assist_ctx.rs
@@ -1,5 +1,6 @@
 //! This module defines `AssistCtx` -- the API surface that is exposed to assists.
 use hir::{db::HirDatabase, InFile, SourceAnalyzer};
+use itertools::Either;
 use ra_db::FileRange;
 use ra_fmt::{leading_indent, reindent};
 use ra_syntax::{
@@ -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, alternative_actions: Vec<AssistAction> },
+    Resolved { assist: ResolvedAssist },
 }
 
 /// `AssistCtx` allows to apply an assist or check if it could be applied.
@@ -92,7 +93,7 @@ impl<'a, DB: HirDatabase> AssistCtx<'a, DB> {
                 f(&mut edit);
                 edit.build()
             };
-            Assist::Resolved { label, action, alternative_actions: Vec::default() }
+            Assist::Resolved { assist: ResolvedAssist { label, action_data: Either::Left(action) } }
         } else {
             Assist::Unresolved { label }
         };
@@ -105,18 +106,20 @@ impl<'a, DB: HirDatabase> AssistCtx<'a, DB> {
         self,
         id: AssistId,
         label: impl Into<String>,
-        f: impl FnOnce() -> (ActionBuilder, Vec<ActionBuilder>),
+        f: impl FnOnce() -> Vec<ActionBuilder>,
     ) -> Option<Assist> {
         let label = AssistLabel { label: label.into(), id };
         let assist = if self.should_compute_edit {
-            let (action, alternative_actions) = f();
+            let actions = f();
+            assert!(!actions.is_empty(), "Assist cannot have no");
+
             Assist::Resolved {
-                label,
-                action: action.build(),
-                alternative_actions: alternative_actions
-                    .into_iter()
-                    .map(ActionBuilder::build)
-                    .collect(),
+                assist: ResolvedAssist {
+                    label,
+                    action_data: Either::Right(
+                        actions.into_iter().map(ActionBuilder::build).collect(),
+                    ),
+                },
             }
         } else {
             Assist::Unresolved { label }
diff --git a/crates/ra_assists/src/doc_tests.rs b/crates/ra_assists/src/doc_tests.rs
index 21bee228d..5dc1ee233 100644
--- a/crates/ra_assists/src/doc_tests.rs
+++ b/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);
 }
diff --git a/crates/ra_assists/src/lib.rs b/crates/ra_assists/src/lib.rs
index 95a530821..a2983ae87 100644
--- a/crates/ra_assists/src/lib.rs
+++ b/crates/ra_assists/src/lib.rs
@@ -14,6 +14,7 @@ mod test_db;
 pub mod ast_transform;
 
 use hir::db::HirDatabase;
+use itertools::Either;
 use ra_db::FileRange;
 use ra_syntax::{TextRange, TextUnit};
 use ra_text_edit::TextEdit;
@@ -41,6 +42,21 @@ pub struct AssistAction {
     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
@@ -65,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, Vec<AssistAction>)>
+pub fn assists<H>(db: &H, range: FileRange) -> Vec<ResolvedAssist>
 where
     H: HirDatabase + 'static,
 {
@@ -76,13 +92,11 @@ where
             .iter()
             .filter_map(|f| f(ctx.clone()))
             .map(|a| match a {
-                Assist::Resolved { label, action, alternative_actions } => {
-                    (label, action, alternative_actions)
-                }
+                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,
@@ -177,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);
@@ -204,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);
@@ -227,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");
@@ -246,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");
@@ -296,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]
@@ -318,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").0.label, "Replace with match");
+        assert_eq!(assists.next().expect("expected assist").label.label, "Extract into variable");
+        assert_eq!(assists.next().expect("expected assist").label.label, "Replace with match");
     }
 }
diff --git a/crates/ra_ide/src/assists.rs b/crates/ra_ide/src/assists.rs
index db6e4e8b7..e30eee5f4 100644
--- a/crates/ra_ide/src/assists.rs
+++ b/crates/ra_ide/src/assists.rs
@@ -4,30 +4,37 @@ use ra_db::{FilePosition, FileRange};
 
 use crate::{db::RootDatabase, FileId, SourceChange, SourceFileEdit};
 
+use itertools::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 alternative_changes: Vec<SourceChange>,
+    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(|(assist_label, action, alternative_actions)| {
+        .map(|assist| {
             let file_id = frange.file_id;
+            let assist_label = &assist.label;
             Assist {
                 id: assist_label.id,
                 label: assist_label.label.clone(),
-                change: action_to_edit(action, file_id, &assist_label),
-                alternative_changes: alternative_actions
-                    .into_iter()
-                    .map(|action| action_to_edit(action, file_id, &assist_label))
-                    .collect(),
+                change_data: match assist.action_data {
+                    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()
diff --git a/crates/ra_lsp_server/Cargo.toml b/crates/ra_lsp_server/Cargo.toml
index c08e67b8e..73bce8b08 100644
--- a/crates/ra_lsp_server/Cargo.toml
+++ b/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" }
+itertools = "0.8"
 
 [dev-dependencies]
 tempfile = "3"
diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs
index ec3c0a557..d76639474 100644
--- a/crates/ra_lsp_server/src/main_loop/handlers.rs
+++ b/crates/ra_lsp_server/src/main_loop/handlers.rs
@@ -3,6 +3,7 @@
 
 use std::{fmt::Write as _, io::Write as _};
 
+use itertools::Either;
 use lsp_server::ErrorCode;
 use lsp_types::{
     CallHierarchyIncomingCall, CallHierarchyIncomingCallsParams, CallHierarchyItem,
@@ -698,18 +699,25 @@ pub fn handle_code_action(
 
     for assist in world.analysis().assists(FileRange { file_id, range })?.into_iter() {
         let title = assist.label.clone();
-        let edit = assist.change.try_conv_with(&world)?;
-        let alternative_edits = assist
-            .alternative_changes
-            .into_iter()
-            .map(|change| change.try_conv_with(&world))
-            .collect::<Result<Vec<_>>>()?;
 
-        let command = Command {
-            title,
-            command: "rust-analyzer.applySourceChange".to_string(),
-            arguments: Some(vec![to_value(edit).unwrap(), to_value(alternative_edits).unwrap()]),
+        let command = match assist.change_data {
+            Either::Left(change) => Command {
+                title,
+                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 {
diff --git a/editors/code/src/commands/index.ts b/editors/code/src/commands/index.ts
index 0ff708b1f..dc075aa82 100644
--- a/editors/code/src/commands/index.ts
+++ b/editors/code/src/commands/index.ts
@@ -34,8 +34,20 @@ function showReferences(ctx: Ctx): Cmd {
 }
 
 function applySourceChange(ctx: Ctx): Cmd {
-    return async (change: sourceChange.SourceChange, alternativeChanges: sourceChange.SourceChange[] | undefined) => {
-        sourceChange.applySourceChange(ctx, change, alternativeChanges);
+    return async (change: sourceChange.SourceChange) => {
+        sourceChange.applySourceChange(ctx, change);
+    };
+}
+
+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);
+        }
     };
 }
 
@@ -59,5 +71,6 @@ export {
     runSingle,
     showReferences,
     applySourceChange,
+    selectAndApplySourceChange,
     reload
 };
diff --git a/editors/code/src/main.ts b/editors/code/src/main.ts
index 430ad31b4..0494ccf63 100644
--- a/editors/code/src/main.ts
+++ b/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);
diff --git a/editors/code/src/source_change.ts b/editors/code/src/source_change.ts
index b19d325d5..a336269ba 100644
--- a/editors/code/src/source_change.ts
+++ b/editors/code/src/source_change.ts
@@ -9,7 +9,7 @@ export interface SourceChange {
     cursorPosition?: lc.TextDocumentPositionParams;
 }
 
-async function applySelectedSourceChange(ctx: Ctx, change: SourceChange) {
+export async function applySourceChange(ctx: Ctx, change: SourceChange) {
     const client = ctx.client;
     if (!client) return;
 
@@ -55,13 +55,3 @@ async function applySelectedSourceChange(ctx: Ctx, change: SourceChange) {
         );
     }
 }
-
-export async function applySourceChange(ctx: Ctx, change: SourceChange, alternativeChanges: SourceChange[] | undefined) {
-    if (alternativeChanges !== undefined && alternativeChanges.length > 0) {
-        const selectedChange = await vscode.window.showQuickPick([change, ...alternativeChanges]);
-        if (!selectedChange) return;
-        await applySelectedSourceChange(ctx, selectedChange);
-    } else {
-        await applySelectedSourceChange(ctx, change);
-    }
-}