From 8cf67ef1c5fa372c2f9c8f95a098247cd419da66 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Wed, 6 Dec 2023 16:32:48 -0800 Subject: [PATCH] Format constructor declarations. Includes everything that can go in a constructor: - Factory and redirecting constructors. - Initializing formals ("this." parameters). - Super parameters. - Constructor initializers. - Assert initializers. Constructors are tricky because there are some constraints between how the parameter list and initializer list are allowed to split. Up until now, aside from setAllowNewlines(), there aren't really any ways that pieces interact. To support this, I added a pretty simple API where when a piece calls CodeWriter.format() on a child piece, it can pass it what state the piece is required to be in. If the solution doesn't have the child in that state, it invalidates the solution. It seems to work pretty well. There is probably a more optimal way to implement that in the solver so that we test these constraints even before formatting and discard the solution more eagerly. I left a TODO comment about that and we can look into it later when we're benchmarking. --- lib/src/back_end/code_writer.dart | 44 +++- lib/src/front_end/ast_node_visitor.dart | 179 ++++++++++--- lib/src/front_end/delimited_list_builder.dart | 7 +- lib/src/front_end/piece_factory.dart | 65 ++++- lib/src/piece/assign.dart | 2 +- lib/src/piece/constructor.dart | 176 +++++++++++++ lib/src/piece/function.dart | 23 +- lib/src/piece/list.dart | 31 +-- lib/src/piece/piece.dart | 8 + test/declaration/constructor.unit | 129 +++++++++ test/declaration/constructor_assert.unit | 82 ++++++ test/declaration/constructor_comment.unit | 184 +++++++++++++ test/declaration/constructor_initializer.unit | 244 ++++++++++++++++++ test/declaration/constructor_parameter.unit | 83 ++++++ test/declaration/external.unit | 14 + 15 files changed, 1176 insertions(+), 95 deletions(-) create mode 100644 lib/src/piece/constructor.dart create mode 100644 test/declaration/constructor.unit create mode 100644 test/declaration/constructor_assert.unit create mode 100644 test/declaration/constructor_comment.unit create mode 100644 test/declaration/constructor_initializer.unit create mode 100644 test/declaration/constructor_parameter.unit diff --git a/lib/src/back_end/code_writer.dart b/lib/src/back_end/code_writer.dart index 96367229..0d5c06a7 100644 --- a/lib/src/back_end/code_writer.dart +++ b/lib/src/back_end/code_writer.dart @@ -53,10 +53,16 @@ class CodeWriter { /// The number of characters in the line currently being written. int _column = 0; - /// Whether this solution has encountered a newline where none is allowed. + /// Whether this solution has encountered a conflict that makes it not a valid + /// solution. + /// + /// This can occur if: /// - /// If true, it means the solution is invalid. - bool _containsInvalidNewline = false; + /// - A mandatory newline occurs like from a line comment or statement where + /// none is permitted. + /// - An outer piece requires a child piece to have a certain state and it + /// doesn't. + bool _isInvalid = false; /// The stack of state for each [Piece] being formatted. /// @@ -126,7 +132,7 @@ class CodeWriter { return Solution(_pieceStates, _buffer.toString(), _selectionStart, _selectionEnd, _nextPieceToExpand, - isValid: !_containsInvalidNewline, overflow: _overflow, cost: _cost); + isValid: !_isInvalid, overflow: _overflow, cost: _cost); } /// Notes that a newline has been written. @@ -138,7 +144,7 @@ class CodeWriter { /// the raw text contains a newline, which can happen in multi-line block /// comments and multi-line string literals. void handleNewline() { - if (!_options.allowNewlines) _containsInvalidNewline = true; + if (!_options.allowNewlines) _isInvalid = true; // Note that this piece contains a newline so that we can propagate that // up to containing pieces too. @@ -228,11 +234,12 @@ class CodeWriter { /// Format [piece] and insert the result into the code being written and /// returned by [finish()]. - void format(Piece piece) { - // Don't bother recursing into the piece tree if we know the solution will - // be discarded. - if (_containsInvalidNewline) return; - + /// + /// If [requireState] is given, then [piece] must be in that state or the + /// solution is considered invalid. This is used to create constraints + /// between pieces. For example, a constructor piece forces the initializer + /// list child piece to split if the parameter list splits. + void format(Piece piece, {State? requireState}) { _pieceOptions.add(_PieceOptions(_options.indent, _options.allowNewlines)); var isUnsolved = !_pieceStates.isBound(piece) && piece.states.length > 1; @@ -240,7 +247,20 @@ class CodeWriter { var state = _pieceStates.pieceState(piece); - _cost += state.cost; + // If the parent piece constrains this child to a certain state, then + // invalidate the solution if it doesn't match. + if (requireState != null && state != requireState) { + _isInvalid = true; + // TODO(perf): The solver doesn't discard invalid states that are caused + // by newlines because sometimes an invalid state like that can lead to + // a valid state by selecting values for other pieces. + // + // But in this case, once a partial solution is invalidated by one piece's + // state conflicting with another's, any solution derived from that is + // also going to be invalid and we can prune that entire solution tree. + } + + _cost += piece.stateCost(state); // TODO(perf): Memoize this. Might want to create a nested PieceWriter // instead of passing in `this` so we can better control what state needs @@ -316,7 +336,7 @@ class CodeWriter { // expand it next. if (!_foundExpandLine && _nextPieceToExpand != null && - (_column > _pageWidth || _containsInvalidNewline)) { + (_column > _pageWidth || _isInvalid)) { // We found a problematic line, so remember it and the piece on it. _foundExpandLine = true; } else if (!_foundExpandLine) { diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index b39171c4..1b548a12 100644 --- a/lib/src/front_end/ast_node_visitor.dart +++ b/lib/src/front_end/ast_node_visitor.dart @@ -13,7 +13,9 @@ import '../piece/adjacent.dart'; import '../piece/assign.dart'; import '../piece/block.dart'; import '../piece/chain.dart'; +import '../piece/constructor.dart'; import '../piece/for.dart'; +import '../piece/function.dart'; import '../piece/if.dart'; import '../piece/infix.dart'; import '../piece/list.dart'; @@ -130,7 +132,17 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override Piece visitAssertInitializer(AssertInitializer node) { - throw UnimplementedError(); + return buildPiece((b) { + b.token(node.assertKeyword); + b.add(createList( + leftBracket: node.leftParenthesis, + [ + node.condition, + if (node.message case var message?) message, + ], + rightBracket: node.rightParenthesis, + )); + }); } @override @@ -343,18 +355,85 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override Piece visitConstructorDeclaration(ConstructorDeclaration node) { - throw UnimplementedError(); + var header = buildPiece((b) { + b.modifier(node.externalKeyword); + b.modifier(node.constKeyword); + b.modifier(node.factoryKeyword); + b.visit(node.returnType); + b.token(node.period); + b.token(node.name); + }); + + var parameters = nodePiece(node.parameters); + + Piece? redirect; + Piece? initializerSeparator; + Piece? initializers; + if (node.redirectedConstructor case var constructor?) { + redirect = AssignPiece( + tokenPiece(node.separator!), nodePiece(constructor), + isValueDelimited: false); + } else if (node.initializers.isNotEmpty) { + initializerSeparator = tokenPiece(node.separator!); + initializers = createList(node.initializers, + style: const ListStyle(commas: Commas.nonTrailing)); + } + + var body = createFunctionBody(node.body); + + return ConstructorPiece(header, parameters, body, + canSplitParameters: node.parameters.parameters + .canSplit(node.parameters.rightParenthesis), + hasOptionalParameter: node.parameters.rightDelimiter != null, + redirect: redirect, + initializerSeparator: initializerSeparator, + initializers: initializers); } @override Piece visitConstructorFieldInitializer(ConstructorFieldInitializer node) { - throw UnimplementedError(); + return buildPiece((b) { + b.token(node.thisKeyword); + b.token(node.period); + b.add(createAssignment(node.fieldName, node.equals, node.expression)); + }); } @override Piece visitConstructorName(ConstructorName node) { - throw UnsupportedError( - 'This node is handled by visitInstanceCreationExpression().'); + // If there is an import prefix and/or constructor name, then allow + // splitting before the `.`. This doesn't look good, but is consistent with + // constructor calls that don't have `new` or `const`. We allow splitting + // in the latter because there is no way to distinguish syntactically + // between a named constructor call and any other kind of method call or + // property access. + var operations = []; + + var builder = AdjacentBuilder(this); + if (node.type.importPrefix case var importPrefix?) { + builder.token(importPrefix.name); + operations.add(builder.build()); + builder.token(importPrefix.period); + } + + // The name of the type being constructed. + var type = node.type; + builder.token(type.name2); + builder.visit(type.typeArguments); + builder.token(type.question); + + // If this is a named constructor, the name. + if (node.name != null) { + operations.add(builder.build()); + builder.token(node.period); + builder.visit(node.name); + } + + // If there was a prefix or constructor name, then make a splittable piece. + // Otherwise, the current piece is a simple identifier for the name. + operations.add(builder.build()); + if (operations.length == 1) return operations.first; + return ChainPiece(operations); } @override @@ -550,7 +629,30 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override Piece visitFieldFormalParameter(FieldFormalParameter node) { - throw UnimplementedError(); + if (node.parameters case var parameters?) { + // A function-typed field formal like: + // + // ``` + // C(this.fn(parameter)); + // ``` + return createFunctionType( + node.type, + fieldKeyword: node.thisKeyword, + period: node.period, + node.name, + node.typeParameters, + parameters, + node.question, + parameter: node); + } else { + return createFormalParameter( + node, + mutableKeyword: node.keyword, + fieldKeyword: node.thisKeyword, + period: node.period, + node.type, + node.name); + } } @override @@ -736,7 +838,7 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override Piece visitFunctionDeclaration(FunctionDeclaration node) { return createFunction( - externalKeyword: node.externalKeyword, + modifiers: [node.externalKeyword], returnType: node.returnType, propertyKeyword: node.propertyKeyword, name: node.name, @@ -974,11 +1076,9 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override Piece visitMethodDeclaration(MethodDeclaration node) { return createFunction( - externalKeyword: node.externalKeyword, - modifierKeyword: node.modifierKeyword, + modifiers: [node.externalKeyword, node.modifierKeyword], returnType: node.returnType, - operatorKeyword: node.operatorKeyword, - propertyKeyword: node.propertyKeyword, + propertyKeyword: node.operatorKeyword ?? node.propertyKeyword, name: node.name, typeParameters: node.typeParameters, parameters: node.parameters, @@ -1166,7 +1266,12 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override Piece visitRedirectingConstructorInvocation( RedirectingConstructorInvocation node) { - throw UnimplementedError(); + return buildPiece((b) { + b.token(node.thisKeyword); + b.token(node.period); + b.visit(node.constructorName); + b.visit(node.argumentList); + }); } @override @@ -1260,22 +1365,8 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override Piece visitSimpleFormalParameter(SimpleFormalParameter node) { - var builder = AdjacentBuilder(this); - startFormalParameter(node, builder); - builder.modifier(node.keyword); - - if ((node.type, node.name) case (var _?, var name?)) { - // Have both a type and name, so allow splitting after the type. - builder.visit(node.type); - var typePiece = builder.build(); - var namePiece = tokenPiece(name); - return VariablePiece(typePiece, [namePiece], hasType: true); - } else { - // Don't have both a type and name, so just write whichever one we have. - builder.visit(node.type); - builder.token(node.name); - return builder.build(); - } + return createFormalParameter(node, node.type, node.name, + mutableKeyword: node.keyword); } @override @@ -1300,7 +1391,12 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override Piece visitSuperConstructorInvocation(SuperConstructorInvocation node) { - throw UnimplementedError(); + return buildPiece((b) { + b.token(node.superKeyword); + b.token(node.period); + b.visit(node.constructorName); + b.visit(node.argumentList); + }); } @override @@ -1310,7 +1406,30 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override Piece visitSuperFormalParameter(SuperFormalParameter node) { - throw UnimplementedError(); + if (node.parameters case var parameters?) { + // A function-typed super parameter like: + // + // ``` + // C(super.fn(parameter)); + // ``` + return createFunctionType( + node.type, + fieldKeyword: node.superKeyword, + period: node.period, + node.name, + node.typeParameters, + parameters, + node.question, + parameter: node); + } else { + return createFormalParameter( + node, + mutableKeyword: node.keyword, + fieldKeyword: node.superKeyword, + period: node.period, + node.type, + node.name); + } } @override diff --git a/lib/src/front_end/delimited_list_builder.dart b/lib/src/front_end/delimited_list_builder.dart index 68f917d9..e8b1ce6b 100644 --- a/lib/src/front_end/delimited_list_builder.dart +++ b/lib/src/front_end/delimited_list_builder.dart @@ -55,9 +55,10 @@ class DelimitedListBuilder { var blockElement = -1; if (_style.allowBlockElement) blockElement = _findBlockElement(); - return ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket, - _style, blockElement, - mustSplit: _mustSplit); + var piece = ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket, + _style, blockElement); + if (_mustSplit) piece.pin(State.split); + return piece; } /// Adds the opening [bracket] to the built list. diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart index ed1b02a5..89d0775d 100644 --- a/lib/src/front_end/piece_factory.dart +++ b/lib/src/front_end/piece_factory.dart @@ -16,6 +16,7 @@ import '../piece/piece.dart'; import '../piece/postfix.dart'; import '../piece/try.dart'; import '../piece/type.dart'; +import '../piece/variable.dart'; import 'adjacent_builder.dart'; import 'ast_node_visitor.dart'; import 'comment_writer.dart'; @@ -219,6 +220,38 @@ mixin PieceFactory { }); } + /// Creates a normal (not function-typed) formal parameter with a name and/or + /// type annotation. + /// + /// If [mutableKeyword] is given, it should be the `var` or `final` keyword. + /// If [fieldKeyword] and [period] are given, the former should be the `this` + /// or `super` keyword for an initializing formal or super parameter. + Piece createFormalParameter( + NormalFormalParameter node, TypeAnnotation? type, Token? name, + {Token? mutableKeyword, Token? fieldKeyword, Token? period}) { + var builder = AdjacentBuilder(this); + startFormalParameter(node, builder); + builder.modifier(mutableKeyword); + builder.visit(type); + + Piece? typePiece; + if (type != null && name != null) { + typePiece = builder.build(); + } + + builder.token(fieldKeyword); + builder.token(period); + builder.token(name); + + // If we have both a type and name, allow splitting between them. + if (typePiece != null) { + var namePiece = builder.build(); + return VariablePiece(typePiece, [namePiece], hasType: true); + } + + return builder.build(); + } + /// Creates a function, method, getter, or setter declaration. /// /// If [modifierKeyword] is given, it should be the `static` or `abstract` @@ -227,8 +260,7 @@ mixin PieceFactory { /// [propertyKeyword] is given, it should be the `get` or `set` keyword on a /// getter or setter declaration. Piece createFunction( - {Token? externalKeyword, - Token? modifierKeyword, + {List modifiers = const [], AstNode? returnType, Token? operatorKeyword, Token? propertyKeyword, @@ -237,8 +269,9 @@ mixin PieceFactory { FormalParameterList? parameters, required FunctionBody body}) { var builder = AdjacentBuilder(this); - builder.modifier(externalKeyword); - builder.modifier(modifierKeyword); + for (var keyword in modifiers) { + builder.modifier(keyword); + } Piece? returnTypePiece; if (returnType != null) { @@ -253,23 +286,37 @@ mixin PieceFactory { builder.visit(parameters); var signature = builder.build(); - var bodyPiece = nodePiece(body); + var bodyPiece = createFunctionBody(body); - return FunctionPiece(returnTypePiece, signature, - body: bodyPiece, spaceBeforeBody: body is! EmptyFunctionBody); + return FunctionPiece(returnTypePiece, signature, bodyPiece); + } + + /// Creates a piece for a function, method, or constructor body. + Piece createFunctionBody(FunctionBody body) { + return buildPiece((b) { + // Don't put a space before `;` bodies. + if (body is! EmptyFunctionBody) b.space(); + b.visit(body); + }); } /// Creates a function type or function-typed formal. /// /// If creating a piece for a function-typed formal, then [parameter] is the /// formal parameter. + /// + /// If this is a function-typed initializing formal (`this.foo()`, then + /// [fieldKeyword] is `this` and [period] is the `.`. Likewise, for a + /// function-typed super parameter, [fieldKeyword] is `super`. Piece createFunctionType( TypeAnnotation? returnType, Token functionKeywordOrName, TypeParameterList? typeParameters, FormalParameterList parameters, Token? question, - {FunctionTypedFormalParameter? parameter}) { + {FormalParameter? parameter, + Token? fieldKeyword, + Token? period}) { var builder = AdjacentBuilder(this); if (parameter != null) startFormalParameter(parameter, builder); @@ -280,6 +327,8 @@ mixin PieceFactory { returnTypePiece = builder.build(); } + builder.token(fieldKeyword); + builder.token(period); builder.token(functionKeywordOrName); builder.visit(typeParameters); builder.visit(parameters); diff --git a/lib/src/piece/assign.dart b/lib/src/piece/assign.dart index c21e5ec7..66da394d 100644 --- a/lib/src/piece/assign.dart +++ b/lib/src/piece/assign.dart @@ -54,7 +54,7 @@ class AssignPiece extends Piece { /// block-like formatting. final bool _isValueDelimited; - AssignPiece(this.target, this.value, {required bool isValueDelimited}) + AssignPiece(this.target, this.value, {bool isValueDelimited = false}) : _isValueDelimited = isValueDelimited; // TODO(tall): The old formatter allows the first operand of a split diff --git a/lib/src/piece/constructor.dart b/lib/src/piece/constructor.dart new file mode 100644 index 00000000..3303b7a4 --- /dev/null +++ b/lib/src/piece/constructor.dart @@ -0,0 +1,176 @@ +// Copyright (c) 2023, 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 constructor declaration. +/// +/// This is somewhat similar to [FunctionPiece], but constructor initializers +/// add a lot of complexity. In particular, there are constraints between how +/// the parameter list is allowed to split and how the initializer list is +/// allowed to split. Only a few combinations of splits are allowed: +/// +/// [State.unsplit] No splits at all, in the parameters or initializers. +/// +/// ``` +/// SomeClass(param) : a = 1, b = 2; +/// ``` +/// +/// [_splitBeforeInitializers] Split before the `:` and between the +/// initializers but not in the parameters. +/// +/// ``` +/// SomeClass(param) +/// : a = 1, +/// b = 2; +/// ``` +/// +/// [_splitBetweenInitializers] Split between the initializers but not before +/// the `:`. This state should only be chosen when the parameters split. If +/// there are no parameters, this state is excluded. +/// +/// ``` +/// SomeClass( +/// param +/// ) : a = 1, +/// b = 2; +/// ``` +/// +/// In addition, this piece deals with indenting initializers appropriately +/// based on whether the parameter list has a `]` or `}` before the `)`. If +/// there are optional parameters, then initializers after the first are +/// indented one space more to line up with the first initializer: +/// +/// ``` +/// SomeClass( +/// mandatory, +/// ) : firstInitializer = 1, +/// second = 2; +/// // ^ Four spaces of indentation. +/// +/// SomeClass([ +/// optional, +/// ]) : firstInitializer = 1, +/// second = 2; +/// // ^ Five spaces of indentation. +/// ``` +class ConstructorPiece extends Piece { + static const _splitBeforeInitializers = State(1, cost: 1); + + static const _splitBetweenInitializers = State(2, cost: 2); + + /// True if there are parameters or comments inside the parameter list. + /// + /// If so, then we allow splitting the parameter list while leaving the `:` + /// on the same line as the `)`. + final bool _canSplitParameters; + + /// Whether the parameter list contains a `]` or `}` closing delimiter before + /// the `)`. + final bool _hasOptionalParameter; + + /// The leading keywords, class name, and constructor name. + final Piece _header; + + /// The constructor parameter list. + final Piece _parameters; + + /// If this is a redirecting constructor, the redirection clause. + final Piece? _redirect; + + /// If there are initializers, the `:` before them. + final Piece? _initializerSeparator; + + /// The constructor initializers, if there are any. + final Piece? _initializers; + + /// The constructor body. + final Piece _body; + + ConstructorPiece(this._header, this._parameters, this._body, + {required bool canSplitParameters, + required bool hasOptionalParameter, + Piece? redirect, + Piece? initializerSeparator, + Piece? initializers}) + : _canSplitParameters = canSplitParameters, + _hasOptionalParameter = hasOptionalParameter, + _redirect = redirect, + _initializerSeparator = initializerSeparator, + _initializers = initializers; + + @override + List get additionalStates => [ + if (_initializers != null) _splitBeforeInitializers, + if (_canSplitParameters && _initializers != null) + _splitBetweenInitializers + ]; + + @override + void format(CodeWriter writer, State state) { + // There are constraints between how the parameters may split and now the + // initializers may split. + var (parameterState, initializerState) = switch (state) { + // If there are no initializers, the parameters can do whatever. + State.unsplit when _initializers == null => (null, null), + // All parameters and initializers on one line. + State.unsplit => (State.unsplit, State.unsplit), + // If the `:` splits, then the parameters can't. + _splitBeforeInitializers => (State.unsplit, State.split), + // The `) :` on its own line. + _splitBetweenInitializers => (State.split, State.split), + _ => throw ArgumentError(), + }; + + // If there's a newline in the header or parameters (like a line comment + // after the `)`), then don't allow the initializers to remain unsplit. + if (_initializers != null && state == State.unsplit) { + writer.setAllowNewlines(false); + } + + writer.format(_header); + writer.format(_parameters, requireState: parameterState); + + if (_redirect case var redirect?) { + writer.space(); + writer.format(redirect); + } + + if (_initializers case var initializers?) { + writer.setAllowNewlines(state != State.unsplit); + writer.splitIf(state == _splitBeforeInitializers, indent: 2); + + writer.format(_initializerSeparator!); + writer.space(); + + // Indent subsequent initializers past the `:`. + if (_hasOptionalParameter && state == _splitBetweenInitializers) { + // If the parameter list ends in `]) : init...` then we need to indent + // +5 to line up subsequent initializers. + writer.setIndent(5); + } else { + writer.setIndent(4); + } + + writer.format(initializers, requireState: initializerState); + } + + writer.setIndent(0); + writer.setAllowNewlines(true); + writer.format(_body); + } + + @override + void forEachChild(void Function(Piece piece) callback) { + callback(_header); + callback(_parameters); + if (_redirect case var redirect?) callback(redirect); + if (_initializerSeparator case var separator?) callback(separator); + if (_initializers case var initializers?) callback(initializers); + callback(_body); + } + + @override + String get debugName => 'Ctor'; +} diff --git a/lib/src/piece/function.dart b/lib/src/piece/function.dart index 88aae1b1..1193c893 100644 --- a/lib/src/piece/function.dart +++ b/lib/src/piece/function.dart @@ -21,22 +21,7 @@ class FunctionPiece extends Piece { /// If this is a function declaration with a (non-empty `;`) body, the body. final Piece? _body; - /// Whether we should write a space between the function signature and body. - /// - /// This is `true` for most bodies except for empty function bodies, like: - /// - /// ``` - /// class C { - /// C(); - /// // ^ No space before `;`. - /// } - /// ``` - final bool _spaceBeforeBody; - - FunctionPiece(this._returnType, this._signature, - {Piece? body, bool spaceBeforeBody = false}) - : _body = body, - _spaceBeforeBody = spaceBeforeBody; + FunctionPiece(this._returnType, this._signature, [this._body]); @override List get additionalStates => @@ -57,10 +42,8 @@ class FunctionPiece extends Piece { } writer.format(_signature); - if (_body case var body?) { - if (_spaceBeforeBody) writer.space(); - writer.format(body); - } + + if (_body case var body?) writer.format(body); } @override diff --git a/lib/src/piece/list.dart b/lib/src/piece/list.dart index 5c3f3984..7b3ec1a9 100644 --- a/lib/src/piece/list.dart +++ b/lib/src/piece/list.dart @@ -61,32 +61,21 @@ class ListPiece extends Piece { /// The details of how this particular list should be formatted. final ListStyle _style; - /// The state when the list is split. - /// - /// We use this instead of [State.split] because the cost is higher for some - /// kinds of lists. - /// - /// Also, if this list does have a block element, then we also increase the - /// cost of splitting the list itself a little more to prefer the block - /// argument splitting when possible. - // TODO(rnystrom): Having to use a different state for this is a little - // cumbersome. Maybe it would be better to most costs out of [State] and - // instead have the [Solver] ask each [Piece] for the cost of its state. - final State _splitState; - /// If this list has an element that can receive block formatting, this is /// the elements's index. Otherwise `-1`. final int _blockElement; ListPiece(this._before, this._elements, this._blanksAfter, this._after, - this._style, this._blockElement, - {required bool mustSplit}) - : _splitState = State(2, cost: _style.splitCost) { - if (mustSplit) pin(_splitState); - } + this._style, this._blockElement); @override - List get additionalStates => [if (_elements.isNotEmpty) _splitState]; + List get additionalStates => [if (_elements.isNotEmpty) State.split]; + + @override + int stateCost(State state) { + if (state == State.split) return _style.splitCost; + return super.stateCost(state); + } @override void format(CodeWriter writer, State state) { @@ -121,13 +110,13 @@ class ListPiece extends Piece { // Only allow newlines in the block element or in all elements if we're // fully split. - writer.setAllowNewlines(i == _blockElement || state == _splitState); + writer.setAllowNewlines(i == _blockElement || state == State.split); var element = _elements[i]; element.format(writer, appendComma: appendComma, // Only allow newlines in comments if we're fully split. - allowNewlinesInComments: state == _splitState); + allowNewlinesInComments: state == State.split); // Write a space or newline between elements. if (!isLast) { diff --git a/lib/src/piece/piece.dart b/lib/src/piece/piece.dart index 116763a3..371bf5e8 100644 --- a/lib/src/piece/piece.dart +++ b/lib/src/piece/piece.dart @@ -55,6 +55,14 @@ abstract class Piece { /// Invokes [callback] on each piece contained in this piece. void forEachChild(void Function(Piece piece) callback); + /// The cost that this piece should apply to the solution when in [state]. + /// + /// This is usually just the state's cost, but some pieces may want to tweak + /// the cost in certain circumstances. + // TODO(tall): Given that we have this API now, consider whether it makes + // sense to remove the cost field from State entirely. + int stateCost(State state) => state.cost; + /// Forces this piece to always use [state]. void pin(State state) { _pinnedState = state; diff --git a/test/declaration/constructor.unit b/test/declaration/constructor.unit new file mode 100644 index 00000000..4e2cde27 --- /dev/null +++ b/test/declaration/constructor.unit @@ -0,0 +1,129 @@ +40 columns | +>>> Semicolon body. +class A { + A(); +} +<<< +class A { + A(); +} +>>> Empty body. +class A { + A() {} +} +<<< +class A { + A() {} +} +>>> Non-empty body. +class A { + A() {body;} +} +<<< +class A { + A() { + body; + } +} +>>> Constant and factory constructors. +class Foo { + const Foo . constant (); + factory Foo . fact () => foo; +} +<<< +class Foo { + const Foo.constant(); + factory Foo.fact() => foo; +} +>>> Named constructor. +class Foo { + Foo . bar () { } + const Foo . baz() ; + factory Foo . bang( int param ) => foo; +} +<<< +class Foo { + Foo.bar() {} + const Foo.baz(); + factory Foo.bang(int param) => foo; +} +>>> Don't split at named constructor. +class SomeLongClassName { + SomeLongClassName.someLongConstructorName(); +} +<<< +class SomeLongClassName { + SomeLongClassName.someLongConstructorName(); +} +>>> Redirecting constructor. +class A { + A ( ) : this . named ( 1 ); + A . named ( int x ) : this ( ); +} +<<< +class A { + A() : this.named(1); + A.named(int x) : this(); +} +>>> Split after `:` in redirecting constructor. +class LongClassName { + LongClassName() : this(argument, another); +} +<<< +class LongClassName { + LongClassName() + : this(argument, another); +} +>>> Split in redirecting constructor argument list. +class LongClassName { + LongClassName() : this(argument, anotherArgument, thirdOne); +} +<<< +class LongClassName { + LongClassName() + : this( + argument, + anotherArgument, + thirdOne, + ); +} +>>> Redirecting factory constructor. +class A { + factory A . nonConst ( ) = B < int >; + const factory A ( int x ) = prefix . B; +} +<<< +class A { + factory A.nonConst() = B; + const factory A(int x) = prefix.B; +} +>>> Split after `=` in redirecting factory constructor. +class VeryLongClassName { + factory VeryLongClassName() = OtherLongClass; +} +<<< +class VeryLongClassName { + factory VeryLongClassName() = + OtherLongClass; +} +>>> Prefer to split at `=` instead of in type arguments. +class LongClass { + factory LongClass() = Other; +} +<<< +class LongClass { + factory LongClass() = + Other; +} +>>> Split in redirecting factory constructor type argument list. +class LongClass { + factory LongClass() = Other; +} +<<< +class LongClass { + factory LongClass() = + Other< + SomeLongType, + AnotherLongType + >; +} \ No newline at end of file diff --git a/test/declaration/constructor_assert.unit b/test/declaration/constructor_assert.unit new file mode 100644 index 00000000..25ea2a67 --- /dev/null +++ b/test/declaration/constructor_assert.unit @@ -0,0 +1,82 @@ +40 columns | +>>> Unsplit assert initializers. +class Foo { + Foo() : assert(1), assert(2); +} +<<< +class Foo { + Foo() : assert(1), assert(2); +} +>>> Split assert initializers. +class Foo { + Foo(parameter, another) : assert(condition, 'some long message'), + assert(cond), + assert(anotherCondition, 'another message'); +} +<<< +class Foo { + Foo(parameter, another) + : assert( + condition, + 'some long message', + ), + assert(cond), + assert( + anotherCondition, + 'another message', + ); +} +>>> Split in assert forces initializers to split. +class Foo { + Foo() : assert( + veryLongConditionExpression); +} +<<< +class Foo { + Foo() + : assert( + veryLongConditionExpression, + ); +} +>>> Align split assert argument lists past the `:`. +class Foo { + Foo(parameter1, parameter2, parameter3) + : assert(condition, 'some long assert message'), + assert(anotherLongCondition, 'a message'); +} +<<< +class Foo { + Foo( + parameter1, + parameter2, + parameter3, + ) : assert( + condition, + 'some long assert message', + ), + assert( + anotherLongCondition, + 'a message', + ); +} +>>> Align split assert argument lists past the `:`. +class Foo { + Foo(parameter1, [parameter2, parameter3]) + : assert(condition, 'some long assert message'), + assert(anotherLongCondition, 'a message'); +} +<<< +class Foo { + Foo( + parameter1, [ + parameter2, + parameter3, + ]) : assert( + condition, + 'some long assert message', + ), + assert( + anotherLongCondition, + 'a message', + ); +} \ No newline at end of file diff --git a/test/declaration/constructor_comment.unit b/test/declaration/constructor_comment.unit new file mode 100644 index 00000000..68cc86ff --- /dev/null +++ b/test/declaration/constructor_comment.unit @@ -0,0 +1,184 @@ +40 columns | +>>> Line comment before redirecting constructor separator. +class A { + A() // comment + : this(); +} +<<< +class A { + A() // comment + : this(); +} +>>> Line comment after redirecting constructor separator. +class A { + A() : // comment + this(); +} +<<< +class A { + A() + : // comment + this(); +} +>>> Line comment before redirecting factory constructor separator. +class A { + factory A() // comment + = B; +} +<<< +class A { + factory A() // comment + = B; +} +>>> Line comment after redirecting factory constructor separator. +class A { + factory A() = // comment + B; +} +<<< +class A { + factory A() = // comment + B; +} +>>> Line comment in parameter list. +class A { + A(param, // comment + param2) : b = 1, c = 2; +} +<<< +class A { + A( + param, // comment + param2, + ) : b = 1, + c = 2; +} +>>> Line comment before initializer separator. +class A { + A(param) // comment + : b = 1, c = 2; +} +<<< +class A { + A(param) // comment + : b = 1, + c = 2; +} +>>> Line comment after initializer separator. +class A { + A(param) :// comment + b = 1, c = 2; +} +<<< +class A { + A(param) + : // comment + b = 1, + c = 2; +} +>>> Line comment with newline after initializer separator. +class A { + A(param) : + // comment + b = 1, c = 2; +} +<<< +class A { + A(param) + : // comment + b = 1, + c = 2; +} +>>> Line comment before initializer comma. +class A { + A(param) : b = 1 // comment + , c = 2; +} +<<< +class A { + A(param) + : b = 1, // comment + c = 2; +} +>>> Line comment after initializer comma. +class A { + A(param) : b = 1, // comment + c = 2; +} +<<< +class A { + A(param) + : b = 1, // comment + c = 2; +} +>>> Line comment between initializer lines. +class A { + A(param) : b = 1, + // comment + c = 2; +} +<<< +class A { + A(param) + : b = 1, + // comment + c = 2; +} +>>> Line comment before semicolon body. +class A { + A(param) : b = 1, c = 2 // comment + ; +} +<<< +### It's weird to force the `=` to split, but a comment here is unusual. +class A { + A(param) + : b = 1, + c = + 2 // comment + ; +} +>>> Line comment after semicolon body. +class A { + A(param) : b = 1, c = 2; // comment +} +<<< +class A { + A(param) : b = 1, c = 2; // comment +} +>>> +class A { + A(param) : b = 1, c = 2; // longer comment +} +<<< +class A { + A(param) + : b = 1, + c = 2; // longer comment +} +>>> Line comment before block body. +class A { + A(param) : b = 1, c = 2 // comment + { body; } +} +<<< +### Weird, but users rarely write this. +class A { + A(param) + : b = 1, + c = + 2 // comment + { + body; + } +} +>>> Line comment after block body. +class A { + A(param) : b = 1, c = 2 { body; } // comment +} +<<< +class A { + A(param) : b = 1, c = 2 { + body; + } // comment +} \ No newline at end of file diff --git a/test/declaration/constructor_initializer.unit b/test/declaration/constructor_initializer.unit new file mode 100644 index 00000000..cdc62499 --- /dev/null +++ b/test/declaration/constructor_initializer.unit @@ -0,0 +1,244 @@ +40 columns | +>>> Unnamed super constructor. +class Foo { + Foo(a, b) : super ( a , other : b ) ; +} +<<< +class Foo { + Foo(a, b) : super(a, other: b); +} +>>> Named super constructor. +class Foo { + Foo(a, b) : super . name ( a , other : b ) ; +} +<<< +class Foo { + Foo(a, b) : super.name(a, other: b); +} +>>> Split a single initializer. +class MyClass { + MyClass(first, second) : super(first, second); +} +<<< +class MyClass { + MyClass(first, second) + : super(first, second); +} +>>> No parameters and unsplit field initializers. +class X { + var x, y; + X() : x = 1, y = 2; +} +<<< +class X { + var x, y; + X() : x = 1, y = 2; +} +>>> No parameters and split field initializers. +class MyClass { + MyClass() : first = "some value", second = "another", + third = "last"; +} +<<< +class MyClass { + MyClass() + : first = "some value", + second = "another", + third = "last"; +} +>>> Unsplit parameters and initializers. +class C { + C(a, b) : super(a), _b = b, _c = 1; + + C.b(a, b) : super(a), _b = b, _c = 1 { body; } +} +<<< +class C { + C(a, b) : super(a), _b = b, _c = 1; + + C.b(a, b) : super(a), _b = b, _c = 1 { + body; + } +} +>>> If parameters split, initializers do too even if they would fit unsplit. +class C { + C(String first, String second, String third) : super(a), _b = b, _c = 1; +} +<<< +class C { + C( + String first, + String second, + String third, + ) : super(a), + _b = b, + _c = 1; +} +>>> +class C { + C(String first, String second, String third) : super(a), _b = b, _c = 1 + { body; } +} +<<< +class C { + C( + String first, + String second, + String third, + ) : super(a), + _b = b, + _c = 1 { + body; + } +} +>>> +class Foo { + Foo(int longArg1, int longArg2, int longArg3) : this._(longArg1); +} +<<< +class Foo { + Foo( + int longArg1, + int longArg2, + int longArg3, + ) : this._(longArg1); +} +>>> Split at `:` and not in parameter list. +class Foo { + Foo(int a, int b): super(aLongIdentifier); +} +<<< +class Foo { + Foo(int a, int b) + : super(aLongIdentifier); +} +>>> Split each initializer but not parameters. +class Foo { + Foo(int a, int b) : super(a), this.b = longExpression; +} +<<< +class Foo { + Foo(int a, int b) + : super(a), + this.b = longExpression; +} +>>> Align initializers when split parameter list has no optional section. +class Foo { + Foo(parameter1, parameter2, parameter3) : initializer1 = 1, initializer2 = 2; +} +<<< +class Foo { + Foo( + parameter1, + parameter2, + parameter3, + ) : initializer1 = 1, + initializer2 = 2; +} +>>> Align initializers when split parameter list has optional section. +class Foo { + Foo(parameter1, [parameter2, parameter3]) : initializer1 = 1, initializer2 = 2; +} +<<< +class Foo { + Foo( + parameter1, [ + parameter2, + parameter3, + ]) : initializer1 = 1, + initializer2 = 2; +} +>>> Align initializers when split parameter list has named section. +class Foo { + Foo(parameter1, {parameter2, parameter3}) : initializer1 = 1, initializer2 = 2; +} +<<< +class Foo { + Foo( + parameter1, { + parameter2, + parameter3, + }) : initializer1 = 1, + initializer2 = 2; +} +>>> Align initializers when unsplit parameter list has no optional section. +class Foo { + Foo(parameter1) : initializer1 = 1, initializer2 = 2; +} +<<< +class Foo { + Foo(parameter1) + : initializer1 = 1, + initializer2 = 2; +} +>>> Align initializers when unsplit parameter list has optional section. +class Foo { + Foo([parameter1]) : initializer1 = 1, initializer2 = 2; +} +<<< +class Foo { + Foo([parameter1]) + : initializer1 = 1, + initializer2 = 2; +} +>>> Align initializers when unsplit parameter list has named section. +class Foo { + Foo({parameter1}) : initializer1 = 1, initializer2 = 2; +} +<<< +class Foo { + Foo({parameter1}) + : initializer1 = 1, + initializer2 = 2; +} +>>> Wrap split initializers past the `:`. +class Foo { + Foo(parameter) + : initializer = function(argument, argument), + initializer2 = function(argument, argument); +} +<<< +class Foo { + Foo(parameter) + : initializer = function( + argument, + argument, + ), + initializer2 = function( + argument, + argument, + ); +} +>>> Wrap split initializers with split optional parameter list past the `:`. +class Foo { + Foo(parameter1, [parameter2, parameter3]) + : initializer = function(argument, argument), + initializer2 = function(argument, argument); +} +<<< +class Foo { + Foo( + parameter1, [ + parameter2, + parameter3, + ]) : initializer = function( + argument, + argument, + ), + initializer2 = function( + argument, + argument, + ); +} +>>> Allow block formatting the initialized value. +class Foo { + Foo() : initializer =function(argument, arg); +} +<<< +class Foo { + Foo() + : initializer = function( + argument, + arg, + ); +} \ No newline at end of file diff --git a/test/declaration/constructor_parameter.unit b/test/declaration/constructor_parameter.unit new file mode 100644 index 00000000..63f9e030 --- /dev/null +++ b/test/declaration/constructor_parameter.unit @@ -0,0 +1,83 @@ +40 columns | +>>> Initializing formal. +class Foo { + Foo(this . a, int this . b, final this . c); + Foo.optional([ this . a, int this . b = 123 ]); + Foo.named({ required this . a, int this . b = 123 }); +} +<<< +class Foo { + Foo(this.a, int this.b, final this.c); + Foo.optional([ + this.a, + int this.b = 123, + ]); + Foo.named({ + required this.a, + int this.b = 123, + }); +} +>>> Function-typed initializing formal. +class Foo { + Foo.function(this . a ( int x ), this . b ( ) ? ); + Foo.optional([ int this . c() , this . d ( ) = x ]); + Foo.named({ required int this . e() , this . f ( ) = x }); +} +<<< +class Foo { + Foo.function( + this.a(int x), + this.b()?, + ); + Foo.optional([ + int this.c(), + this.d() = x, + ]); + Foo.named({ + required int this.e(), + this.f() = x, + }); +} +>>> Super parameter. +class Foo { + Foo(super . a, int super . b, final super . c); + Foo.optional([ super . a, int super . b = 123 ]); + Foo.named({ required super . a, int super . b = 123 }); +} +<<< +class Foo { + Foo( + super.a, + int super.b, + final super.c, + ); + Foo.optional([ + super.a, + int super.b = 123, + ]); + Foo.named({ + required super.a, + int super.b = 123, + }); +} +>>> Function-typed super parameter. +class Foo { + Foo.function(super . a ( int x ), super . b ( ) ? ); + Foo.optional([ int super . c() , super . d ( ) = x ]); + Foo.named({ required int super . e() , super . f ( ) = x }); +} +<<< +class Foo { + Foo.function( + super.a(int x), + super.b()?, + ); + Foo.optional([ + int super.c(), + super.d() = x, + ]); + Foo.named({ + required int super.e(), + super.f() = x, + }); +} \ No newline at end of file diff --git a/test/declaration/external.unit b/test/declaration/external.unit index 80199b5b..2f8d5ccc 100644 --- a/test/declaration/external.unit +++ b/test/declaration/external.unit @@ -71,6 +71,20 @@ class A { int value, ); } +>>> Constructor. +class A { + external A(); + external const A.constant(); + external factory A.fact(); + external const factory A.constantFact(); +} +<<< +class A { + external A(); + external const A.constant(); + external factory A.fact(); + external const factory A.constantFact(); +} >>> Don't split after `external`. class Foo { external var soMuchSoVeryLongFieldNameHere;