'Fill match arms' should work with existing match arms

[slyngbaek]

Addresses #3039

This essentially adds missing match arms. The algorithm for this
can get complicated rather quickly so bail in certain conditions
and rely on a PlaceholderPat.

The algorighm works as such:

• Iterate through the Enum Def Variants

• Attempt to see if the variant already exists as a match arm

• If yes, skip the enum variant. If no, include it.

• If it becomes complicated, rather than exhaustively deal with every
  branch, mark it as a "partial match" and simply include the
  placeholder.

  Conditions for "complication":

  • The match arm contains a match guard
  • Any kind of nested destrucuring

Order the resulting merged match branches as such:

1. Provided match arms
2. Missing enum variant branch arms
3. End with Placeholder if required

• Add extra tests

[SomeoneToIgnore]

Thank you, this is a nice step forward, I really like that now the asssist works on tuple pats for some cases (a bit weird that not for all of them, I've pointed out the cases below).

One funny thing that we might want to avoid is not to add an unreacheable pats like in this case:

Before:

fn main() {
    match Some((22, 33)) {
        None => (),
        Some(i) => (),
    }
}

After:

fn main() {
    match Some((22, 33)) {
        None => (),
        Some(i) => (),
        _ => (),
    }
}

Feels like the assist should not be proposed at all for the cases like this.

[matklad]

Status: we should remove _, and we should add a test that the assist does not trigger if all branches are already covered.

[matklad]

Yes! In general, "the code compiles" is not the primary goal of assists (unlike fixits for diagnostics, which should be correct in that sense)

…

On Wed, 18 Mar 2020 at 21:19, slyngbaek ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/ra_assists/src/handlers/fill_match_arms.rs <#3623 (comment)> : > + _ => vec![pat], + }; + + pats.iter().any(|pat| { + match does_arm_pat_match_variant(pat, arm.guard(), variant_pat) { + ArmMatch::Yes => true, + ArmMatch::No => false, + ArmMatch::Partial => { + has_partial_match = true; + true + } + } + }) + }) + }) + .map(|pat| make::match_arm(iter::once(pat), make::expr_unit())) So for now...just keep using () even though it might result in failed compilation? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3623 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M4AACMGH632KC6XXJTRIEUENANCNFSM4LNCIEGQ> .

[matklad]

bors r+

Thanks!

[matklad]

Ah, looks like this needs a rebase now

[matklad]

bors r+

[bors]

Build succeeded

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