Skip to content

Commit

Permalink
Flow analysis: fix field promotion within cascades of non-promotable …
Browse files Browse the repository at this point in the history 
…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 #52728.

Change-Id: I16a7b27f77c309bdccce86195a53398e32e8f75d
Bug: #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>
  • Loading branch information
@stereotype441
stereotype441 authored and Commit Queue committed Aug 8, 2023
1 parent f53a1f1 commit 9a3420f
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 56 deletions.
124 changes: 69 additions & 55 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 @@ -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);

Expand All @@ -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() {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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),
Expand All @@ -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
Expand Down Expand Up @@ -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 () => {};
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -5412,7 +5431,6 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
promotionKey: promotionKey,
after: _current,
type: type,
isPromotable: true,
isThisOrSuper: false,
ssaNode: ssaNode);
}
Expand Down Expand Up @@ -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);

Expand All @@ -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));
}
Expand Down Expand Up @@ -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));
}
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -6505,24 +6522,21 @@ 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});

_Reference.trivial(
{required super.type,
required super.after,
required this.promotionKey,
required this.isPromotable,
required this.isThisOrSuper,
required this.ssaNode})
: super.trivial();

@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
Expand Down
52 changes: 51 additions & 1 deletion 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 @@ -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'),
]);
});
});

Expand Down Expand Up @@ -9932,7 +9983,6 @@ extension on FlowModel<Type> {
?.promotedTypes
?.last ??
variable.type,
isPromotable: true,
isThisOrSuper: false,
ssaNode: SsaNode(null));

Expand Down

0 comments on commit 9a3420f

Please sign in to comment.