Skip to content

Commit

Permalink
[nnbd_migration] Handle if elements in lists.
Browse files Browse the repository at this point in the history
Change-Id: Ia892668cc31db2453e24778067b7b2c857813482
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/112644
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Mike Fairhurst <mfairhurst@google.com>
  • Loading branch information
MichaelRFairhurst authored and commit-bot@chromium.org committed Aug 12, 2019
1 parent c536510 commit 5d2c17d
Show file tree
Hide file tree
Showing 3 changed files with 176 additions and 9 deletions.
69 changes: 60 additions & 9 deletions pkg/nnbd_migration/lib/src/edge_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
/// return statements.
DecoratedType _currentFunctionType;

/// The [DecoratedType] of the innermost list or set literal being visited, or
/// `null` if the visitor is not inside any function or method.
///
/// This is needed to construct the appropriate nullability constraints for
/// ui as code list elements.
DecoratedType _currentLiteralType;

/// Information about the most recently visited binary expression whose
/// boolean value could possibly affect nullability analysis.
_ConditionInfo _conditionInfo;
Expand Down Expand Up @@ -555,6 +562,44 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
node.typeArguments, calleeType, null);
}

@override
DecoratedType visitIfElement(IfElement node) {
_handleAssignment(node.condition, _notNullType);
NullabilityNode trueGuard;
NullabilityNode falseGuard;
if (identical(_conditionInfo?.condition, node.condition)) {
trueGuard = _conditionInfo.trueGuard;
falseGuard = _conditionInfo.falseGuard;
_variables.recordConditionalDiscard(_source, node,
ConditionalDiscard(trueGuard, falseGuard, _conditionInfo.isPure));
}
if (trueGuard != null) {
_guards.add(trueGuard);
}
try {
_postDominatedLocals.doScoped(
action: () => _handleCollectionElement(node.thenElement));
} finally {
if (trueGuard != null) {
_guards.removeLast();
}
}
if (node.elseElement != null) {
if (falseGuard != null) {
_guards.add(falseGuard);
}
try {
_postDominatedLocals.doScoped(
action: () => _handleCollectionElement(node.elseElement));
} finally {
if (falseGuard != null) {
_guards.removeLast();
}
}
}
return null;
}

@override
DecoratedType visitIfStatement(IfStatement node) {
// TODO(paulberry): should the use of a boolean in an if-statement be
Expand Down Expand Up @@ -681,15 +726,12 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
if (typeArgumentType == null) {
_unimplemented(node, 'Could not compute type argument type');
}
for (var element in node.elements) {
if (element is Expression) {
_handleAssignment(element, typeArgumentType);
} else {
// Handle spread and control flow elements.
element.accept(this);
// TODO(brianwilkerson)
_unimplemented(node, 'Spread or control flow element');
}
final previousLiteralType = _currentLiteralType;
try {
_currentLiteralType = typeArgumentType;
node.elements.forEach(_handleCollectionElement);
} finally {
_currentLiteralType = previousLiteralType;
}
return DecoratedType(listType, _graph.never,
typeArguments: [typeArgumentType]);
Expand Down Expand Up @@ -1206,6 +1248,15 @@ $stackTrace''');
return sourceType;
}

DecoratedType _handleCollectionElement(CollectionElement element) {
if (element is Expression) {
assert(_currentLiteralType != null);
return _handleAssignment(element, _currentLiteralType);
} else {
return element.accept(this);
}
}

void _handleConstructorRedirection(
FormalParameterList parameters, ConstructorName redirectedConstructor) {
var callee = redirectedConstructor.staticElement;
Expand Down
25 changes: 25 additions & 0 deletions pkg/nnbd_migration/test/api_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1938,6 +1938,31 @@ main() {
await _checkSingleFileChanges(content, expected);
}

@failingTest
test_removed_if_element_doesnt_introduce_nullability() async {
// Failing for two reasons: 1. we don't add ! to recover(), and 2. we get
// an unimplemented error.
var content = '''
f(int x) {
<int>[if (x == null) recover(), 0];
}
int recover() {
assert(false);
return null;
}
''';
var expected = '''
f(int x) {
<int>[if (x == null) recover()!, 0];
}
int? recover() {
assert(false);
return null;
}
''';
await _checkSingleFileChanges(content, expected);
}

test_single_file_multiple_changes() async {
var content = '''
int f() => null;
Expand Down
91 changes: 91 additions & 0 deletions pkg/nnbd_migration/test/edge_builder_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1707,6 +1707,76 @@ void f(bool b, int i) {
assertNoEdge(always, decoratedTypeAnnotation('int i').node);
}

test_if_element() async {
await analyze('''
void f(bool b) {
int i1 = null;
int i2 = null;
<int>[if (b) i1 else i2];
}
''');

assertNullCheck(checkExpression('b) i1'),
assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: true));
assertEdge(decoratedTypeAnnotation('int i1').node,
decoratedTypeAnnotation('int>[').node,
hard: false);
assertEdge(decoratedTypeAnnotation('int i2').node,
decoratedTypeAnnotation('int>[').node,
hard: false);
}

@failingTest
test_if_element_guard_equals_null() async {
// failing because of an unimplemented exception in conditional modification
await analyze('''
dynamic f(int i, int j, int k) {
<int>[if (i == null) j/*check*/ else k/*check*/];
}
''');
var nullable_i = decoratedTypeAnnotation('int i').node;
var nullable_j = decoratedTypeAnnotation('int j').node;
var nullable_k = decoratedTypeAnnotation('int k').node;
var nullable_itemType = decoratedTypeAnnotation('int>[').node;
assertNullCheck(
checkExpression('j/*check*/'),
assertEdge(nullable_j, nullable_itemType,
guards: [nullable_i], hard: false));
assertNullCheck(checkExpression('k/*check*/'),
assertEdge(nullable_k, nullable_itemType, hard: false));
var discard = statementDiscard('if (i == null)');
expect(discard.trueGuard, same(nullable_i));
expect(discard.falseGuard, null);
expect(discard.pureCondition, true);
}

test_if_element_nested() async {
await analyze('''
void f(bool b1, bool b2) {
int i1 = null;
int i2 = null;
int i3 = null;
<int>[if (b1) if (b2) i1 else i2 else i3];
}
''');

assertNullCheck(checkExpression('b1)'),
assertEdge(decoratedTypeAnnotation('bool b1').node, never, hard: true));
assertNullCheck(
checkExpression('b2) i1'),
assertEdge(decoratedTypeAnnotation('bool b2').node, never,
hard: false));
assertEdge(decoratedTypeAnnotation('int i1').node,
decoratedTypeAnnotation('int>[').node,
hard: false);
assertEdge(decoratedTypeAnnotation('int i2').node,
decoratedTypeAnnotation('int>[').node,
hard: false);
assertEdge(decoratedTypeAnnotation('int i3').node,
decoratedTypeAnnotation('int>[').node,
hard: false);
}

test_if_guard_equals_null() async {
await analyze('''
int f(int i, int j, int k) {
Expand Down Expand Up @@ -2695,6 +2765,27 @@ void test(bool b1, C c1, C c2, C c3) {
assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: false));
}

test_postDominators_ifElement() async {
await analyze('''
class C {
int m() => 0;
}
void test(bool b, C c1, C c2, C c3) {
<int>[if (b) c1.m() else c2.m()];
c3.m();
}
''');

assertNullCheck(checkExpression('b)'),
assertEdge(decoratedTypeAnnotation('bool b').node, never, hard: true));
assertNullCheck(checkExpression('c1.m'),
assertEdge(decoratedTypeAnnotation('C c1').node, never, hard: false));
assertNullCheck(checkExpression('c2.m'),
assertEdge(decoratedTypeAnnotation('C c2').node, never, hard: false));
assertNullCheck(checkExpression('c3.m'),
assertEdge(decoratedTypeAnnotation('C c3').node, never, hard: true));
}

test_postDominators_ifStatement_conditional() async {
await analyze('''
class C {
Expand Down

0 comments on commit 5d2c17d

Please sign in to comment.