From 9a3420f1d4e0f5e37cf758acc159ed0c651f95e4 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Tue, 8 Aug 2023 17:26:09 +0000 Subject: [PATCH] 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 Commit-Queue: Paul Berry --- .../lib/src/flow_analysis/flow_analysis.dart | 124 ++++++++++-------- .../flow_analysis/flow_analysis_test.dart | 52 +++++++- 2 files changed, 120 insertions(+), 56 deletions(-) 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 638f50c7138d..1e1cd7161ad3 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 @@ -2980,14 +2980,16 @@ class SsaNode { @visibleForTesting final ExpressionInfo? 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> _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> _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> _nonPromotableProperties = {}; SsaNode(this.expressionInfo); @@ -2998,9 +3000,29 @@ class SsaNode { /// [promotionKeyStore], so that type promotions for it can be tracked /// separately from other type promotions. _PropertySsaNode getProperty( - String propertyName, PromotionKeyStore promotionKeyStore) => - _properties[propertyName] ??= + String propertyName, PromotionKeyStore 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? previousSsaNode = + _nonPromotableProperties[propertyName]; + return _nonPromotableProperties[propertyName] = new _PropertySsaNode( + promotionKeyStore.makeTemporaryKey(), + previousSsaNode: previousSsaNode); + } + } @override String toString() { @@ -3055,7 +3077,6 @@ class TrivialVariableReference extends _Reference { {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 extends _Reference { promotionKey: promotionKey, after: current, type: _type, - isPromotable: isPromotable, isThisOrSuper: isThisOrSuper, ifTrue: previousExpressionInfo.ifTrue .rebaseForward(typeOperations, current), @@ -3101,8 +3121,7 @@ class TrivialVariableReference extends _Reference { @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? currentVariableInfo = _current.variableInfo[referenceWithType.promotionKey]; - if (currentVariableInfo != null) { - return _getNonPromotionReasons( - referenceWithType, currentVariableInfo); - } + return _getNonPromotionReasons(referenceWithType, currentVariableInfo); } } return () => {}; @@ -5168,20 +5184,34 @@ class _FlowAnalysisImpl Function() _getNonPromotionReasons( - _Reference reference, VariableModel currentVariableInfo) { + _Reference reference, VariableModel? currentVariableInfo) { if (reference is _PropertyReference) { - List? promotedTypes = currentVariableInfo.promotedTypes; - if (promotedTypes != null) { + _PropertySsaNode? ssaNode = + (reference.ssaNode as _PropertySsaNode).previousSsaNode; + List>? allPreviouslyPromotedTypes; + while (ssaNode != null) { + VariableModel previousVariableInfo = + _current._getInfo(ssaNode.promotionKey); + List? promotedTypes = previousVariableInfo.promotedTypes; + if (promotedTypes != null) { + (allPreviouslyPromotedTypes ??= []).add(promotedTypes); + } + ssaNode = ssaNode.previousSsaNode; + } + if (allPreviouslyPromotedTypes != null) { return () { Map result = {}; - for (Type type in promotedTypes) { - result[type] = new PropertyNotPromoted(reference.propertyName, - reference.propertyMember, reference._type); + for (List 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? 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 propertySsaNode = - targetSsaNode.getProperty(propertyName, promotionKeyStore); + _PropertySsaNode propertySsaNode = targetSsaNode.getProperty( + propertyName, promotionKeyStore, + isPromotable: isPromotable); _PropertyReference propertyReference = new _PropertyReference( 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(null)); } @@ -6440,7 +6456,6 @@ class _PatternContext extends _FlowContext { promotionKey: _matchedValueInfo.promotionKey, after: current, type: matchedType, - isPromotable: true, isThisOrSuper: false, ssaNode: new SsaNode(null)); } @@ -6461,15 +6476,13 @@ class _PropertyReference extends _Reference { 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 extends SsaNode { /// 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? 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 extends ExpressionInfo { /// [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 extends ExpressionInfo { required super.ifTrue, required super.ifFalse, required this.promotionKey, - required this.isPromotable, required this.isThisOrSuper, required this.ssaNode}); @@ -6513,7 +6529,6 @@ class _Reference extends ExpressionInfo { {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 extends ExpressionInfo { @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 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 b9c516df5e9f..522145d6c458 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 @@ -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 { ?.promotedTypes ?.last ?? variable.type, - isPromotable: true, isThisOrSuper: false, ssaNode: SsaNode(null));