Better indentation for property access as binary operator operand. (#1615)

munificent · web-flow · commit 04883d622ebc · 2024-12-06T17:18:09.000-08:00
Better indentation for property access as binary operator operand.

While I was at it, I figured out a more systematic way to hoist leading comments out from the subexpressions containing the token that the comments precede. That fixes this bug and also lets us get rid of many of the one-off places where we were manually hoisting comments out of some subexpressions.

Surprisingly, even though this touches visitNode() which is a fairly hot method, it doesn't seem to measurably harm performance.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,7 +1,9 @@
 ## 3.0.1-wip
 
-* Ensure comment formatting is idempotent (#1606).
 * Handle comments and metadata before variables more gracefully (#1604).
+* Ensure comment formatting is idempotent (#1606).
+* Better indentation of leading comments on property accesses in binary operator
+  operands (#1611).
 
 ## 3.0.0
 
diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart
@@ -14,6 +14,7 @@ import '../piece/case.dart';
 import '../piece/constructor.dart';
 import '../piece/control_flow.dart';
 import '../piece/infix.dart';
+import '../piece/leading_comment.dart';
 import '../piece/list.dart';
 import '../piece/piece.dart';
 import '../piece/type.dart';
@@ -329,10 +330,6 @@ final class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
 
   @override
   void visitConditionalExpression(ConditionalExpression node) {
-    // Hoist any comments before the condition operand so they don't force the
-    // conditional expression to split.
-    var leadingComments = pieces.takeCommentsBefore(node.firstNonCommentToken);
-
     // Flatten a series of else-if-like chained conditionals into a single long
     // infix piece. This produces a flattened style like:
     //
@@ -380,7 +377,7 @@ final class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
       piece.pin(State.split);
     }
 
-    pieces.add(prependLeadingComments(leadingComments, piece));
+    pieces.add(piece);
   }
 
   @override
@@ -390,7 +387,17 @@ final class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
     pieces.token(node.leftParenthesis);
 
     if (node.equalToken case var equals?) {
-      writeInfix(node.name, equals, node.value!, hanging: true);
+      // Hoist comments so that they don't force the `==` to split.
+      pieces.hoistLeadingComments(node.name.firstNonCommentToken, () {
+        return InfixPiece([
+          pieces.build(() {
+            pieces.visit(node.name);
+            pieces.space();
+            pieces.token(equals);
+          }),
+          nodePiece(node.value!)
+        ]);
+      });
     } else {
       pieces.visit(node.name);
     }
@@ -1966,7 +1973,46 @@ final class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
     var previousContext = _parentContext;
     _parentContext = context;
 
-    node.accept(this);
+    // If there are comments before this node, then some of them may be leading
+    // comments. If so, capture them now. We do this here so that the comments
+    // are wrapped around the outermost possible Piece for the AST node. For
+    // example:
+    //
+    //     // Comment
+    //     a.b && c || d ? e : f;
+    //
+    // Here, the node that actually owns the token before the comment is `a`,
+    // which is an identifier expression inside a property access inside an
+    // `&&` expression inside an `||` expression inside `?:` expression. If we
+    // attach the comment to the identifier expression, then the newline from
+    // the comment will force all of those surrounding pieces to split:
+    //
+    //     // Comment
+    //     a
+    //                 .b &&
+    //             c ||
+    //             d
+    //         ? e
+    //         : f;
+    //
+    // Instead, we hoist the comment out of all of those and then have comment
+    // precede them all so that they don't split.
+    var firstToken = node.firstNonCommentToken;
+    if (firstToken.precedingComments != null) {
+      var comments = pieces.takeCommentsBefore(firstToken);
+      var piece = pieces.build(() {
+        node.accept(this);
+      });
+
+      // Check again because the preceding comments may not necessarily end up
+      // as leading comments.
+      if (comments.isNotEmpty) piece = LeadingCommentPiece(comments, piece);
+
+      pieces.add(piece);
+    } else {
+      // No preceding comments, so just visit it inline.
+      node.accept(this);
+    }
 
     _parentContext = previousContext;
   }
diff --git a/lib/src/front_end/chain_builder.dart b/lib/src/front_end/chain_builder.dart
@@ -8,6 +8,7 @@ import 'package:analyzer/dart/ast/token.dart';
 import '../ast_extensions.dart';
 import '../constants.dart';
 import '../piece/chain.dart';
+import '../piece/leading_comment.dart';
 import '../piece/list.dart';
 import '../piece/piece.dart';
 import 'piece_factory.dart';
@@ -75,18 +76,7 @@ final class ChainBuilder {
 
       // When [_root] is a cascade, the chain is the series of cascade sections.
       for (var section in cascade.cascadeSections) {
-        // Hoist any leading comment out so that if the cascade section is a
-        // setter, we don't unnecessarily split at the `=` like:
-        //
-        //     target
-        //       // comment
-        //         ..setter =
-        //             value;
-        var leadingComments =
-            _visitor.pieces.takeCommentsBefore(section.firstNonCommentToken);
-
-        var piece = _visitor.prependLeadingComments(
-            leadingComments, _visitor.nodePiece(section));
+        var piece = _visitor.nodePiece(section);
 
         var callType = switch (section) {
           // Force the cascade to split if there are leading comments before
@@ -96,7 +86,7 @@ final class ChainBuilder {
           //     ..method(
           //       argument,
           //     );
-          _ when leadingComments.isNotEmpty => CallType.unsplittableCall,
+          _ when piece is LeadingCommentPiece => CallType.unsplittableCall,
 
           // If the section is itself a method chain, then force the cascade to
           // split if the method does, as in:
diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart
@@ -12,7 +12,6 @@ import '../piece/control_flow.dart';
 import '../piece/for.dart';
 import '../piece/if_case.dart';
 import '../piece/infix.dart';
-import '../piece/leading_comment.dart';
 import '../piece/list.dart';
 import '../piece/piece.dart';
 import '../piece/sequence.dart';
@@ -285,15 +284,6 @@ mixin PieceFactory {
     return builder.build();
   }
 
-  /// If [leadingComments] is not empty, returns [piece] wrapped in a
-  /// [LeadingCommentPiece] that prepends them.
-  ///
-  /// Otherwise, just returns [piece].
-  Piece prependLeadingComments(List<Piece> leadingComments, Piece piece) {
-    if (leadingComments.isEmpty) return piece;
-    return LeadingCommentPiece(leadingComments, piece);
-  }
-
   /// Writes the leading keyword and parenthesized expression at the beginning
   /// of an `if`, `while`, or `switch` expression or statement.
   void writeControlFlowStart(Token keyword, Token leftParenthesis,
@@ -500,21 +490,18 @@ mixin PieceFactory {
 
           // 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 `(`.
-          var forInPiece =
-              pieces.build(metadata: metadata, inlineMetadata: true, () {
-            pieces.token(keyword);
-            pieces.space();
-            _writeForIn(pattern, forEachParts.inKeyword, forEachParts.iterable);
+          pieces.hoistLeadingComments(
+              metadata.firstOrNull?.beginToken ?? keyword, () {
+            // Use a nested piece so that the metadata precedes the keyword and
+            // not the `(`.
+            return pieces.build(metadata: metadata, inlineMetadata: true, () {
+              pieces.token(keyword);
+              pieces.space();
+              _writeForIn(
+                  pattern, forEachParts.inKeyword, forEachParts.iterable);
+            });
           });
 
-          pieces.add(prependLeadingComments(leadingComments, forInPiece));
           pieces.token(rightParenthesis);
         });
     }
@@ -645,22 +632,21 @@ mixin PieceFactory {
     //     int f() {}
     var firstToken =
         modifiers.nonNulls.firstOrNull ?? returnType.firstNonCommentToken;
-    var leadingComments = pieces.takeCommentsBefore(firstToken);
+    pieces.hoistLeadingComments(firstToken, () {
+      var returnTypePiece = pieces.build(() {
+        for (var keyword in modifiers) {
+          pieces.modifier(keyword);
+        }
 
-    var returnTypePiece = pieces.build(() {
-      for (var keyword in modifiers) {
-        pieces.modifier(keyword);
-      }
+        pieces.visit(returnType);
+      });
 
-      pieces.visit(returnType);
-    });
+      var signature = pieces.build(() {
+        writeFunction();
+      });
 
-    var signature = pieces.build(() {
-      writeFunction();
+      return VariablePiece(returnTypePiece, [signature], hasType: true);
     });
-
-    pieces.add(prependLeadingComments(leadingComments,
-        VariablePiece(returnTypePiece, [signature], hasType: true)));
   }
 
   /// If [parameter] has a [defaultValue] then writes a piece for the parameter
@@ -941,10 +927,6 @@ mixin PieceFactory {
   /// separate tokens, as in `foo is! Bar`.
   void writeInfix(AstNode left, Token operator, AstNode right,
       {bool hanging = false, Token? operator2}) {
-    // Hoist any comments before the first operand so they don't force the
-    // infix operator to split.
-    var leadingComments = pieces.takeCommentsBefore(left.firstNonCommentToken);
-
     var leftPiece = pieces.build(() {
       pieces.visit(left);
       if (hanging) {
@@ -964,8 +946,7 @@ mixin PieceFactory {
       pieces.visit(right);
     });
 
-    pieces.add(prependLeadingComments(
-        leadingComments, InfixPiece([leftPiece, rightPiece])));
+    pieces.add(InfixPiece([leftPiece, rightPiece]));
   }
 
   /// Writes a chained infix operation: a binary operator expression, or
@@ -986,10 +967,6 @@ mixin PieceFactory {
   void writeInfixChain<T extends AstNode>(
       T node, BinaryOperation Function(T node) destructure,
       {int? precedence, bool indent = true}) {
-    // Hoist any comments before the first operand so they don't force the
-    // infix operator to split.
-    var leadingComments = pieces.takeCommentsBefore(node.firstNonCommentToken);
-
     var operands = <Piece>[];
 
     void traverse(AstNode e) {
@@ -1017,8 +994,7 @@ mixin PieceFactory {
       traverse(node);
     }));
 
-    pieces.add(prependLeadingComments(
-        leadingComments, InfixPiece(operands, indent: indent)));
+    pieces.add(InfixPiece(operands, indent: indent));
   }
 
   /// Writes a [ListPiece] for the given bracket-delimited set of elements.
@@ -1421,17 +1397,14 @@ mixin PieceFactory {
   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.hoistLeadingComments(leftHandSide.firstNonCommentToken, () {
+      var leftPiece =
+          nodePiece(leftHandSide, context: NodeContext.forLoopVariable);
+      var sequencePiece = _createForInSequence(inKeyword, sequence);
 
-    pieces.add(prependLeadingComments(
-        leadingComments,
-        ForInPiece(leftPiece, sequencePiece,
-            canBlockSplitSequence: sequence.canBlockSplit)));
+      return ForInPiece(leftPiece, sequencePiece,
+          canBlockSplitSequence: sequence.canBlockSplit);
+    });
   }
 
   /// Writes the `<variable> in <expression>` part of a for-in loop when the
@@ -1444,28 +1417,24 @@ mixin PieceFactory {
       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);
-      });
+    pieces.hoistLeadingComments(identifier.beginToken, () {
+      // Use a nested piece so that the metadata precedes the keyword and
+      // not the `(`.
+      return pieces.build(metadata: identifier.metadata, inlineMetadata: true,
+          () {
+        var leftPiece = pieces.build(() {
+          writeParameter(
+              modifiers: [identifier.keyword],
+              identifier.type,
+              identifier.name);
+        });
 
-      var sequencePiece = _createForInSequence(inKeyword, sequence);
+        var sequencePiece = _createForInSequence(inKeyword, sequence);
 
-      pieces.add(ForInPiece(leftPiece, sequencePiece,
-          canBlockSplitSequence: sequence.canBlockSplit));
+        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.
diff --git a/lib/src/front_end/piece_writer.dart b/lib/src/front_end/piece_writer.dart
@@ -10,6 +10,7 @@ import '../back_end/solver.dart';
 import '../dart_formatter.dart';
 import '../debug.dart' as debug;
 import '../piece/adjacent.dart';
+import '../piece/leading_comment.dart';
 import '../piece/list.dart';
 import '../piece/piece.dart';
 import '../piece/text.dart';
@@ -302,6 +303,28 @@ final class PieceWriter {
     return _splitComments(_comments.takeCommentsBefore(token), token);
   }
 
+  /// Takes any comments preceding [firstToken] and wraps them around the piece
+  /// generated by [buildCallback].
+  ///
+  /// For comments preceding a single AST node, this hoisting is handled
+  /// automatically. But there are some places in the language where there is
+  /// a conceptual piece of syntax that we want to hoist the comments out of
+  /// so that the syntax doesn't split but there isn't actually a single AST
+  /// node associated with that syntax.
+  ///
+  /// This method lets you hoist comments before an arbitary amount of syntax
+  /// visited and built by calling [buildCallback].
+  void hoistLeadingComments(Token firstToken, Piece Function() buildCallback) {
+    var leadingComments = takeCommentsBefore(firstToken);
+
+    var piece = buildCallback();
+    if (leadingComments.isNotEmpty) {
+      piece = LeadingCommentPiece(leadingComments, piece);
+    }
+
+    add(piece);
+  }
+
   /// Begins a new [CodeToken] that can potentially have more code written to
   /// it.
   void _beginCodeToken(Token token) {
diff --git a/test/tall/expression/binary_comment.stmt b/test/tall/expression/binary_comment.stmt
diff --git a/test/tall/regression/1600/1611.unit b/test/tall/regression/1600/1611.unit
