Migrator: Insert required keyword in correct position

Previously, migration might change:

m({@required @deprecated x}) {}

to

m({required @deprecated x}) {}

Which is illegal syntax.

Change-Id: Ib8397e9e22dbb951758a04c085c40905860a7cb3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/170026
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>

Author: srawlins

pkg/nnbd_migration/lib/src/edit_plan.dart

@@ -495,11 +495,11 @@ class EditPlanner {
     }
   }
 
-  /// Creates a new edit plan that removes [node] from the AST.
+  /// Creates a new edit plan that removes [sourceNode] from the AST.
   ///
-  /// [node] must be one element of a variable length sequence maintained by
-  /// [node]'s parent (for example, a statement in a block, an element in a
-  /// list, a declaration in a class, etc.).  If it is not, an exception is
+  /// [sourceNode] must be one element of a variable length sequence maintained
+  /// by [sourceNode]'s parent (for example, a statement in a block, an element
+  /// in a list, a declaration in a class, etc.).  If it is not, an exception is
   /// thrown.
   ///
   /// Optional argument [info] contains information about why the change was
@@ -674,9 +674,10 @@ class EditPlanner {
   ///
   /// Optional argument [info] contains information about why the change was
   /// made.
-  EditPlan tryRemoveNode(AstNode sourceNode, {AtomicEditInfo info}) {
+  EditPlan tryRemoveNode(AstNode sourceNode,
+      {List<AstNode> sequenceNodes, AtomicEditInfo info}) {
     var parent = sourceNode.parent;
-    var sequenceNodes = _computeSequenceNodes(parent);
+    sequenceNodes ??= _computeSequenceNodes(parent);
     if (sequenceNodes == null) {
       return null;
     }
@@ -809,6 +810,8 @@ class EditPlanner {
       return node.elements;
     } else if (node is ArgumentList) {
       return node.arguments;
+    } else if (node is FormalParameter) {
+      return node.metadata;
     } else if (node is FormalParameterList) {
       return node.parameters;
     } else if (node is VariableDeclarationList) {
@@ -1525,7 +1528,8 @@ class _PassThroughBuilderImpl implements PassThroughBuilder {
       AstNode parent, List<AstNode> childNodes) {
     if (parent is Block ||
         parent is ClassDeclaration ||
-        parent is CompilationUnit) {
+        parent is CompilationUnit ||
+        parent is FormalParameter) {
       // These parent types don't use separators.
       return null;
     } else {

pkg/nnbd_migration/lib/src/fix_aggregator.dart

@@ -666,6 +666,14 @@ class NodeChangeForDefaultFormalParameter
   /// contained in the edit.
   AtomicEditInfo addRequiredKeywordInfo;
 
+  /// If non-null, indicates a `@required` annotation which should be removed
+  /// from this node.
+  Annotation annotationToRemove;
+
+  /// If [annotationToRemove] is non-null, the information that should be
+  /// contained in the edit.
+  AtomicEditInfo removeAnnotationInfo;
+
   NodeChangeForDefaultFormalParameter() : super._();
 
   @override
@@ -676,8 +684,17 @@ class NodeChangeForDefaultFormalParameter
   EditPlan _apply(DefaultFormalParameter node, FixAggregator aggregator) {
     var innerPlan = aggregator.innerPlanForNode(node);
     if (!addRequiredKeyword) return innerPlan;
-    return aggregator.planner.surround(innerPlan,
-        prefix: [AtomicEdit.insert('required ', info: addRequiredKeywordInfo)]);
+
+    var offset = node.firstTokenAfterCommentAndMetadata.offset;
+    return aggregator.planner.passThrough(node, innerPlans: [
+      aggregator.planner.insertText(node, offset, [
+        AtomicEdit.insert('required ', info: addRequiredKeywordInfo),
+      ]),
+      if (annotationToRemove != null)
+        aggregator.planner
+            .removeNode(annotationToRemove, info: removeAnnotationInfo),
+      ...aggregator.innerPlansForNode(node),
+    ]);
   }
 }
 

pkg/nnbd_migration/lib/src/fix_builder.dart

@@ -1145,20 +1145,33 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
             cls.name, method.name, element.name),
         {FixReasonTarget.root: node});
     var metadata = parameter.metadata;
-    for (var annotation in metadata) {
-      if (annotation.elementAnnotation.isRequired) {
-        // TODO(paulberry): what if `@required` isn't the first annotation?
-        // Will we produce something that isn't grammatical?
-        (_fixBuilder._getChange(annotation) as NodeChangeForAnnotation)
+    if (metadata != null && metadata.isNotEmpty) {
+      // Only the last annotation can be changed into a `required` keyword;
+      // changing an earlier annotation into a keyword would be illegal.
+      var lastAnnotation = metadata.last;
+      if (lastAnnotation.elementAnnotation.isRequired) {
+        (_fixBuilder._getChange(lastAnnotation) as NodeChangeForAnnotation)
           ..changeToRequiredKeyword = true
           ..changeToRequiredKeywordInfo = info;
         return;
       }
     }
     // Otherwise create a new `required` keyword.
-    (_fixBuilder._getChange(parameter) as NodeChangeForDefaultFormalParameter)
+    var nodeChange = (_fixBuilder._getChange(parameter)
+        as NodeChangeForDefaultFormalParameter)
       ..addRequiredKeyword = true
       ..addRequiredKeywordInfo = info;
+    var requiredAnnotation = metadata?.firstWhere(
+        (annotation) => annotation.elementAnnotation.isRequired,
+        orElse: () => null);
+    if (requiredAnnotation != null) {
+      // If the parameter was annotated with `@required`, but it was not the
+      // last annotation, we remove the annotation in addition to adding the
+      // `required` keyword.
+      nodeChange
+        ..annotationToRemove = requiredAnnotation
+        ..removeAnnotationInfo = info;
+    }
   }
 
   void _makeTypeNameNullable(TypeAnnotation node, DecoratedType decoratedType) {

pkg/nnbd_migration/lib/src/node_builder.dart

@@ -2,7 +2,6 @@
 // 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 'package:_fe_analyzer_shared/src/scanner/token.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/visitor.dart';
 import 'package:analyzer/dart/element/element.dart';
@@ -894,40 +893,3 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
     throw UnimplementedError(buffer.toString());
   }
 }
-
-extension on FormalParameter {
-  // TODO(srawlins): Add this to FormalParameter interface.
-  Token get firstTokenAfterCommentAndMetadata {
-    var parameter = this is DefaultFormalParameter
-        ? (this as DefaultFormalParameter).parameter
-        : this as NormalFormalParameter;
-    if (parameter is FieldFormalParameter) {
-      if (parameter.keyword != null) {
-        return parameter.keyword;
-      } else if (parameter.type != null) {
-        return parameter.type.beginToken;
-      } else {
-        return parameter.thisKeyword;
-      }
-    } else if (parameter is FunctionTypedFormalParameter) {
-      if (parameter.covariantKeyword != null) {
-        return parameter.covariantKeyword;
-      } else if (parameter.returnType != null) {
-        return parameter.returnType.beginToken;
-      } else {
-        return parameter.identifier.token;
-      }
-    } else if (parameter is SimpleFormalParameter) {
-      if (parameter.covariantKeyword != null) {
-        return parameter.covariantKeyword;
-      } else if (parameter.keyword != null) {
-        return parameter.keyword;
-      } else if (parameter.type != null) {
-        return parameter.type.beginToken;
-      } else {
-        return parameter.identifier.token;
-      }
-    }
-    return null;
-  }
-}

pkg/nnbd_migration/lib/src/utilities/hint_utils.dart

@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:_fe_analyzer_shared/src/scanner/token.dart';
+import 'package:analyzer/dart/ast/ast.dart';
 import 'package:nnbd_migration/src/edit_plan.dart';
 
 /// Determines if the given [token] is followed by a nullability hint, and if
@@ -226,3 +227,40 @@ enum HintCommentKind {
   /// required.
   required,
 }
+
+extension FormalParameterExtensions on FormalParameter {
+  // TODO(srawlins): Add this to FormalParameter interface.
+  Token get firstTokenAfterCommentAndMetadata {
+    var parameter = this is DefaultFormalParameter
+        ? (this as DefaultFormalParameter).parameter
+        : this as NormalFormalParameter;
+    if (parameter is FieldFormalParameter) {
+      if (parameter.keyword != null) {
+        return parameter.keyword;
+      } else if (parameter.type != null) {
+        return parameter.type.beginToken;
+      } else {
+        return parameter.thisKeyword;
+      }
+    } else if (parameter is FunctionTypedFormalParameter) {
+      if (parameter.covariantKeyword != null) {
+        return parameter.covariantKeyword;
+      } else if (parameter.returnType != null) {
+        return parameter.returnType.beginToken;
+      } else {
+        return parameter.identifier.token;
+      }
+    } else if (parameter is SimpleFormalParameter) {
+      if (parameter.covariantKeyword != null) {
+        return parameter.covariantKeyword;
+      } else if (parameter.keyword != null) {
+        return parameter.keyword;
+      } else if (parameter.type != null) {
+        return parameter.type.beginToken;
+      } else {
+        return parameter.identifier.token;
+      }
+    }
+    return null;
+  }
+}

pkg/nnbd_migration/test/fix_aggregator_test.dart

@@ -244,6 +244,60 @@ main() {}
     expect(previewInfo.applyTo(code), 'f({required int x}) => 0;');
   }
 
+  Future<void> test_addRequired_afterMetadata() async {
+    await analyze('f({@deprecated int x}) => 0;');
+    var previewInfo = run({
+      findNode.defaultParameter('int x'): NodeChangeForDefaultFormalParameter()
+        ..addRequiredKeyword = true
+    });
+    expect(previewInfo.applyTo(code), 'f({@deprecated required int x}) => 0;');
+  }
+
+  Future<void> test_addRequired_afterMetadata_andRequiredAnnotation() async {
+    addMetaPackage();
+    var content = '''
+import 'package:meta/meta.dart';
+f({@required @deprecated int x}) {}
+''';
+    await analyze(content);
+    var annotation = findNode.annotation('required');
+    var previewInfo = run({
+      findNode.defaultParameter('int x'): NodeChangeForDefaultFormalParameter()
+        ..addRequiredKeyword = true
+        ..annotationToRemove = annotation
+    });
+    expect(previewInfo.applyTo(code), '''
+import 'package:meta/meta.dart';
+f({@deprecated required int x}) {}
+''');
+    expect(previewInfo.values, hasLength(2));
+
+    expect(previewInfo[content.indexOf('int ')], hasLength(1));
+    expect(previewInfo[content.indexOf('int ')].single.isInsertion, true);
+    expect(previewInfo[content.indexOf('@required')], isNotNull);
+    expect(previewInfo[content.indexOf('@required')].single.isDeletion, true);
+  }
+
+  Future<void> test_addRequired_afterMetadata_beforeFinal() async {
+    await analyze('f({@deprecated final int x}) => 0;');
+    var previewInfo = run({
+      findNode.defaultParameter('int x'): NodeChangeForDefaultFormalParameter()
+        ..addRequiredKeyword = true
+    });
+    expect(previewInfo.applyTo(code),
+        'f({@deprecated required final int x}) => 0;');
+  }
+
+  Future<void> test_addRequired_afterMetadata_beforeFunctionTyped() async {
+    await analyze('f({@deprecated int x()}) => 0;');
+    var previewInfo = run({
+      findNode.defaultParameter('int x'): NodeChangeForDefaultFormalParameter()
+        ..addRequiredKeyword = true
+    });
+    expect(
+        previewInfo.applyTo(code), 'f({@deprecated required int x()}) => 0;');
+  }
+
   Future<void> test_addShownName_atEnd_multiple() async {
     await analyze("import 'dart:math' show cos;");
     var previewInfo = run({
@@ -1728,6 +1782,24 @@ int f() => null;
     expect(previewInfo.applyTo(code), 'f(x) => x.y;');
   }
 
+  Future<void>
+      test_requiredAnnotationToRequiredKeyword_leadingAnnotations() async {
+    addMetaPackage();
+    await analyze('''
+import 'package:meta/meta.dart';
+f({@deprecated @required int x}) {}
+''');
+    var annotation = findNode.annotation('required');
+    var previewInfo = run({
+      annotation: NodeChangeForAnnotation()..changeToRequiredKeyword = true
+    });
+    expect(previewInfo.applyTo(code), '''
+import 'package:meta/meta.dart';
+f({@deprecated required int x}) {}
+''');
+    expect(previewInfo.values.single.single.isDeletion, true);
+  }
+
   Future<void> test_requiredAnnotationToRequiredKeyword_prefixed() async {
     addMetaPackage();
     await analyze('''