allow deref patterns to participate in exhaustiveness analysis #140106

dianne · 2025-04-21T08:35:44Z

Per this proposal, this PR allows deref patterns to participate in exhaustiveness analysis. Currently all deref patterns enforce DerefPure bounds on their scrutinees, so this assumes all patterns it's analyzing are well-behaved. This also doesn't support mixed exhaustiveness, and instead emits an error if deref patterns are used together with normal constructors. I think mixed exhaustiveness would be nice to have (especially if we eventually want to support arbitrary Deref impls¹), but it'd require more work to get reasonable diagnostics².

Tracking issue for deref patterns: #87121

r? @Nadrieril

Regardless of whether we support limited exhaustiveness checking for untrusted Deref or always require other arms to be exhaustive, I think it'd be useful to allow mixed matching for user-defined smart pointers. And it'd be strange if it worked there but not for Cow. ↩
I think listing out witnesses of non-exhaustiveness can be confusing when they're not necessarily disjoint, and when you only need to cover some of them, so we'd probably want special formatting and/or explanatory subdiagnostics. And if it's implemented similarly to unions, we'd probably also want some way of merging witnesses; the way witnesses for unions can appear duplicated is pretty unfortunate. I'm not sure yet how the diagnostics should look, especially for deeply nested patterns. ↩

rustbot · 2025-04-21T08:35:50Z

Some changes occurred in exhaustiveness checking

cc @Nadrieril

compiler/rustc_pattern_analysis/src/constructor.rs

dianne · 2025-04-21T08:52:52Z

compiler/rustc_pattern_analysis/src/rustc.rs

+            PatKind::DerefPattern { subpattern, .. } => {
+                // NB(deref_patterns): This assumes the deref pattern is matching on a trusted
+                // `DerefPure` type. If the `Deref` impl isn't trusted, any deref pattern that can
+                // fail (possibly due to expanding or-patterns inside it) must not influence
+                // exhaustiveness analysis.


I'm assuming here that we're not trying to guarantee a single deref per column, but that'd also work if there's a practical way to do it. I'd have to look into how match lowering and or-pattern expansion work to see whether it's possible to at least guarantee it in that case. Of course, it's also consistent (and maybe easier to explain) if they can never participate in exhaustiveness for an untrusted Deref impl.

Not impossible to implement but not sure I'd want to guarantee that. Also this just cannot work with a hypothetical DerefMove: for that we necessarily need to call deref to determine the branch first. So I'm inclined to say "for deref patterns with custom Deref, we require unsafe impl DerefPure".

nit: "pattern that can fail" wasn't immediately clear to me. I might say "If the Deref impl isn't trusted, exhaustiveness must take into account that multiple calls to deref may return different results. Hence multiple deref! patterns cannot be exhaustive together unless each is exhaustive by itself."

dianne · 2025-04-21T08:53:46Z

compiler/rustc_pattern_analysis/src/rustc.rs

@@ -1119,6 +1132,47 @@ pub fn analyze_match<'p, 'tcx>(
    Ok(report)
 }

+fn detect_mixed_deref_pat_ctors<'p, 'tcx>(


Is there a better home for this anywhere? The other use of PatternColumn I could find was in lints.rs, but this isn't really a lint, so it didn't feel right there.

It's looking like maybe this should work for generic PatCx so if/when r-a supports deref patterns it'll be able to run before match analysis there too. So it definitely shouldn't be in rustc.rs or lints.rs. usefulness.rs doesn't quite feel right for something using PatColumn, though it probably should be usefulness::compute_match_usefulness that calls it. pat_column.rs is only the implementation of PatColumn so I'm not sure that's right either.

I looked into rehousing this and making it work for generic PatCx on my local branch. It's a bit awkward, though: in order to avoid doing a pass for every single pattern, it'd still be the responsibility of the frontend to report when the pattern needs to be checked for mixed constructors (i.e. when a deref pattern was lowered), so the API is a bit awkward. Maybe an extra pass for every pattern is inconsequential perf though? I'm not sure what would be best, but I can open a perf experiment if needed. In the mean time, I'll think more about how to handle diagnostics for mixed exhaustiveness, to maybe avoid this check altogether.

Yeah, perf experiment seems the only way to know

As for where to house it, we could rename lints.rs to checks.rs and put it there?

rustbot · 2025-04-21T09:34:25Z

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

bors · 2025-04-29T06:40:37Z

☔ The latest upstream changes (presumably #140415) made this pull request unmergeable. Please resolve the merge conflicts.

dianne · 2025-05-01T01:05:53Z

Rebased to resolve the test/doctest conflicts.

Nadrieril · 2025-05-06T21:16:52Z

@bors try @rust-timer queue

allow deref patterns to participate in exhaustiveness analysis Per [this proposal](https://hackmd.io/4qDDMcvyQ-GDB089IPcHGg#Exhaustiveness), this PR allows deref patterns to participate in exhaustiveness analysis. Currently all deref patterns enforce `DerefPure` bounds on their scrutinees, so this assumes all patterns it's analyzing are well-behaved. This also doesn't support [mixed exhaustiveness](https://hackmd.io/4qDDMcvyQ-GDB089IPcHGg#Mixed-exhaustiveness), and instead emits an error if deref patterns are used together with normal constructors. I think mixed exhaustiveness would be nice to have (especially if we eventually want to support arbitrary `Deref` impls[^1]), but it'd require more work to get reasonable diagnostics[^2]. Tracking issue for deref patterns: rust-lang#87121 r? `@Nadrieril` [^1]: Regardless of whether we support limited exhaustiveness checking for untrusted `Deref` or always require other arms to be exhaustive, I think it'd be useful to allow mixed matching for user-defined smart pointers. And it'd be strange if it worked there but not for `Cow`. [^2]: I think listing out witnesses of non-exhaustiveness can be confusing when they're not necessarily disjoint, and when you only need to cover some of them, so we'd probably want special formatting and/or explanatory subdiagnostics. And if it's implemented similarly to unions, we'd probably also want some way of merging witnesses; the way witnesses for unions can appear duplicated is pretty unfortunate. I'm not sure yet how the diagnostics should look, especially for deeply nested patterns.

bors · 2025-05-06T21:18:07Z

⌛ Trying commit 1a76c9d with merge 377ed5e3433b200e14f360ee68b384e0a3f066d8...

Nadrieril

Pretty much flawless, can't find anything I'd change in this implementation. Started a perf run out of habit, not expecting a change. Happy to merge as-is, got some nits.

Nadrieril · 2025-05-06T21:38:54Z

tests/ui/pattern/deref-patterns/usefulness/mixed-constructors.rs

+        (0, Cow::Owned(_)) => {}
+        (0, Cow::Borrowed(_)) => {}
+        _ => {}
+        (0, true) => {}


To make sure someone doesn't move the check inside exhaustiveness checking:

Suggested change

(0, true) => {}

(1, true) => {}

Nadrieril · 2025-05-06T21:48:37Z

compiler/rustc_pattern_analysis/src/rustc.rs

+            PatKind::DerefPattern { subpattern, .. } => {
+                // NB(deref_patterns): This assumes the deref pattern is matching on a trusted
+                // `DerefPure` type. If the `Deref` impl isn't trusted, any deref pattern that can
+                // fail (possibly due to expanding or-patterns inside it) must not influence
+                // exhaustiveness analysis.


nit: "pattern that can fail" wasn't immediately clear to me. I might say "If the Deref impl isn't trusted, exhaustiveness must take into account that multiple calls to deref may return different results. Hence multiple deref! patterns cannot be exhaustive together unless each is exhaustive by itself."

bors · 2025-05-06T23:25:41Z

☀️ Try build successful - checks-actions
Build commit: 377ed5e (377ed5e3433b200e14f360ee68b384e0a3f066d8)

rust-timer · 2025-05-07T01:15:01Z

Finished benchmarking commit (377ed5e): comparison URL.

Overall result: ❌ regressions - please read the text below

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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.1%, 1.1%]	4
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.1%, 1.1%]	4

Max RSS (memory usage)

Results (primary 0.2%)

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.6%	[0.4%, 0.7%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.7%, -0.6%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-0.7%, 0.7%]	9

Cycles

Results (primary -0.4%)

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.6%	[0.5%, 0.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.8%, -0.4%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.8%, 0.6%]	11

Binary size

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

Bootstrap: 769.964s -> 770.737s (0.10%)
Artifact size: 365.50 MiB -> 365.46 MiB (-0.01%)

dianne · 2025-05-07T01:28:23Z

Applied the comment and test changes; ty! I also added a note about the mixed constructor check. I'll save moving it elsewhere and making it generic for later, since that's a bit more disruptive. (diff)

I only have a bit of experience with perf readouts, but that regression might be spurious? I checked the regressions listed there; none of them spend more time in check_match than usual (within reasonable error), and that should be all this affects. It looks like the previous run was abnormally fast. Did a big perf improvement get merged? I haven't rebased this onto master in a while.

Edit: It does look like a perf improvement had been merged, and that's what this was being compared against. I'll rebase onto master just in case we want to do another perf run, but I'm pretty sure judging by the check_match numbers there's no meaningful perf change here.

This does not yet handle the case of mixed deref patterns with normal constructors; it'll ICE in `Constructor::is_covered_by`. That'll be fixed in a later commit.

Without adding proper support for mixed exhaustiveness, mixing deref patterns with normal constructors would either violate `ConstructorSet::split`'s invariant 4 or 7. We'd either be ignoring rows with normal constructors or we'd have problems in unspecialization from non-disjoint constructors. Checking mixed exhaustivenss similarly to how unions are currently checked should work, but the diagnostics for unions are confusing. Since mixing deref patterns with normal constructors is pretty niche (currently it only makes sense for `Cow`), emitting an error lets us avoid committing to supporting mixed exhaustiveness without a good answer for the diagnostics.

rust-analyzer doesn't construct `DerefPattern(_)` constructors, so these shouldn't crash. It looks like this is how slice patterns are implemented too.

Nadrieril · 2025-05-07T21:13:42Z

The perf diff is small enough to ignore imo. Let's merge!

@bors r+

bors · 2025-05-07T21:13:45Z

📌 Commit 09fed2d has been approved by Nadrieril

It is now in the queue for this repository.

bors · 2025-05-08T02:16:49Z

⌛ Testing commit 09fed2d with merge 7e552b4...

bors · 2025-05-08T05:31:16Z

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing 7e552b4 to master...

github-actions · 2025-05-08T05:33:57Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing ae3e8c6 (parent) -> 7e552b4 (this PR)

Test differences

Show 13 test diffs

Stage 1

[ui] tests/ui/pattern/deref-patterns/usefulness/empty-types.rs: [missing] -> pass (J1)
[ui] tests/ui/pattern/deref-patterns/usefulness/mixed-constructors.rs: [missing] -> pass (J1)
[ui] tests/ui/pattern/deref-patterns/usefulness/non-exhaustive.rs: [missing] -> pass (J1)
[ui] tests/ui/pattern/deref-patterns/usefulness/unreachable-patterns.rs: [missing] -> pass (J1)
errors::verify_pattern_analysis_mixed_deref_pattern_constructors_2: [missing] -> pass (J2)

Stage 2

[ui] tests/ui/pattern/deref-patterns/usefulness/empty-types.rs: [missing] -> pass (J0)
[ui] tests/ui/pattern/deref-patterns/usefulness/mixed-constructors.rs: [missing] -> pass (J0)
[ui] tests/ui/pattern/deref-patterns/usefulness/non-exhaustive.rs: [missing] -> pass (J0)
[ui] tests/ui/pattern/deref-patterns/usefulness/unreachable-patterns.rs: [missing] -> pass (J0)

Additionally, 4 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 7e552b46af72df390ed233b58a7f51650515b2a8 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

x86_64-apple-1: 7521.2s -> 6348.4s (-15.6%)
dist-arm-linux: 4778.2s -> 5328.5s (11.5%)
aarch64-gnu-debug: 4275.6s -> 3959.7s (-7.4%)
dist-x86_64-msvc-alt: 7443.5s -> 7941.2s (6.7%)
dist-apple-various: 5435.9s -> 5733.1s (5.5%)
dist-aarch64-msvc: 8356.9s -> 8801.8s (5.3%)
dist-aarch64-linux: 5362.9s -> 5630.2s (5.0%)
dist-i686-mingw: 8393.5s -> 8023.2s (-4.4%)
dist-loongarch64-musl: 5586.7s -> 5352.9s (-4.2%)
dist-ohos: 10568.0s -> 10975.6s (3.9%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-05-08T07:49:51Z

Finished benchmarking commit (7e552b4): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 0.6%, secondary -4.2%)

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)	2.4%	[0.9%, 5.0%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.0%, -0.4%]	6
Improvements ✅ (secondary)	-4.2%	[-5.5%, -1.6%]	4
All ❌✅ (primary)	0.6%	[-1.0%, 5.0%]	10

Cycles

Results (primary 0.7%, secondary 2.2%)

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.7%	[0.4%, 1.3%]	26
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.4%, 1.3%]	26

Binary size

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

Bootstrap: 769.46s -> 769.201s (-0.03%)
Artifact size: 365.32 MiB -> 365.25 MiB (-0.02%)

rustbot assigned Nadrieril Apr 21, 2025

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 Apr 21, 2025

dianne commented Apr 21, 2025

View reviewed changes

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

This comment has been minimized.

Sign in to view

dianne commented Apr 21, 2025

View reviewed changes

dianne mentioned this pull request Apr 24, 2025

Implement pin-project in pattern matching for &pin mut|const T #139751

Draft

dianne force-pushed the deref-pat-usefulness branch from 2d27aa2 to 1a76c9d Compare May 1, 2025 00:55

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 May 6, 2025

Nadrieril approved these changes May 6, 2025

View reviewed changes

Nadrieril mentioned this pull request May 6, 2025

Tracking issue: deref patterns #87121

Open

6 tasks

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 7, 2025

dianne force-pushed the deref-pat-usefulness branch from 1a76c9d to 2d4e19c Compare May 7, 2025 01:15

dianne added 3 commits May 6, 2025 18:53

let deref patterns participate in usefulness/exhaustiveness

b41d8bd

This does not yet handle the case of mixed deref patterns with normal constructors; it'll ICE in `Constructor::is_covered_by`. That'll be fixed in a later commit.

add exhaustiveness/usefulness tests for deref patterns

cf43bba

add stubbed-out cases for rust-analyzer

09fed2d

rust-analyzer doesn't construct `DerefPattern(_)` constructors, so these shouldn't crash. It looks like this is how slice patterns are implemented too.

dianne force-pushed the deref-pat-usefulness branch from 2d4e19c to 09fed2d Compare May 7, 2025 02:16

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label May 8, 2025

bors merged commit 7e552b4 into rust-lang:master May 8, 2025
7 checks passed

rustbot added this to the 1.88.0 milestone May 8, 2025

dianne deleted the deref-pat-usefulness branch May 8, 2025 05:42

bors mentioned this pull request May 8, 2025

Error on recursive opaque ty in HIR typeck #139419

Open

rustbot removed the perf-regression Performance regression. label May 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow deref patterns to participate in exhaustiveness analysis #140106

allow deref patterns to participate in exhaustiveness analysis #140106

dianne commented Apr 21, 2025

rustbot commented Apr 21, 2025

This comment has been minimized.

dianne Apr 21, 2025

Nadrieril May 6, 2025

Nadrieril May 6, 2025

dianne Apr 21, 2025

dianne Apr 21, 2025 •

edited

Loading

dianne May 1, 2025

Nadrieril May 6, 2025

Nadrieril May 6, 2025

rustbot commented Apr 21, 2025

bors commented Apr 29, 2025

dianne commented May 1, 2025

Nadrieril commented May 6, 2025

This comment has been minimized.

bors commented May 6, 2025

Nadrieril left a comment

Nadrieril May 6, 2025

Nadrieril May 6, 2025

bors commented May 6, 2025

This comment has been minimized.

rust-timer commented May 7, 2025

dianne commented May 7, 2025 •

edited

Loading

Nadrieril commented May 7, 2025

bors commented May 7, 2025

bors commented May 8, 2025

bors commented May 8, 2025

github-actions bot commented May 8, 2025

Stage 1

Stage 2

rust-timer commented May 8, 2025

allow deref patterns to participate in exhaustiveness analysis #140106

allow deref patterns to participate in exhaustiveness analysis #140106

Conversation

dianne commented Apr 21, 2025

Footnotes

rustbot commented Apr 21, 2025

This comment has been minimized.

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

dianne Apr 21, 2025 • 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

rustbot commented Apr 21, 2025

bors commented Apr 29, 2025

dianne commented May 1, 2025

Nadrieril commented May 6, 2025

This comment has been minimized.

bors commented May 6, 2025

Nadrieril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 6, 2025

This comment has been minimized.

rust-timer commented May 7, 2025

Overall result: ❌ regressions - please read the text below

dianne commented May 7, 2025 • edited Loading

Nadrieril commented May 7, 2025

bors commented May 7, 2025

bors commented May 8, 2025

bors commented May 8, 2025

github-actions bot commented May 8, 2025

Test differences

Stage 1

Stage 2

Job duration changes

rust-timer commented May 8, 2025

Overall result: no relevant changes - no action needed

dianne Apr 21, 2025 •

edited

Loading

dianne commented May 7, 2025 •

edited

Loading