Skip to content

Commit 6c5c9d3

Browse files
stereotype441Commit Queue
authored and
Commit Queue
committed
[sound flow analysis] Implement behaviors for is and as.
This change updates the flow analysis logic for `is` and `as` expressions so that when the language feature `sound-flow-analysis` is enabled, the static type of the operand is compared to the type to the right of the `is` or `as` keyword. If one of the types is non-nullable and the other type is `Null`, then the type test is known to fail. For an `as` expression, this means that the code path following the expression will be marked as unreachable. For an `is` expression, this means that any code paths that assume it evaluates to `true` will be marked as unreachable. Note that these new behaviors break assumptions made by three pre-existing flow analysis tests. I was able to adjust one of the tests ("equalityOp_end does not set reachability for `this`") to preserve its old behavior. The other two tests became redundant, so I removed them. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: Ib3a9e96bd39cf7df4c6c297568763c0f25bc9e39 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420164 Reviewed-by: Konstantin Shcheglov <[email protected]> Reviewed-by: Chloe Stefantsova <[email protected]> Commit-Queue: Paul Berry <[email protected]>
1 parent 6863a4b commit 6c5c9d3

File tree

11 files changed

+242
-56
lines changed

11 files changed

+242
-56
lines changed

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

+69-22
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,10 @@ abstract class FlowAnalysis<Node extends Object, Statement extends Node,
210210
/// Call this method after visiting an "as" expression.
211211
///
212212
/// [subExpression] should be the expression to which the "as" check was
213-
/// applied. [type] should be the type being checked.
214-
void asExpression_end(Expression subExpression, Type type);
213+
/// applied, and [subExpressionType] should be its static type. [castType]
214+
/// should be the type being cast to.
215+
void asExpression_end(Expression subExpression,
216+
{required Type subExpressionType, required Type castType});
215217

216218
/// Call this method after visiting the condition part of an assert statement
217219
/// (or assert initializer).
@@ -659,11 +661,13 @@ abstract class FlowAnalysis<Node extends Object, Statement extends Node,
659661
/// Call this method after visiting the LHS of an "is" expression.
660662
///
661663
/// [isExpression] should be the complete expression. [subExpression] should
662-
/// be the expression to which the "is" check was applied. [isNot] should be
663-
/// a boolean indicating whether this is an "is" or an "is!" expression.
664-
/// [type] should be the type being checked.
664+
/// be the expression to which the "is" check was applied, and
665+
/// [subExpressionType] should be its static type. [isNot] should be a
666+
/// boolean indicating whether this is an "is" or an "is!" expression.
667+
/// [checkedType] should be the type being checked.
665668
void isExpression_end(
666-
Expression isExpression, Expression subExpression, bool isNot, Type type);
669+
Expression isExpression, Expression subExpression, bool isNot,
670+
{required Type subExpressionType, required Type checkedType});
667671

668672
/// Return whether the [variable] is definitely unassigned in the current
669673
/// state.
@@ -1214,9 +1218,13 @@ class FlowAnalysisDebug<Node extends Object, Statement extends Node,
12141218
FlowAnalysisOperations<Variable, Type> get operations => _wrapped.operations;
12151219

12161220
@override
1217-
void asExpression_end(Expression subExpression, Type type) {
1218-
_wrap('asExpression_end($subExpression, $type)',
1219-
() => _wrapped.asExpression_end(subExpression, type));
1221+
void asExpression_end(Expression subExpression,
1222+
{required Type subExpressionType, required Type castType}) {
1223+
_wrap(
1224+
'asExpression_end($subExpression, subExpressionType: '
1225+
'$subExpressionType, castType: $castType)',
1226+
() => _wrapped.asExpression_end(subExpression,
1227+
subExpressionType: subExpressionType, castType: castType));
12201228
}
12211229

12221230
@override
@@ -1573,12 +1581,14 @@ class FlowAnalysisDebug<Node extends Object, Statement extends Node,
15731581
}
15741582

15751583
@override
1576-
void isExpression_end(Expression isExpression, Expression subExpression,
1577-
bool isNot, Type type) {
1584+
void isExpression_end(
1585+
Expression isExpression, Expression subExpression, bool isNot,
1586+
{required Type subExpressionType, required Type checkedType}) {
15781587
_wrap(
1579-
'isExpression_end($isExpression, $subExpression, $isNot, $type)',
1580-
() => _wrapped.isExpression_end(
1581-
isExpression, subExpression, isNot, type));
1588+
'isExpression_end($isExpression, $subExpression, $isNot, '
1589+
'subExpressionType: $subExpressionType, checkedType: $checkedType)',
1590+
() => _wrapped.isExpression_end(isExpression, subExpression, isNot,
1591+
subExpressionType: subExpressionType, checkedType: checkedType));
15821592
}
15831593

15841594
@override
@@ -4290,7 +4300,7 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
42904300
implements
42914301
FlowAnalysis<Node, Statement, Expression, Variable, Type>,
42924302
_PropertyTargetHelper<Expression, Type> {
4293-
/// Options affecting the behavior of flow analysis.
4303+
/// Language features enables affecting the behavior of flow analysis.
42944304
final TypeAnalyzerOptions typeAnalyzerOptions;
42954305

42964306
/// The [FlowAnalysisOperations], used to access types, check subtyping, and
@@ -4413,10 +4423,18 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
44134423
FlowAnalysisTypeOperations<Type> get typeOperations => operations;
44144424

44154425
@override
4416-
void asExpression_end(Expression subExpression, Type type) {
4426+
void asExpression_end(Expression subExpression,
4427+
{required Type subExpressionType, required Type castType}) {
4428+
// Depending on types, flow analysis may be able to prove that the `as`
4429+
// expression is guaranteed to fail.
4430+
if (_isTypeCheckGuaranteedToFailWithSoundNullSafety(
4431+
staticType: subExpressionType, checkedType: castType)) {
4432+
_current = _current.setUnreachable();
4433+
}
4434+
44174435
_Reference<Type>? reference = _getExpressionReference(subExpression);
44184436
if (reference == null) return;
4419-
_current = _current.tryPromoteForTypeCast(this, reference, type);
4437+
_current = _current.tryPromoteForTypeCast(this, reference, castType);
44204438
}
44214439

44224440
@override
@@ -4950,16 +4968,19 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
49504968
}
49514969

49524970
@override
4953-
void isExpression_end(Expression isExpression, Expression subExpression,
4954-
bool isNot, Type type) {
4955-
if (operations.isBottomType(type)) {
4971+
void isExpression_end(
4972+
Expression isExpression, Expression subExpression, bool isNot,
4973+
{required Type subExpressionType, required Type checkedType}) {
4974+
if (operations.isBottomType(checkedType) ||
4975+
_isTypeCheckGuaranteedToFailWithSoundNullSafety(
4976+
staticType: subExpressionType, checkedType: checkedType)) {
49564977
booleanLiteral(isExpression, isNot);
49574978
} else {
49584979
_Reference<Type>? subExpressionReference =
49594980
_getExpressionReference(subExpression);
49604981
if (subExpressionReference != null) {
4961-
ExpressionInfo<Type> expressionInfo =
4962-
_current.tryPromoteForTypeCheck(this, subExpressionReference, type);
4982+
ExpressionInfo<Type> expressionInfo = _current.tryPromoteForTypeCheck(
4983+
this, subExpressionReference, checkedType);
49634984
_storeExpressionInfo(
49644985
isExpression, isNot ? expressionInfo._invert() : expressionInfo);
49654986
}
@@ -6063,6 +6084,32 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
60636084
}
60646085
}
60656086

6087+
/// Determines whether an expression having the given [staticType] is
6088+
/// guaranteed to fail an `is` or `as` check using [checkedType] due to sound
6089+
/// null safety.
6090+
///
6091+
/// If [TypeAnalyzerOptions.soundFlowAnalysisEnabled] is `false`, this method
6092+
/// will return `false` regardless of its input. This reflects the fact that
6093+
/// in language versions prior to the introduction of sound flow analysis,
6094+
/// flow analysis assumed that the program might be executing in unsound null
6095+
/// safety mode.
6096+
bool _isTypeCheckGuaranteedToFailWithSoundNullSafety(
6097+
{required Type staticType, required Type checkedType}) {
6098+
if (!typeAnalyzerOptions.soundFlowAnalysisEnabled) return false;
6099+
switch (typeOperations.classifyType(staticType)) {
6100+
case TypeClassification.nonNullable
6101+
when typeOperations.classifyType(checkedType) ==
6102+
TypeClassification.nullOrEquivalent:
6103+
case TypeClassification.nullOrEquivalent
6104+
when typeOperations.classifyType(checkedType) ==
6105+
TypeClassification.nonNullable:
6106+
// Guaranteed to fail due to nullability mismatch.
6107+
return true;
6108+
default:
6109+
return false;
6110+
}
6111+
}
6112+
60666113
FlowModel<Type> _join(FlowModel<Type>? first, FlowModel<Type>? second) =>
60676114
FlowModel.join(this, first, second);
60686115

pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart

+3
Original file line numberDiff line numberDiff line change
@@ -2744,11 +2744,14 @@ class TypeAnalyzerOptions {
27442744

27452745
final bool inferenceUpdate4Enabled;
27462746

2747+
final bool soundFlowAnalysisEnabled;
2748+
27472749
TypeAnalyzerOptions({
27482750
required this.patternsEnabled,
27492751
required this.inferenceUpdate3Enabled,
27502752
required this.respectImplicitlyTypedVarInitializers,
27512753
required this.fieldPromotionEnabled,
27522754
required this.inferenceUpdate4Enabled,
2755+
required this.soundFlowAnalysisEnabled,
27532756
});
27542757
}

pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart

+129-22
Original file line numberDiff line numberDiff line change
@@ -564,7 +564,7 @@ main() {
564564
h.thisType = 'C';
565565
h.addSuperInterfaces('C', (_) => [Type('Object')]);
566566
h.run([
567-
if_(this_.is_('Null'), [
567+
if_(this_.is_('Null', isInverted: true), [
568568
if_(this_.eq(nullLiteral), [
569569
checkReachable(true),
570570
], [
@@ -1693,27 +1693,6 @@ main() {
16931693
_checkIs('int', 'String', null, null, inverted: true);
16941694
});
16951695

1696-
test('isExpression_end does nothing if applied to a non-variable', () {
1697-
h.run([
1698-
if_(expr('Null').is_('int'), [
1699-
checkReachable(true),
1700-
], [
1701-
checkReachable(true),
1702-
]),
1703-
]);
1704-
});
1705-
1706-
test('isExpression_end does nothing if applied to a non-variable, inverted',
1707-
() {
1708-
h.run([
1709-
if_(expr('Null').isNot('int'), [
1710-
checkReachable(true),
1711-
], [
1712-
checkReachable(true),
1713-
]),
1714-
]);
1715-
});
1716-
17171696
test('isExpression_end() does not promote write-captured vars', () {
17181697
var x = Var('x');
17191698
h.run([
@@ -10667,6 +10646,134 @@ main() {
1066710646
});
1066810647
});
1066910648
});
10649+
10650+
group('Sound null safety:', () {
10651+
group('<nonNull> as Null:', () {
10652+
test('When enabled, is guaranteed to throw', () {
10653+
h.run([
10654+
expr('int').as_('Null'),
10655+
checkReachable(false),
10656+
]);
10657+
});
10658+
10659+
test('When disabled, no effect', () {
10660+
h.disableSoundFlowAnalysis();
10661+
h.run([
10662+
expr('int').as_('Null'),
10663+
checkReachable(true),
10664+
]);
10665+
});
10666+
});
10667+
10668+
group('<Null> as <nonNullable>:', () {
10669+
test('When enabled, is guaranteed to throw', () {
10670+
h.run([
10671+
expr('Null').as_('int'),
10672+
checkReachable(false),
10673+
]);
10674+
});
10675+
10676+
test('When disabled, no effect', () {
10677+
h.disableSoundFlowAnalysis();
10678+
h.run([
10679+
expr('Null').as_('int'),
10680+
checkReachable(true),
10681+
]);
10682+
});
10683+
});
10684+
10685+
group('<nonNull> is Null:', () {
10686+
test('When enabled, is guaranteed false', () {
10687+
h.run([
10688+
if_(expr('int').is_('Null'), [
10689+
checkReachable(false),
10690+
], [
10691+
checkReachable(true),
10692+
]),
10693+
]);
10694+
});
10695+
10696+
test('When disabled, no effect', () {
10697+
h.disableSoundFlowAnalysis();
10698+
h.run([
10699+
if_(expr('int').is_('Null'), [
10700+
checkReachable(true),
10701+
], [
10702+
checkReachable(true),
10703+
]),
10704+
]);
10705+
});
10706+
});
10707+
10708+
group('<nonNull> is! Null:', () {
10709+
test('When enabled, is guaranteed false', () {
10710+
h.run([
10711+
if_(expr('int').is_('Null', isInverted: true), [
10712+
checkReachable(true),
10713+
], [
10714+
checkReachable(false),
10715+
]),
10716+
]);
10717+
});
10718+
10719+
test('When disabled, no effect', () {
10720+
h.disableSoundFlowAnalysis();
10721+
h.run([
10722+
if_(expr('int').is_('Null', isInverted: true), [
10723+
checkReachable(true),
10724+
], [
10725+
checkReachable(true),
10726+
]),
10727+
]);
10728+
});
10729+
});
10730+
10731+
group('<Null> is <nonNullable>:', () {
10732+
test('When enabled, is guaranteed false', () {
10733+
h.run([
10734+
if_(expr('Null').is_('int'), [
10735+
checkReachable(false),
10736+
], [
10737+
checkReachable(true),
10738+
]),
10739+
]);
10740+
});
10741+
10742+
test('When disabled, no effect', () {
10743+
h.disableSoundFlowAnalysis();
10744+
h.run([
10745+
if_(expr('Null').is_('int'), [
10746+
checkReachable(true),
10747+
], [
10748+
checkReachable(true),
10749+
]),
10750+
]);
10751+
});
10752+
});
10753+
10754+
group('<Null> is! <nonNullable>:', () {
10755+
test('When enabled, is guaranteed false', () {
10756+
h.run([
10757+
if_(expr('Null').is_('int', isInverted: true), [
10758+
checkReachable(true),
10759+
], [
10760+
checkReachable(false),
10761+
]),
10762+
]);
10763+
});
10764+
10765+
test('When disabled, no effect', () {
10766+
h.disableSoundFlowAnalysis();
10767+
h.run([
10768+
if_(expr('Null').is_('int', isInverted: true), [
10769+
checkReachable(true),
10770+
], [
10771+
checkReachable(true),
10772+
]),
10773+
]);
10774+
});
10775+
});
10776+
});
1067010777
}
1067110778

1067210779
/// Returns the appropriate matcher for expecting an assertion error to be

0 commit comments

Comments
 (0)