Handle comments and metadata before variables more gracefully.

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## 3.0.1-wip
 
 * Ensure comment formatting is idempotent (#1606).
+* Handle comments and metadata before variables more gracefully (#1604).
 
 ## 3.0.0
 

lib/src/front_end/ast_node_visitor.dart

@@ -765,32 +765,32 @@ final class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
 
   @override
   void visitForEachPartsWithDeclaration(ForEachPartsWithDeclaration node) {
-    throw UnsupportedError('This node is handled by createFor().');
+    throw UnsupportedError('This node is handled by writeFor().');
   }
 
   @override
   void visitForEachPartsWithIdentifier(ForEachPartsWithIdentifier node) {
-    throw UnsupportedError('This node is handled by createFor().');
+    throw UnsupportedError('This node is handled by writeFor().');
   }
 
   @override
   void visitForEachPartsWithPattern(ForEachPartsWithPattern node) {
-    throw UnsupportedError('This node is handled by createFor().');
+    throw UnsupportedError('This node is handled by writeFor().');
   }
 
   @override
   void visitForPartsWithDeclarations(ForPartsWithDeclarations node) {
-    throw UnsupportedError('This node is handled by createFor().');
+    throw UnsupportedError('This node is handled by writeFor().');
   }
 
   @override
   void visitForPartsWithExpression(ForPartsWithExpression node) {
-    throw UnsupportedError('This node is handled by createFor().');
+    throw UnsupportedError('This node is handled by writeFor().');
   }
 
   @override
   void visitForPartsWithPattern(ForPartsWithPattern node) {
-    throw UnsupportedError('This node is handled by createFor().');
+    throw UnsupportedError('This node is handled by writeFor().');
   }
 
   @override

lib/src/front_end/delimited_list_builder.dart

@@ -138,6 +138,11 @@ final class DelimitedListBuilder {
     _commentsBeforeComma = CommentSequence.empty;
   }
 
+  /// Adds all of [pieces] to the built list.
+  void addAll(Iterable<Piece> pieces) {
+    pieces.forEach(add);
+  }
+
   /// Adds the contents of [lineBuilder] to this outer [DelimitedListBuilder].
   ///
   /// This is used when preserving newlines inside a collection literal. The

lib/src/front_end/piece_factory.dart

@@ -437,9 +437,16 @@ mixin PieceFactory {
         forPartsPiece = partsList.build();
 
       case ForEachParts forEachParts &&
-            ForEachPartsWithDeclaration(loopVariable: AstNode variable):
+            ForEachPartsWithDeclaration(:var loopVariable):
+        forPartsPiece = pieces.build(() {
+          pieces.token(leftParenthesis);
+          _writeDeclaredForIn(
+              loopVariable, forEachParts.inKeyword, forEachParts.iterable);
+          pieces.token(rightParenthesis);
+        });
+
       case ForEachParts forEachParts &&
-            ForEachPartsWithIdentifier(identifier: AstNode variable):
+            ForEachPartsWithIdentifier(:var identifier):
         // If a for-in loop, treat the for parts like an assignment, so they
         // split like:
         //
@@ -481,7 +488,8 @@ mixin PieceFactory {
         // statement or element.
         forPartsPiece = pieces.build(() {
           pieces.token(leftParenthesis);
-          writeForIn(variable, forEachParts.inKeyword, forEachParts.iterable);
+          _writeForIn(
+              identifier, forEachParts.inKeyword, forEachParts.iterable);
           pieces.token(rightParenthesis);
         });
 
@@ -490,14 +498,23 @@ mixin PieceFactory {
         forPartsPiece = pieces.build(() {
           pieces.token(leftParenthesis);
 
+          // Hoist any leading comments so they don't force the for-in clauses
+          // to split.
+          var leadingComments = const <Piece>[];
+          if (metadata.isEmpty) {
+            leadingComments = pieces.takeCommentsBefore(keyword);
+          }
+
           // Use a nested piece so that the metadata precedes the keyword and
           // not the `(`.
-          pieces.withMetadata(metadata, inlineMetadata: true, () {
+          var forInPiece =
+              pieces.build(metadata: metadata, inlineMetadata: true, () {
             pieces.token(keyword);
             pieces.space();
-
-            writeForIn(pattern, forEachParts.inKeyword, forEachParts.iterable);
+            _writeForIn(pattern, forEachParts.inKeyword, forEachParts.iterable);
           });
+
+          pieces.add(prependLeadingComments(leadingComments, forInPiece));
           pieces.token(rightParenthesis);
         });
     }
@@ -1399,21 +1416,66 @@ mixin PieceFactory {
         canBlockSplitRight: canBlockSplitRight));
   }
 
-  /// Writes a [Piece] for the `<variable> in <expression>` part of a for-in
-  /// loop.
-  void writeForIn(AstNode leftHandSide, Token inKeyword, Expression sequence) {
+  /// Writes the `<variable> in <expression>` part of an identifier or pattern
+  /// for-in loop.
+  void _writeForIn(AstNode leftHandSide, Token inKeyword, Expression sequence) {
+    // Hoist any leading comments so they don't force the for-in clauses to
+    // split.
+    var leadingComments =
+        pieces.takeCommentsBefore(leftHandSide.firstNonCommentToken);
+
     var leftPiece =
         nodePiece(leftHandSide, context: NodeContext.forLoopVariable);
+    var sequencePiece = _createForInSequence(inKeyword, sequence);
+
+    pieces.add(prependLeadingComments(
+        leadingComments,
+        ForInPiece(leftPiece, sequencePiece,
+            canBlockSplitSequence: sequence.canBlockSplit)));
+  }
+
+  /// Writes the `<variable> in <expression>` part of a for-in loop when the
+  /// part before `in` is a variable declaration.
+  ///
+  /// A for-in loop with a variable declaration can have metadata before it,
+  /// which requires some special handling so that we don't push the metadata
+  /// and any comments after it into the left child piece of [ForInPiece].
+  void _writeDeclaredForIn(
+      DeclaredIdentifier identifier, Token inKeyword, Expression sequence) {
+    // Hoist any leading comments so they don't force the for-in clauses
+    // to split.
+    var leadingComments = const <Piece>[];
+    if (identifier.metadata.isEmpty) {
+      leadingComments = pieces
+          .takeCommentsBefore(identifier.firstTokenAfterCommentAndMetadata);
+    }
+
+    // Use a nested piece so that the metadata precedes the keyword and
+    // not the `(`.
+    var forInPiece =
+        pieces.build(metadata: identifier.metadata, inlineMetadata: true, () {
+      var leftPiece = pieces.build(() {
+        writeParameter(
+            modifiers: [identifier.keyword], identifier.type, identifier.name);
+      });
 
-    var sequencePiece = pieces.build(() {
+      var sequencePiece = _createForInSequence(inKeyword, sequence);
+
+      pieces.add(ForInPiece(leftPiece, sequencePiece,
+          canBlockSplitSequence: sequence.canBlockSplit));
+    });
+
+    pieces.add(prependLeadingComments(leadingComments, forInPiece));
+  }
+
+  /// Creates a piece for the `in <sequence>` part of a for-in loop.
+  Piece _createForInSequence(Token inKeyword, Expression sequence) {
+    return pieces.build(() {
       // Put the `in` at the beginning of the sequence.
       pieces.token(inKeyword);
       pieces.space();
       pieces.visit(sequence);
     });
-
-    pieces.add(ForInPiece(leftPiece, sequencePiece,
-        canBlockSplitSequence: sequence.canBlockSplit));
   }
 
   /// Writes a piece for a parameter-like constructor: Either a simple formal

lib/src/front_end/piece_writer.dart

@@ -177,11 +177,18 @@ final class PieceWriter {
     _flushSpace();
     _currentCode = null;
 
-    var metadataPieces = const <Piece>[];
+    var leadingPieces = const <Piece>[];
     if (metadata.isNotEmpty) {
-      metadataPieces = [
+      leadingPieces = [
         for (var annotation in metadata) _visitor.nodePiece(annotation)
       ];
+
+      // If there are comments between the metadata and declaration, then hoist
+      // them out too so they don't get embedded inside the beginning piece of
+      // the declaration. [SequenceBuilder] handles that for most comments
+      // preceding a declaration but won't see these ones because they come
+      // after the metadata.
+      leadingPieces.addAll(takeCommentsBefore(metadata.last.endToken.next!));
     }
 
     _pieces.add([]);
@@ -198,7 +205,7 @@ final class PieceWriter {
         ? builtPieces.first
         : AdjacentPiece(builtPieces);
 
-    if (metadataPieces.isEmpty) {
+    if (leadingPieces.isEmpty) {
       // No metadata, so return the content piece directly.
       return builtPiece;
     } else if (inlineMetadata) {
@@ -210,7 +217,7 @@ final class PieceWriter {
             spaceWhenUnsplit: true,
           ));
 
-      for (var piece in metadataPieces) {
+      for (var piece in leadingPieces) {
         list.add(piece);
       }
 
@@ -219,7 +226,7 @@ final class PieceWriter {
     } else {
       // Wrap the metadata and content in a sequence.
       var sequence = SequenceBuilder(_visitor);
-      for (var piece in metadataPieces) {
+      for (var piece in leadingPieces) {
         sequence.add(piece);
       }
 

test/tall/declaration/metadata_comment.unit

@@ -16,4 +16,100 @@ class C {}
   'DatabaseCallback',
 ) // deprecated
 @Experimental()
-class C {}
+class C {}
+>>> Comment between metadata and field doesn't force type to split.
+class C {
+  @meta
+  // c
+  var a;
+
+  @meta
+  // c
+  int b;
+
+  @meta
+  // c
+  final int c;
+
+  @meta
+  // c
+  late var d;
+
+  @meta
+  // c
+  late int e;
+
+  @meta
+  // c
+  late final int f;
+
+  @meta
+  // c
+  static var g;
+
+  @meta
+  // c
+  static int h;
+
+  @meta
+  // c
+  static final int y;
+}
+<<<
+class C {
+  @meta
+  // c
+  var a;
+
+  @meta
+  // c
+  int b;
+
+  @meta
+  // c
+  final int c;
+
+  @meta
+  // c
+  late var d;
+
+  @meta
+  // c
+  late int e;
+
+  @meta
+  // c
+  late final int f;
+
+  @meta
+  // c
+  static var g;
+
+  @meta
+  // c
+  static int h;
+
+  @meta
+  // c
+  static final int y;
+}
+>>> Comment between metadata and method doesn't force return type to split.
+class C {
+  @meta
+  // c
+  int a() {}
+
+  @meta
+  // c
+  static int b() {}
+}
+<<<
+class C {
+  @meta
+  // c
+  int a() {}
+
+  @meta
+  // c
+  static int b() {}
+}

test/tall/regression/1600/1604.unit

@@ -0,0 +1,12 @@
+>>>
+class C {
+  @override
+  // ignore: hash_and_equals
+  final int hashCode;
+}
+<<<
+class C {
+  @override
+  // ignore: hash_and_equals
+  final int hashCode;
+}