diff --git a/lib/src/back_end/solution.dart b/lib/src/back_end/solution.dart index 4acd22f8..c2e7c306 100644 --- a/lib/src/back_end/solution.dart +++ b/lib/src/back_end/solution.dart @@ -152,10 +152,12 @@ class Score implements Comparable { if (!isValid) return 1; if (!other.isValid) return -1; - // Overflow is always penalized more than line splitting cost. - if (overflow != other.overflow) return overflow.compareTo(other.overflow); + // Even though overflow is "worse" than cost, we order in terms of cost + // because a solution with overflow may lead to a low-cost solution without + // overflow, so we want to explore in cost order. + if (cost != other.cost) return cost.compareTo(other.cost); - return cost.compareTo(other.cost); + return overflow.compareTo(other.overflow); } @override diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index b68be850..900196e2 100644 --- a/lib/src/front_end/ast_node_visitor.dart +++ b/lib/src/front_end/ast_node_visitor.dart @@ -6,6 +6,8 @@ import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/source/line_info.dart'; import '../dart_formatter.dart'; +import '../piece/piece.dart'; +import '../piece/variable.dart'; import '../source_code.dart'; import 'comment_writer.dart'; import 'delimited_list_builder.dart'; @@ -34,7 +36,7 @@ class AstNodeVisitor extends ThrowingAstVisitor AstNodeVisitor(DartFormatter formatter, this.lineInfo, SourceCode source) : writer = PieceWriter(formatter, source); - /// Runs the visitor on [node], formatting its contents. + /// Visits [node] and returns the formatted result. /// /// Returns a [SourceCode] containing the resulting formatted source and /// updated selection, if any. @@ -42,7 +44,41 @@ class AstNodeVisitor extends ThrowingAstVisitor /// This is the only method that should be called externally. Everything else /// is effectively private. SourceCode run(AstNode node) { - visit(node); + // Always treat the code being formatted as contained in a sequence, even + // if we aren't formatting an entire compilation unit. That way, comments + // before and after the node are handled properly. + var sequence = SequenceBuilder(this); + + if (node is CompilationUnit) { + if (node.scriptTag case var scriptTag?) { + sequence.add(scriptTag); + sequence.addBlank(); + } + + // Put a blank line between the library tag and the other directives. + Iterable directives = node.directives; + if (directives.isNotEmpty && directives.first is LibraryDirective) { + sequence.add(directives.first); + sequence.addBlank(); + directives = directives.skip(1); + } + + for (var directive in directives) { + sequence.add(directive); + } + + for (var declaration in node.declarations) { + sequence.add(declaration); + } + } else { + // Just formatting a single statement. + sequence.add(node); + } + + // Write any comments at the end of the code. + sequence.addCommentsBefore(node.endToken.next!); + + writer.push(sequence.build()); // Finish writing and return the complete result. return writer.finish(); @@ -167,32 +203,8 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitCompilationUnit(CompilationUnit node) { - var sequence = SequenceBuilder(this); - - if (node.scriptTag case var scriptTag?) { - sequence.add(scriptTag); - sequence.addBlank(); - } - - // Put a blank line between the library tag and the other directives. - Iterable directives = node.directives; - if (directives.isNotEmpty && directives.first is LibraryDirective) { - sequence.add(directives.first); - sequence.addBlank(); - directives = directives.skip(1); - } - - for (var directive in directives) { - sequence.add(directive); - } - - // TODO(tall): Handle top level declarations. - if (node.declarations.isNotEmpty) throw UnimplementedError(); - - // Write any comments at the end of the file. - sequence.addCommentsBefore(node.endToken.next!); - - writer.push(sequence.build()); + throw UnsupportedError( + 'CompilationUnit should be handled directly by format().'); } @override @@ -854,7 +866,8 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitTopLevelVariableDeclaration(TopLevelVariableDeclaration node) { - throw UnimplementedError(); + visit(node.variables); + token(node.semicolon); } @override @@ -879,17 +892,39 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitVariableDeclaration(VariableDeclaration node) { - throw UnimplementedError(); + token(node.name); + finishAssignment(node.equals, node.initializer); } @override void visitVariableDeclarationList(VariableDeclarationList node) { - throw UnimplementedError(); + // TODO(tall): Format metadata. + if (node.metadata.isNotEmpty) throw UnimplementedError(); + + modifier(node.lateKeyword); + modifier(node.keyword); + + // TODO(tall): Test how splits inside the type annotation (like in a type + // argument list or a function type's parameter list) affect the indentation + // and splitting of the surrounding variable declaration. + visit(node.type); + var header = writer.pop(); + + var variables = []; + for (var variable in node.variables) { + writer.split(); + visit(variable); + commaAfter(variable); + variables.add(writer.pop()); + } + + writer.push(VariablePiece(header, variables, hasType: node.type != null)); } @override void visitVariableDeclarationStatement(VariableDeclarationStatement node) { - throw UnimplementedError(); + visit(node.variables); + token(node.semicolon); } @override diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart index f9ff325c..32e2460f 100644 --- a/lib/src/front_end/piece_factory.dart +++ b/lib/src/front_end/piece_factory.dart @@ -42,14 +42,6 @@ typedef BinaryOperation = (AstNode left, Token operator, AstNode right); mixin PieceFactory implements CommentWriter { void visit(AstNode? node, {void Function()? before, void Function()? after}); - /// Creates metadata annotations for a directive. - /// - /// Always forces the annotations to be on a previous line. - void createDirectiveMetadata(Directive directive) { - // TODO(tall): Implement. See SourceVisitor._visitDirectiveMetadata(). - if (directive.metadata.isNotEmpty) throw UnimplementedError(); - } - /// Creates a [BlockPiece] for a given bracket-delimited block or declaration /// body. void createBlock(Token leftBracket, List nodes, Token rightBracket) { @@ -81,6 +73,14 @@ mixin PieceFactory implements CommentWriter { alwaysSplit: nodes.isNotEmpty)); } + /// Creates metadata annotations for a directive. + /// + /// Always forces the annotations to be on a previous line. + void createDirectiveMetadata(Directive directive) { + // TODO(tall): Implement. See SourceVisitor._visitDirectiveMetadata(). + if (directive.metadata.isNotEmpty) throw UnimplementedError(); + } + /// Creates a dotted or qualified identifier. void createDotted(NodeList components) { for (var component in components) { @@ -229,6 +229,36 @@ mixin PieceFactory implements CommentWriter { writer.push(InfixPiece(operands)); } + /// Creates a [Piece] for some code followed by an `=` and an expression in + /// any place where an `=` appears: + /// + /// * Assignment + /// * Variable declaration + /// * Constructor initializer + /// + /// This method assumes the code to the left of the `=` has already been + /// visited. + /// + /// Does nothing if [equalsOperator] is `null`. + void finishAssignment(Token? equalsOperator, Expression? rightHandSide) { + if (equalsOperator == null) return; + + writer.space(); + token(equalsOperator); + var equals = writer.pop(); + writer.split(); + + visit(rightHandSide); + + var initializer = writer.pop(); + writer.push(InfixPiece([equals, initializer])); + } + + /// Writes an optional modifier that precedes other code. + void modifier(Token? keyword) { + token(keyword, after: writer.space); + } + /// Emit [token], along with any comments and formatted whitespace that comes /// before it. /// diff --git a/lib/src/front_end/sequence_builder.dart b/lib/src/front_end/sequence_builder.dart index 3b78149b..25b96151 100644 --- a/lib/src/front_end/sequence_builder.dart +++ b/lib/src/front_end/sequence_builder.dart @@ -36,7 +36,17 @@ class SequenceBuilder { /// Visits [node] and adds the resulting [Piece] to this sequence, handling /// any comments or blank lines that appear before it. void add(AstNode node) { - addCommentsBefore(node.beginToken); + var token = switch (node) { + // If [node] is an [AnnotatedNode], then [beginToken] includes the + // leading doc comment, which we want to handle separately. So, in that + // case, explicitly skip past the doc comment to the subsequent metadata + // (if there is any), or the beginning of the code. + AnnotatedNode(metadata: [var annotation, ...]) => annotation.beginToken, + AnnotatedNode() => node.firstTokenAfterCommentAndMetadata, + _ => node.beginToken + }; + + addCommentsBefore(token); _visitor.visit(node); _contents.add(_visitor.writer.pop()); _visitor.writer.split(); diff --git a/lib/src/piece/postfix.dart b/lib/src/piece/postfix.dart index 97b0753a..eb933687 100644 --- a/lib/src/piece/postfix.dart +++ b/lib/src/piece/postfix.dart @@ -30,6 +30,12 @@ class PostfixPiece extends Piece { @override void format(CodeWriter writer, State state) { + // If any of the operands split, then force the postfix sequence to split + // too. + // TODO(tall): This will need to be revisited when we use PostfixPiece for + // actual postfix operators where this isn't always desired. + if (state == State.initial) writer.setAllowNewlines(false); + for (var piece in pieces) { writer.splitIf(state == State.split, indent: Indent.expression); writer.format(piece); diff --git a/lib/src/piece/variable.dart b/lib/src/piece/variable.dart new file mode 100644 index 00000000..ace6f502 --- /dev/null +++ b/lib/src/piece/variable.dart @@ -0,0 +1,99 @@ +// 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 variable declaration. +/// +/// Used for local variable declaration statements, top-level variable +/// declarations and field declarations. +/// +/// Typed and untyped variables have slightly different splitting logic. +/// Untyped variables never split after the keyword but do indent subsequent +/// variables: +/// +/// ``` +/// var longVariableName = initializer, +/// anotherVariable = anotherInitializer; +/// ``` +/// +/// Typed variables can split that way too: +/// +/// ``` +/// String longVariableName = initializer, +/// anotherVariable = anotherInitializer; +/// ``` +/// +/// But they can also split after the type annotation. When that happens, the +/// variables aren't indented: +/// +/// ``` +/// VeryLongTypeName +/// longVariableName = initializer, +/// anotherVariable = anotherInitializer; +/// ``` +class VariablePiece extends Piece { + /// Split between each variable in a multiple variable declaration. + static const State _betweenVariables = State(1); + + /// Split after the type annotation and between each variable. + static const State _afterType = State(2); + + /// The leading keywords (`var`, `final`, `late`) and optional type + /// annotation. + final Piece _header; + + /// Each individual variable being declared. + final List _variables; + + /// Whether the variable declaration has a type annotation. + final bool _hasType; + + /// Creates a [VariablePiece]. + /// + /// The [hasType] parameter should be `true` if the variable declaration has + /// a type annotation. + VariablePiece(this._header, this._variables, {required bool hasType}) + : _hasType = hasType; + + @override + List get states => [ + if (_variables.length > 1) _betweenVariables, + if (_hasType) _afterType, + ]; + + @override + void format(CodeWriter writer, State state) { + writer.format(_header); + + // If we split at the variables (but not the type), then indent the + // variables and their initializers. + if (state == _betweenVariables) writer.setIndent(Indent.expression); + + // Force variables to split if an initializer does. + if (_variables.length > 1 && state == State.initial) { + writer.setAllowNewlines(false); + } + + // Split after the type annotation. + writer.splitIf(state == _afterType); + + for (var i = 0; i < _variables.length; i++) { + // Split between variables. + if (i > 0) writer.splitIf(state != State.initial); + + writer.format(_variables[i]); + } + } + + @override + void forEachChild(void Function(Piece piece) callback) { + callback(_header); + _variables.forEach(callback); + } + + @override + String toString() => 'Var'; +} diff --git a/lib/src/testing/test_file.dart b/lib/src/testing/test_file.dart index fccd77de..0e019b50 100644 --- a/lib/src/testing/test_file.dart +++ b/lib/src/testing/test_file.dart @@ -48,9 +48,6 @@ class TestFile { factory TestFile._load(File file, String relativePath) { var lines = file.readAsLinesSync(); - // Ignore comment lines. - lines.removeWhere((line) => line.startsWith('###')); - // The first line may have a "|" to indicate the page width. var i = 0; int? pageWidth; diff --git a/test/statement/variable.stmt b/test/statement/variable.stmt new file mode 100644 index 00000000..63ce894e --- /dev/null +++ b/test/statement/variable.stmt @@ -0,0 +1,125 @@ +40 columns | +>>> Short initializer. +var a = 1 ; +<<< +var a = 1; +>>> Split initializer. +var variableName = veryLongInitializerName; +<<< +var variableName = + veryLongInitializerName; +>>> Split overflowing initializer. +var variableName = thisIsReallyQuiteAVeryLongVariableName; +<<< +var variableName = + thisIsReallyQuiteAVeryLongVariableName; +>>> No split after keyword. +var thisIsReallyQuiteAVeryLongVariableName; +<<< +var thisIsReallyQuiteAVeryLongVariableName; +>>> Multiple unsplit variables with no initializers. +var a , b , c , d; +<<< +var a, b, c, d; +>>> Multiple split variables with no initializers. +var variableA, variableB, variableC, variableD; +<<< +var variableA, + variableB, + variableC, + variableD; +>>> Multiple unsplit variables with initializers. +var x = 3, y = 4; +<<< +var x = 3, y = 4; +>>> +var x = 2, y; +<<< +var x = 2, y; +>>> Multiple split variables with initializers. +var x = initializer, y = initializer, z = initializer; +<<< +var x = initializer, + y = initializer, + z = initializer; +>>> Indent initializers if there are multiple variables. +var veryLongVariable1 = longishInitializer, + short = value, + veryLongVariable2 = longishInitializer; +<<< +var veryLongVariable1 = + longishInitializer, + short = value, + veryLongVariable2 = + longishInitializer; +>>> Final variable. +final x = 1 ; +<<< +final x = 1; +>>> Typed variable. +int x = 1 ; +<<< +int x = 1; +>>> Typed final variable. +final int x = 1; +<<< +final int x = 1; +>>> Late variables. +{ + late var i ; + late final int i ; + late int i ; +} +<<< +{ + late var i; + late final int i; + late int i; +} +>>> Split after type name. +SomeVeryLongTypeName someLongVariableName; +<<< +SomeVeryLongTypeName +someLongVariableName; +>>> Split after type name with multiple variables. +SomeVeryLongTypeName someLongVariableName, another, third; +<<< +SomeVeryLongTypeName +someLongVariableName, +another, +third; +>>> Split in initializer without splitting after type. +int someLongVariableName = veryLong + initializer; +<<< +int someLongVariableName = + veryLong + initializer; +>>> Split in initializers without splitting after type. +int someLongVariableName = longInitializerValue, +another = short, thirdLongVariable = anotherLongInitializer; +<<< +int someLongVariableName = + longInitializerValue, + another = short, + thirdLongVariable = + anotherLongInitializer; +>>> Split after type name and in initializer. +SomeVeryLongTypeName someLongVariableName = longInitializerValue; +<<< +SomeVeryLongTypeName +someLongVariableName = + longInitializerValue; +>>> Split after type name and in initializers. +SomeVeryLongTypeName someLongVariableName = longInitializerValue, +another = short, thirdLongVariable = anotherLongInitializer; +<<< +SomeVeryLongTypeName +someLongVariableName = + longInitializerValue, +another = short, +thirdLongVariable = + anotherLongInitializer; +>>> Prefer to split at "=" over infix operator. +int variableName = argument * argument + argument; +<<< +int variableName = + argument * argument + argument; \ No newline at end of file diff --git a/test/statement/variable_comment.stmt b/test/statement/variable_comment.stmt new file mode 100644 index 00000000..b5247608 --- /dev/null +++ b/test/statement/variable_comment.stmt @@ -0,0 +1,20 @@ +40 columns | +>>> Inline after keyword. +var /* comment */ x; +<<< +var /* comment */ x; +>>> Trailing line comment. +var x; // comment +<<< +var x; // comment +>>> Inside multiple variable declaration list. +var x /* comment */, y; +<<< +var x /* comment */, y; +>>> Always place newline after multi-line block comment. +/* +*/ var i = value; +<<< +/* +*/ +var i = value; \ No newline at end of file diff --git a/test/top_level/variable.unit b/test/top_level/variable.unit new file mode 100644 index 00000000..55964a1e --- /dev/null +++ b/test/top_level/variable.unit @@ -0,0 +1,15 @@ +40 columns | +### The same code is used for formatting top-level and local variables, so we +### don't repeat all of the tests here. Instead, we just ensure that the basics +### are handled for top-level variables and rely on the local variable tests +### for everything else. +<<< Untyped. +late final longVariable = 1, anotherVariable = 2; +<<< +late final longVariable = 1, + anotherVariable = 2; +>>> Typed. +SomeLongTypeName longVariable = longInitializer; +<<< +SomeLongTypeName longVariable = + longInitializer; \ No newline at end of file diff --git a/test/top_level/variable_comment.unit b/test/top_level/variable_comment.unit new file mode 100644 index 00000000..6cc3543e --- /dev/null +++ b/test/top_level/variable_comment.unit @@ -0,0 +1,14 @@ +40 columns | +>>> +var x ; // comment +<<< +var x; // comment +>>> Force blank line before doc comments. +var a = 1; +/// doc +var b = 2; +<<< +var a = 1; + +/// doc +var b = 2; \ No newline at end of file