allow deref patterns to participate in exhaustiveness analysis #140106
Conversation
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.
rustbot
added
S-waiting-on-review
|
Some changes occurred in exhaustiveness checking cc @Nadrieril |
This comment has been minimized.
This comment has been minimized.
| 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. |
There was a problem hiding this comment.
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.
| @@ -1119,6 +1132,47 @@ pub fn analyze_match<'p, 'tcx>( | |||
| Ok(report) | |||
| } | |||
|
|
|||
| fn detect_mixed_deref_pat_ctors<'p, 'tcx>( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
rust-analyzer doesn't construct `DerefPattern(_)` constructors, so these shouldn't crash. It looks like this is how slice patterns are implemented too.
|
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 |
Per this proposal, this PR allows deref patterns to participate in exhaustiveness analysis. Currently all deref patterns enforce
DerefPurebounds 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 arbitraryDerefimpls1), but it'd require more work to get reasonable diagnostics2.Tracking issue for deref patterns: #87121
r? @Nadrieril
Footnotes
Regardless of whether we support limited exhaustiveness checking for untrusted
Derefor 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 forCow. ↩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. ↩