Format for-in loops. (#1314)

Format for-in loops.

I ended up refactoring the code for handling the for parts and inlining
it all into visitForStatement() because it turns out that there isn't
as much sharing between C-style for loops and for-in loops as I was
expecting.

Thanks to the magic of pattern matching and shared case bodies, I was
still able to get it all into a single flattened switch statement for
all the kinds of for parts.

Author: munificent

lib/src/front_end/ast_node_visitor.dart

@@ -339,7 +339,9 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @override
   void visitDeclaredIdentifier(DeclaredIdentifier node) {
-    throw UnimplementedError();
+    modifier(node.keyword);
+    visit(node.type, after: space);
+    token(node.name);
   }
 
   @override
@@ -481,66 +483,114 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @override
   void visitForStatement(ForStatement node) {
+    modifier(node.awaitKeyword);
     token(node.forKeyword);
     var forKeyword = pieces.split();
 
-    // Treat the for loop parts sort of as an argument list where each clause
-    // is a separate argument. This means that when they split, they split like:
-    //
-    // ```
-    // for (
-    //   initializerClause;
-    //   conditionClause;
-    //   incrementClause
-    // ) {
-    //   body;
-    // }
-    // ```
-    var partsList =
-        DelimitedListBuilder(this, const ListStyle(commas: Commas.none));
-    partsList.leftBracket(node.leftParenthesis);
-
-    var forLoopParts = node.forLoopParts;
-    switch (forLoopParts) {
+    Piece forPartsPiece;
+    switch (node.forLoopParts) {
       // Edge case: A totally empty for loop is formatted just as `(;;)` with
       // no splits or spaces anywhere.
       case ForPartsWithExpression(
-            initialization: null,
-            leftSeparator: Token(precedingComments: null),
-            condition: null,
-            rightSeparator: Token(precedingComments: null),
-            updaters: NodeList(isEmpty: true),
-          )
+                initialization: null,
+                leftSeparator: Token(precedingComments: null),
+                condition: null,
+                rightSeparator: Token(precedingComments: null),
+                updaters: NodeList(isEmpty: true),
+              ) &&
+              var forParts
           when node.rightParenthesis.precedingComments == null:
-        token(forLoopParts.leftSeparator);
-        token(forLoopParts.rightSeparator);
+        token(node.leftParenthesis);
+        token(forParts.leftSeparator);
+        token(forParts.rightSeparator);
+        token(node.rightParenthesis);
+        forPartsPiece = pieces.split();
+
+      case ForParts forParts &&
+            ForPartsWithDeclarations(variables: AstNode? initializer):
+      case ForParts forParts &&
+            ForPartsWithExpression(initialization: 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:
+        //
+        // ```
+        // for (
+        //   initializerClause;
+        //   conditionClause;
+        //   incrementClause
+        // ) {
+        //   body;
+        // }
+        // ```
+        var partsList =
+            DelimitedListBuilder(this, const ListStyle(commas: Commas.none));
+        partsList.leftBracket(node.leftParenthesis);
+
+        // The initializer clause.
+        if (initializer != null) {
+          partsList.addCommentsBefore(initializer.beginToken);
+          visit(initializer);
+        } else {
+          // No initializer, so look at the comments before `;`.
+          partsList.addCommentsBefore(forParts.leftSeparator);
+        }
+
+        token(forParts.leftSeparator);
+        partsList.add(pieces.split());
+
+        // The condition clause.
+        if (forParts.condition case var conditionExpression?) {
+          partsList.addCommentsBefore(conditionExpression.beginToken);
+          visit(conditionExpression);
+        } else {
+          partsList.addCommentsBefore(forParts.rightSeparator);
+        }
 
-      case ForPartsWithDeclarations():
-        partsList.addCommentsBefore(forLoopParts.variables.beginToken);
-        visit(forLoopParts.variables);
-        finishForParts(forLoopParts, partsList);
+        token(forParts.rightSeparator);
+        partsList.add(pieces.split());
 
-      case ForPartsWithExpression(:var initialization?):
-        partsList.addCommentsBefore(initialization.beginToken);
-        visit(initialization);
-        finishForParts(forLoopParts, partsList);
+        // The update clauses.
+        if (forParts.updaters.isNotEmpty) {
+          partsList.addCommentsBefore(forParts.updaters.first.beginToken);
+          createList(forParts.updaters,
+              style: const ListStyle(commas: Commas.nonTrailing));
+          partsList.add(pieces.split());
+        }
 
-      case ForPartsWithExpression():
-        // No initializer part.
-        partsList.addCommentsBefore(forLoopParts.leftSeparator);
-        finishForParts(forLoopParts, partsList);
+        partsList.rightBracket(node.rightParenthesis);
+        pieces.split();
+        forPartsPiece = partsList.build();
 
       case ForPartsWithPattern():
-      case ForEachPartsWithDeclaration():
-      case ForEachPartsWithIdentifier():
+        throw UnimplementedError();
+
+      case ForEachParts forEachParts &&
+            ForEachPartsWithDeclaration(loopVariable: AstNode variable):
+      case ForEachParts forEachParts &&
+            ForEachPartsWithIdentifier(identifier: AstNode variable):
+        // If a for-in loop, treat the for parts like an assignment, so they
+        // split like:
+        //
+        // ```
+        // for (var variable in [
+        //   initializer,
+        // ]) {
+        //   body;
+        // }
+        // ```
+        token(node.leftParenthesis);
+        visit(variable);
+
+        finishAssignment(forEachParts.inKeyword, forEachParts.iterable,
+            splitBeforeOperator: true);
+        token(node.rightParenthesis);
+        forPartsPiece = pieces.split();
+
       case ForEachPartsWithPattern():
         throw UnimplementedError();
     }
 
-    partsList.rightBracket(node.rightParenthesis);
-    var forPartsPiece = partsList.build();
-
-    pieces.split();
     visit(node.body);
     var body = pieces.take();
 

lib/src/front_end/piece_factory.dart

@@ -561,22 +561,38 @@ mixin PieceFactory implements CommentWriter {
     if (body.keyword != null) space();
   }
 
-  /// Creates a [Piece] for some code followed by an `=` and an expression in
-  /// any place where an `=` appears:
+  /// Creates a [Piece] with "assignment-like" splitting.
+  ///
+  /// This is used, obviously, for assignments and variable declarations to
+  /// handle splitting after the `=`, but is also used in any context where an
+  /// expression follows something that it "defines" or "initializes":
   ///
   /// * Assignment
   /// * Variable declaration
   /// * Constructor initializer
+  /// * Expression (`=>`) function body
+  /// * Named argument or named record field (`:`)
+  /// * Map entry (`:`)
+  /// * For-in loop iterator (`in`)
   ///
-  /// This is also used for map literal entries and named arguments which are
-  /// also sort of like bindings. In that case, [operator] is the `:`.
-  ///
-  /// This method assumes the code to the left of the `=` or `:` has already
+  /// This method assumes the code to the left of the operator has already
   /// been visited.
-  void finishAssignment(Token operator, Expression rightHandSide) {
-    if (operator.type == TokenType.EQ) space();
-    token(operator);
-    var target = pieces.split();
+  ///
+  /// If [splitBeforeOperator] is `true`, then puts [operator] at the beginning
+  /// of the next line when it splits. Otherwise, puts the operator at the end
+  /// of the preceding line.
+  void finishAssignment(Token operator, Expression rightHandSide,
+      {bool splitBeforeOperator = false}) {
+    Piece target;
+    if (splitBeforeOperator) {
+      target = pieces.split();
+      token(operator);
+      space();
+    } else {
+      if (operator.type == TokenType.EQ) space();
+      token(operator);
+      target = pieces.split();
+    }
 
     visit(rightHandSide);
 
@@ -594,32 +610,6 @@ mixin PieceFactory implements CommentWriter {
         rightBracket: argumentList.rightParenthesis);
   }
 
-  /// Writes the condition and updaters parts of a [ForParts] after the
-  /// subclass's initializer clause has been written.
-  void finishForParts(ForParts forLoopParts, DelimitedListBuilder partsList) {
-    token(forLoopParts.leftSeparator);
-    partsList.add(pieces.split());
-
-    // The condition clause.
-    if (forLoopParts.condition case var conditionExpression?) {
-      partsList.addCommentsBefore(conditionExpression.beginToken);
-      visit(conditionExpression);
-    } else {
-      partsList.addCommentsBefore(forLoopParts.rightSeparator);
-    }
-
-    token(forLoopParts.rightSeparator);
-    partsList.add(pieces.split());
-
-    // The update clauses.
-    if (forLoopParts.updaters.isNotEmpty) {
-      partsList.addCommentsBefore(forLoopParts.updaters.first.beginToken);
-      createList(forLoopParts.updaters,
-          style: const ListStyle(commas: Commas.nonTrailing));
-      partsList.add(pieces.split());
-    }
-  }
-
   /// Finishes writing a named function declaration or anonymous function
   /// expression after the return type (if any) and name (if any) has been
   /// written.

lib/src/piece/assign.dart

@@ -10,7 +10,8 @@ import 'piece.dart';
 ///
 /// This piece is also used for map entries and named arguments where `:` is
 /// followed by an expression or element because those also want to support the
-/// "block-like" formatting of delimited expressions on the right.
+/// "block-like" formatting of delimited expressions on the right, and for the
+/// `in` clause in for-in loops.
 ///
 /// These constructs can be formatted three ways:
 ///
@@ -42,18 +43,21 @@ class AssignPiece extends Piece {
   /// This is only allowed when the value is a delimited expression.
   static const State _insideValue = State(1);
 
-  /// Split after the `=` and allow splitting inside the value.
+  /// Split at the operator and allow splitting inside the value.
   ///
   /// This is more costly because, when it's possible to split inside a
   /// delimited value, we want to prefer that.
-  static const State _atEquals = State(2, cost: 2);
+  static const State _atOperator = State(2, cost: 2);
 
-  /// The left-hand side of the `=` and the `=` itself.
+  /// The left-hand side of the operation. Includes the operator unless it is
+  /// `in`.
   final Piece target;
 
-  /// The right-hand side of the `=`.
+  /// The right-hand side of the operation.
   final Piece value;
 
+  /// Whether the right-hand side is a delimited expression that should receive
+  /// block-like formatting.
   final bool _isValueDelimited;
 
   AssignPiece(this.target, this.value, {required bool isValueDelimited})
@@ -104,7 +108,7 @@ class AssignPiece extends Piece {
 
   @override
   List<State> get additionalStates =>
-      [if (_isValueDelimited) _insideValue, _atEquals];
+      [if (_isValueDelimited) _insideValue, _atOperator];
 
   @override
   void format(CodeWriter writer, State state) {
@@ -116,7 +120,7 @@ class AssignPiece extends Piece {
     if (state != _insideValue) writer.setIndent(Indent.expression);
 
     writer.format(target);
-    writer.splitIf(state == _atEquals);
+    writer.splitIf(state == _atOperator);
     writer.format(value);
   }
 

test/statement/for_in.stmt

@@ -0,0 +1,79 @@
+40 columns                              |
+>>> Declare variable with `var`.
+for  (  var  file  in  files  )  {  body  ;  }
+<<<
+for (var file in files) {
+  body;
+}
+>>> Declare with type annotation.
+for (  List  <  int  >  ints  in  intLists  )  {  body  ;  }
+<<<
+for (List<int> ints in intLists) {
+  body;
+}
+>>> Declare variable with `final` and type.
+for (final Foo foo in foos) { body; }
+<<<
+for (final Foo foo in foos) {
+  body;
+}
+>>> Declare variable with just `final`.
+for (final foo in foos) {
+  body;
+}
+<<<
+for (final foo in foos) {
+  body;
+}
+>>> Use existing variable.
+for (  x  in  things  )  { body; }
+<<<
+for (x in things) {
+  body;
+}
+>>> Await for with declared variable.
+foo() async {
+  await  for  (  var  x  in  y  )  {  body  ;  }
+}
+<<<
+foo() async {
+  await for (var x in y) {
+    body;
+  }
+}
+>>> Await for with existing variable.
+foo() async {
+  await  for  (  x  in  y  )  {  body  ;  }
+}
+<<<
+foo() async {
+  await for (x in y) {
+    body;
+  }
+}
+>>> Split before `in`.
+for (var identifier in iteratableExpression) { body; }
+<<<
+for (var identifier
+    in iteratableExpression) {
+  body;
+}
+>>> Prefer block-like splitting after `in`.
+for (var identifier in [element, element, element]) { body; }
+<<<
+for (var identifier in [
+  element,
+  element,
+  element,
+]) {
+  body;
+}
+>>> Unsplit non-block body.
+for (i in sequence) something(i);
+<<<
+for (i in sequence) something(i);
+>>> Split non-block body.
+for (i in sequence) somethingMuchLonger(i);
+<<<
+for (i in sequence)
+  somethingMuchLonger(i);