Format pattern variable declarations, including in for loops. (#1392)

Format pattern variable declarations, including in for loops.

This required pretty significantly revamping AssignPiece since now
there can be complex syntax on the left side of the `=` too. It went
through several iterations but I think I managed to simplify it down
to something reasonable that still passes all of the tests.

This PR doesn't do pattern assignment, but that should hopefully be a
trivial change after this.

Author: munificent

lib/src/front_end/ast_node_visitor.dart

@@ -384,7 +384,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
     if (node.redirectedConstructor case var constructor?) {
       redirect = AssignPiece(
           tokenPiece(node.separator!), nodePiece(constructor),
-          allowInnerSplit: false);
+          canBlockSplitRight: false);
     } else if (node.initializers.isNotEmpty) {
       initializerSeparator = tokenPiece(node.separator!);
       initializers = createList(node.initializers,
@@ -586,7 +586,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
       var expression = nodePiece(node.expression);
 
       b.add(AssignPiece(operatorPiece, expression,
-          allowInnerSplit: node.expression.canBlockSplit));
+          canBlockSplitRight: node.expression.canBlockSplit));
       b.token(node.semicolon);
     });
   }
@@ -1394,17 +1394,11 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitPatternVariableDeclaration(PatternVariableDeclaration node) {
-    // TODO(tall): This is just a basic implementation for the metadata tests.
-    // It still needs a full implementation and tests.
     return buildPiece((b) {
       b.metadata(node.metadata);
       b.token(node.keyword);
       b.space();
-      b.visit(node.pattern);
-      b.space();
-      b.token(node.equals);
-      b.space();
-      b.visit(node.expression);
+      b.add(createAssignment(node.pattern, node.equals, node.expression));
     });
   }
 
@@ -1879,7 +1873,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
         var initializerPiece = nodePiece(initializer, commaAfter: true);
 
         variables.add(AssignPiece(variablePiece, initializerPiece,
-            allowInnerSplit: initializer.canBlockSplit));
+            canBlockSplitRight: initializer.canBlockSplit));
       } else {
         variables.add(tokenPiece(variable.name, commaAfter: true));
       }

lib/src/front_end/piece_factory.dart

@@ -235,6 +235,8 @@ mixin PieceFactory {
             ForPartsWithDeclarations(variables: AstNode? initializer):
       case ForParts forParts &&
             ForPartsWithExpression(initialization: AstNode? initializer):
+      case ForParts forParts &&
+            ForPartsWithPattern(variables: AstNode? initializer):
         // In a C-style for loop, treat the for loop parts like an argument list
         // where each clause is a separate argument. This means that when they
         // split, they split like:
@@ -285,9 +287,6 @@ mixin PieceFactory {
         partsList.rightBracket(rightParenthesis);
         return partsList.build();
 
-      case ForPartsWithPattern():
-        throw UnimplementedError();
-
       case ForEachParts forEachParts &&
             ForEachPartsWithDeclaration(loopVariable: AstNode variable):
       case ForEachParts forEachParts &&
@@ -335,12 +334,22 @@ mixin PieceFactory {
           b.token(leftParenthesis);
           b.add(createAssignment(
               variable, forEachParts.inKeyword, forEachParts.iterable,
-              splitBeforeOperator: true, allowInnerSplit: true));
+              splitBeforeOperator: true, canBlockSplitLeft: true));
           b.token(rightParenthesis);
         });
 
-      case ForEachPartsWithPattern():
-        throw UnimplementedError();
+      case ForEachParts forEachParts &&
+            ForEachPartsWithPattern(:var keyword, :var pattern):
+        return buildPiece((b) {
+          b.token(leftParenthesis);
+          b.token(keyword);
+          b.space();
+
+          b.add(createAssignment(
+              pattern, forEachParts.inKeyword, forEachParts.iterable,
+              splitBeforeOperator: true));
+          b.token(rightParenthesis);
+        });
     }
   }
 
@@ -958,47 +967,64 @@ mixin PieceFactory {
   /// of the next line when it splits. Otherwise, puts the operator at the end
   /// of the preceding line.
   ///
-  /// If [allowInnerSplit] is `true`, then a newline inside the target or
-  /// right-hand side doesn't force splitting at the operator itself.
-  Piece createAssignment(AstNode target, Token operator, AstNode rightHandSide,
+  /// If [canBlockSplitLeft] is `true`, then the left-hand operand supports
+  /// being block-formatted without indenting it farther, like:
+  ///
+  ///     var [
+  ///       element,
+  ///     ] = list;
+  Piece createAssignment(
+      AstNode leftHandSide, Token operator, AstNode rightHandSide,
       {bool splitBeforeOperator = false,
       bool includeComma = false,
       bool spaceBeforeOperator = true,
-      bool allowInnerSplit = false}) {
-    // If the right-hand side can have block formatting, then a newline in
-    // it doesn't force the operator to split, as in:
+      bool canBlockSplitLeft = false}) {
+    // If an operand can have block formatting, then a newline in it doesn't
+    // force the operator to split, as in:
+    //
+    //    var [
+    //      element,
+    //    ] = list;
+    //
+    // Or:
     //
     //    var list = [
     //      element,
     //    ];
-    allowInnerSplit |= switch (rightHandSide) {
+    canBlockSplitLeft |= switch (leftHandSide) {
+      Expression() => leftHandSide.canBlockSplit,
+      DartPattern() => leftHandSide.canBlockSplit,
+      _ => false
+    };
+
+    var canBlockSplitRight = switch (rightHandSide) {
       Expression() => rightHandSide.canBlockSplit,
       DartPattern() => rightHandSide.canBlockSplit,
       _ => false
     };
 
+    Piece leftPiece;
+    Piece rightPiece;
     if (splitBeforeOperator) {
-      var targetPiece = nodePiece(target);
+      leftPiece = nodePiece(leftHandSide);
 
-      var initializer = buildPiece((b) {
+      rightPiece = buildPiece((b) {
         b.token(operator);
         b.space();
         b.visit(rightHandSide, commaAfter: includeComma);
       });
-
-      return AssignPiece(targetPiece, initializer,
-          allowInnerSplit: allowInnerSplit);
     } else {
-      var targetPiece = buildPiece((b) {
-        b.visit(target);
+      leftPiece = buildPiece((b) {
+        b.visit(leftHandSide);
         b.token(operator, spaceBefore: spaceBeforeOperator);
       });
 
-      var initializer = nodePiece(rightHandSide, commaAfter: includeComma);
-
-      return AssignPiece(targetPiece, initializer,
-          allowInnerSplit: allowInnerSplit);
+      rightPiece = nodePiece(rightHandSide, commaAfter: includeComma);
     }
+
+    return AssignPiece(leftPiece, rightPiece,
+        canBlockSplitLeft: canBlockSplitLeft,
+        canBlockSplitRight: canBlockSplitRight);
   }
 
   /// Creates a piece for a parameter-like constructor: Either a simple formal

lib/src/piece/assign.dart

@@ -19,37 +19,75 @@ import 'piece.dart';
 ///
 ///     var x = 123;
 ///
-/// If the value is a delimited "block-like" expression, then we allow splitting
-/// inside the value but not at the `=` with no additional indentation:
+/// This state also allows splitting the right side if it can be block
+/// formatted:
 ///
 ///     var list = [
 ///       element,
 ///     ];
 ///
-/// [_atOperator] Split after the `=`:
+/// [_blockSplit] Block split in the operands that support it and expression
+/// split in the others. This state requires at least one operand to support
+/// block splitting. Examples:
 ///
-///     var name =
-///         longValueExpression;
+///     var [
+///       element,
+///     ] = value;
+///
+///     var [
+///       element,
+///     ] = longOperand +
+///         anotherOperand;
+///
+///     var (longVariable &&
+///         anotherVariable) = [
+///       element,
+///     ];
+///
+/// [_atOperator] Split at the `=` or `in` operator and allow expression
+/// splitting in either operand:
+///
+///     var (longVariable &&
+///             anotherVariable) =
+///         longOperand +
+///             anotherOperand;
 class AssignPiece extends Piece {
-  /// Split after the operator.
+  /// Split at the operator.
   ///
-  /// This is more costly because it's generally better to split either in the
-  /// value (if it's delimited) or in the target.
-  static const State _atOperator = State(2, cost: 2);
+  /// This is more costly because it's generally better to block split in one
+  /// or both of the operands.
+  static const State _atOperator = State(1, cost: 2);
+
+  /// Block split in one or both of the operands.
+  static const State _blockSplit = State(2);
 
   /// The left-hand side of the operation. Includes the operator unless it is
   /// `in`.
-  final Piece target;
+  final Piece _left;
 
   /// The right-hand side of the operation.
-  final Piece value;
+  final Piece _right;
 
-  /// Whether a newline is allowed in the right-hand side without forcing a
-  /// split at the assignment operator.
-  final bool _allowInnerSplit;
+  /// If `true`, then the left side supports being block-formatted, like:
+  ///
+  ///     var [
+  ///       element1,
+  ///       element2,
+  ///     ] = value;
+  final bool _canBlockSplitLeft;
 
-  AssignPiece(this.target, this.value, {bool allowInnerSplit = false})
-      : _allowInnerSplit = allowInnerSplit;
+  /// If `true` then the right side supports being block-formatted, like:
+  ///
+  ///     var list = [
+  ///       element1,
+  ///       element2,
+  ///     ];
+  final bool _canBlockSplitRight;
+
+  AssignPiece(this._left, this._right,
+      {bool canBlockSplitLeft = false, bool canBlockSplitRight = false})
+      : _canBlockSplitLeft = canBlockSplitLeft,
+        _canBlockSplitRight = canBlockSplitRight;
 
   // TODO(tall): The old formatter allows the first operand of a split
   // conditional expression to be on the same line as the `=`, as in:
@@ -87,29 +125,66 @@ class AssignPiece extends Piece {
   //             operand;
 
   @override
-  List<State> get additionalStates => [_atOperator];
+  List<State> get additionalStates => [
+        // If at least one operand can block split, allow splitting in operands
+        // without splitting at the operator.
+        if (_canBlockSplitLeft || _canBlockSplitRight) _blockSplit,
+        _atOperator,
+      ];
 
   @override
   void format(CodeWriter writer, State state) {
-    // A split in either child piece forces splitting at assignment operator
-    // unless specifically allowed.
-    writer.pushAllowNewlines(_allowInnerSplit || state != State.unsplit);
+    var allowNewlinesInLeft = true;
+    var indentLeft = false;
+    var allowNewlinesInRight = true;
+    var indentRight = false;
+    var collapseIndent = false;
+
+    switch (state) {
+      case State.unsplit:
+        allowNewlinesInLeft = false;
+
+        // Always allow block-splitting the right side if it supports it.
+        allowNewlinesInRight = _canBlockSplitRight;
+
+      case _atOperator:
+        // When splitting at the operator, both operands may split or not and
+        // will be indented if they do.
+        indentLeft = true;
+        indentRight = true;
 
-    // Don't indent a split delimited expression.
-    if (state != State.unsplit) writer.pushIndent(Indent.expression);
+      case _blockSplit:
+        // Indent either operand if it doesn't block split.
+        indentLeft = !_canBlockSplitLeft;
+        indentRight = !_canBlockSplitRight;
+        collapseIndent = true;
+    }
 
-    writer.format(target);
+    writer.pushAllowNewlines(allowNewlinesInLeft);
+    if (indentLeft) {
+      writer.pushIndent(Indent.expression, canCollapse: collapseIndent);
+    }
+
+    writer.format(_left);
     writer.splitIf(state == _atOperator);
-    writer.format(value);
 
-    if (state != State.unsplit) writer.popIndent();
+    if (indentLeft) writer.popIndent();
+    writer.popAllowNewlines();
+
+    writer.pushAllowNewlines(allowNewlinesInRight);
+    if (indentRight) {
+      writer.pushIndent(Indent.expression, canCollapse: collapseIndent);
+    }
+
+    writer.format(_right);
 
+    if (indentRight) writer.popIndent();
     writer.popAllowNewlines();
   }
 
   @override
   void forEachChild(void Function(Piece piece) callback) {
-    callback(target);
-    callback(value);
+    callback(_left);
+    callback(_right);
   }
 }

test/expression/collection_for.stmt

@@ -138,4 +138,30 @@ var l = [for (;;) [if (c) d]];
 >>>
 var l = [for (;;) [for (c in d) e]];
 <<<
-var l = [for (;;) [for (c in d) e]];
+var l = [for (;;) [for (c in d) e]];
+>>> Pattern for-in.
+var list = [
+for (var (longIdentifier && anotherLongOne) in obj) element
+];
+<<<
+var list = [
+  for (var (longIdentifier &&
+          anotherLongOne)
+      in obj)
+    element,
+];
+>>> Pattern for.
+var list = [
+for (var (longIdentifier && anotherLongOne) = obj; cond; inc) element
+];
+<<<
+var list = [
+  for (
+    var (longIdentifier &&
+            anotherLongOne) =
+        obj;
+    cond;
+    inc
+  )
+    element,
+];