Skip to content

Commit e403e6a

Browse files
committed
Flow analysis: rework handling of equality tests.
The new logic classifies the LHS and RHS of the equality check as either null, non-nullable, or potentially nullable, and then either promotes, treats the expression as equivalent to a boolean, or does nothing, as appropriate. This means, for example, that a comparison between a non-nullable value and `null` is now known to evaluate to `true` for reachability analysis. Note: as part of this change, it was tempting to trigger promotion whenever a varialbe is equality compared to an expression of type `Null`, but this would be unsound (consider `(int? x) => x == (x = null) ? true : x.isEven`). So we still only promote when the variable is compared to a literal `null`. Fixes #41985. There's a corresponding spec change out for review: dart-lang/language#1134 Change-Id: Id7f1d4eaa3b0fa57124445bb8352eef32c304feb Bug: #41985 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/155926 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com>
1 parent 2adaa2f commit e403e6a

File tree

18 files changed

+623
-96
lines changed

18 files changed

+623
-96
lines changed

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

Lines changed: 73 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -363,11 +363,12 @@ abstract class FlowAnalysis<Node, Statement extends Node, Expression, Variable,
363363

364364
/// Call this method just after visiting a binary `==` or `!=` expression.
365365
void equalityOp_end(Expression wholeExpression, Expression rightOperand,
366+
Type rightOperandType,
366367
{bool notEqual = false});
367368

368369
/// Call this method just after visiting the left hand side of a binary `==`
369370
/// or `!=` expression.
370-
void equalityOp_rightBegin(Expression leftOperand);
371+
void equalityOp_rightBegin(Expression leftOperand, Type leftOperandType);
371372

372373
/// This method should be called at the conclusion of flow analysis for a top
373374
/// level function or method. Performs assertion checks.
@@ -811,17 +812,20 @@ class FlowAnalysisDebug<Node, Statement extends Node, Expression, Variable,
811812

812813
@override
813814
void equalityOp_end(Expression wholeExpression, Expression rightOperand,
815+
Type rightOperandType,
814816
{bool notEqual = false}) {
815817
_wrap(
816-
'equalityOp_end($wholeExpression, $rightOperand, notEqual: $notEqual)',
817-
() => _wrapped.equalityOp_end(wholeExpression, rightOperand,
818+
'equalityOp_end($wholeExpression, $rightOperand, $rightOperandType, '
819+
'notEqual: $notEqual)',
820+
() => _wrapped.equalityOp_end(
821+
wholeExpression, rightOperand, rightOperandType,
818822
notEqual: notEqual));
819823
}
820824

821825
@override
822-
void equalityOp_rightBegin(Expression leftOperand) {
823-
_wrap('equalityOp_rightBegin($leftOperand)',
824-
() => _wrapped.equalityOp_rightBegin(leftOperand));
826+
void equalityOp_rightBegin(Expression leftOperand, Type leftOperandType) {
827+
_wrap('equalityOp_rightBegin($leftOperand, $leftOperandType)',
828+
() => _wrapped.equalityOp_rightBegin(leftOperand, leftOperandType));
825829
}
826830

827831
@override
@@ -1651,8 +1655,26 @@ class FlowModel<Variable, Type> {
16511655
}
16521656
}
16531657

1658+
/// Enum representing the different classifications of types that can be
1659+
/// returned by [TypeOperations.classifyType].
1660+
enum TypeClassification {
1661+
/// The type is `Null` or an equivalent type (e.g. `Never?`)
1662+
nullOrEquivalent,
1663+
1664+
/// The type is a potentially nullable type, but not equivalent to `Null`
1665+
/// (e.g. `int?`, or a type variable whose bound is potentially nullable)
1666+
potentiallyNullable,
1667+
1668+
/// The type is a non-nullable type.
1669+
nonNullable,
1670+
}
1671+
16541672
/// Operations on types, abstracted from concrete type interfaces.
16551673
abstract class TypeOperations<Variable, Type> {
1674+
/// Classifies the given type into one of the three categories defined by
1675+
/// the [TypeClassification] enum.
1676+
TypeClassification classifyType(Type type);
1677+
16561678
/// Returns the "remainder" of [from] when [what] has been removed from
16571679
/// consideration by an instance check.
16581680
Type factor(Type from, Type what);
@@ -2277,6 +2299,21 @@ class _ConditionalContext<Variable, Type>
22772299
'thenInfo: $_thenInfo)';
22782300
}
22792301

2302+
/// [_FlowContext] representing an equality comparison using `==` or `!=`.
2303+
class _EqualityOpContext<Variable, Type> extends _BranchContext {
2304+
/// The type of the expression on the LHS of `==` or `!=`.
2305+
final Type _leftOperandType;
2306+
2307+
_EqualityOpContext(
2308+
ExpressionInfo<Variable, Type> conditionInfo, this._leftOperandType)
2309+
: super(conditionInfo);
2310+
2311+
@override
2312+
String toString() =>
2313+
'_EqualityOpContext(conditionInfo: $_conditionInfo, lhsType: '
2314+
'$_leftOperandType)';
2315+
}
2316+
22802317
class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
22812318
Type> implements FlowAnalysis<Node, Statement, Expression, Variable, Type> {
22822319
/// The [TypeOperations], used to access types, and check subtyping.
@@ -2420,31 +2457,49 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
24202457

24212458
@override
24222459
void equalityOp_end(Expression wholeExpression, Expression rightOperand,
2460+
Type rightOperandType,
24232461
{bool notEqual = false}) {
2424-
_BranchContext<Variable, Type> context =
2425-
_stack.removeLast() as _BranchContext<Variable, Type>;
2462+
_EqualityOpContext<Variable, Type> context =
2463+
_stack.removeLast() as _EqualityOpContext<Variable, Type>;
24262464
ExpressionInfo<Variable, Type> lhsInfo = context._conditionInfo;
2465+
Type leftOperandType = context._leftOperandType;
24272466
ExpressionInfo<Variable, Type> rhsInfo = _getExpressionInfo(rightOperand);
2428-
Variable variable;
2429-
if (lhsInfo is _NullInfo<Variable, Type> &&
2467+
ExpressionInfo<Variable, Type> equalityInfo;
2468+
TypeClassification leftOperandTypeClassification =
2469+
typeOperations.classifyType(leftOperandType);
2470+
TypeClassification rightOperandTypeClassification =
2471+
typeOperations.classifyType(rightOperandType);
2472+
if (leftOperandTypeClassification == TypeClassification.nullOrEquivalent &&
2473+
rightOperandTypeClassification == TypeClassification.nullOrEquivalent) {
2474+
return booleanLiteral(wholeExpression, !notEqual);
2475+
} else if ((leftOperandTypeClassification ==
2476+
TypeClassification.nullOrEquivalent &&
2477+
rightOperandTypeClassification == TypeClassification.nonNullable) ||
2478+
(rightOperandTypeClassification ==
2479+
TypeClassification.nullOrEquivalent &&
2480+
leftOperandTypeClassification == TypeClassification.nonNullable)) {
2481+
return booleanLiteral(wholeExpression, notEqual);
2482+
} else if (lhsInfo is _NullInfo<Variable, Type> &&
24302483
rhsInfo is _VariableReadInfo<Variable, Type>) {
2431-
variable = rhsInfo._variable;
2484+
assert(
2485+
leftOperandTypeClassification == TypeClassification.nullOrEquivalent);
2486+
equalityInfo =
2487+
_current.tryMarkNonNullable(typeOperations, rhsInfo._variable);
24322488
} else if (rhsInfo is _NullInfo<Variable, Type> &&
24332489
lhsInfo is _VariableReadInfo<Variable, Type>) {
2434-
variable = lhsInfo._variable;
2490+
equalityInfo =
2491+
_current.tryMarkNonNullable(typeOperations, lhsInfo._variable);
24352492
} else {
24362493
return;
24372494
}
2438-
ExpressionInfo<Variable, Type> expressionInfo =
2439-
_current.tryMarkNonNullable(typeOperations, variable);
24402495
_storeExpressionInfo(wholeExpression,
2441-
notEqual ? expressionInfo : ExpressionInfo.invert(expressionInfo));
2496+
notEqual ? equalityInfo : ExpressionInfo.invert(equalityInfo));
24422497
}
24432498

24442499
@override
2445-
void equalityOp_rightBegin(Expression leftOperand) {
2446-
_stack.add(
2447-
new _BranchContext<Variable, Type>(_getExpressionInfo(leftOperand)));
2500+
void equalityOp_rightBegin(Expression leftOperand, Type leftOperandType) {
2501+
_stack.add(new _EqualityOpContext<Variable, Type>(
2502+
_getExpressionInfo(leftOperand), leftOperandType));
24482503
}
24492504

24502505
@override

pkg/_fe_analyzer_shared/test/flow_analysis/definite_unassignment/data/binary_expression.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ ifNull_left() {
88
v;
99
}
1010

11-
ifNull_right(int a) {
11+
ifNull_right(int? a) {
1212
late int v;
1313
a ?? (v = 0);
1414
v;

0 commit comments

Comments
 (0)