mirror of
https://github.com/dart-lang/sdk
synced 2024-09-05 00:13:50 +00:00
Flow analysis: fix field promotion within cascades of non-promotable targets.
Previously, flow analysis used a hack to make it easy to generate "why not promoted" messages when the user tried to promote a non-promotable field: it treated all field accesses as stable for the purpose of assigning SSA nodes, but avoided promoting non-promotable fields by setting the `_Reference.isPromotable` flag to `false`. So, for instance, in the following code, both subexpressions `c.i` got assigned the same SSA node, even though there's no guarantee that `C.x` will return the same value each time it's invoked. class C { int? get i => ...; } f(C c) { if (c.i != null) { var i = c.i; // Inferred type `int?` } } This mostly worked, since the SSA node assigned by flow analysis is only used for promotion, and promotion is disabled for non-promotable fields. However, it broke when the field in question was used as the target of a cascade, because fields within cascades always had their `_Reference.isPromotable` flag set to `true` regardless of whether the corresponding cascade target is promotable. For example: class C { D? get d => ...; } class D { final E? _e; ... } class E { m() { ... } } f(C c) { (c.d) .._e!.m() // OK; promotes _e .._e.m(); // OK; _e is promoted now (c.d) .._e.m(); // OOPS, _e is still promoted; it shouldn't be } See `tests/language/inference_update_2/cascaded_field_promotion_unstable_target_test.dart` for a more detailed example. This CL removes the hack; now, when a non-promotable property is accessed more than once, flow analysis assignes a different SSA node for each access. As a result, the `_Reference.isPromotable` is not needed, because non-promotable fields simply never have the chance to be promoted (since every field access gets a separate SSA node, so type checking one field access has no effect on others). To preserve the ability to generate "why not promoted" messages, the `_PropertySsaNode` class now contains a `previousSsaNode` pointer, which links together the separate SSA nodes allocated for non-promotable properties, so that they form a linked list. The "why not promoted" logic traverses this list to figure out which promotions *would* have occurred if the property had been promotable. In order to make it efficient to create this linked list, the `SsaNode` class also had to acquire a `_nonPromotableProperties` map, which records the SSA node that was allocated the last time each property was accessed. Fixes https://github.com/dart-lang/sdk/issues/52728. Change-Id: I16a7b27f77c309bdccce86195a53398e32e8f75d Bug: https://github.com/dart-lang/sdk/issues/52728 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/318745 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>
This commit is contained in:
parent
f53a1f1cdd
commit
9a3420f1d4
|
@ -2980,14 +2980,16 @@ class SsaNode<Type extends Object> {
|
|||
@visibleForTesting
|
||||
final ExpressionInfo<Type>? expressionInfo;
|
||||
|
||||
/// Map containing the set of properties of the value tracked by this SSA node
|
||||
/// for the purpose of type promotion. Keys are the names of the properties.
|
||||
///
|
||||
/// Note that all property accesses are tracked, regardless of whether they
|
||||
/// are promotable, so that if an error occurs due to the absence of type
|
||||
/// promotion, it will be possible to generate a message explaining to the
|
||||
/// user why type promotion failed.
|
||||
final Map<String, _PropertySsaNode<Type>> _properties = {};
|
||||
/// Map containing the set of promotable properties of the value tracked by
|
||||
/// this SSA node. Keys are the names of the properties.
|
||||
final Map<String, _PropertySsaNode<Type>> _promotableProperties = {};
|
||||
|
||||
/// Map containing the set of non-promotable properties of the value tracked
|
||||
/// by this SSA node. These are tracked even though they're not promotable, so
|
||||
/// that if an error occurs due to the absence of type promotion, it will be
|
||||
/// possible to generate a message explaining to the user why type promotion
|
||||
/// failed.
|
||||
final Map<String, _PropertySsaNode<Type>> _nonPromotableProperties = {};
|
||||
|
||||
SsaNode(this.expressionInfo);
|
||||
|
||||
|
@ -2998,9 +3000,29 @@ class SsaNode<Type extends Object> {
|
|||
/// [promotionKeyStore], so that type promotions for it can be tracked
|
||||
/// separately from other type promotions.
|
||||
_PropertySsaNode<Type> getProperty(
|
||||
String propertyName, PromotionKeyStore<Object> promotionKeyStore) =>
|
||||
_properties[propertyName] ??=
|
||||
String propertyName, PromotionKeyStore<Object> promotionKeyStore,
|
||||
{required bool isPromotable}) {
|
||||
if (isPromotable) {
|
||||
// The property is promotable, meaning it is known to produce the same (or
|
||||
// equivalent) value every time it is queried. So we only create an SSA
|
||||
// node if the property hasn't been accessed before; otherwise we return
|
||||
// the old SSA node unchanged.
|
||||
return _promotableProperties[propertyName] ??=
|
||||
new _PropertySsaNode(promotionKeyStore.makeTemporaryKey());
|
||||
} else {
|
||||
// The property isn't promotable, meaning it is not known to produce the
|
||||
// same (or equivalent) value every time it is queried. So we create a
|
||||
// fresh SSA node for every access; but we record the previous SSA node in
|
||||
// `_PropertySsaNode.previousSsaNode` so that the "why not promoted" logic
|
||||
// can figure out what promotions *would* have occurred if the field had
|
||||
// been promotable.
|
||||
_PropertySsaNode<Type>? previousSsaNode =
|
||||
_nonPromotableProperties[propertyName];
|
||||
return _nonPromotableProperties[propertyName] = new _PropertySsaNode(
|
||||
promotionKeyStore.makeTemporaryKey(),
|
||||
previousSsaNode: previousSsaNode);
|
||||
}
|
||||
}
|
||||
|
||||
@override
|
||||
String toString() {
|
||||
|
@ -3055,7 +3077,6 @@ class TrivialVariableReference<Type extends Object> extends _Reference<Type> {
|
|||
{required super.type,
|
||||
required super.after,
|
||||
required super.promotionKey,
|
||||
required super.isPromotable,
|
||||
required super.isThisOrSuper,
|
||||
required super.ssaNode})
|
||||
: super.trivial();
|
||||
|
@ -3085,7 +3106,6 @@ class TrivialVariableReference<Type extends Object> extends _Reference<Type> {
|
|||
promotionKey: promotionKey,
|
||||
after: current,
|
||||
type: _type,
|
||||
isPromotable: isPromotable,
|
||||
isThisOrSuper: isThisOrSuper,
|
||||
ifTrue: previousExpressionInfo.ifTrue
|
||||
.rebaseForward(typeOperations, current),
|
||||
|
@ -3101,8 +3121,7 @@ class TrivialVariableReference<Type extends Object> extends _Reference<Type> {
|
|||
|
||||
@override
|
||||
String toString() => 'TrivialVariableReference(type: $_type, after: $after, '
|
||||
'promotionKey: $promotionKey, isPromotable: $isPromotable, '
|
||||
'isThisOrSuper: $isThisOrSuper)';
|
||||
'promotionKey: $promotionKey, isThisOrSuper: $isThisOrSuper)';
|
||||
}
|
||||
|
||||
/// An instance of the [VariableModel] class represents the information gathered
|
||||
|
@ -5018,10 +5037,7 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
|
|||
if (referenceWithType != null) {
|
||||
VariableModel<Type>? currentVariableInfo =
|
||||
_current.variableInfo[referenceWithType.promotionKey];
|
||||
if (currentVariableInfo != null) {
|
||||
return _getNonPromotionReasons(
|
||||
referenceWithType, currentVariableInfo);
|
||||
}
|
||||
return _getNonPromotionReasons(referenceWithType, currentVariableInfo);
|
||||
}
|
||||
}
|
||||
return () => {};
|
||||
|
@ -5168,20 +5184,34 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
|
|||
}
|
||||
|
||||
Map<Type, NonPromotionReason> Function() _getNonPromotionReasons(
|
||||
_Reference<Type> reference, VariableModel<Type> currentVariableInfo) {
|
||||
_Reference<Type> reference, VariableModel<Type>? currentVariableInfo) {
|
||||
if (reference is _PropertyReference<Type>) {
|
||||
List<Type>? promotedTypes = currentVariableInfo.promotedTypes;
|
||||
if (promotedTypes != null) {
|
||||
_PropertySsaNode<Type>? ssaNode =
|
||||
(reference.ssaNode as _PropertySsaNode<Type>).previousSsaNode;
|
||||
List<List<Type>>? allPreviouslyPromotedTypes;
|
||||
while (ssaNode != null) {
|
||||
VariableModel<Type> previousVariableInfo =
|
||||
_current._getInfo(ssaNode.promotionKey);
|
||||
List<Type>? promotedTypes = previousVariableInfo.promotedTypes;
|
||||
if (promotedTypes != null) {
|
||||
(allPreviouslyPromotedTypes ??= []).add(promotedTypes);
|
||||
}
|
||||
ssaNode = ssaNode.previousSsaNode;
|
||||
}
|
||||
if (allPreviouslyPromotedTypes != null) {
|
||||
return () {
|
||||
Map<Type, NonPromotionReason> result = <Type, NonPromotionReason>{};
|
||||
for (Type type in promotedTypes) {
|
||||
result[type] = new PropertyNotPromoted(reference.propertyName,
|
||||
reference.propertyMember, reference._type);
|
||||
for (List<Type> previouslyPromotedTypes
|
||||
in allPreviouslyPromotedTypes!) {
|
||||
for (Type type in previouslyPromotedTypes) {
|
||||
result[type] = new PropertyNotPromoted(reference.propertyName,
|
||||
reference.propertyMember, reference._type);
|
||||
}
|
||||
}
|
||||
return result;
|
||||
};
|
||||
}
|
||||
} else {
|
||||
} else if (currentVariableInfo != null) {
|
||||
Variable? variable =
|
||||
promotionKeyStore.variableForKey(reference.promotionKey);
|
||||
if (variable == null) {
|
||||
|
@ -5317,34 +5347,23 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
|
|||
case ExpressionPropertyTarget(:var expression):
|
||||
_Reference<Type>? targetReference = _getExpressionReference(expression);
|
||||
if (targetReference == null) return null;
|
||||
// If `targetReference` refers to a non-promotable property or variable,
|
||||
// then the result of the property access is also non-promotable (e.g.
|
||||
// `x._nonFinalField._finalField` is not promotable, because
|
||||
// `_nonFinalField` might change at any time). Note that even though the
|
||||
// control flow paths for `SuperPropertyTarget` and `ThisPropertyTarget`
|
||||
// skip this code, we still need to check `isThisOrSuper`, because
|
||||
// `ThisPropertyTarget` is only used for property accesses via
|
||||
// *implicit* `this`.
|
||||
if (!targetReference.isPromotable && !targetReference.isThisOrSuper) {
|
||||
isPromotable = false;
|
||||
}
|
||||
targetSsaNode = targetReference.ssaNode;
|
||||
}
|
||||
_PropertySsaNode<Type> propertySsaNode =
|
||||
targetSsaNode.getProperty(propertyName, promotionKeyStore);
|
||||
_PropertySsaNode<Type> propertySsaNode = targetSsaNode.getProperty(
|
||||
propertyName, promotionKeyStore,
|
||||
isPromotable: isPromotable);
|
||||
_PropertyReference<Type> propertyReference = new _PropertyReference<Type>(
|
||||
propertyName: propertyName,
|
||||
propertyMember: propertyMember,
|
||||
promotionKey: propertySsaNode.promotionKey,
|
||||
after: _current,
|
||||
type: staticType,
|
||||
isPromotable: isPromotable,
|
||||
ssaNode: propertySsaNode);
|
||||
if (wholeExpression != null) {
|
||||
_storeExpressionInfo(wholeExpression, propertyReference);
|
||||
_storeExpressionReference(wholeExpression, propertyReference);
|
||||
}
|
||||
if (!propertyReference.isPromotable) {
|
||||
if (!isPromotable) {
|
||||
return null;
|
||||
}
|
||||
Type? promotedType =
|
||||
|
@ -5412,7 +5431,6 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
|
|||
promotionKey: promotionKey,
|
||||
after: _current,
|
||||
type: type,
|
||||
isPromotable: true,
|
||||
isThisOrSuper: false,
|
||||
ssaNode: ssaNode);
|
||||
}
|
||||
|
@ -5535,7 +5553,6 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
|
|||
promotionKey: promotionKeyStore.thisPromotionKey,
|
||||
after: _current,
|
||||
type: staticType,
|
||||
isPromotable: false,
|
||||
isThisOrSuper: true,
|
||||
ssaNode: isSuper ? _superSsaNode : _thisSsaNode);
|
||||
|
||||
|
@ -5546,7 +5563,6 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
|
|||
promotionKey: variableKey,
|
||||
after: _current,
|
||||
type: info.promotedTypes?.last ?? unpromotedType,
|
||||
isPromotable: true,
|
||||
isThisOrSuper: false,
|
||||
ssaNode: info.ssaNode ?? new SsaNode<Type>(null));
|
||||
}
|
||||
|
@ -6440,7 +6456,6 @@ class _PatternContext<Type extends Object> extends _FlowContext {
|
|||
promotionKey: _matchedValueInfo.promotionKey,
|
||||
after: current,
|
||||
type: matchedType,
|
||||
isPromotable: true,
|
||||
isThisOrSuper: false,
|
||||
ssaNode: new SsaNode<Type>(null));
|
||||
}
|
||||
|
@ -6461,15 +6476,13 @@ class _PropertyReference<Type extends Object> extends _Reference<Type> {
|
|||
required this.propertyName,
|
||||
required this.propertyMember,
|
||||
required super.promotionKey,
|
||||
required super.isPromotable,
|
||||
required super.ssaNode})
|
||||
: super.trivial(isThisOrSuper: false);
|
||||
|
||||
@override
|
||||
String toString() => '_PropertyReference('
|
||||
'type: $_type, after: $after, propertyName: $propertyName, '
|
||||
'propertyMember: $propertyMember, promotionKey: $promotionKey, '
|
||||
'isPromotable: $isPromotable)';
|
||||
'propertyMember: $propertyMember, promotionKey: $promotionKey)';
|
||||
}
|
||||
|
||||
/// Data structure representing a unique value returned by the invocation of a
|
||||
|
@ -6479,7 +6492,14 @@ class _PropertySsaNode<Type extends Object> extends SsaNode<Type> {
|
|||
/// promotion.
|
||||
final int promotionKey;
|
||||
|
||||
_PropertySsaNode(this.promotionKey) : super(null);
|
||||
/// If this property is not promotable, then a fresh SSA node is assigned at
|
||||
/// the time of each access; when that occurs, this field points to the
|
||||
/// previous SSA node associated with the same property; otherwise it is
|
||||
/// `null`. This is used by the "why not promoted" logic to figure out what
|
||||
/// promotions *would* have occurred if the property had been promotable.
|
||||
final _PropertySsaNode<Type>? previousSsaNode;
|
||||
|
||||
_PropertySsaNode(this.promotionKey, {this.previousSsaNode}) : super(null);
|
||||
}
|
||||
|
||||
/// Specialization of [ExpressionInfo] for the case where the expression is a
|
||||
|
@ -6489,9 +6509,6 @@ class _Reference<Type extends Object> extends ExpressionInfo<Type> {
|
|||
/// [FlowModel.variableInfo].
|
||||
final int promotionKey;
|
||||
|
||||
/// Whether the thing referred to by this expression is promotable.
|
||||
final bool isPromotable;
|
||||
|
||||
/// Whether the thing referred to by this expression is `this` (or the
|
||||
/// pseudo-expression `super`).
|
||||
final bool isThisOrSuper;
|
||||
|
@ -6505,7 +6522,6 @@ class _Reference<Type extends Object> extends ExpressionInfo<Type> {
|
|||
required super.ifTrue,
|
||||
required super.ifFalse,
|
||||
required this.promotionKey,
|
||||
required this.isPromotable,
|
||||
required this.isThisOrSuper,
|
||||
required this.ssaNode});
|
||||
|
||||
|
@ -6513,7 +6529,6 @@ class _Reference<Type extends Object> extends ExpressionInfo<Type> {
|
|||
{required super.type,
|
||||
required super.after,
|
||||
required this.promotionKey,
|
||||
required this.isPromotable,
|
||||
required this.isThisOrSuper,
|
||||
required this.ssaNode})
|
||||
: super.trivial();
|
||||
|
@ -6521,8 +6536,7 @@ class _Reference<Type extends Object> extends ExpressionInfo<Type> {
|
|||
@override
|
||||
String toString() => '_Reference(type: $_type, after: $after, '
|
||||
'ifTrue: $ifTrue, ifFalse: $ifFalse, promotionKey: $promotionKey, '
|
||||
'isPromotable: $isPromotable, isThisOrSuper: $isThisOrSuper, '
|
||||
'ssaNode: $ssaNode)';
|
||||
'isThisOrSuper: $isThisOrSuper, ssaNode: $ssaNode)';
|
||||
}
|
||||
|
||||
/// [_FlowContext] representing a construct that can contain one or more
|
||||
|
|
|
@ -6454,6 +6454,57 @@ main() {
|
|||
]);
|
||||
});
|
||||
});
|
||||
|
||||
test('unstable target', () {
|
||||
h.addMember('C', 'd', 'D', promotable: false);
|
||||
h.addMember('D', '_i', 'int?', promotable: true);
|
||||
var c = Var('c');
|
||||
h.run([
|
||||
declare(c, initializer: expr('C')),
|
||||
// The value of `c.d` is cached in a temporary variable, call it `t0`.
|
||||
c.property('d').cascade([
|
||||
// `t0._i` could be null at this point.
|
||||
(t0) => t0.property('_i').checkType('int?'),
|
||||
// But now we promote it to non-null
|
||||
(t0) => t0.property('_i').nonNullAssert,
|
||||
// And the promotion sticks for the duration of the cascade.
|
||||
(t0) => t0.property('_i').checkType('int'),
|
||||
]),
|
||||
// Now, a new value of `c.d` is computed, and cached in a new
|
||||
// temporary variable, call it `t1`.
|
||||
c.property('d').cascade([
|
||||
// even though `t0._i` was promoted above, `t1._i` could still be
|
||||
// null at this point.
|
||||
(t1) => t1.property('_i').checkType('int?'),
|
||||
// But now we promote it to non-null
|
||||
(t1) => t1.property('_i').nonNullAssert,
|
||||
// And the promotion sticks for the duration of the cascade.
|
||||
(t1) => t1.property('_i').checkType('int'),
|
||||
]),
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
test('field becomes promotable after type test', () {
|
||||
// In this test, `C._property` is not promotable, but `D` extends `C`, and
|
||||
// `D._property` is promotable. (This could happen if, for example,
|
||||
// `C._property` is an abstract getter, and `D._property` is a final
|
||||
// field). If `_property` is type-tested while the type of the target is
|
||||
// `C`, but then `_property` is accessed while the type of the target is
|
||||
// `D`, no promotion occurs, because the thing that is type tested is
|
||||
// non-promotable.
|
||||
h.addMember('C', '_property', 'int?', promotable: false);
|
||||
h.addMember('D', '_property', 'int?', promotable: true);
|
||||
h.addSuperInterfaces('D', (_) => [Type('C'), Type('Object')]);
|
||||
var x = Var('x');
|
||||
h.run([
|
||||
declare(x, initializer: expr('C')),
|
||||
x.property('_property').nonNullAssert,
|
||||
x.as_('D'),
|
||||
checkNotPromoted(x.property('_property')),
|
||||
x.property('_property').nonNullAssert,
|
||||
checkPromoted(x.property('_property'), 'int'),
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
|
@ -9932,7 +9983,6 @@ extension on FlowModel<Type> {
|
|||
?.promotedTypes
|
||||
?.last ??
|
||||
variable.type,
|
||||
isPromotable: true,
|
||||
isThisOrSuper: false,
|
||||
ssaNode: SsaNode(null));
|
||||
|
||||
|
|
Loading…
Reference in a new issue