Migration: Replace ?. with . when target already migrated.

With this change, the migration tool no longer warns "Null-aware
access will be unnecessary in strong checking mode" for the specific
case where the target of a null-aware access is a non-nullable method
or getter invocation, and the method or getter in question is in
already-migrated code.  The whole point of the warning is to help the
user identify situations where the migration tool might have made the
wrong decision about code that it's currently migrating; that's not
useful if the method or getter was migrated previously.  So instead,
in these circumstances, the migration tool simply changes `?.` to `.`.

Fixes https://github.com/dart-lang/sdk/issues/49601.

Bug: https://github.com/dart-lang/sdk/issues/49601
Change-Id: I8fe26df696dafa28ac1104f4b721f29bfe7ba164
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/256646
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>

pkg/nnbd_migration/lib/src/fix_aggregator.dart

@@ -1013,23 +1013,29 @@ class NodeChangeForMethodName extends NodeChange<SimpleIdentifier> {
 /// Common infrastructure used by [NodeChange] objects that operate on AST nodes
 /// with that can be null-aware (method invocations and propety accesses).
 mixin NodeChangeForNullAware<N extends Expression> on NodeChange<N> {
-  /// Indicates whether null-awareness should be removed.
-  bool removeNullAwareness = false;
+  /// Indicates how null-awareness should be handled.
+  NullAwarenessRemovalType nullAwarenessRemoval = NullAwarenessRemovalType.none;
 
   @override
-  Iterable<String> get _toStringParts =>
-      [...super._toStringParts, if (removeNullAwareness) 'removeNullAwareness'];
+  Iterable<String> get _toStringParts => [
+        ...super._toStringParts,
+        if (nullAwarenessRemoval == NullAwarenessRemovalType.strong)
+          'removeNullAwareness (strong)'
+        else if (nullAwarenessRemoval == NullAwarenessRemovalType.weak)
+          'removeNullAwareness (weak)'
+      ];
 
   /// Returns an [EditPlan] that removes null awareness, if appropriate.
   /// Otherwise returns `null`.
   EditPlan? _applyNullAware(N node, FixAggregator aggregator) {
-    if (!removeNullAwareness) return null;
-    var description = aggregator._warnOnWeakCode
+    if (nullAwarenessRemoval == NullAwarenessRemovalType.none) return null;
+    var weak = aggregator._warnOnWeakCode &&
+        nullAwarenessRemoval == NullAwarenessRemovalType.weak;
+    var description = weak
         ? NullabilityFixDescription.nullAwarenessUnnecessaryInStrongMode
         : NullabilityFixDescription.removeNullAwareness;
     return aggregator.planner.removeNullAwareness(node,
-        info: AtomicEditInfo(description, const {}),
-        isInformative: aggregator._warnOnWeakCode);
+        info: AtomicEditInfo(description, const {}), isInformative: weak);
   }
 }
 
@@ -1366,6 +1372,19 @@ class NoValidMigrationChange extends ExpressionChange {
   String describe() => 'NoValidMigrationChange';
 }
 
+/// Enum used by [NodeChangeForNullAware] to indicate how null awareness should
+/// be handled.
+enum NullAwarenessRemovalType {
+  /// Do not remove null awareness.
+  none,
+
+  /// If warning on weak code, issue a warning; otherwise remove null awareness.
+  weak,
+
+  /// Remove null awareness unconditionally.
+  strong,
+}
+
 /// [ExpressionChange] describing the addition of an `!` after an expression.
 class NullCheckChange extends ExpressionChange {
   /// The hint that is causing this `!` to be added, if any.

pkg/nnbd_migration/lib/src/fix_builder.dart

@@ -266,6 +266,20 @@ class FixBuilder {
     }
   }
 
+  /// If [node] is a property access or method invocation, returns the element
+  /// it invokes.  Otherwise returns `null`.
+  Element? _findPropertyOrMethodElement(Expression node) {
+    if (node is PrefixedIdentifier) {
+      return node.identifier.staticElement;
+    } else if (node is PropertyAccess) {
+      return node.propertyName.staticElement;
+    } else if (node is MethodInvocation) {
+      return node.methodName.staticElement;
+    } else {
+      return null;
+    }
+  }
+
   /// Returns the [NodeChange] object accumulating changes for the given [node],
   /// creating it if necessary.
   NodeChange _getChange(AstNode node) =>
@@ -284,7 +298,18 @@ class FixBuilder {
       throw StateError('Unexpected expression type: ${node.runtimeType}');
     }
     if (!_typeSystem.isPotentiallyNullable(target!.staticType!)) {
-      (_getChange(node) as NodeChangeForNullAware).removeNullAwareness = true;
+      var element = _findPropertyOrMethodElement(target);
+      if (element != null) {
+        var library = element.library;
+        if (library!.isNonNullableByDefault &&
+            !_graph.isBeingMigrated(library.source)) {
+          (_getChange(node) as NodeChangeForNullAware).nullAwarenessRemoval =
+              NullAwarenessRemovalType.strong;
+          return false;
+        }
+      }
+      (_getChange(node) as NodeChangeForNullAware).nullAwarenessRemoval =
+          NullAwarenessRemovalType.weak;
       return false;
     }
     return true;

pkg/nnbd_migration/test/api_test.dart

@@ -7274,6 +7274,158 @@ main() {
     await _checkSingleFileChanges(content, expected);
   }
 
+  Future<void> test_null_aware_call_on_migrated_get() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D get g;
+}
+abstract class D {
+  int f2();
+}
+''';
+    // Since `.g` is a non-nullable getter in an already-migrated class, the
+    // `?.` can safely be replaced with `.`.  We can safely make this change
+    // even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C c) => c.g?.f2();
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C c) => c.g.f2();
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_call_on_migrated_get_null_shorting() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D get g;
+}
+abstract class D {
+  int f2();
+}
+''';
+    // Since `.g` is a non-nullable getter in an already-migrated class, the
+    // `?.` can safely be replaced with `.`.  We can safely make this change
+    // even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C/*?*/ c) => c?.g?.f2();
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C? c) => c?.g.f2();
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_call_on_migrated_method() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D f();
+}
+abstract class D {
+  int f2();
+}
+''';
+    // Since `.f()` is a method with a non-nullable return type in an
+    // already-migrated class, the `?.` can safely be replaced with `.`.  We can
+    // safely make this change even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C c) => c.f()?.f2();
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C c) => c.f().f2();
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_call_on_migrated_method_null_shorting() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D f();
+}
+abstract class D {
+  int f2();
+}
+''';
+    // Since `.f()` is a method with a non-nullable return type in an
+    // already-migrated class, the `?.` can safely be replaced with `.`.  We can
+    // safely make this change even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C/*?*/ c) => c?.f()?.f2();
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C? c) => c?.f().f2();
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_call_on_nullable_get() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D? get g;
+}
+abstract class D {
+  int f2();
+}
+''';
+    // Since `.g` is a nullable getter in an already-migrated class, we don't
+    // replace `?.` with `.`.
+    var content = '''
+import 'migrated.dart';
+f(C c) => c.g?.f2();
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C c) => c.g?.f2();
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput});
+  }
+
+  Future<void> test_null_aware_call_on_nullable_method() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D? f();
+}
+abstract class D {
+  int f2();
+}
+''';
+    // Since `.f()` is a method with a nullable return type in an
+    // already-migrated class, we don't replace `?.` with `.`.
+    var content = '''
+import 'migrated.dart';
+f(C c) => c.f()?.f2();
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C c) => c.f()?.f2();
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput});
+  }
+
   Future<void> test_null_aware_call_on_private_param_not_nullable() async {
     // The null-aware access on `i` is *not* considered a strong enough signal
     // that `i` is meant to be nullable, because the migration tool can see all
@@ -7319,6 +7471,62 @@ int f(int i) {
     await _checkSingleFileChanges(content, expected);
   }
 
+  Future<void> test_null_aware_call_on_unmigrated_get() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class D {
+  int f2();
+}
+''';
+    // Since `.g` is unmigrated, we don't replace `?.` with `.` in "warn on weak
+    // code" mode.
+    var content = '''
+import 'migrated.dart';
+abstract class C {
+  D get g;
+}
+f(C c) => c.g?.f2();
+''';
+    var expected = '''
+import 'migrated.dart';
+abstract class C {
+  D get g;
+}
+f(C c) => c.g?.f2();
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_call_on_unmigrated_method() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class D {
+  int f2();
+}
+''';
+    // Since `.f()` is unmigrated, we don't replace `?.` with `.` in "warn on
+    // weak code" mode.
+    var content = '''
+import 'migrated.dart';
+abstract class C {
+  D f();
+}
+f(C c) => c.f()?.f2();
+''';
+    var expected = '''
+import 'migrated.dart';
+abstract class C {
+  D f();
+}
+f(C c) => c.f()?.f2();
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
   Future<void> test_null_aware_call_tearoff() async {
     // Kind of a weird use case because `f?.call` is equivalent to `f`, but
     // let's make sure we analyze it correctly.
@@ -7371,6 +7579,110 @@ main() {
     await _checkSingleFileChanges(content, expected);
   }
 
+  Future<void> test_null_aware_get_on_migrated_get() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D get g;
+}
+abstract class D {
+  int get g2;
+}
+''';
+    // Since `.g` is a non-nullable getter in an already-migrated class, the
+    // `?.` can safely be replaced with `.`.  We can safely make this change
+    // even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C c) => c.g?.g2;
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C c) => c.g.g2;
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_get_on_migrated_get_null_shorting() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D get g;
+}
+abstract class D {
+  int get g2;
+}
+''';
+    // Since `.g` is a non-nullable getter in an already-migrated class, the
+    // `?.` can safely be replaced with `.`.  We can safely make this change
+    // even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C/*?*/ c) => c?.g?.g2;
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C? c) => c?.g.g2;
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_get_on_migrated_method() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D f();
+}
+abstract class D {
+  int get g2;
+}
+''';
+    // Since `.f()` is a method with a non-nullable return type in an
+    // already-migrated class, the `?.` can safely be replaced with `.`.  We can
+    // safely make this change even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C c) => c.f()?.g2;
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C c) => c.f().g2;
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_get_on_migrated_method_null_shorting() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D f();
+}
+abstract class D {
+  int get g2;
+}
+''';
+    // Since `.f()` is a method with a non-nullable return type in an
+    // already-migrated class, the `?.` can safely be replaced with `.`.  We can
+    // safely make this change even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C/*?*/ c) => c?.f()?.g2;
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C? c) => c?.f().g2;
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
   Future<void> test_null_aware_get_on_private_param_not_nullable() async {
     // The null-aware access on `i` is *not* considered a strong enough signal
     // that `i` is meant to be nullable, because the migration tool can see all
@@ -7502,6 +7814,110 @@ main() {
     await _checkSingleFileChanges(content, expected);
   }
 
+  Future<void> test_null_aware_set_on_migrated_get() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D get g;
+}
+abstract class D {
+  set s(int i);
+}
+''';
+    // Since `.g` is a non-nullable getter in an already-migrated class, the
+    // `?.` can safely be replaced with `.`.  We can safely make this change
+    // even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C c) => c.g?.s = 0;
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C c) => c.g.s = 0;
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_set_on_migrated_get_null_shorting() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D get g;
+}
+abstract class D {
+  set s(int i);
+}
+''';
+    // Since `.g` is a non-nullable getter in an already-migrated class, the
+    // `?.` can safely be replaced with `.`.  We can safely make this change
+    // even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C/*?*/ c) => c?.g?.s = 0;
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C? c) => c?.g.s = 0;
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_set_on_migrated_method() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D f();
+}
+abstract class D {
+  set s(int i);
+}
+''';
+    // Since `.f()` is a method with a non-nullable return type in an
+    // already-migrated class, the `?.` can safely be replaced with `.`.  We can
+    // safely make this change even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C c) => c.f()?.s = 0;
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C c) => c.f().s = 0;
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
+  Future<void> test_null_aware_set_on_migrated_method_null_shorting() async {
+    var migratedInput = '''
+// @dart=2.12
+abstract class C {
+  D f();
+}
+abstract class D {
+  set s(int i);
+}
+''';
+    // Since `.f()` is a method with a non-nullable return type in an
+    // already-migrated class, the `?.` can safely be replaced with `.`.  We can
+    // safely make this change even if we are in "warn on weak code" mode.
+    var content = '''
+import 'migrated.dart';
+f(C/*?*/ c) => c?.f()?.s = 0;
+''';
+    var expected = '''
+import 'migrated.dart';
+f(C? c) => c?.f().s = 0;
+''';
+    await _checkSingleFileChanges(content, expected,
+        migratedInput: {'$projectPath/lib/migrated.dart': migratedInput},
+        warnOnWeakCode: true);
+  }
+
   Future<void> test_null_aware_set_on_private_param_not_nullable() async {
     // The null-aware access on `c` is *not* considered a strong enough signal
     // that `c` is meant to be nullable, because the migration tool can see all

pkg/nnbd_migration/test/fix_aggregator_test.dart

@@ -1790,7 +1790,7 @@ int f() => null;
     var methodInvocation = findNode.methodInvocation('?.');
     var previewInfo = run({
       methodInvocation: NodeChangeForMethodInvocation()
-        ..removeNullAwareness = true
+        ..nullAwarenessRemoval = NullAwarenessRemovalType.strong
     })!;
     expect(previewInfo.applyTo(code!), 'f(x) => x.m();');
   }
@@ -1802,7 +1802,7 @@ int f() => null;
     var argument = findNode.simple('x);');
     var previewInfo = run({
       methodInvocation: NodeChangeForMethodInvocation()
-        ..removeNullAwareness = true,
+        ..nullAwarenessRemoval = NullAwarenessRemovalType.strong,
       argument: NodeChangeForExpression()
         ..addExpressionChange(NullCheckChange(MockDartType()), _MockInfo())
     })!;
@@ -1816,7 +1816,7 @@ int f() => null;
     var cast = findNode.as_('as');
     var previewInfo = run({
       methodInvocation: NodeChangeForMethodInvocation()
-        ..removeNullAwareness = true,
+        ..nullAwarenessRemoval = NullAwarenessRemovalType.strong,
       cast: NodeChangeForAsExpression()..removeAs = true
     })!;
     expect(previewInfo.applyTo(code!), 'f(x) => x.m();');
@@ -1829,7 +1829,7 @@ int f() => null;
     var typeAnnotation = findNode.typeAnnotation('int');
     var previewInfo = run({
       methodInvocation: NodeChangeForMethodInvocation()
-        ..removeNullAwareness = true,
+        ..nullAwarenessRemoval = NullAwarenessRemovalType.strong,
       typeAnnotation: NodeChangeForTypeAnnotation()
         ..recordNullability(
             MockDecoratedType(
@@ -1843,7 +1843,8 @@ int f() => null;
     await analyze('f(x) => x?.y;');
     var propertyAccess = findNode.propertyAccess('?.');
     var previewInfo = run({
-      propertyAccess: NodeChangeForPropertyAccess()..removeNullAwareness = true
+      propertyAccess: NodeChangeForPropertyAccess()
+        ..nullAwarenessRemoval = NullAwarenessRemovalType.strong
     })!;
     expect(previewInfo.applyTo(code!), 'f(x) => x.y;');
   }
@@ -1853,7 +1854,8 @@ int f() => null;
     var propertyAccess = findNode.propertyAccess('?.');
     var cast = findNode.as_('as');
     var previewInfo = run({
-      propertyAccess: NodeChangeForPropertyAccess()..removeNullAwareness = true,
+      propertyAccess: NodeChangeForPropertyAccess()
+        ..nullAwarenessRemoval = NullAwarenessRemovalType.strong,
       cast: NodeChangeForAsExpression()..removeAs = true
     })!;
     expect(previewInfo.applyTo(code!), 'f(x) => x.y;');
@@ -2057,7 +2059,7 @@ f(int i) {
     await analyze(content);
     var previewInfo = run({
       findNode.propertyAccess('?.'): NodeChangeForPropertyAccess()
-        ..removeNullAwareness = true
+        ..nullAwarenessRemoval = NullAwarenessRemovalType.weak
     }, warnOnWeakCode: true)!;
     expect(previewInfo.applyTo(code!), content);
     expect(previewInfo, hasLength(1));

pkg/nnbd_migration/test/fix_builder_test.dart

@@ -92,8 +92,10 @@ class FixBuilderTest extends EdgeBuilderTestBase {
           'addNames', unorderedEquals(['IterableExtension']));
 
   static final isRemoveNullAwareness =
-      TypeMatcher<NodeChangeForPropertyAccess>()
-          .having((c) => c.removeNullAwareness, 'removeNullAwareness', true);
+      TypeMatcher<NodeChangeForPropertyAccess>().having(
+          (c) => c.nullAwarenessRemoval != NullAwarenessRemovalType.none,
+          'removeNullAwareness',
+          true);
 
   static final isRemoveAs = TypeMatcher<NodeChangeForAsExpression>()
       .having((c) => c.removeAs, 'removeAs', true);
@@ -3281,7 +3283,10 @@ int/*!*/ f(C/*!*/ c) => c?.i;
     visitSubexpression(propertyAccess, 'int', changes: {
       propertyAccess: TypeMatcher<NodeChangeForPropertyAccess>()
           .havingNullCheckWithInfo(isNotNull)
-          .having((c) => c.removeNullAwareness, 'removeNullAwareness', true)
+          .having(
+              (c) => c.nullAwarenessRemoval != NullAwarenessRemovalType.none,
+              'removeNullAwareness',
+              true)
     });
   }