From faa03308f6e601a2909f20668373c86abcdd3878 Mon Sep 17 00:00:00 2001
From: Paul Berry <paulberry@google.com>
Date: Fri, 24 Sep 2021 13:25:24 +0000
Subject: [PATCH] Migration: don't add `!`s to expressions of type `Null`.

Previously, we had a rule in the migration tool that we didn't add
`!`s to the literal `null`, because such a change would be guaranteed
to fail at runtime; instead we issued a warning to the user that there
is no valid migration for their code, so user intervention is
required.

This change extends the rule to apply to any expression whose type is
`Null`.  Notably, this makes migration of code using Mockito much
easier, because it prevents the migration tool from trying to add a
`!` to expressions like `any`, `anyNamed(...)`, `argThat(...)`,
`captureThat(...)`, etc. (all of which have a return type of `Null`).

Bug: https://b.corp.google.com/issues/200173910
Change-Id: I70ce5da5002e6254397024da5a9e9f69eb04f21c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/214313
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
---
 pkg/nnbd_migration/lib/nnbd_migration.dart    |  3 ++-
 pkg/nnbd_migration/lib/src/fix_builder.dart   |  2 +-
 pkg/nnbd_migration/test/api_test.dart         | 18 ++++++++++++++++++
 pkg/nnbd_migration/test/fix_builder_test.dart | 16 ++++++++++++++++
 4 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/pkg/nnbd_migration/lib/nnbd_migration.dart b/pkg/nnbd_migration/lib/nnbd_migration.dart
index d184cb0e1cc..a780bfa860d 100644
--- a/pkg/nnbd_migration/lib/nnbd_migration.dart
+++ b/pkg/nnbd_migration/lib/nnbd_migration.dart
@@ -124,7 +124,8 @@ class NullabilityFixDescription {
   /// Informative message: there is no valid migration for `null` in a
   /// non-nullable context.
   static const noValidMigrationForNull = NullabilityFixDescription._(
-      appliedMessage: 'No valid migration for `null` in a non-nullable context',
+      appliedMessage: 'No valid migration for expression with type `Null` in '
+          'a non-nullable context',
       kind: NullabilityFixKind.noValidMigrationForNull);
 
   /// Informative message: a null-aware access won't be necessary in strong
diff --git a/pkg/nnbd_migration/lib/src/fix_builder.dart b/pkg/nnbd_migration/lib/src/fix_builder.dart
index 21b38d46c36..7524133d501 100644
--- a/pkg/nnbd_migration/lib/src/fix_builder.dart
+++ b/pkg/nnbd_migration/lib/src/fix_builder.dart
@@ -635,7 +635,7 @@ class MigrationResolutionHooksImpl
     var resultType =
         _fixBuilder!._typeSystem.promoteToNonNull(type as TypeImpl);
     _flowAnalysis!.nonNullAssert_end(node);
-    return node is NullLiteral && hint == null
+    return type.isDartCoreNull && hint == null
         ? NoValidMigrationChange(resultType)
         : NullCheckChange(resultType, hint: hint);
   }
diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart
index 98151989123..7e35d77b337 100644
--- a/pkg/nnbd_migration/test/api_test.dart
+++ b/pkg/nnbd_migration/test/api_test.dart
@@ -6746,6 +6746,24 @@ void f() {
     await _checkSingleFileChanges(content, expected);
   }
 
+  Future<void> test_null_typed_expression_wiithout_valid_migration() async {
+    var content = '''
+void f(int/*!*/ x) {}
+void g() {
+  f(h());
+}
+Null h() => null;
+''';
+    var expected = '''
+void f(int x) {}
+void g() {
+  f(h());
+}
+Null h() => null;
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
   Future<void> test_nullable_use_of_typedef() async {
     var content = '''
 typedef F<T> = int Function(T);
diff --git a/pkg/nnbd_migration/test/fix_builder_test.dart b/pkg/nnbd_migration/test/fix_builder_test.dart
index 242c69f56e9..76c2bcaab03 100644
--- a/pkg/nnbd_migration/test/fix_builder_test.dart
+++ b/pkg/nnbd_migration/test/fix_builder_test.dart
@@ -2307,6 +2307,22 @@ _f(bool/*?*/ x) => x && x;
         changes: {findNode.simple('x &&'): isNullCheck});
   }
 
+  Future<void> test_nullExpression_noValidMigration() async {
+    await analyze('''
+int/*!*/ f() => g();
+Null g() => null;
+''');
+    var invocation = findNode.methodInvocation('g();');
+    // Note: in spite of the fact that we leave the method invocation alone, we
+    // analyze it as though it has type `Never`, because it's in a context where
+    // `null` doesn't work.
+    visitSubexpression(invocation, 'Never', changes: {
+      invocation: isNodeChangeForExpression.havingNoValidMigrationWithInfo(
+          isInfo(NullabilityFixDescription.noValidMigrationForNull,
+              {FixReasonTarget.root: TypeMatcher<NullabilityEdge>()}))
+    });
+  }
+
   Future<void> test_nullLiteral() async {
     await analyze('''
 f() => null;