Guard patterns: MIR lowering#154545
Draft
Human9000-bit wants to merge 13 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.
dianne
reviewed
Apr 3, 2026
Comment on lines
+649
to
+651
|
|
||
| #[type_visitable(ignore)] | ||
| pub scope: Option<region::Scope>, |
Contributor
There was a problem hiding this comment.
Is there a reason this is included for each pattern node? Guard expressions should already have scope information on them (since all expressions can be associated to their temporary scopes), and the scope to return to when a guard fails can probably be the same for the whole match.
Contributor
Author
There was a problem hiding this comment.
you mean taking ItemLocalId from expression from guards is fine?
Contributor
There was a problem hiding this comment.
I mean the individual guards' ItemLocalIds shouldn't matter for guard pattern lowering. You shouldn't have to consider them when picking what scope to use for the in_if_then_scope call. The scope provided to in_if_then_scope is the scope to return to on guard failure; i.e., failing a guard leaves all the scopes nested within that chosen scope, dropping their contents. When a guard fails, we need to drop the bindings created for the guard, so the chosen scope should be above the scope of the match arm's bindings, which itself should be above the guards' scopes. This is why, for match, the scope of the match expression is used; it's the scope above the arms' scopes (which contain the arms' bindings).
Comment on lines
+973
to
+974
| /// Scope of this sub-branch | ||
| scope: Option<Scope>, |
Contributor
There was a problem hiding this comment.
The scope to return to when a guard fails can be the same for the entire match, I think? I don't think it needs to vary per sub-branch.
Comment on lines
+1437
to
+1438
| /// Scope of this sub-branch; used mostly by guard patterns | ||
| scope: Option<Scope>, |
Contributor
There was a problem hiding this comment.
As above, I don't think this needs to vary per sub-branch?
Comment on lines
+455
to
+459
| // In some cases during MIR lowering we need a Scope | ||
| let scope = region::Scope { | ||
| local_id: pat.hir_id.local_id, | ||
| data: region::ScopeData::MatchGuard, | ||
| }; |
Contributor
There was a problem hiding this comment.
If you use a scope, you'll also need to enter the same scope while building the MIR; I think that's what the ICEs in PR CI are about. You should also create the scope in the scope resolution visitor, since that should match up with MIR building. As above, though, I don't think should be on every guard pattern.
There's meant to be exactly one ScopeData::MatchGuard per match arm with a guard, nested within the arm's scope; it's used for if let guard bindings and temporaries so that they're dropped before the arm's pattern's bindings.
Contributor
Author
There was a problem hiding this comment.
I don't understand. How to enter a scope in MIR build? Like, self.in_scope()?
Contributor
There was a problem hiding this comment.
Yep. When the MIR builder leaves a scope (by getting to the end of the closure provided to Builder::in_scope), it emits the drops that were scheduled in that scope (i.e. the drops for locals in that scope). As such, it's important for the MIR builder to enter the scopes of whatever it's lowering at the appropriate times.
These things are kind of spread out, so here's a couple examples:
Expressions, statements, etc. have an associated ScopeData::Node scope:
- This is created for all applicable nodes by the scope resolution visitor here.
- This is included in the THIR for each HIR expression here by wrapping the THIR for each HIR expression in an
ExprKind::Scope. - The various places during MIR building that handle expressions call
Builder::in_scopeon that scope when they encounter anExprKind::Scope.
For match, in addition to the scope for the whole match expression, match arms also each have a scope for the arm. If an arm has a guard, there's also a scope for the guard bindings and temporaries nested within that (which extends through the arm body, so that if let guards' bindings and temps live through the arm):
dianne
reviewed
Apr 3, 2026
Comment on lines
2463
to
2475
| // Bindings for guards require some extra handling to automatically | ||
| // insert implicit references/dereferences. | ||
| // This always schedules storage drops, so we may need to unschedule them below. | ||
| self.bind_matched_candidate_for_guard(block, sub_branch.bindings.iter()); | ||
| let guard_frame = GuardFrame { | ||
| locals: sub_branch | ||
| .bindings | ||
| .iter() | ||
| .map(|b| GuardFrameLocal::new(b.var_id)) | ||
| .collect(), | ||
| }; | ||
| debug!("entering guard building context: {:?}", guard_frame); | ||
| self.guard_context.push(guard_frame); |
Contributor
There was a problem hiding this comment.
Something to watch out for here: each MatchTreeBranch needs a consistent set of bindings across all sub-branches. I think this means that if any sub-branch has a guard, all sub-branches will need StorageLives for all the RefForGuards.
Co-authored-by: Dianne <diannes.gm@gmail.com>
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
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