Allow assists with multiple selectable actions #2716

SomeoneToIgnore · 2019-12-31T20:40:41Z

This PR prepares an infra for #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:

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

crates/ra_assists/src/doc_tests.rs

SomeoneToIgnore · 2019-12-31T20:51:42Z

I had to use #[allow(dead_code)] twice for methods that I'm using in the auto import assist branch to add an assist with multiple actions.

I wanted to include those metods in this PR nevertheless since they are more related to this change and showcase the API for the assist with multiple actions.

crates/ra_assists/src/lib.rs

SomeoneToIgnore · 2019-12-31T21:03:17Z

cargo test passes for me locally on two different machines and the CI does not show the logs currently, so I have no idea what's wrong.

lnicola · 2019-12-31T22:55:22Z

The CI logs load for me, there are two failing heavy tests (test_missing_module_code_action). I think those are only run on CI.

SomeoneToIgnore · 2019-12-31T23:03:40Z

TIL we have heavy tests, thank you.
I've found the way to download the logs and fix the issue.

kiljacken · 2019-12-31T23:32:10Z

For future reference you can enable the slow test by setting RUN_SLOW_TESTS=1 in environment variables :)

flodiebold · 2020-01-01T01:50:34Z

If I understand it correctly, this change would break other existing clients like the emacs integration, until they're adapted. Maybe there's a way of extending the protocol in a compatible way?

SomeoneToIgnore · 2020-01-01T06:16:00Z

Good point. What are the compatibility guarantees RA promises to keep currently?
I have thought that there are none, since it's an experimental tool built from sources.

There are obviously ways to keep the compatibility, we can either create a new command or fiddle with the parameters of an existing one, for instance.
Just not sure if it's worth it.

What if we fix the emacs client instead?
We should pass a list as a parameter now, I think it might be a one-liner fix somewhere around here:
https://github.com/rust-analyzer/rust-analyzer/blob/master/editors/emacs/ra-emacs-lsp.el#L41

matklad

Left high-level design advice!

matklad · 2020-01-01T09:30:06Z

crates/ra_assists/src/assist_ctx.rs

@@ -77,20 +77,40 @@ impl<'a, DB: HirDatabase> AssistCtx<'a, DB> {
        f(ctx)
    }

-    pub(crate) fn add_assist(
+    pub(crate) fn add_assist_with_single_action(


Assists with a single action will always be a majority. For this reason, it makes sense to organize the API such that it builds a single-action assist by default, and you need extra steps to add more actions.

That means that, ideally, existing assists and tests should not not see any changes at tall, as the existing API is already optimized for single action use-case.

So, I suggest leaving add_assist / check_assist APIs as is, and instead adding a separate add_assist_group API.

Evaluating this direction further, since currently this is the only way of grouping that works for sure.

What if we add an extra Vec<AssistAction> field to the Assist struct, something like:

pub(crate) enum Assist { Unresolved { label: AssistLabel }, Resolved { label: AssistLabel, action: AssistAction, alternative_actions: Vec<AssistAction> }, }

and modify the protocol part correspondingly:

export async function applySourceChange(ctx: Ctx, change: SourceChange, alternative_changes: SourceChange[] | undefined)

?

That way the backward compatibility is preserved (checked by adjusting the protocol part only on a fresh master) and we enforce at least one assist presence on a compile time.

editors/code/src/commands/index.ts

editors/code/src/source_change.ts

matklad · 2020-01-01T10:06:08Z

crates/ra_assists/src/lib.rs

@@ -63,7 +64,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<(AssistLabel, Vec<AssistAction>)>


I thin the return type should look like this:

pub fn assists<H>(db: &H, range: FileRange) -> ( Vec<WithLabel< Either< AssistId, Vec<WithLabel<AssistId>> > >>, FxHashMap<AssistId, AssistAction> ) { ... } struct WithLabel<T> { label: String, value: T, }

but we probably should introduce some named types to avoid >>>>>.

In other words, with lhs we should be able to draw a depth-one tree

do foo (id1) do baz (id2) with spam (id3) with eggs (id4) do quux (id5)

and with rhs we shoudl be able to map each id to an action!

I've tried this approach btw and it's not that straightforward, I think:

Currently AssistAction has an optional label that should be set in the builder, so we cannot really return Vec<WithLabel<AssistId>> currently.
Forcing the users to input the label every time means changing all current assists and duplicating the Assist label for a single AsssistAction.

There's no FxHashMap crate in ra_assists and it feels like we don't really need it that much: it looks like we should rather return Vec<Assist> where Assist will be something like

struct Assist { label: String, id: AssistId, action_data: Either<AssistAction, Vec<AssistAction>>, }

or leave it as it is currently (with alternative_actions).
Former appoach will also require some additional changes in tests again, since now they are relying on a single action to be present.

So it feels like maximum I can do here is to add some wrapper named struct to hold all data returned and a few methods to it.

matklad · 2020-01-01T10:32:42Z

What if we fix the emacs client instead?

I believe vim client(s) also support this extension. Really, we should "just" upstream our extensions for the assists into the protocol (I think the relevant disucssion is microsoft/language-server-protocol#724)

kjeremy · 2020-01-01T18:07:20Z

Why doesn't returning multiple items in the reply handle this? See: https://microsoft.github.io/language-server-protocol/specifications/specification-3-15/#textDocument_codeAction

SomeoneToIgnore · 2020-01-01T20:17:38Z

@kjeremy , do you mean returning multiple actions instead of one action with multiple edits?

I keep the auto import action case in my head and from its perspective my thoughts to go this paths are the following:

it's the same action semantically, auto import with multiple options, not many separate actions
the list of options for importing might be big for some cases and scrolling through it is not that pleasant (afaik, there are no ways to filter out the list of actions or jump to any of those via any keystrokes)

So, the current way is more like a convenient grouping, allowing the user to quickly select the needed action and then to think on the options provided by it.

Your proposal is great in a sense that it's easier to implement, another thing that concerns me there (besides the bullets above) is the AssistIds I need to come up with for those import actions.
Here's how it looks like:

Does anybody have a preference on which approach to go with?

matklad · 2020-01-01T21:01:19Z

I feel that we'll need assist groups eventually, so I'd love to add the required plumbing! But just returning, say, the first three candidates also seems fine!

kjeremy · 2020-01-01T21:58:02Z

@SomeoneToIgnore yes I think that's what I mean grouped by CodeActionKind (I think we can add our own too).

SomeoneToIgnore · 2020-01-01T22:44:48Z

I'd like to continue with grouping after all the time I've put into it, but have no idea how to achieve this with the way @kjeremy proposes.

I've added the custom code action name to the action creation and to the server capabilities and to the client capabilities, but still get the non-grouped actions listed.

Did I miss anything?

SomeoneToIgnore · 2020-01-02T00:07:08Z

I've implemented the #2716 (comment) idea of mine, no more breaking changes in the protocol, double checked that on master with client changes only.

The #2716 (comment) is not addressed yet, let's pick the final approach first.

matklad · 2020-01-08T15:06:19Z

editors/code/src/source_change.ts

@@ -55,3 +55,13 @@ export async function applySourceChange(ctx: Ctx, change: SourceChange) {
        );
    }
 }
+
+export async function applySourceChange(ctx: Ctx, change: SourceChange, alternativeChanges: SourceChange[] | undefined) {


It still seems to me that overloading this function is wrong. Depending on the arguments you pass to it, it can be either just apply some side-effect, or it can interactively ask user for some input. I'd rather avoid mixing interactive and non-interactive usages in the same function.

Yeah, the client side split is great, I like it.
The question is how to pass the data from a backend, that what I wanted to discuss in
#2716 (comment)

So far it seems that I'd better go with the option #2 from #2716 (comment)

I think you'll need to pick a different action name in conv_rs, but other than that, the current backend setup looks reasonable to me.

I can't say that I feel that it is unquestionably perfect, but I can't suggest any immediate obvious improvements :)

Ok, I've come up with the changes that hopefully make the overall thing better.

matklad

I like this now!

I'd love to try one other way to encode groups, but I'll do this after this PR is merged, as the idea is vague, and I don't want to block on it.

crates/ra_assists/src/lib.rs

matklad · 2020-01-15T16:36:57Z

r=me with itertools replaced with either

bors delegate+

bors · 2020-01-15T16:36:59Z

✌️ SomeoneToIgnore can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

SomeoneToIgnore · 2020-01-15T18:43:06Z

Thanks for spending the time on reviewing it!

bors r+

2716: Allow assists with multiple selectable actions r=SomeoneToIgnore a=SomeoneToIgnore This PR prepares an infra for #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>

bors · 2020-01-15T18:55:32Z

Build succeeded

Rust (macos-latest)
Rust (ubuntu-latest)
TypeScript

matklad · 2020-01-16T14:31:17Z

I'd love to try one other way to encode groups, but I'll do this after this PR is merged, as the idea is vague, and I don't want to block on it.

Actually, I'll hold up until the auto-import PR is submitted and merged, to avoid merge conflicts and to have a better example to work from

SomeoneToIgnore commented Dec 31, 2019

View reviewed changes

crates/ra_assists/src/doc_tests.rs Outdated Show resolved Hide resolved

SomeoneToIgnore force-pushed the action-with-multiple-changes branch from 540c1de to 3e352e3 Compare December 31, 2019 20:50

SomeoneToIgnore changed the title ~~Allow actions with multiple selectable edits~~ Allow assists with multiple selectable actions Dec 31, 2019

SomeoneToIgnore commented Dec 31, 2019

View reviewed changes

crates/ra_assists/src/lib.rs Outdated Show resolved Hide resolved

SomeoneToIgnore force-pushed the action-with-multiple-changes branch 3 times, most recently from 7c9719b to ebfd47f Compare December 31, 2019 23:22

matklad reviewed Jan 1, 2020

View reviewed changes

SomeoneToIgnore force-pushed the action-with-multiple-changes branch 2 times, most recently from 6192ef4 to 0cb9092 Compare January 2, 2020 00:04

matklad reviewed Jan 8, 2020

View reviewed changes

SomeoneToIgnore force-pushed the action-with-multiple-changes branch 3 times, most recently from d93f75e to 7c6a71b Compare January 12, 2020 22:02

SomeoneToIgnore requested a review from matklad January 13, 2020 20:27

SomeoneToIgnore mentioned this pull request Jan 14, 2020

Assign CodeAction Kind #2833

Closed

matklad reviewed Jan 15, 2020

View reviewed changes

crates/ra_assists/src/lib.rs Outdated Show resolved Hide resolved

SomeoneToIgnore added 4 commits January 15, 2020 20:16

Another attempt to add multiple edits

73dc8b6

Apply the api design suggestions

78a2125

itertools::Either -> either::Either

d51cf77

Reduce visibility

79b7740

SomeoneToIgnore force-pushed the action-with-multiple-changes branch from 7c6a71b to 79b7740 Compare January 15, 2020 18:21

bors bot merged commit 79b7740 into rust-lang:master Jan 15, 2020

SomeoneToIgnore deleted the action-with-multiple-changes branch January 15, 2020 18:55

SomeoneToIgnore mentioned this pull request Jan 17, 2020

Auto import #2180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow assists with multiple selectable actions #2716

Allow assists with multiple selectable actions #2716

SomeoneToIgnore commented Dec 31, 2019

SomeoneToIgnore commented Dec 31, 2019 •

edited

Loading

SomeoneToIgnore commented Dec 31, 2019

lnicola commented Dec 31, 2019 •

edited

Loading

SomeoneToIgnore commented Dec 31, 2019

kiljacken commented Dec 31, 2019

flodiebold commented Jan 1, 2020

SomeoneToIgnore commented Jan 1, 2020 •

edited

Loading

matklad left a comment

matklad Jan 1, 2020

SomeoneToIgnore Jan 1, 2020

matklad Jan 1, 2020

SomeoneToIgnore Jan 7, 2020

matklad commented Jan 1, 2020

kjeremy commented Jan 1, 2020

SomeoneToIgnore commented Jan 1, 2020

matklad commented Jan 1, 2020

kjeremy commented Jan 1, 2020

SomeoneToIgnore commented Jan 1, 2020 •

edited

Loading

SomeoneToIgnore commented Jan 2, 2020

matklad Jan 8, 2020

SomeoneToIgnore Jan 8, 2020

matklad Jan 8, 2020

SomeoneToIgnore Jan 11, 2020

matklad left a comment

matklad commented Jan 15, 2020

bors bot commented Jan 15, 2020

SomeoneToIgnore commented Jan 15, 2020

bors bot commented Jan 15, 2020

matklad commented Jan 16, 2020

Allow assists with multiple selectable actions #2716

Allow assists with multiple selectable actions #2716

Conversation

SomeoneToIgnore commented Dec 31, 2019

SomeoneToIgnore commented Dec 31, 2019 • edited Loading

SomeoneToIgnore commented Dec 31, 2019

lnicola commented Dec 31, 2019 • edited Loading

SomeoneToIgnore commented Dec 31, 2019

kiljacken commented Dec 31, 2019

flodiebold commented Jan 1, 2020

SomeoneToIgnore commented Jan 1, 2020 • edited Loading

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Jan 1, 2020

kjeremy commented Jan 1, 2020

SomeoneToIgnore commented Jan 1, 2020

matklad commented Jan 1, 2020

kjeremy commented Jan 1, 2020

SomeoneToIgnore commented Jan 1, 2020 • edited Loading

SomeoneToIgnore commented Jan 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad left a comment

Choose a reason for hiding this comment

matklad commented Jan 15, 2020

bors bot commented Jan 15, 2020

SomeoneToIgnore commented Jan 15, 2020

bors bot commented Jan 15, 2020

Build succeeded

matklad commented Jan 16, 2020

SomeoneToIgnore commented Dec 31, 2019 •

edited

Loading

lnicola commented Dec 31, 2019 •

edited

Loading

SomeoneToIgnore commented Jan 1, 2020 •

edited

Loading

SomeoneToIgnore commented Jan 1, 2020 •

edited

Loading