From efeef51a4d2f1788f6b85f133bb6fd18682f0a80 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Fri, 6 Dec 2024 11:09:28 -0800 Subject: [PATCH 1/3] Fix the name of a regression test directory. It was supposed to be rounded down to the nearest hundred. Oops. --- test/tall/regression/{1606 => 1600}/1606.unit | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/tall/regression/{1606 => 1600}/1606.unit (100%) diff --git a/test/tall/regression/1606/1606.unit b/test/tall/regression/1600/1606.unit similarity index 100% rename from test/tall/regression/1606/1606.unit rename to test/tall/regression/1600/1606.unit From 8a5d46fe0909256ab7f55ace98ec2632a527ee74 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Fri, 6 Dec 2024 12:40:23 -0800 Subject: [PATCH 2/3] 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. --- CHANGELOG.md | 2 + lib/src/front_end/ast_node_visitor.dart | 60 +++++++++++++++++++++--- lib/src/front_end/chain_builder.dart | 16 ++----- lib/src/front_end/piece_factory.dart | 47 +++++-------------- lib/src/front_end/piece_writer.dart | 23 +++++++++ test/tall/expression/binary_comment.stmt | 24 +++++++++- test/tall/regression/1600/1611.unit | 38 +++++++++++++++ 7 files changed, 155 insertions(+), 55 deletions(-) create mode 100644 test/tall/regression/1600/1611.unit diff --git a/CHANGELOG.md b/CHANGELOG.md index aec802fd..9d682f6e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index 78701316..9f516f26 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 3d0f2752..4026c004 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, @@ -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 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) { @@ -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. diff --git a/lib/src/front_end/piece_writer.dart b/lib/src/front_end/piece_writer.dart index 970edc94..a888e251 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'; @@ -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) { 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 From 5317abfa583443349ae22ab9fcd4204f1d83b628 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Fri, 6 Dec 2024 17:12:37 -0800 Subject: [PATCH 3/3] Update to use hoistLeadingComments(). --- lib/src/front_end/piece_factory.dart | 74 ++++++++++++---------------- 1 file changed, 32 insertions(+), 42 deletions(-) diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart index fc927a1e..b97132f5 100644 --- a/lib/src/front_end/piece_factory.dart +++ b/lib/src/front_end/piece_factory.dart @@ -490,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); }); } @@ -1400,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 @@ -1423,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.