implement fill match arm assist for tuple of enums by JoshMcguigan · Pull Request #3689 · rust-lang/rust-analyzer

JoshMcguigan · 2020-03-23T06:34:19Z

This updates the fill match arm assist to work in cases where the user is matching on a tuple of enums.

Note, for now this does not apply when some match arms exist (other than the trivial _), but I think this could be added in the future.

I think this also lays the groundwork for filling match arms when matching on tuples of non-enum values, for example a tuple of an enum and a boolean.

edwin0cheng · 2020-03-23T06:51:46Z

+            return None;
+        }
+
+        enum_defs


IMHO, since using multi_cartesian_product is quite convoluted, I think it will be nice to add a comment describing what is going on here.

I added a comment here. Let me know if you prefer I say something different.

SomeoneToIgnore

Awesome, I've noticed that I happen to match over tuples rather frequently, so this assist should be very handy, thank you.

I've left a few commments, those are mostly some minor things except for the panics (that would be really nice to address in the same PR).

On a more global note, it feels like we can generalize the whole approach (with some recursive match_ast! maybe) to avoid various else if resolve_... branches in the code doing more or less the same thing in the beginning.
But that's a separate thing, not required in this PR.

SomeoneToIgnore · 2020-03-23T08:06:04Z

 format-buf = "1.0.0"
 join_to_string = "0.1.3"
 rustc-hash = "1.1.0"
+itertools = "0.8.2"


There's a 0.9.0 version out in the wild, maybe we can use that, just to be up to date here at least.

It seems all the other crates in this repo use 0.8.2, so changing only this crate would mean compiling both versions. IMO it seems nice to use the same version of a dependency across all the crates in a single project like this, just to reduce mental overhead in cases where the API has actually changed.

I'm not sure how dependency upgrades are typically done in this project, so let me know if there is some existing standard practice, but would you mind if I pushed this to a follow-up PR?

SomeoneToIgnore · 2020-03-23T08:10:39Z

+                let patterns = variants
+                    .into_iter()
+                    .filter_map(|variant| build_pat(ctx.db, module, variant))
+                    .collect::<Vec<_>>();


.collect::<Vec<_>>()

Nit: You don't really need to collect anything here and can pass the iterator directly into the make::tuple_pat

Good catch, fixed.

SomeoneToIgnore · 2020-03-23T08:21:49Z

+            .iter()
+            .map(|ty| match ty.as_adt() {
+                Some(Adt::Enum(e)) => e,
+                _ => panic!("handle the case of tuple containing non-enum"),


I don't think panicking during assist construction just because we did not implement something is a good idea: first of all, it pollutes the logs with big traces that do not say anything meaningful besides that the developers did not cover all cases (and this might be frequent, since this construction also happens on hover).
Also, with this panic, calling the Quick fix shortcut will show endless progress bar in VS Code, which is a bit distracting :)

So I propose to simply match the only case here and maybe leave a comment about the rest of the cases.

Oops, I mean to go back and fix that. Fixed.

Note that you can use todo!() or a //TODO comment for such cases: CI would prevent mering code with todos into master.

SomeoneToIgnore · 2020-03-23T08:24:35Z

 }

+pub fn tuple_pat(pats: impl IntoIterator<Item = ast::Pat>) -> ast::TuplePat {
+    let pats_str = pats.into_iter().map(|p| p.syntax().to_string()).join(", ");


Nit: p.syntax().to_string() can be just p.to_string() due to the recent improvements in the ast API.

SomeoneToIgnore · 2020-03-23T08:34:51Z

        );
    }

+    #[test]


Currently, the assist panics on a single element tuple:

match (Some(22), ) { }

Might be a good idea to fix it :)

This one was trickier than I thought. The panic comes from make::tuple_pat. I didn't dig much into it, but I think it is because a single element tuple doesn't get parsed as a TuplePat.

For now I handle this case by marking the assist as non-applicable if tuple_len < 2. I hope that is acceptable for now.

SomeoneToIgnore · 2020-03-23T08:41:14Z

    })
 }

+fn resolve_tuple_of_enum_def(


resolve_enum_def contains autoderef invocation.
This helps to detect more enum-related cases and might be a cool thing to support in resolve_tuple_of_enum_def too, now or later in some other PR.

Currently this code

let ref_opt = &Some(22); match (ref_opt, ref_opt) { }

will invoke the panic!("handle the case of tuple containing non-enum"), line and fail since we don't recognise the enum behind the reference.

This all kind of hints that we might want to approach the resolution somehow more unified, but that is maybe an another task outside of this PR's scope.

Thanks for pointing this out. I've added the autoderef to resolve_tuple_of_enum_def, but I agree it does feel like there should be some way to reduce some of the duplication here.

matklad

Overall, looks good to me!

I think after this PR it makes sense to look into graduating this assist into a diagnostic, similar to how we've dealt with missing struct fields:

#1163

That is, we should implement proper exhaustiveness checking at the hir layer.

matklad · 2020-03-23T10:09:33Z

 use std::iter;

+use itertools::Itertools;
+


We generally treat all non-std external crates the same, so there's no need for a blank line here.

Veetaha · 2020-03-23T10:55:59Z

+                ast::Pat::from(make::tuple_pat(patterns))
+            })
+            .filter(|variant_pat| is_variant_missing(&mut arms, variant_pat))
+            .map(|pat| make::match_arm(iter::once(pat), make::expr_unit()))


This is not strictly related to this PR, but imho it would be better to put empty curly braces {} instead of a unit literal since the user almost all the time writes some block expression as a match arm (or a non-block expression which means removing the (), or {} anyway).

I did see this suggestion in #3687, but I'd prefer to handle that in a separate PR if you don't mind?

SomeoneToIgnore

Great, way fewer panics and even more functionality than before.
Thank you for the PR and welcome with the follow-ups.

JoshMcguigan · 2020-03-23T16:22:23Z

I believe I've cleaned up / fixed all review feedback. I'm not sure who typically presses the 'Resolve conversation` button.

Let me know if you'd like to see anything else before this merges.

lnicola · 2020-03-23T16:30:11Z

I'm not sure who typically presses the 'Resolve conversation` button.

GitHub does that when you change the code in a newer commit.

SomeoneToIgnore · 2020-03-23T16:32:06Z

GitHub does that when you change the code in a newer commit.

That only works for me when I apply the code suggestions.

I'm not sure who typically presses the 'Resolve conversation` button.

Afaik, it's visible only for you, here's my version of the interface, with no button at all:

But there's a limited number of persons who can write bors r+ and fire the final check & merge, that's what important.

matklad · 2020-03-23T16:41:29Z

bors r+

Thanks!

To reiterate, if you feel like this, moving this to hir (somewhere around here) as a proper exhaustive match checking, that would be awesome. This will probably require reading rustc source code to learn how to do this properly though.

bors · 2020-03-23T16:50:49Z

Build succeeded

JoshMcguigan · 2020-03-23T23:11:58Z

bors r+

Thanks!

To reiterate, if you feel like this, moving this to hir (somewhere around here) as a proper exhaustive match checking, that would be awesome. This will probably require reading rustc source code to learn how to do this properly though.

Thanks for the merge. I'll take a look at moving this to hir after tackling some of the low hanging fruit follow ups mentioned in this PR.

@SomeoneToIgnore

3697: update itertools version to 0.9.0 r=matklad a=JoshMcguigan Updating `itertools` version per [feedback on #3689](#3689 (comment)) from @SomeoneToIgnore. Worth noting that `chalk` still uses `itertools` v0.8.2, so perhaps it is worth a PR to update that repo as well so we don't have to build both versions? Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>

@Veetaha

3700: fill match arms with empty block rather than unit tuple r=matklad a=JoshMcguigan As requested by @Veetaha in #3689 and #3687, this modifies the fill match arms assist to create match arms as an empty block `{}` rather than a unit tuple `()`. In one test I left one of the pre-existing match arms as a unit tuple, and added a body to another match arm, to demonstrate that the contents of existing match arms persist. Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>

3706: missing match arms diagnostic r=flodiebold a=JoshMcguigan Following up on #3689 (comment), this PR creates a missing match arms diagnostic. At the moment this is a very early draft, but I wanted to open it just to get some initial feedback. Initial questions: * Have I roughly created the correct boilerplate? * Inside the new `validate_match` function: * Am I correct in thinking I want to do validation by comparing the match arms against `match_expr`? And when analyzing `match_expr` I should be looking at it as a `hir_def::expr::Expr`? * I mostly copied the chained if-let statements from the struct validation. Shouldn't there be a non-failable way to get an AstPtr from the hir data structures? Thanks for all the guidance. Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>

implement fill match arm assist for tuple of enums

2afccbe

edwin0cheng reviewed Mar 23, 2020

View reviewed changes

SomeoneToIgnore reviewed Mar 23, 2020

View reviewed changes

matklad reviewed Mar 23, 2020

View reviewed changes

Veetaha reviewed Mar 23, 2020

View reviewed changes

review comments

bc48c9d

SomeoneToIgnore approved these changes Mar 23, 2020

View reviewed changes

SomeoneToIgnore requested a review from matklad March 23, 2020 13:29

bors Bot merged commit eff1b3f into rust-lang:master Mar 23, 2020

JoshMcguigan mentioned this pull request Mar 23, 2020

update itertools version to 0.9.0 #3697

Merged

JoshMcguigan mentioned this pull request Mar 24, 2020

fill match arms with empty block rather than unit tuple #3700

Merged

JoshMcguigan mentioned this pull request Mar 24, 2020

missing match arms diagnostic #3706

Merged

JoshMcguigan mentioned this pull request Apr 18, 2020

Match check enum record #3894

Merged

                       );
                   }
+                  #[test]

Uh oh!

Uh oh!

Conversation

JoshMcguigan commented Mar 23, 2020

Uh oh!

edwin0cheng Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

Uh oh!

JoshMcguigan commented Mar 23, 2020

Uh oh!

lnicola commented Mar 23, 2020

Uh oh!

SomeoneToIgnore commented Mar 23, 2020

Uh oh!

matklad commented Mar 23, 2020

Uh oh!

bors Bot commented Mar 23, 2020

Build succeeded

Uh oh!

JoshMcguigan commented Mar 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

edwin0cheng Mar 23, 2020 •

edited

Loading