-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Stabilize if let
guards (feature(if_let_guard)
)
#141295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
Some changes occurred to the CTFE machinery Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in compiler/rustc_codegen_ssa |
6fe74d9
to
5ee8970
Compare
rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
eb0e4b4
to
0358002
Compare
This comment has been minimized.
This comment has been minimized.
92a5204
to
ab138ce
Compare
This comment has been minimized.
This comment has been minimized.
5ceca48
to
a20c4f6
Compare
This comment has been minimized.
This comment has been minimized.
1dd9974
to
5796073
Compare
cc @Nadrieril |
This needs a fcp so I'd like to roll this to someone more familiar with this feature |
r? @est31 |
@est31 huge thanks for everything! From the moment I jumped into this feature, you've been there guiding me through all of this. You patiently answered all my questions, helped me understand the complex parts, and when I wanted to try something new and asked to handle the stabilization PR, you trusted me with it even though I was just getting started Honestly, most of the work and insights here are yours. You didn't just help technically - you made it possible for someone new like me to actually contribute something meaningful. That kind of mentorship means a lot, really appreciate it! |
Rollup of 14 pull requests Successful merges: - #138062 (Enable Non-determinism of float operations in Miri and change std tests ) - #140560 (Allow `#![doc(test(attr(..)))]` everywhere) - #141001 (Make NonZero<char> possible) - #141295 (Stabilize `if let` guards (`feature(if_let_guard)`)) - #141435 (Add (back) `unsupported_calling_conventions` lint to reject more invalid calling conventions) - #141447 (Document representation of `Option<unsafe fn()>`) - #142008 (const-eval error: always say in which item the error occurred) - #142053 (Add new Tier-3 targets: `loongarch32-unknown-none*`) - #142065 (Stabilize `const_eq_ignore_ascii_case`) - #142116 (Fix bootstrap tracing imports) - #142126 (Treat normalizing consts like normalizing types in deeply normalize) - #142140 (compiler: Sort and doc ExternAbi variants) - #142148 (compiler: Treat ForceWarning as a Warning for diagnostic level) - #142154 (get rid of spurious cfg(bootstrap)) r? `@ghost` `@rustbot` modify labels: rollup
@bors r- I appreciate and share the enthusiasm, but this shouldn't merge until the Reference PR is approved, which it is not yet. |
The reference PR was (before this r-) marked S-waiting-on-stabilization so I would have assumed that means that this was not blocked on the reference PR and that it was approved |
Yes, we on lang-docs could probably use to better document the system here. In this case, the intended signals were that:
(That the Reference PR was also marked |
☔ The latest upstream changes (presumably #142220) made this pull request unmergeable. Please resolve the merge conflicts. |
a20a9aa
to
87bedfc
Compare
I'm sorry to bring this up after the FCP, but having worked some more on implementing guard patterns, I've noticed the drop order here actually isn't quite what I'd expect, and there are in fact still some missing drop order tests. All the tests I can find in #140981 for assert_drop_order(1..=3, |o| {
// `if let` guards' drops are *after* pattern bindings'
match [LogDrop(o, 2), LogDrop(o, 1)] {
[x, y] if let z = LogDrop(o, 3) => {}
_ => unreachable!(),
}
});
assert_drop_order(1..=3, |o| {
// I would expect this drop order.
match [LogDrop(o, 3), LogDrop(o, 2)] {
[x, y] => {
let z = LogDrop(o, 1);
}
}
}); This also means dropck problems (playground): // `x` is dropped first, resulting in a dropck failure.
match SignificantDrop(0) {
x if let y = SignificantDrop(&x) => {}
_ => unreachable!(),
}
// `y` is dropped first; everything's fine.
match SignificantDrop(0) {
x => {
let y = SignificantDrop(&x);
}
}
|
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
Given the near miss -- despite the care we took to avoid it -- it now feels like, for this and these sort of things, maybe we should expect proof that the testing is exhaustive against the code that's expected to be equivalent, e.g. by some automated means to generate tests for all possible cases. |
@dianne, this is a valuable catch, there is nothing to sorry about, it's better to find this before we stabilize the feature, rather than after about your test, the reason i was confident about this part because we had this test compare-if-let-guard which shows that there is not differences between if let inside match's arm and if let guard, but, it doesnt show exactly the problem with dropping match bindings (which i was not knowing about) i analyzed the MIR and can confirm the problem
for some reasons compiler creates guard bindings before pattern bindings
this means pattern bindings would be dropped first (reverse order), causing the dropck issues you identified |
I believe creating guard bindings before pattern bindings is intentional: it's needed for https://doc.rust-lang.org/reference/expressions/match-expr.html#r-expr.match.guard.value. I think the drops will need manual rescheduling to untwist them1; some amount of manual rescheduling is already done for or-patterns, so there's precedent. Ideally I'd like to implement this by specifying the drop order explicitly, but that's soft-blocked on #1421632 (and it might just be easier anyway to use the drop order we get from lowering the guard, but shuffled to happen after the arm's bindings are dropped). Footnotes
|
I'm not sure if the team has already seen #142163, @traviscross, but I wanted to bring it to your attention. It's a fairly significant issue, and if it wasn't brought up at the last meeting a couple of days ago, I think it would be worth putting on the agenda soon (though I'm not entirely familiar with how these meetings are planned) As for the issue itself, I currently see two possible approaches:
I’m not saying this isn’t a real issue — it definitely is — but it seems to be more of an external problem rather than one specific to this feature. I'm not sure what the best path forward for this stabilization PR is, so I just wanted to share the two directions which are the most obvious to me |
That's also my understanding. The only thing that makes There might also be reason to specify and stabilize
Fixing #142163 is a breaking change regardless, but I'd consider it mostly orthogonal to |
I'm curious to hear from @est31 here as well. Even if it's part of a more general issue, it'd be better if possible if we could do something here to avoid increasing the surface area of it, and I'm curious to hear from everyone what the options might be there. |
Separately, I'm curious to hear if there might be some structured way that we could test this to assure ourselves that we really have covered all of the possible drop order cases before the next attempt at stabilization. |
Summary
This proposes the stabilization of
if let
guards (tracking issue: #51114, RFC: rust-lang/rfcs#2294). This feature allowsif let
expressions to be used directly within match arm guards, enabling conditional pattern matching within guard clauses.What is being stabilized
The ability to use
if let
expressions within match arm guards.Example:
Motivation
The primary motivation for
if let
guards is to reduce nesting and improve readability when conditional logic depends on pattern matching. Without this feature, such logic requires nestedif let
statements within match arms:Implementation and Testing
The feature has been implemented and tested comprehensively across different scenarios:
Core Functionality Tests
Scoping and variable binding:
scope.rs
- Verifies that bindings created inif let
guards are properly scoped and available in match armsshadowing.rs
- Tests that variable shadowing works correctly within guardsscoping-consistency.rs
- Ensures temporaries in guards remain valid for the duration of their match armsType system integration:
type-inference.rs
- Confirms type inference works correctly inif let
guardstypeck.rs
- Verifies type mismatches are caught appropriatelyPattern matching semantics:
exhaustive.rs
- Validates thatif let
guards are correctly handled in exhaustiveness analysismove-guard-if-let.rs
andmove-guard-if-let-chain.rs
- Test that conditional moves in guards are tracked correctly by the borrow checkerError Handling and Diagnostics
warns.rs
- Tests warnings for irrefutable patterns and unreachable code in guardsparens.rs
- Ensures parentheses aroundlet
expressions are properly rejectedmacro-expanded.rs
- Verifies macro expansions that produce invalid constructs are caughtguard-mutability-2.rs
- Tests mutability and ownership violations in guardsast-validate-guards.rs
- Validates AST-level syntax restrictionsDrop Order and Temporaries
Key insight: Unlike
let_chains
in regularif
expressions,if let
guards do not have drop order inconsistencies because:drop-order.rs
- Tests that temporaries in guards are dropped at the correct timecompare-drop-order.rs
- Compares drop order betweenif let
guards and nestedif let
in match arms, confirming they behave identically across all editionslet chain
was made by @est31Edition Compatibility
This feature stabilizes on all editions, unlike
let_chains
which was limited to edition 2024. This is safe because:if let
guards don't suffer from the drop order issues that affectedlet_chains
in regularif
expressionsInteractions with Future Features
The lang team has reviewed potential interactions with planned "guard patterns" and determined that stabilizing
if let
guards now does not create obstacles for future work. The scoping and evaluation semantics established here align with what guard patterns will need.Unresolved Issues
All blocking issues have been resolved:
let chains
insideif let
guard is the sameRelated:
if let
guards documentation reference#1823