exhaustiveness: Explain why a given pattern is considered unreachable #128034

Nadrieril · 2024-07-21T13:41:30Z

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

Nadrieril · 2024-07-21T13:43:11Z

tests/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr

+LL |         (1 | 2,) => {}
+   |         -------- matches all the values already
 LL |         (1,) => {}
-   |         ^^^^
+   |         ^^^^ unreachable pattern


@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.

What about "unreachable pattern because it is already fully covered by a(n) earlier one(s)"?

or slightly shorter: "unreachable pattern is already fully covered by an earlier one"

I like this. What about the "matches all the values already"?

tests/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr

Nadrieril · 2024-07-21T16:35:01Z

Turns out we can always get a covering set of patterns! I updated the code and added a proof-sketch as to why.

Nadrieril · 2024-07-21T19:21:56Z

@bors try @rust-timer queue

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`

bors · 2024-07-21T19:23:07Z

⌛ Trying commit fbc2f03 with merge 49ee7bc...

bors · 2024-07-21T21:12:55Z

☀️ Try build successful - checks-actions
Build commit: 49ee7bc (49ee7bc0950a2e4d6de1d7b8e1dad394785091fe)

rust-timer · 2024-07-21T22:29:25Z

Finished benchmarking commit (49ee7bc): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking 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
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.1%, 1.6%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.6%, -2.4%]	2
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 772.021s -> 773.1s (0.14%)
Artifact size: 328.86 MiB -> 328.93 MiB (0.02%)

Nadrieril · 2024-07-24T06:53:14Z

@rustbot ready

compiler/rustc_pattern_analysis/src/pat.rs

compiler-errors · 2024-07-25T15:19:46Z

@bors r+ rollup=never

bors · 2024-07-25T15:19:49Z

📌 Commit 940769a has been approved by compiler-errors

It is now in the queue for this repository.

estebank · 2024-07-25T16:57:59Z

tests/ui/pattern/usefulness/empty-types.min_exh_pats.stderr

@@ -403,18 +456,24 @@ error: unreachable pattern
   |
 LL |         _ => {}
   |         ^
+   |
+   = note: this pattern matches no values because `[!; 3]` is uninhabited


We should look into making this the span label and maybe have the note be a link to an explanation of uninhabited types (!).

estebank · 2024-07-25T16:59:13Z

tests/ui/pattern/usefulness/explain-unreachable-pats.stderr

+LL |         (_, true) => {}
+   |         ^^^^^^^^^ unreachable pattern
+   |
+note: these patterns collectively make the last one unreachable


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.

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

estebank · 2024-07-25T16:59:41Z

@Nadrieril thank you for doing this! It's gonna be super helpful.

bors · 2024-07-26T10:51:07Z

⌛ Testing commit 940769a with merge 355efac...

tgross35 · 2024-07-26T11:48:32Z

tests/ui/error-codes/E0001.stderr

+LL |         Some(_) => {/* ... */}
+   |         ------- matches some of the same values
+LL |         None => {/* ... */}
+   |         ---- matches some of the same values
+LL |         _ => {/* ... */}
+   |         ^ collectively making this unreachable


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)

I don't know what "spatula" and "forks" mean, I think just used the default principal/non-principal span mechanism in diagnostics

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.

I see. It looks a lot clearer with color at least

bors · 2024-07-26T13:11:55Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 355efac to master...

rust-timer · 2024-07-26T14:31:47Z

Finished benchmarking commit (355efac): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.1%, 0.2%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.1%	[-4.9%, -1.3%]	2
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	-3.1%	[-4.9%, -1.3%]	2

Cycles

Results (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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 771.116s -> 771.493s (0.05%)
Artifact size: 328.99 MiB -> 328.96 MiB (-0.01%)

Nadrieril · 2024-07-26T23:16:31Z

@estebank let's chat about this, I can make another PR to tweak the messages

Nadrieril · 2024-07-27T08:38:04Z

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`

rustbot assigned compiler-errors Jul 21, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 21, 2024

Nadrieril commented Jul 21, 2024

View reviewed changes

tests/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr Outdated Show resolved Hide resolved

Nadrieril force-pushed the explain-unreachable branch from 6cf3c4c to 1b18b16 Compare July 21, 2024 16:34

Nadrieril force-pushed the explain-unreachable branch 3 times, most recently from 9ffcdd3 to fbc2f03 Compare July 21, 2024 17:29

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 21, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 21, 2024

Nadrieril added 3 commits July 24, 2024 08:02

Move rustc-specific entrypoint to the rustc module

bab8ede

Add some tests

c4d6a4a

Explain why a given pattern is considered unreachable

64ac2b8

Nadrieril force-pushed the explain-unreachable branch from fbc2f03 to 6d2a803 Compare July 24, 2024 06:40

Improve "covered_by_many" error

940769a

Nadrieril force-pushed the explain-unreachable branch from 6d2a803 to 940769a Compare July 24, 2024 06:46

compiler-errors reviewed Jul 24, 2024

View reviewed changes

compiler/rustc_pattern_analysis/src/pat.rs Show resolved Hide resolved

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 25, 2024

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 25, 2024

estebank reviewed Jul 25, 2024

View reviewed changes

tgross35 reviewed Jul 26, 2024

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 26, 2024

bors merged commit 355efac into rust-lang:master Jul 26, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Jul 26, 2024

This was referenced Jul 26, 2024

Stabilize min_exhaustive_patterns #122792

Merged

Emit a warning if a match is too complex #122685

Open

Nadrieril deleted the explain-unreachable branch July 26, 2024 23:15

traviscross added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 27, 2024

Nadrieril mentioned this pull request Aug 19, 2024

Tweak unreachable lint wording #129281

Merged

Mark-Simulacrum added relnotes Marks issues that should be documented in the release notes of the next release. and removed relnotes Marks issues that should be documented in the release notes of the next release. labels Aug 25, 2024

rustbot mentioned this pull request Aug 25, 2024

Tracking issue for release notes of #128034: exhaustiveness: Explain why a given pattern is considered unreachable #129574

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exhaustiveness: Explain why a given pattern is considered unreachable #128034

exhaustiveness: Explain why a given pattern is considered unreachable #128034

Nadrieril commented Jul 21, 2024 •

edited

Loading

Nadrieril Jul 21, 2024 •

edited

Loading

estebank Jul 25, 2024

compiler-errors Jul 25, 2024

Nadrieril Jul 25, 2024

Nadrieril commented Jul 21, 2024 •

edited

Loading

Nadrieril commented Jul 21, 2024

This comment has been minimized.

bors commented Jul 21, 2024

bors commented Jul 21, 2024

This comment has been minimized.

rust-timer commented Jul 21, 2024

Nadrieril commented Jul 24, 2024

compiler-errors commented Jul 25, 2024

bors commented Jul 25, 2024

estebank Jul 25, 2024

estebank Jul 25, 2024

Nadrieril Jul 27, 2024 •

edited

Loading

estebank commented Jul 25, 2024

bors commented Jul 26, 2024

tgross35 Jul 26, 2024

Nadrieril Jul 27, 2024

tgross35 Jul 27, 2024

Nadrieril Jul 27, 2024

bors commented Jul 26, 2024

rust-timer commented Jul 26, 2024

Nadrieril commented Jul 26, 2024

Nadrieril commented Jul 27, 2024

exhaustiveness: Explain why a given pattern is considered unreachable #128034

exhaustiveness: Explain why a given pattern is considered unreachable #128034

Conversation

Nadrieril commented Jul 21, 2024 • edited Loading

Nadrieril Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nadrieril commented Jul 21, 2024 • edited Loading

Nadrieril commented Jul 21, 2024

This comment has been minimized.

bors commented Jul 21, 2024

bors commented Jul 21, 2024

This comment has been minimized.

rust-timer commented Jul 21, 2024

Overall result: ❌✅ regressions and improvements - no action needed

Nadrieril commented Jul 24, 2024

compiler-errors commented Jul 25, 2024

bors commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nadrieril Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

estebank commented Jul 25, 2024

bors commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 26, 2024

rust-timer commented Jul 26, 2024

Overall result: ❌ regressions - no action needed

Nadrieril commented Jul 26, 2024

Nadrieril commented Jul 27, 2024

Nadrieril commented Jul 21, 2024 •

edited

Loading

Nadrieril Jul 21, 2024 •

edited

Loading

Nadrieril commented Jul 21, 2024 •

edited

Loading

Nadrieril Jul 27, 2024 •

edited

Loading