Guard patterns: MIR lowering#154545
Draft
Human9000-bit wants to merge 18 commits intorust-lang:mainfrom
Draft
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
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%) |
rustbot
removed
the
S-waiting-on-perf
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.
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
the problem was in unimplemented exhaustiveness checking, which caused SIGILL
Co-authored-by: dianne <diannes.gm@gmail.com>
`arm_match_scope` isn't provided Co-authored-by: dianne <diannes.gm@gmail.com>
Co-authored-by: dianne <diannes.gm@gmail.com>
Human9000-bit
force-pushed
the
guard-patterns-mir
branch
from
7126433 to
150b90b
Compare
Human9000-bit
force-pushed
the
guard-patterns-mir
branch
from
150b90b to
aa8d1a0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dianne
reviewed
Apr 5, 2026
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Human9000-bit
force-pushed
the
guard-patterns-mir
branch
from
354b140 to
af5ca07
Compare
Human9000-bit
commented
Apr 5, 2026
Comment on lines
984
to
990
| enum PossiblyOr<T> { | ||
| /// A single item. | ||
| Value(T), | ||
| /// Holds the place for an or-pattern's item. This ensures their drops are scheduled in the | ||
| /// order the items appear. See rust-lang/rust#142163 for more information. | ||
| FromOrPattern, | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Is the naming ok?
Contributor
There was a problem hiding this comment.
It's a bit hard to know what PossiblyOr could mean without looking at the docs; just glancing at it, I read the "or" as the English word, not as a shortened form of "data inlined from a sub-pattern of an or-pattern". I'm not sure I'd recommend this exact name (it's a mouthful!), but something like OrderedPatternData would capture what it's for: it's used for the bits of PatternExtraData that are order-sensitive, in order to preserve their orderings.
I'm also wary about "item" in the comments, since that term has a different technical meaning in Rust. Since this is pretty abstract, it might be worth spelling out the concrete uses and/or putting a doc comment on the enum itself to give a high-level description of it.
Similarly, "value" has a technical meaning in Rust, so I'd caution against using that as the name of the constructor for a single datum.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: dianne <diannes.gm@gmail.com>
Human9000-bit
force-pushed
the
guard-patterns-mir
branch
from
af5ca07 to
b9272dd
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: dianne <diannes.gm@gmail.com>
Human9000-bit
force-pushed
the
guard-patterns-mir
branch
from
b9272dd to
0ac72c5
Compare
This comment has been minimized.
This comment has been minimized.
This reverts commit 608d568.
If `arm` is not provided, we still need `match_scope` in order to properly lower guard patterns
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
dianne
reviewed
Apr 10, 2026
Comment on lines
+201
to
+218
| match (arm.guard, arm.pat.kind) { | ||
| (Some(arm_guard), PatKind::Guard(_, pat_guard)) => { | ||
| // We introduce a new scope to contain bindings and temporaries from `if let` guards, to | ||
| // ensure they're dropped before the arm's pattern's bindings. This extends to the end of | ||
| // the arm body and is the scope of its locals as well. | ||
| visitor | ||
| .enter_scope(Scope { local_id: arm.hir_id.local_id, data: ScopeData::MatchGuard }); | ||
| visitor.cx.var_parent = visitor.cx.parent; | ||
| resolve_cond(visitor, arm_guard); | ||
| resolve_cond(visitor, pat_guard); | ||
| } | ||
| (Some(guard), _) | (_, PatKind::Guard(_, guard)) => { | ||
| visitor | ||
| .enter_scope(Scope { local_id: arm.hir_id.local_id, data: ScopeData::MatchGuard }); | ||
| visitor.cx.var_parent = visitor.cx.parent; | ||
| resolve_cond(visitor, guard); | ||
| } | ||
| _ => (), |
Contributor
There was a problem hiding this comment.
Isn't this only checking the top-level pattern? Also, how is this meant to interact with the pattern being visited in the resolve_pat call above? Eventually, we'll also need to visit guard conditions for patterns outside of match.
| .map(|_| region::Scope { data: region::ScopeData::MatchGuard, ..arm.scope }); | ||
| .map(|_| region::Scope { data: region::ScopeData::MatchGuard, ..arm.scope }) | ||
| .or_else(|| { | ||
| if let PatKind::Guard { condition, .. } = arm.pattern.kind { |
Contributor
There was a problem hiding this comment.
Isn't this only checking the top-level pattern?
For what it's worth, the MatchGuard scope is only strictly needed for guards that can introduce bindings that live into the, i.e. if let guards. Eventually, I'd like if let guard patterns as well, but if it's easier to deal with for now, we don't really need a MatchGuard scope for arms with guard patterns, since they don't support let conditions yet.
Comment on lines
+438
to
+441
| Some(region::Scope { | ||
| local_id: self.thir[condition].temp_scope_id, | ||
| data: region::ScopeData::MatchGuard, | ||
| }) |
Contributor
There was a problem hiding this comment.
The local_id of a MatchGuard scope should be the whole arm, at least with how they work currently. There's a single one for an arm with guards on it, since that's where the if let bindings and temps live. If it would be less confusing, we could put the scope in the THIR like usual, instead of needing to construct it reusing the local_id from the arm's scope here.
Comment on lines
81
to
+82
| LL | } => (), // What value should `s` have in the arm? | ||
| | - borrow later used here | ||
| | - borrow later used here |
Contributor
There was a problem hiding this comment.
The spans in these borrowck errors should be pointing to the end of the guard, I think, rather than the start of the arm.
Comment on lines
-2469
to
+2499
| self.bind_matched_candidate_for_guard(block, sub_branch.bindings.iter()); | ||
| let _ = self.in_scope( | ||
| (match_scope, self.source_info(arm_span)), | ||
| LintLevel::Inherited, | ||
| |this| { | ||
| this.bind_matched_candidate_for_guard(block, sub_branch.bindings.iter()); | ||
| BasicBlock::ZERO.unit() | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
What's this for? We should already be in the match_scope; we enter it before calling anything related to match lowering.
| bool, | ||
| ), | ||
| ) { | ||
| guard_pat_present |= pattern.kind.is_guard(); |
Contributor
There was a problem hiding this comment.
Won't this only catch if a guard pattern has been encountered thus far? If I'm understanding right, you're using this to tell whether to declare RefForGuard locals, but I think it'd be simplest if they were always present if there's a guard anywhere in the pattern/arm, since then we can always bind all of them when entering a guard; we don't need to analyze whether we need to or not.
Comment on lines
+761
to
+763
| if let PatKind::Guard { condition, .. } = pattern.kind { | ||
| self.declare_guard_bindings(condition, scope_span, visibility_scope); | ||
| }; |
Contributor
There was a problem hiding this comment.
declare_guard_bindings is specifically for if let guards, I think?
| } | ||
|
|
||
| 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.
It'd be easiest to not worry about non-match matches just yet; there's a lot more complexity in getting them to work (particularly with regards to scoping).
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