Skip to content

Commit

Permalink
flow analysis: don't promote scrutinee of irrefutable patterns.
Browse files Browse the repository at this point in the history 
As discussed in
dart-lang/language#2857 (comment),
we only want a pattern match to promote the scrutinee in refutable
contexts (if-case and switch).

Bug: dart-lang/language#2857, #50419
Change-Id: I187ef632e5da95b931e5cda34db06491a5228c98
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/288000
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
  • Loading branch information
@stereotype441
stereotype441 authored and Commit Queue committed Mar 21, 2023
1 parent 38a2a91 commit adbf363
Show file tree
Hide file tree
Showing 13 changed files with 260 additions and 202 deletions.
29 changes: 20 additions & 9 deletions pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4101,7 +4101,8 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
// Note that we don't need to take any action to handle
// `before(E1) = matched(P)`, because we store both the "matched" state for
// patterns and the "before" state for expressions in `_current`.
_pushPattern(_pushScrutinee(scrutinee, scrutineeType));
_pushPattern(_pushScrutinee(scrutinee, scrutineeType,
allowScrutineePromotion: true));
}

@override
Expand Down Expand Up @@ -4438,7 +4439,7 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,

@override
void patternAssignment_afterRhs(Expression rhs, Type rhsType) {
_pushPattern(_pushScrutinee(rhs, rhsType));
_pushPattern(_pushScrutinee(rhs, rhsType, allowScrutineePromotion: false));
}

@override
Expand All @@ -4449,7 +4450,8 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,

@override
void patternForIn_afterExpression(Type elementType) {
_pushPattern(_pushScrutinee(null, elementType));
_pushPattern(
_pushScrutinee(null, elementType, allowScrutineePromotion: false));
}

@override
Expand All @@ -4461,7 +4463,8 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
@override
void patternVariableDeclaration_afterInitializer(
Expression initializer, Type initializerType) {
_pushPattern(_pushScrutinee(initializer, initializerType));
_pushPattern(_pushScrutinee(initializer, initializerType,
allowScrutineePromotion: false));
}

@override
Expand Down Expand Up @@ -4672,7 +4675,7 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
void switchStatement_expressionEnd(
Statement? switchStatement, Expression scrutinee, Type scrutineeType) {
EqualityInfo<Type> matchedValueInfo =
_pushScrutinee(scrutinee, scrutineeType);
_pushScrutinee(scrutinee, scrutineeType, allowScrutineePromotion: true);
_current = _current.split();
_SwitchStatementContext<Type> context = new _SwitchStatementContext<Type>(
_current.reachable.parent!, _current, matchedValueInfo);
Expand Down Expand Up @@ -5245,19 +5248,27 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
/// that's being matched directly, as happens when in `for-in` loops).
/// [scrutineeType] should be the static type of the scrutinee.
///
/// [allowScrutineePromotion] indicates whether pattern matches should cause
/// the scrutinee to be promoted.
///
/// The returned value is the [EqualityInfo] representing the value being
/// matched. It should be passed to [_pushPattern].
EqualityInfo<Type> _pushScrutinee(Expression? scrutinee, Type scrutineeType) {
EqualityInfo<Type> _pushScrutinee(Expression? scrutinee, Type scrutineeType,
{required bool allowScrutineePromotion}) {
EqualityInfo<Type>? scrutineeInfo = scrutinee == null
? null
: _computeEqualityInfo(scrutinee, scrutineeType);
_stack.add(new _ScrutineeContext<Type>(
previousScrutineeReference: _scrutineeReference));
ReferenceWithType<Type>? scrutineeReference = scrutineeInfo?._reference;
_scrutineeReference = scrutineeReference;
SsaNode<Type>? scrutineeSsaNode = scrutineeReference == null
? new SsaNode<Type>(null)
: _current.infoFor(scrutineeReference.promotionKey).ssaNode;
SsaNode<Type>? scrutineeSsaNode;
if (!allowScrutineePromotion || scrutineeReference == null) {
scrutineeSsaNode = new SsaNode<Type>(null);
} else {
scrutineeSsaNode =
_current.infoFor(scrutineeReference.promotionKey).ssaNode;
}
return new EqualityInfo._(
scrutineeInfo?._expressionInfo,
scrutineeType,
Expand Down
73 changes: 59 additions & 14 deletions pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6450,14 +6450,15 @@ main() {
});
});

test('Promotes scrutinee', () {
test('Does not promote scrutinee', () {
// The code below is equivalent to:
// int x;
// dynamic y = ...;
// (x && _) = y;
// // y is now promoted to `int`.
// The promotion occurs because the assignment to `x` performs an
// implicit downcast.
// // y is *not* promoted to `int`.
// Although the assignment to `x` performs an implicit downcast, we
// don't promote `y` because patterns in irrefutable contexts don't
// trigger scrutinee promotion.
var x = Var('x');
var y = Var('y');
h.run([
Expand All @@ -6468,7 +6469,7 @@ main() {
.and(wildcard()..errorId = 'WILDCARD')
.assign(y.expr)
.stmt,
checkPromoted(y, 'int'),
checkNotPromoted(y),
], expectedErrors: {
'unnecessaryWildcardPattern(pattern: WILDCARD, '
'kind: logicalAndPatternOperand)',
Expand Down Expand Up @@ -6544,9 +6545,16 @@ main() {
h.run([
declare(x, initializer: expr('(dynamic,)')),
declare(y, type: 'int'),
recordPattern([y.pattern().recordField()]).assign(x.expr).stmt,
checkPromoted(x, '(int,)'),
]);
recordPattern([y.pattern().recordField()])
.and(wildcard(expectInferredType: '(int,)')
..errorId = 'WILDCARD')
.assign(x.expr)
.stmt,
checkNotPromoted(x),
], expectedErrors: {
'unnecessaryWildcardPattern(pattern: WILDCARD, '
'kind: logicalAndPatternOperand)'
});
});

test('Supertype of matched value type', () {
Expand All @@ -6555,9 +6563,16 @@ main() {
h.run([
declare(x, initializer: expr('(int,)')),
declare(y, type: 'num'),
recordPattern([y.pattern().recordField()]).assign(x.expr).stmt,
recordPattern([y.pattern().recordField()])
.and(wildcard(expectInferredType: '(int,)')
..errorId = 'WILDCARD')
.assign(x.expr)
.stmt,
checkNotPromoted(x),
]);
], expectedErrors: {
'unnecessaryWildcardPattern(pattern: WILDCARD, '
'kind: logicalAndPatternOperand)'
});
});
});
});
Expand Down Expand Up @@ -6828,6 +6843,36 @@ main() {
});
});

group('For-in statement:', () {
test('does not promote iterable', () {
var x = Var('x');
h.run([
declare(x, initializer: expr('List<dynamic>')),
patternForIn(
wildcard(type: 'int'),
x.expr,
[],
),
checkNotPromoted(x),
]);
});
});

group('For-in collection element:', () {
test('does not promote iterable', () {
var x = Var('x');
h.run([
declare(x, initializer: expr('List<dynamic>')),
patternForInElement(
wildcard(type: 'int'),
x.expr,
expr('Object').asCollectionElement,
).inContextElementType('Object'),
checkNotPromoted(x),
]);
});
});

group('If-case element:', () {
test('guarded', () {
var x = Var('x');
Expand Down Expand Up @@ -7975,23 +8020,23 @@ main() {
});

group('Pattern assignment:', () {
test('Promotes RHS', () {
test('Does not promote RHS', () {
var x = Var('x');
h.run([
declare(x, initializer: expr('num')),
wildcard().as_('int').assign(x.expr).stmt,
checkPromoted(x, 'int'),
checkNotPromoted(x),
]);
});
});

group('Pattern variable declaration:', () {
test('Promotes RHS', () {
test('Does not promote RHS', () {
var x = Var('x');
h.run([
declare(x, initializer: expr('num')),
match(wildcard().as_('int'), x.expr),
checkPromoted(x, 'int'),
checkNotPromoted(x),
]);
});
});
Expand Down
2 changes: 2 additions & 0 deletions pkg/_fe_analyzer_shared/test/mini_ast.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -882,6 +882,8 @@ class MiniAstOperations

static final Map<String, Type> _coreGlbs = {
'?, int': Type('int'),
'(int,), ?': Type('(int,)'),
'(num,), ?': Type('(num,)'),
'Object?, double': Type('double'),
'Object?, int': Type('int'),
'double, int': Type('Never'),
Expand Down
64 changes: 32 additions & 32 deletions pkg/front_end/testcases/patterns/non_type_object_pattern.dart.strong.expect
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,14 +261,14 @@ static method testUnresolved(dynamic o) → dynamic {
final Unresolved(:o) = o;
^";
{
final synthesized dynamic #1#0 = o{invalid-type};
if(!(#1#0{invalid-type} is{ForNonNullableByDefault} invalid-type && (let final dynamic #t2 = o = #1#0{invalid-type}{<invalid>}.o in true)))
final synthesized dynamic #1#0 = o;
if(!(#1#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t2 = o = #1#0{invalid-type}{<invalid>}.o in true)))
throw new _in::ReachabilityError::•();
}
{
invalid-type field;
final synthesized invalid-type #2#0 = o{invalid-type};
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t3 = field = #2#0{<invalid>}.field in true)) {
final synthesized dynamic #2#0 = o;
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t3 = field = #2#0{invalid-type}{<invalid>}.field in true)) {
}
}
#L1:
Expand Down Expand Up @@ -316,14 +316,14 @@ static method testNonType(dynamic o) → dynamic {
final NonType(:o) = o;
^";
{
final synthesized dynamic #1#0 = o{invalid-type};
if(!(#1#0{invalid-type} is{ForNonNullableByDefault} invalid-type && (let final dynamic #t8 = o = #1#0{invalid-type}{<invalid>}.o in true)))
final synthesized dynamic #1#0 = o;
if(!(#1#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t8 = o = #1#0{invalid-type}{<invalid>}.o in true)))
throw new _in::ReachabilityError::•();
}
{
invalid-type field;
final synthesized invalid-type #2#0 = o{invalid-type};
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t9 = field = #2#0{<invalid>}.field in true)) {
final synthesized dynamic #2#0 = o;
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t9 = field = #2#0{invalid-type}{<invalid>}.field in true)) {
}
}
#L3:
Expand Down Expand Up @@ -371,14 +371,14 @@ static method testExtension(dynamic o) → dynamic {
final Extension(:o) = o;
^";
{
final synthesized dynamic #1#0 = o{invalid-type};
if(!(#1#0{invalid-type} is{ForNonNullableByDefault} invalid-type && (let final dynamic #t14 = o = #1#0{invalid-type}{<invalid>}.o in true)))
final synthesized dynamic #1#0 = o;
if(!(#1#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t14 = o = #1#0{invalid-type}{<invalid>}.o in true)))
throw new _in::ReachabilityError::•();
}
{
invalid-type field;
final synthesized invalid-type #2#0 = o{invalid-type};
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t15 = field = #2#0{<invalid>}.field in true)) {
final synthesized dynamic #2#0 = o;
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t15 = field = #2#0{invalid-type}{<invalid>}.field in true)) {
}
}
#L5:
Expand Down Expand Up @@ -426,14 +426,14 @@ static method testPrefixedUnresolved(dynamic o) → dynamic {
final prefix.Unresolved(:o) = o;
^";
{
final synthesized dynamic #1#0 = o{invalid-type};
if(!(#1#0{invalid-type} is{ForNonNullableByDefault} invalid-type && (let final dynamic #t20 = o = #1#0{invalid-type}{<invalid>}.o in true)))
final synthesized dynamic #1#0 = o;
if(!(#1#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t20 = o = #1#0{invalid-type}{<invalid>}.o in true)))
throw new _in::ReachabilityError::•();
}
{
invalid-type field;
final synthesized invalid-type #2#0 = o{invalid-type};
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t21 = field = #2#0{<invalid>}.field in true)) {
final synthesized dynamic #2#0 = o;
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t21 = field = #2#0{invalid-type}{<invalid>}.field in true)) {
}
}
#L7:
Expand Down Expand Up @@ -481,14 +481,14 @@ static method testPrefixedNonType(dynamic o) → dynamic {
final prefix.NonType(:o) = o;
^";
{
final synthesized dynamic #1#0 = o{invalid-type};
if(!(#1#0{invalid-type} is{ForNonNullableByDefault} invalid-type && (let final dynamic #t26 = o = #1#0{invalid-type}{<invalid>}.o in true)))
final synthesized dynamic #1#0 = o;
if(!(#1#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t26 = o = #1#0{invalid-type}{<invalid>}.o in true)))
throw new _in::ReachabilityError::•();
}
{
invalid-type field;
final synthesized invalid-type #2#0 = o{invalid-type};
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t27 = field = #2#0{<invalid>}.field in true)) {
final synthesized dynamic #2#0 = o;
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t27 = field = #2#0{invalid-type}{<invalid>}.field in true)) {
}
}
#L9:
Expand Down Expand Up @@ -536,14 +536,14 @@ static method testPrefixedExtension(dynamic o) → dynamic {
final prefix.Extension(:o) = o;
^";
{
final synthesized dynamic #1#0 = o{invalid-type};
if(!(#1#0{invalid-type} is{ForNonNullableByDefault} invalid-type && (let final dynamic #t32 = o = #1#0{invalid-type}{<invalid>}.o in true)))
final synthesized dynamic #1#0 = o;
if(!(#1#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t32 = o = #1#0{invalid-type}{<invalid>}.o in true)))
throw new _in::ReachabilityError::•();
}
{
invalid-type field;
final synthesized invalid-type #2#0 = o{invalid-type};
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t33 = field = #2#0{<invalid>}.field in true)) {
final synthesized dynamic #2#0 = o;
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t33 = field = #2#0{invalid-type}{<invalid>}.field in true)) {
}
}
#L11:
Expand Down Expand Up @@ -591,14 +591,14 @@ static method testUnresolvedPrefix(dynamic o) → dynamic {
final unresolved.Type(:o) = o;
^";
{
final synthesized dynamic #1#0 = o{invalid-type};
if(!(#1#0{invalid-type} is{ForNonNullableByDefault} invalid-type && (let final dynamic #t38 = o = #1#0{invalid-type}{<invalid>}.o in true)))
final synthesized dynamic #1#0 = o;
if(!(#1#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t38 = o = #1#0{invalid-type}{<invalid>}.o in true)))
throw new _in::ReachabilityError::•();
}
{
invalid-type field;
final synthesized invalid-type #2#0 = o{invalid-type};
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t39 = field = #2#0{<invalid>}.field in true)) {
final synthesized dynamic #2#0 = o;
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t39 = field = #2#0{invalid-type}{<invalid>}.field in true)) {
}
}
#L13:
Expand Down Expand Up @@ -646,14 +646,14 @@ static method testMemberAccess(dynamic o) → dynamic {
final NonType.hashCode(:o) = o;
^";
{
final synthesized dynamic #1#0 = o{invalid-type};
if(!(#1#0{invalid-type} is{ForNonNullableByDefault} invalid-type && (let final dynamic #t44 = o = #1#0{invalid-type}{<invalid>}.o in true)))
final synthesized dynamic #1#0 = o;
if(!(#1#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t44 = o = #1#0{invalid-type}{<invalid>}.o in true)))
throw new _in::ReachabilityError::•();
}
{
invalid-type field;
final synthesized invalid-type #2#0 = o{invalid-type};
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t45 = field = #2#0{<invalid>}.field in true)) {
final synthesized dynamic #2#0 = o;
if(#2#0 is{ForNonNullableByDefault} invalid-type && (let final dynamic #t45 = field = #2#0{invalid-type}{<invalid>}.field in true)) {
}
}
#L15:
Expand Down
Loading

0 comments on commit adbf363

Please sign in to comment.