Migration: implement "exact nullability" propagation.

This allows us to properly handle contraviariant uses of generics,
where the nullability of a generic parameter needs to be propagated in
the reverse of the usual dependency direction.

Change-Id: Id31f906bb7e3fbbf0846144a4715b9a6591aceda
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/111303
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Paul Berry <[email protected]>

pkg/nnbd_migration/lib/src/nullability_node.dart

@@ -114,7 +114,7 @@ class NullabilityGraph {
 
   /// During execution of [_propagateDownstream], a list of all the substitution
   /// nodes that have not yet been resolved.
-  final List<NullabilityNodeForSubstitution> _pendingSubstitutions = [];
+  List<NullabilityNodeForSubstitution> _pendingSubstitutions = [];
 
   /// Records that [sourceNode] is immediately upstream from [destinationNode].
   ///
@@ -236,25 +236,15 @@ class NullabilityGraph {
           continue;
         }
         if (node is NullabilityNodeMutable && !node.isNullable) {
-          node._state = _NullabilityState.ordinaryNullable;
-          // Was not previously nullable, so we need to propagate.
-          _pendingEdges.addAll(node._downstreamEdges);
-          if (node is NullabilityNodeForSubstitution) {
-            _pendingSubstitutions.add(node);
-          }
+          _setNullable(node);
         }
       }
       if (_pendingSubstitutions.isEmpty) break;
-      var node = _pendingSubstitutions.removeLast();
-      if (node.innerNode.isNullable || node.outerNode.isNullable) {
-        // No further propagation is needed, since some other connection already
-        // propagated nullability to either the inner or outer node.
-        continue;
+      var oldPendingSubstitutions = _pendingSubstitutions;
+      _pendingSubstitutions = [];
+      for (var node in oldPendingSubstitutions) {
+        _resolvePendingSubstitution(node);
       }
-      // Heuristically choose to propagate to the inner node since this seems
-      // to lead to better quality migrations.
-      _pendingEdges.add(NullabilityEdge._(node.innerNode, const [],
-          _NullabilityEdgeKind.soft, _SubstitutionHeuristicOrigin()));
     }
   }
 
@@ -276,6 +266,67 @@ class NullabilityGraph {
       }
     }
   }
+
+  void _resolvePendingSubstitution(
+      NullabilityNodeForSubstitution substitutionNode) {
+    // If both nodes pointed to by the substitution node are in the non-nullable
+    // state, then no resolution is needed; the substitution node can’t be
+    // satisfied.
+    if (substitutionNode.innerNode._state == _NullabilityState.nonNullable &&
+        substitutionNode.outerNode._state == _NullabilityState.nonNullable) {
+      // TODO(paulberry): should we report an error?
+      return;
+    }
+
+    // Otherwise, if the outer node is in a nullable state, then no resolution
+    // is needed because the substitution node is already satisfied.
+    if (substitutionNode.outerNode.isNullable) {
+      return;
+    }
+
+    // Otherwise, if the inner node is in the non-nullable state, then we set
+    // the outer node to the ordinary nullable state.
+    if (substitutionNode.innerNode._state == _NullabilityState.nonNullable) {
+      _setNullable(substitutionNode.outerNode as NullabilityNodeMutable);
+      return;
+    }
+
+    // Otherwise, we set the inner node to the exact nullable state, and we
+    // propagate this state upstream as far as possible using the following
+    // rule: if there is an edge A → B, where A is in the undetermined or
+    // ordinary nullable state, and B is in the exact nullable state, then A’s
+    // state is changed to exact nullable.
+    var pendingNodes = [substitutionNode.innerNode];
+    while (pendingNodes.isNotEmpty) {
+      var node = pendingNodes.removeLast();
+      if (node is NullabilityNodeMutable) {
+        var oldState =
+            _setNullable(node, newState: _NullabilityState.exactNullable);
+        if (oldState != _NullabilityState.exactNullable) {
+          // Was not previously in the "exact nullable" state.  Need to
+          // propagate.
+          for (var edge in node._upstreamEdges) {
+            pendingNodes.add(edge.primarySource);
+          }
+        }
+      }
+    }
+  }
+
+  _NullabilityState _setNullable(NullabilityNodeMutable node,
+      {_NullabilityState newState = _NullabilityState.ordinaryNullable}) {
+    assert(newState.isNullable);
+    var oldState = node._state;
+    node._state = newState;
+    if (!oldState.isNullable) {
+      // Was not previously nullable, so we need to propagate.
+      _pendingEdges.addAll(node._downstreamEdges);
+      if (node is NullabilityNodeForSubstitution) {
+        _pendingSubstitutions.add(node);
+      }
+    }
+    return oldState;
+  }
 }
 
 /// Same as [NullabilityGraph], but extended with extra methods for easier
@@ -369,6 +420,11 @@ abstract class NullabilityNode {
   /// annotate the nullability of that type.
   String get debugSuffix => '?($this)';
 
+  /// After nullability propagation, this getter can be used to query whether
+  /// the type associated with this node should be considered "exact nullable".
+  @visibleForTesting
+  bool get isExactNullable;
+
   /// After nullability propagation, this getter can be used to query whether
   /// the type associated with this node should be considered nullable.
   bool get isNullable;
@@ -476,6 +532,9 @@ abstract class NullabilityNodeMutable extends NullabilityNode {
       : _state = initialState,
         super._();
 
+  @override
+  bool get isExactNullable => _state == _NullabilityState.exactNullable;
+
   @override
   bool get isNullable => _state.isNullable;
 }
@@ -497,6 +556,9 @@ enum _NullabilityEdgeKind {
 abstract class _NullabilityNodeCompound extends NullabilityNodeMutable {
   _NullabilityNodeCompound() : super._();
 
+  @override
+  bool get isExactNullable => _components.any((c) => c.isExactNullable);
+
   @override
   bool get isNullable => _components.any((c) => c.isNullable);
 
@@ -515,9 +577,12 @@ class _NullabilityNodeImmutable extends NullabilityNode {
   @override
   String get debugSuffix => isNullable ? '?' : '';
 
+  @override
+  bool get isExactNullable => isNullable;
+
   @override
   _NullabilityState get _state => isNullable
-      ? _NullabilityState.ordinaryNullable
+      ? _NullabilityState.exactNullable
       : _NullabilityState.nonNullable;
 }
 
@@ -562,5 +627,3 @@ class _NullabilityState {
   @override
   String toString() => name;
 }
-
-class _SubstitutionHeuristicOrigin extends EdgeOrigin {}

pkg/nnbd_migration/test/api_test.dart

@@ -474,18 +474,14 @@ void test(C<int> c) {
     // propagate to a field in the class, and thence (via return values of other
     // methods) to most users of the class.  Whereas if we add nullability at
     // the call site it's possible that other call sites won't need it.
-    //
-    // TODO(paulberry): possible improvement: detect that since C uses T in a
-    // contravariant way, and deduce that test should change to
-    // `void test(C<int?> c)`
     var expected = '''
 class C<T> {
   void f(T t) {}
 }
 void g(C<int?> c, int? i) {
   c.f(i);
 }
-void test(C<int> c) {
+void test(C<int?> c) {
   g(c, null);
 }
 ''';
@@ -524,14 +520,11 @@ void test(List<int> x) {
   f(x, null);
 }
 ''';
-    // TODO(paulberry): possible improvement: detect that since add uses T in
-    // a contravariant way, and deduce that test should change to
-    // `void test(List<int?> x)`
     var expected = '''
 void f(List<int?> x, int? i) {
   x.add(i);
 }
-void test(List<int> x) {
+void test(List<int?> x) {
   f(x, null);
 }
 ''';
@@ -1046,6 +1039,40 @@ main() {
     await _checkSingleFileChanges(content, expected);
   }
 
+  test_generic_exact_propagation() async {
+    var content = '''
+class C<T> {
+  List<T> values;
+  C() : values = <T>[];
+  void add(T t) => values.add(t);
+  T operator[](int i) => values[i];
+}
+void f() {
+  C<int> x = new C<int>();
+  g(x);
+}
+void g(C<int> y) {
+  y.add(null);
+}
+''';
+    var expected = '''
+class C<T> {
+  List<T> values;
+  C() : values = <T>[];
+  void add(T t) => values.add(t);
+  T operator[](int i) => values[i];
+}
+void f() {
+  C<int?> x = new C<int?>();
+  g(x);
+}
+void g(C<int?> y) {
+  y.add(null);
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
   test_generic_function_type_syntax_inferred_dynamic_return() async {
     var content = '''
 abstract class C {

pkg/nnbd_migration/test/nullability_node_test.dart

@@ -253,6 +253,75 @@ class NullabilityNodeTest {
     assertUnsatisfied([]);
   }
 
+  test_propagation_downstream_reverse_substitution_exact() {
+    // always -> subst(1, 2)
+    // 3 -> 1
+    // 4 -> 3
+    var n1 = newNode(1);
+    var n2 = newNode(2);
+    var n3 = newNode(3);
+    var n4 = newNode(4);
+    connect(always, subst(n1, n2));
+    connect(n3, n1);
+    connect(n4, n3);
+    propagate();
+    expect(n1.isNullable, true);
+    expect(n1.isExactNullable, true);
+    expect(n2.isNullable, false);
+    expect(n3.isNullable, true);
+    expect(n3.isExactNullable, true);
+    expect(n4.isNullable, true);
+    expect(n4.isExactNullable, true);
+  }
+
+  test_propagation_downstream_reverse_substitution_inner_non_nullable() {
+    // 1 -> never (hard)
+    // always -> subst(1, 2)
+    // 3 -> 2
+    var n1 = newNode(1);
+    var n2 = newNode(2);
+    var n3 = newNode(3);
+    connect(n1, never, hard: true);
+    connect(always, subst(n1, n2));
+    connect(n3, n2);
+    propagate();
+    expect(n1.isNullable, false);
+    expect(n2.isNullable, true);
+    expect(n2.isExactNullable, false);
+    expect(n3.isNullable, false);
+  }
+
+  test_propagation_downstream_reverse_substitution_outer_already_nullable() {
+    // always -> 2
+    // always -> subst(1, 2)
+    // 3 -> 2
+    var n1 = newNode(1);
+    var n2 = newNode(2);
+    var n3 = newNode(3);
+    connect(always, n2);
+    connect(always, subst(n1, n2));
+    connect(n3, n2);
+    propagate();
+    expect(n1.isNullable, false);
+    expect(n2.isNullable, true);
+    expect(n2.isExactNullable, false);
+    expect(n3.isNullable, false);
+  }
+
+  test_propagation_downstream_reverse_substitution_unsatisfiable() {
+    // 1 -> never (hard)
+    // 2 -> never (hard)
+    // always -> subst(1, 2)
+    var n1 = newNode(1);
+    var n2 = newNode(2);
+    connect(n1, never, hard: true);
+    connect(n2, never, hard: true);
+    connect(always, subst(n1, n2));
+    propagate();
+    expect(n1.isNullable, false);
+    expect(n2.isNullable, false);
+  }
+
   test_propagation_downstream_through_lub_both() {
     // always -> 1
     // always -> 2