Migration: rework the ExpressionChecks data structure.
We no longer need to keep pointers to the nullability nodes directly
in the ExpressionChecks data structure; we merely need to record which
edges, if unsatisfied, would require a check.
This allowed the tests using assertNullCheck to be clarified--they now
simply verify that the appropriate edge exists, and then pass that
edge to assertNullCheck to verify that the ExpressionChecks object
points to the appropriate edge.
Change-Id: Iaaee51d1f23ca6f86a2fbf0a15ded6a844b2811b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106383
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
diff --git a/pkg/nnbd_migration/lib/src/expression_checks.dart b/pkg/nnbd_migration/lib/src/expression_checks.dart
index 58bae89..fd87c85 100644
--- a/pkg/nnbd_migration/lib/src/expression_checks.dart
+++ b/pkg/nnbd_migration/lib/src/expression_checks.dart
@@ -11,38 +11,51 @@
/// set of runtime checks that might need to be performed on the value of an
/// expression.
///
-/// TODO(paulberry): the only check we support now is [nullCheck], which checks
-/// that the expression is not null. We need to add other checks, e.g. to check
-/// that a List<int?> is actually a List<int>.
+/// TODO(paulberry): we don't currently have any way of distinguishing checks
+/// based on the nullability of the type itself (which can be checked by adding
+/// a trailing `!`) from checks based on type parameters (which will have to be
+/// checked using an `as` expression).
class ExpressionChecks extends PotentialModification implements EdgeOrigin {
/// Source offset where a trailing `!` might need to be inserted.
final int offset;
- /// Nullability node indicating whether the expression's value is nullable.
- final NullabilityNode valueNode;
+ /// List of all nullability edges that are related to this potential check.
+ ///
+ /// TODO(paulberry): update this data structure to keep track of all the ways
+ /// in which edges can be related to an [ExpressionChecks], including:
+ ///
+ /// - An edge which, if unsatisfied, indicates that the expression needs to be
+ /// null-checked.
+ /// - An edge which, if unsatisfied, indicates that a type parameter of the
+ /// expression needs to be checked for nullability (e.g. by the migration
+ /// engine inserting a test like `as List<int>?`)
+ /// - An edge which, if unsatisfied, indicates that a return type of the
+ /// expression needs to be checked for nullability (e.g. by the migration
+ /// engine inserting a test like `as int Function(...)?`)
+ /// - An edge which, if unsatisfied, indicates that a parameter type of the
+ /// expression needs to be checked for nullability (e.g. by the migration
+ /// engine inserting a test like `as void Function(int?)?`)
+ ///
+ /// ...and so on.
+ final List<NullabilityEdge> edges = [];
- /// Nullability node indicating whether the expression's context requires a
- /// nullable value.
- final NullabilityNode contextNode;
-
- /// Nullability nodes guarding execution of the expression. If any of the
- /// nodes in this list turns out to be non-nullable, the expression is dead
- /// code and will be removed by the migration tool.
- final List<NullabilityNode> guards;
-
- ExpressionChecks(this.offset, this.valueNode, this.contextNode,
- Iterable<NullabilityNode> guards)
- : guards = guards.toList();
+ ExpressionChecks(this.offset);
@override
bool get isEmpty {
- for (var guard in guards) {
- if (!guard.isNullable) return true;
+ for (var edge in edges) {
+ if (!edge.isSatisfied) return false;
}
- return !valueNode.isNullable || contextNode.isNullable;
+ return true;
}
@override
- Iterable<SourceEdit> get modifications =>
- isEmpty ? [] : [SourceEdit(offset, 0, '!')];
+ Iterable<SourceEdit> get modifications {
+ // TODO(paulberry): this assumes that the check that needs to be done is for
+ // the nullability of the type itself (in which case all we need is a simple
+ // null check). Need to support checks that will have to be addressed by
+ // adding an `as` expression, e.g. `as List<int>?` to verify that a list is
+ // reified to contain only non-null ints.
+ return isEmpty ? [] : [SourceEdit(offset, 0, '!')];
+ }
}
diff --git a/pkg/nnbd_migration/lib/src/graph_builder.dart b/pkg/nnbd_migration/lib/src/graph_builder.dart
index 08e7138..358601b 100644
--- a/pkg/nnbd_migration/lib/src/graph_builder.dart
+++ b/pkg/nnbd_migration/lib/src/graph_builder.dart
@@ -655,8 +655,10 @@
void _checkAssignment(DecoratedType destinationType, DecoratedType sourceType,
ExpressionChecks expressionChecks,
{@required bool hard}) {
- _graph.connect(sourceType.node, destinationType.node, expressionChecks,
+ var edge = _graph.connect(
+ sourceType.node, destinationType.node, expressionChecks,
guards: _guards, hard: hard);
+ expressionChecks?.edges?.add(edge);
// TODO(paulberry): generalize this.
if ((_isSimple(sourceType) || destinationType.type.isObject) &&
_isSimple(destinationType)) {
@@ -702,8 +704,7 @@
}
ExpressionChecks expressionChecks;
if (canInsertChecks) {
- expressionChecks = ExpressionChecks(
- expression.end, sourceType.node, destinationType.node, _guards);
+ expressionChecks = ExpressionChecks(expression.end);
_variables.recordExpressionChecks(_source, expression, expressionChecks);
}
_checkAssignment(destinationType, sourceType, expressionChecks,
diff --git a/pkg/nnbd_migration/test/migration_visitor_test.dart b/pkg/nnbd_migration/test/migration_visitor_test.dart
index c20e7f8..6633808 100644
--- a/pkg/nnbd_migration/test/migration_visitor_test.dart
+++ b/pkg/nnbd_migration/test/migration_visitor_test.dart
@@ -56,26 +56,13 @@
}
}
- /// Verifies that a null check will occur under the proper circumstances.
+ /// Verifies that a null check will occur when the given edge is unsatisfied.
///
/// [expressionChecks] is the object tracking whether or not a null check is
- /// needed. [valueNode] is the node representing the possibly-nullable value
- /// that is the source of the assignment or use. [contextNode] is the node
- /// representing the possibly-nullable value that is the destination of the
- /// assignment (if the value is being assigned), or `null` if the value is
- /// being used in a circumstance where `null` is not permitted. [guards] is
- /// a list of nullability nodes for which there are enclosing if statements
- /// checking that the corresponding values are non-null.
+ /// needed.
void assertNullCheck(
- ExpressionChecks expressionChecks, NullabilityNode valueNode,
- {NullabilityNode contextNode, List<NullabilityNode> guards = const []}) {
- expect(expressionChecks.valueNode, same(valueNode));
- if (contextNode == null) {
- expect(expressionChecks.contextNode, same(never));
- } else {
- expect(expressionChecks.contextNode, same(contextNode));
- }
- expect(expressionChecks.guards, guards);
+ ExpressionChecks expressionChecks, NullabilityEdge expectedEdge) {
+ expect(expressionChecks.edges, contains(expectedEdge));
}
/// Gets the [ExpressionChecks] associated with the expression whose text
@@ -139,8 +126,8 @@
c.x = i;
}
''');
- assertNullCheck(
- checkExpression('c.x'), decoratedTypeAnnotation('C c').node);
+ assertNullCheck(checkExpression('c.x'),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true));
}
test_assignmentExpression_field_target_check_cascaded() async {
@@ -152,8 +139,8 @@
c..x = i;
}
''');
- assertNullCheck(
- checkExpression('c..x'), decoratedTypeAnnotation('C c').node);
+ assertNullCheck(checkExpression('c..x'),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true));
}
test_assignmentExpression_indexExpression_index() async {
@@ -191,7 +178,8 @@
c[i] = j;
}
''');
- assertNullCheck(checkExpression('c['), decoratedTypeAnnotation('C c').node);
+ assertNullCheck(checkExpression('c['),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true));
}
test_assignmentExpression_indexExpression_value() async {
@@ -268,8 +256,8 @@
c.s = i;
}
''');
- assertNullCheck(
- checkExpression('c.s'), decoratedTypeAnnotation('C c').node);
+ assertNullCheck(checkExpression('c.s'),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true));
}
@failingTest
@@ -311,8 +299,8 @@
int f(int i, int j) => i + j;
''');
- assertNullCheck(
- checkExpression('i +'), decoratedTypeAnnotation('int i').node);
+ assertNullCheck(checkExpression('i +'),
+ assertEdge(decoratedTypeAnnotation('int i').node, never, hard: true));
}
test_binaryExpression_add_left_check_custom() async {
@@ -323,8 +311,8 @@
Int f(Int i, Int j) => i + j;
''');
- assertNullCheck(
- checkExpression('i +'), decoratedTypeAnnotation('Int i').node);
+ assertNullCheck(checkExpression('i +'),
+ assertEdge(decoratedTypeAnnotation('Int i').node, never, hard: true));
}
test_binaryExpression_add_result_custom() async {
@@ -335,9 +323,11 @@
Int f(Int i, Int j) => (i + j);
''');
- assertNullCheck(checkExpression('(i + j)'),
- decoratedTypeAnnotation('Int operator+').node,
- contextNode: decoratedTypeAnnotation('Int f').node);
+ assertNullCheck(
+ checkExpression('(i + j)'),
+ assertEdge(decoratedTypeAnnotation('Int operator+').node,
+ decoratedTypeAnnotation('Int f').node,
+ hard: false));
}
test_binaryExpression_add_result_not_null() async {
@@ -353,8 +343,8 @@
int f(int i, int j) => i + j;
''');
- assertNullCheck(
- checkExpression('j;'), decoratedTypeAnnotation('int j').node);
+ assertNullCheck(checkExpression('j;'),
+ assertEdge(decoratedTypeAnnotation('int j').node, never, hard: true));
}
test_binaryExpression_add_right_check_custom() async {
@@ -366,8 +356,10 @@
''');
assertNullCheck(
- checkExpression('j/*check*/'), decoratedTypeAnnotation('Int j').node,
- contextNode: decoratedTypeAnnotation('Int other').node);
+ checkExpression('j/*check*/'),
+ assertEdge(decoratedTypeAnnotation('Int j').node,
+ decoratedTypeAnnotation('Int other').node,
+ hard: true));
}
test_binaryExpression_equal() async {
@@ -408,7 +400,7 @@
var nullable_b = decoratedTypeAnnotation('bool b').node;
var check_b = checkExpression('b ?');
- assertNullCheck(check_b, nullable_b);
+ assertNullCheck(check_b, assertEdge(nullable_b, never, hard: true));
}
test_conditionalExpression_general() async {
@@ -423,8 +415,8 @@
var nullable_conditional = decoratedExpressionType('(b ?').node;
assertConditional(nullable_conditional, nullable_i, nullable_j);
var nullable_return = decoratedTypeAnnotation('int f').node;
- assertNullCheck(checkExpression('(b ? i : j)'), nullable_conditional,
- contextNode: nullable_return);
+ assertNullCheck(checkExpression('(b ? i : j)'),
+ assertEdge(nullable_conditional, nullable_return, hard: false));
}
test_conditionalExpression_left_non_null() async {
@@ -496,8 +488,10 @@
''');
assertNullCheck(
- checkExpression('i/*3*/'), decoratedTypeAnnotation('int/*2*/').node,
- contextNode: decoratedTypeAnnotation('int/*1*/').node);
+ checkExpression('i/*3*/'),
+ assertEdge(decoratedTypeAnnotation('int/*2*/').node,
+ decoratedTypeAnnotation('int/*1*/').node,
+ hard: true));
}
test_functionDeclaration_parameter_named_default_notNull() async {
@@ -585,7 +579,7 @@
var int_1 = decoratedTypeAnnotation('int/*1*/');
var int_2 = decoratedTypeAnnotation('int/*2*/');
var i_3 = checkExpression('i/*3*/');
- assertNullCheck(i_3, int_2.node, contextNode: int_1.node);
+ assertNullCheck(i_3, assertEdge(int_2.node, int_1.node, hard: true));
assertEdge(int_2.node, int_1.node, hard: true);
}
@@ -598,8 +592,8 @@
''');
var nullable_i = decoratedTypeAnnotation('int i').node;
var nullable_j = decoratedTypeAnnotation('int j').node;
- assertNullCheck(checkExpression('j/*check*/'), nullable_j,
- contextNode: nullable_i);
+ assertNullCheck(checkExpression('j/*check*/'),
+ assertEdge(nullable_j, nullable_i, hard: true));
}
test_functionInvocation_parameter_named_missing() async {
@@ -638,8 +632,8 @@
}
''');
- assertNullCheck(checkExpression('null'), always,
- contextNode: decoratedTypeAnnotation('int').node);
+ assertNullCheck(checkExpression('null'),
+ assertEdge(always, decoratedTypeAnnotation('int').node, hard: false));
}
test_functionInvocation_return() async {
@@ -651,8 +645,10 @@
''');
assertNullCheck(
- checkExpression('(f())'), decoratedTypeAnnotation('int/*1*/').node,
- contextNode: decoratedTypeAnnotation('int/*2*/').node);
+ checkExpression('(f())'),
+ assertEdge(decoratedTypeAnnotation('int/*1*/').node,
+ decoratedTypeAnnotation('int/*2*/').node,
+ hard: false));
}
test_if_condition() async {
@@ -662,8 +658,8 @@
}
''');
- assertNullCheck(
- checkExpression('b) {}'), decoratedTypeAnnotation('bool b').node);
+ assertNullCheck(checkExpression('b) {}'),
+ assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: true));
}
test_if_conditional_control_flow_after() async {
@@ -708,10 +704,12 @@
var nullable_j = decoratedTypeAnnotation('int j').node;
var nullable_k = decoratedTypeAnnotation('int k').node;
var nullable_return = decoratedTypeAnnotation('int f').node;
- assertNullCheck(checkExpression('j/*check*/'), nullable_j,
- contextNode: nullable_return, guards: [nullable_i]);
- assertNullCheck(checkExpression('k/*check*/'), nullable_k,
- contextNode: nullable_return);
+ assertNullCheck(
+ checkExpression('j/*check*/'),
+ assertEdge(nullable_j, nullable_return,
+ guards: [nullable_i], hard: false));
+ assertNullCheck(checkExpression('k/*check*/'),
+ assertEdge(nullable_k, nullable_return, hard: false));
var discard = statementDiscard('if (i == null)');
expect(discard.trueGuard, same(nullable_i));
expect(discard.falseGuard, null);
@@ -732,10 +730,10 @@
var nullable_i = decoratedTypeAnnotation('int i').node;
var nullable_j = decoratedTypeAnnotation('int j').node;
var nullable_return = decoratedTypeAnnotation('int f').node;
- assertNullCheck(checkExpression('i/*check*/'), nullable_i,
- contextNode: nullable_return);
- assertNullCheck(checkExpression('j/*check*/'), nullable_j,
- contextNode: nullable_return);
+ assertNullCheck(checkExpression('i/*check*/'),
+ assertEdge(nullable_i, nullable_return, hard: false));
+ assertNullCheck(checkExpression('j/*check*/'),
+ assertEdge(nullable_j, nullable_return, hard: false));
}
test_if_without_else() async {
@@ -750,8 +748,8 @@
var nullable_i = decoratedTypeAnnotation('int i').node;
var nullable_return = decoratedTypeAnnotation('int f').node;
- assertNullCheck(checkExpression('i/*check*/'), nullable_i,
- contextNode: nullable_return);
+ assertNullCheck(checkExpression('i/*check*/'),
+ assertEdge(nullable_i, nullable_return, hard: false));
}
test_indexExpression_index() async {
@@ -797,7 +795,8 @@
}
int f(C c) => c[0];
''');
- assertNullCheck(checkExpression('c['), decoratedTypeAnnotation('C c').node);
+ assertNullCheck(checkExpression('c['),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true));
}
test_indexExpression_target_check_cascaded() async {
@@ -807,8 +806,8 @@
}
C f(C c) => c..[0];
''');
- assertNullCheck(
- checkExpression('c..['), decoratedTypeAnnotation('C c').node);
+ assertNullCheck(checkExpression('c..['),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true));
}
test_indexExpression_target_demonstrates_non_null_intent() async {
@@ -991,11 +990,11 @@
var nullable_t = decoratedTypeAnnotation('T t').node;
var check_i = checkExpression('i/*check*/');
var nullable_c_t_or_nullable_t =
- check_i.contextNode as NullabilityNodeForSubstitution;
+ check_i.edges.single.destinationNode as NullabilityNodeForSubstitution;
expect(nullable_c_t_or_nullable_t.innerNode, same(nullable_c_t));
expect(nullable_c_t_or_nullable_t.outerNode, same(nullable_t));
- assertNullCheck(check_i, nullable_i,
- contextNode: nullable_c_t_or_nullable_t);
+ assertNullCheck(check_i,
+ assertEdge(nullable_i, nullable_c_t_or_nullable_t, hard: true));
}
test_methodInvocation_parameter_generic() async {
@@ -1010,9 +1009,11 @@
assertEdge(decoratedTypeAnnotation('int/*3*/').node,
decoratedTypeAnnotation('int/*1*/').node,
hard: false);
- assertNullCheck(checkExpression('c/*check*/'),
- decoratedTypeAnnotation('C<int/*3*/>/*4*/').node,
- contextNode: decoratedTypeAnnotation('C<int/*1*/>/*2*/').node);
+ assertNullCheck(
+ checkExpression('c/*check*/'),
+ assertEdge(decoratedTypeAnnotation('C<int/*3*/>/*4*/').node,
+ decoratedTypeAnnotation('C<int/*1*/>/*2*/').node,
+ hard: true));
}
test_methodInvocation_parameter_named() async {
@@ -1026,8 +1027,8 @@
''');
var nullable_i = decoratedTypeAnnotation('int i').node;
var nullable_j = decoratedTypeAnnotation('int j').node;
- assertNullCheck(checkExpression('j/*check*/'), nullable_j,
- contextNode: nullable_i);
+ assertNullCheck(checkExpression('j/*check*/'),
+ assertEdge(nullable_j, nullable_i, hard: true));
}
test_methodInvocation_return_type() async {
@@ -1066,8 +1067,8 @@
}
''');
- assertNullCheck(
- checkExpression('c.m'), decoratedTypeAnnotation('C c').node);
+ assertNullCheck(checkExpression('c.m'),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true));
}
test_methodInvocation_target_check_cascaded() async {
@@ -1080,8 +1081,8 @@
}
''');
- assertNullCheck(
- checkExpression('c..m'), decoratedTypeAnnotation('C c').node);
+ assertNullCheck(checkExpression('c..m'),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true));
}
test_methodInvocation_target_demonstrates_non_null_intent() async {
@@ -1123,8 +1124,8 @@
}
''');
- assertNullCheck(checkExpression('(null)'), always,
- contextNode: decoratedTypeAnnotation('int').node);
+ assertNullCheck(checkExpression('(null)'),
+ assertEdge(always, decoratedTypeAnnotation('int').node, hard: false));
}
test_prefixedIdentifier_field_type() async {
@@ -1161,8 +1162,8 @@
}
''');
- assertNullCheck(
- checkExpression('c.x'), decoratedTypeAnnotation('C c').node);
+ assertNullCheck(checkExpression('c.x'),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: true));
}
test_prefixedIdentifier_target_demonstrates_non_null_intent() async {
@@ -1187,7 +1188,7 @@
var nullable_b = decoratedTypeAnnotation('bool b').node;
var check_b = checkExpression('b;');
- assertNullCheck(check_b, nullable_b);
+ assertNullCheck(check_b, assertEdge(nullable_b, never, hard: true));
var return_f = decoratedTypeAnnotation('bool f').node;
assertEdge(never, return_f, hard: false);
@@ -1229,8 +1230,9 @@
}
''');
- assertNullCheck(
- checkExpression('c).x'), decoratedTypeAnnotation('C c').node);
+ // TODO(paulberry): this is wrong. It should be a hard edge.
+ assertNullCheck(checkExpression('c).x'),
+ assertEdge(decoratedTypeAnnotation('C c').node, never, hard: false));
}
test_return_implicit_null() async {
@@ -1251,8 +1253,8 @@
}
''');
- assertNullCheck(checkExpression('null'), always,
- contextNode: decoratedTypeAnnotation('int').node);
+ assertNullCheck(checkExpression('null'),
+ assertEdge(always, decoratedTypeAnnotation('int').node, hard: false));
}
test_return_null_generic() async {
@@ -1265,7 +1267,8 @@
''');
var tNode = decoratedTypeAnnotation('T f').node;
assertEdge(always, tNode, hard: false);
- assertNullCheck(checkExpression('null'), always, contextNode: tNode);
+ assertNullCheck(
+ checkExpression('null'), assertEdge(always, tNode, hard: false));
}
test_simpleIdentifier_local() async {
@@ -1413,8 +1416,9 @@
return testUnit;
}
- void assertEdge(NullabilityNode source, NullabilityNode destination,
- {@required bool hard}) {
+ NullabilityEdge assertEdge(
+ NullabilityNode source, NullabilityNode destination,
+ {@required bool hard, List<NullabilityNode> guards}) {
var edges = getEdges(source, destination);
if (edges.length == 0) {
fail('Expected edge $source -> $destination, found none');
@@ -1423,6 +1427,10 @@
} else {
var edge = edges[0];
expect(edge.hard, hard);
+ if (guards != null) {
+ expect(edge.guards, unorderedEquals(guards));
+ }
+ return edge;
}
}
