[es6] Ensure that for-in/of loops have a proper TDZ for their lexically-bound variables

The enumerable expression in a for-in/of loop is supposed to have a TDZ for any
lexically bound names in that loop (there can be more than one with destructuring).

This patch accomplishes this with an almost-correct desugaring. The only thing missing
is proper debugger support (the let declarations added by the desugaring, while invisible
to code due to shadowing, are visible to the debugger).

BUG=v8:4210
LOG=n

Review URL: https://codereview.chromium.org/1218543003

Cr-Commit-Position: refs/heads/master@{#29396}

diff --git a/src/parser.cc b/src/parser.cc
index b9dcbfd..fea0d0a 100644 (file)
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -3555,11 +3555,14 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
         //
         // into
         //
-        //   <let x' be a temporary variable>
-        //   for (x' in/of e) {
-        //     let/const/var x;
-        //     x = x';
-        //     b;
+        //   {
+        //     <let x' be a temporary variable>
+        //     for (x' in/of e) {
+        //       let/const/var x;
+        //       x = x';
+        //       b;
+        //     }
+        //     let x;  // for TDZ
         //   }
 
         Variable* temp = scope_->DeclarationScope()->NewTemporary(
@@ -3568,12 +3571,14 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
             factory()->NewForEachStatement(mode, labels, stmt_pos);
         Target target(&this->target_stack_, loop);
 
-        // The expression does not see the lexical loop variables.
-        scope_ = saved_scope;
         Expression* enumerable = ParseExpression(true, CHECK_OK);
-        scope_ = for_scope;
+
         Expect(Token::RPAREN, CHECK_OK);
 
+        Scope* body_scope = NewScope(scope_, BLOCK_SCOPE);
+        body_scope->set_start_position(scanner()->location().beg_pos);
+        scope_ = body_scope;
+
         Statement* body = ParseSubStatement(NULL, CHECK_OK);
 
         Block* body_block =
@@ -3601,17 +3606,46 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
         VariableProxy* temp_proxy =
             factory()->NewVariableProxy(temp, each_beg_pos, each_end_pos);
         InitializeForEachStatement(loop, temp_proxy, enumerable, body_block);
+        scope_ = for_scope;
+        body_scope->set_end_position(scanner()->location().end_pos);
+        body_scope = body_scope->FinalizeBlockScope();
+        if (body_scope != nullptr) {
+          body_block->set_scope(body_scope);
+        }
+
+        // Create a TDZ for any lexically-bound names.
+        if (is_strict(language_mode()) &&
+            IsLexicalVariableMode(parsing_result.descriptor.mode)) {
+          DCHECK_NULL(init_block);
+
+          init_block =
+              factory()->NewBlock(nullptr, 1, false, RelocInfo::kNoPosition);
+
+          for (int i = 0; i < lexical_bindings.length(); ++i) {
+            // TODO(adamk): This needs to be some sort of special
+            // INTERNAL variable that's invisible to the debugger
+            // but visible to everything else.
+            VariableProxy* tdz_proxy = NewUnresolved(lexical_bindings[i], LET);
+            Declaration* tdz_decl = factory()->NewVariableDeclaration(
+                tdz_proxy, LET, scope_, RelocInfo::kNoPosition);
+            Variable* tdz_var = Declare(tdz_decl, DeclarationDescriptor::NORMAL,
+                                        true, CHECK_OK);
+            tdz_var->set_initializer_position(position());
+          }
+        }
+
         scope_ = saved_scope;
         for_scope->set_end_position(scanner()->location().end_pos);
         for_scope = for_scope->FinalizeBlockScope();
-        if (for_scope != nullptr) {
-          body_block->set_scope(for_scope);
-        }
         // Parsed for-in loop w/ variable declarations.
         if (init_block != nullptr) {
           init_block->AddStatement(loop, zone());
+          if (for_scope != nullptr) {
+            init_block->set_scope(for_scope);
+          }
           return init_block;
         } else {
+          DCHECK_NULL(for_scope);
           return loop;
         }
       } else {

diff --git a/test/mjsunit/es6/debug-blockscopes.js b/test/mjsunit/es6/debug-blockscopes.js
index 39849da..31208d4 100644 (file)
--- a/test/mjsunit/es6/debug-blockscopes.js
+++ b/test/mjsunit/es6/debug-blockscopes.js
@@ -372,13 +372,16 @@ function for_loop_1() {
 
 listener_delegate = function(exec_state) {
   CheckScopeChain([debug.ScopeType.Block,
+                   debug.ScopeType.Block,
                    debug.ScopeType.Local,
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({x:'y'}, 0, exec_state);
   // The function scope contains a temporary iteration variable, but it is
   // hidden to the debugger.
-  CheckScopeContent({}, 1, exec_state);
+  // TODO(adamk): This variable is only used to provide a TDZ for the enumerable
+  // expression and should not be visible to the debugger.
+  CheckScopeContent({x:undefined}, 1, exec_state);
 };
 for_loop_1();
 EndTest();
@@ -398,6 +401,7 @@ function for_loop_2() {
 listener_delegate = function(exec_state) {
   CheckScopeChain([debug.ScopeType.Block,
                    debug.ScopeType.Block,
+                   debug.ScopeType.Block,
                    debug.ScopeType.Local,
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
@@ -405,7 +409,9 @@ listener_delegate = function(exec_state) {
   CheckScopeContent({x:'y'}, 1, exec_state);
   // The function scope contains a temporary iteration variable, hidden to the
   // debugger.
-  CheckScopeContent({}, 2, exec_state);
+  // TODO(adamk): This variable is only used to provide a TDZ for the enumerable
+  // expression and should not be visible to the debugger.
+  CheckScopeContent({x:undefined}, 2, exec_state);
 };
 for_loop_2();
 EndTest();

diff --git a/test/mjsunit/es6/debug-stepnext-for.js b/test/mjsunit/es6/debug-stepnext-for.js
index 79cb378..83a9fc3 100644 (file)
--- a/test/mjsunit/es6/debug-stepnext-for.js
+++ b/test/mjsunit/es6/debug-stepnext-for.js
@@ -110,7 +110,7 @@ var expected = [
   // Exit.
   "y0","z0",
 ]
-print("expected:\n"+ JSON.stringify(log));
+print("expected:\n"+ JSON.stringify(expected));
 
 assertArrayEquals(expected, log);
 assertEquals(48, s);

diff --git a/test/mjsunit/es6/regress/regress-2506.js b/test/mjsunit/es6/regress/regress-2506.js
index b5cc91d..5f88fcd 100644 (file)
--- a/test/mjsunit/es6/regress/regress-2506.js
+++ b/test/mjsunit/es6/regress/regress-2506.js
@@ -18,15 +18,15 @@ assertEquals(3, f[2]());
 
 let x = 1;
 s = 0;
-for (const x of [x, x+1, x+2]) {
-  s += x;
+for (const z of [x, x+1, x+2]) {
+  s += z;
 }
 assertEquals(6, s);
 
 s = 0;
 var q = 1;
-for (const q of [q, q+1, q+2]) {
-  s += q;
+for (const x of [q, q+1, q+2]) {
+  s += x;
 }
 assertEquals(6, s);
 
@@ -56,15 +56,15 @@ assertThrows("'use strict'; for (const x in [1,2,3]) { x++ }", TypeError);
 
   let x = 1;
   s = 0;
-  for (const x of [x, x+1, x+2]) {
-    s += x;
+  for (const q of [x, x+1, x+2]) {
+    s += q;
   }
   assertEquals(6, s);
 
   s = 0;
   var q = 1;
-  for (const q of [q, q+1, q+2]) {
-    s += q;
+  for (const x of [q, q+1, q+2]) {
+    s += x;
   }
   assertEquals(6, s);
 

diff --git a/test/mjsunit/harmony/destructuring.js b/test/mjsunit/harmony/destructuring.js
index c57dbaf..198d4c0 100644 (file)
--- a/test/mjsunit/harmony/destructuring.js
+++ b/test/mjsunit/harmony/destructuring.js
@@ -728,3 +728,11 @@
   assertThrows("function f({x}) { var x; }; f({});", SyntaxError);
   assertThrows("'use strict'; function f({x}) { let x = 0; }; f({});", SyntaxError);
 }());
+
+
+(function TestForInOfTDZ() {
+  assertThrows("'use strict'; let x = {}; for (let [x, y] of {x});", ReferenceError);
+  assertThrows("'use strict'; let x = {}; for (let [y, x] of {x});", ReferenceError);
+  assertThrows("'use strict'; let x = {}; for (let [x, y] in {x});", ReferenceError);
+  assertThrows("'use strict'; let x = {}; for (let [y, x] in {x});", ReferenceError);
+}());

diff --git a/test/mjsunit/strong/declaration-after-use.js b/test/mjsunit/strong/declaration-after-use.js
index 5d99060..5f3ef2a 100644 (file)
--- a/test/mjsunit/strong/declaration-after-use.js
+++ b/test/mjsunit/strong/declaration-after-use.js
@@ -193,10 +193,6 @@ function assertThrowsHelper(code) {
     i;
   }
 
-  let var6 = [1, 2];
-  // The second var6 resolves to outside (not to the first var6).
-  for (let var6 of var6) { var6; }
-
   try {
     throw "error";
   } catch (e) {

diff --git a/test/test262-es6/test262-es6.status b/test/test262-es6/test262-es6.status
index 8996c22..7257956 100644 (file)
--- a/test/test262-es6/test262-es6.status
+++ b/test/test262-es6/test262-es6.status
@@ -464,12 +464,6 @@
   # https://code.google.com/p/v8/issues/detail?id=3673
   'language/statements/class/definition/basics': [FAIL],
 
-  # https://code.google.com/p/v8/issues/detail?id=4210
-  'language/statements/for-in/const-bound-names-fordecl-tdz-for-in': [PASS, FAIL],
-  'language/statements/for-in/let-bound-names-fordecl-tdz-for-in': [PASS, FAIL],
-  'language/statements/for-of/const-bound-names-fordecl-tdz-for-of': [PASS, FAIL],
-  'language/statements/for-of/let-bound-names-fordecl-tdz-for-of': [PASS, FAIL],
-
   # Destructuring
   # https://code.google.com/p/v8/issues/detail?id=811
   'language/statements/for-of/body-dstr-assign': [FAIL],