Guard patterns: MIR lowering#154545
Open
Human9000-bit wants to merge 31 commits intorust-lang:mainfrom
Open
Conversation
Collaborator
|
Some changes occurred in match lowering cc @Nadrieril |
This comment has been minimized.
This comment has been minimized.
9a8cf61 to
1d0134e
Compare
Member
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Mar 29, 2026
Guard patterns: MIR lowering
Zalathar
reviewed
Mar 29, 2026
1d0134e to
d3deeac
Compare
Contributor
Author
|
@Zalathar could you schedule perf run once again? |
Member
|
It’ll be faster to just let the old try job keep running. The new changes shouldn’t affect perf, so I think benchmarking the current job will be fine. |
dianne
reviewed
Mar 29, 2026
Contributor
There was a problem hiding this comment.
This will need some //@ run-pass tests to make sure the runtime semantics are correct, possibly also with //@ compile-flags: -Zvalidate-mir -Zlint-mir to help catch drop scheduling bugs. Getting scoping right and scheduling drops properly for guard patterns in all cases is a little subtle and will end up being the trickiest part of this; I know my first stab at that produced broken MIR ^^
You'll also want to look into how fake borrows work; patterns with guards on them will need fake borrows to make sure the guards can't modify any places being tested. For match and irrefutable let, this is needed for soundness (and in other cases, we should probably be consistent with that). At a glance, it doesn't look like this is setting has_guard for candidates, so certain things like fake borrows won't work. Likewise, this will need tests. I think some other things might use has_guard too, like or-pattern simplification.
As a meta note, I do have some opinions about how guard patterns should be implemented from my own attempt at lowering them to MIR last year. I'll try not just to compare this to what I'd do, since I'd effectively be reviewing my own code, but it might help to have more eyes on it just in case.
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (94df5ce): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 484.385s -> 484.355s (-0.01%) |
Contributor
Author
|
@dianne, just asking: in your local implementation, what were the signs of incorrect scoping and drop scheduling? |
Contributor
|
(Note: I had to merge this PR with the main branch locally before compiling, to work around #154408. So, line numbers might not be accurate.) This code causes an ICE with this PR: #![feature(guard_patterns)]
fn main() {
let x = String::from("abc");
match x {
(y if false) | y => {}
_ => {}
}
}Error outputThe following code compiles without errors with this PR, and causes a SIGTRAP at run time. #![feature(guard_patterns)]
fn main() -> ! {
let (_ if panic!()) = 1;
}The following code compiles without error with this PR and prints "abc" at run time. #![feature(guard_patterns)]
fn main() {
let x = String::from("abc");
let (y if false) = x;
println!("{y}");
} |
Contributor
iirc I can also give more direction on how these things work, where to look, etc., if you'd like ^^ I'm new to mentoring, so I'm not sure what balance would be best there; please let me know!
Oh, that's kind of worrying. Is exhaustiveness not checked at all for guard patterns currently? Having at least a stopgap for that could be good. Neither of those should compile, of course, but it should be exhaustiveness checking's responsibility, not MIR lowering. Edit: yeah, it looks like exhaustiveness wasn't part of #153828. That's fine; guard patterns are a work in progress. But it probably should be tackled in its own PR, not here. |
Contributor
|
This code compiles and prints #![feature(guard_patterns)]
#![expect(unused_parens)]
fn main() {
let x = (true, 1);
match x {
(true, ((y @ 1) | (y @ 1)) if false) => {
println!("{y}");
}
_ => {}
}
} |
Contributor
Author
|
Thanks, @theemathas, for feedback #![feature(guard_patterns)]
#![allow(incomplete_features)]
fn main() {
generic_usage(true, false, true);
}
fn generic_usage(x: bool, y: bool, z: bool) -> bool {
match (x, y) {
(true if z, false if !z) => true,
(false if z, true if z) => false,
(true, true) => true,
(false, false) => false
}
} |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: dianne <diannes.gm@gmail.com>
2a23009 to
dbe705d
Compare
dianne
reviewed
Apr 17, 2026
| } | ||
|
|
||
| let success = self.bind_pattern(self.source_info(pat.span), branch, &[], expr_span, None); | ||
| let match_scope = self.local_scope(); |
Contributor
There was a problem hiding this comment.
Un-resolving this. Could you add fixmes on each of these or revert to just passing None as the arm_match_scope? I'd like to be as clear as possible in marking code that's intentionally incorrect/incomplete.
Contributor
There was a problem hiding this comment.
Maybe this is nit-picking, but could you reframe the test so it's about something more precise than "MIR correctness"? I'm not sure what exactly that would mean here, but it's not something UI tests test.
Contributor
There was a problem hiding this comment.
Could you flesh this out and/or add more tests? Most details aren't covered by this, such as guard failure, evaluation order, or-patterns, and temporary scopes (and the ways those intersect). This may warrant multiple, more targeted files. Given how many things need to be tested about guard patterns' runtime semantics, it might get pretty crowded having it all in one file.
I wouldn't expect tests for things we're intentionally leaving for future work (like closures and non-match matches), but for anything that needed to be handled in this PR or otherwise has some special/unique behavior, we should have a test.
Contributor
Author
There was a problem hiding this comment.
Yep, test(s) should definitely be reworked
Comment on lines
+785
to
+786
| fn pat_has_guard(&self, pat: &Pat<'tcx>) -> bool { | ||
| match &pat.kind { |
Contributor
There was a problem hiding this comment.
probably this would more maintainable as a Pat::walk?
Comment on lines
-2427
to
-2428
| // Lower an instance of the arm guard (if present) for this candidate, | ||
| // and then perform bindings for the arm body. |
Contributor
There was a problem hiding this comment.
it may be nice to still have an updated version of this comment
Comment on lines
+2608
to
+2625
| fn extract_arm_span_scope( | ||
| &mut self, | ||
| sub_branch: &mut MatchTreeSubBranch<'tcx>, | ||
| arm: Option<&Arm<'tcx>>, | ||
| ) -> (Span, Scope) { | ||
| if let Some(arm) = arm { | ||
| let mut span = arm.span; | ||
| if let Some(arm_guard) = arm.guard { | ||
| span = self.thir[arm_guard].span; | ||
| sub_branch.guard_patterns.push(arm_guard); | ||
| }; | ||
| return (span, arm.scope); | ||
| } else { | ||
| let span = sub_branch.span; | ||
| let arm_scope = self.local_scope(); | ||
| return (span, arm_scope); | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
a few thoughts:
- if we need to tell where a match comes from, it'd be nice to have something more informative than just an
Option<&Arm<'_>> - it might be cleaner to decouple the span and scope? the scope is important for correctness, so we should try to make whatever we're doing there as clean and maintainable as possible. for the span, I'm fine doing something hackier, but in that case it shouldn't be tied up with anything important
- since the scope in
elsebranch is a placeholder for something unimplemented, we shouldn't be returning a real incorrect result from it. we should either ICE or not try to get a scope in the first place until we're supporting non-matchmatches. or at the very least we should have a fixme - please don't mutate the list of guards here. it took me a while to figure out what was going on. can't we add the arm's guard to the list of guards sometime much earlier?
Candidate::newcurrently takes aHasMatchGuard; could it take an optional arm guard or something? or maybe there's somewhere even earlier it could go?
Contributor
Author
There was a problem hiding this comment.
decouple the span and scope
Like, splitting it into 2 functions: one for scope and another for span?
Contributor
There was a problem hiding this comment.
well, that's maybe getting too far ahead regardless. as long as we're only handling match for now, we don't need to handle the general case for scopes or spans. I don't think there's much use in adding general machinery for that when we don't have anything in place yet that it'd need to work with; the design constraints aren't there yet
Contributor
Author
There was a problem hiding this comment.
can't we add the arm's guard to the list of guards sometime much earlier?
or maybe there's somewhere even earlier it could go?
I don't understand, why should we merge guards as early as possible?
Contributor
There was a problem hiding this comment.
personally, I find it cleaner and less error-prone to handle things uniformly. if we have the choice between representing guards in one way or in two different ways, we should try to just do it one way, at least if there's a natural way to do so. ideally, once guard patterns are more complete, I think it'd be nice to get rid of guards being present on match arms in the THIR entirely (representing them as guard patterns). until then, having as much code as possible only need to care about one way of representing guards feels like an okay compromise. it's just a stylistic preference, but I think Builder::bind_and_guard_matched_candidate is complex and we should try and keep it simple, minimizing how many cases we have to handle in it. treating match arm guards the same as a guard at the top level of a pattern (to the extent that we currently can) is a natural way to do that, I think.
Comment on lines
+151
to
+152
| if !pats[0].extra_data.guard_patterns.is_empty() { | ||
| extra_data.guard_patterns.push(super::OrderedPatternData::FromOrPattern); |
Contributor
There was a problem hiding this comment.
I don't think this check works for guards. For bindings, it relies on the set of bound variables in each alternative being the same. Checking that there's bindings in the first alternative is the same as checking whether any and all alternatives have bindings (assuming the pattern is well-formed). And by checking that there's bindings, we ensure we won't add a FromOrPattern collection of bindings to represent an or-pattern that's being simplified into a single sub-branch; trying to inline bindings from an or-pattern that's no longer there would break the inliner. Conversely, omitting the FromOrPattern when there's no bindings is fine; nothing is inlined, but there's nothing to inline. It will always be nonempty when there's bindings, even if they're nested within further or-patterns, since in that case there'll be a FromOrPattern item in the list.
I think the simplest thing to do would be have a single check that at least one of the alternatives' extra_data is nonempty, then add FromOrPattern placeholders for both bindings and guard conditions if so.
Contributor
Author
There was a problem hiding this comment.
if I implemented that as you intended, that turns out to be over generalized or smth
| if let Some((arm, match_scope)) = arm_match_scope | ||
| && let Some(guard) = arm.guard | ||
| { | ||
| if !sub_branch.guard_patterns.is_empty() || arm.is_some_and(|arm| arm.guard.is_some()) { |
Contributor
There was a problem hiding this comment.
it'd be nice to have all guards in the sub-branch at this point, so we don't have to check both that and an optional arm
Also tweak wording around arm/guard patterns in docs and comments Co-authored-by: dianne <diannes.gm@gmail.com>
This comment has been minimized.
This comment has been minimized.
13250af to
4d88621
Compare
This comment has been minimized.
This comment has been minimized.
25bd0e6 to
8a5cd76
Compare
This comment has been minimized.
This comment has been minimized.
144ba98 to
296d6fe
Compare
This comment has been minimized.
This comment has been minimized.
296d6fe to
8718db6
Compare
This comment has been minimized.
This comment has been minimized.
8718db6 to
9a5d01b
Compare
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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.
View all comments
This pr implements THIR -> MIR lowering of guard patterns:
PatKind::Guardis encountered, we lower the subpattern and push ExprId of a condition toextra_data.guard_patternsin order-preserving mannerMatchTreeSubBranchbind_ang_guard_matched_candidatewe merge arm and guard patterns into a singleVec<Exprid>r? @dianne
cc @Nadrieril, @max-niederman
Tracking issue: #129967