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.

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.

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

Rebased to resolve the test/doctest conflicts.

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

@bors try @rust-timer queue

rust-timer · 2025-05-06T21:16:54Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

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-06T23:25:44Z

Queued 377ed5e with parent 1a95cc6, future comparison URL.
There is currently 1 preceding artifact in the queue.
It will probably take at least ~1.8 hours until the benchmark run finishes.

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 added 4 commits April 30, 2025 17:34

let deref patterns participate in usefulness/exhaustiveness

17127b4

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

6334394

add stubbed-out cases for rust-analyzer

1a76c9d

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 2d27aa2 to 1a76c9d Compare May 1, 2025 00:55

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

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

rust-timer commented May 6, 2025

bors commented May 6, 2025

Nadrieril left a comment

Nadrieril May 6, 2025

Nadrieril May 6, 2025

bors commented May 6, 2025

rust-timer commented May 6, 2025

allow deref patterns to participate in exhaustiveness analysis #140106

Are you sure you want to change the base?

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

rust-timer commented May 6, 2025

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

rust-timer commented May 6, 2025

dianne Apr 21, 2025 •

edited

Loading