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.

Sign up for GitHub

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

Jump to bottom

docs(style): add let-chain style rules #110568

Closed
wants to merge 1 commit into from
+74 −7
Closed

docs(style): add let-chain style rules #110568

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 74 additions & 7 deletions src/doc/style-guide/src/expressions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,8 +485,11 @@ self.pre_comment.as_ref().map_or(

### Control flow expressions

This section covers `if`, `if let`, `loop`, `while`, `while let`, and `for`
expressions.
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`).
Comment on lines +488 to +492
Copy link
Member Author

Choose a reason for hiding this comment

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

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


The keyword, any initial clauses, and the opening brace of the block should be
on a single line. The usual rules for [block formatting](#blocks) should be
Expand All @@ -512,10 +515,11 @@ if let ... {
}
```

If the control line needs to be broken, then prefer to break before the `=` in
`* let` expressions 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:
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:
Comment on lines +518 to +522
Copy link
Member Author

Choose a reason for hiding this comment

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

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


```rust
while let Some(foo)
Expand All @@ -536,6 +540,70 @@ if a_long_expression
{
...
}

if let Some(a) = b
&& another_long_expression
|| a_third_long_expression
{
...
}

if let Some(relatively_long_thing)
= a_long_expression
&& another_long_expression
|| a_third_long_expression
{
Comment on lines +551 to +555
Copy link
Member Author

Choose a reason for hiding this comment

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

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

...
}

if some_expr
&& another_long_expression
&& let Some(relatively_long_thing)
= a_long_expression
|| a_third_long_expression
{
...
}
```

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
Copy link
Member Author

Choose a reason for hiding this comment

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

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 &&

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 &&"

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

`ident` (which can optionally be preceded by any number of unary prefix operators)
and the second being a single-line `let` clause. Otherwise,
the control line must be broken and formatted according to the above rules. For example:

```rust
if a && let Some(b) = foo() {
// ...
}

let operator = if !from_hir_call && let Some(p) = parent {
// ...
}

if a
|| let Some(b) = foo()
Comment on lines +584 to +585
Copy link
Member Author

Choose a reason for hiding this comment

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

Example referenced above. I do not like the way this looks

{
// ..
}

if let Some(b) = foo()
&& a
{
// ..
}

if foo()
&& let Some(b) = bar
{
// ...
}

if gen_pos != GenericArgPosition::Type
&& let Some(b) = gen_args.bindings.first()
{
// ..
}
```

Where the initial clause is multi-lined and ends with one or more closing
Expand All @@ -554,7 +622,6 @@ if !self.config.file_lines().intersects(
}
```


#### Single line `if else`

Formatters may place an `if else` or `if let else` on a single line if it occurs
Expand Down