Skip to content

Commit eec56c0

Browse files
stereotype441Commit Queue
stereotype441
authored and
Commit Queue
committed
[flow analysis] Mark false branches of trivial is tests unreachable.
When an `is` test is trivially satisfied (i.e. `expr is T`, when the static type of `expr` is a subtype of `T`), the `is` test is guaranteed by soundness to evaluate to `true`, so any code path that follows from the `is` test evaluating to `false` is unreachable. This reasoning wasn't valid prior to sound null safety, because in mixed mode programs, it was possible for an expression to evaluate to `null` even if its static type wasn't nullable, and hence `expr is T` might evaluate to `false` even if the static type of `expr` was a subtype of `T`. So this change is gated on the `sound-flow-analysis` language flag (which is enabled in Dart 3.9). Fixes #60718. Change-Id: I66a65580b738162f23b6fb468b71fcac66bfbb95 Bug: #60718 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/431740 Commit-Queue: Paul Berry <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>
1 parent 27952c5 commit eec56c0

File tree

4 files changed

+413
-6
lines changed

4 files changed

+413
-6
lines changed

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

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3137,10 +3137,16 @@ class FlowModel<Type extends Object> {
31373137

31383138
Type factoredType = helper.typeOperations.factor(previousType, type);
31393139
Type? typeIfFalse;
3140+
bool ifFalseIsUnreachable = false;
31403141
if (helper.typeOperations.isBottomType(factoredType)) {
3141-
// Promoting to `Never` would mark the code as unreachable. But it might
3142-
// be reachable due to mixed mode unsoundness. So don't promote.
3142+
// Do not promote to `Never` (even if it would be sound to do so); it's
3143+
// not useful.
31433144
typeIfFalse = null;
3145+
if (helper.typeAnalyzerOptions.soundFlowAnalysisEnabled) {
3146+
ifFalseIsUnreachable = true;
3147+
} else {
3148+
// The code path might be reachable due to mixed mode unsoundness.
3149+
}
31443150
} else if (!helper.isValidPromotionStep(
31453151
previousType: previousType,
31463152
newType: factoredType,
@@ -3158,6 +3164,10 @@ class FlowModel<Type extends Object> {
31583164
typeIfFalse,
31593165
);
31603166

3167+
if (ifFalseIsUnreachable) {
3168+
ifFalse = ifFalse.setUnreachable();
3169+
}
3170+
31613171
return new ExpressionInfo<Type>(
31623172
type: helper.boolType,
31633173
ifTrue: ifTrue,
@@ -5791,6 +5801,11 @@ class _FlowAnalysisImpl<
57915801
isExpression,
57925802
isNot ? expressionInfo._invert() : expressionInfo,
57935803
);
5804+
} else if (_isTypeCheckGuaranteedToSucceedWithSoundNullSafety(
5805+
staticType: subExpressionType,
5806+
checkedType: checkedType,
5807+
)) {
5808+
booleanLiteral(isExpression, !isNot);
57945809
}
57955810
}
57965811
}
@@ -7229,6 +7244,23 @@ class _FlowAnalysisImpl<
72297244
}
72307245
}
72317246

7247+
/// Determines whether an expression having the given [staticType] is
7248+
/// guaranteed to fail an `is` or `as` check using [checkedType] due to sound
7249+
/// null safety.
7250+
///
7251+
/// If [TypeAnalyzerOptions.soundFlowAnalysisEnabled] is `false`, this method
7252+
/// will return `false` regardless of its input. This reflects the fact that
7253+
/// in language versions prior to the introduction of sound flow analysis,
7254+
/// flow analysis assumed that the program might be executing in unsound null
7255+
/// safety mode.
7256+
bool _isTypeCheckGuaranteedToSucceedWithSoundNullSafety({
7257+
required Type staticType,
7258+
required Type checkedType,
7259+
}) {
7260+
if (!typeAnalyzerOptions.soundFlowAnalysisEnabled) return false;
7261+
return typeOperations.isSubtypeOf(staticType, checkedType);
7262+
}
7263+
72327264
FlowModel<Type> _join(FlowModel<Type>? first, FlowModel<Type>? second) =>
72337265
FlowModel.join(this, first, second);
72347266

‎pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart‎

Lines changed: 59 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1611,6 +1611,8 @@ main() {
16111611
String? expectedPromotedTypeThen,
16121612
String? expectedPromotedTypeElse, {
16131613
bool inverted = false,
1614+
bool expectedReachableThen = true,
1615+
bool expectedReachableElse = true,
16141616
}) {
16151617
var x = Var('x');
16161618
late SsaNode<SharedTypeView> ssaBeforePromotion;
@@ -1620,12 +1622,12 @@ main() {
16201622
if_(
16211623
x.is_(tryPromoteType, isInverted: inverted),
16221624
[
1623-
checkReachable(true),
1625+
checkReachable(expectedReachableThen),
16241626
checkPromoted(x, expectedPromotedTypeThen),
16251627
getSsaNodes((nodes) => expect(nodes[x], same(ssaBeforePromotion))),
16261628
],
16271629
[
1628-
checkReachable(true),
1630+
checkReachable(expectedReachableElse),
16291631
checkPromoted(x, expectedPromotedTypeElse),
16301632
getSsaNodes((nodes) => expect(nodes[x], same(ssaBeforePromotion))),
16311633
],
@@ -1642,11 +1644,18 @@ main() {
16421644
});
16431645

16441646
test('isExpression_end does not promote to a supertype', () {
1645-
_checkIs('int', 'int?', null, null);
1647+
_checkIs('int', 'int?', null, null, expectedReachableElse: false);
16461648
});
16471649

16481650
test('isExpression_end does not promote to a supertype, inverted', () {
1649-
_checkIs('int', 'int?', null, null, inverted: true);
1651+
_checkIs(
1652+
'int',
1653+
'int?',
1654+
null,
1655+
null,
1656+
inverted: true,
1657+
expectedReachableThen: false,
1658+
);
16501659
});
16511660

16521661
test('isExpression_end does not promote to an unrelated type', () {
@@ -12631,6 +12640,52 @@ main() {
1263112640
checkPromoted(x, 'num'),
1263212641
]);
1263312642
});
12643+
12644+
group('False branch for trivially satisfied "is" test:', () {
12645+
group('When enabled, sets unreachable:', () {
12646+
test('Promotable target', () {
12647+
var x = Var('x');
12648+
h.run([
12649+
declare(x, initializer: expr('int')),
12650+
if_(x.is_('int', isInverted: true), [
12651+
checkNotPromoted(x),
12652+
checkReachable(false),
12653+
]),
12654+
]);
12655+
});
12656+
12657+
test('Non-promotable target', () {
12658+
h.run([
12659+
if_(expr('int').is_('int', isInverted: true), [
12660+
checkReachable(false),
12661+
]),
12662+
]);
12663+
});
12664+
});
12665+
12666+
group('When disabled, leaves reachable:', () {
12667+
test('Promotable target', () {
12668+
h.disableSoundFlowAnalysis();
12669+
var x = Var('x');
12670+
h.run([
12671+
declare(x, initializer: expr('int')),
12672+
if_(x.is_('int', isInverted: true), [
12673+
checkNotPromoted(x),
12674+
checkReachable(true),
12675+
]),
12676+
]);
12677+
});
12678+
12679+
test('Non-promotable target', () {
12680+
h.disableSoundFlowAnalysis();
12681+
h.run([
12682+
if_(expr('int').is_('int', isInverted: true), [
12683+
checkReachable(true),
12684+
]),
12685+
]);
12686+
});
12687+
});
12688+
});
1263412689
});
1263512690

1263612691
group('Demotion and type of interest promotion:', () {

‎tests/language/sound_flow_analysis/trivially_satisfied_type_check_disabled_test.dart‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
// Exercises flow analysis of a trivially satisfied type check (an `is` test
6+
// that is guaranteed to succeed) when `sound-flow-analysis` is disabled.
7+
8+
// @dart = 3.8
9+
10+
import '../static_type_helper.dart';
11+
12+
// If `x` is of type `T`, flow analysis does not consider `x is T` to be
13+
// guaranteed to evaluate to `true`.
14+
testIsExact({required int intValue, required int Function() intFunction}) {
15+
{
16+
// <var> is int
17+
int? shouldBeDemoted1 = 0;
18+
int? shouldBeDemoted2 = 0;
19+
if (intValue is int) {
20+
shouldBeDemoted1 = null;
21+
} else {
22+
shouldBeDemoted2 = null;
23+
// `intValue` is not promoted to `Never`. Note: since
24+
// `Never.expectStaticType<anything>` will silently succeed, we check this
25+
// by first wrapping `intValue` in a list, and then checking the type of
26+
// the list.
27+
[intValue].expectStaticType<Exactly<List<int>>>();
28+
}
29+
shouldBeDemoted1.expectStaticType<Exactly<int?>>();
30+
shouldBeDemoted2.expectStaticType<Exactly<int?>>();
31+
}
32+
33+
{
34+
// <expr> is int
35+
int? shouldBeDemoted1 = 0;
36+
int? shouldBeDemoted2 = 0;
37+
if (intFunction() is int) {
38+
shouldBeDemoted1 = null;
39+
} else {
40+
shouldBeDemoted2 = null;
41+
}
42+
shouldBeDemoted1.expectStaticType<Exactly<int?>>();
43+
shouldBeDemoted2.expectStaticType<Exactly<int?>>();
44+
}
45+
}
46+
47+
// If `x` is of type `T`, flow analysis does not consider `x is! T` to be
48+
// guaranteed to evaluate to `false`.
49+
testIsNotExact({required int intValue, required int Function() intFunction}) {
50+
{
51+
// <var> is! int
52+
int? shouldBeDemoted1 = 0;
53+
int? shouldBeDemoted2 = 0;
54+
if (intValue is! int) {
55+
shouldBeDemoted1 = null;
56+
// `intValue` is not promoted to `Never`. Note: since
57+
// `Never.expectStaticType<anything>` will silently succeed, we check this
58+
// by first wrapping `intValue` in a list, and then checking the type of
59+
// the list.
60+
[intValue].expectStaticType<Exactly<List<int>>>();
61+
} else {
62+
shouldBeDemoted2 = null;
63+
}
64+
shouldBeDemoted1.expectStaticType<Exactly<int?>>();
65+
shouldBeDemoted2.expectStaticType<Exactly<int?>>();
66+
}
67+
68+
{
69+
// <expr> is! int
70+
int? shouldBeDemoted1 = 0;
71+
int? shouldBeDemoted2 = 0;
72+
if (intFunction() is! int) {
73+
shouldBeDemoted1 = null;
74+
} else {
75+
shouldBeDemoted2 = null;
76+
}
77+
shouldBeDemoted1.expectStaticType<Exactly<int?>>();
78+
shouldBeDemoted2.expectStaticType<Exactly<int?>>();
79+
}
80+
}
81+
82+
// If `x` is of type `T`, and `T <: U`, flow analysis does not consider `x is U`
83+
// to be guaranteed to evaluate to `true`.
84+
testIsSupertype({required int intValue, required int Function() intFunction}) {
85+
{
86+
// <var> is num
87+
int? shouldBeDemoted1 = 0;
88+
int? shouldBeDemoted2 = 0;
89+
if (intValue is num) {
90+
shouldBeDemoted1 = null;
91+
} else {
92+
shouldBeDemoted2 = null;
93+
// `intValue` is not promoted to `Never`. Note: since
94+
// `Never.expectStaticType<anything>` will silently succeed, we check this
95+
// by first wrapping `intValue` in a list, and then checking the type of
96+
// the list.
97+
[intValue].expectStaticType<Exactly<List<int>>>();
98+
}
99+
shouldBeDemoted1.expectStaticType<Exactly<int?>>();
100+
shouldBeDemoted2.expectStaticType<Exactly<int?>>();
101+
}
102+
103+
{
104+
// <expr> is num
105+
int? shouldBeDemoted1 = 0;
106+
int? shouldBeDemoted2 = 0;
107+
if (intFunction() is num) {
108+
shouldBeDemoted1 = null;
109+
} else {
110+
shouldBeDemoted2 = null;
111+
}
112+
shouldBeDemoted1.expectStaticType<Exactly<int?>>();
113+
shouldBeDemoted2.expectStaticType<Exactly<int?>>();
114+
}
115+
}
116+
117+
// If `x` is of type `T`, and `T <: U`, flow analysis does not consider `x is!
118+
// U` to be guaranteed to evaluate to `false`.
119+
testIsNotSupertype({
120+
required int intValue,
121+
required int Function() intFunction,
122+
}) {
123+
{
124+
// <var> is! num
125+
int? shouldBeDemoted1 = 0;
126+
int? shouldBeDemoted2 = 0;
127+
if (intValue is! num) {
128+
shouldBeDemoted1 = null;
129+
// `intValue` is not promoted to `Never`. Note: since
130+
// `Never.expectStaticType<anything>` will silently succeed, we check this
131+
// by first wrapping `intValue` in a list, and then checking the type of
132+
// the list.
133+
[intValue].expectStaticType<Exactly<List<int>>>();
134+
} else {
135+
shouldBeDemoted2 = null;
136+
}
137+
shouldBeDemoted1.expectStaticType<Exactly<int?>>();
138+
shouldBeDemoted2.expectStaticType<Exactly<int?>>();
139+
}
140+
141+
{
142+
// <expr> is! num
143+
int? shouldBeDemoted1 = 0;
144+
int? shouldBeDemoted2 = 0;
145+
if (intFunction() is! num) {
146+
shouldBeDemoted1 = null;
147+
} else {
148+
shouldBeDemoted2 = null;
149+
}
150+
shouldBeDemoted1.expectStaticType<Exactly<int?>>();
151+
shouldBeDemoted2.expectStaticType<Exactly<int?>>();
152+
}
153+
}
154+
155+
main() {
156+
testIsExact(intValue: 0, intFunction: () => 0);
157+
testIsNotExact(intValue: 0, intFunction: () => 0);
158+
testIsSupertype(intValue: 0, intFunction: () => 0);
159+
testIsNotSupertype(intValue: 0, intFunction: () => 0);
160+
}

0 commit comments

Comments
 (0)