Format fields and methods. (#1313)

Format fields and methods.

Almost all of the real work was already done to handle variables and
functions, so this just extends that to fields and methods defined in
classes and covers a few loose ends:

- External declarations in their various forms.
- The covariant modifier on parameters and fields.
- Operator declarations (which apparently were never tested on the old
  style!).
- Getters and setters.
- The `static`, `abstract`, and `late` modifiers.

I also tweaked the cost heuristics around splitting between return
types, parameter lists, and `=>` expression bodies. It looks pretty
weird to split after a return type, so I made that more costly than
splitting at the body or parameter list.

I also decided to put the tests for member declarations inside
declaration/ instead of member/. My impression is that there won't be
a huge number of separate files in declaration/ and member/, so it's
easier to just merge them together.

Author: munificent

lib/src/back_end/code_writer.dart

@@ -261,7 +261,11 @@ class CodeWriter {
       _overflow += _column - _pageWidth;
     }
 
+    // If we found a problematic line, and there is a piece on the line that
+    // we can try to split, then remember that piece so that the solution will
+    // expand it next.
     if (!_foundExpandLine &&
+        _nextPieceToExpand != null &&
         (_column > _pageWidth || _containsInvalidNewline)) {
       // We found a problematic line, so remember it and the piece on it.
       _foundExpandLine = true;

lib/src/front_end/ast_node_visitor.dart

@@ -434,7 +434,12 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @override
   void visitFieldDeclaration(FieldDeclaration node) {
-    throw UnimplementedError();
+    modifier(node.externalKeyword);
+    modifier(node.staticKeyword);
+    modifier(node.abstractKeyword);
+    modifier(node.covariantKeyword);
+    visit(node.fields);
+    token(node.semicolon);
   }
 
   @override
@@ -575,19 +580,14 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @override
   void visitFunctionDeclaration(FunctionDeclaration node) {
-    modifier(node.externalKeyword);
-
-    Piece? returnType;
-    if (node.returnType case var returnTypeNode?) {
-      visit(returnTypeNode);
-      returnType = pieces.split();
-    }
-
-    // TODO(tall): Get or set keywords for getters and setters.
-    if (node.propertyKeyword != null) throw UnimplementedError();
-    token(node.name);
-
-    finishFunction(returnType, node.functionExpression);
+    createFunction(
+        externalKeyword: node.externalKeyword,
+        returnType: node.returnType,
+        propertyKeyword: node.propertyKeyword,
+        name: node.name,
+        typeParameters: node.functionExpression.typeParameters,
+        parameters: node.functionExpression.parameters,
+        body: node.functionExpression.body);
   }
 
   @override
@@ -597,7 +597,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @override
   void visitFunctionExpression(FunctionExpression node) {
-    finishFunction(null, node);
+    finishFunction(null, node.typeParameters, node.parameters, node.body);
   }
 
   @override
@@ -805,7 +805,16 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @override
   void visitMethodDeclaration(MethodDeclaration node) {
-    throw UnimplementedError();
+    createFunction(
+        externalKeyword: node.externalKeyword,
+        modifierKeyword: node.modifierKeyword,
+        returnType: node.returnType,
+        operatorKeyword: node.operatorKeyword,
+        propertyKeyword: node.propertyKeyword,
+        name: node.name,
+        typeParameters: node.typeParameters,
+        parameters: node.parameters,
+        body: node.body);
   }
 
   @override
@@ -1228,6 +1237,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @override
   void visitTopLevelVariableDeclaration(TopLevelVariableDeclaration node) {
+    modifier(node.externalKeyword);
     visit(node.variables);
     token(node.semicolon);
   }

lib/src/front_end/piece_factory.dart

@@ -158,6 +158,39 @@ mixin PieceFactory implements CommentWriter {
     }
   }
 
+  /// Creates a function, method, getter, or setter declaration.
+  ///
+  /// If [modifierKeyword] is given, it should be the `static` or `abstract`
+  /// modifier on a method declaration. If [operatorKeyword] is given, it
+  /// should be the `operator` keyword on an operator declaration. If
+  /// [propertyKeyword] is given, it should be the `get` or `set` keyword on a
+  /// getter or setter declaration.
+  void createFunction(
+      {Token? externalKeyword,
+      Token? modifierKeyword,
+      AstNode? returnType,
+      Token? operatorKeyword,
+      Token? propertyKeyword,
+      required Token name,
+      TypeParameterList? typeParameters,
+      FormalParameterList? parameters,
+      required FunctionBody body}) {
+    modifier(externalKeyword);
+    modifier(modifierKeyword);
+
+    Piece? returnTypePiece;
+    if (returnType != null) {
+      visit(returnType);
+      returnTypePiece = pieces.split();
+    }
+
+    modifier(operatorKeyword);
+    modifier(propertyKeyword);
+    token(name);
+
+    finishFunction(returnTypePiece, typeParameters, parameters, body);
+  }
+
   /// Creates a function type or function-typed formal.
   void createFunctionType(
       TypeAnnotation? returnType,
@@ -429,9 +462,6 @@ mixin PieceFactory implements CommentWriter {
       ({Token leftBracket, List<AstNode> members, Token rightBracket})? body,
       Token? semicolon}) {
     if (metadata.isNotEmpty) throw UnimplementedError('Type metadata.');
-    if (body != null && body.members.isNotEmpty) {
-      throw UnimplementedError('Type members.');
-    }
 
     modifiers.forEach(modifier);
     token(keyword);
@@ -506,7 +536,7 @@ mixin PieceFactory implements CommentWriter {
         leftBracket: leftBracket,
         elements,
         rightBracket: rightBracket,
-        style: const ListStyle(commas: Commas.nonTrailing, splitCost: 2));
+        style: const ListStyle(commas: Commas.nonTrailing, splitCost: 3));
   }
 
   /// Writes the parts of a formal parameter shared by all formal parameter
@@ -515,7 +545,7 @@ mixin PieceFactory implements CommentWriter {
     if (parameter.metadata.isNotEmpty) throw UnimplementedError();
 
     modifier(parameter.requiredKeyword);
-    if (parameter.covariantKeyword != null) throw UnimplementedError();
+    modifier(parameter.covariantKeyword);
   }
 
   /// Handles the `async`, `sync*`, or `async*` modifiers on a function body.
@@ -588,24 +618,25 @@ mixin PieceFactory implements CommentWriter {
   /// Finishes writing a named function declaration or anonymous function
   /// expression after the return type (if any) and name (if any) has been
   /// written.
-  void finishFunction(Piece? returnType, FunctionExpression function) {
-    visit(function.typeParameters);
-    visit(function.parameters);
+  void finishFunction(Piece? returnType, TypeParameterList? typeParameters,
+      FormalParameterList? parameters, FunctionBody body) {
+    visit(typeParameters);
+    visit(parameters);
 
-    Piece parameters;
-    Piece? body;
-    if (function.body case EmptyFunctionBody body) {
+    Piece parametersPiece;
+    Piece? bodyPiece;
+    if (body is EmptyFunctionBody) {
       // If the body is just `;`, then don't allow a space or split before the
       // semicolon by making it part of the parameters piece.
       token(body.semicolon);
-      parameters = pieces.split();
+      parametersPiece = pieces.split();
     } else {
-      parameters = pieces.split();
-      visit(function.body);
-      body = pieces.take();
+      parametersPiece = pieces.split();
+      visit(body);
+      bodyPiece = pieces.take();
     }
 
-    pieces.give(FunctionPiece(returnType, parameters, body));
+    pieces.give(FunctionPiece(returnType, parametersPiece, bodyPiece));
   }
 
   /// Writes an optional modifier that precedes other code.

lib/src/piece/function.dart

@@ -9,6 +9,8 @@ import 'piece.dart';
 ///
 /// Handles splitting between the return type and the rest of the function.
 class FunctionPiece extends Piece {
+  static const _splitAfterReturnType = State(1, cost: 2);
+
   /// The return type annotation, if any.
   final Piece? _returnType;
 
@@ -22,20 +24,21 @@ class FunctionPiece extends Piece {
   FunctionPiece(this._returnType, this._signature, [this._body]);
 
   @override
-  List<State> get additionalStates => [if (_returnType != null) State.split];
+  List<State> get additionalStates =>
+      [if (_returnType != null) _splitAfterReturnType];
 
   @override
   void format(CodeWriter writer, State state) {
     if (_returnType case var returnType?) {
       // A split inside the return type forces splitting after the return type.
-      writer.setAllowNewlines(state == State.split);
+      writer.setAllowNewlines(state == _splitAfterReturnType);
 
       writer.format(returnType);
 
       // A split in the type parameters or parameters does not force splitting
       // after the return type.
       writer.setAllowNewlines(true);
-      writer.splitIf(state == State.split);
+      writer.splitIf(state == _splitAfterReturnType);
     }
 
     writer.format(_signature);

test/README.md

@@ -63,10 +63,12 @@ These tests are all run by `short_format_test.dart`.
 The newer tall style tests are:
 
 ```
-declaration/  - Typedef, class, enum, and extension declarations.
+declaration/  - Typedef, class, enum, extension, mixin, and member declarations.
+                Includes constructors, getters, setters, methods, and fields,
+                but not functions and variables, which are in their own
+                directories below.
 expression/   - Expressions.
 invocation/   - Function and member invocations.
-member/       - Constructor, method, field, getter, and setter declarations.
 selection/    - Test preserving selection information.
 statement/    - Statements.
 top_level/    - Top-level directives.

test/declaration/external.unit

@@ -0,0 +1,84 @@
+40 columns                              |
+>>> Top-level variable.
+  external   final   a ,  b  ;
+  external   final    Set < int  >  a ,  b  ;
+  external   var   a  ;
+  external   List < int >   a  ;
+<<<
+external final a, b;
+external final Set<int> a, b;
+external var a;
+external List<int> a;
+>>> Static field.
+class C {
+  external  static    final   a ,  b  ;
+external    static  final    Set < int  >  a ,  b  ;
+   external   static   var   a  ;
+  external  static    List < int >   a  ;
+}
+<<<
+class C {
+  external static final a, b;
+  external static final Set<int> a, b;
+  external static var a;
+  external static List<int> a;
+}
+>>> Instance field.
+class C {
+  external   final   a ,  b  ;
+  external   final    Set < String  >  a ,  b  ;
+  external   var   a  ;
+  external   List < int >   a  ;
+}
+<<<
+class C {
+  external final a, b;
+  external final Set<String> a, b;
+  external var a;
+  external List<int> a;
+}
+>>> Top-level function.
+external  int  function();
+external  int  get getter;
+external  void  set setter(int value);
+<<<
+external int function();
+external int get getter;
+external void set setter(int value);
+>>> Instance member.
+class A {
+  external  int  function();
+  external  int  get getter;
+  external  void  set setter(int value);
+}
+<<<
+class A {
+  external int function();
+  external int get getter;
+  external void set setter(int value);
+}
+>>> Static member.
+class A {
+  external  static  int function();
+  external  static  int get getter;
+  external  static  void set setter(int value);
+}
+<<<
+class A {
+  external static int function();
+  external static int get getter;
+  external static void set setter(
+    int value,
+  );
+}
+>>> Don't split after `external`.
+class Foo {
+  external var soMuchSoVeryLongFieldNameHere;
+  external SuperLongTypeAnnotation field;
+}
+<<<
+class Foo {
+  external var soMuchSoVeryLongFieldNameHere;
+  external SuperLongTypeAnnotation
+  field;
+}