dart-lang / dart_style

An opinionated formatter/linter for Dart code
https://pub.dev/packages/dart_style
BSD 3-Clause "New" or "Revised" License
646 stars 121 forks source link

Avoid gratuitous splits after `=`. #1554

Closed munificent closed 2 months ago

munificent commented 2 months ago

This makes two changes that improve how assignments are formatted:

  1. If a comment is before a cascade setter, don't force the setter to split.

Prior to this change, if you had:

target
  // Comment
  ..setter = value;

The formatter would give you:

target
  // Comment
  ..setter =
      value;

It attached the comment to the ..setter, which was the LHS of the assignment. Then, because there was a newline inside the LHS, it forced a split at the =. Instead, we hoist those leading comments out, similar to how we handle binary operators. In fact, I did some refactoring to get rid of duplicate code in InfixPiece that handled leading comments.

  1. If the LHS of an assignment is a call chain, don't force the assignment to split if the chain does.

In the process of fixing 1, I ran into a similar problem. If you had:

target
    // Comment
    .setter = value;

The formatter would give you:

target
    // Comment
    .setter =
        value;

However, the solution is different in this case. With cascade setters, the target of the = is just the ..setter. With a non-cascade setter, the target is the entire target // Comment \n.setter part. That does contain a newline, and we can't hoist the comment out of the assignment because the target really is the entire target // Comment \n.setter expression.

Instead, we treat call chains on the LHS of assignments as "block formattable". "Block" is kind of a misnomer here because what it really means is "allow a newline without splitting at the = or increasing indentation". I can't think of a good term for that.

That change fixes the above example, but also generally improves how setter calls are formatted when the target is a large call chain expression like:

// Before this PR:
target.method(
      argument,
    ).setter =
    value;

// After:
target.method(argument)
    .setter = value;

This second change isn't entirely... principled? But I tested on a giant corpus and when it kicks in, it invariably produces nicer output.

Fix #1429.