Support "block argument" formatting. (#1322)

Support "block argument" formatting.

Sometimes in an argument list, we don't want to force it to full split
even if an argument contains an inner newline. In other words, we want
to prefer:

```
test('hi', () {
  body;
});
```

Over:

```
test(
  'hi',
  () {
    body;
  },
);
```

The latter is what you would normally get when an argument contains a
newline. This change enables the former. The heuristics for what kinds
of arguments are allowed to have this block-like formatting, how many
of them, and what other arguments are allowed are pretty fuzzy.

I picked a simple set of rules here (which follow the prototype), but
left some TODOs because I suspect we will iterate on them later once we
can run the new formatter on larger codebases.

The heuristic for what kinds of arguments are block-like is the same
rule that we use for "delimited" expressions after an assignment. I
think it's good that those two agree and helps make the style overall
more consistent. Basically, any time the right-hand side of an `=` or
`:` doesn't need to split, that same kind of expression can be block
formatted:

```
var list = [
  element,
];

function([
  element,
]);
```

Block formatting is used for argument lists, asserts, and switch values.

I also simplified AssignPiece. It used an extra unnecessary to allow
newlines inside delimited value expressions without splitting after "=".

Also renamed "isDelimited" (never a great name) to "canBlockSplit",
which I think better captures its use.

Author: munificent

lib/src/ast_extensions.dart

@@ -81,11 +81,11 @@ extension AstNodeExtensions on AstNode {
     if (node is SpreadElement) {
       var expression = node.expression;
       if (expression is ListLiteral) {
-        if (!expression.elements.isEmptyBody(expression.rightBracket)) {
+        if (expression.elements.canSplit(expression.rightBracket)) {
           return expression.leftBracket;
         }
       } else if (expression is SetOrMapLiteral) {
-        if (!expression.elements.isEmptyBody(expression.rightBracket)) {
+        if (expression.elements.canSplit(expression.rightBracket)) {
           return expression.leftBracket;
         }
       }
@@ -99,44 +99,91 @@ extension AstIterableExtensions on Iterable<AstNode> {
   /// Whether there is a comma token immediately following this.
   bool get hasCommaAfter => isNotEmpty && last.hasCommaAfter;
 
-  /// Whether the collection literal or block containing these nodes and
-  /// terminated by [rightBracket] is empty or not.
+  /// Whether the delimited construct containing these nodes and terminated by
+  /// [rightBracket] can have a split inside it.
   ///
-  /// An empty collection must have no elements or comments inside. Collections
-  /// like that are treated specially because they cannot be split inside.
-  bool isEmptyBody(Token rightBracket) =>
-      isEmpty && rightBracket.precedingComments == null;
+  /// We disallow splitting for entirely empty delimited constructs like `[]`,
+  /// but allow a split if there are elements or comments inside.
+  bool canSplit(Token rightBracket) =>
+      isNotEmpty || rightBracket.precedingComments != null;
 }
 
 extension ExpressionExtensions on Expression {
-  /// Whether this expression is a "delimited" one that allows block-like
-  /// formatting in some contexts. For example, in an assignment, a split in
-  /// the assigned value is usually indented:
+  /// Whether this expression is a non-empty delimited container for inner
+  /// expressions that allows "block-like" formatting in some contexts. For
+  /// example, in an assignment, a split in the assigned value is usually
+  /// indented:
   ///
   /// ```
   /// var variableName =
   ///     longValue;
   /// ```
   ///
-  /// But not if the initializer is a delimited expression and we don't split
-  /// at the `=`:
+  /// But if the initializer is block-like, we don't split at the `=`:
   ///
   /// ```
   /// var variableName = [
   ///   element,
   /// ];
   /// ```
-  bool get isDelimited => switch (this) {
-        FunctionExpression() => true,
-        InstanceCreationExpression() => true,
-        ListLiteral() => true,
-        MethodInvocation() => true,
-        ParenthesizedExpression(:var expression) => expression.isDelimited,
-        RecordLiteral() => true,
-        SetOrMapLiteral() => true,
-        SwitchExpression() => true,
-        _ => false,
-      };
+  ///
+  /// Likewise, in an argument list, block-like expressions can avoid splitting
+  /// the surrounding argument list:
+  ///
+  /// ```
+  /// function([
+  ///   element,
+  /// ]);
+  /// ```
+  ///
+  /// Completely empty delimited constructs like `[]` and `foo()` don't allow
+  /// splitting inside them, so are not considered block-like.
+  bool get canBlockSplit {
+    // Unwrap named expressions to get the real expression inside.
+    var expression = this;
+    if (expression is NamedExpression) {
+      expression = expression.expression;
+    }
+
+    // TODO(tall): We should also allow multi-line strings to be formatted
+    // like block arguments, at least in some cases like:
+    //
+    // ```
+    // function('''
+    //   Lots of
+    //   text
+    // ''');
+    // ```
+
+    // TODO(tall): Consider whether immediately-invoked function expressions
+    // should be block argument candidates, like:
+    //
+    // ```
+    // function(() {
+    //   body;
+    // }());
+    // ```
+    return switch (expression) {
+      // A function expression can use either a non-empty parameter list or a
+      // non-empty block body for block formatting.
+      FunctionExpression(:var parameters?, :var body) =>
+        parameters.parameters.canSplit(parameters.rightParenthesis) ||
+            (body is BlockFunctionBody &&
+                body.block.statements.canSplit(body.block.rightBracket)),
+      ListLiteral(:var elements, :var rightBracket) ||
+      SetOrMapLiteral(:var elements, :var rightBracket) =>
+        elements.canSplit(rightBracket),
+      RecordLiteral(:var fields, :var rightParenthesis) =>
+        fields.canSplit(rightParenthesis),
+      SwitchExpression(:var cases, :var rightBracket) =>
+        cases.canSplit(rightBracket),
+      InstanceCreationExpression(:var argumentList) ||
+      MethodInvocation(:var argumentList) =>
+        argumentList.arguments.canSplit(argumentList.rightParenthesis),
+      ParenthesizedExpression(:var expression) => expression.canBlockSplit,
+      _ => false,
+    };
+  }
 
   /// Whether this is an argument in an argument list with a trailing comma.
   bool get isTrailingCommaArgument {

lib/src/front_end/ast_node_visitor.dart

@@ -115,10 +115,8 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @override
   void visitArgumentList(ArgumentList node) {
-    createList(
-        leftBracket: node.leftParenthesis,
-        node.arguments,
-        rightBracket: node.rightParenthesis);
+    createArgumentList(
+        node.leftParenthesis, node.arguments, node.rightParenthesis);
   }
 
   @override
@@ -134,14 +132,13 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
   @override
   void visitAssertStatement(AssertStatement node) {
     token(node.assertKeyword);
-    createList(
-      [
-        node.condition,
-        if (node.message case var message?) message,
-      ],
-      leftBracket: node.leftParenthesis,
-      rightBracket: node.rightParenthesis,
-    );
+    createArgumentList(
+        node.leftParenthesis,
+        [
+          node.condition,
+          if (node.message case var message?) message,
+        ],
+        node.rightParenthesis);
     token(node.semicolon);
   }
 
@@ -438,7 +435,6 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
               spaceWhenUnsplit: true, splitListIfBeforeSplits: true));
       builder.leftBracket(node.leftBracket);
       node.constants.forEach(builder.visit);
-
       builder.rightBracket(semicolon: node.semicolon, node.rightBracket);
       pieces.give(builder.build());
     } else {
@@ -959,8 +955,10 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @override
   void visitMethodInvocation(MethodInvocation node) {
-    // TODO(tall): Support method invocation with explicit target expressions.
-    if (node.target != null) throw UnimplementedError();
+    // TODO(tall): Support splitting at `.` or `?.`. Right now we just format
+    // it inline so that we can use method calls in other tests.
+    visit(node.target);
+    token(node.operator);
 
     visit(node.methodName);
     visit(node.typeArguments);

lib/src/front_end/comment_writer.dart

@@ -34,14 +34,14 @@ import 'piece_writer.dart';
 /// couple of different ways.
 ///
 /// Comments between top-level declarations, member declarations inside types,
-/// and statements are handled directly by [SequenceBuilder].
+/// and statements are handled directly by [SequenceBuilder]. Comments inside
+/// argument lists, collection literals, and other similar constructs are
+/// handled directly be [DelimitedPieceBuilder].
 ///
 /// All other comments occur inside the middle of some expression or other
 /// construct. These get directly embedded in the [TextPiece] of the code being
 /// written. When that [TextPiece] is output later, it will include the comments
 /// as well.
-// TODO(tall): When argument lists and their comment handling is supported,
-// mention that here.
 mixin CommentWriter {
   PieceWriter get pieces;
 

lib/src/front_end/delimited_list_builder.dart

@@ -44,8 +44,15 @@ class DelimitedListBuilder {
   /// literal, etc.
   DelimitedListBuilder(this._visitor, [this._style = const ListStyle()]);
 
-  ListPiece build() =>
-      ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket, _style);
+  /// Creates the final [ListPiece] out of the added brackets, delimiters,
+  /// elements, and style.
+  ListPiece build() {
+    var blockElement = -1;
+    if (_style.allowBlockElement) blockElement = _findBlockElement();
+
+    return ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket,
+        _style, blockElement);
+  }
 
   /// Adds the opening [bracket] to the built list.
   ///
@@ -119,8 +126,8 @@ class DelimitedListBuilder {
   /// [addCommentsBefore()] for the first token in the [piece].
   ///
   /// Assumes there is no comma after this piece.
-  void add(Piece piece) {
-    _elements.add(ListElement(piece));
+  void add(Piece piece, [BlockFormat format = BlockFormat.none]) {
+    _elements.add(ListElement(piece, format));
     _commentsBeforeComma = CommentSequence.empty;
   }
 
@@ -136,9 +143,16 @@ class DelimitedListBuilder {
     // Handle comments between the preceding element and this one.
     addCommentsBefore(element.firstNonCommentToken);
 
+    // See if it's an expression that supports block formatting.
+    var format = switch (element) {
+      FunctionExpression() when element.canBlockSplit => BlockFormat.function,
+      Expression() when element.canBlockSplit => BlockFormat.block,
+      _ => BlockFormat.none,
+    };
+
     // Traverse the element itself.
     _visitor.visit(element);
-    add(_visitor.pieces.split());
+    add(_visitor.pieces.split(), format);
 
     var nextToken = element.endToken.next!;
     if (nextToken.lexeme == ',') {
@@ -373,4 +387,47 @@ class DelimitedListBuilder {
       leading: leadingComments
     );
   }
+
+  /// If [_blockCandidates] contains a single expression that can receive
+  /// block formatting, then returns its index. Otherwise returns `-1`.
+  int _findBlockElement() {
+    // TODO(tall): These heuristics will probably need some iteration.
+    var functions = <int>[];
+    var others = <int>[];
+
+    for (var i = 0; i < _elements.length; i++) {
+      switch (_elements[i].blockFormat) {
+        case BlockFormat.function:
+          functions.add(i);
+        case BlockFormat.block:
+          others.add(i);
+        case BlockFormat.none:
+          break; // Not a block element.
+      }
+    }
+
+    // A function expression takes precedence over other block arguments.
+    if (functions.length == 1) return functions.first;
+
+    // Otherwise, if there is single block argument, it can be block formatted.
+    if (functions.isEmpty && others.length == 1) return others.first;
+
+    // There are no block arguments, or it's ambiguous as to which one should
+    // be it.
+    // TODO(tall): The old formatter allows multiple block arguments, like:
+    //
+    // ```
+    // function(() {
+    //   body;
+    // }, () {
+    //   more;
+    // });
+    // ```
+    //
+    // This doesn't seem very common in the Flutter repo, but does occur
+    // sometimes. We'll probably want to experiment to see if it's worth
+    // supporting multiple block arguments. If so, we should at least require
+    // them to be contiguous with no non-block arguments in the middle.
+    return -1;
+  }
 }

lib/src/front_end/piece_factory.dart

@@ -49,6 +49,16 @@ typedef BinaryOperation = (AstNode left, Token operator, AstNode right);
 mixin PieceFactory implements CommentWriter {
   void visit(AstNode? node, {void Function()? before, void Function()? after});
 
+  /// Creates a [ListPiece] for an argument list.
+  void createArgumentList(
+      Token leftBracket, Iterable<AstNode> elements, Token rightBracket) {
+    return createList(
+        leftBracket: leftBracket,
+        elements,
+        rightBracket: rightBracket,
+        style: const ListStyle(allowBlockElement: true));
+  }
+
   /// Creates a [BlockPiece] for a given bracket-delimited block or declaration
   /// body.
   ///
@@ -425,20 +435,16 @@ mixin PieceFactory implements CommentWriter {
   /// Visits the `switch (expr)` part of a switch statement or expression.
   void createSwitchValue(Token switchKeyword, Token leftParenthesis,
       Expression value, Token rightParenthesis) {
-    // Format like an argument list since it is an expression surrounded by
-    // parentheses.
-    var builder = DelimitedListBuilder(
-        this, const ListStyle(commas: Commas.none, splitCost: 2));
-
     // Attach the `switch ` as part of the `(`.
     token(switchKeyword);
     space();
 
-    builder.leftBracket(leftParenthesis);
-    builder.visit(value);
-    builder.rightBracket(rightParenthesis);
-
-    pieces.give(builder.build());
+    createList(
+        leftBracket: leftParenthesis,
+        [value],
+        rightBracket: rightParenthesis,
+        style: const ListStyle(
+            commas: Commas.none, splitCost: 2, allowBlockElement: true));
   }
 
   /// Creates a class, enum, extension, mixin, or mixin application class
@@ -605,7 +611,7 @@ mixin PieceFactory implements CommentWriter {
 
     var initializer = pieces.take();
     pieces.give(AssignPiece(target, initializer,
-        isValueDelimited: rightHandSide.isDelimited));
+        isValueDelimited: rightHandSide.canBlockSplit));
   }
 
   /// Finishes writing a named function declaration or anonymous function