diff --git a/CHANGELOG.md b/CHANGELOG.md index e9b613c2..f6235f10 100644 --- 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 index 4d727ec7..8b084314 100644 --- 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 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 with PieceFactory { piece.pin(State.split); } - pieces.add(prependLeadingComments(leadingComments, piece)); + pieces.add(piece); } @override @@ -390,7 +387,17 @@ final class AstNodeVisitor extends ThrowingAstVisitor 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 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 index 4cb114f5..dd0cc123 100644 --- 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 index f2e8c9a5..b97132f5 100644 --- 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 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 []; - 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 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 = []; 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 ` in ` 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 []; - 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 ` part of a for-in loop. diff --git a/lib/src/front_end/piece_writer.dart b/lib/src/front_end/piece_writer.dart index f4535a87..171b5e2a 100644 --- 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 index 524a496f..99185402 100644 --- a/test/tall/expression/binary_comment.stmt +++ b/test/tall/expression/binary_comment.stmt @@ -94,4 +94,26 @@ value = <<< value = // comment - a + b + c; \ No newline at end of file + 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; diff --git a/test/tall/regression/1600/1611.unit b/test/tall/regression/1600/1611.unit new file mode 100644 index 00000000..3b3afa0c --- /dev/null +++ b/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; \ No newline at end of file