Skip to content

Commit

Permalink
Flow analysis: properly model if/else nature of switches.
Browse files Browse the repository at this point in the history
A switch statement like this one:

    switch (E) {
      case P1 when G1:
        S1;
      case P2 when G2:
        S2;
      case P3 when G3:
    }

Is equivalent to an if/else chain like this:

    var tmp = E;
    if (tmp case P1 when G1) {
      S1;
    } else if (tmp case P2 when G2) {
      S2;
    } else if (tmp case P3 when G3) {
      S3;
    }

Therefore, if the failure of a particular pattern/guard combination to
match implies a type promotion, it makes sense for that promotion to
be carried into later cases.  For example:

    int? x = ...;
    switch (E) {
      case _ when x == null:
        break;
      default:
        x.isEven; // OK because `x` known to be non-null.
    }

This enabled some more thorough testing of type promotion in switches,
which then caught a bug introduced in a previous CL: when the switch
scrutinee is a variable reference, and we are trying to determine
whether it is safe for a pattern to promote the scrutinee variable, we
were checking the wrong SSA node to determine whether the variable had
been reassigned.  For example:

    Object x;
    switch (x) {
      case _ when f(x = ...);
        break;
      case int _:
        // `x` is not promoted to `int` because it is no longer the
	// same as the cached scrutinee.
        break;
    }

Bug: #50419
Change-Id: Ie8d6cf0fc662aa5ef0ac81eb2343952028dd2abb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/278533
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
  • Loading branch information
stereotype441 authored and Commit Queue committed Jan 9, 2023
1 parent 773bd2f commit 5b234e4
Show file tree
Hide file tree
Showing 6 changed files with 267 additions and 33 deletions.
102 changes: 75 additions & 27 deletions pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,9 @@ abstract class FlowAnalysis<Node extends Object, Statement extends Node,
/// Call this method just after visiting the initializer of a late variable.
void lateInitializer_end();

/// Call this method just before visiting the subpatterns of a list pattern.
void listPattern_begin();

/// Call this method before visiting the LHS of a logical binary operation
/// ("||" or "&&").
void logicalBinaryOp_begin();
Expand Down Expand Up @@ -1259,6 +1262,11 @@ class FlowAnalysisDebug<Node extends Object, Statement extends Node,
_wrap('lateInitializer_end()', () => _wrapped.lateInitializer_end());
}

@override
void listPattern_begin() {
_wrap('listPattern_begin()', () => _wrapped.listPattern_begin());
}

@override
void logicalBinaryOp_begin() {
_wrap('logicalBinaryOp_begin()', () => _wrapped.logicalBinaryOp_begin());
Expand Down Expand Up @@ -3556,7 +3564,7 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
// If there's a scrutinee, and its value is known to be the same as that of
// the synthetic cache variable, promote it too.
if (scrutineeReference != null &&
_current.infoFor(matchedValueReference.promotionKey).ssaNode ==
_current.infoFor(scrutineeReference.promotionKey).ssaNode ==
_scrutineeSsaNode) {
ifTrue = ifTrue
.tryPromoteForTypeCheck(this, scrutineeReference, staticType)
Expand Down Expand Up @@ -3982,6 +3990,14 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
functionExpression_end();
}

@override
void listPattern_begin() {
// As a temporary measure, just assume the pattern might or might not match.
// This avoids some bogus "unreachable code" warnings in analyzer tests.
// TODO(paulberry): replace this with a full implementation.
_unmatched = _join(_unmatched!, _current);
}

@override
void logicalBinaryOp_begin() {
_current = _current.split();
Expand Down Expand Up @@ -4181,16 +4197,17 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,

@override
void switchStatement_beginAlternative() {
assert(_stack.last is _SwitchAlternativesContext<Type>);
_SwitchAlternativesContext<Type> context =
_stack.last as _SwitchAlternativesContext<Type>;
_current = context._switchStatementContext._unmatched;
_pushPattern();
}

@override
void switchStatement_beginAlternatives() {
_SwitchStatementContext<Type> context =
_stack.last as _SwitchStatementContext<Type>;
_current = context._previous.split();
_stack.add(new _SwitchAlternativesContext<Type>(_current));
_stack.add(new _SwitchAlternativesContext<Type>(context));
}

@override
Expand Down Expand Up @@ -4219,30 +4236,29 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,

@override
void switchStatement_endAlternative(Expression? guard) {
// TODO(paulberry): make use of `unmatched`
// ignore: unused_local_variable
FlowModel<Type> unmatched = _popPattern(guard);
_SwitchAlternativesContext<Type> context =
_stack.last as _SwitchAlternativesContext<Type>;
// Future alternatives will be analyzed under the assumption that this
// alternative didn't match. This models the fact that a switch statement
// behaves like a chain of if/else tests.
context._switchStatementContext._unmatched = unmatched;
context._combinedModel = _join(context._combinedModel, _current);
_current = context._previous;
}

@override
void switchStatement_endAlternatives(Statement? node,
{required bool hasLabels}) {
_SwitchAlternativesContext<Type> alternativesContext =
_stack.removeLast() as _SwitchAlternativesContext<Type>;
_SimpleStatementContext<Type> switchContext =
_stack.last as _SimpleStatementContext<Type>;
_SwitchStatementContext<Type> switchContext =
_stack.last as _SwitchStatementContext<Type>;
if (hasLabels) {
AssignedVariablesNodeInfo info = _assignedVariables.getInfoForNode(node!);
_current = switchContext._previous
.conservativeJoin(this, info.written, info.captured);
} else {
_current =
(alternativesContext._combinedModel ?? alternativesContext._previous)
.unsplit();
_current = alternativesContext._combinedModel ?? switchContext._unmatched;
}
// TODO(paulberry): consider doing a split here if unreachable, and a join
// later, so that one case matching everything won't prevent promotion in
Expand Down Expand Up @@ -4438,13 +4454,35 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
@override
void _dumpState() {
print(' current: $_current');
print(' expressionWithInfo: $_expressionWithInfo');
print(' expressionInfo: $_expressionInfo');
print(' expressionWithReference: $_expressionWithReference');
print(' expressionReference: $_expressionReference');
print(' stack:');
for (_FlowContext stackEntry in _stack.reversed) {
print(' $stackEntry');
if (_unmatched != null) {
print(' unmatched: $_unmatched');
}
if (_scrutineeReference != null) {
print(' scrutineeReference: $_scrutineeReference');
}
if (_scrutineeSsaNode != null) {
print(' scrutineeSsaNode: $_scrutineeSsaNode');
}
if (_scrutineeType != null) {
print(' scrutineeType: $_scrutineeType');
}
if (_expressionWithInfo != null) {
print(' expressionWithInfo: $_expressionWithInfo');
}
if (_expressionInfo != null) {
print(' expressionInfo: $_expressionInfo');
}
if (_expressionWithReference != null) {
print(' expressionWithReference: $_expressionWithReference');
}
if (_expressionReference != null) {
print(' expressionReference: $_expressionReference');
}
if (_stack.isNotEmpty) {
print(' stack:');
for (_FlowContext stackEntry in _stack.reversed) {
print(' $stackEntry');
}
}
}

Expand Down Expand Up @@ -5052,6 +5090,9 @@ class _LegacyTypePromotion<Node extends Object, Statement extends Node,
@override
void lateInitializer_end() {}

@override
void listPattern_begin() {}

@override
void logicalBinaryOp_begin() {
_writeStackForAnd.add({});
Expand Down Expand Up @@ -5561,16 +5602,16 @@ class _SimpleStatementContext<Type extends Object>
}

class _SwitchAlternativesContext<Type extends Object> extends _FlowContext {
final FlowModel<Type> _previous;
/// The enclosing [_SwitchStatementContext].
final _SwitchStatementContext<Type> _switchStatementContext;

FlowModel<Type>? _combinedModel;

_SwitchAlternativesContext(this._previous);
_SwitchAlternativesContext(this._switchStatementContext);

@override
Map<String, Object?> get _debugFields => super._debugFields
..['previous'] = _previous
..['combinedModel'] = _combinedModel;
Map<String, Object?> get _debugFields =>
super._debugFields..['combinedModel'] = _combinedModel;

@override
String get _debugType => '_SwitchAlternativesContext';
Expand All @@ -5582,12 +5623,19 @@ class _SwitchStatementContext<Type extends Object>
/// The static type of the value being matched.
final Type _scrutineeType;

/// Flow state for the code path where no switch cases have matched yet. If
/// we think of a switch statement as syntactic sugar for a chain of if-else
/// statements, this is the flow state on entry to the next `if`.
FlowModel<Type> _unmatched;

_SwitchStatementContext(
super.checkpoint, super._previous, this._scrutineeType);
super.checkpoint, super._previous, this._scrutineeType)
: _unmatched = _previous;

@override
Map<String, Object?> get _debugFields =>
super._debugFields..['scrutineeType'] = _scrutineeType;
Map<String, Object?> get _debugFields => super._debugFields
..['scrutineeType'] = _scrutineeType
..['unmatched'] = _unmatched;

@override
String get _debugType => '_SwitchStatementContext';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,7 @@ mixin TypeAnalyzer<
}
}
// Stack: ()
flow.listPattern_begin();
Node? previousRestPattern;
for (Node element in elements) {
if (isRestPatternElement(element)) {
Expand Down
Loading

0 comments on commit 5b234e4

Please # to comment.