deref patterns: implement implicit deref patterns #138528
Open
+943
−222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
Some changes occurred in match checking cc @Nadrieril |
dianne
commented
Mar 15, 2025
There was a problem hiding this comment.
The implicit and explicit deref patterns report different expected types when matching on a String since string literal patterns are AdjustMode::Pass (meaning the String doesn't get peeled either). As a fun bonus, I think that means this is compatible with the string_deref_patterns feature, if we don't end up going with a more general solution.
dianne
commented
Mar 15, 2025
Comment on lines
+571
to
+576
| // If an inline const pattern has a library-defined pointer-like type and | ||
| // `deref_patterns` is enabled, don't implicitly add deref patterns for its ADT. | ||
| // Nothing in the library that implements `DerefPure` supports structural equality, | ||
| // but we don't check that until `const_to_pat` in THIR construction. By avoiding | ||
| // type errors here, we can get a more meaningful error there. | ||
| ty::Adt(adt, _) => AdjustMode::peel_until_adt(Some(*adt)), |
There was a problem hiding this comment.
I think this'll be dead if #138492 lands: no other expression patterns can be ty::Adt(..), right? I don't love leaving untested code in, so maybe this case should be removed (and maybe replaced with a debug assert?) if so. It's not too catastrophic if that assumption ends up being wrong: it'd lead to confusing diagnostics for a relatively niche error.
This comment has been minimized.
This comment has been minimized.
dianne
force-pushed
the
implicit-deref-patterns
branch
from
be33911 to
dc134ca
Compare
This comment has been minimized.
This comment has been minimized.
Since this uses `pat_adjustments`, I've also tweaked the documentation to mention implicit deref patterns and made sure the pattern migration diagnostic logic accounts for it. I'll adjust `ExprUseVisitor` in a later commit and add some tests there for closure capture inference.
See the doc comment on `ResolvedPat` for more information. This is to get the pattern's type's ADT def for (tuple) struct patterns, in order to know when to stop adding implicit deref patterns. Since we don't know the type we're matching against until we've peeled the scrutinee, `ResolvedPat` provides a way to finish checking the pattern once that type is known. The refactor's unfortunate, but this was the least disruptive way I could think of doing it. All the state that needs to be kept between resolution and type-checking is captured in a closure so it doesn't need to be manually passed around in `check_pat`. This first part only handles path patterns. Their resolutions were already needed early for adjust mode calculation, so I think it makes sense to handle that with `ResolvedPat` too.
See the previous commit for details. This doesn't yet extract the struct pat's type's ADT def before peeling, but it should now be possible.
See the previous two commits. This one required the most restructuring/indentation.
This is the use for the previous commits' refactors; see the messages there for more information.
There's a bit of a hack here, which I don't love, but the other ways I could think of doing this were substantially messier. I'll be saving that for a separate commit to reduce the noise in this one. I also haven't written that separate commit yet though, since I'm not convinced the alternatives I've thought of are better than this. Please let me know if one of these (or better yet, something else) sounds good: - The callback argument to `cat_pattern` could take a `PatOrAdjust<'hir>` enum instead of a `Pat<'hir>`, which would correspond either to a (potentially scrutinee-borrowing) pattern or a (currently definitely scrutinee-borrowing) implicit overloaded deref. Matching on `PatOrAdjust` is really noisy, unfortunately, and would require a separate `cat_pattern_unadjusted` helper that bypasses it to avoid reindenting `maybe_read_scrutinee`. - `cat_pattern` could invoke the delegate's `borrow` callback directly. This results in some repeated work between `walk_pat` and `maybe_read_scrutinee` for refutable patterns (but hopefully that wouldn't have correctness implications..?). `cat_pattern` would also need to take the `HirId` of the top-level scrutinee expression as an argument for use in diagnostics.
Implicit deref patterns allow previously ill-typed programs. Make sure they're still ill-typed without the feature gate. I've thrown in a test for `deref!(_)` too, though it seems it refers to `deref_patterns` as a library feature.
This makes the existing code a little less fragile, but it's also future-proofing. In another PR, I plan on lowering deref patterns on boxes to THIR `PatKind::Deref` rather than `PatKind::DerefPattern`, which should allow moving out of them when matching (like how box patterns are implemented). Having an enum means we can change just `Ty::pat_adjust_kind` to handle this for implicit deref patterns, rather than checking for `ty.is_ref() || ty.is_box()` everywhere we were checking `ty.is_ref()`. This could maybe be made a bit more robust by introducing a `PatAdjustment` struct analogous to `ty::adjustment::Adjustment`, but I think that'd probably be too heavy-weight for how this is used. If there were more kinds of or subtleties to pattern adjustments, that might be helpful, though. I'm not totally sure this belongs in `rustc_middle::ty::adjustment`, but the only other place I could think of it was `rustc_middle::ty::util`. I think this placement makes enough sense to avoid putting more things in `ty::util`, even if it's not a perfect fit.
dianne
force-pushed
the
implicit-deref-patterns
branch
from
dc134ca to
6a5b56a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements implicit deref patterns (per https://hackmd.io/4qDDMcvyQ-GDB089IPcHGg#Implicit-deref-patterns) and adds tests and an unstable book chapter.
Best reviewed commit-by-commit. Overall there's a lot of additions, but a lot of that is tests, documentation, and simple(?) refactoring.
Tracking issue: #87121
r? @Nadrieril