Migration: Clean up handling of LUB and substitution nodes.
Previously we created artifical edges from a LUB node's components to
the LUB node; now the nullability propagation algorithm understands
how to propagate nullability downstream through LUB and substitution
nodes, so no artificial edges are needed.
Change-Id: Ib2fef14c3dcd58a013ad8e057fd55597a1deb4fc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105402
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/nnbd_migration/lib/src/graph_builder.dart b/pkg/nnbd_migration/lib/src/graph_builder.dart
index 3610a6f..61c5623 100644
--- a/pkg/nnbd_migration/lib/src/graph_builder.dart
+++ b/pkg/nnbd_migration/lib/src/graph_builder.dart
@@ -235,7 +235,7 @@
var elseType = node.elseExpression.accept(this);
assert(_isSimple(elseType)); // TODO(paulberry)
var overallType = DecoratedType(node.staticType,
- NullabilityNode.forLUB(node, thenType.node, elseType.node, _graph));
+ NullabilityNode.forLUB(node, thenType.node, elseType.node));
_variables.recordDecoratedExpressionType(node, overallType);
return overallType;
}
@@ -616,6 +616,7 @@
bool _isSimple(DecoratedType type) {
if (type.type.isBottom) return true;
if (type.type.isVoid) return true;
+ if (type.type is TypeParameterType) return true;
if (type.type is! InterfaceType) return false;
if ((type.type as InterfaceType).typeParameters.isNotEmpty) return false;
return true;
diff --git a/pkg/nnbd_migration/lib/src/nullability_node.dart b/pkg/nnbd_migration/lib/src/nullability_node.dart
index 453887d..6e4336e 100644
--- a/pkg/nnbd_migration/lib/src/nullability_node.dart
+++ b/pkg/nnbd_migration/lib/src/nullability_node.dart
@@ -60,16 +60,7 @@
var sources = [sourceNode]..addAll(guards);
var edge = NullabilityEdge(destinationNode, sources, hard);
for (var source in sources) {
- _allSourceNodes.add(source);
- if (source is NullabilityNodeMutable) {
- source._downstreamEdges.add(edge);
- } else if (source == NullabilityNode.always) {
- _downstreamFromAlways.add(edge);
- } else {
- // We don't need to track nodes that are downstream from `never` because
- // `never` will never be nullable.
- assert(source == NullabilityNode.never);
- }
+ _connectDownstream(source, edge);
}
if (destinationNode is NullabilityNodeMutable) {
destinationNode._upstreamEdges.add(edge);
@@ -144,6 +135,24 @@
_propagateDownstream();
}
+ void _connectDownstream(NullabilityNode source, NullabilityEdge edge) {
+ _allSourceNodes.add(source);
+ if (source is NullabilityNodeMutable) {
+ source._downstreamEdges.add(edge);
+ if (source is _NullabilityNodeCompound) {
+ for (var component in source._components) {
+ _connectDownstream(component, edge);
+ }
+ }
+ } else if (source == NullabilityNode.always) {
+ _downstreamFromAlways.add(edge);
+ } else {
+ // We don't need to track nodes that are downstream from `never` because
+ // `never` will never be nullable.
+ assert(source == NullabilityNode.never);
+ }
+ }
+
Iterable<NullabilityEdge> _getDownstreamEdges(NullabilityNode node) {
if (node is NullabilityNodeMutable) {
return node._downstreamEdges;
@@ -176,7 +185,7 @@
}
for (var source in edge.sources) {
if (!source.isNullable) {
- // Note all sources are nullable, so this edge doesn't apply yet.
+ // Not all sources are nullable, so this edge doesn't apply yet.
continue nextEdge;
}
}
@@ -264,11 +273,8 @@
/// [joinNullabilities] callback. TODO(paulberry): this should become
/// unnecessary once constraint solving is performed directly using
/// [NullabilityNode] objects.
- factory NullabilityNode.forLUB(
- Expression conditionalExpression,
- NullabilityNode a,
- NullabilityNode b,
- NullabilityGraph graph) = NullabilityNodeForLUB._;
+ factory NullabilityNode.forLUB(Expression conditionalExpression,
+ NullabilityNode a, NullabilityNode b) = NullabilityNodeForLUB._;
/// Creates a [NullabilityNode] representing the nullability of a type
/// substitution where [outerNode] is the nullability node for the type
@@ -372,17 +378,15 @@
/// Derived class for nullability nodes that arise from the least-upper-bound
/// implied by a conditional expression.
-class NullabilityNodeForLUB extends NullabilityNodeMutable {
+class NullabilityNodeForLUB extends _NullabilityNodeCompound {
final NullabilityNode left;
final NullabilityNode right;
- NullabilityNodeForLUB._(
- Expression expression, this.left, this.right, NullabilityGraph graph)
- : super._() {
- graph.connect(left, this);
- graph.connect(right, this);
- }
+ NullabilityNodeForLUB._(Expression expression, this.left, this.right);
+
+ @override
+ Iterable<NullabilityNode> get _components => [left, right];
@override
String get _debugPrefix => 'LUB($left, $right)';
@@ -390,7 +394,7 @@
/// Derived class for nullability nodes that arise from type variable
/// substitution.
-class NullabilityNodeForSubstitution extends NullabilityNodeMutable {
+class NullabilityNodeForSubstitution extends _NullabilityNodeCompound {
/// Nullability node representing the inner type of the substitution.
///
/// For example, if this NullabilityNode arose from substituting `int*` for
@@ -405,7 +409,10 @@
/// `*` in `T*`.
final NullabilityNode outerNode;
- NullabilityNodeForSubstitution._(this.innerNode, this.outerNode) : super._();
+ NullabilityNodeForSubstitution._(this.innerNode, this.outerNode);
+
+ @override
+ Iterable<NullabilityNode> get _components => [innerNode, outerNode];
@override
String get _debugPrefix => 'Substituted($innerNode, $outerNode)';
@@ -439,6 +446,15 @@
bool get isNullable => _state.isNullable;
}
+abstract class _NullabilityNodeCompound extends NullabilityNodeMutable {
+ _NullabilityNodeCompound() : super._();
+
+ @override
+ bool get isNullable => _components.any((c) => c.isNullable);
+
+ Iterable<NullabilityNode> get _components;
+}
+
class _NullabilityNodeImmutable extends NullabilityNode {
@override
final String _debugPrefix;
diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart
index 1419873..cf2d0d1 100644
--- a/pkg/nnbd_migration/test/api_test.dart
+++ b/pkg/nnbd_migration/test/api_test.dart
@@ -316,7 +316,7 @@
await _checkSingleFileChanges(content, expected);
}
- test_data_flow_generic_inward() async {
+ test_data_flow_generic_contravariant_inward() async {
var content = '''
class C<T> {
void f(T t) {}
@@ -354,6 +354,38 @@
await _checkSingleFileChanges(content, expected);
}
+ test_data_flow_generic_covariant_outward() async {
+ var content = '''
+class C<T> {
+ T getValue() => null;
+}
+int f(C<int> x) => x.getValue();
+''';
+ var expected = '''
+class C<T> {
+ T? getValue() => null;
+}
+int? f(C<int> x) => x.getValue();
+''';
+ await _checkSingleFileChanges(content, expected);
+ }
+
+ test_data_flow_generic_covariant_substituted() async {
+ var content = '''
+abstract class C<T> {
+ T getValue();
+}
+int f(C<int/*?*/> x) => x.getValue();
+''';
+ var expected = '''
+abstract class C<T> {
+ T getValue();
+}
+int? f(C<int?/*?*/> x) => x.getValue();
+''';
+ await _checkSingleFileChanges(content, expected);
+ }
+
test_data_flow_indexed_get_index_value() async {
var content = '''
class C {
diff --git a/pkg/nnbd_migration/test/migration_visitor_test.dart b/pkg/nnbd_migration/test/migration_visitor_test.dart
index 1660bb8..8ff7c98 100644
--- a/pkg/nnbd_migration/test/migration_visitor_test.dart
+++ b/pkg/nnbd_migration/test/migration_visitor_test.dart
@@ -1048,6 +1048,19 @@
contextNode: decoratedTypeAnnotation('int').node);
}
+ test_return_null_generic() async {
+ await analyze('''
+class C<T> {
+ T f() {
+ return null;
+ }
+}
+''');
+ var tNode = decoratedTypeAnnotation('T f').node;
+ assertEdge(always, tNode, hard: false);
+ assertNullCheck(checkExpression('null'), always, contextNode: tNode);
+ }
+
test_simpleIdentifier_local() async {
await analyze('''
main() {
