Correct indentation on parameters with both metadata and default values. (#1478)

Correct indentation on parameters with both metadata and default values.

The AST node for a parameter with both metadata and a default value
looks like:

    DefaultValueFormalParameter
      FormalParameter

The inner node has the metadata and the outer one has the default value.
Prior to this PR, that meant that we'd create an AssignPiece for the
default value whose left-hand side piece was the function parameter and
its metadata. That meant that when the metadata split, the outer
AssignPiece would be forced to split.

This slightly awkward change makes the piece structure reflect how the
user (and, alas, not Analyzer) thinks of the syntax where the metadata
is part of the entire parameter and the default value is inside the
parameter.

Fix #1461.

Co-authored-by: Nate Bosch <[email protected]>

Author: munificent

lib/src/front_end/ast_node_visitor.dart

@@ -465,12 +465,9 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
 
   @override
   void visitDefaultFormalParameter(DefaultFormalParameter node) {
-    if (node.separator case var separator?) {
-      writeAssignment(node.parameter, separator, node.defaultValue!,
-          spaceBeforeOperator: separator.type == TokenType.EQ);
-    } else {
-      pieces.visit(node.parameter);
-    }
+    // Visit the inner parameter. It will then access its parent to extract the
+    // default value.
+    pieces.visit(node.parameter);
   }
 
   @override

lib/src/front_end/piece_factory.dart

@@ -527,8 +527,16 @@ mixin PieceFactory {
   /// If [fieldKeyword] and [period] are given, the former should be the `this`
   /// or `super` keyword for an initializing formal or super parameter.
   void writeFormalParameter(
-      NormalFormalParameter node, TypeAnnotation? type, Token? name,
+      FormalParameter node, TypeAnnotation? type, Token? name,
       {Token? mutableKeyword, Token? fieldKeyword, Token? period}) {
+    // If the parameter has a default value, the parameter node will be wrapped
+    // in a DefaultFormalParameter node containing the default.
+    (Token separator, Expression value)? defaultValueRecord;
+    if (node.parent
+        case DefaultFormalParameter(:var separator?, :var defaultValue?)) {
+      defaultValueRecord = (separator, defaultValue);
+    }
+
     writeParameter(
         metadata: node.metadata,
         modifiers: [
@@ -539,7 +547,8 @@ mixin PieceFactory {
         type,
         fieldKeyword: fieldKeyword,
         period: period,
-        name);
+        name,
+        defaultValue: defaultValueRecord);
   }
 
   /// Writes a function, method, getter, or setter declaration.
@@ -604,10 +613,38 @@ mixin PieceFactory {
     });
   }
 
+  /// If [parameter] has a [defaultValue] then writes a piece for the parameter
+  /// followed by that default value.
+  ///
+  /// Otherwise, just writes [parameter].
+  void writeDefaultValue(
+      Piece parameter, (Token separator, Expression value)? defaultValue) {
+    if (defaultValue == null) {
+      pieces.add(parameter);
+      return;
+    }
+
+    var (separator, value) = defaultValue;
+    var operatorPiece = pieces.build(() {
+      if (separator.type == TokenType.EQ) pieces.space();
+      pieces.token(separator);
+      if (separator.type != TokenType.EQ) pieces.space();
+    });
+
+    var valuePiece = nodePiece(value, context: NodeContext.assignment);
+
+    pieces.add(AssignPiece(
+        left: parameter,
+        operatorPiece,
+        valuePiece,
+        canBlockSplitRight: value.canBlockSplit));
+  }
+
   /// Writes a function type or function-typed formal.
   ///
   /// If creating a piece for a function-typed formal, then [parameter] is the
-  /// formal parameter.
+  /// formal parameter. If there is a default value, then [defaultValue] is
+  /// the `=` or `:` separator followed by the constant expression.
   ///
   /// If this is a function-typed initializing formal (`this.foo()`), then
   /// [fieldKeyword] is `this` and [period] is the `.`. Likewise, for a
@@ -621,44 +658,58 @@ mixin PieceFactory {
       {FormalParameter? parameter,
       Token? fieldKeyword,
       Token? period}) {
-    Piece? returnTypePiece;
-    if (parameter != null && returnType != null) {
-      // Attach any parameter metadata and modifiers to the return type.
-      returnTypePiece =
-          pieces.build(metadata: parameter.metadata, inlineMetadata: true, () {
-        pieces.modifier(parameter.requiredKeyword);
-        pieces.modifier(parameter.covariantKeyword);
-        pieces.visit(returnType);
-      });
-    } else if (returnType != null) {
-      returnTypePiece = nodePiece(returnType);
+    // 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);
     }
 
-    // If there's no return type, attach the metadata to the signature.
-    var signatureMetadata = const <Annotation>[];
-    if (parameter != null && returnType == null) {
-      signatureMetadata = parameter.metadata;
-    }
+    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);
+          }
 
-    var signature =
-        pieces.build(metadata: signatureMetadata, inlineMetadata: true, () {
-      // If there's no return type, attach the parameter modifiers to the
-      // signature.
-      if (parameter != null && returnType == null) {
-        pieces.modifier(parameter.requiredKeyword);
-        pieces.modifier(parameter.covariantKeyword);
+          pieces.visit(returnType);
+        });
       }
 
-      pieces.token(fieldKeyword);
-      pieces.token(period);
-      pieces.token(functionKeywordOrName);
-      pieces.visit(typeParameters);
-      pieces.visit(parameters);
-      pieces.token(question);
-    });
+      var signature = pieces.build(() {
+        // If there's no return type, attach the parameter modifiers to the
+        // signature.
+        if (parameter != null && returnType == null) {
+          pieces.modifier(parameter.requiredKeyword);
+          pieces.modifier(parameter.covariantKeyword);
+        }
 
-    pieces.add(FunctionPiece(returnTypePiece, signature,
-        isReturnTypeFunctionType: returnType is GenericFunctionType));
+        pieces.token(fieldKeyword);
+        pieces.token(period);
+        pieces.token(functionKeywordOrName);
+        pieces.visit(typeParameters);
+        pieces.visit(parameters);
+        pieces.token(question);
+      });
+
+      var function = FunctionPiece(returnTypePiece, signature,
+          isReturnTypeFunctionType: returnType is GenericFunctionType);
+
+      // 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
+      // block-splittable. But since it's a FunctionPiece and not directly a
+      // ListPiece, AssignPiece doesn't support block-splitting it. If #1466 is
+      // fixed, that may enable us to handle block-splitting here too. In
+      // 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);
+    });
   }
 
   /// Writes a piece for the header -- everything from the `if` keyword to the
@@ -1316,11 +1367,15 @@ mixin PieceFactory {
   /// Writes a piece for a parameter-like constructor: Either a simple formal
   /// parameter or a record type field, which is syntactically similar to a
   /// parameter.
+  ///
+  /// If the parameter has a default value, then [defaultValue] contains the
+  /// `:` or `=` separator and the constant value expression.
   void writeParameter(TypeAnnotation? type, Token? name,
       {List<Annotation> metadata = const [],
       List<Token?> modifiers = const [],
       Token? fieldKeyword,
-      Token? period}) {
+      Token? period,
+      (Token separator, Expression value)? defaultValue}) {
     // Begin a piece to attach metadata to the parameter.
     pieces.withMetadata(metadata, inlineMetadata: true, () {
       Piece? typePiece;
@@ -1351,14 +1406,20 @@ mixin PieceFactory {
         });
       }
 
+      Piece parameterPiece;
       if (typePiece != null && namePiece != null) {
         // We have both a type and name, allow splitting between them.
-        pieces.add(VariablePiece(typePiece, [namePiece], hasType: true));
-      } else if (typePiece != null) {
-        pieces.add(typePiece);
-      } else if (namePiece != null) {
-        pieces.add(namePiece);
+        parameterPiece = VariablePiece(typePiece, [namePiece], hasType: true);
+      } else {
+        // Will have at least a type or name.
+        parameterPiece = typePiece ?? namePiece!;
       }
+
+      // If there's a default value, include it. We do that inside here so that
+      // any metadata surrounds the entire assignment instead of being part of
+      // the assignment's left-hand side where a split in the metadata would
+      // force a split at the default value separator.
+      writeDefaultValue(parameterPiece, defaultValue);
     });
   }
 

test/tall/function/default_value.unit

@@ -45,4 +45,30 @@ function([
   ],
 ]) {
   body;
-}
+}
+>>> Function typed parameter with default value.
+f([callback() = constantFunction]) {}
+<<<
+f([callback() = constantFunction]) {}
+>>> Split in function typed parameter parameter list with default value.
+f([callback(SomeLongType longParameter, AnotherType anotherParameter) = constantFunction]) {}
+<<<
+### TODO(rnystrom): The output isn't ideal here, but code like this is very
+### rare. See the comment in PieceFactory.writeFunctionType().
+f([
+  callback(
+        SomeLongType longParameter,
+        AnotherType anotherParameter,
+      ) =
+      constantFunction,
+]) {}
+>>> Split in function typed parameter default value.
+f([callback(SomeType parameter) = const CallableClass(someLongConstantArgument, anotherConstantArgument)]) {}
+<<<
+f([
+  callback(SomeType parameter) =
+      const CallableClass(
+        someLongConstantArgument,
+        anotherConstantArgument,
+      ),
+]) {}

test/tall/function/metadata.unit

@@ -87,4 +87,12 @@ class Foo {
     @bar super.field, [
     @foo() @baz super.another,
   ]);
-}
+}
+>>> Function typed parameter with default value.
+f([@metadata @another(argument, argument) callback() = constantFunction]) {}
+<<<
+f([
+  @metadata
+  @another(argument, argument)
+  callback() = constantFunction,
+]) {}

test/tall/regression/0200/0247.unit

@@ -12,9 +12,7 @@
     @Option(help: 'The git Uri containing the jefe.yaml.', abbr: 'g')
     String gitUri,
     @Option(help: 'The directory to install into', abbr: 'd')
-        String installDirectory:
-        '.',
+    String installDirectory: '.',
     @Flag(help: 'Skips the checkout of the develop branch', abbr: 's')
-        bool skipCheckout:
-        false,
+    bool skipCheckout: false,
   }) async {}

test/tall/regression/0300/0387.unit

@@ -7,15 +7,12 @@ greet(@Rest(valueHelp: 'who', help: 'Name(s) to greet.') List<String> who,
     @Option(name: 'greeting', help: 'Alternate word to greet with e.g. "Hi".')
     String salutation: 'Hello'}) {}
 <<<
-### TODO(1461): The indentation is wrong here.
 greet(
   @Rest(valueHelp: 'who', help: 'Name(s) to greet.') List<String> who, {
   @Group.start(title: 'Output')
-      @Option(help: 'How many !\'s to append.')
-      int enthusiasm:
-      0,
+  @Option(help: 'How many !\'s to append.')
+  int enthusiasm: 0,
   @Flag(abbr: 'l', help: 'Put names on separate lines.') bool lineMode: false,
   @Option(name: 'greeting', help: 'Alternate word to greet with e.g. "Hi".')
-      String salutation:
-      'Hello',
+  String salutation: 'Hello',
 }) {}