Avoid gratuitous splits after =. (#1554)

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:

```dart
target
  // Comment
  ..setter = value;
```

The formatter would give you:

```dart
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.

2. 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:

```dart
target
    // Comment
    .setter = value;
```

The formatter would give you:

```dart
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:

```dart
// 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.

Author: munificent

lib/src/back_end/solver.dart

@@ -134,7 +134,7 @@ class Solver {
     if (debug.traceSolver) {
       debug.unindent();
       debug.log(debug.bold('Solved $root to $best'));
-      debug.log(solution.code.toDebugString());
+      debug.log(best.code.toDebugString());
       debug.log('');
     }
 

lib/src/front_end/ast_node_visitor.dart

@@ -132,7 +132,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
 
   @override
   void visitAdjacentStrings(AdjacentStrings node) {
-    var piece = InfixPiece(const [], node.strings.map(nodePiece).toList(),
+    var piece = InfixPiece(node.strings.map(nodePiece).toList(),
         indent: node.indentStrings);
 
     // Adjacent strings always split.
@@ -366,7 +366,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
       }
     }
 
-    var piece = InfixPiece(leadingComments, operands);
+    var piece = InfixPiece(operands);
 
     // If conditional expressions are directly nested, force them all to split,
     // both parents and children.
@@ -376,7 +376,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
       piece.pin(State.split);
     }
 
-    pieces.add(piece);
+    pieces.add(prependLeadingComments(leadingComments, piece));
   }
 
   @override
@@ -1766,8 +1766,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
             var patternPiece = nodePiece(member.guardedPattern.pattern);
 
             if (member.guardedPattern.whenClause case var whenClause?) {
-              pieces.add(
-                  InfixPiece(const [], [patternPiece, nodePiece(whenClause)]));
+              pieces.add(InfixPiece([patternPiece, nodePiece(whenClause)]));
             } else {
               pieces.add(patternPiece);
             }

lib/src/front_end/chain_builder.dart

@@ -75,9 +75,29 @@ class ChainBuilder {
 
       // When [_root] is a cascade, the chain is the series of cascade sections.
       for (var section in cascade.cascadeSections) {
-        var piece = _visitor.nodePiece(section);
+        // Hoist any leading comment out so that if the cascade section is a
+        // setter, we don't unnecessarily split at the `=` like:
+        //
+        //     target
+        //       // comment
+        //         ..setter =
+        //             value;
+        var leadingComments =
+            _visitor.pieces.takeCommentsBefore(section.firstNonCommentToken);
+
+        var piece = _visitor.prependLeadingComments(
+            leadingComments, _visitor.nodePiece(section));
 
         var callType = switch (section) {
+          // Force the cascade to split if there are leading comments before
+          // the cascade section to avoid:
+          //
+          //     target// comment
+          //     ..method(
+          //       argument,
+          //     );
+          _ when leadingComments.isNotEmpty => CallType.unsplittableCall,
+
           // If the section is itself a method chain, then force the cascade to
           // split if the method does, as in:
           //

lib/src/front_end/piece_factory.dart

@@ -12,6 +12,7 @@ import '../piece/control_flow.dart';
 import '../piece/for.dart';
 import '../piece/if_case.dart';
 import '../piece/infix.dart';
+import '../piece/leading_comment.dart';
 import '../piece/list.dart';
 import '../piece/piece.dart';
 import '../piece/sequence.dart';
@@ -284,6 +285,15 @@ mixin PieceFactory {
     return builder.build();
   }
 
+  /// If [leadingComments] is not empty, returns [piece] wrapped in a
+  /// [LeadingCommentPiece] that prepends them.
+  ///
+  /// Otherwise, just returns [piece].
+  Piece prependLeadingComments(List<Piece> leadingComments, Piece piece) {
+    if (leadingComments.isEmpty) return piece;
+    return LeadingCommentPiece(leadingComments, piece);
+  }
+
   /// Writes the leading keyword and parenthesized expression at the beginning
   /// of an `if`, `while`, or `switch` expression or statement.
   void writeControlFlowStart(Token keyword, Token leftParenthesis,
@@ -853,7 +863,7 @@ mixin PieceFactory {
         switch (combinatorNode) {
           case HideCombinator(hiddenNames: var names):
           case ShowCombinator(shownNames: var names):
-            clauses.add(InfixPiece(const [], [
+            clauses.add(InfixPiece([
               tokenPiece(combinatorNode.keyword),
               for (var name in names) tokenPiece(name.token, commaAfter: true),
             ]));
@@ -921,7 +931,8 @@ mixin PieceFactory {
       pieces.visit(right);
     });
 
-    pieces.add(InfixPiece(leadingComments, [leftPiece, rightPiece]));
+    pieces.add(prependLeadingComments(
+        leadingComments, InfixPiece([leftPiece, rightPiece])));
   }
 
   /// Writes a chained infix operation: a binary operator expression, or
@@ -973,7 +984,8 @@ mixin PieceFactory {
       traverse(node);
     }));
 
-    pieces.add(InfixPiece(leadingComments, operands, indent: indent));
+    pieces.add(prependLeadingComments(
+        leadingComments, InfixPiece(operands, indent: indent)));
   }
 
   /// Writes a [ListPiece] for the given bracket-delimited set of elements.
@@ -1225,7 +1237,7 @@ mixin PieceFactory {
       var clauses = <Piece>[];
 
       void typeClause(Token keyword, List<AstNode> types) {
-        clauses.add(InfixPiece(const [], [
+        clauses.add(InfixPiece([
           tokenPiece(keyword),
           for (var type in types) nodePiece(type, commaAfter: true),
         ]));
@@ -1321,6 +1333,26 @@ mixin PieceFactory {
     //      element,
     //    ];
     canBlockSplitLeft |= switch (leftHandSide) {
+      // Treat method chains and cascades on the LHS as if they were blocks.
+      // They don't really fit the "block" term, but it looks much better to
+      // force a method chain to split on the left than to try to avoid
+      // splitting it and split at the assignment instead:
+      //
+      //    // Worse:
+      //    target.method(
+      //          argument,
+      //        ).setter =
+      //        value;
+      //
+      //    // Better:
+      //    target.method(argument)
+      //        .setter = value;
+      //
+      MethodInvocation() => true,
+      PropertyAccess() => true,
+      PrefixedIdentifier() => true,
+
+      // Otherwise, it must be an actual block construct.
       Expression() => leftHandSide.canBlockSplit,
       DartPattern() => leftHandSide.canBlockSplit,
       _ => false

lib/src/piece/infix.dart

@@ -9,27 +9,6 @@ import 'piece.dart';
 ///
 ///     a + b + c
 class InfixPiece extends Piece {
-  /// Pieces for leading comments that appear before the first operand.
-  ///
-  /// We hoist these comments out from the first operand's first token so that
-  /// a newline in these comments doesn't erroneously force the infix operator
-  /// to split. For example:
-  ///
-  ///     value =
-  ///         // comment
-  ///         a + b;
-  ///
-  /// Here, the `// comment` will be hoisted out and stored in
-  /// [_leadingComments] instead of being a leading comment in the [CodePiece]
-  /// for `a`. If we left the comment in `a`, then the newline after the line
-  /// comment would force the `+` operator to split yielding:
-  ///
-  ///     value =
-  ///         // comment
-  ///         a +
-  ///             b;
-  final List<Piece> _leadingComments;
-
   /// The series of operands.
   ///
   /// Since we don't split on both sides of the operator, the operators will be
@@ -41,26 +20,16 @@ class InfixPiece extends Piece {
   /// Whether operands after the first should be indented if split.
   final bool _indent;
 
-  InfixPiece(this._leadingComments, this._operands, {bool indent = true})
-      : _indent = indent;
+  InfixPiece(this._operands, {bool indent = true}) : _indent = indent;
 
   @override
   List<State> get additionalStates => const [State.split];
 
   @override
-  bool allowNewlineInChild(State state, Piece child) {
-    if (state == State.split) return true;
-
-    // Comments before the operands don't force the operator to split.
-    return _leadingComments.contains(child);
-  }
+  bool allowNewlineInChild(State state, Piece child) => state == State.split;
 
   @override
   void format(CodeWriter writer, State state) {
-    for (var comment in _leadingComments) {
-      writer.format(comment);
-    }
-
     if (_indent) writer.pushIndent(Indent.expression);
 
     for (var i = 0; i < _operands.length; i++) {
@@ -78,7 +47,6 @@ class InfixPiece extends Piece {
 
   @override
   void forEachChild(void Function(Piece piece) callback) {
-    _leadingComments.forEach(callback);
     _operands.forEach(callback);
   }
 

lib/src/piece/leading_comment.dart

@@ -0,0 +1,46 @@
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+import '../back_end/code_writer.dart';
+import 'piece.dart';
+
+/// A piece for a series of leading comments preceding some other piece.
+///
+/// We use this and hoist comments out from the inner piece so that a newline
+/// in the comments doesn't erroneously force the inner piece to split. For
+/// example, if comments preceding an infix operator's left operand:
+///
+///     value =
+///         // comment
+///         a + b;
+///
+/// Here, the `// comment` will be hoisted out and stored in a
+/// [LeadingCommentPiece] instead of being a leading comment in the [CodePiece]
+/// for `a`. If we left the comment in `a`, then the newline after the line
+/// comment would force the `+` operator to split yielding:
+///
+///     value =
+///         // comment
+///         a +
+///             b;
+class LeadingCommentPiece extends Piece {
+  final List<Piece> _comments;
+  final Piece _piece;
+
+  LeadingCommentPiece(this._comments, this._piece);
+
+  @override
+  void format(CodeWriter writer, State state) {
+    for (var comment in _comments) {
+      writer.format(comment);
+    }
+
+    writer.format(_piece);
+  }
+
+  @override
+  void forEachChild(void Function(Piece piece) callback) {
+    _comments.forEach(callback);
+    callback(_piece);
+  }
+}

test/tall/invocation/cascade_comment.stmt

@@ -47,4 +47,20 @@ receiver
   // comment 2
   ..cascade2()
   // comment 3
-  ..cascade3();
+  ..cascade3();
+>>> Comment before setter.
+target
+  // comment
+  ..setter = value;
+<<<
+target
+  // comment
+  ..setter = value;
+>>> Comment before single method cascade.
+target
+// comment
+  ..method(argument);
+<<<
+target
+  // comment
+  ..method(argument);

test/tall/invocation/chain_comment.stmt

@@ -90,4 +90,10 @@ target
     )
     .third(
       // c3
-    );
+    );
+>>> Line comment before setter.
+target // c
+.prop = value;
+<<<
+target // c
+    .prop = value;