Skip to content

Commit 1540a33

Browse files
stereotype441Commit Queue
authored and
Commit Queue
committed
[sound flow analysis] Implement behaviors for null-aware map entries.
It turns out that the flow analysis logic for null-aware map entries has always presumed sound null safety. That is, given a map entry with a null-aware key (`{?x: y}`), flow analysis assumed that if the key was non-nullable, then the value was guaranteed to execute. However, another piece of logic that could have been implemented, and wasn't, was that if the key had static type `Null`, then the value was guaranteed _not_ to execute. This logic would have been sound even without assuming sound null safety (because even in unsound null safety mode, the type `Null` was only inhabited by the value `null`). This change implements the missing logic. Even though it doesn't strictly depend on the assumption sound null safety, it still makes sense to guard it by the `sound-flow-analysis` flag, because (a) it's a potentially breaking change, and (b) it brings the flow analysis behavior of null-aware map entries into alignment with the other behaviors that are being implemented as part of the `sound-flow-analysis` feature. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: Ie711f582660a31a411be0dc339995df140feb04f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420800 Reviewed-by: Chloe Stefantsova <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]>
1 parent 0bf9183 commit 1540a33

File tree

2 files changed

+69
-2
lines changed

2 files changed

+69
-2
lines changed

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

+15-2
Original file line numberDiff line numberDiff line change
@@ -5196,8 +5196,21 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
51965196
} else {
51975197
shortcutState = _current;
51985198
}
5199-
if (operations.classifyType(keyType) == TypeClassification.nonNullable) {
5200-
shortcutState = shortcutState.setUnreachable();
5199+
switch (operations.classifyType(keyType)) {
5200+
case TypeClassification.nonNullable:
5201+
// The control flow path that skips the value expression is unreachable.
5202+
shortcutState = shortcutState.setUnreachable();
5203+
case TypeClassification.nullOrEquivalent:
5204+
// The control flow path containing the value expression is unreachable.
5205+
// This functionality was added as part of the `sound-flow-analysis`
5206+
// language feature, even though it would have been a sound reasoning
5207+
// step before then.
5208+
if (typeAnalyzerOptions.soundFlowAnalysisEnabled) {
5209+
_current = _current.setUnreachable();
5210+
}
5211+
case TypeClassification.potentiallyNullable:
5212+
// Both control flow paths are reachable.
5213+
break;
52015214
}
52025215
_stack.add(new _NullAwareMapEntryContext<Type>(shortcutState));
52035216
}

pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart

+54
Original file line numberDiff line numberDiff line change
@@ -10968,6 +10968,60 @@ main() {
1096810968
]);
1096910969
});
1097010970
});
10971+
10972+
group('{ ?<nonNullable>: <expr> }', () {
10973+
test('When enabled, guaranteed to execute <expr>', () {
10974+
var x = Var('x');
10975+
h.run([
10976+
declare(x, type: 'int'),
10977+
mapLiteral(keyType: 'dynamic', valueType: 'dynamic', [
10978+
mapEntry(expr('int'), x.write(expr('int')), isKeyNullAware: true),
10979+
]),
10980+
checkAssigned(x, true),
10981+
]);
10982+
});
10983+
10984+
test('When disabled, guaranteed to execute <expr>', () {
10985+
// Flow analysis has considered `{ ?<nonNullable>: <expr> }` as
10986+
// guaranteed to execute <expr> since null-aware map entries were added
10987+
// to the language.
10988+
h.disableSoundFlowAnalysis();
10989+
var x = Var('x');
10990+
h.run([
10991+
declare(x, type: 'int'),
10992+
mapLiteral(keyType: 'dynamic', valueType: 'dynamic', [
10993+
mapEntry(expr('int'), x.write(expr('int')), isKeyNullAware: true),
10994+
]),
10995+
checkAssigned(x, true),
10996+
]);
10997+
});
10998+
});
10999+
11000+
group('{ ?<Null>: <expr> }', () {
11001+
test('When enabled, guaranteed to skip execution of <expr>', () {
11002+
h.run([
11003+
mapLiteral(keyType: 'dynamic', valueType: 'dynamic', [
11004+
mapEntry(expr('Null'), checkReachable(false), isKeyNullAware: true),
11005+
]),
11006+
]);
11007+
});
11008+
11009+
test('When disabled, not guaranteed to skip execution of <expr>', () {
11010+
// Note: it would always have been sound for flow analysis to reason
11011+
// that `{ ?<Null>: <expr> }` was guaranteed to skip execution of
11012+
// `<expr>` (even when flow analysis had to assume that code might be
11013+
// running in unsound null safety mode). But this functionality wasn't
11014+
// implemented. It's been added as part of `sound-flow-analysis`; this
11015+
// test verifies that the old behavior is preserved when
11016+
// `sound-flow-analysis` is disabled.
11017+
h.disableSoundFlowAnalysis();
11018+
h.run([
11019+
mapLiteral(keyType: 'dynamic', valueType: 'dynamic', [
11020+
mapEntry(expr('Null'), checkReachable(true), isKeyNullAware: true),
11021+
]),
11022+
]);
11023+
});
11024+
});
1097111025
});
1097211026
}
1097311027

0 commit comments

Comments
 (0)