Skip to content
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

Document let_chains again #1740

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Document let_chains again #1740

wants to merge 4 commits into from

Conversation

est31
Copy link
Member

@est31 est31 commented Feb 24, 2025

Mostly a reapply of #1179 which was reverted by #1251. The reapply is not trivial as in the meantime there has been #1561 which caused merge conflicts. Also add some modifiers.

Tracking issue: rust-lang/rust#53667
Stabilization report: rust-lang/rust#132833

@rustbot rustbot added the S-waiting-on-review Status: The marked PR is awaiting review from a maintainer label Feb 24, 2025
@ehuss ehuss added the S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository label Feb 25, 2025
@ehuss
Copy link
Contributor

ehuss commented Mar 6, 2025

Some notes about temporaries and drop order:

TC's tests added to rust-lang/rust: https://github.com/rust-lang/rust/pull/133605/files

TC's more up-to-date example playground: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=0ce26b5398487e95672adbf389ad946b

Niko's writeup: rust-lang/rust#132833 (comment)

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scopes

I think the rule in scopes.md labeled names.scopes.pattern-bindings.if-let needs to be updated. My rough understanding is that the scope of an if let binding extends into the following _IfCondition_s and the consequent block.

Edition

At the bottom of if-expr.md, can you add something like:

r[expr.if.edition2024]
> **Edition differences**: Before the 2024 edition, let chains are not supported and only a single _IfCondition_ is allowed in an `if` expression.

I think it makes sense to be at the end of the "Chains of conditions" section? Also just want to double-check the accuracy here.

Destructors and drop scopes

I'd like to see some updates to the destructors page to explain the drop scopes affected by this change. I'm uncertain exactly which changes to make, but I think roughly they are:

  • destructors.scope.desugaring -- if let (now just if) can't be directly converted to a match expression, so I don't know how to phrase that.
  • destructors.scope.temporary.enclosing -- if let should be rephrased somehow. Perhaps it could be phrased as just if, and somewhere say that multiple _IfCondition_s are translated to the equivalent nested if with single conditions?
    • Or, somehow word it in a similar way to how lazy boolean expressions are worded?
  • destructors.scope.temporary.edition2024 -- I'm uncertain if this needs rewording?

Comment on lines 16 to 17
> &nbsp;&nbsp; &nbsp;&nbsp; [_Expression_]<sub>_except struct expression_</sub>\
> &nbsp;&nbsp; | `let` [_Pattern_] `=` [_Scrutinee_]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expression cannot be a lazy-boolean, correct?

(Scrutinee already says it doesn't allow struct. It's a little redundant to say that here, but maybe it helps with clarity?)

Suggested change
> &nbsp;&nbsp; &nbsp;&nbsp; [_Expression_]<sub>_except struct expression_</sub>\
> &nbsp;&nbsp; | `let` [_Pattern_] `=` [_Scrutinee_]
> &nbsp;&nbsp; &nbsp;&nbsp; [_Expression_]<sub>_except struct expression or lazy boolean expression_</sub>\
> &nbsp;&nbsp; | `let` [_Pattern_] `=` [_Scrutinee_]<sub>_except struct expression or lazy boolean expression_</sub>
>
> [^if-condition-2024]: Editions before 2024 only allow a single _IfCondition_.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expression cannot be a lazy-boolean, correct?

yeah, no || is allowed here. Only &&. see disallowed-positions.rs

An `if` expression is a conditional branch in program control.
The syntax of an `if` expression is a condition operand, followed by a consequent block, any number of `else if` conditions and blocks, and an optional trailing `else` block.
The syntax of an `if` expression is a sequence of one or more condition operands separated by `&&`,
followed by a consequent block, any number of `else if` conditions and blocks, and an optional trailing `else` block.

r[expr.if.condition-bool]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggesting to rename this, since the condition can now have let in addition to boolean expressions.

Suggested change
r[expr.if.condition-bool]
r[expr.if.condition]

> &nbsp;&nbsp; (`else` ( [_BlockExpression_] | _IfExpression_ ) )<sup>\?</sup>
>
> _IfConditions_ :\
> &nbsp;&nbsp; _IfCondition_ ( && _IfCondition_ )*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> &nbsp;&nbsp; _IfCondition_ ( && _IfCondition_ )*
> &nbsp;&nbsp; _IfCondition_ ( && _IfCondition_ )*[^if-condition-2024]

@ehuss ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: The marked PR is awaiting review from a maintainer labels Mar 13, 2025
@est31
Copy link
Member Author

est31 commented Mar 17, 2025

Thanks for the review. Applied your suggestions and addressed the other review points as well.

if let (now just if) can't be directly converted to a match expression, so I don't know how to phrase that.

When I read that sentence, I thought it's fine, because there is probably a way to express everything with nested match plus break from loops. But it's a bit underspecified, so I made it a little bit more explicit that it's not just a straightforward translation to match.

I made the new sentence suggest the new meaning a bit more clearly, but it's also now stating a bit of a tautology.

  • destructors.scope.temporary.edition2024 -- I'm uncertain if this needs rewording?

I think it's fine. One could swap out the if let with some longer wording, but it already feels complicated enough for me, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants