Rework how function types, type annotations, and typedefs are handled. (#1493)

In the process of trying to simplify the number of Piece classes, I noticed that FunctionPiece basically exists to split between the return type annotation and the rest of a function. That's pretty similar to how VariablePiece handles splitting between a variable's type annotation and name.

I unified those, but then it made typedefs format funny. Looking into
it, it's because typedefs have `=` but weren't using AssignPiece. Instead, they just never allowed splitting at the `=`. So I made that uniform with the rest of the style and used AssignPiece here.

That led to some weird looking code in cases like:

    Function(int someParameter) someVariable;

If that line doesn't fit, the formatter has to decide whether to split inside the type annotation or between the type and variable. There were different heuristics for return types followed by function names versus type annotations followed by variable names. Unifying those led to some weird output like:

    Function(
      int someParameter,
    ) Function(
      int someParameter,
    ) someVariable;

This is a variable whose type is a function that returns another function. Admittedly, no one writes code like this. Ultimately, I felt like the weirdness was from allowing the variable name to hang off the end of a split annotation. In most places in the style, if an inner construct splits, the outer one does too.

So I changed that. If a type annotation splits, then we also split after the type annotation too. That means after a return type before a function name, or after a variable type before a variable. So instead of allowing:

    SomeGenericClass<
      LongTypeArgument,
      AnotherLongTypeArgument
    > variable;

The split in the type argument list forces the variable name to split too:

    SomeGenericClass<
      LongTypeArgument,
      AnotherLongTypeArgument
    >
    variable;

I think I like how this looks a little more but I'm not sure. In practice, it doesn't matter much because it's rare for a type annotation to be long enough to split, but it does happen. For what it's worth, it's consistent with metadata on variables. If the metadata splits, we also split before the variable too:

    @SomeMetadata(
      'annotation argument',
      'another argument',
    )
    variable;

Thoughts?

Author: munificent

lib/src/front_end/ast_node_visitor.dart

@@ -223,6 +223,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
 
   @override
   void visitBlockFunctionBody(BlockFunctionBody node) {
+    pieces.space();
     writeFunctionBodyModifiers(node);
     pieces.visit(node.block);
   }
@@ -430,7 +431,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
       initializers = createCommaSeparated(node.initializers);
     }
 
-    var body = createFunctionBody(node.body);
+    var body = nodePiece(node.body);
 
     pieces.add(ConstructorPiece(header, parameters, body,
         canSplitParameters: node.parameters.parameters
@@ -595,6 +596,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
   @override
   void visitExpressionFunctionBody(ExpressionFunctionBody node) {
     var operatorPiece = pieces.build(() {
+      pieces.space();
       writeFunctionBodyModifiers(node);
       pieces.token(node.functionDefinition);
     });
@@ -860,12 +862,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
       pieces.token(node.name);
       pieces.visit(node.typeParameters);
       pieces.space();
-      pieces.token(node.equals);
-      // Don't bother allowing splitting after the `=`. It's always better to
-      // split inside the type parameter, type argument, or parameter lists of
-      // the typedef or the aliased type.
-      pieces.space();
-      pieces.visit(node.type);
+      pieces.add(AssignPiece(tokenPiece(node.equals), nodePiece(node.type)));
       pieces.token(node.semicolon);
     });
   }
@@ -1324,6 +1321,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
 
   @override
   void visitNativeFunctionBody(NativeFunctionBody node) {
+    pieces.space();
     pieces.token(node.nativeKeyword);
     pieces.visit(node.stringLiteral, spaceBefore: true);
     pieces.token(node.semicolon);
@@ -1859,43 +1857,44 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
 
   @override
   void visitVariableDeclarationList(VariableDeclarationList node) {
-    var header = pieces.build(
-        metadata: node.metadata,
+    pieces.withMetadata(node.metadata,
         // If the variable is part of a for loop, it looks weird to force the
         // metadata to split since it's in a sort of expression-ish location:
         //
         //     for (@meta var x in list) ...
         inlineMetadata: _parentContext == NodeContext.forLoopVariable, () {
-      pieces.modifier(node.lateKeyword);
-      pieces.modifier(node.keyword);
-      pieces.visit(node.type);
-    });
+      var header = pieces.build(() {
+        pieces.modifier(node.lateKeyword);
+        pieces.modifier(node.keyword);
+        pieces.visit(node.type);
+      });
 
-    var variables = <Piece>[];
-    for (var variable in node.variables) {
-      if ((variable.equals, variable.initializer)
-          case (var equals?, var initializer?)) {
-        var variablePiece = tokenPiece(variable.name);
+      var variables = <Piece>[];
+      for (var variable in node.variables) {
+        if ((variable.equals, variable.initializer)
+            case (var equals?, var initializer?)) {
+          var variablePiece = tokenPiece(variable.name);
 
-        var equalsPiece = pieces.build(() {
-          pieces.space();
-          pieces.token(equals);
-        });
+          var equalsPiece = pieces.build(() {
+            pieces.space();
+            pieces.token(equals);
+          });
 
-        var initializerPiece = nodePiece(initializer,
-            commaAfter: true, context: NodeContext.assignment);
+          var initializerPiece = nodePiece(initializer,
+              commaAfter: true, context: NodeContext.assignment);
 
-        variables.add(AssignPiece(
-            left: variablePiece,
-            equalsPiece,
-            initializerPiece,
-            canBlockSplitRight: initializer.canBlockSplit));
-      } else {
-        variables.add(tokenPiece(variable.name, commaAfter: true));
+          variables.add(AssignPiece(
+              left: variablePiece,
+              equalsPiece,
+              initializerPiece,
+              canBlockSplitRight: initializer.canBlockSplit));
+        } else {
+          variables.add(tokenPiece(variable.name, commaAfter: true));
+        }
       }
-    }
 
-    pieces.add(VariablePiece(header, variables, hasType: node.type != null));
+      pieces.add(VariablePiece(header, variables, hasType: node.type != null));
+    });
   }
 
   @override

lib/src/front_end/piece_factory.dart

@@ -10,7 +10,6 @@ import '../piece/assign.dart';
 import '../piece/clause.dart';
 import '../piece/control_flow.dart';
 import '../piece/for.dart';
-import '../piece/function.dart';
 import '../piece/if_case.dart';
 import '../piece/infix.dart';
 import '../piece/list.dart';
@@ -570,49 +569,56 @@ mixin PieceFactory {
       TypeParameterList? typeParameters,
       FormalParameterList? parameters,
       required FunctionBody body}) {
-    void writeModifiers() {
-      for (var keyword in modifiers) {
-        pieces.modifier(keyword);
-      }
-    }
-
     // Create a piece to attach metadata to the function.
     pieces.withMetadata(metadata, () {
-      Piece? returnTypePiece;
-      // Create a piece for the return type if there is one.
-      if (returnType != null) {
-        returnTypePiece = pieces.build(() {
-          writeModifiers();
-          pieces.visit(returnType);
-        });
-      }
-
-      var signature = pieces.build(() {
+      writeFunctionAndReturnType(modifiers, returnType, () {
         // If there's no return type, attach modifiers to the signature.
-        if (returnType == null) writeModifiers();
+        if (returnType == null) {
+          for (var keyword in modifiers) {
+            pieces.modifier(keyword);
+          }
+        }
 
         pieces.modifier(operatorKeyword);
         pieces.modifier(propertyKeyword);
         pieces.token(name);
         pieces.visit(typeParameters);
         pieces.visit(parameters);
+        pieces.visit(body);
       });
+    });
+  }
+
+  /// Writes a return type followed by either a function signature (when writing
+  /// a function type annotation or function-typed formal) or a signature and a
+  /// body (when writing a function declaration).
+  ///
+  /// The [writeFunction] callback should write the function's signature and
+  /// body if there is one.
+  ///
+  /// If there is no return type, invokes [writeFunction] directly and returns.
+  /// Otherwise, writes the return type and function and wraps them in a piece
+  /// to allow splitting after the return type.
+  void writeFunctionAndReturnType(List<Token?> modifiers,
+      TypeAnnotation? returnType, void Function() writeFunction) {
+    if (returnType == null) {
+      writeFunction();
+      return;
+    }
 
-      var bodyPiece = createFunctionBody(body);
+    var returnTypePiece = pieces.build(() {
+      for (var keyword in modifiers) {
+        pieces.modifier(keyword);
+      }
 
-      pieces.add(FunctionPiece(returnTypePiece, signature,
-          isReturnTypeFunctionType: returnType is GenericFunctionType,
-          body: bodyPiece));
+      pieces.visit(returnType);
     });
-  }
 
-  /// Creates a piece for a function, method, or constructor body.
-  Piece createFunctionBody(FunctionBody body) {
-    return pieces.build(() {
-      // Don't put a space before `;` bodies.
-      if (body is! EmptyFunctionBody) pieces.space();
-      pieces.visit(body);
+    var signature = pieces.build(() {
+      writeFunction();
     });
+
+    pieces.add(VariablePiece(returnTypePiece, [signature], hasType: true));
   }
 
   /// If [parameter] has a [defaultValue] then writes a piece for the parameter
@@ -660,30 +666,9 @@ mixin PieceFactory {
       {FormalParameter? parameter,
       Token? fieldKeyword,
       Token? period}) {
-    // If the type is a function-typed parameter with a default value, then
-    // grab the default value from the parent node.
-    (Token separator, Expression value)? defaultValueRecord;
-    if (parameter?.parent
-        case DefaultFormalParameter(:var separator?, :var defaultValue?)) {
-      defaultValueRecord = (separator, defaultValue);
-    }
-
     var metadata = parameter?.metadata ?? const <Annotation>[];
     pieces.withMetadata(metadata, inlineMetadata: true, () {
-      Piece? returnTypePiece;
-      if (returnType != null) {
-        returnTypePiece = pieces.build(() {
-          // Attach any parameter modifiers to the return type.
-          if (parameter != null) {
-            pieces.modifier(parameter.requiredKeyword);
-            pieces.modifier(parameter.covariantKeyword);
-          }
-
-          pieces.visit(returnType);
-        });
-      }
-
-      var signature = pieces.build(() {
+      void write() {
         // If there's no return type, attach the parameter modifiers to the
         // signature.
         if (parameter != null && returnType == null) {
@@ -697,10 +682,11 @@ mixin PieceFactory {
         pieces.visit(typeParameters);
         pieces.visit(parameters);
         pieces.token(question);
-      });
+      }
 
-      var function = FunctionPiece(returnTypePiece, signature,
-          isReturnTypeFunctionType: returnType is GenericFunctionType);
+      var returnTypeModifiers = parameter != null
+          ? [parameter.requiredKeyword, parameter.covariantKeyword]
+          : const <Token?>[];
 
       // TODO(rnystrom): It would be good if the AssignPiece created for the
       // default value could treat the parameter list on the left-hand side as
@@ -710,7 +696,19 @@ mixin PieceFactory {
       // practice, it doesn't really matter since function-typed formals are
       // deprecated, default values on function-typed parameters are rare, and
       // when both occur, they rarely split.
-      writeDefaultValue(function, defaultValueRecord);
+      // If the type is a function-typed parameter with a default value, then
+      // grab the default value from the parent node and attach it to the
+      // function.
+      if (parameter?.parent
+          case DefaultFormalParameter(:var separator?, :var defaultValue?)) {
+        var function = pieces.build(() {
+          writeFunctionAndReturnType(returnTypeModifiers, returnType, write);
+        });
+
+        writeDefaultValue(function, (separator, defaultValue));
+      } else {
+        writeFunctionAndReturnType(returnTypeModifiers, returnType, write);
+      }
     });
   }
 

lib/src/piece/function.dart

@@ -8,7 +8,8 @@ import 'piece.dart';
 /// A variable declaration.
 ///
 /// Used for local variable declaration statements, top-level variable
-/// declarations and field declarations.
+/// declarations and field declarations. Also used to handle splitting between
+/// a function or function type's return type and the rest of the function.
 ///
 /// Typed and untyped variables have slightly different splitting logic.
 /// Untyped variables never split after the keyword but do indent subsequent
@@ -60,7 +61,7 @@ class VariablePiece extends Piece {
 
   @override
   void format(CodeWriter writer, State state) {
-    writer.format(_header);
+    writer.format(_header, allowNewlines: state != State.unsplit);
 
     // If we split at the variables (but not the type), then indent the
     // variables and their initializers.