docs(style): add let-chain style rules #110568
Conversation
|
r? @yaahc (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Have opened this as a draft intentionally, both to ensure this accurately reflects the position we came to over several meetings, and to facilitate some discussion around subsequent thoughts and questions I kept waffling on while working on this (added inline below) |
| This section covers `for` and `loop` expressions, as well as `if` and `while` | ||
| expressions with their sub-expression variants. This includes those with a | ||
| single `let` sub-expression (i.e. `if let` and `while let`) | ||
| as well as "let-chains": those with one or more `let` sub-expressions and | ||
| one or more bool-type conditions (i.e. `if a && let Some(b) = c`). |
There was a problem hiding this comment.
Was ambivalent and self-argumentative around whether it would be more fluid to incorporate the new rules into the existing narratives or to create a net-new section. I eventually landed on the former as I thought it read more naturally and allowed for greater reuse of existing rule prose.
I do not feel strongly, and could be convinced otherwise pretty easily
| If the control line needs to be broken, then prefer breaking before the `=` for any | ||
| `let` sub-expression in an `if` or `while` expression that does not fit, | ||
| and before `in` in a `for` expression; the following line should be block indented. | ||
| If the control line is broken for any reason, then the opening brace should be on its | ||
| own line and not indented. Examples: |
There was a problem hiding this comment.
This is one of the aspects that's been a struggle for me, as I feel like there's some important missing text here even without the additions for let-chains.
Specifically, what happens of the pattern itself ends up needing to be formatted across multiple lines?
The rules for let statements cover this scenario, and I imagine (though didn't check) rustfmt would do the same for a non-chained if let. However, the written rules here for expressions do already differ from those for statements (notably whether to break before or after the assignment operator) so I think this is an opportunity for increased clarity and explicitness we should consider for the 2024 edition, if not earlier.
I was tempted to create a new section that covered the let expressions explicitly, but ultimately decided it would be ideal if in the future we could consolidate the rules between the statement and expression contexts, so have punted for now
| if let Some(relatively_long_thing) | ||
| = a_long_expression | ||
| && another_long_expression | ||
| || a_third_long_expression | ||
| { |
There was a problem hiding this comment.
I think this is a good case for breaking after the assignment operator (rustfmt's current, technically buggy behavior in these control flow expressions, as well as the current behavior and rules for statements). I feel this is technically consistent with current rules, but IMO it would benefit from having the different indentation/spacing for the a_long_expression line instead of the assignment operator in the let expr being the same indent as the binops for the subsequent clauses in the chain
| ``` | ||
|
|
||
| A let-chain control line is allowed to be formatted on a single line provided | ||
| it only consists of two clauses, separated by `&&`, with the first being an |
There was a problem hiding this comment.
We'd explicitly documented the desire to limit this to &&, so I captured it as such here. I actually don't know whether any other binops are supported within chains, but I think it would be in our best interest to not scope the binops here unless we feel strongly that && vs || should make a difference. I included an example with || below to demonstrate that the concerning scenario I raised against style option 2 (which led us to the eventual option 5 captured in this PR) could still exist if we restrict to &&
There was a problem hiding this comment.
I actually don't know whether any other binops are supported within chains
|| isn't allowed in a chain if there are any lets. Just FYI, the examples shown here aren't valid due to that rule.
There was a problem hiding this comment.
Thanks that's helpful. I think one consideration though is what constitutes valid in the style/formatting context. I.e. at what stage does this get rejected, will it cause the parser to error out prior to creating the AST or does it occur later? Is there a potential for the supported operators to change in the future?
There's existing precedent within the Style Guide that covers "invalid" flavors of syntax, e.g.
https://github.com/rust-lang/rust/blob/master/src/doc/style-guide/src/items.md#where-clauses
// Note that where clauses on `type` aliases are not enforced and should not
// be used.
type Foo<T>
where
T: Bound
= Bar<T>;
Which I always assume was the case because rustfmt has to account for it so long as the AST can still be produced. If || is rejected during lexing/parsing and we don't think that'll ever change then I'm happy to remove it, but otherwise my inclination would be to update the examples to remove the || while also relaxing the rule to remove the explicit "separated by &&"
There was a problem hiding this comment.
The validation is not done during parsing. It is in the AST validator (which IIRC, is done just after expansion).
The RFC mentioned that || might be possible in the future, but I'm not aware of any serious consideration for it currently, and I suspect the addition would require another RFC.
| if a | ||
| || let Some(b) = foo() |
There was a problem hiding this comment.
Example referenced above. I do not like the way this looks
|
cc @rust-lang/style as I guess the team mentions don't go out on draft PRs |
|
Will update this to reflect our plans for where/how we capture nightly-only syntax (#111119) I'm also going to modify the snippets to remove the |
|
☔ The latest upstream changes (presumably #113911) made this pull request unmergeable. Please resolve the merge conflicts. |
|
We discussed this in the style team triage meeting. It's waiting on draft -- not sure if @calebcartwright is looking for any review here, and afaict the only question was "how do we deal with @rustbot author |
rustbot
added
S-waiting-on-author
|
Closing because:
|
|
|
||
| A let-chain control line is allowed to be formatted on a single line provided | ||
| it only consists of two clauses, separated by `&&`, with the first being an | ||
| `ident` (which can optionally be preceded by any number of unary prefix operators) |
There was a problem hiding this comment.
I think we should change this to "ident or literal", so that we don't break after false && or true &&.
|
|
||
| A let-chain control line is allowed to be formatted on a single line provided | ||
| it only consists of two clauses, separated by `&&`, with the first being an | ||
| `ident` (which can optionally be preceded by any number of unary prefix operators) |
There was a problem hiding this comment.
style team meeting change/addition discussed: ident or lit
No description provided.