For loop metadata (#1406)

* Add DelimitedListBuilder.addLeftBracket().

The existing leftBracket() method takes a couple of optional parameters
to build up a Piece in different ways. But an upcoming change also adds
yet another way that the left bracket Piece might need to be
constructed.

Instead of adding yet another optional parameter, I just made a separate
addLeftBracket() method that takes a complete Piece made any way you
want and then changed the existing callsites to use that unless they
only need a single token.

* Change the Piece structure for Object patterns.

This change is mainly to ensure that every outermost Piece for a pattern
that can be block-formatted is a ListPiece. That will be needed in a
later commit so that when a pattern appears to the left of an `=`, we
can constrain it to block split sometimes.

But this change also slightly changes the formatting of object patterns
in a way that I think looks better: if the type arguments in an object
pattern split, it forces the fields to split too.

* Turn createForLoopParts() into createFor().

The only two calls createForLoopParts() are in visitForStatement() and
visitForElement() and those two are very similar. In a future commit,
they will have more code to share, so reorganizing now to make it easier
to share it.

* Move the operator in AssignPiece to a separate child piece.

Prior to this change, the operator was directly attached to either the
LHS piece (for ` =`) or the RHS (for `in `). This pulls it out to its
own piece. The formatting is unchanged by this commit.

In a future commit, we'll use this to be able to constrain the LHS
child piece without the operator getting in the way.

* Rework AssignPiece formatting.

Formatting assignments is complex because LHS can be block split, or
expression split, or not split. Likewise with the RHS, and thus all nine
combinations. Even figuring out what the formatting should be is tricky.

The "in" clause of a for-in loop also uses the same formatting logic,
which adds more complexity because now it's nested inside a surrounding
statement-like form and parentheses.

It gets even weirder when you have metadata before the variable (which
isn't in this commit).

This change tweaks the formatting rules to, as best as I can tell, make
the edge cases look as nice as I can get. It also adds more tests for
corners that weren't covered. The main changes are:

- If the RHS of an `in` expression splits, then go ahead and split
  before the `in` too. It's pretty rare for splits in `in` to occur in
  the wild, but when they do, the Flutter tends to keep it tightly
  packed with the rest the for loop variable. The initial style tried
  to follow that. But it gets really weird if you happen to have
  metadata in there. I also don't love how it looks. Usually, when a
  split occurs in an operand or clause, we split at the preceding
  operator or keyword too.

  This commit does that. If an expression split occurs to the right of
  `in`, it splits before the `in` too.

- Tweak the relative priority between block splitting on the LHS of an
  assignment versus at the operator itself. The main way this is visible
  is that `=>` function bodies (which also use AssignPiece) now prefer
  to split after the `=>` instead of in the parameter list. I think
  that looks better.

- If the LHS *can* block split but chooses not to, then don't allow the
  RHS to expression split. This is the most important tweak. Before
  this change, the formatter could produce:

      for (var [i] in veryLongIterator +
          longExpression) {
        ;
      }

  That was never intended. The `in` shouldn't get buried in the middle
  of a line like that. This change fixes that by having the AssignPiece
  explicitly constrain the LHS to split when it's a block and it's
  going to allow the RHS to expression split.

* Format metadata on for loop variables.

This is the actual *new* syntax supported by this PR.

* Allow splitting between the type and name in a for loop variable.

This was an oversight. All other type annotated variables allow
splitting between the type and name (even though it's rare).

* Consistently call buildPiece() directly in the argument to
addLeftBracket().

The other callsites all do, so may as well do so here too.

Author: munificent

lib/src/front_end/ast_node_visitor.dart

@@ -14,7 +14,6 @@ import '../piece/adjacent_strings.dart';
 import '../piece/assign.dart';
 import '../piece/case.dart';
 import '../piece/constructor.dart';
-import '../piece/for.dart';
 import '../piece/if.dart';
 import '../piece/infix.dart';
 import '../piece/list.dart';
@@ -375,8 +374,12 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
     Piece? initializerSeparator;
     Piece? initializers;
     if (node.redirectedConstructor case var constructor?) {
-      redirect = AssignPiece(
-          tokenPiece(node.separator!), nodePiece(constructor),
+      var separator = buildPiece((b) {
+        b.token(node.separator);
+        b.space();
+      });
+
+      redirect = AssignPiece(separator, nodePiece(constructor),
           canBlockSplitRight: false);
     } else if (node.initializers.isNotEmpty) {
       initializerSeparator = tokenPiece(node.separator!);
@@ -436,11 +439,11 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitDeclaredIdentifier(DeclaredIdentifier node) {
-    return buildPiece((b) {
-      b.modifier(node.keyword);
-      b.visit(node.type, spaceAfter: true);
-      b.token(node.name);
-    });
+    return createParameter(
+        metadata: node.metadata,
+        modifiers: [node.keyword],
+        node.type,
+        node.name);
   }
 
   @override
@@ -521,7 +524,13 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
           this,
           const ListStyle(
               spaceWhenUnsplit: true, splitListIfBeforeSplits: true));
-      builder.leftBracket(node.leftBracket, preceding: header);
+
+      builder.addLeftBracket(buildPiece((b) {
+        b.add(header);
+        b.space();
+        b.token(node.leftBracket);
+      }));
+
       node.constants.forEach(builder.visit);
       builder.rightBracket(semicolon: node.semicolon, node.rightBracket);
       metadataBuilder.add(builder.build());
@@ -677,11 +686,13 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
     // If all parameters are optional, put the `[` or `{` right after `(`.
     var builder = DelimitedListBuilder(this);
-    if (node.parameters.isNotEmpty && firstOptional == 0) {
-      builder.leftBracket(node.leftParenthesis, delimiter: node.leftDelimiter);
-    } else {
-      builder.leftBracket(node.leftParenthesis);
-    }
+
+    builder.addLeftBracket(buildPiece((b) {
+      b.token(node.leftParenthesis);
+      if (node.parameters.isNotEmpty && firstOptional == 0) {
+        b.token(node.leftDelimiter);
+      }
+    }));
 
     for (var i = 0; i < node.parameters.length; i++) {
       // If this is the first optional parameter, put the delimiter before it.
@@ -698,70 +709,57 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitForElement(ForElement node) {
-    var forKeyword = buildPiece((b) {
-      b.modifier(node.awaitKeyword);
-      b.token(node.forKeyword);
-    });
-
-    var forPartsPiece = createForLoopParts(
-        node.leftParenthesis, node.forLoopParts, node.rightParenthesis);
-    var body = nodePiece(node.body);
-
-    var forPiece = ForPiece(forKeyword, forPartsPiece, body,
-        hasBlockBody: node.body.isSpreadCollection);
-
-    // It looks weird to have multiple nested control flow elements on the same
-    // line, so force the outer one to split if there is an inner one.
-    if (node.body.isControlFlowElement) {
-      forPiece.pin(State.split);
-    }
-
-    return forPiece;
+    return createFor(
+        awaitKeyword: node.awaitKeyword,
+        forKeyword: node.forKeyword,
+        leftParenthesis: node.leftParenthesis,
+        forLoopParts: node.forLoopParts,
+        rightParenthesis: node.rightParenthesis,
+        body: node.body,
+        hasBlockBody: node.body.isSpreadCollection,
+        forceSplitBody: node.body.isControlFlowElement);
   }
 
   @override
   Piece visitForStatement(ForStatement node) {
-    var forKeyword = buildPiece((b) {
-      b.modifier(node.awaitKeyword);
-      b.token(node.forKeyword);
-    });
-
-    var forPartsPiece = createForLoopParts(
-        node.leftParenthesis, node.forLoopParts, node.rightParenthesis);
-    var body = nodePiece(node.body);
-
-    return ForPiece(forKeyword, forPartsPiece, body,
+    return createFor(
+        awaitKeyword: node.awaitKeyword,
+        forKeyword: node.forKeyword,
+        leftParenthesis: node.leftParenthesis,
+        forLoopParts: node.forLoopParts,
+        rightParenthesis: node.rightParenthesis,
+        body: node.body,
         hasBlockBody: node.body is Block);
   }
 
   @override
   Piece visitForEachPartsWithDeclaration(ForEachPartsWithDeclaration node) {
-    throw UnsupportedError('This node is handled by visitForStatement().');
+    throw UnsupportedError('This node is handled by createFor().');
   }
 
   @override
   Piece visitForEachPartsWithIdentifier(ForEachPartsWithIdentifier node) {
-    throw UnsupportedError('This node is handled by visitForStatement().');
+    throw UnsupportedError('This node is handled by createFor().');
   }
 
   @override
   Piece visitForEachPartsWithPattern(ForEachPartsWithPattern node) {
-    throw UnsupportedError('This node is handled by visitForStatement().');
+    throw UnsupportedError('This node is handled by createFor().');
   }
 
   @override
   Piece visitForPartsWithDeclarations(ForPartsWithDeclarations node) {
-    throw UnsupportedError('This node is handled by visitForStatement().');
+    throw UnsupportedError('This node is handled by createFor().');
   }
 
   @override
   Piece visitForPartsWithExpression(ForPartsWithExpression node) {
-    throw UnsupportedError('This node is handled by visitForStatement().');
+    throw UnsupportedError('This node is handled by createFor().');
   }
 
   @override
   Piece visitForPartsWithPattern(ForPartsWithPattern node) {
-    throw UnsupportedError('This node is handled by visitForStatement().');
+    throw UnsupportedError('This node is handled by createFor().');
   }
 
   @override
@@ -1297,14 +1295,16 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitObjectPattern(ObjectPattern node) {
-    return buildPiece((b) {
+    var builder = DelimitedListBuilder(this);
+
+    builder.addLeftBracket(buildPiece((b) {
       b.visit(node.type);
-      b.add(createCollection(
-        node.leftParenthesis,
-        node.fields,
-        node.rightParenthesis,
-      ));
-    });
+      b.token(node.leftParenthesis);
+    }));
+
+    node.fields.forEach(builder.visit);
+    builder.rightBracket(node.rightParenthesis);
+    return builder.build();
   }
 
   @override
@@ -1384,7 +1384,13 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
   @override
   Piece visitPatternVariableDeclaration(PatternVariableDeclaration node) {
     return buildPiece((b) {
-      b.metadata(node.metadata);
+      // If the variable is part of a for loop, it looks weird to force the
+      // metadata to split since it's in a sort of expression-ish location:
+      //
+      //     for (@meta var (x, y) in pairs) ...
+      b.metadata(node.metadata,
+          inline: node.parent is ForEachPartsWithPattern ||
+              node.parent is ForPartsWithPattern);
       b.token(node.keyword);
       b.space();
       b.add(createAssignment(node.pattern, node.equals, node.expression));
@@ -1481,14 +1487,12 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
     var builder = DelimitedListBuilder(this, listStyle);
 
     // If all parameters are optional, put the `{` right after `(`.
-    if (positionalFields.isEmpty && namedFields != null) {
-      builder.leftBracket(
-        node.leftParenthesis,
-        delimiter: namedFields.leftBracket,
-      );
-    } else {
-      builder.leftBracket(node.leftParenthesis);
-    }
+    builder.addLeftBracket(buildPiece((b) {
+      b.token(node.leftParenthesis);
+      if (positionalFields.isEmpty && namedFields != null) {
+        b.token(namedFields.leftBracket);
+      }
+    }));
 
     for (var positionalField in positionalFields) {
       builder.visit(positionalField);
@@ -1678,7 +1682,12 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
     var list = DelimitedListBuilder(this,
         const ListStyle(spaceWhenUnsplit: true, splitListIfBeforeSplits: true));
-    list.leftBracket(node.leftBracket, preceding: value);
+
+    list.addLeftBracket(buildPiece((b) {
+      b.add(value);
+      b.space();
+      b.token(node.leftBracket);
+    }));
 
     for (var member in node.cases) {
       list.visit(member);
@@ -1830,7 +1839,12 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
   @override
   Piece visitVariableDeclarationList(VariableDeclarationList node) {
     var header = buildPiece((b) {
-      b.metadata(node.metadata);
+      // If the variable is part of a for loop, it looks weird to force the
+      // metadata to split since it's in a sort of expression-ish location:
+      //
+      //     for (@meta var x in list) ...
+      b.metadata(node.metadata,
+          inline: node.parent is ForPartsWithDeclarations);
       b.modifier(node.lateKeyword);
       b.modifier(node.keyword);
 
@@ -1844,15 +1858,19 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
     for (var variable in node.variables) {
       if ((variable.equals, variable.initializer)
           case (var equals?, var initializer?)) {
-        var variablePiece = buildPiece((b) {
-          b.token(variable.name);
+        var variablePiece = tokenPiece(variable.name);
+
+        var equalsPiece = buildPiece((b) {
           b.space();
           b.token(equals);
         });
 
         var initializerPiece = nodePiece(initializer, commaAfter: true);
 
-        variables.add(AssignPiece(variablePiece, initializerPiece,
+        variables.add(AssignPiece(
+            left: variablePiece,
+            equalsPiece,
+            initializerPiece,
             canBlockSplitRight: initializer.canBlockSplit));
       } else {
         variables.add(tokenPiece(variable.name, commaAfter: true));

lib/src/front_end/delimited_list_builder.dart

@@ -61,23 +61,13 @@ class DelimitedListBuilder {
   }
 
   /// Adds the opening [bracket] to the built list.
-  ///
-  /// If [delimiter] is given, it is a second bracket occurring immediately
-  /// after [bracket]. This is used for parameter lists where all parameters
-  /// are optional or named, as in:
-  ///
-  ///     function([parameter]);
-  ///
-  /// Here, [bracket] will be `(` and [delimiter] will be `[`.
-  void leftBracket(Token bracket, {Piece? preceding, Token? delimiter}) {
-    _leftBracket = _visitor.buildPiece((b) {
-      if (preceding != null) {
-        b.add(preceding);
-        b.space();
-      }
-      b.token(bracket);
-      b.token(delimiter);
-    });
+  void leftBracket(Token bracket) {
+    addLeftBracket(_visitor.tokenPiece(bracket));
+  }
+
+  /// Adds the opening bracket [piece] to the built list.
+  void addLeftBracket(Piece piece) {
+    _leftBracket = piece;
   }
 
   /// Adds the closing [bracket] to the built list along with any comments that