When the RHS of => is a function call, prefer to split at the =>. (#1523)

We use the same AssignPiece rule for `=`, `=>`, and `:`. And that rule uniformly handles all kinds of block-formattable right hand sides: collection literals, switch expressions, and function calls. For the most part, that works well and provides nice consistent formatting. Users generally like:

```dart
// Better:
variable = [
  long,
  list,
  literal,
];

// Worse:
variable =
    [long, list, literal];
```

And:

```dart
// Better:
SomeWidget(
  children: [
    long,
    list,
    literal,
  ],
);

// Worse:
SomeWidget(
  children:
      [long, list, literal],
);
```

And (with somewhat less consensus):

```dart
// Better:
variable = function(
  long,
  argument,
  list,
);

// Worse:
variable =
    function(long, argument, list);
```

Also, users have long requested and seem to like:

```dart
// Better:
class C {
  List<String> makeStuff() => [
    long,
    list,
    literal,
  ];
}

// Worse:
class C {
  List<String> makeStuff() =>
      [long, list, literal];
}
```

So based on all that, I just used uniform rules for all combinations of assignment constructs and delimited constructs. That means that this behavior falls out implicitly:

```dart
// Better:
class C {
  String doThing() => function(
    long,
    argument,
    list,
  );
}

// Worse:
class C {
  String doThing() =>
      function(long, argument, list);
}
```

But it turns out that that particular combination of `=>` with a function call on the right doesn't get the same user sentiment. Instead, most (including me) prefer:

```dart
class C {
  String doThing() =>
      function(long, argument, list);
}
```

I think it's because this keeps the function name next to its arguments. With the other block-like forms: list literals, etc. there isn't really anything particularly interesting going on in the opening delimiter, so it makes sense to keep it on the same line as the `=>` since it's pretty much just punctuation. But a function call is a single coherent operation including the function and its arguments.

So this PR tweaks the cost rule for AssignPiece. When the operator is `=>` and the RHS is a function call, we prefer to split at the `=>` if that lets the RHS stay unsplit.

Author: munificent

lib/src/front_end/ast_node_visitor.dart

@@ -605,7 +605,9 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
         nodePiece(node.expression, context: NodeContext.assignment);
 
     pieces.add(AssignPiece(operatorPiece, expression,
-        canBlockSplitRight: node.expression.canBlockSplit));
+        canBlockSplitRight: node.expression.canBlockSplit,
+        avoidBlockSplitRight:
+            node.expression.blockFormatType == BlockFormat.invocation));
     pieces.token(node.semicolon);
   }
 

lib/src/piece/assign.dart

@@ -73,7 +73,7 @@ class AssignPiece extends Piece {
   static const State _blockSplitLeft = State(1);
 
   /// Allow the right-hand side to block split.
-  static const State _blockSplitRight = State(2);
+  static const State _blockSplitRight = State(2, cost: 0);
 
   /// Split at the operator.
   static const State _atOperator = State(3);
@@ -109,13 +109,34 @@ class AssignPiece extends Piece {
   ///     ];
   final bool _canBlockSplitRight;
 
+  /// If `true` then prefer to split at the operator instead of block splitting
+  /// the right side.
+  ///
+  /// This is `true` for `=>` functions whose body is a function call. This
+  /// keeps the called function next to its arguments instead having the
+  /// function name stick to the `=>` while the arguments split. In other words,
+  /// prefer:
+  ///
+  ///     someMethod() =>
+  ///         someFunction(argument, another);
+  ///
+  /// Over:
+  ///
+  ///     someMethod() => someFunction(
+  ///       argument,
+  ///       another,
+  ///     );
+  final bool _avoidBlockSplitRight;
+
   AssignPiece(this._operator, this._right,
       {Piece? left,
       bool canBlockSplitLeft = false,
-      bool canBlockSplitRight = false})
+      bool canBlockSplitRight = false,
+      bool avoidBlockSplitRight = false})
       : _left = left,
         _canBlockSplitLeft = canBlockSplitLeft,
-        _canBlockSplitRight = canBlockSplitRight;
+        _canBlockSplitRight = canBlockSplitRight,
+        _avoidBlockSplitRight = avoidBlockSplitRight;
 
   @override
   List<State> get additionalStates => [
@@ -126,6 +147,16 @@ class AssignPiece extends Piece {
         _atOperator,
       ];
 
+  @override
+  int stateCost(State state) {
+    // Allow block splitting the right side, but increase the cost so that we
+    // prefer splitting at the operator and not splitting in the right piece if
+    // possible.
+    if (state == _blockSplitRight && _avoidBlockSplitRight) return 1;
+
+    return super.stateCost(state);
+  }
+
   @override
   void applyConstraints(State state, Constrain constrain) {
     // Force the left side to block split when in that state.
@@ -140,63 +171,35 @@ class AssignPiece extends Piece {
   }
 
   @override
-  bool allowNewlineInChild(State state, Piece child) {
-    if (state == State.unsplit) {
-      if (child == _left) return false;
-
-      // Always allow block-splitting the right side if it supports it.
-      if (child == _right) return _canBlockSplitRight;
-    }
-
-    return true;
-  }
+  bool allowNewlineInChild(State state, Piece child) => state != State.unsplit;
 
   @override
   void format(CodeWriter writer, State state) {
-    switch (state) {
-      case State.unsplit:
-        _writeLeft(writer, allowNewlines: false);
-        _writeOperator(writer);
-        // Always allow block-splitting the right side if it supports it.
-        _writeRight(writer, allowNewlines: _canBlockSplitRight);
-
-      case _atOperator:
-        // When splitting at the operator, both operands may split or not and
-        // will be indented if they do.
-        writer.pushIndent(Indent.expression);
-        _writeLeft(writer);
-        _writeOperator(writer, split: state == _atOperator);
-        _writeRight(writer);
-        writer.popIndent();
-
-      case _blockSplitLeft:
-        _writeLeft(writer);
-        _writeOperator(writer);
-        _writeRight(writer, indent: !_canBlockSplitRight);
-
-      case _blockSplitRight:
-        _writeLeft(writer);
-        _writeOperator(writer, split: state == _atOperator);
-        _writeRight(writer);
-    }
-  }
+    // When splitting at the operator, both operands may split or not and
+    // will be indented if they do.
+    if (state == _atOperator) writer.pushIndent(Indent.expression);
 
-  void _writeLeft(CodeWriter writer, {bool allowNewlines = true}) {
     if (_left case var left?) writer.format(left);
-  }
 
-  void _writeOperator(CodeWriter writer, {bool split = false}) {
     writer.pushIndent(Indent.expression);
     writer.format(_operator);
     writer.popIndent();
-    writer.splitIf(split);
-  }
+    writer.splitIf(state == _atOperator);
 
-  void _writeRight(CodeWriter writer,
-      {bool indent = false, bool allowNewlines = true}) {
-    if (indent) writer.pushIndent(Indent.expression);
+    // If the left side block splits and the right doesn't, then indent the
+    // right side if it splits as in:
+    //
+    //     var [
+    //       a,
+    //       b,
+    //     ] = long +
+    //         expression;
+    var indentRight = state == _blockSplitLeft && !_canBlockSplitRight;
+    if (indentRight) writer.pushIndent(Indent.expression);
     writer.format(_right);
-    if (indent) writer.popIndent();
+    if (indentRight) writer.popIndent();
+
+    if (state == _atOperator) writer.popIndent();
   }
 
   @override

test/tall/function/default_value.unit

@@ -66,9 +66,10 @@ f([
 f([callback(SomeType parameter) = const CallableClass(someLongConstantArgument, anotherConstantArgument)]) {}
 <<<
 f([
-  callback(SomeType parameter) =
-      const CallableClass(
-        someLongConstantArgument,
-        anotherConstantArgument,
-      ),
+  callback(
+    SomeType parameter,
+  ) = const CallableClass(
+    someLongConstantArgument,
+    anotherConstantArgument,
+  ),
 ]) {}

test/tall/function/expression_body.unit

@@ -52,13 +52,11 @@ function() => (
   longElement,
   longElement,
 );
->>> Prefer block-like splitting for function calls.
+>>> Avoid block-like splitting for function calls.
 function() => another(argument, argument);
 <<<
-function() => another(
-  argument,
-  argument,
-);
+function() =>
+    another(argument, argument);
 >>> Use block-like splitting for switch expressions.
 function() => switch (value) { 1 => 'one', 2 => 'two' };
 <<<

test/tall/regression/0100/0108.unit

@@ -276,10 +276,8 @@ class ElementBinder {
           eventName,
           (_) => zone.run(
             () => bindAssignableProps.forEach(
-              (propAndExp) => propAndExp[1].assign(
-                scope.context,
-                jsNode[propAndExp[0]],
-              ),
+              (propAndExp) =>
+                  propAndExp[1].assign(scope.context, jsNode[propAndExp[0]]),
             ),
           ),
         ),

test/tall/regression/0200/0217.stmt

@@ -3,8 +3,8 @@ Annotation _getAnnotation(AnnotatedNode node, String name) => nodeMetadata
     .firstWhere((annotation) => _isAnnotationType(annotation, name),
         orElse: () => null);
 <<<
-Annotation _getAnnotation(AnnotatedNode node, String name) => nodeMetadata
-    .firstWhere(
+Annotation _getAnnotation(AnnotatedNode node, String name) =>
+    nodeMetadata.firstWhere(
       (annotation) => _isAnnotationType(annotation, name),
       orElse: () => null,
     );
@@ -13,8 +13,8 @@ Annotation _getAnnotation(AnnotatedNode node, String name) => nodeMetadata
       .firstWhere((annotation) => _isAnnotationType(annotation, name),
           orElse: () => null);
 <<<
-  Annotation _getAnnotation(AnnotatedNode node, String name) => nodeMetadata
-      .firstWhere(
+  Annotation _getAnnotation(AnnotatedNode node, String name) =>
+      nodeMetadata.firstWhere(
         (annotation) => _isAnnotationType(annotation, name),
         orElse: () => null,
       );