Don't warn an empty pattern unreachable if we're not sure the data is valid #118308

Nadrieril · 2023-11-26T09:58:11Z

Exhaustiveness checking used to be naive about the possibility of a place containing invalid data. This could cause it to emit an "unreachable pattern" lint on an arm that was in fact reachable, as in #117119.

This PR fixes that. We now track whether a place that is matched on may hold invalid data. This also forced me to be extra precise about how exhaustiveness manages empty types.

Note that this now errs in the opposite direction: the following arm is truly unreachable (because the binding causes a read of the value) but not linted as such. I'd rather not recommend writing a match ... {} that has the implicit side-effect of loading the value. Never patterns will solve this cleanly.

match union.value {
    _x => unreachable!(),
}

I recommend reviewing commit by commit. I went all-in on the test suite because this went through a lot of iterations and I kept everything. The bit I'm least confident in is is_known_valid_scrutinee in check_match.rs.

Fixes #117119.

rustbot · 2023-11-26T09:58:20Z

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

Nadrieril · 2023-11-26T14:44:45Z

Perf looked neutral on my machine but just in case

@bors try @rust-timer queue

bors · 2023-11-26T14:45:55Z

⌛ Trying commit 4f93aba with merge f8b2ffa...

…take-3, r=<try> Don't warn an empty pattern unreachable if we're not sure the data is valid Exhaustiveness checking used to be naive about the possibility of a place containing invalid data. This could cause it to emit an "unreachable pattern" lint on an arm that was in fact reachable, as in rust-lang#117119. This PR fixes that. We now track whether a place that is matched on may hold invalid data. This also forced me to be extra precise about how exhaustiveness manages empty types. Note that this now errs in the opposite direction: the following arm is truly unreachable but not linted as such. I'd rather not recommend writing a `match ... {}` that has the implicit side-effect of loading the value. [Never patterns](rust-lang#118155) will solve this cleanly. ```rust match union.value { _x => unreachable!(), } ``` I recommend reviewing commit by commit. I went all-in on the test suite because this went through a lot of iterations and I kept everything. The bit I'm least confident in is `is_known_valid_scrutinee` in `check_match.rs`. Fixes rust-lang#117119.

bors · 2023-11-26T16:13:49Z

☀️ Try build successful - checks-actions
Build commit: f8b2ffa (f8b2ffa5ce68fff676c4335b8a04d4d40ddecfcc)

rust-timer · 2023-11-26T20:04:24Z

Finished benchmarking commit (f8b2ffa): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.8%	[0.8%, 0.9%]	2
Regressions ❌ (secondary)	1.0%	[0.9%, 1.1%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.8%, 0.9%]	2

Max RSS (memory usage)

Results

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.4%	[2.7%, 4.7%]	3
Improvements ✅ (primary)	-2.4%	[-2.7%, -2.1%]	2
Improvements ✅ (secondary)	-3.1%	[-5.1%, -0.6%]	6
All ❌✅ (primary)	-2.4%	[-2.7%, -2.1%]	2

Cycles

Results

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)	5.5%	[4.9%, 6.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 674.277s -> 675.27s (0.15%)
Artifact size: 313.37 MiB -> 313.38 MiB (0.00%)

Nadrieril · 2023-11-29T01:18:43Z

r? @compiler-errors if you have the time

compiler-errors · 2023-11-29T01:24:56Z

Do you have justification for the perf regression? Is there any way around it? I assume if no, then we can land this since it's obviously a correctness fix, but I do want to be certain that it's necessary before accepting this (admittedly somewhat minor) perf hit.

Nadrieril · 2023-11-29T02:19:08Z

Yeah I expected a perf regression because of the validity tracking. It adds an extra step for every constructor we try, and match-stress has an enum with 8000 of these. I don't think there's a way around it, I need to know the validity of every inspected place.

My guess is that it's particularly impactful because it allocates; that's not easily avoided, I'm planning to try arena allocation of the Matrix in the future

compiler-errors · 2023-11-29T02:20:59Z

OK I shall review this PR tomorrow then with that in mind.

compiler/rustc_middle/src/ty/inhabitedness/inhabited_predicate.rs

compiler-errors · 2023-12-08T17:15:08Z

compiler/rustc_mir_build/src/thir/pattern/check_match.rs

+            | Repeat { .. }
+            | StaticRef { .. }
+            | ThreadLocalRef { .. }
+            | Tuple { .. }


We don't need to recurse into aggregates?

Oh, this function conservatively returns true?

No, unless I misunderstand what these constructs do, this means we have match (x, y, z) { ... }. A (x, y, z) expression evaluates to a value, not a place, and thus cannot hold invalid data without UB. The conservative direction would in fact be false.

compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs

- `ConstructorSet` knows about both empty and nonempty constructors; - If an empty constructor is present in the column, we output it in `split().present`.

This is for post-monomorphization cycles. These are only caught later (in drop elaboration for the example that I saw), so we need to handle them here. This issue wasn't noticed before because exhaustiveness only checked inhabitedness when `exhaustive_patterns` was on. The preceding commit now check inhabitedness always, which revealed the problem.

… valid

compiler-errors · 2023-12-09T17:12:35Z

@bors r+ rollup=never

bors · 2023-12-09T17:12:37Z

📌 Commit c3df51a has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-12-09T19:03:06Z

⌛ Testing commit c3df51a with merge 06e02d5...

bors · 2023-12-09T20:59:43Z

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

rust-timer · 2023-12-09T22:21:10Z

Finished benchmarking commit (06e02d5): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.6%	[0.2%, 0.9%]	4
Regressions ❌ (secondary)	1.0%	[1.0%, 1.0%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.2%, 0.9%]	4

Max RSS (memory usage)

Results

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.8%	[0.5%, 1.5%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.4%	[-2.7%, -0.5%]	3
Improvements ✅ (secondary)	-1.8%	[-5.0%, -0.6%]	5
All ❌✅ (primary)	-0.1%	[-2.7%, 1.5%]	7

Cycles

Results

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.5%	[0.4%, 0.5%]	2
Regressions ❌ (secondary)	3.0%	[2.4%, 3.5%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.4%, 0.5%]	2

Binary size

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

Bootstrap: 667.911s -> 668.865s (0.14%)
Artifact size: 314.09 MiB -> 314.11 MiB (0.01%)

RalfJung · 2023-12-10T07:39:27Z

compiler/rustc_mir_build/src/thir/pattern/check_match.rs

+    fn is_known_valid_scrutinee(&self, scrutinee: &Expr<'tcx>) -> bool {
+        use ExprKind::*;
+        match &scrutinee.kind {
+            // Both pointers and references can validly point to a place with invalid data.


For references I'd say this is an open question, but we want to be conservative here.

Ah indeed, I'll update the comment to say that. Does the rest of this function seem right to you?

I don't quite understand how it is used, but it treats unions and raw ptr deref specially so that seems right. For reference deref I guess we have to see how we want to treat them in the end, but it seems good to be conservative -- and I assume "not known to be valid" is the conservative answer here.

What I am missing is test coverage, is there any test for the case of a union with an uninhabited field, or raw pointers?

I assume "not known to be valid" is the conservative answer here

Yep

Here's the test cases: https://github.com/Nadrieril/rust/blob/c3df51a976dfa69f0f1869997fdf79516ba6afab/tests/ui/pattern/usefulness/empty-types.rs#L191-L271

Ah, nice! The tests for references should also have the comment about this being undecided so the lint being conservative.

Feel free to r? me on the PR that adds the comments.

I'm sneaking the comments along with #118803

rustbot · 2023-12-11T15:49:52Z

Assignment is not allowed on a closed PR.

… r=compiler-errors add test for coercing never to infinite type Closes rust-lang#113197. This was fixed in rust-lang#118308, probably rust-lang@1978168.

Rollup merge of rust-lang#119323 - lukas-code:test-never-to-infinite, r=compiler-errors add test for coercing never to infinite type Closes rust-lang#113197. This was fixed in rust-lang#118308, probably rust-lang@1978168.

rustbot assigned TaKO8Ki Nov 26, 2023

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 Nov 26, 2023