[dartdevc] fix for-in loop variable shadowing a var used in initialzer

Dart for-in loops allow `var x = [1]; for (var x in x) {}`, which is not
allowed in JS. If this pattern is detected, a temporary variable is
introduced so the for-in initializer expression is evaluated outside of
the JS for-of loop.

(This issue seems to be unique to for-in loops. For loops and other
kinds of variable declarations of the form `var x = ...` are not
allowed to use `x` in the initializer, even if `x` is declared in an
outer scope.)

Also fixes an out-of-date comment in the DDC+Analyzer backend.

Change-Id: I35b272a5a311f7b6f104cc82a99cc83a6ed5c247
Reviewed-on: https://dart-review.googlesource.com/c/79142
Commit-Queue: Alan Knight <alanknight@google.com>
Auto-Submit: Jenny Messerly <jmesserly@google.com>
Reviewed-by: Alan Knight <alanknight@google.com>

pkg/dev_compiler/lib/src/analyzer/code_generator.dart

@@ -5498,13 +5498,19 @@ class CodeGenerator extends Object
 
     var init = _visitExpression(node.identifier);
     var iterable = _visitExpression(node.iterable);
+
     var body = _visitScope(node.body);
     if (init == null) {
-      var id = node.loopVariable.identifier;
-      init = js.call('let #', _emitVariableDef(id));
+      var id = _emitVariableDef(node.loopVariable.identifier);
+      init = js.call('let #', id);
       if (_annotatedNullCheck(node.loopVariable.declaredElement)) {
         body = JS.Block([_nullParameterCheck(JS.Identifier(id.name)), body]);
       }
+      if (variableIsReferenced(id.name, iterable)) {
+        var temp = JS.TemporaryId('iter');
+        return JS.Block(
+            [iterable.toVariableDeclaration(temp), JS.ForOf(init, temp, body)]);
+      }
     }
     return JS.ForOf(init, iterable, body);
   }
@@ -5512,10 +5518,10 @@ class CodeGenerator extends Object
   JS.Statement _emitAwaitFor(ForEachStatement node) {
     // Emits `await for (var value in stream) ...`, which desugars as:
     //
-    // var iter = new StreamIterator(stream);
+    // let iter = new StreamIterator(stream);
     // try {
     //   while (await iter.moveNext()) {
-    //     var value = iter.current;
+    //     let value = iter.current;
     //     ...
     //   }
     // } finally {

pkg/dev_compiler/lib/src/analyzer/module_compiler.dart

@@ -44,18 +44,23 @@ import 'extension_types.dart' show ExtensionTypeSet;
 
 /// Compiles a set of Dart files into a single JavaScript module.
 ///
-/// For a single [BuildUnit] definition, this will produce a [JSModuleFile].
-/// Those objects are record types that record the data consumed and produced
-/// for a single compile.
+/// For a single build unit, this will produce a [JSModuleFile].
+///
+/// A build unit is a collection of Dart sources that is sufficient to be
+/// compiled together. This can be as small as a single Dart library file, but
+/// if the library has parts, or if the library has cyclic dependencies on other
+/// libraries, those must be included as well. A common build unit is the lib
+/// directory of a Dart package.
 ///
 /// This class exists to cache global state associated with a single in-memory
-/// AnalysisContext, such as information about extension types in the Dart SDK.
-/// It can be used once to produce a single module, or reused to save warm-up
-/// time. (Currently there is no warm up, but there may be in the future.)
+/// [AnalysisContext], such as information about extension types in the Dart
+/// SDK. It can be used once to produce a single module, or reused to save
+/// warm-up time. (Currently there is no warm up, but there may be in the
+/// future.)
 ///
 /// The SDK source code is assumed to be immutable for the life of this class.
 ///
-/// For all other files, it is up to the [AnalysisContext] to decide whether or
+/// For all other files, it is up to the analysis context to decide whether or
 /// not any caching is performed. By default an analysis context will assume
 /// sources are immutable for the life of the context, and cache information
 /// about them.

pkg/dev_compiler/lib/src/compiler/shared_compiler.dart

@@ -157,3 +157,27 @@ abstract class SharedCompiler<Library> {
     return js.call('#.# = #', args);
   }
 }
+
+/// Whether a variable with [name] is referenced in the [node].
+bool variableIsReferenced(String name, JS.Node node) {
+  var finder = _IdentifierFinder.instance;
+  finder.nameToFind = name;
+  finder.found = false;
+  node.accept(finder);
+  return finder.found;
+}
+
+class _IdentifierFinder extends JS.BaseVisitor<void> {
+  String nameToFind;
+  bool found = false;
+
+  static final instance = _IdentifierFinder();
+
+  visitIdentifier(node) {
+    if (node.name == nameToFind) found = true;
+  }
+
+  visitNode(node) {
+    if (!found) super.visitNode(node);
+  }
+}

pkg/dev_compiler/lib/src/js_ast/nodes.dart

@@ -1192,16 +1192,18 @@ class This extends Expression {
   void visitChildren(NodeVisitor visitor) {}
 
   static bool foundIn(Node node) {
-    _thisFinder.found = false;
-    node.accept(_thisFinder);
-    return _thisFinder.found;
+    var finder = _ThisFinder._instance;
+    finder.found = false;
+    node.accept(finder);
+    return finder.found;
   }
 }
 
-final _thisFinder = _ThisFinder();
-
-class _ThisFinder extends BaseVisitor {
+class _ThisFinder extends BaseVisitor<void> {
   bool found = false;
+
+  static final _instance = _ThisFinder();
+
   visitThis(This node) {
     found = true;
   }

pkg/dev_compiler/lib/src/kernel/compiler.dart

@@ -3342,6 +3342,11 @@ class ProgramCompiler extends Object
             [_nullParameterCheck(_emitVariableRef(node.variable)), body]);
       }
 
+      if (variableIsReferenced(node.variable.name, iterable)) {
+        var temp = JS.TemporaryId('iter');
+        return JS.Block(
+            [iterable.toVariableDeclaration(temp), JS.ForOf(init, temp, body)]);
+      }
       return JS.ForOf(init, iterable, body);
     });
   }

pkg/dev_compiler/web/web_command.dart

@@ -29,7 +29,7 @@ import 'package:args/command_runner.dart';
 
 import 'package:dev_compiler/src/analyzer/context.dart' show AnalyzerOptions;
 import 'package:dev_compiler/src/analyzer/module_compiler.dart'
-    show BuildUnit, CompilerOptions, JSModuleFile, ModuleCompiler;
+    show CompilerOptions, JSModuleFile, ModuleCompiler;
 
 import 'package:dev_compiler/src/compiler/module_builder.dart';
 import 'package:js/js.dart';

tests/language_2/for_in_test.dart

@@ -6,47 +6,59 @@ import "package:expect/expect.dart";
 
 // Dart test for testing for in on a list literal.
 
-class ForInTest {
-  static testMain() {
-    testSimple();
-    testGenericSyntax1();
-    testGenericSyntax2();
-    testGenericSyntax3();
-    testGenericSyntax4();
-  }
-
-  static void testSimple() {
-    var list = [1, 3, 5];
-    var sum = 0;
-    for (var i in list) {
-      sum += i;
-    }
-    Expect.equals(9, sum);
-  }
-
-  static void testGenericSyntax1() {
-    List<List<String>> aCollection = [];
-    for (List<String> strArrArr in aCollection) {}
-  }
-
-  static void testGenericSyntax2() {
-    List<List<String>> aCollection = [];
-    List<String> strArrArr;
-    for (strArrArr in aCollection) {}
-  }
-
-  static void testGenericSyntax3() {
-    List<List<List<String>>> aCollection = [];
-    for (List<List<String>> strArrArr in aCollection) {}
-  }
-
-  static void testGenericSyntax4() {
-    List<List<List<String>>> aCollection = [];
-    List<List<String>> strArrArr;
-    for (strArrArr in aCollection) {}
-  }
-}
-
 main() {
-  ForInTest.testMain();
+  testSimple();
+  testGenericSyntax1();
+  testGenericSyntax2();
+  testGenericSyntax3();
+  testGenericSyntax4();
+  testShadowLocal1();
+  testShadowLocal2();
+}
+
+void testSimple() {
+  var list = [1, 3, 5];
+  var sum = 0;
+  for (var i in list) {
+    sum += i;
+  }
+  Expect.equals(9, sum);
+}
+
+void testGenericSyntax1() {
+  List<List<String>> aCollection = [];
+  for (List<String> strArrArr in aCollection) {}
+}
+
+void testGenericSyntax2() {
+  List<List<String>> aCollection = [];
+  List<String> strArrArr;
+  for (strArrArr in aCollection) {}
+}
+
+void testGenericSyntax3() {
+  List<List<List<String>>> aCollection = [];
+  for (List<List<String>> strArrArr in aCollection) {}
+}
+
+void testGenericSyntax4() {
+  List<List<List<String>>> aCollection = [];
+  List<List<String>> strArrArr;
+  for (strArrArr in aCollection) {}
+}
+
+void testShadowLocal1() {
+  List<int> x = [1, 2, 3];
+  var i = 0;
+  for (var x in x) {
+    Expect.equals(x, ++i);
+  }
+}
+
+void testShadowLocal2() {
+  Object x = [1, 2, 3];
+  var i = 0;
+  for (x in x) {
+    Expect.equals(x, ++i);
+  }
 }

tests/language_2/for_test.dart

@@ -81,4 +81,10 @@ class ForTest {
 
 main() {
   ForTest.testMain();
+  testShadowLocal();
+}
+
+void testShadowLocal() {
+  List<int> x = [1, 2, 3];
+  for (var x = x;;) break; //# 01: compile-time error
 }

tests/language_2/language_2_analyzer.status

@@ -29,6 +29,7 @@ enum_syntax_test/05: Fail # Issue 34341
 enum_syntax_test/06: Fail # Issue 34341
 f_bounded_quantification2_test: CompileTimeError # Issue 34583
 f_bounded_quantification4_test: CompileTimeError # Issue 34583
+for_test/01: MissingCompileTimeError
 forwarding_stub_tearoff_test: CompileTimeError # Issue 34329
 generic_local_functions_test: CompileTimeError # Issue 28515
 generic_methods_generic_function_parameter_test: CompileTimeError # Issue 28515

tests/language_2/language_2_dartdevc.status

@@ -53,6 +53,7 @@ f_bounded_quantification2_test: CompileTimeError # Issue 34583
 f_bounded_quantification3_test: RuntimeError # Issue 29920
 f_bounded_quantification4_test: CompileTimeError # Issue 34583
 field_wierd_name_test: Crash
+for_test/01: MissingCompileTimeError
 forwarding_stub_tearoff_generic_test: RuntimeError
 forwarding_stub_tearoff_test: CompileTimeError
 function_propagation_test: RuntimeError