From 776f2d34e9a077843fe605104e13ffabe7246342 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Wed, 31 Jul 2019 03:56:28 +0000 Subject: [PATCH] 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 Commit-Queue: Paul Berry --- .../lib/src/nullability_node.dart | 101 ++++++++++++++---- pkg/nnbd_migration/test/api_test.dart | 45 ++++++-- .../test/nullability_node_test.dart | 69 ++++++++++++ 3 files changed, 187 insertions(+), 28 deletions(-) diff --git a/pkg/nnbd_migration/lib/src/nullability_node.dart b/pkg/nnbd_migration/lib/src/nullability_node.dart index ae35885388e4..cf2b66287417 100644 --- a/pkg/nnbd_migration/lib/src/nullability_node.dart +++ b/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 _pendingSubstitutions = []; + List _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 {} diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart index 3fbee32e03ac..acf39ee7af94 100644 --- a/pkg/nnbd_migration/test/api_test.dart +++ b/pkg/nnbd_migration/test/api_test.dart @@ -474,10 +474,6 @@ void test(C 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 c)` var expected = ''' class C { void f(T t) {} @@ -485,7 +481,7 @@ class C { void g(C c, int? i) { c.f(i); } -void test(C c) { +void test(C c) { g(c, null); } '''; @@ -524,14 +520,11 @@ void test(List 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 x)` var expected = ''' void f(List x, int? i) { x.add(i); } -void test(List x) { +void test(List x) { f(x, null); } '''; @@ -1046,6 +1039,40 @@ main() { await _checkSingleFileChanges(content, expected); } + test_generic_exact_propagation() async { + var content = ''' +class C { + List values; + C() : values = []; + void add(T t) => values.add(t); + T operator[](int i) => values[i]; +} +void f() { + C x = new C(); + g(x); +} +void g(C y) { + y.add(null); +} +'''; + var expected = ''' +class C { + List values; + C() : values = []; + void add(T t) => values.add(t); + T operator[](int i) => values[i]; +} +void f() { + C x = new C(); + g(x); +} +void g(C y) { + y.add(null); +} +'''; + await _checkSingleFileChanges(content, expected); + } + test_generic_function_type_syntax_inferred_dynamic_return() async { var content = ''' abstract class C { diff --git a/pkg/nnbd_migration/test/nullability_node_test.dart b/pkg/nnbd_migration/test/nullability_node_test.dart index 2ec1ff9ea09b..d122c94d2135 100644 --- a/pkg/nnbd_migration/test/nullability_node_test.dart +++ b/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