feat: support associated values in "Generate Enum Variant" assist #12837

DorianListens · 2022-07-20T18:22:46Z

This change adds support for associated values to the "Generate Enum Variant" assist.

I've split the implementation out into 4 steps to make code review easier:

Add "add_variant" support to the structural ast editing system in edit_in_place
Migrate generate_enum_variant to use structural ast editing instead of string manipulation
Support tuple fields
Support record fields

Please let me know if I should leave the commits as-is, or squash before merging.

DorianListens · 2022-07-27T16:43:16Z

Question: should I leave this as a "fix", or is it more of a "feat"? I can definitely see the argument either way, and am currently leaning towards changing this to a "feat".

Also, for the changelog, here are before and after examples:

Before:

Screen.Recording.2022-07-27.at.11.37.35.AM.mov

After:

Screen.Recording.2022-07-27.at.11.40.40.AM.mov

Veykril · 2022-07-27T18:05:40Z

Well, it adds to a feature oppposed to just a bug fix, so tagging this as one sounds good to me (We aren't too strict about the tagging really)

DorianListens · 2022-07-28T20:21:21Z

@fasterthanlime @maartenflippo Looks like y'all did the initial implementation for this assist, nice work! Pinging you here just in case you've got code review thoughts.

Veykril · 2022-07-28T20:23:15Z

I'll try to get back to this tomorrow (I sometimes put off reviewing assist PRs as they are usually more work to review :) )

DorianListens · 2022-07-28T20:25:16Z

I'll try to get back to this tomorrow (I sometimes put off reviewing assist PRs as they are usually more work to review :) )

No worries at all! I recognize I've mostly been opening assist related PRs, while the steering issue says that most of the focus right now is on completions. Hope I haven't caused you too much pain from context switching!

Veykril · 2022-07-28T20:26:57Z

Oh don't worry about the steering issue. It doesn't dictate what we work on, it's more a guide of what would be good to focus on :)

fasterthanlime · 2022-07-28T20:58:05Z

Looks like y'all did the initial implementation for this assist, nice work! Pinging you here just in case you've got code review thoughts.

No thoughts here, really excited about this feature though! (it's definitely a feature in my mind)

Veykril · 2022-08-02T13:56:00Z

crates/ide-assists/src/handlers/generate_enum_variant.rs

-    let path_expr: ast::PathExpr = ctx.find_node_at_offset()?;
-    let path = path_expr.path()?;


This should check if the parent of the path is a PathExpr, RecordExpr, PathPat, RecordPat or UseTree (or whatever node of a use tree has leaf paths, don't remember). In other locations paths cannot be enum variants so we shouldn't trigger there (unless I missed them, but a PathType for example can't)

Okay, that makes sense. The previous version only looked for PathExpr, which is too strict, but I can see how looking for Path by itself is not strict enough.

I'll see if I can come up with a PathType example that shouldn't ever work and add a test case for it.

Okay, I've done this now.

Veykril · 2022-08-02T13:59:59Z

crates/ide-assists/src/handlers/generate_enum_variant.rs

+fn make_field_list(ctx: &AssistContext<'_>, path: &ast::Path) -> Option<ast::FieldList> {
+    let scope = ctx.sema.scope(&path.syntax())?;
+    if let Some(call_expr) =
+        path.syntax().parent().and_then(|it| it.parent()).and_then(ast::CallExpr::cast)
+    {
+        make_tuple_field_list(call_expr, ctx, &scope)
+    } else if let Some(record_expr) = path.syntax().parent().and_then(ast::RecordExpr::cast) {
+        make_record_field_list(record_expr, ctx, &scope)
+    } else {
+        None
+    }
+}


With the checks from my previous comment, you could maybe even have a classification enum for the parent that is being used here afterwards like

enum PathClass { RecordExpr(ast::RecordExpr), RecordPat(ast::RecordPat), ... }

Also as you can see, this assist could apply to more than just expressions. But I think that might be better for a followup PR

Good point, I hadn't really considered trying this in pattern position, but I suppose it could work, like in a match arm condition or something. 🤯

I introduced this enum (I called it PathParent), and looked into supporting RecordPat and TupleStructPat, but I agree that those are better suited to an additional follow on change.

Veykril · 2022-08-02T14:01:10Z

Looks good to me aside from the missing check.
@bors delegate+

bors · 2022-08-02T14:01:13Z

✌️ @DorianListens can now approve this pull request

PathType path parents don't support this assist

DorianListens · 2022-08-02T19:49:56Z

@bors r=@Veykril

bors · 2022-08-02T19:49:57Z

📌 Commit 1980c11 has been approved by Veykril

It is now in the queue for this repository.

bors · 2022-08-02T19:50:04Z

⌛ Testing commit 1980c11 with merge 1c03f45...

bors · 2022-08-02T20:01:19Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 1c03f45 to master...

DorianListens force-pushed the dscheidt/generate-enum-data branch from db069f2 to 592d7ca Compare July 21, 2022 06:04

DorianListens changed the title ~~fix: support associated data in "Generate Enum Variant" assist~~ feat: support associated data in "Generate Enum Variant" assist Jul 27, 2022

DorianListens force-pushed the dscheidt/generate-enum-data branch from 592d7ca to 292a133 Compare July 27, 2022 18:33

DorianListens changed the title ~~feat: support associated data in "Generate Enum Variant" assist~~ feat: support associated values in "Generate Enum Variant" assist Jul 27, 2022

DorianListens force-pushed the dscheidt/generate-enum-data branch from 292a133 to b38ece5 Compare July 28, 2022 06:29

DorianListens force-pushed the dscheidt/generate-enum-data branch 2 times, most recently from b3cae38 to f76f2ea Compare July 30, 2022 03:51

Veykril reviewed Aug 2, 2022

View reviewed changes

DorianListens force-pushed the dscheidt/generate-enum-data branch 2 times, most recently from 1076a98 to d9c45ac Compare August 2, 2022 16:50

DorianListens added 6 commits August 2, 2022 14:37

Support adding variants via structural editing

405dd77

convert generate_enum_variant to use add_variant

bea1fec

Support tuple fields in generate_enum_variant

15d8049

Support record fields in generate_enum_variant

e4638de

Be more strict about supported Paths in generate_enum_variant

111694d

PathType path parents don't support this assist

Support PathPat paths in generate_enum_variant

1980c11

DorianListens force-pushed the dscheidt/generate-enum-data branch from d9c45ac to 1980c11 Compare August 2, 2022 18:37

bors merged commit 1c03f45 into rust-lang:master Aug 2, 2022

		let path_expr: ast::PathExpr = ctx.find_node_at_offset()?;
		let path = path_expr.path()?;

feat: support associated values in "Generate Enum Variant" assist #12837

feat: support associated values in "Generate Enum Variant" assist #12837

Uh oh!

Conversation

DorianListens commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DorianListens commented Jul 27, 2022

Uh oh!

Veykril commented Jul 27, 2022

Uh oh!

DorianListens commented Jul 28, 2022

Uh oh!

Veykril commented Jul 28, 2022

Uh oh!

DorianListens commented Jul 28, 2022

Uh oh!

Veykril commented Jul 28, 2022

Uh oh!

fasterthanlime commented Jul 28, 2022

Uh oh!

Veykril Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

DorianListens Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

DorianListens Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

Veykril Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

DorianListens Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

DorianListens Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

Veykril commented Aug 2, 2022

Uh oh!

bors commented Aug 2, 2022

Uh oh!

DorianListens commented Aug 2, 2022

Uh oh!

bors commented Aug 2, 2022

Uh oh!

bors commented Aug 2, 2022

Uh oh!

bors commented Aug 2, 2022

Uh oh!

Uh oh!

DorianListens commented Jul 20, 2022 •

edited

Loading