From cbcb51062279e67b8bcf4f79fba29e2179f3ee9f Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Thu, 2 Nov 2023 13:39:09 -0700 Subject: [PATCH 1/4] Generalize import combinator pieces to other clauses. For imports and exports, there is logic around how to split and format the "show" and "hide" combinators. I plan to use the same logic for "extends", "implements", and "with" clauses in classes so this generalizes that code to not be specific to imports. It's also a little simpler and cleaner now. --- lib/src/constants.dart | 3 - lib/src/front_end/piece_factory.dart | 57 +++++---- lib/src/piece/adjacent.dart | 27 ++++ lib/src/piece/clause.dart | 130 +++++++++++++++++++ lib/src/piece/import.dart | 182 --------------------------- 5 files changed, 188 insertions(+), 211 deletions(-) create mode 100644 lib/src/piece/adjacent.dart create mode 100644 lib/src/piece/clause.dart delete mode 100644 lib/src/piece/import.dart diff --git a/lib/src/constants.dart b/lib/src/constants.dart index 385db06a..be0b6f9a 100644 --- a/lib/src/constants.dart +++ b/lib/src/constants.dart @@ -79,7 +79,4 @@ class Indent { /// The ":" on a wrapped constructor initialization list. static const constructorInitializer = 4; - - /// A split name in a show or hide combinator. - static const combinatorName = 8; } diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart index f1c92420..54646183 100644 --- a/lib/src/front_end/piece_factory.dart +++ b/lib/src/front_end/piece_factory.dart @@ -7,9 +7,10 @@ import 'package:analyzer/dart/ast/token.dart'; import '../ast_extensions.dart'; import '../piece/assign.dart'; import '../piece/block.dart'; +import '../piece/clause.dart'; import '../piece/function.dart'; import '../piece/if.dart'; -import '../piece/import.dart'; +import '../piece/adjacent.dart'; import '../piece/infix.dart'; import '../piece/list.dart'; import '../piece/piece.dart'; @@ -224,9 +225,8 @@ mixin PieceFactory implements CommentWriter { token(keyword); space(); visit(directive.uri); - var directivePiece = pieces.take(); + var importPieces = [pieces.take()]; - Piece? configurationsPiece; if (directive.configurations.isNotEmpty) { var configurations = []; for (var configuration in directive.configurations) { @@ -235,44 +235,49 @@ mixin PieceFactory implements CommentWriter { configurations.add(pieces.take()); } - configurationsPiece = PostfixPiece(configurations); + importPieces.add(PostfixPiece(configurations)); } - Piece? asClause; if (asKeyword != null) { pieces.split(); token(deferredKeyword, after: space); token(asKeyword); space(); visit(prefix); - asClause = PostfixPiece([pieces.take()]); + importPieces.add(PostfixPiece([pieces.take()])); } - var combinators = []; - for (var combinatorNode in directive.combinators) { - pieces.split(); - token(combinatorNode.keyword); - var combinator = ImportCombinator(pieces.take()); - combinators.add(combinator); - - switch (combinatorNode) { - case HideCombinator(hiddenNames: var names): - case ShowCombinator(shownNames: var names): - for (var name in names) { - pieces.split(); - token(name.token); - commaAfter(name); - combinator.names.add(pieces.take()); - } - default: - throw StateError('Unknown combinator type $combinatorNode.'); + if (directive.combinators.isNotEmpty) { + var combinators = []; + for (var combinatorNode in directive.combinators) { + pieces.split(); + token(combinatorNode.keyword); + var combinatorKeyword = pieces.split(); + + switch (combinatorNode) { + case HideCombinator(hiddenNames: var names): + case ShowCombinator(shownNames: var names): + var parts = []; + for (var name in names) { + pieces.split(); + token(name.token); + commaAfter(name); + parts.add(pieces.take()); + } + + var combinator = ClausePiece(combinatorKeyword, parts); + combinators.add(combinator); + default: + throw StateError('Unknown combinator type $combinatorNode.'); + } } + + importPieces.add(ClausesPiece(combinators)); } token(directive.semicolon); - pieces.give(ImportPiece( - directivePiece, configurationsPiece, asClause, combinators)); + pieces.give(AdjacentPiece(importPieces)); } /// Creates a single infix operation. diff --git a/lib/src/piece/adjacent.dart b/lib/src/piece/adjacent.dart new file mode 100644 index 00000000..47ada2a9 --- /dev/null +++ b/lib/src/piece/adjacent.dart @@ -0,0 +1,27 @@ +// 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 simple piece that just writes its child pieces one after the other. +class AdjacentPiece extends Piece { + final List _pieces; + + AdjacentPiece(this._pieces); + + @override + void format(CodeWriter writer, State state) { + for (var piece in _pieces) { + writer.format(piece); + } + } + + @override + void forEachChild(void Function(Piece piece) callback) { + _pieces.forEach(callback); + } + + @override + String toString() => 'Adjacent'; +} diff --git a/lib/src/piece/clause.dart b/lib/src/piece/clause.dart new file mode 100644 index 00000000..d83135a7 --- /dev/null +++ b/lib/src/piece/clause.dart @@ -0,0 +1,130 @@ +// 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 '../constants.dart'; +import 'piece.dart'; + +/// A list of "clauses" where each clause starts with a keyword and has a +/// comma-separated list of items under it. +/// +/// Used for `show` and `hide` combinators in import and export directives, and +/// `extends`, `implements`, and `with` clauses in type declarations. +/// +/// Clauses can be chained on one line if they all fit, like: +/// +/// ``` +/// import 'animals.dart' show Ant, Bat hide Cat, Dog; +/// ``` +/// +/// Or can split before all of the clauses, like: +/// +/// ``` +/// import 'animals.dart' +/// show Ant, Bat +/// hide Cat, Dog; +/// ``` +/// +/// They can also split before every item in any of the clauses. If they do so, +/// then the clauses must split too. So these are allowed: +/// +/// ``` +/// import 'animals.dart' +/// show +/// Ant, +/// Bat +/// hide Cat, Dog; +/// +/// import 'animals.dart' +/// show Ant, Bat +/// hide +/// Cat, +/// Dog; +/// +/// import 'animals.dart' +/// show +/// Ant, +/// Bat +/// hide +/// Cat, +/// Dog; +/// ``` +/// +/// But these are not: +/// +/// ``` +/// // Wrap list but not keyword: +/// import 'animals.dart' show +/// Ant, +/// Bat +/// hide Cat, Dog; +/// +/// // Wrap one keyword but not both: +/// import 'animals.dart' +/// show Ant, Bat hide Cat, Dog; +/// +/// import 'animals.dart' show Ant, Bat +/// hide Cat, Dog; +/// ``` +/// +/// This ensures that when any wrapping occurs, the keywords are always at the +/// beginning of the line. +class ClausesPiece extends Piece { + final List _clauses; + + ClausesPiece(this._clauses); + + @override + List get additionalStates => const [State.split]; + + @override + void format(CodeWriter writer, State state) { + // If any of the lists inside any of the clauses split, split at the + // keywords too. + writer.setAllowNewlines(state == State.split); + for (var clause in _clauses) { + writer.splitIf(state == State.split, indent: Indent.expression); + writer.format(clause); + } + } + + @override + void forEachChild(void Function(Piece piece) callback) { + _clauses.forEach(callback); + } + + @override + String toString() => 'Clauses'; +} + +/// A keyword followed by a comma-separated list of items described by that +/// keyword. +class ClausePiece extends Piece { + final Piece _keyword; + + /// The list of items in the clause. + final List _parts; + + ClausePiece(this._keyword, this._parts); + + @override + List get additionalStates => const [State.split]; + + @override + void format(CodeWriter writer, State state) { + writer.format(_keyword); + for (var part in _parts) { + writer.splitIf(state == State.split, indent: Indent.expression); + writer.format(part); + } + } + + @override + void forEachChild(void Function(Piece piece) callback) { + callback(_keyword); + _parts.forEach(callback); + } + + @override + String toString() => 'Clause'; +} diff --git a/lib/src/piece/import.dart b/lib/src/piece/import.dart deleted file mode 100644 index 9314dcb1..00000000 --- a/lib/src/piece/import.dart +++ /dev/null @@ -1,182 +0,0 @@ -// 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 '../constants.dart'; -import 'piece.dart'; - -/// An import or export directive and its `show` and `hide` combinators. -/// -/// Contains pieces for the keyword and URI, the optional `as` clause for -/// imports, and the configurations (`if` clauses). -/// -/// Combinators can be split like so: -/// -/// [State.unsplit] All on one line: -/// -/// ``` -/// import 'animals.dart' show Ant, Bat hide Cat, Dog; -/// ``` -/// -/// [_beforeCombinators] Wrap before each keyword: -/// -/// ``` -/// import 'animals.dart' -/// show Ant, Bat -/// hide Cat, Dog; -/// ``` -/// -/// [_firstCombinator] Wrap before each keyword and split the first list of -/// names (only used when there are multiple combinators): -/// -/// ``` -/// import 'animals.dart' -/// show -/// Ant, -/// Bat -/// hide Cat, Dog; -/// ``` -/// -/// [_secondCombinator]: Wrap before each keyword and split the second list of -/// names (only used when there are multiple combinators): -/// -/// ``` -/// import 'animals.dart' -/// show Ant, Bat -/// hide -/// Cat, -/// Dog; -/// ``` -/// -/// [State.split] Wrap before each keyword and split both lists of names: -/// -/// ``` -/// import 'animals.dart' -/// show -/// Ant, -/// Bat -/// hide -/// Cat, -/// Dog; -/// ``` -/// -/// These are not allowed: -/// -/// ``` -/// // Wrap list but not keyword: -/// import 'animals.dart' show -/// Ant, -/// Bat -/// hide Cat, Dog; -/// -/// // Wrap one keyword but not both: -/// import 'animals.dart' -/// show Ant, Bat hide Cat, Dog; -/// -/// import 'animals.dart' show Ant, Bat -/// hide Cat, Dog; -/// ``` -/// -/// This ensures that when any wrapping occurs, the keywords are always at the -/// beginning of the line. -class ImportPiece extends Piece { - /// Split before combinator keywords. - static const _beforeCombinators = State(1); - - /// Split before each name in the first combinator. - static const _firstCombinator = State(2); - - /// Split before each name in the second combinator. - static const _secondCombinator = State(3); - - /// The main directive and its URI. - final Piece _directive; - - /// If the directive has `if` configurations, this is them. - final Piece? _configurations; - - /// The `as` clause for this directive. - /// - /// Null if this is not an import or it has no library prefix. - final Piece? _asClause; - - final List _combinators; - - ImportPiece(this._directive, this._configurations, this._asClause, - this._combinators) { - assert(_combinators.length <= 2); - } - - @override - List get additionalStates => [ - _beforeCombinators, - if (_combinators.length > 1) ...[ - _firstCombinator, - _secondCombinator, - ], - State.split - ]; - - @override - void format(CodeWriter writer, State state) { - writer.format(_directive); - writer.formatOptional(_configurations); - writer.formatOptional(_asClause); - - if (_combinators.isNotEmpty) { - _combinators[0].format(writer, - splitKeyword: state != State.unsplit, - splitNames: state == _firstCombinator || state == State.split); - } - - if (_combinators.length > 1) { - _combinators[1].format(writer, - splitKeyword: state != State.unsplit, - splitNames: state == _secondCombinator || state == State.split); - } - } - - @override - void forEachChild(void Function(Piece piece) callback) { - callback(_directive); - if (_configurations case var configurations?) callback(configurations); - if (_asClause case var asClause?) callback(asClause); - - for (var combinator in _combinators) { - combinator.forEachChild(callback); - } - } - - @override - String toString() => 'Import'; -} - -/// A single `show` or `hide` combinator within an import or export directive. -class ImportCombinator { - /// The `show` or `hide` keyword. - final Piece keyword; - - /// The names being shown or hidden. - final List names = []; - - ImportCombinator(this.keyword); - - void format(CodeWriter writer, - {required bool splitKeyword, required bool splitNames}) { - writer.setAllowNewlines(true); - writer.splitIf(splitKeyword, indent: Indent.expression); - writer.setAllowNewlines(splitKeyword); - writer.format(keyword); - for (var name in names) { - writer.splitIf(splitNames, indent: Indent.combinatorName); - writer.setAllowNewlines(splitNames); - writer.format(name); - } - } - - void forEachChild(void Function(Piece piece) callback) { - callback(keyword); - names.forEach(callback); - } -} From ab182653c59af4674f57d0def3fdf3bea41192de Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Thu, 2 Nov 2023 14:10:25 -0700 Subject: [PATCH 2/4] Fix import order. --- lib/src/front_end/piece_factory.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart index 54646183..14599cda 100644 --- a/lib/src/front_end/piece_factory.dart +++ b/lib/src/front_end/piece_factory.dart @@ -5,12 +5,12 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/token.dart'; import '../ast_extensions.dart'; +import '../piece/adjacent.dart'; import '../piece/assign.dart'; import '../piece/block.dart'; import '../piece/clause.dart'; import '../piece/function.dart'; import '../piece/if.dart'; -import '../piece/adjacent.dart'; import '../piece/infix.dart'; import '../piece/list.dart'; import '../piece/piece.dart'; From f5b4ce40b79ed355e282e92e665d5c42c3b7809d Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Thu, 2 Nov 2023 16:23:44 -0700 Subject: [PATCH 3/4] Format class declarations. This handles all of the stuff that can appear outside of the braces in a class declaration: - The `class` keyword and any class modifiers. - The name. - Type parameters, if any. - The `extends`, `with`, and/or `implements` clauses. - The `native` clause. It doesn't handle members yet. It also handles ensuring there is a blank line before and after class declarations. In the process of doing that, I also migrated over similar code for function declarations where we ensure there is a blank line after non-empty functions. --- lib/src/front_end/ast_node_visitor.dart | 53 +++- lib/src/front_end/delimited_list_builder.dart | 2 +- lib/src/front_end/piece_factory.dart | 101 ++++++- lib/src/piece/block.dart | 2 +- lib/src/piece/clause.dart | 44 ++- lib/src/piece/type.dart | 38 +++ test/declaration/class.unit | 76 ++++++ test/declaration/class_clause.unit | 254 ++++++++++++++++++ test/declaration/class_comment.unit | 76 ++++++ test/declaration/class_type_parameter.unit | 30 +++ test/function/local.stmt | 40 +++ test/function/other.unit | 32 ++- test/selection/selection.unit | 16 +- test/statement/block_comment.stmt | 2 +- test/tall_format_test.dart | 1 + 15 files changed, 744 insertions(+), 23 deletions(-) create mode 100644 lib/src/piece/type.dart create mode 100644 test/declaration/class.unit create mode 100644 test/declaration/class_clause.unit create mode 100644 test/declaration/class_comment.unit create mode 100644 test/declaration/class_type_parameter.unit diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index 75529cc7..b1903913 100644 --- a/lib/src/front_end/ast_node_visitor.dart +++ b/lib/src/front_end/ast_node_visitor.dart @@ -74,8 +74,29 @@ class AstNodeVisitor extends ThrowingAstVisitor sequence.visit(directive); } + var needsBlank = true; for (var declaration in node.declarations) { + // Add a blank line before types with bodies. + var hasBody = declaration is ClassDeclaration || + declaration is EnumDeclaration || + declaration is ExtensionDeclaration; + + if (hasBody) needsBlank = true; + + if (needsBlank) sequence.addBlank(); sequence.visit(declaration); + + needsBlank = false; + if (hasBody) { + // Add a blank line after type declarations with bodies. + needsBlank = true; + } else if (declaration is FunctionDeclaration) { + // Add a blank line after non-empty block functions. + var body = declaration.functionExpression.body; + if (body is BlockFunctionBody) { + needsBlank = body.block.statements.isNotEmpty; + } + } } } else { // Just formatting a single statement. @@ -193,7 +214,26 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitClassDeclaration(ClassDeclaration node) { - throw UnimplementedError(); + createType( + node.metadata, + [ + node.abstractKeyword, + node.baseKeyword, + node.interfaceKeyword, + node.finalKeyword, + node.sealedKeyword, + node.mixinKeyword, + ], + node.classKeyword, + node.name, + node.typeParameters, + node.extendsClause, + node.withClause, + node.implementsClause, + node.nativeClause, + node.leftBracket, + node.members, + node.rightBracket); } @override @@ -374,7 +414,7 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitExtendsClause(ExtendsClause node) { - throw UnimplementedError(); + assert(false, 'This node is handled by PieceFactory.createType().'); } @override @@ -600,7 +640,7 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitImplementsClause(ImplementsClause node) { - throw UnimplementedError(); + assert(false, 'This node is handled by PieceFactory.createType().'); } @override @@ -761,7 +801,10 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitNativeClause(NativeClause node) { - throw UnimplementedError(); + space(); + token(node.nativeKeyword); + space(); + visit(node.name); } @override @@ -1235,7 +1278,7 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitWithClause(WithClause node) { - throw UnimplementedError(); + assert(false, 'This node is handled by PieceFactory.createType().'); } @override diff --git a/lib/src/front_end/delimited_list_builder.dart b/lib/src/front_end/delimited_list_builder.dart index 00dc6cdb..df888749 100644 --- a/lib/src/front_end/delimited_list_builder.dart +++ b/lib/src/front_end/delimited_list_builder.dart @@ -3,11 +3,11 @@ // BSD-style license that can be found in the LICENSE file. import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/token.dart'; -import 'package:dart_style/src/front_end/comment_writer.dart'; import '../comment_type.dart'; import '../piece/list.dart'; import '../piece/piece.dart'; +import 'comment_writer.dart'; import 'piece_factory.dart'; /// Incrementally builds a [ListPiece], handling commas, comments, and diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart index 14599cda..046c0ecc 100644 --- a/lib/src/front_end/piece_factory.dart +++ b/lib/src/front_end/piece_factory.dart @@ -15,6 +15,7 @@ import '../piece/infix.dart'; import '../piece/list.dart'; import '../piece/piece.dart'; import '../piece/postfix.dart'; +import '../piece/type.dart'; import 'ast_node_visitor.dart'; import 'comment_writer.dart'; import 'delimited_list_builder.dart'; @@ -59,24 +60,44 @@ mixin PieceFactory implements CommentWriter { /// if (condition) { /// } else {} /// ``` - void createBlock(Block block, {bool forceSplit = false}) { - token(block.leftBracket); + void createBody(Token leftBracket, List contents, Token rightBracket, + {bool forceSplit = false}) { + token(leftBracket); var leftBracketPiece = pieces.split(); var sequence = SequenceBuilder(this); - for (var node in block.statements) { + for (var node in contents) { sequence.visit(node); + + // If the node has a non-empty braced body, then require a blank line + // between it and the next node. + if (node.hasNonEmptyBody) sequence.addBlank(); } // Place any comments before the "}" inside the block. - sequence.addCommentsBefore(block.rightBracket); + sequence.addCommentsBefore(rightBracket); - token(block.rightBracket); + token(rightBracket); var rightBracketPiece = pieces.take(); pieces.give(BlockPiece( leftBracketPiece, sequence.build(), rightBracketPiece, - alwaysSplit: forceSplit || block.statements.isNotEmpty)); + alwaysSplit: forceSplit || contents.isNotEmpty)); + } + + /// Creates a [BlockPiece] for a given [Block]. + /// + /// If [forceSplit] is `true`, then the block will split even if empty. This + /// is used, for example, with empty blocks in `if` statements followed by + /// `else` clauses: + /// + /// ``` + /// if (condition) { + /// } else {} + /// ``` + void createBlock(Block block, {bool forceSplit = false}) { + createBody(block.leftBracket, block.statements, block.rightBracket, + forceSplit: forceSplit); } /// Creates a piece for a `break` or `continue` statement. @@ -387,6 +408,74 @@ mixin PieceFactory implements CommentWriter { pieces.give(builder.build()); } + /// Creates a class, enum, extension, etc. declaration with a body containing + /// members. + void createType( + NodeList metadata, + List modifiers, + Token keyword, + Token name, + TypeParameterList? typeParameters, + ExtendsClause? extendsClause, + WithClause? withClause, + ImplementsClause? implementsClause, + NativeClause? nativeClause, + Token leftBracket, + List members, + Token rightBracket) { + if (metadata.isNotEmpty) throw UnimplementedError('Type metadata.'); + if (members.isNotEmpty) throw UnimplementedError('Type members.'); + + modifiers.forEach(modifier); + token(keyword); + space(); + token(name); + visit(typeParameters); + var header = pieces.split(); + + var clauses = []; + + void typeClause(Token keyword, List types) { + token(keyword); + var keywordPiece = pieces.split(); + + var typePieces = []; + for (var type in types) { + visit(type); + commaAfter(type); + typePieces.add(pieces.split()); + } + + clauses.add(ClausePiece(keywordPiece, typePieces)); + } + + if (extendsClause != null) { + typeClause(extendsClause.extendsKeyword, [extendsClause.superclass]); + } + + if (withClause != null) { + typeClause(withClause.withKeyword, withClause.mixinTypes); + } + + if (implementsClause != null) { + typeClause( + implementsClause.implementsKeyword, implementsClause.interfaces); + } + + ClausesPiece? clausesPiece; + if (clauses.isNotEmpty) { + clausesPiece = + ClausesPiece(clauses, allowLeadingClause: extendsClause != null); + } + + visit(nativeClause); + space(); + createBody(leftBracket, members, rightBracket); + var body = pieces.take(); + + pieces.give(TypePiece(header, clausesPiece, body)); + } + /// Creates a [ListPiece] for a type argument or type parameter list. void createTypeList( Token leftBracket, Iterable elements, Token rightBracket) { diff --git a/lib/src/piece/block.dart b/lib/src/piece/block.dart index e11ec930..b2ac2967 100644 --- a/lib/src/piece/block.dart +++ b/lib/src/piece/block.dart @@ -28,7 +28,7 @@ class BlockPiece extends Piece { } @override - List get additionalStates => const [State.split]; + List get additionalStates => [if (contents.isNotEmpty) State.split]; @override void format(CodeWriter writer, State state) { diff --git a/lib/src/piece/clause.dart b/lib/src/piece/clause.dart index d83135a7..63834b7e 100644 --- a/lib/src/piece/clause.dart +++ b/lib/src/piece/clause.dart @@ -70,20 +70,49 @@ import 'piece.dart'; /// This ensures that when any wrapping occurs, the keywords are always at the /// beginning of the line. class ClausesPiece extends Piece { + /// State where we split between the clauses but not before the first one. + static const State _betweenClauses = State(1); + final List _clauses; - ClausesPiece(this._clauses); + /// If `true`, then we're allowed to split between the clauses without + /// splitting before the first one too. + /// + /// This is used for class declarations where the `extends` clauses is treated + /// a little specially because it's a deeper coupling to the class and so we + /// want it to stay on the top line even if the other clauses split, like: + /// + /// ``` + /// class BaseClass extends Derived + /// implements OtherThing { + /// ... + /// } + /// ``` + final bool _allowLeadingClause; + + ClausesPiece(this._clauses, {bool allowLeadingClause = false}) + : _allowLeadingClause = allowLeadingClause; @override - List get additionalStates => const [State.split]; + List get additionalStates => + [if (_allowLeadingClause) _betweenClauses, State.split]; @override void format(CodeWriter writer, State state) { - // If any of the lists inside any of the clauses split, split at the - // keywords too. - writer.setAllowNewlines(state == State.split); for (var clause in _clauses) { - writer.splitIf(state == State.split, indent: Indent.expression); + if (_allowLeadingClause && clause == _clauses.first) { + // Before the leading clause, only split when in the fully split state. + // A split inside the first clause forces a split before the keyword. + writer.splitIf(state == State.split, indent: Indent.expression); + writer.setAllowNewlines(state == State.split); + } else { + // For the other clauses (or if there is no leading one), split in the + // fully split state and any split inside and clause forces all of them + // to split. + writer.setAllowNewlines(state != State.unsplit); + writer.splitIf(state != State.unsplit, indent: Indent.expression); + } + writer.format(clause); } } @@ -112,6 +141,9 @@ class ClausePiece extends Piece { @override void format(CodeWriter writer, State state) { + // If any of the parts inside the clause split, split the list. + writer.setAllowNewlines(state != State.unsplit); + writer.format(_keyword); for (var part in _parts) { writer.splitIf(state == State.split, indent: Indent.expression); diff --git a/lib/src/piece/type.dart b/lib/src/piece/type.dart new file mode 100644 index 00000000..9957d999 --- /dev/null +++ b/lib/src/piece/type.dart @@ -0,0 +1,38 @@ +// 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 'clause.dart'; +import 'piece.dart'; + +/// Piece for a type declaration with a body containing members. +class TypePiece extends Piece { + /// The leading keywords and modifiers, type name, and type parameters. + final Piece _header; + + /// The `extends`, `with`, and/or `implements` clauses, if there are any. + final ClausesPiece? _clauses; + + /// The `native` clause, if any, and the type body. + final Piece _body; + + TypePiece(this._header, this._clauses, this._body); + + @override + void format(CodeWriter writer, State state) { + writer.format(_header); + if (_clauses case var clauses?) writer.format(clauses); + writer.space(); + writer.format(_body); + } + + @override + void forEachChild(void Function(Piece piece) callback) { + callback(_header); + if (_clauses case var clauses?) callback(clauses); + callback(_body); + } + + @override + String toString() => 'Type'; +} diff --git a/test/declaration/class.unit b/test/declaration/class.unit new file mode 100644 index 00000000..a9a75c67 --- /dev/null +++ b/test/declaration/class.unit @@ -0,0 +1,76 @@ +40 columns | +>>> Empty body. +class A { + + +} +<<< +class A {} +>>> Don't split empty body. +class LongClassNameWithExactLength____ {} +<<< +class LongClassNameWithExactLength____ {} +>>> Force a blank line before and after a class declaration. +var x = 1; class A {} var y = 2; + + +class B {} + + + +var z = 3; +<<< +var x = 1; + +class A {} + +var y = 2; + +class B {} + +var z = 3; +>>> Class modifiers. +class C1 {} +base class C2 {} +interface class C3 {} +final class C4 {} +sealed class C5 {} +abstract class C6 {} +abstract base class C7 {} +abstract interface class C8 {} +abstract final class C9 {} +mixin class C10 {} +base mixin class C11 {} +abstract mixin class C12 {} +abstract base mixin class C13 {} +<<< +class C1 {} + +base class C2 {} + +interface class C3 {} + +final class C4 {} + +sealed class C5 {} + +abstract class C6 {} + +abstract base class C7 {} + +abstract interface class C8 {} + +abstract final class C9 {} + +mixin class C10 {} + +base mixin class C11 {} + +abstract mixin class C12 {} + +abstract base mixin class C13 {} +>>> Native class. +class SomeClass native "Zapp" { +} +<<< +class SomeClass native "Zapp" {} \ No newline at end of file diff --git a/test/declaration/class_clause.unit b/test/declaration/class_clause.unit new file mode 100644 index 00000000..c9b120f6 --- /dev/null +++ b/test/declaration/class_clause.unit @@ -0,0 +1,254 @@ +40 columns | +>>> Unsplit extends clause. +class A extends B {} +<<< +class A extends B {} +>>> Unsplit implements clause. +class A implements B {} +<<< +class A implements B {} +>>> Unsplit with clause. +class A extends B with C {} +<<< +class A extends B with C {} +>>> Unsplit multiple clauses. +class A extends B with C implements D {} +<<< +class A extends B with C implements D {} +>>> Split at `extends`. +class SomeClass extends VeryLongBaseClass {} +<<< +class SomeClass + extends VeryLongBaseClass {} +>>> Split at `implements`. +class SomeClass implements VeryLongBaseClass {} +<<< +class SomeClass + implements VeryLongBaseClass {} +>>> Split at `implements` but not between interfaces. +class SomeClass implements Interface, AnotherOne {} +<<< +class SomeClass + implements Interface, AnotherOne {} +>>> Split at `implements` and interfaces. +class SomeClass implements Interface, Another, Third {} +<<< +class SomeClass + implements + Interface, + Another, + Third {} +>>> Split at `with`. +class SomeLongClass with VeryLongMixin {} +<<< +class SomeLongClass + with VeryLongMixin {} +>>> Split at `with` but not between mixins. +class SomeClass with Mixin, AnotherOne {} +<<< +class SomeClass + with Mixin, AnotherOne {} +>>> Split at `with` and mixins. +class SomeClass with Mixin, Another, Third, Fourth {} +<<< +class SomeClass + with + Mixin, + Another, + Third, + Fourth {} +>>> Split within `with` but not `implements`. +class SomeClass extends A with Mixin, Another, Third, Fourth, Fifth implements Interface {} +<<< +class SomeClass extends A + with + Mixin, + Another, + Third, + Fourth, + Fifth + implements Interface {} +>>> Split within `implements` but not `with`. +class SomeClass extends A with Mixin implements Interface, Another, Third {} +<<< +class SomeClass extends A + with Mixin + implements + Interface, + Another, + Third {} +>>> Split within both `with` and `implements`. +class SomeClass extends A with Mixin, Another, Third, Fourth, Fifth implements Interface, Another, Third {} +<<< +class SomeClass extends A + with + Mixin, + Another, + Third, + Fourth, + Fifth + implements + Interface, + Another, + Third {} +>>> Split at `extends` splits `implements` too. +class AVeryLongSomeClass extends BaseClass implements I {} +<<< +class AVeryLongSomeClass + extends BaseClass + implements I {} +>>> +class AVeryLongSomeClass extends BaseClass implements Interface {} +<<< +class AVeryLongSomeClass + extends BaseClass + implements Interface {} +>>> Split at `extends` splits `with` too. +class AVeryLongSomeClass extends BaseClass with Mixin {} +<<< +class AVeryLongSomeClass + extends BaseClass + with Mixin {} +>>> Can split `with` clause without splitting `extends`. +class SomeClass extends A with Mixin, Another {} +<<< +class SomeClass extends A + with Mixin, Another {} +>>> +class SomeClass extends A with Mixin, Another, Third, Fourth {} +<<< +class SomeClass extends A + with + Mixin, + Another, + Third, + Fourth {} +>>> Can split `implements` clause without splitting `extends`. +class SomeClass extends A implements Type, Another {} +<<< +class SomeClass extends A + implements Type, Another {} +>>> +class SomeClass extends A implements Type, Another, Third, Fourth {} +<<< +class SomeClass extends A + implements + Type, + Another, + Third, + Fourth {} +>>> Can split `with` and `implements` without splitting `extends`. +class SomeVeryLongClass extends A with Mixin implements Interface {} +<<< +class SomeVeryLongClass extends A + with Mixin + implements Interface {} +>>> +class SomeClass extends A with Mixin, Another implements Interface, Another {} +<<< +class SomeClass extends A + with Mixin, Another + implements Interface, Another {} +>>> Unsplit generic superclass. +class SomeClass extends C {} +<<< +class SomeClass extends C {} +>>> Split before `extends` on generic superclass. +class SomeClass extends C {} +<<< +class SomeClass + extends C {} +>>> Split in generic superclass. +class SomeClass extends C {} +<<< +class SomeClass + extends + C< + VeryLongType, + AnotherLongType + > {} +>>> Unsplit generic mixin. +class SomeClass with C {} +<<< +class SomeClass with C {} +>>> Split before `with` on generic mixin. +class SomeClass with Mixin {} +<<< +class SomeClass + with Mixin {} +>>> Split in generic mixin. +class SomeClass with Mixin {} +<<< +class SomeClass + with + Mixin< + VeryLongType, + AnotherLongType + > {} +>>> Unsplit generic superinterface. +class SomeClass implements C {} +<<< +class SomeClass implements C {} +>>> Split before `implements` on generic superinterface. +class SomeClass implements C {} +<<< +class SomeClass + implements C {} +>>> Split in generic superinterface. +class SomeClass implements C {} +<<< +class SomeClass + implements + C< + VeryLongType, + AnotherLongType + > {} +>>> Split in generic clause forces entire clause list to split. +class SomeClass with A, B, C {} +<<< +class SomeClass + with + A, + B< + LongTypeArgument, + AnotherLongType + >, + C {} +>>> Split in generic `with` clause forces `implements` clause to split. +class SomeClass with B implements C {} +<<< +class SomeClass + with + B< + LongTypeArgument, + AnotherLongType + > + implements C {} +>>> Split in generic `implements` clause forces `with` clause to split. +class SomeClass with B implements C {} +<<< +class SomeClass + with B + implements + C< + LongTypeArgument, + AnotherLongType + > {} +>>> Split in generic `with` clause does not force `extends` clause to split. +class C extends A with B {} +<<< +class C extends A + with + B< + LongTypeArgument, + AnotherLongType + > {} +>>> Split in generic `implements` clause does not force `extends` clause to split. +class C extends A implements B {} +<<< +class C extends A + implements + B< + LongTypeArgument, + AnotherLongType + > {} \ No newline at end of file diff --git a/test/declaration/class_comment.unit b/test/declaration/class_comment.unit new file mode 100644 index 00000000..a5a8aba6 --- /dev/null +++ b/test/declaration/class_comment.unit @@ -0,0 +1,76 @@ +40 columns | +>>> Empty class containing line comment. +class C { // comment +} +<<< +class C { + // comment +} +>>> Empty class containing inline block comment. +class C { /* comment */ } +<<< +class C {/* comment */} +>>> Empty class containing non-inline block comment. +class C { + + /* comment */ + + +} +<<< +class C { + /* comment */ +} +>>> Empty class containing multi-line block comment. +class C { /* comment +line */ } +<<< +class C { + /* comment +line */ +} +>>> +class C { + + /* comment +line */ + + +} +<<< +class C { + /* comment +line */ +} +>>> Remove blank lines before and after comments at end of class. +class C { + + + + // comment + + + +} +<<< +class C { + // comment +} +>>> Force blank line before doc comment between classes. +class Foo {} /// Doc comment. +class Bar {} +<<< +class Foo {} + +/// Doc comment. +class Bar {} +>>> Force newline after JavaDoc-style block comment. +class Foo {} +/** +*/ class Bar {} +<<< +class Foo {} + +/** +*/ +class Bar {} \ No newline at end of file diff --git a/test/declaration/class_type_parameter.unit b/test/declaration/class_type_parameter.unit new file mode 100644 index 00000000..6e28b06b --- /dev/null +++ b/test/declaration/class_type_parameter.unit @@ -0,0 +1,30 @@ +40 columns | +>>> Single type parameter. +class Foo < T > {} +<<< +class Foo {} +>>> Multiple type parameters on one line. +class Foo < A , B , C , D > {} +<<< +class Foo {} +>>> Bounds. +class A < T extends int , R extends T ? > {} +<<< +class A {} +>>> Split type parameters. +class LongClassName {} +<<< +class LongClassName< + First, + Second, + Third +> {} +>>> Split inside type parameter bound splits type parameters. +class LongClassName> {} +<<< +class LongClassName< + T extends Map< + LongTypeArgument, + Another + > +> {} \ No newline at end of file diff --git a/test/function/local.stmt b/test/function/local.stmt index cbee7a74..4bfdba9c 100644 --- a/test/function/local.stmt +++ b/test/function/local.stmt @@ -11,4 +11,44 @@ main() { int localFunction(String parameter) { body; } +} +>>> Force blank line after non-empty local function. +{ + a() {;} + b(); + + + c() {;}d(){;} + + +} +<<< +{ + a() { + ; + } + + b(); + + c() { + ; + } + + d() { + ; + } +} +>>> Do not force blank line after empty local function. +{ a() {} b() {} } +<<< +{ + a() {} + b() {} +} +>>> Do not force blank line after expression body local function. +{ a() => null; b() => null; } +<<< +{ + a() => null; + b() => null; } \ No newline at end of file diff --git a/test/function/other.unit b/test/function/other.unit index 4bc18c7f..0274f9a7 100644 --- a/test/function/other.unit +++ b/test/function/other.unit @@ -2,4 +2,34 @@ >>> External function. external void printToConsole ( line ) ; <<< -external void printToConsole(line); \ No newline at end of file +external void printToConsole(line); +>>> Force blank line after non-empty top-level function. +a() {;} +var x = 1; + + +c() {;}d(){;} +<<< +a() { + ; +} + +var x = 1; + +c() { + ; +} + +d() { + ; +} +>>> Do not force blank line after empty top-level function. +a() {} b() {} +<<< +a() {} +b() {} +>>> Do not force blank line after expression body top-level function. +a() => null; b() => null; +<<< +a() => null; +b() => null; \ No newline at end of file diff --git a/test/selection/selection.unit b/test/selection/selection.unit index 10ddc620..14b1cea8 100644 --- a/test/selection/selection.unit +++ b/test/selection/selection.unit @@ -1,5 +1,17 @@ 40 columns | ->>> inside script tag +>>> Inside script tag. #!scr‹ip›t <<< -#!scr‹ip›t \ No newline at end of file +#!scr‹ip›t +>>> Inside blank lines between declarations. +class Foo {} + + +‹ › + + +class Bar {} +<<< +class Foo {} + +‹›class Bar {} \ No newline at end of file diff --git a/test/statement/block_comment.stmt b/test/statement/block_comment.stmt index 526cbbf8..06369baf 100644 --- a/test/statement/block_comment.stmt +++ b/test/statement/block_comment.stmt @@ -86,7 +86,7 @@ line */ c; } ->>> comment before labeled statement +>>> Comment before labeled statement. { /* c */ a: b: c; } diff --git a/test/tall_format_test.dart b/test/tall_format_test.dart index e90eb784..3f7b3ae9 100644 --- a/test/tall_format_test.dart +++ b/test/tall_format_test.dart @@ -10,6 +10,7 @@ import 'package:test/test.dart'; import 'utils.dart'; void main() async { + await testDirectory('declaration', tall: true); await testDirectory('expression', tall: true); await testDirectory('function', tall: true); await testDirectory('invocation', tall: true); From a7f5f9091cac5bb8c0a5390aa701245a4f2a105f Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Fri, 3 Nov 2023 10:33:46 -0700 Subject: [PATCH 4/4] Simplify handling blanks between declarations in a compilation unit. --- lib/src/ast_extensions.dart | 2 ++ lib/src/front_end/ast_node_visitor.dart | 20 +++++--------------- lib/src/piece/type.dart | 2 ++ 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/lib/src/ast_extensions.dart b/lib/src/ast_extensions.dart index 30e4f40e..2f8c3f44 100644 --- a/lib/src/ast_extensions.dart +++ b/lib/src/ast_extensions.dart @@ -33,6 +33,8 @@ extension AstNodeExtensions on AstNode { body = node.body; } else if (node is FunctionDeclarationStatement) { body = node.functionDeclaration.functionExpression.body; + } else if (node is FunctionDeclaration) { + body = node.functionExpression.body; } return body is BlockFunctionBody && body.block.statements.isNotEmpty; diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index b90c6be9..7cdf6db3 100644 --- a/lib/src/front_end/ast_node_visitor.dart +++ b/lib/src/front_end/ast_node_visitor.dart @@ -6,6 +6,7 @@ import 'package:analyzer/dart/ast/token.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/source/line_info.dart'; +import '../ast_extensions.dart'; import '../constants.dart'; import '../dart_formatter.dart'; import '../piece/block.dart'; @@ -75,29 +76,18 @@ class AstNodeVisitor extends ThrowingAstVisitor sequence.visit(directive); } - var needsBlank = true; for (var declaration in node.declarations) { - // Add a blank line before types with bodies. var hasBody = declaration is ClassDeclaration || declaration is EnumDeclaration || declaration is ExtensionDeclaration; - if (hasBody) needsBlank = true; + // Add a blank line before types with bodies. + if (hasBody) sequence.addBlank(); - if (needsBlank) sequence.addBlank(); sequence.visit(declaration); - needsBlank = false; - if (hasBody) { - // Add a blank line after type declarations with bodies. - needsBlank = true; - } else if (declaration is FunctionDeclaration) { - // Add a blank line after non-empty block functions. - var body = declaration.functionExpression.body; - if (body is BlockFunctionBody) { - needsBlank = body.block.statements.isNotEmpty; - } - } + // Add a blank line after type or function declarations with bodies. + if (hasBody || declaration.hasNonEmptyBody) sequence.addBlank(); } } else { // Just formatting a single statement. diff --git a/lib/src/piece/type.dart b/lib/src/piece/type.dart index 9957d999..152b59e1 100644 --- a/lib/src/piece/type.dart +++ b/lib/src/piece/type.dart @@ -6,6 +6,8 @@ import 'clause.dart'; import 'piece.dart'; /// Piece for a type declaration with a body containing members. +/// +/// Used for class, enum, and extension declarations. class TypePiece extends Piece { /// The leading keywords and modifiers, type name, and type parameters. final Piece _header;