Format call chains!

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);
+    }
+  }
+}