Add nullability edge for public functions/methods' arguments

That forces them to be nullable, unless there is a
reason to be non-nullable.

I excluded setters and generic arguments from this
nullification. Also we don't touch `operator ==`, where
changing argument type to `Object?` doesn't make sense.

This is a response to some teams asking for the migration
tool to be super conservative. Otherwise people migrate
some methods to have non-nullable arguments and they are
still called with `null`s at runtime in mixed mode.

That was the only way I found to implement the "nullable
arguments by default" request.

It broke a good deal of tests. I updated them mostly to use
private functions, so the old behavior is preserved, but
sometimes I couldn't figure out a way to trigger the desired
codepath with the new behavior... I also suspect that now
there is a non-zero amount of tests that are passing but
don't really exercise what they were supposed to exercise.

Change-Id: I2eb8020008c6cd5c7507e7782e977d70bceac58b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/285680
Commit-Queue: Ilya Yanok <yanok@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>

pkg/nnbd_migration/lib/instrumentation.dart

@@ -290,6 +290,7 @@ enum EdgeOriginKind {
   nullAwareAccess,
   optionalFormalParameter,
   parameterInheritance,
+  publicMethodArgument,
   quiverCheckNotNull,
   returnTypeInheritance,
   stackTraceTypeOrigin,

pkg/nnbd_migration/lib/src/edge_builder.dart

@@ -1038,6 +1038,8 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
                 node, null, getter.declaration, declaredElement);
           }
         }
+      } else if (declaredElement != null && declaredElement.isPublic) {
+        _makeArgumentsNullable(declaredElement, node);
       }
     }
     return null;
@@ -2684,6 +2686,16 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
     _dispatch(returnType);
     _createFlowAnalysis(node, parameters);
     _dispatch(parameters);
+
+    // Be over conservative with public methods' arguments:
+    // Unless we have reasons for non-nullability, assume they are nullable.
+    // Soft edge to `always` node does exactly this.
+    if (declaredElement.isPublic &&
+        declaredElement is! PropertyAccessorElement &&
+        // operator == treats `null` specially.
+        !(declaredElement.isOperator && declaredElement.name == '==')) {
+      _makeArgumentsNullable(declaredElement, node);
+    }
     _currentFunctionType = _variables.decoratedElementType(declaredElement);
     _currentFieldFormals = declaredElement is ConstructorElement
         ? _computeFieldFormalMap(declaredElement)
@@ -3402,6 +3414,20 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
     }
   }
 
+  void _makeArgumentsNullable(
+      ExecutableElement declaredElement, Declaration node) {
+    for (final p in declaredElement.parameters) {
+      if (p is! FieldFormalParameterElement &&
+          p is! SuperFormalParameterElement &&
+          p is! ConstVariableElement) {
+        final decoratedType = _variables.decoratedElementType(p);
+        if (decoratedType.type is TypeParameterType) continue;
+        _graph.makeNullable(
+            decoratedType.node, PublicMethodArgumentOrigin(source, node));
+      }
+    }
+  }
+
   EdgeOrigin _makeEdgeOrigin(DecoratedType sourceType, Expression expression,
       {bool isSetupAssignment = false}) {
     if (sourceType.type!.isDynamic) {

pkg/nnbd_migration/lib/src/edge_origin.dart

@@ -650,6 +650,17 @@ class ParameterInheritanceOrigin extends EdgeOrigin {
   EdgeOriginKind get kind => EdgeOriginKind.parameterInheritance;
 }
 
+/// Edge origin arising from arguments of public methods.
+class PublicMethodArgumentOrigin extends EdgeOrigin {
+  PublicMethodArgumentOrigin(super.source, AstNode super.node);
+
+  @override
+  String get description => 'public method argument';
+
+  @override
+  EdgeOriginKind get kind => EdgeOriginKind.publicMethodArgument;
+}
+
 /// Edge origin resulting from the presence of a call to quiver's
 /// `checkNotNull`.
 ///

pkg/nnbd_migration/test/edge_builder_flow_analysis_test.dart

@@ -1031,12 +1031,12 @@ bool g(int j) => j.isEven;
   Future<void>
       test_functionDeclaration_resets_unconditional_control_flow() async {
     await analyze('''
-void f(bool b, int i, int j) {
+void _f(bool b, int i, int j) {
   assert(i != null);
   if (b) return;
   assert(j != null);
 }
-void g(int k) {
+void _g(int k) {
   assert(k != null);
 }
 ''');

pkg/nnbd_migration/test/edge_builder_test.dart

@@ -3274,9 +3274,9 @@ void f(List<int> l) {
   Future<void> test_for_each_element_with_declaration_implicit_type() async {
     await analyze('''
 void f(List<int> l) {
-  [for (var i in l) g(i)];
+  [for (var i in l) _g(i)];
 }
-int g(int j) => 0;
+int _g(int j) => 0;
 ''');
     var jNode = decoratedTypeAnnotation('int j').node;
     var iMatcher = anyNode;
@@ -3355,10 +3355,10 @@ void f(List<int> l) {
     await analyze('''
 void f(List<int> l) {
   for (var i in l) {
-    g(i);
+    _g(i);
   }
 }
-void g(int j) {}
+void _g(int j) {}
 ''');
     var jNode = decoratedTypeAnnotation('int j').node;
     var iMatcher = anyNode;
@@ -3659,9 +3659,9 @@ void test(int/*2*/ i) {
 
   Future<void> test_functionInvocation_parameter_functionTyped() async {
     await analyze('''
-void f(void g()) {}
+void _f(void g()) {}
 void test() {
-  f(null);
+  _f(null);
 }
 ''');
 
@@ -3740,9 +3740,9 @@ void g() {
 
   Future<void> test_functionInvocation_parameter_null() async {
     await analyze('''
-void f(int i) {}
+void _f(int i) {}
 void test() {
-  f(null);
+  _f(null);
 }
 ''');
 
@@ -4758,12 +4758,12 @@ class C {
       test_methodDeclaration_resets_unconditional_control_flow() async {
     await analyze('''
 class C {
-  void f(bool b, int i, int j) {
+  void _f(bool b, int i, int j) {
     assert(i != null);
     if (b) return;
     assert(j != null);
   }
-  void g(int k) {
+  void _g(int k) {
     assert(k != null);
   }
 }
@@ -8073,7 +8073,11 @@ void f<T extends Foo>(T t) {
     assertEdge(decoratedTypeAnnotation('T t').node, never, hard: true);
     // TODO(mfairhurst): fix this: https://github.com/dart-lang/sdk/issues/39852
     //assertEdge(decoratedTypeAnnotation('Foo>').node, never, hard: true);
-    assertEdge(inSet(alwaysPlus), decoratedTypeAnnotation('int x').node,
+    // There is a direct soft edge from always, since it's a public method,
+    // but we want to test there is also an edge resulting from t.bar(null)
+    // usage in f.
+    assertEdge(inSet(alwaysPlus.difference({always})),
+        decoratedTypeAnnotation('int x').node,
         hard: false);
   }
 
@@ -8090,7 +8094,11 @@ void f<T extends R, R extends Foo>(T t) {
     assertEdge(decoratedTypeAnnotation('T t').node, never, hard: true);
     // TODO(mfairhurst): fix this: https://github.com/dart-lang/sdk/issues/39852
     //assertEdge(decoratedTypeAnnotation('Foo>').node, never, hard: true);
-    assertEdge(inSet(alwaysPlus), decoratedTypeAnnotation('int x').node,
+    // There is a direct soft edge from always, since it's a public method,
+    // but we want to test there is also an edge resulting from t.bar(null)
+    // usage in f.
+    assertEdge(inSet(alwaysPlus.difference({always})),
+        decoratedTypeAnnotation('int x').node,
         hard: false);
   }
 
@@ -8107,7 +8115,11 @@ void f<T extends Foo<dynamic>>(T t) {
     assertEdge(decoratedTypeAnnotation('T t').node, never, hard: true);
     // TODO(mfairhurst): fix this: https://github.com/dart-lang/sdk/issues/39852
     //assertEdge(decoratedTypeAnnotation('Foo>').node, never, hard: true);
-    assertEdge(inSet(alwaysPlus), decoratedTypeAnnotation('int x').node,
+    // There is a direct soft edge from always, since it's a public method,
+    // but we want to test there is also an edge resulting from t.bar(null)
+    // usage in f.
+    assertEdge(inSet(alwaysPlus.difference({always})),
+        decoratedTypeAnnotation('int x').node,
         hard: false);
   }
 

pkg/nnbd_migration/test/fix_builder_test.dart

@@ -443,7 +443,7 @@ class _C {
 }
 _f(_C c) => c['foo'] += 0;
 ''');
-    visitAssignmentTarget(findNode.index('c['), 'int', 'num');
+    visitAssignmentTarget(findNode.index('c['), 'int', 'num?');
   }
 
   Future<void>
@@ -455,7 +455,7 @@ class _C {
 }
 _f(_C/*?*/ c) => c['foo'] += 0;
 ''');
-    visitAssignmentTarget(findNode.index('c['), 'int', 'num',
+    visitAssignmentTarget(findNode.index('c['), 'int', 'num?',
         changes: {findNode.simple('c['): isNullCheck});
   }
 
@@ -468,7 +468,7 @@ class _C {
 }
 _f(_C c, String/*?*/ s) => c[s] += 0;
 ''');
-    visitAssignmentTarget(findNode.index('c['), 'int', 'num',
+    visitAssignmentTarget(findNode.index('c['), 'int', 'num?',
         changes: {findNode.simple('s]'): isNullCheck});
   }
 
@@ -524,7 +524,7 @@ class _C {
 }
 _f(_C c) => c['foo'] = 0;
 ''');
-    visitAssignmentTarget(findNode.index('c['), null, 'num');
+    visitAssignmentTarget(findNode.index('c['), null, 'num?');
   }
 
   Future<void> test_assignmentTarget_indexExpression_simple_check_lhs() async {
@@ -535,7 +535,7 @@ class _C {
 }
 _f(_C/*?*/ c) => c['foo'] = 0;
 ''');
-    visitAssignmentTarget(findNode.index('c['), null, 'num',
+    visitAssignmentTarget(findNode.index('c['), null, 'num?',
         changes: {findNode.simple('c['): isNullCheck});
   }
 
@@ -547,7 +547,7 @@ class _C {
 }
 _f(_C c, String/*?*/ s) => c[s] = 0;
 ''');
-    visitAssignmentTarget(findNode.index('c['), null, 'num',
+    visitAssignmentTarget(findNode.index('c['), null, 'num?',
         changes: {findNode.simple('s]'): isNullCheck});
   }
 
@@ -1685,7 +1685,7 @@ _f(int/*?*/ x) {
   }
 
   Future<void> test_implicit_downcast() async {
-    await analyze('int f(num x) => x;');
+    await analyze('int _f(num x) => x;');
     var xRef = findNode.simple('x;');
     visitSubexpression(xRef, 'int', changes: {
       xRef: isNodeChangeForExpression.havingIntroduceAsWithInfo(

pkg/nnbd_migration/test/front_end/info_builder_test.dart

@@ -580,9 +580,9 @@ void f(C/*!*/ a, int b) {
 }
 ''', migratedContent: '''
 abstract class C {
-  C/*?*/ operator+(int  i);
+  C/*?*/ operator+(int? i);
 }
-void f(C/*!*/ a, int  b) {
+void f(C/*!*/ a, int? b) {
   a += b;
 }
 ''');
@@ -839,7 +839,7 @@ void f(C/*!*/ a) {
 }
 ''', migratedContent: '''
 abstract class C {
-  C/*?*/ operator+(int  i);
+  C/*?*/ operator+(int? i);
 }
 void f(C/*!*/ a) {
   a++;
@@ -1025,14 +1025,14 @@ C/*!*/ _f(C c) => c + c;
 ''';
     var migratedContent = '''
 class C {
-  C? operator+(C  c) => null;
+  C? operator+(C? c) => null;
 }
 C/*!*/ _f(C  c) => (c + c)!;
 ''';
     var unit = await buildInfoForSingleTestFile(originalContent,
         migratedContent: migratedContent);
     var regions = unit.fixRegions;
-    expect(regions, hasLength(3));
+    expect(regions, hasLength(4));
     assertRegion(
         region: regions[0],
         offset: migratedContent.indexOf('? operator'),
@@ -1040,12 +1040,17 @@ C/*!*/ _f(C  c) => (c + c)!;
         explanation: "Changed type 'C' to be nullable");
     assertRegion(
         region: regions[1],
+        offset: migratedContent.indexOf('? c'),
+        length: 1,
+        explanation: "Changed type 'C' to be nullable");
+    assertRegion(
+        region: regions[2],
         offset: migratedContent.indexOf('/*!*/'),
         length: 5,
         explanation: "Type 'C' was not made nullable due to a hint",
         kind: NullabilityFixKind.typeNotMadeNullableDueToHint);
     assertRegion(
-        region: regions[2],
+        region: regions[3],
         offset: migratedContent.indexOf('!;'),
         length: 1,
         explanation: 'Added a non-null assertion to nullable expression',
@@ -1076,7 +1081,7 @@ int? f(List<int >  values, int/*?*/ x)
     var unit = await buildInfoForSingleTestFile('''
 int f(int/*!*/ x, int y) => x ??= y;
 ''', migratedContent: '''
-int  f(int/*!*/ x, int  y) => x ??= y;
+int  f(int/*!*/ x, int? y) => x ??= y;
 ''', warnOnWeakCode: false, removeViaComments: false);
     var codeToRemove = ' ??= y';
     var removalOffset = unit.content!.indexOf(codeToRemove);
@@ -1096,7 +1101,7 @@ int  f(int/*!*/ x, int  y) => x ??= y;
     var unit = await buildInfoForSingleTestFile('''
 int f(int/*!*/ x, int y) => x ??= y;
 ''', migratedContent: '''
-int  f(int/*!*/ x, int  y) => x ??= y;
+int  f(int/*!*/ x, int? y) => x ??= y!;
 ''', warnOnWeakCode: true);
     var operator = '??=';
     var operatorOffset = unit.content!.indexOf(operator);
@@ -1235,7 +1240,7 @@ void f(num n, int/*?*/ i) {
     // Note: even though `as int` is removed, it still shows up in the
     // preview, since we show deleted text.
     var migratedContent = '''
-void f(num  n, int/*?*/ i) {
+void f(num? n, int/*?*/ i) {
   if (n is! int ) return;
   print((n as int).isEven);
   print(i! + 1);
@@ -1244,17 +1249,17 @@ void f(num  n, int/*?*/ i) {
     var unit = await buildInfoForSingleTestFile(originalContent,
         migratedContent: migratedContent, removeViaComments: false);
     var regions = unit.fixRegions;
-    expect(regions, hasLength(4));
-    assertRegionPair(regions, 0,
+    expect(regions, hasLength(5));
+    assertRegionPair(regions, 1,
         kind: NullabilityFixKind.makeTypeNullableDueToHint);
     assertRegion(
-        region: regions[2],
+        region: regions[3],
         offset: migratedContent.indexOf(' as int'),
         length: ' as int'.length,
         explanation: 'Discarded a downcast that is now unnecessary',
         kind: NullabilityFixKind.removeAs);
     assertRegion(
-        region: regions[3],
+        region: regions[4],
         offset: migratedContent.indexOf('! + 1'),
         explanation: 'Added a non-null assertion to nullable expression',
         kind: NullabilityFixKind.checkExpression);
@@ -1287,7 +1292,7 @@ int f(Object o) {
 }
 ''';
     var migratedContent = '''
-int  f(Object  o) {
+int  f(Object? o) {
   if (o is! String ) return 0;
   return o /* no valid migration */;
 }
@@ -1428,51 +1433,55 @@ void f() {
 
   Future<void> test_trace_nullableType() async {
     var unit = await buildInfoForSingleTestFile('''
-void f(int i) {} // f
-void g(int i) { // g
-  f(i);
+void _f(int i) {} // _f
+void _g(int i) { // _g
+  _f(i);
 }
 void h() {
-  g(null);
+  _g(null);
 }
 ''', migratedContent: '''
-void f(int? i) {} // f
-void g(int? i) { // g
-  f(i);
+void _f(int? i) {} // _f
+void _g(int? i) { // _g
+  _f(i);
 }
 void h() {
-  g(null);
+  _g(null);
 }
 ''');
     var region = unit.regions
         .where((regionInfo) =>
-            regionInfo.offset == unit.content!.indexOf('? i) {} // f'))
+            regionInfo.offset == unit.content!.indexOf('? i) {} // _f'))
         .single;
     expect(region.traces, hasLength(1));
     var trace = region.traces.single;
     expect(trace.description, 'Nullability reason');
     var entries = trace.entries;
     expect(entries, hasLength(6));
-    // Entry 0 is the nullability of f's argument
-    assertTraceEntry(unit, entries[0], 'f',
-        unit.content!.indexOf('int? i) {} // f'), contains('parameter 0 of f'),
+    // Entry 0 is the nullability of _f's argument
+    assertTraceEntry(
+        unit,
+        entries[0],
+        '_f',
+        unit.content!.indexOf('int? i) {} // _f'),
+        contains('parameter 0 of _f'),
         hintActions: {
           HintActionKind.addNullableHint,
           HintActionKind.addNonNullableHint
         });
 
-    // Entry 1 is the edge from g's argument to f's argument, due to g's call to
-    // f.
+    // Entry 1 is the edge from _g's argument to _f's argument, due to g's call
+    // to _f.
     assertTraceEntry(
-        unit, entries[1], 'g', unit.content!.indexOf('i);'), 'data flow');
-    // Entry 2 is the nullability of g's argument
-    assertTraceEntry(unit, entries[2], 'g',
-        unit.content!.indexOf('int? i) { // g'), contains('parameter 0 of g'),
+        unit, entries[1], '_g', unit.content!.indexOf('i);'), 'data flow');
+    // Entry 2 is the nullability of _g's argument
+    assertTraceEntry(unit, entries[2], '_g',
+        unit.content!.indexOf('int? i) { // _g'), contains('parameter 0 of _g'),
         hintActions: {
           HintActionKind.addNullableHint,
           HintActionKind.addNonNullableHint
         });
-    // Entry 3 is the edge from null to g's argument, due to h's call to g.
+    // Entry 3 is the edge from null to _g's argument, due to h's call to _g.
     assertTraceEntry(
         unit, entries[3], 'h', unit.content!.indexOf('null'), 'data flow');
     // Entry 4 is the nullability of the null literal.

pkg/nnbd_migration/test/migration_cli_test.dart

@@ -956,18 +956,18 @@ import 'test.dart';
       projectContents['lib/test.dart'] = '''
 import 'skip.dart';
 import 'analyze_but_do_not_migrate.dart';
-void f(int x) {}
-void g(int${migrated ? '?' : ''} x) {}
-void h(int${migrated ? '?' : ''} x) {}
-void call_h() => h(null);
+int f() => 1;
+int${migrated ? '?' : ''} g() => 1;
+int${migrated ? '?' : ''} h() => 1;
+bool call_h() => h() == null;
 ''';
       projectContents['lib/skip.dart'] = '''
 import 'test.dart';
-void call_f() => f(null);
+bool call_f() => f() == null;
 ''';
       projectContents['lib/analyze_but_do_not_migrate.dart'] = '''
 import 'test.dart';
-void call_g() => g(null);
+bool call_g() => g() == null;
 ''';
       return projectContents;
     }
@@ -1337,7 +1337,7 @@ int f() => null;
   }
 
   test_lifecycle_preview_rerun_with_ignore_errors() async {
-    var origSourceText = 'void f(int i) {}';
+    var origSourceText = 'void _f(int i) {}';
     var projectContents = simpleProject(sourceText: origSourceText);
     var projectDir = createProjectDir(projectContents);
     var cli = _createCli();
@@ -1347,10 +1347,12 @@ int f() => null;
       var uri = Uri.parse(url);
       var testPath =
           resourceProvider.pathContext.join(projectDir, 'lib', 'test.dart');
-      resourceProvider.getFile(testPath).writeAsStringSync('void f(int? i) {}');
+      resourceProvider
+          .getFile(testPath)
+          .writeAsStringSync('void _f(int? i) {}');
       // We haven't rerun, so getting the file details from the server should
       // still yield the original source text, with informational space.
-      expect(await getSourceFromServer(uri, testPath), 'void f(int  i) {}');
+      expect(await getSourceFromServer(uri, testPath), 'void _f(int  i) {}');
       var response = await httpPost(uri.replace(path: 'rerun-migration'),
           headers: {'Content-Type': 'application/json; charset=UTF-8'});
       assertHttpSuccess(response);
@@ -1358,12 +1360,12 @@ int f() => null;
       expect(body['success'], isTrue);
       expect(body['errors'], isNull);
       // Now that we've rerun, the server should yield the new source text
-      expect(await getSourceFromServer(uri, testPath), 'void f(int?  i) {}');
+      expect(await getSourceFromServer(uri, testPath), 'void _f(int?  i) {}');
     });
   }
 
   test_lifecycle_preview_rerun_with_new_analysis_errors() async {
-    var origSourceText = 'void f(int i) {}';
+    var origSourceText = 'void _f(int i) {}';
     var projectContents = simpleProject(sourceText: origSourceText);
     var projectDir = createProjectDir(projectContents);
     var cli = _createCli();
@@ -1372,11 +1374,11 @@ int f() => null;
       var uri = Uri.parse(url);
       var testPath =
           resourceProvider.pathContext.join(projectDir, 'lib', 'test.dart');
-      var newSourceText = 'void f(int? i) {}';
+      var newSourceText = 'void _f(int? i) {}';
       resourceProvider.getFile(testPath).writeAsStringSync(newSourceText);
       // We haven't rerun, so getting the file details from the server should
       // still yield the original source text, with informational space.
-      expect(await getSourceFromServer(uri, testPath), 'void f(int  i) {}');
+      expect(await getSourceFromServer(uri, testPath), 'void _f(int  i) {}');
       var response = await httpPost(uri.replace(path: 'rerun-migration'),
           headers: {'Content-Type': 'application/json; charset=UTF-8'});
       assertHttpSuccess(response);
@@ -1390,7 +1392,7 @@ int f() => null;
           equals(
               "This requires the 'non-nullable' language feature to be enabled"));
       expect(error['location'],
-          equals(resourceProvider.pathContext.join('lib', 'test.dart:1:11')));
+          equals(resourceProvider.pathContext.join('lib', 'test.dart:1:12')));
       expect(error['code'], equals('experiment_not_enabled'));
     });
   }