Better indentation for property access as binary operator operand.

CHANGELOG.md

@@ -1,6 +1,8 @@
 ## 3.0.1-wip
 
 * Ensure comment formatting is idempotent (#1606).
+* Better indentation of leading comments on property accesses in binary operator
+  operands (#1611).
 
 ## 3.0.0
 

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

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:

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,
@@ -628,22 +618,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
@@ -924,10 +913,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) {
@@ -947,8 +932,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
@@ -969,10 +953,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) {
@@ -1000,8 +980,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.

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';
@@ -295,6 +296,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) {

test/tall/expression/binary_comment.stmt

@@ -94,4 +94,26 @@ value =
 <<<
 value =
     // comment
-    a + b + c;
+    a + b + c;
+>>> Comment before unsplit call chain operand.
+firstOperand &&
+    // Comment.
+    longTarget.someLongProperty &&
+    lastOperand;
+<<<
+firstOperand &&
+    // Comment.
+    longTarget.someLongProperty &&
+    lastOperand;
+>>> Comment before split call chain operand.
+firstOperand &&
+    // Comment.
+    longTarget.someLongProperty.anotherProperty &&
+    lastOperand;
+<<<
+firstOperand &&
+    // Comment.
+    longTarget
+        .someLongProperty
+        .anotherProperty &&
+    lastOperand;

test/tall/regression/1600/1611.unit

@@ -0,0 +1,38 @@
+>>>
+aaaa aaAaaAaaaaAaaaaaaaaaAaaAaaAaaAaaaaaa(
+  AaaaaaaaaaAaaaAaaaaaaaaAaaaaaa aaaaAaaaaaa,
+) =>
+    // Aaaa for Aaaaaaa AA aaaaaaaa.
+    aaAaAaaaaaaaAaaaaaa &&
+    // Aaaaaaaaaa is aaaaaaaa aaaa aaa Aaa Aaaaa Aaaaa aaaaaaaaaa.
+        aaaaAaaaaaa
+        .aaaAaaaAaaAaaaAaaa(
+          AaaaAaaaaaaaaaAaaa_Aaaa.AAA_AAA_AAAAA_AAA_AAAAAAAAAA__AAAA,
+          aAaaaaaaAaAaaaaaaaaAaaaaaAaaaaaaaAaaaaaa: true,
+        )
+        .aaAaAaaAaaaaaaaa &&
+    aaaaAaaaaaa
+        .aaaAaaaAaaAaaaAaaa(
+          AaaaAaaaaaaaaaAaaa_Aaaa.AAA_AA_AAAAAAA_AAA_AAA_A1__AAAA,
+          aAaaaaaaAaAaaaaaaaaAaaaaaAaaaaaaaAaaaaaa: true,
+        )
+        .aaAaAaaAaaaaaaaa;
+<<<
+aaaa aaAaaAaaaaAaaaaaaaaaAaaAaaAaaAaaaaaa(
+  AaaaaaaaaaAaaaAaaaaaaaaAaaaaaa aaaaAaaaaaa,
+) =>
+    // Aaaa for Aaaaaaa AA aaaaaaaa.
+    aaAaAaaaaaaaAaaaaaa &&
+    // Aaaaaaaaaa is aaaaaaaa aaaa aaa Aaa Aaaaa Aaaaa aaaaaaaaaa.
+    aaaaAaaaaaa
+        .aaaAaaaAaaAaaaAaaa(
+          AaaaAaaaaaaaaaAaaa_Aaaa.AAA_AAA_AAAAA_AAA_AAAAAAAAAA__AAAA,
+          aAaaaaaaAaAaaaaaaaaAaaaaaAaaaaaaaAaaaaaa: true,
+        )
+        .aaAaAaaAaaaaaaaa &&
+    aaaaAaaaaaa
+        .aaaAaaaAaaAaaaAaaa(
+          AaaaAaaaaaaaaaAaaa_Aaaa.AAA_AA_AAAAAAA_AAA_AAA_A1__AAAA,
+          aAaaaaaaAaAaaaaaaaaAaaaaaAaaaaaaaAaaaaaa: true,
+        )
+        .aaAaAaaAaaaaaaaa;