diff --git a/lib/src/back_end/solver.dart b/lib/src/back_end/solver.dart index df087f89..323f32c9 100644 --- a/lib/src/back_end/solver.dart +++ b/lib/src/back_end/solver.dart @@ -134,7 +134,7 @@ class Solver { if (debug.traceSolver) { debug.unindent(); debug.log(debug.bold('Solved $root to $best')); - debug.log(solution.code.toDebugString()); + debug.log(best.code.toDebugString()); debug.log(''); } diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index 9aa06aa0..904122b7 100644 --- a/lib/src/front_end/ast_node_visitor.dart +++ b/lib/src/front_end/ast_node_visitor.dart @@ -132,7 +132,7 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override void visitAdjacentStrings(AdjacentStrings node) { - var piece = InfixPiece(const [], node.strings.map(nodePiece).toList(), + var piece = InfixPiece(node.strings.map(nodePiece).toList(), indent: node.indentStrings); // Adjacent strings always split. @@ -366,7 +366,7 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { } } - var piece = InfixPiece(leadingComments, operands); + var piece = InfixPiece(operands); // If conditional expressions are directly nested, force them all to split, // both parents and children. @@ -376,7 +376,7 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { piece.pin(State.split); } - pieces.add(piece); + pieces.add(prependLeadingComments(leadingComments, piece)); } @override @@ -1766,8 +1766,7 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { var patternPiece = nodePiece(member.guardedPattern.pattern); if (member.guardedPattern.whenClause case var whenClause?) { - pieces.add( - InfixPiece(const [], [patternPiece, nodePiece(whenClause)])); + pieces.add(InfixPiece([patternPiece, nodePiece(whenClause)])); } else { pieces.add(patternPiece); } diff --git a/lib/src/front_end/chain_builder.dart b/lib/src/front_end/chain_builder.dart index 13aaac68..dafe1d8d 100644 --- a/lib/src/front_end/chain_builder.dart +++ b/lib/src/front_end/chain_builder.dart @@ -75,9 +75,29 @@ class ChainBuilder { // When [_root] is a cascade, the chain is the series of cascade sections. for (var section in cascade.cascadeSections) { - var piece = _visitor.nodePiece(section); + // 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 callType = switch (section) { + // Force the cascade to split if there are leading comments before + // the cascade section to avoid: + // + // target// comment + // ..method( + // argument, + // ); + _ when leadingComments.isNotEmpty => 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 b2f2cc3e..8ffe303d 100644 --- a/lib/src/front_end/piece_factory.dart +++ b/lib/src/front_end/piece_factory.dart @@ -12,6 +12,7 @@ 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'; @@ -284,6 +285,15 @@ 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, @@ -853,7 +863,7 @@ mixin PieceFactory { switch (combinatorNode) { case HideCombinator(hiddenNames: var names): case ShowCombinator(shownNames: var names): - clauses.add(InfixPiece(const [], [ + clauses.add(InfixPiece([ tokenPiece(combinatorNode.keyword), for (var name in names) tokenPiece(name.token, commaAfter: true), ])); @@ -921,7 +931,8 @@ mixin PieceFactory { pieces.visit(right); }); - pieces.add(InfixPiece(leadingComments, [leftPiece, rightPiece])); + pieces.add(prependLeadingComments( + leadingComments, InfixPiece([leftPiece, rightPiece]))); } /// Writes a chained infix operation: a binary operator expression, or @@ -973,7 +984,8 @@ mixin PieceFactory { traverse(node); })); - pieces.add(InfixPiece(leadingComments, operands, indent: indent)); + pieces.add(prependLeadingComments( + leadingComments, InfixPiece(operands, indent: indent))); } /// Writes a [ListPiece] for the given bracket-delimited set of elements. @@ -1225,7 +1237,7 @@ mixin PieceFactory { var clauses = []; void typeClause(Token keyword, List types) { - clauses.add(InfixPiece(const [], [ + clauses.add(InfixPiece([ tokenPiece(keyword), for (var type in types) nodePiece(type, commaAfter: true), ])); @@ -1321,6 +1333,26 @@ mixin PieceFactory { // element, // ]; canBlockSplitLeft |= switch (leftHandSide) { + // Treat method chains and cascades on the LHS as if they were blocks. + // They don't really fit the "block" term, but it looks much better to + // force a method chain to split on the left than to try to avoid + // splitting it and split at the assignment instead: + // + // // Worse: + // target.method( + // argument, + // ).setter = + // value; + // + // // Better: + // target.method(argument) + // .setter = value; + // + MethodInvocation() => true, + PropertyAccess() => true, + PrefixedIdentifier() => true, + + // Otherwise, it must be an actual block construct. Expression() => leftHandSide.canBlockSplit, DartPattern() => leftHandSide.canBlockSplit, _ => false diff --git a/lib/src/piece/infix.dart b/lib/src/piece/infix.dart index 5e7aacce..989cb1d2 100644 --- a/lib/src/piece/infix.dart +++ b/lib/src/piece/infix.dart @@ -9,27 +9,6 @@ import 'piece.dart'; /// /// a + b + c class InfixPiece extends Piece { - /// Pieces for leading comments that appear before the first operand. - /// - /// We hoist these comments out from the first operand's first token so that - /// a newline in these comments doesn't erroneously force the infix operator - /// to split. For example: - /// - /// value = - /// // comment - /// a + b; - /// - /// Here, the `// comment` will be hoisted out and stored in - /// [_leadingComments] instead of being a leading comment in the [CodePiece] - /// for `a`. If we left the comment in `a`, then the newline after the line - /// comment would force the `+` operator to split yielding: - /// - /// value = - /// // comment - /// a + - /// b; - final List _leadingComments; - /// The series of operands. /// /// Since we don't split on both sides of the operator, the operators will be @@ -41,26 +20,16 @@ class InfixPiece extends Piece { /// Whether operands after the first should be indented if split. final bool _indent; - InfixPiece(this._leadingComments, this._operands, {bool indent = true}) - : _indent = indent; + InfixPiece(this._operands, {bool indent = true}) : _indent = indent; @override List get additionalStates => const [State.split]; @override - bool allowNewlineInChild(State state, Piece child) { - if (state == State.split) return true; - - // Comments before the operands don't force the operator to split. - return _leadingComments.contains(child); - } + bool allowNewlineInChild(State state, Piece child) => state == State.split; @override void format(CodeWriter writer, State state) { - for (var comment in _leadingComments) { - writer.format(comment); - } - if (_indent) writer.pushIndent(Indent.expression); for (var i = 0; i < _operands.length; i++) { @@ -78,7 +47,6 @@ class InfixPiece extends Piece { @override void forEachChild(void Function(Piece piece) callback) { - _leadingComments.forEach(callback); _operands.forEach(callback); } diff --git a/lib/src/piece/leading_comment.dart b/lib/src/piece/leading_comment.dart new file mode 100644 index 00000000..2025c4f5 --- /dev/null +++ b/lib/src/piece/leading_comment.dart @@ -0,0 +1,46 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. +import '../back_end/code_writer.dart'; +import 'piece.dart'; + +/// A piece for a series of leading comments preceding some other piece. +/// +/// We use this and hoist comments out from the inner piece so that a newline +/// in the comments doesn't erroneously force the inner piece to split. For +/// example, if comments preceding an infix operator's left operand: +/// +/// value = +/// // comment +/// a + b; +/// +/// Here, the `// comment` will be hoisted out and stored in a +/// [LeadingCommentPiece] instead of being a leading comment in the [CodePiece] +/// for `a`. If we left the comment in `a`, then the newline after the line +/// comment would force the `+` operator to split yielding: +/// +/// value = +/// // comment +/// a + +/// b; +class LeadingCommentPiece extends Piece { + final List _comments; + final Piece _piece; + + LeadingCommentPiece(this._comments, this._piece); + + @override + void format(CodeWriter writer, State state) { + for (var comment in _comments) { + writer.format(comment); + } + + writer.format(_piece); + } + + @override + void forEachChild(void Function(Piece piece) callback) { + _comments.forEach(callback); + callback(_piece); + } +} diff --git a/test/tall/invocation/cascade_comment.stmt b/test/tall/invocation/cascade_comment.stmt index 05d9a354..b9f87d3e 100644 --- a/test/tall/invocation/cascade_comment.stmt +++ b/test/tall/invocation/cascade_comment.stmt @@ -47,4 +47,20 @@ receiver // comment 2 ..cascade2() // comment 3 - ..cascade3(); \ No newline at end of file + ..cascade3(); +>>> Comment before setter. +target + // comment + ..setter = value; +<<< +target + // comment + ..setter = value; +>>> Comment before single method cascade. +target +// comment + ..method(argument); +<<< +target + // comment + ..method(argument); \ No newline at end of file diff --git a/test/tall/invocation/chain_comment.stmt b/test/tall/invocation/chain_comment.stmt index 5213bc0b..ebd65081 100644 --- a/test/tall/invocation/chain_comment.stmt +++ b/test/tall/invocation/chain_comment.stmt @@ -90,4 +90,10 @@ target ) .third( // c3 - ); \ No newline at end of file + ); +>>> Line comment before setter. +target // c +.prop = value; +<<< +target // c + .prop = value; \ No newline at end of file diff --git a/test/tall/regression/1400/1429.stmt b/test/tall/regression/1400/1429.stmt new file mode 100644 index 00000000..820d85bf --- /dev/null +++ b/test/tall/regression/1400/1429.stmt @@ -0,0 +1,110 @@ +>>> +main() { + target + ..first() + // comment + ..setter = + value + ..another(); +} +<<< +main() { + target + ..first() + // comment + ..setter = value + ..another(); +} +>>> (indent 2) + model + ..setOnUpdate(onUpdate: updateCallback, updateOnChange: updateOnChange) + // Models for components within a page will be disposed by the page's model, + // so we don't want the component widget to dispose them until the page is + // itself disposed. + ..disposeOnWidgetDisposal = + false; +<<< + model + ..setOnUpdate(onUpdate: updateCallback, updateOnChange: updateOnChange) + // Models for components within a page will be disposed by the page's model, + // so we don't want the component widget to dispose them until the page is + // itself disposed. + ..disposeOnWidgetDisposal = false; +>>> (indent 4) + final reactComponentClass = + createReactDartComponentClass( + dartInteropStatics, + componentStatics, + jsConfig, + ) + // ignore: invalid_use_of_protected_member + ..dartComponentVersion = + ReactDartComponentVersion.component + // This is redundant since we also set `name` below, but some code may depend on reading displayName + // so we'll leave this in place for now. + ..displayName = + displayName; +<<< + final reactComponentClass = + createReactDartComponentClass( + dartInteropStatics, + componentStatics, + jsConfig, + ) + // ignore: invalid_use_of_protected_member + ..dartComponentVersion = ReactDartComponentVersion.component + // This is redundant since we also set `name` below, but some code may depend on reading displayName + // so we'll leave this in place for now. + ..displayName = displayName; +>>> (indent 4) + PdfDocumentHelper.getHelper( + _helper.crossTable!.document!, + ).catalog.beginSaveList ??= + []; +<<< + PdfDocumentHelper.getHelper(_helper.crossTable!.document!) + .catalog + .beginSaveList ??= []; +>>> (indent 12) + PdfSecurityHelper.getHelper( + _helper._security!, + ).encryptor.encryptOnlyAttachment = + false; +<<< + PdfSecurityHelper.getHelper(_helper._security!) + .encryptor + .encryptOnlyAttachment = false; +>>> (indent 6) + PdfGridHelper.getHelper( + PdfGridRowHelper.getHelper(_helper.row!).grid, + ).hasColumnSpan = + true; +<<< + PdfGridHelper.getHelper(PdfGridRowHelper.getHelper(_helper.row!).grid) + .hasColumnSpan = true; +>>> (indent 2) + Widget build(BuildContext context) => CompositionRoot( + configureOverrides: configureOverrides, + compose: + (builder) => + builder + //Adds a singleton CounterController to the container + ..addSingleton((container) => CounterController()), + child: + //We need the BuildContext from the Builder here so the children + //can access the container in the CompositionRoot + Builder(elided), + ); +<<< + Widget build(BuildContext context) => CompositionRoot( + configureOverrides: configureOverrides, + compose: + (builder) => + builder + //Adds a singleton CounterController to the container + ..addSingleton((container) => CounterController()), + child: + //We need the BuildContext from the Builder here so the children + //can access the container in the CompositionRoot + Builder(elided), + ); \ No newline at end of file