Unify "why not promoted" representations for for-loops and other writes.

This CL changes the responsibility for doing flow analysis of the
implicit variable write in a `for-in` loop as follows: if the `for-in`
loop does not declare a variable, but it assigns to a local variable,
then the flow analysis client is responsible for calling `write` on
entry to the loop.

This in turn allows us to use a single code path to track "why not
promoted" information related to all possible local variable writes;
we no longer need as much special case logic to handle for-in loops.

To make the analyzer integration slightly cleaner, we change the
argument type of FlowAnalysis.write to Node rather than Expression, so
that the analyzer can pass in the ForEachParts object when analyzing a
for-in loop.

Bug: #44898
Change-Id: I24b47be8eac2e276cd291a5b2f2e4444c911138f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193837
Reviewed-by: Konstantin Shcheglov <[email protected]>

Author: stereotype441

pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart

@@ -300,16 +300,16 @@ class AssignedVariablesNodeInfo<Variable extends Object> {
 /// Non-promotion reason describing the situation where a variable was not
 /// promoted due to an explicit write to the variable appearing somewhere in the
 /// source code.
-class DemoteViaExplicitWrite<Variable extends Object, Expression extends Object>
+class DemoteViaExplicitWrite<Variable extends Object>
     extends NonPromotionReason {
   /// The local variable that was not promoted.
   final Variable variable;
 
-  /// The expression that wrote to the variable; this corresponds to an
-  /// expression that was passed to [FlowAnalysis.write].
-  final Expression writeExpression;
+  /// The node that wrote to the variable; this corresponds to a node that was
+  /// passed to [FlowAnalysis.write].
+  final Object node;
 
-  DemoteViaExplicitWrite(this.variable, this.writeExpression);
+  DemoteViaExplicitWrite(this.variable, this.node);
 
   @override
   String get documentationLink => 'http://dart.dev/go/non-promo-write';
@@ -318,46 +318,14 @@ class DemoteViaExplicitWrite<Variable extends Object, Expression extends Object>
   String get shortName => 'explicitWrite';
 
   @override
-  R accept<R, Node extends Object, Expression extends Object,
-              Variable extends Object, Type extends Object>(
-          NonPromotionReasonVisitor<R, Node, Expression, Variable, Type>
-              visitor) =>
+  R accept<R, Node extends Object, Variable extends Object,
+              Type extends Object>(
+          NonPromotionReasonVisitor<R, Node, Variable, Type> visitor) =>
       visitor.visitDemoteViaExplicitWrite(
-          this as DemoteViaExplicitWrite<Variable, Expression>);
+          this as DemoteViaExplicitWrite<Variable>);
 
   @override
-  String toString() => 'DemoteViaExplicitWrite($writeExpression)';
-}
-
-/// Non-promotion reason describing the situation where a variable was not
-/// promoted due to the variable appearing before the word `in` in a "for each"
-/// statement or a "for each" collection element.
-class DemoteViaForEachVariableWrite<Variable extends Object,
-    Node extends Object> extends NonPromotionReason {
-  /// The local variable that was not promoted.
-  final Variable variable;
-
-  /// The "for each" statement or collection element that wrote to the variable.
-  final Node node;
-
-  DemoteViaForEachVariableWrite(this.variable, this.node);
-
-  @override
-  String get documentationLink => 'http://dart.dev/go/non-promo-write';
-
-  @override
-  String get shortName => 'explicitWrite';
-
-  @override
-  R accept<R, Node extends Object, Expression extends Object,
-              Variable extends Object, Type extends Object>(
-          NonPromotionReasonVisitor<R, Node, Expression, Variable, Type>
-              visitor) =>
-      visitor.visitDemoteViaForEachVariableWrite(
-          this as DemoteViaForEachVariableWrite<Variable, Node>);
-
-  @override
-  String toString() => 'DemoteViaForEachVariableWrite($node)';
+  String toString() => 'DemoteViaExplicitWrite($node)';
 }
 
 /// A collection of flow models representing the possible outcomes of evaluating
@@ -565,11 +533,8 @@ abstract class FlowAnalysis<Node extends Object, Statement extends Node,
   /// - Call [forEach_end].
   ///
   /// [node] should be the same node that was passed to
-  /// [AssignedVariables.endNode] for the for statement.  [loopVariable] should
-  /// be the variable assigned to by the loop (if it is promotable, otherwise
-  /// null).  [writtenType] should be the type written to that variable (i.e.
-  /// if the loop iterates over `List<Foo>`, it should be `Foo`).
-  void forEach_bodyBegin(Node node, Variable? loopVariable, Type writtenType);
+  /// [AssignedVariables.endNode] for the for statement.
+  void forEach_bodyBegin(Node node);
 
   /// Call this method just before visiting the body of a "for-in" statement or
   /// collection element.  See [forEach_bodyBegin] for details.
@@ -982,7 +947,7 @@ abstract class FlowAnalysis<Node extends Object, Statement extends Node,
 
   /// Register write of the given [variable] in the current state.
   /// [writtenType] should be the type of the value that was written.
-  /// [expression] should be the whole expression performing the write.
+  /// [node] should be the syntactic construct performing the write.
   /// [writtenExpression] should be the expression that was written, or `null`
   /// if the expression that was written is not directly represented in the
   /// source code (this happens, for example, with compound assignments and with
@@ -991,7 +956,7 @@ abstract class FlowAnalysis<Node extends Object, Statement extends Node,
   /// This should also be used for the implicit write to a non-final variable in
   /// its initializer, to ensure that the type is promoted to non-nullable if
   /// necessary; in this case, [viaInitializer] should be `true`.
-  void write(Expression expression, Variable variable, Type writtenType,
+  void write(Node node, Variable variable, Type writtenType,
       Expression? writtenExpression);
 
   /// Prints out a summary of the current state of flow analysis, intended for
@@ -1162,9 +1127,9 @@ class FlowAnalysisDebug<Node extends Object, Statement extends Node,
   }
 
   @override
-  void forEach_bodyBegin(Node node, Variable? loopVariable, Type writtenType) {
-    return _wrap('forEach_bodyBegin($node, $loopVariable, $writtenType)',
-        () => _wrapped.forEach_bodyBegin(node, loopVariable, writtenType));
+  void forEach_bodyBegin(Node node) {
+    return _wrap(
+        'forEach_bodyBegin($node)', () => _wrapped.forEach_bodyBegin(node));
   }
 
   @override
@@ -1513,12 +1478,10 @@ class FlowAnalysisDebug<Node extends Object, Statement extends Node,
   }
 
   @override
-  void write(Expression expression, Variable variable, Type writtenType,
+  void write(Node node, Variable variable, Type writtenType,
       Expression? writtenExpression) {
-    _wrap(
-        'write($expression, $variable, $writtenType, $writtenExpression)',
-        () => _wrapped.write(
-            expression, variable, writtenType, writtenExpression));
+    _wrap('write($node, $variable, $writtenType, $writtenExpression)',
+        () => _wrapped.write(node, variable, writtenType, writtenExpression));
   }
 
   @override
@@ -2348,21 +2311,17 @@ abstract class NonPromotionReason {
   String get shortName;
 
   /// Implementation of the visitor pattern for non-promotion reasons.
-  R accept<R, Node extends Object, Expression extends Object,
-          Variable extends Object, Type extends Object>(
-      NonPromotionReasonVisitor<R, Node, Expression, Variable, Type> visitor);
+  R accept<R, Node extends Object, Variable extends Object,
+          Type extends Object>(
+      NonPromotionReasonVisitor<R, Node, Variable, Type> visitor);
 }
 
 /// Implementation of the visitor pattern for non-promotion reasons.
 abstract class NonPromotionReasonVisitor<R, Node extends Object,
-    Expression extends Object, Variable extends Object, Type extends Object> {
+    Variable extends Object, Type extends Object> {
   NonPromotionReasonVisitor._() : assert(false, 'Do not extend this class');
 
-  R visitDemoteViaExplicitWrite(
-      DemoteViaExplicitWrite<Variable, Expression> reason);
-
-  R visitDemoteViaForEachVariableWrite(
-      DemoteViaForEachVariableWrite<Variable, Node> reason);
+  R visitDemoteViaExplicitWrite(DemoteViaExplicitWrite<Variable> reason);
 
   R visitPropertyNotPromoted(PropertyNotPromoted<Type> reason);
 
@@ -2389,10 +2348,9 @@ class PropertyNotPromoted<Type extends Object> extends NonPromotionReason {
   String get shortName => 'propertyNotPromoted';
 
   @override
-  R accept<R, Node extends Object, Expression extends Object,
-              Variable extends Object, Type extends Object>(
-          NonPromotionReasonVisitor<R, Node, Expression, Variable, Type>
-              visitor) =>
+  R accept<R, Node extends Object, Variable extends Object,
+              Type extends Object>(
+          NonPromotionReasonVisitor<R, Node, Variable, Type> visitor) =>
       visitor.visitPropertyNotPromoted(this as PropertyNotPromoted<Type>);
 }
 
@@ -2627,10 +2585,9 @@ class ThisNotPromoted extends NonPromotionReason {
   String get shortName => 'thisNotPromoted';
 
   @override
-  R accept<R, Node extends Object, Expression extends Object,
-              Variable extends Object, Type extends Object>(
-          NonPromotionReasonVisitor<R, Node, Expression, Variable, Type>
-              visitor) =>
+  R accept<R, Node extends Object, Variable extends Object,
+              Type extends Object>(
+          NonPromotionReasonVisitor<R, Node, Variable, Type> visitor) =>
       visitor.visitThisNotPromoted(this);
 }
 
@@ -3680,22 +3637,14 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
   }
 
   @override
-  void forEach_bodyBegin(Node node, Variable? loopVariable, Type writtenType) {
+  void forEach_bodyBegin(Node node) {
     AssignedVariablesNodeInfo<Variable> info =
         _assignedVariables._getInfoForNode(node);
     _current = _current.conservativeJoin(info._written, info._captured).split();
     _SimpleStatementContext<Variable, Type> context =
         new _SimpleStatementContext<Variable, Type>(
             _current.reachable.parent!, _current);
     _stack.add(context);
-    if (loopVariable != null) {
-      _current = _current.write(
-          new DemoteViaForEachVariableWrite<Variable, Node>(loopVariable, node),
-          loopVariable,
-          writtenType,
-          new SsaNode<Variable, Type>(null),
-          typeOperations);
-    }
   }
 
   @override
@@ -4220,15 +4169,15 @@ class _FlowAnalysisImpl<Node extends Object, Statement extends Node,
   }
 
   @override
-  void write(Expression expression, Variable variable, Type writtenType,
+  void write(Node node, Variable variable, Type writtenType,
       Expression? writtenExpression) {
     ExpressionInfo<Variable, Type>? expressionInfo = writtenExpression == null
         ? null
         : _getExpressionInfo(writtenExpression);
     SsaNode<Variable, Type> newSsaNode = new SsaNode<Variable, Type>(
         expressionInfo is _TrivialExpressionInfo ? null : expressionInfo);
     _current = _current.write(
-        new DemoteViaExplicitWrite<Variable, Expression>(variable, expression),
+        new DemoteViaExplicitWrite<Variable>(variable, node),
         variable,
         writtenType,
         newSsaNode,
@@ -4509,8 +4458,7 @@ class _LegacyTypePromotion<Node extends Object, Statement extends Node,
   void for_updaterBegin() {}
 
   @override
-  void forEach_bodyBegin(
-      Node node, Variable? loopVariable, Type? writtenType) {}
+  void forEach_bodyBegin(Node node) {}
 
   @override
   void forEach_end() {}
@@ -4815,7 +4763,7 @@ class _LegacyTypePromotion<Node extends Object, Statement extends Node,
   }
 
   @override
-  void write(Expression expression, Variable variable, Type writtenType,
+  void write(Node node, Variable variable, Type writtenType,
       Expression? writtenExpression) {
     assert(
         _assignedVariables._anywhere._written.contains(variable),

pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart

@@ -5500,8 +5500,8 @@ main() {
         x.expr.whyNotPromoted((reasons) {
           expect(reasons.keys, unorderedEquals([Type('int')]));
           var nonPromotionReason =
-              reasons.values.single as DemoteViaExplicitWrite<Var, Expression>;
-          expect(nonPromotionReason.writeExpression, same(writeExpression));
+              reasons.values.single as DemoteViaExplicitWrite<Var>;
+          expect(nonPromotionReason.node, same(writeExpression));
         }).stmt,
       ]);
     });
@@ -5523,13 +5523,9 @@ main() {
         checkNotPromoted(x),
         x.expr.whyNotPromoted((reasons) {
           expect(reasons.keys, unorderedEquals([Type('int'), Type('int?')]));
-          expect(
-              (reasons[Type('int')] as DemoteViaExplicitWrite<Var, Expression>)
-                  .writeExpression,
+          expect((reasons[Type('int')] as DemoteViaExplicitWrite<Var>).node,
               same(writeExpression));
-          expect(
-              (reasons[Type('int?')] as DemoteViaExplicitWrite<Var, Expression>)
-                  .writeExpression,
+          expect((reasons[Type('int?')] as DemoteViaExplicitWrite<Var>).node,
               same(writeExpression));
         }).stmt,
       ]);
@@ -5553,8 +5549,8 @@ main() {
         x.expr.whyNotPromoted((reasons) {
           expect(reasons.keys, unorderedEquals([Type('int')]));
           var nonPromotionReason =
-              reasons.values.single as DemoteViaExplicitWrite<Var, Expression>;
-          expect(nonPromotionReason.writeExpression, same(writeExpression));
+              reasons.values.single as DemoteViaExplicitWrite<Var>;
+          expect(nonPromotionReason.node, same(writeExpression));
         }).stmt,
       ]);
     });
@@ -5577,8 +5573,8 @@ main() {
         checkPromoted(x, 'num'),
         x.expr.whyNotPromoted((reasons) {
           var nonPromotionReason =
-              reasons[Type('int')] as DemoteViaExplicitWrite;
-          expect(nonPromotionReason.writeExpression, same(writeExpression));
+              reasons[Type('int')] as DemoteViaExplicitWrite<Var>;
+          expect(nonPromotionReason.node, same(writeExpression));
         }).stmt,
       ]);
     });
@@ -5759,10 +5755,9 @@ class _MockNonPromotionReason extends NonPromotionReason {
 
   String get shortName => fail('Unexpected call to shortName');
 
-  R accept<R, Node extends Object, Expression extends Object,
-              Variable extends Object, Type extends Object>(
-          NonPromotionReasonVisitor<R, Node, Expression, Variable, Type>
-              visitor) =>
+  R accept<R, Node extends Object, Variable extends Object,
+              Type extends Object>(
+          NonPromotionReasonVisitor<R, Node, Variable, Type> visitor) =>
       fail('Unexpected call to accept');
 }
 

pkg/_fe_analyzer_shared/test/mini_ast.dart

@@ -1143,7 +1143,11 @@ class _ForEach extends Statement {
   void _visit(
       Harness h, FlowAnalysis<Node, Statement, Expression, Var, Type> flow) {
     var iteratedType = h._getIteratedType(iterable._visit(h, flow));
-    flow.forEach_bodyBegin(this, variable, iteratedType);
+    flow.forEach_bodyBegin(this);
+    var variable = this.variable;
+    if (variable != null && !declaresVariable) {
+      flow.write(this, variable, iteratedType, null);
+    }
     body._visit(h, flow);
     flow.forEach_end();
   }

pkg/analyzer/lib/src/dart/resolver/for_resolver.dart

@@ -132,13 +132,12 @@ class ForResolver {
           ?.declare(loopVariable.declaredElement!, true);
     }
 
-    _resolver.flowAnalysis?.flow?.forEach_bodyBegin(
-      node,
-      identifierElement is PromotableElement
-          ? identifierElement
-          : loopVariable?.declaredElement,
-      elementType ?? DynamicTypeImpl.instance,
-    );
+    _resolver.flowAnalysis?.flow?.forEach_bodyBegin(node);
+    if (identifierElement is PromotableElement &&
+        forEachParts is ForEachPartsWithIdentifier) {
+      _resolver.flowAnalysis?.flow?.write(forEachParts, identifierElement,
+          elementType ?? DynamicTypeImpl.instance, null);
+    }
 
     _resolveBody(body);