Handle comments and metadata before variables more gracefully. (#1612)

Handle comments and metadata before variables more gracefully.

Comments and metadata before a declaration are always tricky because the formatter by default attaches comments to the innermost piece following the comment. So in:

```dart
// Comment
int field;
```

We want the comment attached to the Piece for the entire field declaration. But the formatter won't see the comment until it hits the `int` token and will end up attaching it to the piece for the type annotation which is embedded inside the field piece. That in turn means that the formatter will think there is a newline inside the type (because there is one between the comment and `int`) which then forces a split after the type annotation too:

```dart
// Comment
int
field;
```

In most places, this is handled by having the surrounding SequenceBuilder grab the comment before visiting the subsequent declaration. But that doesn't work when you have a comment after metadata:

```dart
@meta
// Comment
int field;
```

Now the comment isn't before the field declaration. It's stuck inside it. But we still want to hoist it out.

This PR fixes that for every place I could find: fields, functions, variables, and for-loop variables. The latter is particularly hard because for-in loops have some weird formatting already and it's also just a weird place for splits to occur.

I wish I had a cleaner more systematic way of handling these but despite trying for most of today, I haven't been able to come up with a cleaner approach. This at least gets the formatter producing better output.

Fix #1604.

Author: munificent

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