Format top-level and local variable declarations. (#1280)

Format top-level and local variable declarations.

This required tweaking the way partial solutions are ordered so that the solver doesn't avoid overflowing solutions that will ultimately lead to non-overflowing lower cost ones.

Author: munificent

lib/src/back_end/solution.dart

@@ -152,10 +152,12 @@ class Score implements Comparable<Score> {
     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

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,15 +36,49 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
   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.
   ///
   /// 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<Directive> 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<void>
 
   @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<Directive> 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<void>
 
   @override
   void visitTopLevelVariableDeclaration(TopLevelVariableDeclaration node) {
-    throw UnimplementedError();
+    visit(node.variables);
+    token(node.semicolon);
   }
 
   @override
@@ -879,17 +892,39 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @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 = <Piece>[];
+    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

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<AstNode> 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<SimpleIdentifier> 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.
   ///

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();

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);

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<Piece> _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<State> 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';
+}

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;