Be more specific on non-exhaustive matches on non-sum-types

[estebank]

When matching on non-sum types we tell people they should add a new fallback case

error[E0004]: non-exhaustive patterns: `(&_, _)` not covered
 --> src/main.rs:4:11
  |
4 |     match (a, b) {
  |           ^^^^^^ pattern `(&_, _)` not covered
  |
  = note: the matched value is of type `(&str, &str)`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
  |
6 ~         ("c", "d") => {}
7 +         (&_, _) => todo!()
  |

but we should go further and check whether the fallback is because of a free-form non-sum type (namely, not an enum) and explain that, for example, &str can hold any arbitrary string and match needs an arm to account for it.

simonw/advent-of-code-2022-in-rust#3 (comment)

[lyming2007]

When matching on non-sum types we tell people they should add a new fallback case

error[E0004]: non-exhaustive patterns: `(&_, _)` not covered
 --> src/main.rs:4:11
  |
4 |     match (a, b) {
  |           ^^^^^^ pattern `(&_, _)` not covered
  |
  = note: the matched value is of type `(&str, &str)`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
  |
6 ~         ("c", "d") => {}
7 +         (&_, _) => todo!()
  |

but we should go further and check whether the fallback is because of a free-form non-sum type (namely, not an enum) and explain that, for example, &str can hold any arbitrary string and match needs an arm to account for it.

simonw/advent-of-code-2022-in-rust#3 (comment)

So what ideally output would you suggest?

[estebank]

I'd want us to add some text about how the final match arm should cover every possible value of a (&str, &str).

[mattjperez]

@rustbot claim

[mattjperez]

Sorry, should have asked before claiming. @lyming2007 were you gonna work on this?

[mattjperez]

@estebank While implementing this to check for strings, both single strings and as members of a tuple, I noticed some differences for enums.

enum Enum {
    One,
    Two,
    Three,
}
fn main() {
    use Enum::*;
    let a = One;
    let b = Two;
    match (a, b) {
        (One, One) => {}
        (Two, One) => {}
        //(Three, _) => {}
    }
}

Compiling playground v0.0.1 (/playground)
error[[E0004]](https://doc.rust-lang.org/stable/error-index.html#E0004): non-exhaustive patterns: `(Enum::Three, _)` not covered
  --> src/main.rs:10:11
   |
10 |     match (a, b) {
   |           ^^^^^^ pattern `(Enum::Three, _)` not covered
   |
   = note: the matched value is of type `(Enum, Enum)`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
   |
12 ~         (Two, One) => {}
13 +         (Enum::Three, _) => todo!()
   |

For more information about this error, try `rustc --explain E0004`.
error: could not compile `playground` due to previous error

If you follow the suggestion, this is the following output

Compiling playground v0.0.1 (/playground)
error[[E0004]](https://doc.rust-lang.org/stable/error-index.html#E0004): non-exhaustive patterns: `(Enum::One, Enum::Two)`, `(Enum::One, Enum::Three)`, `(Enum::Two, Enum::Two)` and 1 more not covered
  --> src/main.rs:10:11
   |
10 |     match (a, b) {
   |           ^^^^^^ patterns `(Enum::One, Enum::Two)`, `(Enum::One, Enum::Three)`, `(Enum::Two, Enum::Two)` and 1 more not covered
   |
   = note: the matched value is of type `(Enum, Enum)`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern as shown, or multiple match arms
   |
14 ~         (Three, _) => {}
15 +         _ => todo!()
   |

For more information about this error, try `rustc --explain E0004`.
error: could not compile `playground` due to previous error

It includes both "patterns not covered" and the full wildcard arm.

Shouldn't they have similar diagnostics since they're effectively the same case, except for the position in the tuple?

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=777b19ef41c600fbe50a29b920434547

[lyming2007]

Sorry, should have asked before claiming. @lyming2007 were you gonna work on this?

no problem. Go ahead please. :)

[estebank]

Shouldn't they have similar diagnostics since they're effectively the same case, except for the position in the tuple?

Yes they should. I'm guessing we have an early return in the pattern exhaustiveness check that trips up the suggestion. If you can fix it, that'd be great, but don't spend yourself on it if you can't, just open a ticket for that case. As long as invalid code continues to be rejected, and the subsequent suggestions get you there, the status quo is ok.

[Nadrieril]

The difference of diagnostic is due to how exhaustiveness is implemented: it looks at patterns from left to right, and bails as soon as it's sure that something is missing. Avoiding the asymmetry isn't unthinkable but would need a deep rewrite the algorithm

[Nadrieril]

As far as fixing this issue is concerned, I can give some pointers. We want to detect when the user is matching on a type that can't be matched exhaustively and tell them what's happening. A check exactly like this is already done for usize/isize here. However this check is currently only done at the top-level, i.e. if the usize isn't nested within something else. For a case like (_, _) we need to recursively look into the pattern.

So as a first step we need to change the usize/isize case to make it work on cases like (_, _) or Some(_). I think this should be rather straightforward but ping me if not. Then the case for &str and others should be essentially identical.

Note the Constructor::NonExhaustive on the check I linked above. This distinguishes between the following cases:

match Some("") {
    None => {}
}
match Some("") {
    None => {}
    Some("a") => {}
    Some("b") => {}
}

Both cases report Some(_) as missing (playground), but not for the same reason. We only want to explain more in the second case. Fortunately the exhaustiveness algorithm distinguishes between the two. The first is represented by Constructor::Wildcard, and the second by Constructor::NonExhaustive.

[mattjperez]

@rustbot release-assignment

[Nadrieril]

Work in that direction is being done in #114397

[Nadrieril]

The first part of what I suggested has been merged in #114397! What remains to be done is to add more cases to this if statement. We want to test for str, and we want to move the existing "this is marked non_exhaustive" note there too. @sebastiantoh if you feel like it?

[sebastiantoh]

sure, I've opened a PR!

we want to move the existing "this is marked non_exhaustive" note there too.

I'm not sure I understand why we want this. Is str actually marked as #[non_exhaustive]? I tried looking into the stdlib source code, but couldn't find how str is actually defined

[Nadrieril]

No sorry, I'm talking about doing two different things.

The first thing is handling str just like you did for usize, with an appropriate message.

The second thing is changing how we handle the _ case of non_exhaustive enums. Currently this is handled here, which only checks the top-level type. We want to move that check to your if statement so it catches cases like Some(_).

The logic of collect_non_exhaustive_tys should be able to stay exactly the same.

[sebastiantoh]

The second thing is changing how we handle the _ case of non_exhaustive enums. Currently this is handled here, which only checks the top-level type. We want to move that check to your if statement so it catches cases like Some(_).

The logic of collect_non_exhaustive_tys should be able to stay exactly the same.

Thanks for the explanation! I tried adding the check but it seems that when matching on non-exhaustive enums, the witness's constructor is Wildcard instead of NonExhaustive. As such, I had to change the linked line of code to this - hope that's a right change!