Suboptimal formatting of `let`-bound chains of `||` with `==` inside them.

[RalfJung]

Input

                    let assign_to_field =
                        context == PlaceContext::MutatingUse(MutatingUseContext::Store)
                        || context == PlaceContext::MutatingUse(MutatingUseContext::Drop)
                        || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput);

Output

                    let assign_to_field = context
                        == PlaceContext::MutatingUse(MutatingUseContext::Store)
                        || context == PlaceContext::MutatingUse(MutatingUseContext::Drop)
                        || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput);

Expected output

                    let assign_to_field =
                        context == PlaceContext::MutatingUse(MutatingUseContext::Store)
                        || context == PlaceContext::MutatingUse(MutatingUseContext::Drop)
                        || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput);

The reformatting actually makes the code look worse (linebreak in the middle of one of the comparisons) without a need (such as a line being too long).

Meta

• This is via ./x.py fmt in current rustc

[calebcartwright]

Though I understand how this formatting isn't ideal, I do believe it's correct given then Style Guide prescriptions for this case that govern how rustfmt formats as well as the max width constraints.

Due to the indentation levels it's not possible to fit the lhs and first rhs line on the same line (let assign_to_field = context == PlaceContext::MutatingUse(MutatingUseContext::Store would exceed max width when starting at that position). And even if the entire rhs was kept together on its own line as indicated in your expected output snippet, the subsequent lines would have to be indented again

                            let assign_to_field =
                                context == PlaceContext::MutatingUse(MutatingUseContext::Store)
                                    || context == PlaceContext::MutatingUse(MutatingUseContext::Drop)
                                    || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput);

(edit - corrected some comments about max width as I think I was working with some incorrect indentation levels at first that did not match the provided snippets)

[joshtriplett]

This does seem to me like formatting we could do somewhat better with, and it doesn't seem impossible to handle. In general, we don't do a great job of detecting multi-line things that are similar and should get line-broken to accent that similarity.

I wonder if this could be improved in rustfmt 2.0?

[RalfJung]

In general, we don't do a great job of detecting multi-line things that are similar and should get line-broken to accent that similarity.

Agreed -- there are also several issues related to match formatting that boil down to this high-level point.

[calebcartwright]

Let me know if I'm missing something obvious, but I still don't see a way in which this could be formatted any differently without either exceeding max_width, or contradicting the rules for let statements and/or the rules for binary expressions.

While it wouldn't necessarily be the way I'd write it instinctively, it seems completely aligned to the rules dictated in the guide.

I fear there's often an underlying theme where folks want rustfmt to ignore the width wrapping rules in specific cases where it might "look better" to ignore the width rule. Although I can understand that perspective in certain circumstances, I worry about the practicalities of something like that which I believe ends up inevitably more subjective than objective.

I wonder if this could be improved in rustfmt 2.0?

I wouldn't count on rustfmt 2.0 ever happening, absent a rustc 2.0 release. The original rfc for rustfmt included text for major/breaking versions, but when we proposed doing such not too long ago it was quite unequivocally rejected by both the dev tools team and a coule folks from the core team as well (though they were speaking for themselves individually)

[RalfJung]

The original example at the top of this issue is within the width both in the actual and expected formatting. I am not sure which rules are violated by what I called the expected formatting, but in my opinion that just goes to shows the rules still need to be tweaked. It is pretty hard to come up with general rules that always look good, so if we discover unintended consequences of those rules I don't think we should be afraid to improve the rules.

[calebcartwright]

I am not sure which rules are violated by what I called the expected formatting

As per this rule:

If line-breaking, put the operator on a new line and block indent. Put each sub-expression on its own line. E.g.

Your expected output is missing that mandatory block indentation, and would instead have to be this:

context == PlaceContext::MutatingUse(MutatingUseContext::Store)
    || context == PlaceContext::MutatingUse(MutatingUseContext::Drop)
    || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput);

With the extra, mandatory, indentation levels resulting in the width excess.

so if we discover unintended consequences of those rules I don't think we should be afraid to improve the rules.

I don't think fear is a factor.

I completely agree with you that it's not feasible to come up with generalized rules that always look good, which is why I don't think that's ever a realistic goal with pretty printer style formatters; it's more of a "good enough in most cases" with an implicit acknowledgement there will be some non-ideal cases.

We still have the stability/idempotence problem where we're simultaneously being asked for breaking formatting changes but prohibited from doing so intentionally. However, if someone's able to articulate a new rule/rule adjustment that's both deterministic and consistent with all other relevant rules then that's certainly a path that can be explored (though that's more of a style guide topic vs. rustfmt topic). In the meantime though, I think it's important to point out when an ask contradicts the existing rules that dictate rustfmt's behavior.