Guard patterns: MIR lowering#154545
Open
Human9000-bit wants to merge 33 commits into
Open
Conversation
Collaborator
|
Some changes occurred in match lowering cc @Nadrieril |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Human9000-bit
force-pushed
the
guard-patterns-mir
branch
from
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
rustbot
added
the
S-waiting-on-perf
Zalathar
reviewed
Mar 29, 2026
Human9000-bit
force-pushed
the
guard-patterns-mir
branch
from
1d0134e to
d3deeac
Compare
Member
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%) |
rustbot
removed
the
S-waiting-on-perf
Member
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}");
}
_ => {}
}
} |
Member
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.
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
Co-authored-by: dianne <diannes.gm@gmail.com>
Co-authored-by: dianne <diannes.gm@gmail.com>
Also tweak wording around arm/guard patterns in docs and comments Co-authored-by: dianne <diannes.gm@gmail.com>
rust-cloud-vms
Bot
force-pushed
the
guard-patterns-mir
branch
from
cef0cf1 to
9ff6ae5
Compare
Member
Author
|
gotta clean git history |
This comment has been minimized.
This comment has been minimized.
Member
Author
|
uh well it actually fails with explicit bugging |
rust-cloud-vms
Bot
force-pushed
the
guard-patterns-mir
branch
from
0c6b9a7 to
8a3fbab
Compare
Member
sounds like the invariants weren't as we expected then, good thing we caught it. I'd suggest reverting the signature of |
This comment has been minimized.
This comment has been minimized.
rust-cloud-vms
Bot
force-pushed
the
guard-patterns-mir
branch
from
8c90218 to
94465eb
Compare
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot)For more information how to resolve CI failures of this job, visit this link. |
Nadrieril
reviewed
Jun 15, 2026
Comment on lines
+91
to
+93
| if let Some(guard_pat) = inter_pat.guard { | ||
| guard_patterns.push(super::OrderedPatternData::One(guard_pat)); | ||
| } |
Member
There was a problem hiding this comment.
Uh, seems you're adding this guard twice? Once here and once in squash_inter_pat
Member
Author
There was a problem hiding this comment.
Thanks for noticing! I'll fix it now.
Nadrieril
reviewed
Jun 15, 2026
Comment on lines
+151
to
153
| if or_subpats.iter().any(|pat| !pat.extra_data.guards.is_empty()) { | ||
| extra_data.guards.push(super::OrderedPatternData::FromOrPattern); | ||
| } |
Member
There was a problem hiding this comment.
What if the or-pattern had a nested guard, e.g. (X | (Y if guard())) | Z? I think this would miss it? Not exactly sure why this works for bindings, can you write a comment that explains the invariant you have in mind? And please add a test like that one
Nadrieril
reviewed
Jun 15, 2026
Comment on lines
1090
to
1091
| /// For subcandidates, this is copied from the parent candidate, so it indicates | ||
| /// whether the enclosing match arm has a guard. |
Member
There was a problem hiding this comment.
Suggested change
| /// For subcandidates, this is copied from the parent candidate, so it indicates | |
| /// whether the enclosing match arm has a guard. | |
| /// For subcandidates, this indicates that the pattern is inside | |
| /// a guard pattern or a match arm guard. |
Nadrieril
reviewed
Jun 15, 2026
Comment on lines
+1143
to
+1146
| // if there're 2 or more guards, one of them is definitely guard pattern | ||
| let contains_guards = flat_pat.extra_data.guards.len() >= 2 | ||
| // or there is only one guard and we know there aren't any arm guards in there, it is guard pat as well. | ||
| || flat_pat.extra_data.guards.len() == 1 && !under_guard; |
Member
There was a problem hiding this comment.
I'm confused by this logic. What's the meaning of contains_guards you're trying to capture here?
Member
Author
There was a problem hiding this comment.
I though contains_guards is supposed to indicate whether the candidate contains guard patterns, isn't it?
Member
There was a problem hiding this comment.
your logic doesn't capture that properly: a pattern like (X | (Y if guard())) will have flat_pat.extra_data.guards.len() == 0 but it contains a guard. and under_guard is (or should be!) recursive anyway so I see what you're trying to do (cancel out the arm guard) but that's not what you're doing (e.g. the middle pattern of (X if guard()) if other_guard()) should have both under_guard and contains_guard)
Nadrieril
reviewed
Jun 15, 2026
| ty, | ||
| user_tys, | ||
| ArmHasGuard(guard.is_some()), | ||
| ArmHasGuard(has_guard), |
Member
There was a problem hiding this comment.
Could you rename that variant to ArmContainsGuard, since ArmHasGuard could be understood to mean "the arm itself has a guard", i.e. looks like pat if guard(), but the logic also includes guard patterns not of that form
Nadrieril
reviewed
Jun 15, 2026
| match_pairs: flat_pat.match_pairs, | ||
| extra_data: flat_pat.extra_data, | ||
| has_guard, | ||
| under_guard, |
Member
There was a problem hiding this comment.
Also you must update under_guards to be correct here:
Suggested change
| under_guard, | |
| under_guard: under_guard || !flat_pat.extra_data.guards.is_empty(), |
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