Format call chains! (#1356)

Format call chains! \o/

This handles unsplit call chains, fully split ones, as well as
block-style calls, like:

```
target.call(
  argument,
);
```

This doesn't fully implement the proposal I sketched out because it only
allows a single block call. There's a long TODO(tall) comment about
whether we want to do that, but I figured it makes sense to do that as a
separate iteration since there is so much in here.

But it implements most of it, and I've migrated all of the existing
tests over. It ended up being both simpler than the old formatter while
also producing better output.

The old formatter has a restriction where the indentation of a piece of
code must be fixed regardless of which way it splits. It can't handle:

```
// Block style, so indent argument +2:
target.property.call(
  argument
);

// Can't do block style, so indent +6:
target
    .slightlyLongerProperty
    .call(
      argument
    );
```

That leads to it sometimes producing gross output like:

```
target
    .longProperty
    .anotherLongProperty
    .method(
  wtfIsGoingOnHere,
);
```

To try to avoid that, it has lots of heuristics during Chunk
construction to try to not end up with output like that during line
splitting.

The new formatter doesn't need any of those heuristics because it can
freely choose indentation during line splitting.

Author: munificent

lib/src/front_end/ast_node_visitor.dart

@@ -12,7 +12,6 @@ import '../dart_formatter.dart';
 import '../piece/adjacent.dart';
 import '../piece/assign.dart';
 import '../piece/block.dart';
-import '../piece/chain.dart';
 import '../piece/constructor.dart';
 import '../piece/for.dart';
 import '../piece/if.dart';
@@ -22,6 +21,7 @@ import '../piece/piece.dart';
 import '../piece/variable.dart';
 import '../source_code.dart';
 import 'adjacent_builder.dart';
+import 'chain_builder.dart';
 import 'comment_writer.dart';
 import 'delimited_list_builder.dart';
 import 'piece_factory.dart';
@@ -412,18 +412,9 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitConstructorName(ConstructorName node) {
-    // If there is an import prefix and/or constructor name, then allow
-    // splitting before the `.`. This doesn't look good, but is consistent with
-    // constructor calls that don't have `new` or `const`. We allow splitting
-    // in the latter because there is no way to distinguish syntactically
-    // between a named constructor call and any other kind of method call or
-    // property access.
-    var operations = <Piece>[];
-
     var builder = AdjacentBuilder(this);
     if (node.type.importPrefix case var importPrefix?) {
       builder.token(importPrefix.name);
-      operations.add(builder.build());
       builder.token(importPrefix.period);
     }
 
@@ -435,16 +426,13 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
     // If this is a named constructor, the name.
     if (node.name != null) {
-      operations.add(builder.build());
       builder.token(node.period);
       builder.visit(node.name);
     }
 
     // If there was a prefix or constructor name, then make a splittable piece.
     // Otherwise, the current piece is a simple identifier for the name.
-    operations.add(builder.build());
-    if (operations.length == 1) return operations.first;
-    return ChainPiece(operations);
+    return builder.build();
   }
 
   @override
@@ -1027,35 +1015,19 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitIndexExpression(IndexExpression node) {
-    // TODO(tall): Allow splitting before and/or after the `[` when method
-    // chain formatting is fully implemented. For now, we just output the code
-    // so that tests of other language features that contain index expressions
-    // can run.
-    return buildPiece((b) {
-      b.visit(node.target);
-      b.token(node.leftBracket);
-      b.visit(node.index);
-      b.token(node.rightBracket);
-    });
+    Piece? targetPiece;
+    if (node.target case var target?) targetPiece = nodePiece(target);
+    return createIndexExpression(targetPiece, node);
   }
 
   @override
   Piece visitInstanceCreationExpression(InstanceCreationExpression node) {
     var builder = AdjacentBuilder(this);
     builder.token(node.keyword, spaceAfter: true);
 
-    // If there is an import prefix and/or constructor name, then allow
-    // splitting before the `.`. This doesn't look good, but is consistent with
-    // constructor calls that don't have `new` or `const`. We allow splitting
-    // in the latter because there is no way to distinguish syntactically
-    // between a named constructor call and any other kind of method call or
-    // property access.
-    var operations = <Piece>[];
-
     var constructor = node.constructorName;
     if (constructor.type.importPrefix case var importPrefix?) {
       builder.token(importPrefix.name);
-      operations.add(builder.build());
       builder.token(importPrefix.period);
     }
 
@@ -1066,19 +1038,13 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
     // If this is a named constructor call, the name.
     if (constructor.name case var name?) {
-      operations.add(builder.build());
       builder.token(constructor.period);
       builder.visit(name);
     }
 
     builder.visit(node.argumentList);
-    operations.add(builder.build());
 
-    if (operations.length > 1) {
-      return ChainPiece(operations);
-    } else {
-      return operations.first;
-    }
+    return builder.build();
   }
 
   @override
@@ -1195,15 +1161,23 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitMethodInvocation(MethodInvocation node) {
-    return buildPiece((b) {
-      // TODO(tall): Support splitting at `.` or `?.`. Right now we just format
-      // it inline so that we can use method calls in other tests.
-      b.visit(node.target);
-      b.token(node.operator);
-      b.visit(node.methodName);
-      b.visit(node.typeArguments);
-      b.visit(node.argumentList);
-    });
+    // If there's no target, this is a "bare" function call like "foo(1, 2)",
+    // or a section in a cascade.
+    //
+    // If it looks like a constructor or static call, we want to keep the
+    // target and method together instead of including the method in the
+    // subsequent method chain.
+    if (node.target == null || node.looksLikeStaticCall) {
+      return buildPiece((b) {
+        b.visit(node.target);
+        b.token(node.operator);
+        b.visit(node.methodName);
+        b.visit(node.typeArguments);
+        b.visit(node.argumentList);
+      });
+    }
+
+    return ChainBuilder(this, node).build();
   }
 
   @override
@@ -1354,14 +1328,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitPrefixedIdentifier(PrefixedIdentifier node) {
-    // TODO(tall): Allow splitting before the `.` when method chain formatting
-    // is fully implemented. For now, we just output the code so that tests
-    // of other language features that contain prefixed identifiers can run.
-    return buildPiece((b) {
-      b.visit(node.prefix);
-      b.token(node.period);
-      b.visit(node.identifier);
-    });
+    return ChainBuilder(this, node).build();
   }
 
   @override
@@ -1382,14 +1349,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitPropertyAccess(PropertyAccess node) {
-    // TODO(tall): Allow splitting before the `.` when method chain formatting
-    // is fully implemented. For now, we just output the code so that tests
-    // of other language features that contain property accesses can run.
-    return buildPiece((b) {
-      b.visit(node.target);
-      b.token(node.operator);
-      b.visit(node.propertyName);
-    });
+    return ChainBuilder(this, node).build();
   }
 
   @override

lib/src/front_end/chain_builder.dart

@@ -0,0 +1,207 @@
+// 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 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/token.dart';
+
+import '../ast_extensions.dart';
+import '../piece/chain.dart';
+import '../piece/piece.dart';
+import 'piece_factory.dart';
+
+/// Creates [Chain] pieces from method calls and property accesses, along with
+/// postfix operations (`!`, index operators, and function invocation
+/// expressions) that follow them.
+///
+/// In the AST for method calls, selectors are nested bottom up such that this
+/// expression:
+///
+///     obj.a(1)[2].c(3)
+///
+/// Is structured like:
+///
+///           .c()
+///           /  \
+///          []   3
+///         /  \
+///       .a()  2
+///       /  \
+///     obj   1
+///
+/// This means visiting the AST from top down visits the selectors from right
+/// to left. It's easier to format if we organize them as a linear series of
+/// selectors from left to right. Further, we want to organize it into a
+/// two-tier hierarchy. We have an outer list of method calls and property
+/// accesses. Then each of those may have one or more postfix selectors
+/// attached: indexers, null-assertions, or invocations. This mirrors how they
+/// are formatted.
+///
+/// This lets us create a single [ChainPiece] for the entire series of dotted
+/// operations, so that we can control splitting them or not as a unit.
+class ChainBuilder {
+  final PieceFactory _visitor;
+
+  /// The left-most target of the chain.
+  late Piece _target;
+
+  /// Whether the target expression may contain newlines when the chain is not
+  /// fully split. (It may always contain newlines when the chain splits.)
+  ///
+  /// This is true for most expressions but false for delimited ones to avoid
+  /// ugly formatting like:
+  ///
+  ///     function(
+  ///       argument,
+  ///     )
+  ///         .method();
+  late final bool _allowSplitInTarget;
+
+  /// The dotted property accesses and method calls following the target.
+  final List<ChainCall> _calls = [];
+
+  ChainBuilder(this._visitor, Expression expression) {
+    _unwrapCall(expression);
+  }
+
+  Piece build() {
+    // If there are no calls, there's no chain.
+    if (_calls.isEmpty) return _target;
+
+    // Count the number of contiguous properties at the beginning of the chain.
+    var leadingProperties = 0;
+    while (leadingProperties < _calls.length &&
+        _calls[leadingProperties].type == CallType.property) {
+      leadingProperties++;
+    }
+
+    // See if there is a call that we can block format. It can either be the
+    // very last call, if non-empty:
+    //
+    //     target.property.method().last(
+    //       argument,
+    //     );
+    //
+    // Or the second-to-last if the last call can't split:
+    //
+    //     target.property.method().penultimate(
+    //       argument,
+    //     ).toList();
+    var blockCallIndex = switch (_calls) {
+      [..., ChainCall(canSplit: true)] => _calls.length - 1,
+      [..., ChainCall(canSplit: true), ChainCall(canSplit: false)] =>
+        _calls.length - 2,
+      _ => -1,
+    };
+
+    return ChainPiece(_target, _calls, leadingProperties, blockCallIndex,
+        allowSplitInTarget: _allowSplitInTarget);
+  }
+
+  /// Given [expression], which is the outermost expression for some call chain,
+  /// recursively traverses the selectors to fill in the list of [_calls].
+  ///
+  /// Initializes [_target] with the innermost subexpression that isn't a part
+  /// of the call chain. For example, given:
+  ///
+  ///     foo.bar()!.baz[0][1].bang()
+  ///
+  /// This returns `foo` and fills [_calls] with:
+  ///
+  ///     .bar()!
+  ///     .baz[0][1]
+  ///     .bang()
+  void _unwrapCall(Expression expression) {
+    switch (expression) {
+      case Expression(looksLikeStaticCall: true):
+        // Don't include things that look like static method or constructor
+        // calls in the call chain because that tends to split up named
+        // constructors from their class.
+        _visitTarget(expression);
+
+      // Selectors.
+      case MethodInvocation(:var target?):
+        _unwrapCall(target);
+
+        var callPiece = _visitor.buildPiece((b) {
+          b.token(expression.operator);
+          b.visit(expression.methodName);
+          b.visit(expression.typeArguments);
+          b.visit(expression.argumentList);
+        });
+
+        var canSplit = expression.argumentList.arguments
+            .canSplit(expression.argumentList.rightParenthesis);
+        _calls.add(ChainCall(callPiece,
+            canSplit ? CallType.splittableCall : CallType.unsplittableCall));
+
+      case PropertyAccess(:var target?):
+        _unwrapCall(target);
+
+        var piece = _visitor.buildPiece((b) {
+          b.token(expression.operator);
+          b.visit(expression.propertyName);
+        });
+
+        _calls.add(ChainCall(piece, CallType.property));
+
+      case PrefixedIdentifier(:var prefix):
+        _unwrapCall(prefix);
+
+        var piece = _visitor.buildPiece((b) {
+          b.token(expression.period);
+          b.visit(expression.identifier);
+        });
+
+        _calls.add(ChainCall(piece, CallType.property));
+
+      // Postfix expressions.
+      case FunctionExpressionInvocation():
+        _unwrapPostfix(expression.function, (target) {
+          return _visitor.buildPiece((b) {
+            b.add(target);
+            b.visit(expression.typeArguments);
+            b.visit(expression.argumentList);
+          });
+        });
+
+      case IndexExpression():
+        _unwrapPostfix(expression.target!, (target) {
+          return _visitor.createIndexExpression(target, expression);
+        });
+
+      case PostfixExpression() when expression.operator.type == TokenType.BANG:
+        _unwrapPostfix(expression.operand, (target) {
+          return _visitor.buildPiece((b) {
+            b.add(target);
+            b.token(expression.operator);
+          });
+        });
+
+      default:
+        // Otherwise, it isn't a selector so we've reached the target.
+        _visitTarget(expression);
+    }
+  }
+
+  /// Creates and stores the resulting Piece for [target] as well as whether it
+  /// allows being split.
+  void _visitTarget(Expression target) {
+    _allowSplitInTarget = target.canBlockSplit;
+    _target = _visitor.nodePiece(target);
+  }
+
+  void _unwrapPostfix(
+      Expression operand, Piece Function(Piece target) createPostfix) {
+    _unwrapCall(operand);
+    // If we don't have a preceding call to hang the postfix expression off of,
+    // wrap it around the target expression. For example:
+    //
+    //     (list + another)!
+    if (_calls.isEmpty) {
+      _target = createPostfix(_target);
+    } else {
+      _calls.last.wrapPostfix(createPostfix);
+    }
+  }
+}