Flow anaylsis: fix definite assignment tracking for for-each loop vars

Change-Id: Ia0dfd94070e28832c5ffc6fb892626d9036042a4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/117820
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Paul Berry <[email protected]>

Author: stereotype441

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

@@ -244,9 +244,9 @@ class FlowAnalysisHelper {
     return false;
   }
 
-  void loopVariable(DeclaredIdentifier loopVariable) {
-    if (loopVariable != null) {
-      flow.add(loopVariable.declaredElement, assigned: true);
+  void loopVariable(DeclaredIdentifier declaredVariable) {
+    if (declaredVariable != null) {
+      flow.add(declaredVariable.declaredElement, assigned: false);
     }
   }
 
@@ -427,6 +427,16 @@ class _AssignedVariablesVisitor extends RecursiveAstVisitor<void> {
       iterable.accept(this);
 
       assignedVariables.beginNode();
+      if (forLoopParts is ForEachPartsWithIdentifier) {
+        var element = forLoopParts.identifier.staticElement;
+        if (element is VariableElement) {
+          assignedVariables.write(element);
+        }
+      } else if (forLoopParts is ForEachPartsWithDeclaration) {
+        assignedVariables.write(forLoopParts.loopVariable.declaredElement);
+      } else {
+        throw new StateError('Unrecognized for loop parts');
+      }
       body.accept(this);
       assignedVariables.endNode(node);
     } else {

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

@@ -3850,18 +3850,19 @@ class ResolverVisitor extends ScopedVisitor {
       Expression iterable = forLoopParts.iterable;
       DeclaredIdentifier loopVariable;
       DartType valueType;
+      Element identifierElement;
       if (forLoopParts is ForEachPartsWithDeclaration) {
         loopVariable = forLoopParts.loopVariable;
         valueType = loopVariable?.type?.type ?? UnknownInferredType.instance;
       } else if (forLoopParts is ForEachPartsWithIdentifier) {
         SimpleIdentifier identifier = forLoopParts.identifier;
         identifier?.accept(this);
-        Element element = identifier?.staticElement;
-        if (element is VariableElement) {
-          valueType = element.type;
-        } else if (element is PropertyAccessorElement) {
-          if (element.parameters.isNotEmpty) {
-            valueType = element.parameters[0].type;
+        identifierElement = identifier?.staticElement;
+        if (identifierElement is VariableElement) {
+          valueType = identifierElement.type;
+        } else if (identifierElement is PropertyAccessorElement) {
+          if (identifierElement.parameters.isNotEmpty) {
+            valueType = identifierElement.parameters[0].type;
           }
         }
       }
@@ -3880,7 +3881,10 @@ class ResolverVisitor extends ScopedVisitor {
       _flowAnalysis?.loopVariable(loopVariable);
       loopVariable?.accept(this);
       _flowAnalysis?.flow?.forEach_bodyBegin(
-          _flowAnalysis.assignedVariables.writtenInNode(node));
+          _flowAnalysis.assignedVariables.writtenInNode(node),
+          identifierElement is VariableElement
+              ? identifierElement
+              : loopVariable.declaredElement);
       node.body?.accept(this);
       _flowAnalysis?.flow?.forEach_end();
 
@@ -3920,6 +3924,7 @@ class ResolverVisitor extends ScopedVisitor {
       Expression iterable = forLoopParts.iterable;
       DeclaredIdentifier loopVariable;
       SimpleIdentifier identifier;
+      Element identifierElement;
       if (forLoopParts is ForEachPartsWithDeclaration) {
         loopVariable = forLoopParts.loopVariable;
       } else if (forLoopParts is ForEachPartsWithIdentifier) {
@@ -3933,12 +3938,12 @@ class ResolverVisitor extends ScopedVisitor {
         valueType = typeAnnotation?.type ?? UnknownInferredType.instance;
       }
       if (identifier != null) {
-        Element element = identifier.staticElement;
-        if (element is VariableElement) {
-          valueType = element.type;
-        } else if (element is PropertyAccessorElement) {
-          if (element.parameters.isNotEmpty) {
-            valueType = element.parameters[0].type;
+        identifierElement = identifier.staticElement;
+        if (identifierElement is VariableElement) {
+          valueType = identifierElement.type;
+        } else if (identifierElement is PropertyAccessorElement) {
+          if (identifierElement.parameters.isNotEmpty) {
+            valueType = identifierElement.parameters[0].type;
           }
         }
       }
@@ -3957,8 +3962,10 @@ class ResolverVisitor extends ScopedVisitor {
       loopVariable?.accept(this);
 
       _flowAnalysis?.flow?.forEach_bodyBegin(
-        _flowAnalysis.assignedVariables.writtenInNode(node),
-      );
+          _flowAnalysis.assignedVariables.writtenInNode(node),
+          identifierElement is VariableElement
+              ? identifierElement
+              : loopVariable.declaredElement);
 
       Statement body = node.body;
       if (body != null) {

pkg/front_end/lib/src/fasta/flow_analysis/flow_analysis.dart

@@ -407,10 +407,15 @@ class FlowAnalysis<Statement, Expression, Variable, Type> {
   /// - Call [forEach_end].
   ///
   /// [loopAssigned] should be the set of variables that are assigned anywhere
-  /// in the loop's body.
-  void forEach_bodyBegin(Set<Variable> loopAssigned) {
+  /// in the loop's body.  [loopVariable] should be the loop variable, if it's a
+  /// local variable, or `null` otherwise.
+  void forEach_bodyBegin(Set<Variable> loopAssigned, Variable loopVariable) {
     _stack.add(_current);
     _current = _current.removePromotedAll(loopAssigned, _referencedVariables);
+    if (loopVariable != null) {
+      assert(loopAssigned.contains(loopVariable));
+      _current = _current.write(typeOperations, loopVariable);
+    }
   }
 
   /// Call this method just before visiting the body of a "for-in" statement or

pkg/front_end/test/fasta/flow_analysis/flow_analysis_test.dart

@@ -331,18 +331,31 @@ main() {
         h.declare(x, initialized: true);
         h.promote(x, 'int');
         expect(flow.promotedType(x).type, 'int');
-        flow.forEach_bodyBegin({x});
+        flow.forEach_bodyBegin({x}, null);
         expect(flow.promotedType(x), isNull);
         flow.forEach_end();
       });
     });
 
+    test('forEach_bodyBegin() writes to loop variable', () {
+      var h = _Harness();
+      var x = h.addVar('x', 'int?');
+      h.run((flow) {
+        h.declare(x, initialized: false);
+        expect(flow.isAssigned(x), false);
+        flow.forEach_bodyBegin({x}, x);
+        expect(flow.isAssigned(x), true);
+        flow.forEach_end();
+        expect(flow.isAssigned(x), false);
+      });
+    });
+
     test('forEach_end() restores state before loop', () {
       var h = _Harness();
       var x = h.addVar('x', 'int?');
       h.run((flow) {
         h.declare(x, initialized: true);
-        flow.forEach_bodyBegin({});
+        flow.forEach_bodyBegin({}, null);
         h.promote(x, 'int');
         expect(flow.promotedType(x).type, 'int');
         flow.forEach_end();

pkg/front_end/test/flow_analysis/definite_assignment/data/for_each.dart

@@ -33,9 +33,33 @@ forEach_continue(bool c) {
   /*unassigned*/ v2;
 }
 
+forEach_assigns_to_identifier() {
+  late int i;
+  for (i in [0, 1, 2]) {
+    i;
+  }
+  /*unassigned*/ i;
+}
+
+forEach_assigns_to_declared_var() {
+  for (int i in [0, 1, 2]) {
+    i;
+  }
+}
+
 collection_forEach() {
   late Object v1, v2;
   [for (var _ in (v1 = [0, 1, 2])) (v2 = 0)];
   v1;
   /*unassigned*/ v2;
 }
+
+collection_forEach_assigns_to_identifier() {
+  late int i;
+  [for (i in [0, 1, 2]) i];
+  /*unassigned*/ i;
+}
+
+collection_forEach_assigns_to_declared_var() {
+  [for (int i in [0, 1, 2]) i];
+}

pkg/nnbd_migration/lib/src/edge_builder.dart

@@ -1816,7 +1816,8 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
         _flowAnalysis.add(parts.loopVariable.declaredElement, assigned: true);
       }
       _checkExpressionNotNull(parts.iterable);
-      _flowAnalysis.forEach_bodyBegin(_assignedVariables.writtenInNode(node));
+      _flowAnalysis.forEach_bodyBegin(
+          _assignedVariables.writtenInNode(node), null);
     }
 
     // The condition may fail/iterable may be empty, so the body gets a new