From 62572e011e73da2669be6e724243405e42304d78 Mon Sep 17 00:00:00 2001 From: adamk Date: Tue, 30 Jun 2015 17:27:15 -0700 Subject: [PATCH] [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} --- src/parser.cc | 56 ++++++++++++++++++++++------ test/mjsunit/es6/debug-blockscopes.js | 10 ++++- test/mjsunit/es6/debug-stepnext-for.js | 2 +- test/mjsunit/es6/regress/regress-2506.js | 16 ++++---- test/mjsunit/harmony/destructuring.js | 8 ++++ test/mjsunit/strong/declaration-after-use.js | 4 -- test/test262-es6/test262-es6.status | 6 --- 7 files changed, 70 insertions(+), 32 deletions(-) diff --git a/src/parser.cc b/src/parser.cc index b9dcbfd..fea0d0a 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -3555,11 +3555,14 @@ Statement* Parser::ParseForStatement(ZoneList* labels, // // into // - // - // for (x' in/of e) { - // let/const/var x; - // x = x'; - // b; + // { + // + // 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* 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* 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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], -- 2.7.4