Wrapping rules for let-chains?

[camsteffen]

Feature tracking issue: rust-lang/rust#53667

Should we introduce a new guideline for wrapping let-chains?

The rationale is that let-chains allow let bindings to be anywhere in a line of code. let is no longer consitently at the left side. It is good for let bindings to stand out and be easy to discover since they introduce new variables for the following scope. Oftentimes when reading code, I look backwards with the question "where did this variable come from?"

Possible answers

1. No change. Wrap let expressions the same as any other expression.

2. Always wrap let expressions. let is only ever preceded by one token in the same line: if, while, &&, ||, etc.

3. Wrap let expressions if they begin N characters or more past the current indentation.

   For example, a let-expression may be on the same line if it is 15 characters from the left margin. If it is 16 characters from the left margin, it must break to the next line.

4. Wrap let expressions if there are 2 (?) or more let expressions in the condition

5. Allow single_identifier && let on one line (if it fits line length limits), but require a line break before the && for any other expression. Always wrap after a let expression.

Potential concerns for each answer

1. The introduction of x may be hard to find in this case:

   if my_long_condition(foo, bar, baz) && some_more && let Some(x) = foo() {
       // lots of code...
       use_x(x);

2. Wrapping for this case seems a little unnecessary since it only shifts let leftward by one character:

   if a
       && let Some(b) = foo()
   {

3. Added complexity to formatting logic. Difficult to follow/verify the guideline without automated tooling. The subtlety of the logic may be surprising to users.
4. This case is suboptimal (similar to 1):

   if my_function(Some(get_thing()), variable, and_another) && let Some(x) = foo() {

Other considerations

The rules for if and while will be the same.

If a let expression wraps, it will cause all of the && or ||-separated expressions to be on separate lines.

if something
    && let Some(x) = foo() && something_else { // bad - `&& something_else` should be on its own line

If there is just one let binding at the beginning, the condition may be on one line:

if let Some(foo) = do_something() && another_condition {

[ytmimi]

What if the wrapping rules had more to do with the number of items in the chain? For example, something along the lines of "if there are two or more Let expressions in the chain than always wrap". That way each Let expression gets its own line and will be more readable. If there's just a single Let it's probably easy enough to figure out what's going on, and if we need to wrap the existing wrapping rules should cover us.

So this:

if let Some(ref first) = opt && let second = first {
}

Would always become this:

if let Some(ref first) = opt
    && let second = first
{
}

But your earlier example, would remain unbroken:

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

A similar rule could be introduced that states if there are 3 or more items in the chain and the Let expression isn't the first chain item then always break. So we'd break in both of the following cases:

// Let at the end
if my_long_condition(foo, bar, baz)
    && some_more
    && let Some(x) = foo()
{
    // lots of code...
    use_x(x);
}

// Let in the middle
if my_long_condition(foo, bar, baz)
  && let Some(x) = foo()
  && some_more
{
    // lots of code...
    use_x(x);
}

But we wouldn't break here:

if let Some(x) = foo() && my_long_condition(foo, bar, baz) && some_more {
}

One might argue that it's still hard to understand where x is coming from in the above so if the rule were amended to be "If there are 3 or more items in the chain and there is at least 1 Let expression then always break", then we would break:

if let Some(x) = foo()
  && my_long_condition(foo, bar, baz)
  && some_more
{
}

I agree that these rules might be confusing to some users, but I think the improved readability is also important to consider.

Let me know your thoughts!

[camsteffen]

What if the wrapping rules had more to do with the number of items in the chain?

I think the horizontal position of let is the important thing, not the number of lets. I wouldn't gamble that the following case is rare:

if my_function(Some(get_thing()), variable, and_another) && let Some(x) = foo() {

edit: I'll add this idea to the OP

[terhechte]

What about breaking after the && so that the lets line up nicely?

// Let in the middle
if let Some(first) = my_long_condition(foo, bar, baz) &&
   let Some(x) = foo() &&
   some_more
{
    // lots of code...
    use_x(x);
}

This would also improve (at least a bit) the following case

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

[camsteffen]

What about breaking after the && so that the lets line up nicely?

This would be a diversion from the current style guide on Binary Operations, and I don't think changing that is an option at this point. Also that would not resolve the main concern here - the case where all the conditions fit on one line.

[calebcartwright]

Thanks for sharing your perspective @terhechte! @camsteffen is correct about the binops though, so that wouldn't be an available option anyway

[joshtriplett]

EDIT: Ignore this, it's already implied by option 2 together with other existing formatting rules.

I'd like to propose two additional variations on option (2):

2a) Option 2, with an additional requirement: always break before and after every let expression, such that let pat = expr is only ever preceded by one token in the same line (if, while, &&) and a boolean expression never appears after it:

if condition()
    && let Some(binding) = expr
    && condition2(binding) && condition3()
    && let Some(binding2) = expr2
    && condition4(binding, binding2)
{
    body();
}

2b) When a let expression is present anywhere in a condition, always break each top-level expression (whether let or boolean) onto its own line:

if condition()
    && let Some(binding) = expr
    && condition2(binding)
    && condition3()
    && let Some(binding2) = expr2
    && condition4(binding, binding2)
{
    body();
}

Of these two, (2a) would be more consistent with how we currently format boolean expressions, while (2b) seems preferable in general (and I'd propose doing that for all boolean expressions that require breaking if we ever have a rustfmt change over an edition, but that's unrelated to this issue).

[camsteffen]

I thought we already break all binary operations if there are any line breaks?

[joshtriplett]

Ah, it would appear that we do, thank you.

In that case, I think this reduces to option 2.

[calebcartwright]

While I understand the angle around visual scanning, I'm concerned that option 2 (always wrap) is a bit too blunt an instrument where something more surgical would be helpful.

A few questions/things to note

• The formatting of the potential concern for option 2 isn't correct because it would fail to comply with the If the control line is broken for any reason, then the opening brace should be on its own line and not indented. clause for control flow expressions. It would instead have to be formatted as below. Obviously subjective, but this makes me grimace:

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

• I understand the scenario that option 2 is trying to guard against (vertical scanning up through the block and then horizontal scanning to the end of the control line), however, do we really think that type of scenario will be more common than the simpler type shown above?
• The existing rules in relevant contexts where these will typically appear (e.g. guards, control flows) include their own wrapping rules. So while the scenario option 2 protects against can, and certainly will, occur, it does require somewhat of a perfect storm wherein it's long enough to start being a scanning challenge, but too short to trigger those other wrapping rules (and obviously every expression in the control line would need to be single-line formattable too)

[camsteffen]

The formatting of the potential concern for option 2 isn't correct

Good point! Fixed.

it does require somewhat of a perfect storm

Yeah that may be true (it's unusual for let to actually end up very far to the right). But in a way that may be a bad thing. Your brain might get used to seeing let on the left side cause that's how it usually works out, but in the unusual case that it isn't, you might miss it. It would be interesting see statistics on that. In real code using let-chains, what is the distribution of the horizontal position of let?

[calebcartwright]

In real code using let-chains, what is the distribution of the horizontal position of let?

Agreed! Not sure there's a good way to systematically inspect, but even getting feedback from individuals/teams would be helpful.

I should also note that I'm absolutely open to incorporating a config option in rustfmt to give users some flexibility over this aspect of formatting, though obviously the default behavior remains an important question.

I suppose there's also some variants on option 3, but which could incorporate factors like the total length of the control line and/or the number of elements, when let exprs are present (e.g. wrap the control line if the control line contains one or more let expressions unless the control line is not "small"). These types of rules do indeed have a higher cognitive load, but we do also have some fairly longstanding precedents e.g. various "small" measures, and chains to a degree as well

[c410-f3r]

If you guys want to customize the formatting of let chains, then option 3 is the most flexible and satisfies most scenarios/objectives.