Migration: accept prefix /*required*/ hint when present.

Previously, such a hint would influence the nullability graph, which
would usually cause the migration tool to decide that a `required`
keyword was necessary.  However, this didn't always work (for example
if the type was nullable, the hint would be ignored).  Also, even when
the hint was accepted, the tool would insert a `required` keyword and
leave the hint; now it converts the hint into the keyword by dropping
the `/*` and `*/`.

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

pkg/nnbd_migration/lib/src/edit_plan.dart

@@ -185,8 +185,9 @@ class EditPlanner {
   EditPlanner(this.lineInfo, this.sourceText, {this.removeViaComments = false});
 
   /// Creates a new edit plan that consists of executing [innerPlan], and then
-  /// converting the late [hint] into an explicit `late`.
-  NodeProducingEditPlan acceptLateHint(
+  /// converting the [hint] (which precedes the node) into text that is not
+  /// commented out.
+  NodeProducingEditPlan acceptPrefixHint(
       NodeProducingEditPlan innerPlan, HintComment hint,
       {AtomicEditInfo info}) {
     var affixPlan = innerPlan is _CommentAffixPlan
@@ -200,8 +201,9 @@ class EditPlanner {
   }
 
   /// Creates a new edit plan that consists of executing [innerPlan], and then
-  /// converting the nullability [hint] into an explicit `?` or `!`.
-  NodeProducingEditPlan acceptNullabilityOrNullCheckHint(
+  /// converting the [hint] (which follows the node) into text that is not
+  /// commented out.
+  NodeProducingEditPlan acceptSuffixHint(
       NodeProducingEditPlan innerPlan, HintComment hint,
       {AtomicEditInfo info}) {
     var affixPlan = innerPlan is _CommentAffixPlan

pkg/nnbd_migration/lib/src/fix_aggregator.dart

@@ -746,6 +746,10 @@ class NodeChangeForDefaultFormalParameter
   /// contained in the edit.
   AtomicEditInfo addRequiredKeywordInfo;
 
+  /// If [addRequiredKeyword] is `true`, and there is a `/*required*/` hint,
+  /// the hint comment that should be converted into a simple `required` string.
+  HintComment requiredHint;
+
   /// If non-null, indicates a `@required` annotation which should be removed
   /// from this node.
   Annotation annotationToRemove;
@@ -762,8 +766,13 @@ class NodeChangeForDefaultFormalParameter
 
   @override
   EditPlan _apply(DefaultFormalParameter node, FixAggregator aggregator) {
-    var innerPlan = aggregator.innerPlanForNode(node);
-    if (!addRequiredKeyword) return innerPlan;
+    if (!addRequiredKeyword) return aggregator.innerPlanForNode(node);
+
+    if (requiredHint != null) {
+      var innerPlan = aggregator.innerPlanForNode(node);
+      return aggregator.planner.acceptPrefixHint(innerPlan, requiredHint,
+          info: addRequiredKeywordInfo);
+    }
 
     var offset = node.firstTokenAfterCommentAndMetadata.offset;
     return aggregator.planner.passThrough(node, innerPlans: [
@@ -1205,8 +1214,7 @@ abstract class NodeChangeForType<N extends AstNode> extends NodeChange<N> {
     if (_makeNullable) {
       var hint = _nullabilityHint;
       if (hint != null) {
-        return aggregator.planner.acceptNullabilityOrNullCheckHint(
-            innerPlan, hint,
+        return aggregator.planner.acceptSuffixHint(innerPlan, hint,
             info: AtomicEditInfo(
                 NullabilityFixDescription.makeTypeNullableDueToHint(typeName),
                 fixReasons,
@@ -1302,7 +1310,7 @@ class NodeChangeForVariableDeclarationList
       var description = lateHint.kind == HintCommentKind.late_
           ? NullabilityFixDescription.addLateDueToHint
           : NullabilityFixDescription.addLateFinalDueToHint;
-      plan = aggregator.planner.acceptLateHint(plan, lateHint,
+      plan = aggregator.planner.acceptPrefixHint(plan, lateHint,
           info: AtomicEditInfo(description, {}, hintComment: lateHint));
     }
     return plan;
@@ -1348,8 +1356,7 @@ class NullCheckChange extends ExpressionChange {
   NodeProducingEditPlan applyExpression(FixAggregator aggregator,
       NodeProducingEditPlan innerPlan, AtomicEditInfo info) {
     if (hint != null) {
-      return aggregator.planner
-          .acceptNullabilityOrNullCheckHint(innerPlan, hint, info: info);
+      return aggregator.planner.acceptSuffixHint(innerPlan, hint, info: info);
     } else {
       return aggregator.planner
           .addUnaryPostfix(innerPlan, TokenType.BANG, info: info);

pkg/nnbd_migration/lib/src/fix_builder.dart

@@ -1113,17 +1113,20 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
   void visitDefaultFormalParameter(DefaultFormalParameter node) {
     var element = node.declaredElement;
     if (node.defaultValue == null) {
+      var requiredHint =
+          _fixBuilder._variables.getRequiredHint(_fixBuilder.source, node);
       var nullabilityNode =
           _fixBuilder._variables.decoratedElementType(element).node;
       if (!nullabilityNode.isNullable) {
         if (element.isNamed) {
-          _addRequiredKeyword(node, nullabilityNode);
+          _addRequiredKeyword(node, nullabilityNode, requiredHint);
         } else {
           _fixBuilder._addProblem(
               node, const NonNullableUnnamedOptionalParameter());
         }
-      } else if (element.metadata.any((m) => m.isRequired)) {
-        _addRequiredKeyword(node, nullabilityNode);
+      } else if (requiredHint != null ||
+          element.metadata.any((m) => m.isRequired)) {
+        _addRequiredKeyword(node, nullabilityNode, requiredHint);
       }
     }
     super.visitDefaultFormalParameter(node);
@@ -1199,8 +1202,8 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
     super.visitTypeName(node);
   }
 
-  void _addRequiredKeyword(
-      DefaultFormalParameter parameter, NullabilityNode node) {
+  void _addRequiredKeyword(DefaultFormalParameter parameter,
+      NullabilityNode node, HintComment requiredHint) {
     // Change an existing `@required` annotation into a `required` keyword if
     // possible.
     final element = parameter.declaredElement;
@@ -1226,7 +1229,8 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
     var nodeChange = (_fixBuilder._getChange(parameter)
         as NodeChangeForDefaultFormalParameter)
       ..addRequiredKeyword = true
-      ..addRequiredKeywordInfo = info;
+      ..addRequiredKeywordInfo = info
+      ..requiredHint = requiredHint;
     var requiredAnnotation = metadata?.firstWhere(
         (annotation) => annotation.elementAnnotation.isRequired,
         orElse: () => null);

pkg/nnbd_migration/test/api_test.dart

@@ -124,6 +124,32 @@ abstract class _ProvisionalApiTestBase extends AbstractContextTest {
 
 /// Mixin containing test cases for the provisional API.
 mixin _ProvisionalApiTestCases on _ProvisionalApiTestBase {
+  Future<void> test_accept_required_hint() async {
+    var content = '''
+f({/*required*/ int i}) {}
+''';
+    var expected = '''
+f({required int i}) {}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
+  Future<void> test_accept_required_hint_nullable() async {
+    var content = '''
+f({/*required*/ int i}) {}
+g() {
+  f(i: null);
+}
+''';
+    var expected = '''
+f({required int? i}) {}
+g() {
+  f(i: null);
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
   Future<void> test_add_explicit_parameter_type() async {
     var content = '''
 abstract class C {

pkg/nnbd_migration/test/edit_plan_test.dart

@@ -64,7 +64,7 @@ class EditPlanTest extends AbstractSingleUnitTest {
     await analyze(code);
     var hint = getPrefixHint(findNode.simple('int').token);
     var changes = checkPlan(
-        planner.acceptLateHint(
+        planner.acceptPrefixHint(
             planner.passThrough(findNode.simple('int')), hint),
         'late int x = 0;');
     expect(changes.keys, unorderedEquals([0, 7]));
@@ -77,7 +77,7 @@ class EditPlanTest extends AbstractSingleUnitTest {
     await analyze(code);
     var hint = getPrefixHint(findNode.simple('int').token);
     checkPlan(
-        planner.acceptLateHint(
+        planner.acceptPrefixHint(
             planner.passThrough(findNode.simple('int')), hint),
         'late int x = 0;');
   }
@@ -87,7 +87,7 @@ class EditPlanTest extends AbstractSingleUnitTest {
     await analyze(code);
     var hint = getPrefixHint(findNode.simple('int').token);
     checkPlan(
-        planner.acceptLateHint(
+        planner.acceptPrefixHint(
             planner.passThrough(findNode.simple('int')), hint),
         '@deprecated late int x = 0;');
   }
@@ -103,7 +103,7 @@ class C {
     var parameter = findNode.fieldFormalParameter('void this.f(int i)');
     var typeName = planner.passThrough(parameter);
     checkPlan(
-        planner.acceptNullabilityOrNullCheckHint(
+        planner.acceptSuffixHint(
             typeName, getPostfixHint(parameter.parameters.rightParenthesis)),
         '''
 class C {
@@ -118,7 +118,7 @@ class C {
     var parameter = findNode.functionTypedFormalParameter('void g(int i)');
     var typeName = planner.passThrough(parameter);
     checkPlan(
-        planner.acceptNullabilityOrNullCheckHint(
+        planner.acceptSuffixHint(
             typeName, getPostfixHint(parameter.parameters.rightParenthesis)),
         'f(void g(int i)?) {}');
   }
@@ -128,9 +128,7 @@ class C {
     await analyze(code);
     var intRef = findNode.simple('int');
     var typeName = planner.passThrough(intRef);
-    checkPlan(
-        planner.acceptNullabilityOrNullCheckHint(
-            typeName, getPostfixHint(intRef.token)),
+    checkPlan(planner.acceptSuffixHint(typeName, getPostfixHint(intRef.token)),
         'int? x = 0;');
   }
 
@@ -141,7 +139,7 @@ class C {
     checkPlan(
         planner.extract(
             xRef.parent.parent,
-            planner.acceptNullabilityOrNullCheckHint(
+            planner.acceptSuffixHint(
                 planner.passThrough(xRef), getPostfixHint(xRef.token))),
         'f(x) => x!;');
   }