Skip to content

Commit 4ce0cf3

Browse files
stereotype441Commit Queue
authored andcommitted
[sound flow analysis] Implement behaviors for null-aware accesses.
This change updates the flow analysis logic for `??` and `??=` expressions, so that when the language feature `sound-flow-analysis` is enabled, the static type of the left hand side is checked for nullability. If it's non-nullable, then the right hand side of the expresison is considered unreachable. These new behaviors break assumptions made by two pre-existing flow analysis tests. I changed those tests to run with `sound-flow-analysis` disabled. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: I33d6d256bd3c41b764245f50ad34eb5c8b33878e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420740 Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]> Reviewed-by: Chloe Stefantsova <[email protected]>
1 parent ce10ab5 commit 4ce0cf3

File tree

2 files changed

+32
-17
lines changed

2 files changed

+32
-17
lines changed

pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -4896,25 +4896,21 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
48964896
} else {
48974897
shortcutState = _current;
48984898
}
4899-
if (operations.classifyType(leftHandSideType) ==
4900-
TypeClassification.nullOrEquivalent) {
4901-
shortcutState = shortcutState.setUnreachable();
4899+
switch (operations.classifyType(leftHandSideType)) {
4900+
case TypeClassification.nullOrEquivalent:
4901+
// The control path that skips the "if null" code is unreachable.
4902+
shortcutState = shortcutState.setUnreachable();
4903+
case TypeClassification.nonNullable:
4904+
// The control path containing the "if null" code is unreachable,
4905+
// assuming sound null safety.
4906+
if (typeAnalyzerOptions.soundFlowAnalysisEnabled) {
4907+
_current = _current.setUnreachable();
4908+
}
4909+
case TypeClassification.potentiallyNullable:
4910+
// Both control flow paths are reachable.
4911+
break;
49024912
}
49034913
_stack.add(new _IfNullExpressionContext<Type>(shortcutState));
4904-
// Note: we are now on the RHS of the `??`, and so at this point in the
4905-
// flow, it is known that the LHS evaluated to `null`. It's tempting to
4906-
// update `_current` to reflect this (either promoting the type of the LHS,
4907-
// if it's a variable reference, or marking the flow as unreachable, if the
4908-
// LHS had a non-nullable static type). However:
4909-
// - In the case where the LHS was a variable reference, we can't promote
4910-
// it, because we don't promote to `Null` (see
4911-
// https://github.com/dart-lang/language/issues/1505#issuecomment-975706918)
4912-
// - In the case where the LHS had a non-nullable static type, it still
4913-
// might have been `null` due to mixed-mode unsoundness, so we can't mark
4914-
// the flow as unreachable without allowing the unsoundness to escalate
4915-
// (see https://github.com/dart-lang/language/issues/1143)
4916-
//
4917-
// So we just leave `_current` as is.
49184914
}
49194915

49204916
@override

pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1292,6 +1292,8 @@ main() {
12921292
});
12931293

12941294
test('ifNullExpression does not detect when RHS is unreachable', () {
1295+
// Note: sound flow analysis changes this behavior.
1296+
h.disableSoundFlowAnalysis();
12951297
h.run([
12961298
expr('int')
12971299
.ifNull(second(checkReachable(true), expr('int')))
@@ -1321,6 +1323,8 @@ main() {
13211323
test(
13221324
'ifNullExpression sets shortcut reachability correctly for non-null '
13231325
'type', () {
1326+
// Note: sound flow analysis changes this behavior.
1327+
h.disableSoundFlowAnalysis();
13241328
h.run([
13251329
expr('Object')
13261330
.ifNull(second(checkReachable(true), throw_(expr('Object'))))
@@ -10949,6 +10953,21 @@ main() {
1094910953
]);
1095010954
});
1095110955
});
10956+
10957+
group('<nonNullable> ?? <expr>', () {
10958+
test('When enabled, <expr> is dead', () {
10959+
h.run([
10960+
expr('int').ifNull(checkReachable(false)),
10961+
]);
10962+
});
10963+
10964+
test('When disabled, <expr> is live', () {
10965+
h.disableSoundFlowAnalysis();
10966+
h.run([
10967+
expr('int').ifNull(checkReachable(true)),
10968+
]);
10969+
});
10970+
});
1095210971
});
1095310972
}
1095410973

0 commit comments

Comments
 (0)