Migration: add explicit parameter types to function literals when necessary.

Under NNBD rules, certain function literals have their types inferred
as `Object?` instead of `dynamic`.  We need to add explicit `dynamic`
to prevent follow-on errors if those parameters are used in a dynamic
fashion.

Change-Id: I12bfd3a3da2bda1ae11b01a5d029dbea3d512cb6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134567
Reviewed-by: Mike Fairhurst <mfairhurst@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>

pkg/analyzer/lib/src/generated/migration.dart

@@ -25,6 +25,12 @@ abstract class MigrationResolutionHooks
   /// applied.
   DartType modifyExpressionType(Expression expression, DartType dartType);
 
+  /// Called after the resolver has inferred the type of a function literal
+  /// parameter.  Should return the type that the parameter has after migrations
+  /// have been applied.
+  DartType modifyInferredParameterType(
+      ParameterElement parameter, DartType type);
+
   /// Called when the resolver starts or stops making use of a [FlowAnalysis]
   /// instance.
   void setFlowAnalysis(

pkg/analyzer/lib/src/generated/static_type_analyzer.dart

@@ -164,6 +164,10 @@ class StaticTypeAnalyzer extends SimpleAstVisitor<void> {
           if (inferredType.isDartCoreNull) {
             inferredType = _typeProvider.objectType;
           }
+          if (_migrationResolutionHooks != null) {
+            inferredType = _migrationResolutionHooks
+                .modifyInferredParameterType(p, inferredType);
+          }
           if (!inferredType.isDynamic) {
             p.type = inferredType;
             inferred = true;

pkg/nnbd_migration/lib/src/fix_aggregator.dart

@@ -397,6 +397,28 @@ class NodeChangeForPropertyAccess
   }
 }
 
+/// Implementation of [NodeChange] specialized for operating on
+/// [SimpleFormalParameter] nodes.
+class NodeChangeForSimpleFormalParameter
+    extends NodeChange<SimpleFormalParameter> {
+  /// If not `null`, an explicit type annotation that should be added to the
+  /// parameter.
+  DartType addExplicitType;
+
+  NodeChangeForSimpleFormalParameter() : super._();
+
+  @override
+  EditPlan _apply(SimpleFormalParameter node, FixAggregator aggregator) {
+    var innerPlan = aggregator.innerPlanForNode(node);
+    if (addExplicitType == null) return innerPlan;
+    return aggregator.planner.surround(innerPlan, prefix: [
+      AtomicEdit.insert(
+          addExplicitType.getDisplayString(withNullability: true)),
+      AtomicEdit.insert(' ')
+    ]);
+  }
+}
+
 /// Implementation of [NodeChange] specialized for operating on [TypeAnnotation]
 /// nodes.
 class NodeChangeForTypeAnnotation extends NodeChange<TypeAnnotation> {
@@ -491,6 +513,10 @@ class _NodeChangeVisitor extends GeneralizingAstVisitor<NodeChange<AstNode>> {
   NodeChange visitPropertyAccess(PropertyAccess node) =>
       NodeChangeForPropertyAccess();
 
+  @override
+  NodeChange visitSimpleFormalParameter(SimpleFormalParameter node) =>
+      NodeChangeForSimpleFormalParameter();
+
   @override
   NodeChange visitTypeName(TypeName node) => NodeChangeForTypeAnnotation();
 

pkg/nnbd_migration/lib/src/fix_builder.dart

@@ -93,6 +93,10 @@ class FixBuilder {
 
   final MigrationResolutionHooksImpl migrationResolutionHooks;
 
+  /// Parameter elements for which an explicit type should be added, and what
+  /// that type should be.
+  final Map<ParameterElement, DartType> _addedParameterTypes = {};
+
   factory FixBuilder(
       Source source,
       DecoratedClassHierarchy decoratedClassHierarchy,
@@ -401,6 +405,18 @@ class MigrationResolutionHooksImpl implements MigrationResolutionHooks {
         return type;
       });
 
+  @override
+  DartType modifyInferredParameterType(
+      ParameterElement parameter, DartType type) {
+    var postMigrationType = parameter.type;
+    if (postMigrationType != type) {
+      // TODO(paulberry): make sure we test all kinds of parameters
+      _fixBuilder._addedParameterTypes[parameter] = postMigrationType;
+      return postMigrationType;
+    }
+    return type;
+  }
+
   @override
   void setFlowAnalysis(
       FlowAnalysis<AstNode, Statement, Expression, PromotableElement, DartType>
@@ -564,6 +580,17 @@ class _FixBuilderPostVisitor extends GeneralizingAstVisitor<void>
     }
   }
 
+  @override
+  void visitSimpleFormalParameter(SimpleFormalParameter node) {
+    if (node.type == null) {
+      var typeToAdd = _fixBuilder._addedParameterTypes[node.declaredElement];
+      if (typeToAdd != null) {
+        (_fixBuilder._getChange(node) as NodeChangeForSimpleFormalParameter)
+            .addExplicitType = typeToAdd;
+      }
+    }
+  }
+
   @override
   void visitVariableDeclarationList(VariableDeclarationList node) {
     if (node.type == null) {

pkg/nnbd_migration/test/api_test.dart

@@ -94,6 +94,101 @@ abstract class _ProvisionalApiTestBase extends AbstractContextTest {
 
 /// Mixin containing test cases for the provisional API.
 mixin _ProvisionalApiTestCases on _ProvisionalApiTestBase {
+  Future<void> test_add_explicit_parameter_type() async {
+    var content = '''
+abstract class C {
+  void m<T>(T Function(T) callback);
+}
+void test(C c) {
+  c.m((value) => value + 1);
+}
+''';
+    // Under the new NNBD rules, `value` gets an inferred type of `Object?`.  We
+    // need to change this to `dynamic` to avoid an "undefined operator +"
+    // error.
+    var expected = '''
+abstract class C {
+  void m<T>(T Function(T) callback);
+}
+void test(C c) {
+  c.m((dynamic value) => value + 1);
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
+  @FailingTest(issue: 'https://github.com/dart-lang/sdk/issues/40476')
+  Future<void> test_add_explicit_parameter_type_interpolation() async {
+    var content = r'''
+abstract class C {
+  void m<T>(T Function(T) callback);
+}
+void test(C c) {
+  c.m((value) => '$value';
+}
+''';
+    // Under the new NNBD rules, `value` gets an inferred type of `Object?`,
+    // whereas it previously had a type of `dynamic`.  But we don't need to fix
+    // it because `Object?` supports `toString`.
+    var expected = r'''
+abstract class C {
+  void m<T>(T Function(T) callback);
+}
+void test(C c) {
+  c.m((value) => '$value';
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
+  @FailingTest(issue: 'https://github.com/dart-lang/sdk/issues/40476')
+  Future<void> test_add_explicit_parameter_type_object_method() async {
+    var content = '''
+abstract class C {
+  void m<T>(T Function(T) callback);
+}
+void test(C c) {
+  c.m((value) => value.toString());
+}
+''';
+    // Under the new NNBD rules, `value` gets an inferred type of `Object?`,
+    // whereas it previously had a type of `dynamic`.  But we don't need to fix
+    // it because `Object?` supports `toString`.
+    var expected = '''
+abstract class C {
+  void m<T>(T Function(T) callback);
+}
+void test(C c) {
+  c.m((value) => value.toString());
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
+  @FailingTest(issue: 'https://github.com/dart-lang/sdk/issues/40476')
+  Future<void> test_add_explicit_parameter_type_unused() async {
+    var content = '''
+abstract class C {
+  void m<T>(T Function(T) callback);
+}
+void test(C c) {
+  c.m((value) => 0);
+}
+''';
+    // Under the new NNBD rules, `value` gets an inferred type of `Object?`,
+    // whereas it previously had a type of `dynamic`.  But we don't need to fix
+    // it because it's unused.
+    var expected = '''
+abstract class C {
+  void m<T>(T Function(T) callback);
+}
+void test(C c) {
+  c.m((value) => 0);
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
   Future<void> test_add_required() async {
     var content = '''
 int f({String s}) => s.length;