-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Stabilize let chains in the 2024 edition #132833
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
Process-wise, we normally do the FCP on the stabilization PR, and I think that'd make the most sense here. Let us know if you need us on lang to weigh in on anything specifically, e.g. any of the open questions, to make such a stabilization PR possible. cc @rust-lang/lang for visibility. And cc @rust-lang/style given the open item on the style guide. @rustbot labels +I-style-nominated Putting on the edition hat, since this would be an edition item in some sense, we should talk about this in our next edition call. @rustbot labels +I-edition-nominated |
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
@traviscross understood, I've converted it to a pull request using the |
Beautiful, thanks. Let's nominate this for lang discussion too then. @rustbot labels +I-lang-nominated |
This comment has been minimized.
This comment has been minimized.
79e1519
to
ee7488a
Compare
@rustbot labels -I-edition-nominated We talked this one through in the edition call. Since there's no breakage or migration here, we're happy for this one to land in Rust 2024 either ahead of or after the release of Rust 2024 itself as long as there is an edition guide chapter for it. |
aa24aa5
to
d57626e
Compare
Thanks @calebcartwright for filing the style guide pull request and guiding the PR towards merging. It's really great news that this feature is (again) doing the last steps towards stabilization. I have rebased the pull request. @fee1-dead if you want you can already take a look now.
There has been a msg in the zulip thread here in January, so I've been aware of that since that point of time. That thread had been in the "Open questions / blockers" section of the pull request description since a long time. In February I've listed the open blockers and also asked for someone to file a PR. I should probably have responded to your msg a week later on March 7 about formatting, probably my msg above hasn't been clear enough. In the end we were one month hard blocked on formatting, which isn't that much in the grand scheme of things (note that this PR is open for 6 months at this point). People have been busy with the edition 2024 release earlier this year which had priority also for the style team. Many of us are free time contributors who don't have this as a day job, me included. |
This comment has been minimized.
This comment has been minimized.
77c404c
to
c373f03
Compare
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
/// The specified `edition` should be that of the whole `if` or `while` construct: the same | ||
/// span that we later decide the drop behaviour on (editions ..=2021 vs 2024..) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The specified `edition` should be that of the whole `if` or `while` construct: the same | |
/// span that we later decide the drop behaviour on (editions ..=2021 vs 2024..) | |
/// The specified `edition` should be that of the whole `if` or `while` | |
/// construct, i.e. the same span we use to later decide whether the drop | |
/// behavior should be that of `..=2021` or that of `2024..`, as this is | |
/// important for let chains. |
(Mostly wanted to add the trailing period, but then reworded a bit for hopefully better clarity.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reworded
// Scoping code checks the top level edition of the `if`: let's match it here. | ||
// Also check all editions in between, just to make sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Scoping code checks the top level edition of the `if`: let's match it here. | |
// Also check all editions in between, just to make sure. | |
// Scoping code checks the top level edition of the `if`; let's | |
// match it here. Also check all editions in between, just to | |
// make sure. |
(Replacing colon with semicolon as is appropriate here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a ; there.
ungate_let_exprs(this, lhs); | ||
} | ||
ExprKind::Let(..) => { | ||
this.psess.gated_spans.ungate_last(sym::let_chains, expr.span) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs for this function say:
Using this is discouraged unless you have a really good reason to.
Perhaps we should add a comment to document our good reason.
Also, since it seems unlikely to me that we'll ever stabilize let chains in Rust ..=2021
-- that is, unless we ever decide to make a breaking change to earlier editions to align the drop order rules -- I wonder if it wouldn't be better to just pull let chains out of older editions entirely and remove use of the feature gate (especially as this commit also reduces test coverage for this feature in older editions).
Alternatively, we could put let chains for older editions under a different feature gate so that we can mark let_chains
itself as stable -- it'd be bit odd still having the let_chains
gate marked as unstable after this PR in rustc_feature
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it wouldn't be better to just pull let chains out of older editions entirely and remove use of the feature gate.
Back when I wrote the PR, the compiler has still been on edition 2021, and 2024 hasn't been stable yet. So it was a hard criterion to support 2021 in some way, too.
Now it's months later, and I guess it's feasible also to nightly users that we can immediately drop support for edition 2021 and below. But I'd still prefer to do that in a followup to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should add a comment to document our good reason.
I wanted to be careful, especially when seeing PRs like #95008 and #98633. But I think with #115677, we probably have good recognition of when some let
is in a disallowed location. So I've made a change to remove ungate_last
usages and to instead not gate in the first place.
// Remove the last feature gating of a `let` expression since it's stable. | ||
self.psess.gated_spans.ungate_last(sym::let_chains, cond.span); | ||
if has_let_expr(&cond) { | ||
// Let chains are allowed in match guards, but only there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Let chains are allowed in match guards, but only there | |
// Let chains are allowed in match guards, but only there. |
Also, perhaps you could elaborate on this comment. It's currently not very clarifying for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the entire comment. the point was that match guards support let chains, but if you have an if
inside a block inside a match guard, let chains aren't supported again.
c373f03
to
767073c
Compare
This comment has been minimized.
This comment has been minimized.
The edition gate is a bit stricter than the drop behaviour, which is fine. The cases we want to avoid are the opposite: not gated but 2021 drop behaviour.
767073c
to
07dccf5
Compare
Also, stabilize while let chains on editions 2021 and before.
07dccf5
to
b4bd8b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I also understanding this correctly that this PR will also stablize the following on all editions?
fn main() {
let x: Option<Option<i32>> = None;
while let Some(x) = x && let Some(_) = x {}
}
Has there been any discussion to stabilize that? My understanding is that only 2024 was discussed and FCP'd, but maybe I missed something. If only 2024 was discussed then we probably can't stabilize any form of let chains with edition < 2024.
/// Whether let chains are allowed on all editions, or it's edition dependent. | ||
/// In case of edition dependence, specify the currently present edition. | ||
pub enum LetChainsPolicy { | ||
AlwaysAllowed, | ||
Edition(Edition), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Whether let chains are allowed on all editions, or it's edition dependent. | |
/// In case of edition dependence, specify the currently present edition. | |
pub enum LetChainsPolicy { | |
AlwaysAllowed, | |
Edition(Edition), | |
} | |
/// Whether let chains are allowed on all editions, or it's edition dependent. | |
/// In case of edition dependence, specify the currently present edition. | |
pub enum LetChainsContext { | |
AlwaysAllowed, | |
EditionDependent { current_edition: Edition }, | |
} |
When you write it as LetChainsPolicy
it feels like the edition that you are passing in is some sort of condition that must be passed, as compared to the current edition. I believe this change would help clarify things better.
Stabilization report
This proposes the stabilization of
let_chains
(tracking issue, RFC 2497) in the 2024 edition of Rust.What is being stabilized
The ability to
&&
-chainlet
statements insideif
andwhile
is being stabilized, allowing intermixture with boolean expressions. The patterns inside thelet
sub-expressions can be irrefutable or refutable.The feature will only be stabilized for the 2024 edition and future editions. Users of past editions will get an error with a hint to update the edition.
closes #53667
Why 2024 edition?
Rust generally tries to ship new features to all editions. So even the oldest editions receive the newest features. However, sometimes a feature requires a breaking change so much that offering the feature without the breaking change makes no sense. This occurs rarely, but has happened in the 2018 edition already with
async
andawait
syntax. It required an edition boundary in order forasync
/await
to become keywords, and the entire feature foots on those keywords.In the instance of let chains, the issue is the drop order of
if let
chains. If we wantif let
chains to be compatible withif let
, drop order makes it hard for us to generate correct MIR. It would be strange to have different behaviour forif let ... {}
andif true && let ... {}
. So it's better to stay consistent withif let
.In edition 2024, drop order changes have been introduced to make
if let
temporaries be lived more shortly. These changes also affectedif let
chains. These changes make sense even if you don't take theif let
chains MIR generation problem into account. But if we want to use them as the solution to the MIR generation problem, we need to restrict let chains to edition 2024 and beyond: for let chains, it's not just a change towards more sensible behaviour, but one required for correct function.Introduction considerations
As edition 2024 is very new, this stabilization PR only makes it possible to use let chains on 2024 without that feature gate, it doesn't mark that feature gate as stable/removed. I would propose to continue offering the
let_chains
feature (behind a feature gate) for a limited time (maybe 3 months after stabilization?) on older editions to allow nightly users to adopt edition 2024 at their own pace. After that, the feature gate shall be marked as stabilized, not removed, and replaced by an error on editions 2021 and below.Implementation history
let_chains
in Rust 1.64 #94927let_else
does not interact withlet_chains
#94974let_chains
] Forbidlet
inside parentheses #95008let
s in certain places #97295let_chains
blocker #98633;
for;
within let-chains #117743{
in let-chains #117770let
or==
on typo'd let-chain #118191irrefutable_let_patterns
on leading patterns ifelse if
let-chains #129394let_chains
references reference#1179Adoption history
In the compiler
Outside of the compiler
Tests
Intentional restrictions
partially-macro-expanded.rs
,macro-expanded.rs
: it is possible to use macros to expand to both the pattern and the expression inside a let chain, but not to the entirelet pat = expr
operand.parens.rs
:if (let pat = expr)
is not allowed in chainsensure-that-let-else-does-not-interact-with-let-chains.rs
:let...else
doesn't support chaining.Overlap with match guards
move-guard-if-let-chain.rs
: test for theuse moved value
error working well in match guards. could maybe be extended with let chains that have more than onelet
shadowing.rs
: shadowing in if let guards works as expectedast-validate-guards.rs
: let chains in match guards require the match guards feature gateSimple cases from the early days
PR #88642 has added some tests with very simple usages of
let else
, mostly as regression tests to early bugs.then-else-blocks.rs
ast-lowering-does-not-wrap-let-chains.rs
issue-90722.rs
issue-92145.rs
Drop order/MIR scoping tests
issue-100276.rs
: let expressions on RHS aren't terminating scopesdrop_order.rs
: exhaustive temporary drop order test for various Rust constructs, including let chainsscope.rs
: match guard scoping testdrop-scope.rs
: another match guard scoping test, ensuring that temporaries in if-let guards live for the armdrop_order_if_let_rescope.rs
: if let rescoping on edition 2024, including chainsmir_let_chains_drop_order.rs
: comprehensive drop order test for let chains, distinguishes editions 2021 and 2024.issue-99938.rs
,issue-99852.rs
both bad MIR ICEs fixed by #102394Linting
irrefutable-lets.rs
: trailing and leading irrefutable let patterns get linted for, others don't. The lint is turned off forelse if
.issue-121070-let-range.rs
: regression test for false positive of the unused parens lint, precedence requires the()
s hereParser: intentional restrictions
disallowed-positions.rs
:let
in expression context is rejected everywhere except at the top levelinvalid-let-in-a-valid-let-context.rs
: nestedlet
is not allowed (let's are no legal expressions just because they are allowed inif
andwhile
).Parser: recovery
issue-103381.rs
: Graceful recovery of incorrect chaining ofif
andif let
semi-in-let-chain.rs
: Ensure that stray;
s in let chains give nice errors (if_chain!
users might be accustomed to;
s)deli-ident-issue-1.rs
,brace-in-let-chain.rs
: Ensure that stray unclosed{
s in let chains give nice errors and hintsMisc
conflicting_bindings.rs
: the conflicting bindings check also works in let chains. Personally, I'd extend it to chains with multiple let's as well.let-chains-attr.rs
: attributes work on let chainsTangential tests with
#![feature(let_chains)]
if-let.rs
: MC/DC coverage tests for let chainslogical_or_in_conditional.rs
: not really about let chains, more about dropping/scoping behaviour of||
stringify.rs
: exhaustive test of thestringify
macroexpanded-interpolation.rs
,expanded-exhaustive.rs
: Exhaustive test of-Zunpretty
diverges-not.rs
: Never type, mostly tangential to let chainsPossible future work
if let Pat(bindings) = expr {}
to be written asif expr is Pat(bindings) {}
(RFC 3573).if let
chains are a natural extension of the already existingif let
syntax, and I'd argue orthogonal towardsis
syntax.let
-chains andis
are not mutually exclusive lang-team#297let ... else
statements. There is no proposed RFC for this however, nor is it implemented on nightly.if
keyword as well, but on stable Rust, they don't supportlet
. The functionality is available via an unstable feature (if_let_guard
tracking issue). Stabilization of let chains affects this feature in so far as match guards containing let chains now only need theif_let_guard
feature gate be present instead of also thelet_chains
feature (NOTE: this PR doesn't implement this simplification, it's left for future work).Open questions / blockers
let
(I don't think this is a blocker): #117977move-guard-if-let-chain.rs
andconflicting_bindings.rs
to have chains with multiple let's: done in 133093let_else
. I think we can live withlet pat = expr
not evaluating asexpr
for macro_rules macros, especially given thatlet pat = expr
is not a legal expression anywhere except insideif
andwhile
.let_chains
again reference#1740