Add implementation of extract struct from enum variant #4576

mcrakhman · 2020-05-22T20:34:49Z

Hi guys! I implemented the extraction functionality including modifying multiple files. The only thing I didn't change the cursor position. I've done it with a previous API, but now snippets have been introduced and I need to figure out how to do it.

Please bear in mind that I am a newcomer in the rust-analyzer (and also Rust) world, so I tried to implement the feature to the best of my knowledge, but the API is very new to me, so I am very welcome to introducing changes etc.

crates/ra_assists/src/assist_context.rs

kjeremy · 2020-05-22T21:01:33Z

cargo xtask format is your friend :)

crates/ra_assists/Cargo.toml

crates/ra_assists/src/handlers/extract_struct_from_enum_variant.rs

crates/ra_assists/src/assist_context.rs

…readability

lnicola · 2020-05-24T10:54:10Z

crates/ra_assists/src/handlers/extract_struct_from_enum_variant.rs

+
+{}"#,
+        visibility_string,
+        variant_name,


This is actually a field_list, right?

No, field list is below, I decompose variant into field list and variant name and in field list I add pub modifiers

crates/ra_assists/src/handlers/extract_struct_from_enum_variant.rs

mcrakhman · 2020-05-24T13:11:12Z

@matklad Hi! Is it ok to go or should I change something/implement differently? Thanks!

matklad · 2020-05-27T15:56:32Z

Ok, I finally got to looking into this!

I like it! It's also the first transformation which is done properly, by collecting usages and fixing them up.

Though, this awesomeness is exactly what makes me take a pause here... At the moment, we eagerly compute all the assists. That means, that, if you just place the cursor on an enum, we'll kick the search, which I feel might be bad from performance and stability perspective ("find usages" tend to surface latent bugs really quick).

So, I have two proposals here:

We scale this down to do only the local transformation, without find_usages (which would be a shame)
We scale this up, and add ability to compute assists lazily (ie, computing the actual edit only when the user applies the assist).

The second option (which is more interesting) shouldnt' be too hard, but it'll be fiddly, as it requires some client-side tweaks as well. Basically, we'll need to add resolveCodeAction protocol extension and then make VS Code use this request to compute an actual edit.

matklad · 2020-05-27T15:57:42Z

and add ability to compute assists lazily

To be less confusing, the lazy-computation is already fully implemented internally, it's just not threaded through to the client, as LSP currently lacks this ability (which is a shame).

kjeremy · 2020-05-27T16:46:08Z

We scale this up, and add ability to compute assists lazily (ie, computing the actual edit only when the user applies the assist).

The second option (which is more interesting) shouldnt' be too hard, but it'll be fiddly, as it requires some client-side tweaks as well. Basically, we'll need to add resolveCodeAction protocol extension and then make VS Code use this request to compute an actual edit.

There should definitely be a codeAction/resolve request and I am kind of surprised there isn't. Here's the upstream issue for that: microsoft/language-server-protocol#787

We could do this lazily though by using a Command as well.

mcrakhman · 2020-05-27T21:59:00Z

and add ability to compute assists lazily

To be less confusing, the lazy-computation is already fully implemented internally, it's just not threaded through to the client, as LSP currently lacks this ability (which is a shame).

It definitely makes sense to do it lazily. Though just to be sure want to clarify the approach.

As far as I understand we need enable the author of each respective assists to decide whether he/she wants to provide an edit right away or (if it is expensive) to issue a command resolveCodeAction. Right now all assists are either resolved or unresolved.

Now we would probably need to change that, so the variable resolve would be removed from Assists, i.e. some assists could be resolved and some unresolved. Also we would need to add a separate handler like lsp_ext::ResolveCodeActionRequest to resolve unresolved assists.

Is my understanding correct or am I missing something?

matklad · 2020-05-28T07:47:03Z

As far as I understand we need enable the author of each respective assists to decide

I guess this is theoretically best solution, but I think it makes sense to start with (and 70% sure that it also makes sense to end with) just making all assists lazy unconditionally.

mcrakhman · 2020-05-30T13:42:51Z

As far as I understand we need enable the author of each respective assists to decide

I guess this is theoretically best solution, but I think it makes sense to start with (and 70% sure that it also makes sense to end with) just making all assists lazy unconditionally.

Hi! Created a first version of the changes, not sure if had done it the right way though. So what I did is I created an experimental feature resolveCodeAction which will compute assists lazily if enabled.

Then each time we get the response for codeAction we don't expect any commands or edits (we assert this). From one point it can be good to still provide for more flexibility if we decide to change the server (i.e. accept not only empty codeActions but also ones with commands and edits), on the other hand this will require additional effort (though probably not very big). I am ok to implement that if you think that is needed.

Also right now we are calling resolveCodeAction with label which should work, because probably labels have to be unique, otherwise it would be confusing for the end user, but still id comparison would probably be better.

mcrakhman · 2020-06-02T09:08:33Z

@matklad Friendly ping :-) There are a lot of changes and I am afraid that if this would pile up, I would have to merge/rebase again. Thanks!

matklad · 2020-06-02T09:12:09Z

Yeah, sorry, we are in the process of organizing the review process :) I'll look into this later today, but the main thing is that I think it makes sense to split protocol changes into a separate PR.

mcrakhman · 2020-06-02T09:36:50Z

Yeah, sorry, we are in the process of organizing the review process :) I'll look into this later today, but the main thing is that I think it makes sense to split protocol changes into a separate PR.

Cool, if it is ok, after your review, I will make a separate PR and also will address the changes you think are necessary.

matklad · 2020-06-02T11:09:33Z

cc microsoft/language-server-protocol#1008 (comment) for the general discussion of the "resolve" pattern LSP should have had :-)

mcrakhman · 2020-06-02T11:40:44Z

cc microsoft/language-server-protocol#1008 (comment) for the general discussion of the "resolve" pattern LSP should have had :-)

It seems that I've done this accordingly, the only thing which is different and should be changed is that we need to provide id of assist and not label to resolveCodeAction and the id should come in the payload of codeAction response.

matklad

Yup, protocol changes look good to me, let's split them into a separate PR though.

I haven't looked into the actual assist though, at the moment, I am slighlty worried with AsName reexport, we shouldn't be doing that (which is not really explained anywhere yet....)

matklad · 2020-06-02T10:44:18Z

crates/rust-analyzer/src/config.rs

+
+            let resolve_code_action =
+                experimental.get("resolveCodeAction").and_then(|it| it.as_bool()) == Some(true);
+            self.client_caps.resolve_code_action = resolve_code_action


Suggested change

self.client_caps.resolve_code_action = resolve_code_action

self.client_caps.resolve_code_action = resolve_code_action;

matklad · 2020-06-02T11:40:03Z

crates/ra_ide/src/lib.rs

@@ -147,7 +147,7 @@ pub struct Assist {
    pub id: AssistId,
    pub label: String,
    pub group_label: Option<String>,
-    pub source_change: SourceChange,
+    pub source_change: Option<SourceChange>,


I think it's better to split SourceChange out of this struct completely. Than, I think we should be able to just pub use ra_assists::Assist instead of defining a mirrored struct.

matklad · 2020-06-02T11:41:09Z

crates/ra_ide/src/lib.rs

        self.with_db(|db| {
            ra_assists::Assist::resolved(db, config, frange)
                .into_iter()
                .map(|assist| Assist {
                    id: assist.assist.id,
                    label: assist.assist.label,
                    group_label: assist.assist.group.map(|it| it.0),
-                    source_change: assist.source_change,
+                    source_change: Some(assist.source_change),
                })
                .collect()
        })


The signatures of these two methods should be -> Cancelable<Vec<Assist>> and -> Cancelable<Vec<ResolvedAssist>>

matklad · 2020-06-02T11:44:19Z

crates/rust-analyzer/src/lsp_ext.rs

+#[serde(rename_all = "camelCase")]
+pub struct ResolveCodeActionParams {
+    pub code_action_params: lsp_types::CodeActionParams,
+    pub label: String,


Here, we should use id and not the label. The motivation for this is that (long term) we should be able to select the correct assist by id, without checking even availability.

Ie, label is a custom text which might be influenced by code at the cursor, while ID is known statically.

There might be a sligthly complication in that, with group assists, I think there might be two assits with the same ID (ie, two imports for different types). In this case, I think we should keep id as a string, but concat an :<number> on the LSP layer for diambiguation.

matklad · 2020-06-02T11:45:09Z

crates/rust-analyzer/src/lsp_ext.rs

@@ -98,6 +98,22 @@ pub struct JoinLinesParams {
    pub ranges: Vec<Range>,
 }

+pub enum ResolveCodeActionRequest {}
+
+impl Request for ResolveCodeActionRequest {


Could you also add docs for this to https://github.com/rust-analyzer/rust-analyzer/blob/21132a7a748a70351f5d1ae6d8c51a3a0065013e/docs/dev/lsp-extensions.md?

matklad · 2020-06-02T11:48:33Z

Also cc https://github.com/rust-analyzer/rust-analyzer/pull/4703/files about the general review process. This PR is actually a great case study, because it hits this "expanding API" case.

mcrakhman · 2020-06-02T19:37:57Z

@matklad In the process of making the PR I encountered several questions subject to discussion:

What shall we do with diagnostics? Shall we leave it as the only unresolved CodeAction that will produce actual edits for now? It seems like this would be the easiest solution and probably computing them should not be so expensive, provided that everything else is computed lazily.
Shall we in resolveCodeAction call all the assists or only the specific one? The latter should be better and probably would not require much effort. We can change the resolve from bool to Option<AssistId> or something like that.
You are right that for groups we cannot just use the AssistId, we also need to add the indicator which will indicate the option in the group for example. Though it can be hard to determine the ordering of the variants as in theory they can come in arbitrary order from the handler.
On a second thought this probably shouldn't be the case as we may want to guarantee that in case of group assists they came in exactly same order, but still it may be the case that after we apply changes the source code is different and then we would have different groups, so if we apply any numbering after we get the groups on lets say handler level, we may execute an incorrect assist group.
But may be it make sense to use id and label so we can identify the Assist? Because it seems in any given position there will not be two assists with the same label and id. Otherwise it would not make much sense UX-wise.
The other option would be to give an option to each handler to provide additional data as separate parameter along with id (e.g. string, hash etc), so we can later identify the correct result.
What do you think? Or you had something else in mind?

mcrakhman · 2020-06-03T18:58:33Z

@matklad Seems also finished this one, can we merge now, or there is still some things to be checked? Thanks!

mcrakhman · 2020-06-04T08:12:38Z

Yup, protocol changes look good to me, let's split them into a separate PR though.

I haven't looked into the actual assist though, at the moment, I am slighlty worried with AsName reexport, we shouldn't be doing that (which is not really explained anywhere yet....)

Removed AsName, so now we have two methods, one working with ModPath, the other with String, though not sure if we need two methods, because we convert to string right at the beginning of insert_use_statement so we can as well change its signature to work with strings.

crates/ra_assists/src/handlers/extract_struct_from_enum_variant.rs

mcrakhman · 2020-06-05T15:49:38Z

@matklad Comments resolved :-)

mcrakhman · 2020-06-08T13:18:55Z

@matklad Comments resolved :-)

Hi! Just wondering if any more work needed here?

matklad · 2020-06-08T16:28:32Z

bors r+

Seems like a good start!

I think we also wan to extend this to handle named structs. And it seems like the whole "search & modify" infrastracture can become, well, and infrastructure somewhere in ra_ide_db, but it's unclear how exactly it should look.

bors · 2020-06-08T16:36:02Z

Build succeeded:

Add preliminary implementation of extract struct from enum variant

5cd4eb6

mcrakhman mentioned this pull request May 22, 2020

Assist idea: extract enum variant to struct #4468

Closed

mcrakhman added 2 commits May 22, 2020 22:43

Formatting and remove unused imports

04a3578

Refactor AssistDirector

97ffe3c

kjeremy reviewed May 22, 2020

View reviewed changes

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

kjeremy reviewed May 22, 2020

View reviewed changes

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

More formatting

ef1aaeb

mcrakhman added 2 commits May 23, 2020 01:23

Better naming and fix some review comments

fce1020

Use default instead of new in AssistDirector

4984520

bnjjj reviewed May 22, 2020

View reviewed changes

Further review fixes

6ee1c60

mcrakhman changed the title ~~[WIP] Add implementation of extract struct from enum variant~~ Add implementation of extract struct from enum variant May 22, 2020

lnicola reviewed May 23, 2020

View reviewed changes

crates/ra_assists/src/assist_context.rs Show resolved Hide resolved

mcrakhman added 2 commits May 23, 2020 11:53

Remove unwraps where possible

3a244e0

Remove unnecessary set_file and change variable positions for better …

e2974ba

…readability

lnicola reviewed May 24, 2020

View reviewed changes

crates/ra_assists/src/handlers/extract_struct_from_enum_variant.rs Outdated Show resolved Hide resolved

lnicola reviewed May 24, 2020

View reviewed changes

crates/ra_assists/src/handlers/extract_struct_from_enum_variant.rs Outdated Show resolved Hide resolved

Further refactoring under review comments

08aa8e1

matklad mentioned this pull request May 28, 2020

Support for 'codeAction/resolve' microsoft/language-server-protocol#787

Closed

matklad reviewed Jun 2, 2020

View reviewed changes

mcrakhman mentioned this pull request Jun 3, 2020

Implementation of lazy assits #4717

Merged

mcrakhman added 3 commits June 3, 2020 20:10

Fix incorrect behaviour if not resolved

a9d5675

Merge branch 'master' into assists_extract_enum

eefa10b

Fixed tests

a6d3c77

mcrakhman force-pushed the assists_extract_enum branch from f15708c to a6d3c77 Compare June 3, 2020 18:44

Remove AsName import

b0c8a2b

matklad reviewed Jun 5, 2020

View reviewed changes

crates/ra_assists/src/handlers/extract_struct_from_enum_variant.rs Outdated Show resolved Hide resolved

Remove unnecessary return

74c3e7a

matklad reviewed Jun 5, 2020

View reviewed changes

crates/ra_assists/src/handlers/extract_struct_from_enum_variant.rs Outdated Show resolved Hide resolved

matklad reviewed Jun 5, 2020

View reviewed changes

crates/ra_assists/src/handlers/extract_struct_from_enum_variant.rs Outdated Show resolved Hide resolved

Fix review comments

5dda995

bors bot merged commit 3a7c218 into rust-lang:master Jun 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add implementation of extract struct from enum variant #4576

Add implementation of extract struct from enum variant #4576

mcrakhman commented May 22, 2020 •

edited

kjeremy commented May 22, 2020

lnicola May 24, 2020

mcrakhman May 24, 2020

mcrakhman commented May 24, 2020

matklad commented May 27, 2020

matklad commented May 27, 2020

kjeremy commented May 27, 2020

mcrakhman commented May 27, 2020

matklad commented May 28, 2020

mcrakhman commented May 30, 2020 •

edited

mcrakhman commented Jun 2, 2020

matklad commented Jun 2, 2020

mcrakhman commented Jun 2, 2020

matklad commented Jun 2, 2020

mcrakhman commented Jun 2, 2020

matklad left a comment

matklad Jun 2, 2020

matklad Jun 2, 2020

matklad Jun 2, 2020

matklad Jun 2, 2020

matklad Jun 2, 2020

matklad commented Jun 2, 2020

mcrakhman commented Jun 2, 2020 •

edited

mcrakhman commented Jun 3, 2020

mcrakhman commented Jun 4, 2020

mcrakhman commented Jun 5, 2020

mcrakhman commented Jun 8, 2020

matklad commented Jun 8, 2020

bors bot commented Jun 8, 2020

	self.client_caps.resolve_code_action = resolve_code_action
	self.client_caps.resolve_code_action = resolve_code_action;

Add implementation of extract struct from enum variant #4576

Add implementation of extract struct from enum variant #4576

Conversation

mcrakhman commented May 22, 2020 • edited

kjeremy commented May 22, 2020

lnicola May 24, 2020

Choose a reason for hiding this comment

mcrakhman May 24, 2020

Choose a reason for hiding this comment

mcrakhman commented May 24, 2020

matklad commented May 27, 2020

matklad commented May 27, 2020

kjeremy commented May 27, 2020

mcrakhman commented May 27, 2020

matklad commented May 28, 2020

mcrakhman commented May 30, 2020 • edited

mcrakhman commented Jun 2, 2020

matklad commented Jun 2, 2020

mcrakhman commented Jun 2, 2020

matklad commented Jun 2, 2020

mcrakhman commented Jun 2, 2020

matklad left a comment

Choose a reason for hiding this comment

matklad Jun 2, 2020

Choose a reason for hiding this comment

matklad Jun 2, 2020

Choose a reason for hiding this comment

matklad Jun 2, 2020

Choose a reason for hiding this comment

matklad Jun 2, 2020

Choose a reason for hiding this comment

matklad Jun 2, 2020

Choose a reason for hiding this comment

matklad commented Jun 2, 2020

mcrakhman commented Jun 2, 2020 • edited

mcrakhman commented Jun 3, 2020

mcrakhman commented Jun 4, 2020

mcrakhman commented Jun 5, 2020

mcrakhman commented Jun 8, 2020

matklad commented Jun 8, 2020

bors bot commented Jun 8, 2020

mcrakhman commented May 22, 2020 •

edited

mcrakhman commented May 30, 2020 •

edited

mcrakhman commented Jun 2, 2020 •

edited