Skip to content

Commit ab16d79

Browse files
committed
Revert "Flow analysis changes to fix mixed-mode unsoundness loophole."
This reverts commit d833f2f. Reason for revert: Broke build, e.g. https://ci.chromium.org/p/dart/builders/ci/dart-sdk-mac/12688 Original change's description: > Flow analysis changes to fix mixed-mode unsoundness loophole. > > This is the flow analysis portion of the fix to > dart-lang/language#1143. Follow-up changes > will be needed in the CFE and/or backends to ensure that exceptions > are thrown under appropriate circumstances. > > This CL also makes some improvements to flow analysis's reachability > analysis so that it accounts for nullability of the target when > analyzing the reachability of `??=` and `?.`. Hopefully these > improvements should make the fix to > dart-lang/language#1143 clearer and more > consistent. > > Change-Id: I5fa5c070f13fd57ac4c2fb87f2d67588861594b0 > Bug: dart-lang/language#1143 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/160440 > Reviewed-by: Konstantin Shcheglov <[email protected]> > Reviewed-by: Johnni Winther <[email protected]> > Commit-Queue: Paul Berry <[email protected]> [email protected],[email protected],[email protected] Change-Id: If1215b19975e0958d612dd69767088095d853879 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: dart-lang/language#1143 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/161580 Reviewed-by: Paul Berry <[email protected]>
1 parent 7363adc commit ab16d79

File tree

18 files changed

+88
-918
lines changed

18 files changed

+88
-918
lines changed

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

Lines changed: 26 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -362,16 +362,7 @@ abstract class FlowAnalysis<Node, Statement extends Node, Expression, Variable,
362362
void doStatement_end(Expression condition);
363363

364364
/// Call this method just after visiting a binary `==` or `!=` expression.
365-
///
366-
/// Return value indicates whether flow analysis believes that a successful
367-
/// equality check is reachable. If `false` is returned, the client should
368-
/// ensure that the `==` test behaves like `x == y && throw ...`.
369-
///
370-
/// Note that if `notEqual` is `true`, then the return value describes the
371-
/// behavior of the underlying `==` test. So if `notEqual` is `true` and
372-
/// `false` is returned, the client should ensure that the `!=` test behaves
373-
/// like `!(x == y && throw ...)`.
374-
bool equalityOp_end(Expression wholeExpression, Expression rightOperand,
365+
void equalityOp_end(Expression wholeExpression, Expression rightOperand,
375366
Type rightOperandType,
376367
{bool notEqual = false});
377368

@@ -484,13 +475,7 @@ abstract class FlowAnalysis<Node, Statement extends Node, Expression, Variable,
484475

485476
/// Call this method after visiting the LHS of an if-null expression ("??")
486477
/// or if-null assignment ("??=").
487-
///
488-
/// Return value indicates whether flow analysis believes that the right hand
489-
/// side is reachable. If `false` is returned, the client should ensure that
490-
/// `x ?? y` behaves like `x ?? throw ...` (or, correspondingly, that
491-
/// `x ??= y` behaves like `x ??= throw ...`).
492-
bool ifNullExpression_rightBegin(
493-
Expression leftHandSide, Type leftHandSideType);
478+
void ifNullExpression_rightBegin(Expression leftHandSide);
494479

495480
/// Call this method after visiting the "then" part of an if statement, and
496481
/// before visiting the "else" part.
@@ -526,16 +511,7 @@ abstract class FlowAnalysis<Node, Statement extends Node, Expression, Variable,
526511
/// be the expression to which the "is" check was applied. [isNot] should be
527512
/// a boolean indicating whether this is an "is" or an "is!" expression.
528513
/// [type] should be the type being checked.
529-
///
530-
/// Return value indicates whether flow analysis believes that a failure of
531-
/// the `is` test is reachable. If `false` is returned, the client should
532-
/// ensure that the `is` test behaves like `x is T || throw ...`.
533-
///
534-
/// Note that if `isNot` is `true`, then the return value describes the
535-
/// behavior of the underlying `if` test. So if `isNot` is `true` and `false`
536-
/// is returned, the client should ensure that the `is!` test behaves like
537-
/// `!(x is T || throw ...)`.
538-
bool isExpression_end(
514+
void isExpression_end(
539515
Expression isExpression, Expression subExpression, bool isNot, Type type);
540516

541517
/// Return whether the [variable] is definitely unassigned in the current
@@ -582,22 +558,12 @@ abstract class FlowAnalysis<Node, Statement extends Node, Expression, Variable,
582558
/// [target] should be the expression just before the null-aware operator, or
583559
/// `null` if the null-aware access starts a cascade section.
584560
///
585-
/// [targetType] should be the type of the expression just before the
586-
/// null-aware operator, and should be non-null even if the null-aware access
587-
/// starts a cascade section.
588-
///
589561
/// Note that [nullAwareAccess_end] should be called after the conclusion
590562
/// of any null-shorting that is caused by the `?.`. So, for example, if the
591563
/// code being analyzed is `x?.y?.z(x)`, [nullAwareAccess_rightBegin] should
592564
/// be called once upon reaching each `?.`, but [nullAwareAccess_end] should
593565
/// not be called until after processing the method call to `z(x)`.
594-
///
595-
/// Return value indicates whether flow analysis believes that a null target
596-
/// is reachable. If `false` is returned, the client should ensure that
597-
/// `x?.y` behaves like `x!.y`. (Note that this is necessary even if `y`
598-
/// exists on `Object`--see
599-
/// https://github.com/dart-lang/language/issues/1143#issuecomment-682096575.)
600-
bool nullAwareAccess_rightBegin(Expression target, Type targetType);
566+
void nullAwareAccess_rightBegin(Expression target);
601567

602568
/// Call this method when encountering an expression that is a `null` literal.
603569
void nullLiteral(Expression expression);
@@ -845,17 +811,15 @@ class FlowAnalysisDebug<Node, Statement extends Node, Expression, Variable,
845811
}
846812

847813
@override
848-
bool equalityOp_end(Expression wholeExpression, Expression rightOperand,
814+
void equalityOp_end(Expression wholeExpression, Expression rightOperand,
849815
Type rightOperandType,
850816
{bool notEqual = false}) {
851-
return _wrap(
817+
_wrap(
852818
'equalityOp_end($wholeExpression, $rightOperand, $rightOperandType, '
853819
'notEqual: $notEqual)',
854820
() => _wrapped.equalityOp_end(
855821
wholeExpression, rightOperand, rightOperandType,
856-
notEqual: notEqual),
857-
isQuery: true,
858-
isPure: false);
822+
notEqual: notEqual));
859823
}
860824

861825
@override
@@ -938,14 +902,9 @@ class FlowAnalysisDebug<Node, Statement extends Node, Expression, Variable,
938902
}
939903

940904
@override
941-
bool ifNullExpression_rightBegin(
942-
Expression leftHandSide, Type leftHandSideType) {
943-
return _wrap(
944-
'ifNullExpression_rightBegin($leftHandSide, $leftHandSideType)',
945-
() => _wrapped.ifNullExpression_rightBegin(
946-
leftHandSide, leftHandSideType),
947-
isQuery: true,
948-
isPure: false);
905+
void ifNullExpression_rightBegin(Expression leftHandSide) {
906+
return _wrap('ifNullExpression_rightBegin($leftHandSide)',
907+
() => _wrapped.ifNullExpression_rightBegin(leftHandSide));
949908
}
950909

951910
@override
@@ -972,14 +931,12 @@ class FlowAnalysisDebug<Node, Statement extends Node, Expression, Variable,
972931
}
973932

974933
@override
975-
bool isExpression_end(Expression isExpression, Expression subExpression,
934+
void isExpression_end(Expression isExpression, Expression subExpression,
976935
bool isNot, Type type) {
977-
return _wrap(
936+
_wrap(
978937
'isExpression_end($isExpression, $subExpression, $isNot, $type)',
979-
() =>
980-
_wrapped.isExpression_end(isExpression, subExpression, isNot, type),
981-
isQuery: true,
982-
isPure: false);
938+
() => _wrapped.isExpression_end(
939+
isExpression, subExpression, isNot, type));
983940
}
984941

985942
@override
@@ -1034,10 +991,9 @@ class FlowAnalysisDebug<Node, Statement extends Node, Expression, Variable,
1034991
}
1035992

1036993
@override
1037-
bool nullAwareAccess_rightBegin(Expression target, Type targetType) {
1038-
return _wrap('nullAwareAccess_rightBegin($target, $targetType)',
1039-
() => _wrapped.nullAwareAccess_rightBegin(target, targetType),
1040-
isQuery: true, isPure: false);
994+
void nullAwareAccess_rightBegin(Expression target) {
995+
_wrap('nullAwareAccess_rightBegin($target)',
996+
() => _wrapped.nullAwareAccess_rightBegin(target));
1041997
}
1042998

1043999
@override
@@ -2500,7 +2456,7 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
25002456
}
25012457

25022458
@override
2503-
bool equalityOp_end(Expression wholeExpression, Expression rightOperand,
2459+
void equalityOp_end(Expression wholeExpression, Expression rightOperand,
25042460
Type rightOperandType,
25052461
{bool notEqual = false}) {
25062462
_EqualityOpContext<Variable, Type> context =
@@ -2515,16 +2471,14 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
25152471
typeOperations.classifyType(rightOperandType);
25162472
if (leftOperandTypeClassification == TypeClassification.nullOrEquivalent &&
25172473
rightOperandTypeClassification == TypeClassification.nullOrEquivalent) {
2518-
booleanLiteral(wholeExpression, !notEqual);
2519-
return true;
2474+
return booleanLiteral(wholeExpression, !notEqual);
25202475
} else if ((leftOperandTypeClassification ==
25212476
TypeClassification.nullOrEquivalent &&
25222477
rightOperandTypeClassification == TypeClassification.nonNullable) ||
25232478
(rightOperandTypeClassification ==
25242479
TypeClassification.nullOrEquivalent &&
25252480
leftOperandTypeClassification == TypeClassification.nonNullable)) {
2526-
booleanLiteral(wholeExpression, notEqual);
2527-
return false;
2481+
return booleanLiteral(wholeExpression, notEqual);
25282482
} else if (lhsInfo is _NullInfo<Variable, Type> &&
25292483
rhsInfo is _VariableReadInfo<Variable, Type>) {
25302484
assert(
@@ -2536,11 +2490,10 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
25362490
equalityInfo =
25372491
_current.tryMarkNonNullable(typeOperations, lhsInfo._variable);
25382492
} else {
2539-
return true;
2493+
return;
25402494
}
25412495
_storeExpressionInfo(wholeExpression,
25422496
notEqual ? equalityInfo : ExpressionInfo.invert(equalityInfo));
2543-
return equalityInfo.ifFalse.reachable;
25442497
}
25452498

25462499
@override
@@ -2666,8 +2619,7 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
26662619
}
26672620

26682621
@override
2669-
bool ifNullExpression_rightBegin(
2670-
Expression leftHandSide, Type leftHandSideType) {
2622+
void ifNullExpression_rightBegin(Expression leftHandSide) {
26712623
ExpressionInfo<Variable, Type> lhsInfo = _getExpressionInfo(leftHandSide);
26722624
FlowModel<Variable, Type> promoted;
26732625
if (lhsInfo is _VariableReadInfo<Variable, Type>) {
@@ -2678,12 +2630,7 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
26782630
} else {
26792631
promoted = _current;
26802632
}
2681-
if (typeOperations.classifyType(leftHandSideType) ==
2682-
TypeClassification.nonNullable) {
2683-
_current = _current.setReachable(false);
2684-
}
26852633
_stack.add(new _SimpleContext<Variable, Type>(promoted));
2686-
return _current.reachable;
26872634
}
26882635

26892636
@override
@@ -2723,21 +2670,20 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
27232670
}
27242671

27252672
@override
2726-
bool isExpression_end(Expression isExpression, Expression subExpression,
2673+
void isExpression_end(Expression isExpression, Expression subExpression,
27272674
bool isNot, Type type) {
27282675
ExpressionInfo<Variable, Type> subExpressionInfo =
27292676
_getExpressionInfo(subExpression);
27302677
Variable variable;
27312678
if (subExpressionInfo is _VariableReadInfo<Variable, Type>) {
27322679
variable = subExpressionInfo._variable;
27332680
} else {
2734-
return true;
2681+
return;
27352682
}
27362683
ExpressionInfo<Variable, Type> expressionInfo =
27372684
_current.tryPromoteForTypeCheck(typeOperations, variable, type);
27382685
_storeExpressionInfo(isExpression,
27392686
isNot ? ExpressionInfo.invert(expressionInfo) : expressionInfo);
2740-
return expressionInfo.ifFalse.reachable;
27412687
}
27422688

27432689
@override
@@ -2814,16 +2760,8 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
28142760
}
28152761

28162762
@override
2817-
bool nullAwareAccess_rightBegin(Expression target, Type targetType) {
2818-
assert(targetType != null);
2819-
bool shortingIsReachable = true;
2820-
FlowModel<Variable, Type> shortingModel = _current;
2821-
if (typeOperations.classifyType(targetType) ==
2822-
TypeClassification.nonNullable) {
2823-
shortingModel = shortingModel.setReachable(false);
2824-
shortingIsReachable = false;
2825-
}
2826-
_stack.add(new _SimpleContext<Variable, Type>(shortingModel));
2763+
void nullAwareAccess_rightBegin(Expression target) {
2764+
_stack.add(new _SimpleContext<Variable, Type>(_current));
28272765
if (target != null) {
28282766
ExpressionInfo<Variable, Type> targetInfo = _getExpressionInfo(target);
28292767
if (targetInfo is _VariableReadInfo<Variable, Type>) {
@@ -2832,7 +2770,6 @@ class _FlowAnalysisImpl<Node, Statement extends Node, Expression, Variable,
28322770
.ifTrue;
28332771
}
28342772
}
2835-
return shortingIsReachable;
28362773
}
28372774

28382775
@override

0 commit comments

Comments
 (0)