Flow analysis: fix field promotion based on condition variables.
When the result of an `is` test or null check is stored in a boolean
variable, and later recalled for use in flow control, the flow models
that were computed at the time the variable was stored need to be
updated to reflect any further change to flow state that happened
between the test and the usage. This is done by
`FlowModel.rebaseForward` method. `rebaseForward` takes two flow
models as input: `this`, which represents the flow state that was
computed at the time the condition variable was stored, and `base`,
which represents the flow state at the time the condition variable is
recalled.
Flow analysis adds promotion keys for variables to the flow state at
the time their declarations are encountered, and in certain
circumstances removes them after they go out of scope. But for
properties, it only adds promotion keys when the promotion occurs. So
prior to the addition of field promotion, if `this` contained a
promotion key that wasn't present in `base`, that could only mean that
the promotion key was associated with a variable that had gone out of
scope; accordingly, it was safe for `rebaseForward` to simply ignore
that key. (It did so implicitly, by only ever examining the promotion
keys in `this`). But with the addition of field promotion, it is now
possible that the promotion key represents a property that was
promoted in `this`, and hence the promotion needs to be kept. This CL
adds the necessary logic to keep the promotion.
In addition, there is a subtle difference in the relationship between
the `PromotionModel` and `SsaNode` data structures for local variables
versus properties. For local variables, the promotion key is
determined solely from the variable name; then, this promotion key is
looked up in the current `FlowModel` to obtain a `PromotionModel`, and
the `PromotionModel` contains a prointer to the `SsaNode`. For
properties, the property name is looked up in the
`promotableProperties` map of the parent `SsaNode`; this points to a
`_PropertySsaNode`, which contains the promotion key, and when this
promotion key is looked up in the current `FlowModel` to obtain a
`PromotionModel`, that `PromotionModel` contains a pointer to a bogus
`SsaNode`.
For local variables, the `SsaNode` pointed to by the `PromotionModel`
is important, because if it's different between `this` and `base`,
then the variable in question received a new value between the time
the condition variable was stored and the time the condition variable
was recalled; therefore the promotion should be disregarded. However,
for properties, the `SsaNode` pointed to by the `PromotionModel` is
bogus, so if it's different between `this` and `base`, that shouldn't
block promotion. This CL adds the necessary logic to avoid the
`SsaNode` check for properties.
This situation is very confusing so I've added more detail to the
comment above `PromotionModel.SsaNode` explaining it. In a future CL I
will try to clean up the confusing situation by eliminating the bogus
`SsaNode`s pointed to by `PromotionModel`s for properties.
Fixes https://github.com/dart-lang/sdk/issues/53273.
Bug: https://github.com/dart-lang/sdk/issues/53273
Change-Id: I1d528e25de1eb2ed63d0ee1a00faa5ad5b5061ca
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/321752
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
diff --git a/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart b/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
index 4a0316f..62c76fb 100644
--- a/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
+++ b/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
@@ -2280,7 +2280,7 @@
/// will be promoted in the output model, provided that hasn't been reassigned
/// since then (which would make the promotion unsound).
FlowModel<Type> rebaseForward(
- TypeOperations<Type> typeOperations, FlowModel<Type> base) {
+ FlowModelHelper<Type> helper, FlowModel<Type> base) {
// The rebased model is reachable iff both `this` and the new base are
// reachable.
Reachability newReachable = reachable.rebaseForward(base.reachable);
@@ -2312,7 +2312,9 @@
if (newWriteCaptured) {
// Write captured variables can't be promoted.
newPromotedTypes = null;
- } else if (baseModel.ssaNode != thisModel.ssaNode) {
+ } else if (helper.promotionKeyStore.variableForKey(promotionKey) !=
+ null &&
+ baseModel.ssaNode != thisModel.ssaNode) {
// The variable may have been written to since `thisModel`, so we can't
// use any of the promotions from `thisModel`.
newPromotedTypes = baseModel.promotedTypes;
@@ -2322,12 +2324,14 @@
// usual "promotion chain" invariant (each promoted type is a subtype of
// the previous).
newPromotedTypes = PromotionModel.rebasePromotedTypes(
- typeOperations, thisModel.promotedTypes, baseModel.promotedTypes);
+ helper.typeOperations,
+ thisModel.promotedTypes,
+ baseModel.promotedTypes);
}
// Tests are kept regardless of whether they are in `this` model or the
// new base model.
List<Type> newTested = PromotionModel.joinTested(
- thisModel.tested, baseModel.tested, typeOperations);
+ thisModel.tested, baseModel.tested, helper.typeOperations);
// The variable is definitely assigned if it was definitely assigned
// either in `this` model or the new base model.
bool newAssigned = thisModel.assigned || baseModel.assigned;
@@ -2346,15 +2350,24 @@
if (!identical(newModel, thisModel)) promotionInfoMatchesThis = false;
if (!identical(newModel, baseModel)) promotionInfoMatchesBase = false;
}
- // newPromotionInfo is now correct. However, if there are any variables
- // present in `this` that aren't present in `base`, we may erroneously think
- // that `newPromotionInfo` matches `this`. If so, correct that.
- if (promotionInfoMatchesThis) {
- for (int promotionKey in promotionInfo.keys) {
- if (!base.promotionInfo.containsKey(promotionKey)) {
- promotionInfoMatchesThis = false;
- break;
- }
+ // Check promotion keys that exist in `this` model but not in the new `base`
+ // model. This happens when either:
+ // - The promotion key is associated with a local variable that was in scope
+ // at the time `this` model was created, but is no longer in scope as of
+ // the `base` model, or:
+ // - The promotion key is associated with a property that was promoted in
+ // `this` model.
+ //
+ // In the first case, it doesn't matter what we do, because the variable is
+ // no longer in scope. But in the second case, we need to preserve the
+ // promotion.
+ for (var MapEntry(
+ key: int promotionKey,
+ value: PromotionModel<Type> thisModel
+ ) in promotionInfo.entries) {
+ if (!base.promotionInfo.containsKey(promotionKey)) {
+ newPromotionInfo[promotionKey] = thisModel;
+ promotionInfoMatchesBase = false;
}
}
assert(promotionInfoMatchesThis ==
@@ -2844,6 +2857,14 @@
/// `null` if the variable has been write captured.
///
/// Not relevant for properties.
+ ///
+ /// Note: currently this field stores a bogus SSA node when the PromotionModel
+ /// represents a property; it's not the SSA node of the value stored in the
+ /// property, and it's not guaranteed to be a stable value (even if the
+ /// property itself is promotable, and thus stable).
+ /// TODO(paulberry): either ensure that flow analysis doesn't try to access
+ /// this field when the PromotionModel represents a property, or ensure that
+ /// this field always properly reflects the SSA node of the property.
final SsaNode<Type>? ssaNode;
/// Non-promotion history of this variable. Not relevant for properties.
@@ -3829,7 +3850,7 @@
/// [current] should be the current flow model, and [typeOperations] should be
/// the callback object provided by the client for manipulating types.
_Reference<Type> addPreviousInfo(ExpressionInfo<Type>? previousExpressionInfo,
- TypeOperations<Type> typeOperations, FlowModel<Type> current) {
+ FlowModelHelper<Type> helper, FlowModel<Type> current) {
if (previousExpressionInfo != null && previousExpressionInfo.isNonTrivial) {
// [previousExpression] contained non-trivial flow analysis information,
// so we need to rebase its [ifTrue] and [ifFalse] flow models. We don't
@@ -3841,10 +3862,9 @@
after: current,
type: _type,
isThisOrSuper: isThisOrSuper,
- ifTrue: previousExpressionInfo.ifTrue
- .rebaseForward(typeOperations, current),
- ifFalse: previousExpressionInfo.ifFalse
- .rebaseForward(typeOperations, current),
+ ifTrue: previousExpressionInfo.ifTrue.rebaseForward(helper, current),
+ ifFalse:
+ previousExpressionInfo.ifFalse.rebaseForward(helper, current),
ssaNode: ssaNode);
} else {
// [previousExpression] didn't contain any non-trivial flow analysis
@@ -5184,7 +5204,7 @@
PromotionModel<Type> promotionModel = _current._getInfo(variableKey);
_Reference<Type> expressionInfo =
_variableReference(variableKey, unpromotedType).addPreviousInfo(
- promotionModel.ssaNode?.expressionInfo, operations, _current);
+ promotionModel.ssaNode?.expressionInfo, this, _current);
_storeExpressionReference(expression, expressionInfo);
_storeExpressionInfo(expression, expressionInfo);
return promotionModel.promotedTypes?.last;
@@ -5445,7 +5465,7 @@
_PatternContext<Type> context = _stack.last as _PatternContext<Type>;
_Reference<Type> newReference = context
.createReference(getMatchedValueType(), _current)
- .addPreviousInfo(context._matchedValueInfo, typeOperations, _current);
+ .addPreviousInfo(context._matchedValueInfo, this, _current);
_EqualityCheckResult equalityCheckResult =
_equalityCheck(newReference, equalityOperand_end(operand, operandType));
if (equalityCheckResult is _NoEqualityInformation) {
@@ -5711,7 +5731,7 @@
}
return _makeTemporaryReference(
scrutineeSsaNode ?? new SsaNode<Type>(null), scrutineeType)
- .addPreviousInfo(scrutineeInfo, typeOperations, _current);
+ .addPreviousInfo(scrutineeInfo, this, _current);
}
/// Associates [expression], which should be the most recently visited
diff --git a/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart b/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
index 795b7cd..40935d9 100644
--- a/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
+++ b/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
@@ -4168,23 +4168,14 @@
test('reachability', () {
var reachable = FlowModel<Type>(Reachability.initial);
var unreachable = reachable.setUnreachable();
- expect(reachable.rebaseForward(h.typeOperations, reachable),
- same(reachable));
- expect(reachable.rebaseForward(h.typeOperations, unreachable),
- same(unreachable));
+ expect(reachable.rebaseForward(h, reachable), same(reachable));
+ expect(reachable.rebaseForward(h, unreachable), same(unreachable));
expect(
- unreachable
- .rebaseForward(h.typeOperations, reachable)
- .reachable
- .overallReachable,
+ unreachable.rebaseForward(h, reachable).reachable.overallReachable,
false);
- expect(
- unreachable
- .rebaseForward(h.typeOperations, reachable)
- .promotionInfo,
+ expect(unreachable.rebaseForward(h, reachable).promotionInfo,
same(unreachable.promotionInfo));
- expect(unreachable.rebaseForward(h.typeOperations, unreachable),
- same(unreachable));
+ expect(unreachable.rebaseForward(h, unreachable), same(unreachable));
});
test('assignments', () {
@@ -4203,7 +4194,7 @@
var s2 = s0
._write(h, null, a, Type('int'), new SsaNode<Type>(null))
._write(h, null, c, Type('int'), new SsaNode<Type>(null));
- var result = s1.rebaseForward(h.typeOperations, s2);
+ var result = s1.rebaseForward(h, s2);
expect(result._infoFor(h, a).assigned, true);
expect(result._infoFor(h, b).assigned, true);
expect(result._infoFor(h, c).assigned, true);
@@ -4223,7 +4214,7 @@
// In s1, a and b are write captured. In s2, a and c are.
var s1 = s0._conservativeJoin(h, [a, b], [a, b]);
var s2 = s1._conservativeJoin(h, [a, c], [a, c]);
- var result = s1.rebaseForward(h.typeOperations, s2);
+ var result = s1.rebaseForward(h, s2);
expect(
result._infoFor(h, a),
_matchVariableModel(writeCaptured: true, unassigned: false),
@@ -4249,11 +4240,11 @@
var s1 = s0._conservativeJoin(h, [a], [a]);
var s2 = s0._tryPromoteForTypeCheck(h, a, 'int').ifTrue;
expect(
- s1.rebaseForward(h.typeOperations, s2)._infoFor(h, a),
+ s1.rebaseForward(h, s2)._infoFor(h, a),
_matchVariableModel(writeCaptured: true, chain: isNull),
);
expect(
- s2.rebaseForward(h.typeOperations, s1)._infoFor(h, a),
+ s2.rebaseForward(h, s1)._infoFor(h, a),
_matchVariableModel(writeCaptured: true, chain: isNull),
);
});
@@ -4274,7 +4265,7 @@
var s2 = otherType == null
? s0
: s0._tryPromoteForTypeCheck(h, x, otherType).ifTrue;
- var result = s2.rebaseForward(h.typeOperations, s1);
+ var result = s2.rebaseForward(h, s1);
if (expectedChain == null) {
expect(result.promotionInfo,
contains(h.promotionKeyStore.keyForVariable(x)));
@@ -4343,7 +4334,7 @@
var expectedFinallyChain = before.toList()..addAll(inFinally);
_checkChain(
finallyModel._infoFor(h, x).promotedTypes, expectedFinallyChain);
- var result = tryModel.rebaseForward(h.typeOperations, finallyModel);
+ var result = tryModel.rebaseForward(h, finallyModel);
_checkChain(result._infoFor(h, x).promotedTypes, expectedResult);
// And verify that the inputs are unchanged.
_checkChain(initialModel._infoFor(h, x).promotedTypes, before);
@@ -4385,11 +4376,11 @@
var s1 = s0._tryPromoteForTypeCheck(h, a, 'int').ifFalse;
var s2 = s0._tryPromoteForTypeCheck(h, a, 'String').ifFalse;
expect(
- s1.rebaseForward(h.typeOperations, s2)._infoFor(h, a),
+ s1.rebaseForward(h, s2)._infoFor(h, a),
_matchVariableModel(ofInterest: ['int', 'String']),
);
expect(
- s2.rebaseForward(h.typeOperations, s1)._infoFor(h, a),
+ s2.rebaseForward(h, s1)._infoFor(h, a),
_matchVariableModel(ofInterest: ['int', 'String']),
);
});
@@ -4398,8 +4389,8 @@
var x = Var('x')..type = Type('Object?');
var s0 = FlowModel<Type>(Reachability.initial);
var s1 = s0._declare(h, x, true);
- expect(s1.rebaseForward(h.typeOperations, s0), same(s0));
- expect(s0.rebaseForward(h.typeOperations, s1), same(s1));
+ expect(s1.rebaseForward(h, s0), same(s1));
+ expect(s0.rebaseForward(h, s1), same(s1));
});
});
});
@@ -6794,6 +6785,167 @@
]);
});
});
+
+ group('Via local condition variable:', () {
+ group('without intervening promotion:', () {
+ // These tests exercise the code path in `FlowModel.rebaseForward` where
+ // `this` model (which represents the state captured at the time the
+ // condition variable is written) contains a promotion key for the
+ // field, but the `base` model (which represents state just prior to
+ // reading from the condition variable) doesn't contain any promotion
+ // key for the field. Furthermore, since no other promotions occur
+ // between writing and reading the condition variable, `rebaseForward`
+ // will not create a fresh `FlowModel`; it will simply return `this`
+ // model.
+ test('using null check', () {
+ h.addMember('C', '_field', 'int?', promotable: true);
+ var c = Var('c');
+ var b = Var('b');
+ h.run([
+ declare(c, initializer: expr('C')),
+ declare(b, initializer: c.property('_field').notEq(nullLiteral)),
+ if_(b, [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ ]);
+ });
+
+ test('using `is` test', () {
+ h.addMember('C', '_field', 'Object', promotable: true);
+ h.addSuperInterfaces('C', (_) => [Type('Object')]);
+ var c = Var('c');
+ var b = Var('b');
+ h.run([
+ declare(c, initializer: expr('C')),
+ declare(b, initializer: c.property('_field').is_('int')),
+ if_(b, [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ ]);
+ });
+ });
+
+ group('with intervening related promotion:', () {
+ // These tests exercise the code path in `FlowModel.rebaseForward` where
+ // `this` model (which represents the state captured at the time the
+ // condition variable is written) and the `base` model (which represents
+ // state just prior to reading from the condition variable) both contain
+ // a promotion key for the field.
+ test('using null check', () {
+ h.addMember('C', '_field', 'int?', promotable: true);
+ var c = Var('c');
+ var b = Var('b');
+ h.run([
+ declare(c, initializer: expr('C')),
+ declare(b, initializer: c.property('_field').notEq(nullLiteral)),
+ if_(c.property('_field').notEq(nullLiteral), [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ if_(b, [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ ]);
+ });
+
+ test('using `is` test', () {
+ h.addMember('C', '_field', 'Object', promotable: true);
+ h.addSuperInterfaces('C', (_) => [Type('Object')]);
+ var c = Var('c');
+ var b = Var('b');
+ h.run([
+ declare(c, initializer: expr('C')),
+ declare(b, initializer: c.property('_field').is_('int')),
+ if_(c.property('_field').is_('int'), [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ if_(b, [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ ]);
+ });
+ });
+
+ group('with intervening unrelated promotion:', () {
+ // These tests exercise the code path in `FlowModel.rebaseForward` where
+ // `this` model (which represents the state captured at the time the
+ // condition variable is written) contains a promotion key for the
+ // field, but the `base` model (which represents state just prior to
+ // reading from the condition variable) doesn't contain any promotion
+ // key for the field. Since a different variable is promoted in between
+ // writing and reading the condition variable, `rebaseForward` will be
+ // forced to create a fresh `FlowModel`; it will not be able to simply
+ // return `this` model.
+ test('using null check', () {
+ h.addMember('C', '_field', 'int?', promotable: true);
+ var c = Var('c');
+ var unrelated = Var('unrelated');
+ var b = Var('b');
+ h.run([
+ declare(c, initializer: expr('C')),
+ declare(unrelated, initializer: expr('int?')),
+ declare(b, initializer: c.property('_field').notEq(nullLiteral)),
+ unrelated.nonNullAssert,
+ if_(b, [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ ]);
+ });
+
+ test('using `is` test', () {
+ h.addMember('C', '_field', 'Object', promotable: true);
+ h.addSuperInterfaces('C', (_) => [Type('Object')]);
+ var c = Var('c');
+ var unrelated = Var('unrelated');
+ var b = Var('b');
+ h.run([
+ declare(c, initializer: expr('C')),
+ declare(unrelated, initializer: expr('int?')),
+ declare(b, initializer: c.property('_field').is_('int')),
+ unrelated.nonNullAssert,
+ if_(b, [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ ]);
+ });
+ });
+
+ group('disabled by intervening assignment:', () {
+ test('using null check', () {
+ h.addMember('C', '_field', 'int?', promotable: true);
+ var c = Var('c');
+ var b = Var('b');
+ h.run([
+ declare(c, initializer: expr('C')),
+ declare(b, initializer: c.property('_field').notEq(nullLiteral)),
+ if_(c.property('_field').notEq(nullLiteral), [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ c.write(expr('C')),
+ if_(b, [
+ checkNotPromoted(c.property('_field')),
+ ]),
+ ]);
+ });
+
+ test('using `is` test', () {
+ h.addMember('C', '_field', 'Object', promotable: true);
+ h.addSuperInterfaces('C', (_) => [Type('Object')]);
+ var c = Var('c');
+ var b = Var('b');
+ h.run([
+ declare(c, initializer: expr('C')),
+ declare(b, initializer: c.property('_field').is_('int')),
+ if_(c.property('_field').is_('int'), [
+ checkPromoted(c.property('_field'), 'int'),
+ ]),
+ c.write(expr('C')),
+ if_(b, [
+ checkNotPromoted(c.property('_field')),
+ ]),
+ ]);
+ });
+ });
+ });
});
group('Patterns:', () {
diff --git a/tests/language/inference_update_2/condition_variable_test.dart b/tests/language/inference_update_2/condition_variable_test.dart
new file mode 100644
index 0000000..7ab67b9
--- /dev/null
+++ b/tests/language/inference_update_2/condition_variable_test.dart
@@ -0,0 +1,96 @@
+// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Tests that field promotion works when the promotion condition is stored in a
+// local variable.
+
+import '../static_type_helper.dart';
+
+class C {
+ final Object? _o;
+ C(this._o);
+}
+
+void usingNullCheck_withoutInterveningPromotion(C c) {
+ bool b = c._o != null;
+ if (b) {
+ c._o.expectStaticType<Exactly<Object>>();
+ }
+}
+
+void usingIsTest_withoutInterveningPromotion(C c) {
+ bool b = c._o is int;
+ if (b) {
+ c._o.expectStaticType<Exactly<int>>();
+ }
+}
+
+void usingNullCheck_withInterveningRelatedPromotion(C c) {
+ bool b = c._o != null;
+ if (c._o != null) {
+ c._o.expectStaticType<Exactly<Object>>();
+ }
+ if (b) {
+ c._o.expectStaticType<Exactly<Object>>();
+ }
+}
+
+void usingIsTest_withInterveningRelatedPromotion(C c) {
+ bool b = c._o is int;
+ if (c._o is int) {
+ c._o.expectStaticType<Exactly<int>>();
+ }
+ if (b) {
+ c._o.expectStaticType<Exactly<int>>();
+ }
+}
+
+void usingNullCheck_withInterveningUnrelatedPromotion(C c, int? i) {
+ bool b = c._o != null;
+ i!;
+ if (b) {
+ c._o.expectStaticType<Exactly<Object>>();
+ }
+}
+
+void usingIsTest_withInterveningUnrelatedPromotion(C c, int? i) {
+ bool b = c._o is int;
+ i!;
+ if (b) {
+ c._o.expectStaticType<Exactly<int>>();
+ }
+}
+
+void usingNullCheck_disabledByAssignment(C c, C c2) {
+ bool b = c._o != null;
+ if (c._o != null) {
+ c._o.expectStaticType<Exactly<Object>>();
+ }
+ c = c2;
+ if (b) {
+ c._o.expectStaticType<Exactly<Object?>>();
+ }
+}
+
+void usingIsTest_disabledByAssignment(C c, C c2) {
+ bool b = c._o is int;
+ if (c._o is int) {
+ c._o.expectStaticType<Exactly<int>>();
+ }
+ c = c2;
+ if (b) {
+ c._o.expectStaticType<Exactly<Object?>>();
+ }
+}
+
+main() {
+ usingNullCheck_withoutInterveningPromotion(C(0));
+ usingIsTest_withoutInterveningPromotion(C(0));
+ usingNullCheck_withInterveningRelatedPromotion(C(0));
+ usingIsTest_withInterveningRelatedPromotion(C(0));
+ usingNullCheck_withInterveningUnrelatedPromotion(C(0), 1);
+ usingIsTest_withInterveningUnrelatedPromotion(C(0), 1);
+ usingNullCheck_disabledByAssignment(C(0), C(1));
+ usingIsTest_disabledByAssignment(C(0), C(1));
+}
