-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
exhaustiveness: Explain why a given pattern is considered unreachable #128034
Conversation
LL | (1 | 2,) => {} | ||
| -------- matches all the values already | ||
LL | (1,) => {} | ||
| ^^^^ | ||
| ^^^^ unreachable pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@estebank do you have a better idea for the message here? I want to express that the pattern above already matches all the values matched by the pattern below but I'm not sure what terminology is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about "unreachable pattern because it is already fully covered by a(n) earlier one(s)"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or slightly shorter: "unreachable pattern is already fully covered by an earlier one"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. What about the "matches all the values already"?
6cf3c4c
to
1b18b16
Compare
Turns out we can always get a covering set of patterns! I updated the code and added a proof-sketch as to why. |
9ffcdd3
to
fbc2f03
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
exhaustiveness: Explain why a given pattern is considered unreachable This PR tells the user why a given pattern is considered unreachable. I reused the intersection information we were already computing; even though it's incomplete I convinced myself that it is sufficient to always get a set of patterns that cover the unreachable one. I'm not a fan of the diagnostic messages I came up with, I'm open to suggestions. Fixes rust-lang#127870. This is also the other one of the two diagnostic improvements I wanted to do before rust-lang#122792. Note: the first commit is rust-lang#128015, the second is an unrelated drive-by tweak. r? `@compiler-errors`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (49ee7bc): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary -2.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 772.021s -> 773.1s (0.14%) |
fbc2f03
to
6d2a803
Compare
6d2a803
to
940769a
Compare
@rustbot ready |
@bors r+ rollup=never |
@@ -403,18 +456,24 @@ error: unreachable pattern | |||
| | |||
LL | _ => {} | |||
| ^ | |||
| | |||
= note: this pattern matches no values because `[!; 3]` is uninhabited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should look into making this the span label and maybe have the note be a link to an explanation of uninhabited types (!).
LL | (_, true) => {} | ||
| ^^^^^^^^^ unreachable pattern | ||
| | ||
note: these patterns collectively make the last one unreachable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not assured that the unreachable pattern was the last one, so we should tweak the wording of this note message to be something along the lines "these patterns collectively make the other one unreachable" or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it may not be the last one in term of spans, but it has to be the last one in terms of match order
@Nadrieril thank you for doing this! It's gonna be super helpful. |
LL | Some(_) => {/* ... */} | ||
| ------- matches some of the same values | ||
LL | None => {/* ... */} | ||
| ---- matches some of the same values | ||
LL | _ => {/* ... */} | ||
| ^ collectively making this unreachable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the flat line a bit hard to parse, looks more like a separator or a leader to another arrow than an indicator itself. Any specific reason this uses the spatula rather than the forks?
(regardless, this change is wonderful)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what "spatula" and "forks" mean, I think just used the default principal/non-principal span mechanism in diagnostics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was being too creative, but
______--
spatula
^^---
fork
I figured it wasn't anything done specifically here. This diagnostic just winds up with more alternating lines of code and ----
than many others, and I think it is somewhat ambiguous whether ----
refers to the line up or down.
Obviously not a big deal since it is just a hint anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It looks a lot clearer with color at least
☀️ Test successful - checks-actions |
Finished benchmarking commit (355efac): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -3.1%, secondary -3.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 3.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 771.116s -> 771.493s (0.05%) |
@estebank let's chat about this, I can make another PR to tweak the messages |
Is this worthy of relnotes? I think it's cute |
…ording, r=estebank Tweak unreachable lint wording Some tweaks to the notes added in rust-lang#128034. r? `@estebank`
Rollup merge of rust-lang#129281 - Nadrieril:tweak-unreachable-lint-wording, r=estebank Tweak unreachable lint wording Some tweaks to the notes added in rust-lang#128034. r? `@estebank`
This PR tells the user why a given pattern is considered unreachable. I reused the intersection information we were already computing; even though it's incomplete I convinced myself that it is sufficient to always get a set of patterns that cover the unreachable one.
I'm not a fan of the diagnostic messages I came up with, I'm open to suggestions.
Fixes #127870. This is also the other one of the two diagnostic improvements I wanted to do before #122792.
Note: the first commit is an unrelated drive-by tweak.
r? @compiler-errors