From 971df386b34ea83d4010fa57864f6cf6b845fc7b Mon Sep 17 00:00:00 2001 From: "rossberg@chromium.org" Date: Fri, 23 Aug 2013 09:25:37 +0000 Subject: [PATCH] Fix scoping of function declarations in eval inside non-trivial local scope R=mstarzinger@chromium.org BUG=v8:2594 Review URL: https://codereview.chromium.org/22901010 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@16286 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/parser.cc | 37 ++++++++++++- src/parser.h | 1 + src/scopes.cc | 26 +++++---- test/mjsunit/regress/regress-2594.js | 104 +++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 13 deletions(-) create mode 100644 test/mjsunit/regress/regress-2594.js diff --git a/src/parser.cc b/src/parser.cc index 4947790..ccab7ec 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -542,6 +542,7 @@ Parser::Parser(CompilationInfo* info) scanner_(isolate_->unicode_cache()), reusable_preparser_(NULL), top_scope_(NULL), + original_scope_(NULL), current_function_state_(NULL), target_stack_(NULL), extension_(info->extension()), @@ -622,6 +623,7 @@ FunctionLiteral* Parser::DoParseProgram(CompilationInfo* info, if (!info->context().is_null()) { scope = Scope::DeserializeScopeChain(*info->context(), scope, zone()); } + original_scope_ = scope; if (info->is_eval()) { if (!scope->is_global_scope() || info->language_mode() != CLASSIC_MODE) { scope = NewScope(scope, EVAL_SCOPE); @@ -749,6 +751,7 @@ FunctionLiteral* Parser::ParseLazy(Utf16CharacterStream* source) { scope = Scope::DeserializeScopeChain(info()->closure()->context(), scope, zone()); } + original_scope_ = scope; FunctionState function_state(this, scope, isolate()); ASSERT(scope->language_mode() != STRICT_MODE || !info()->is_classic_mode()); ASSERT(scope->language_mode() != EXTENDED_MODE || @@ -4279,10 +4282,38 @@ FunctionLiteral* Parser::ParseFunctionLiteral( // Function declarations are function scoped in normal mode, so they are // hoisted. In harmony block scoping mode they are block scoped, so they // are not hoisted. + // + // One tricky case are function declarations in a local sloppy-mode eval: + // their declaration is hoisted, but they still see the local scope. E.g., + // + // function() { + // var x = 0 + // try { throw 1 } catch (x) { eval("function g() { return x }") } + // return g() + // } + // + // needs to return 1. To distinguish such cases, we need to detect + // (1) whether a function stems from a sloppy eval, and + // (2) whether it actually hoists across the eval. + // Unfortunately, we do not represent sloppy eval scopes, so we do not have + // either information available directly, especially not when lazily compiling + // a function like 'g'. We hence rely on the following invariants: + // - (1) is the case iff the innermost scope of the deserialized scope chain + // under which we compile is _not_ a declaration scope. This holds because + // in all normal cases, function declarations are fully hoisted to a + // declaration scope and compiled relative to that. + // - (2) is the case iff the current declaration scope is still the original + // one relative to the deserialized scope chain. Otherwise we must be + // compiling a function in an inner declaration scope in the eval, e.g. a + // nested function, and hoisting works normally relative to that. + Scope* declaration_scope = top_scope_->DeclarationScope(); + Scope* original_declaration_scope = original_scope_->DeclarationScope(); Scope* scope = - (function_type == FunctionLiteral::DECLARATION && !is_extended_mode()) - ? NewScope(top_scope_->DeclarationScope(), FUNCTION_SCOPE) - : NewScope(top_scope_, FUNCTION_SCOPE); + function_type == FunctionLiteral::DECLARATION && !is_extended_mode() && + (original_scope_ == original_declaration_scope || + declaration_scope != original_declaration_scope) + ? NewScope(declaration_scope, FUNCTION_SCOPE) + : NewScope(top_scope_, FUNCTION_SCOPE); ZoneList* body = NULL; int materialized_literal_count = -1; int expected_property_count = -1; diff --git a/src/parser.h b/src/parser.h index 68a74b7..2f26b84 100644 --- a/src/parser.h +++ b/src/parser.h @@ -855,6 +855,7 @@ class Parser BASE_EMBEDDED { Scanner scanner_; preparser::PreParser* reusable_preparser_; Scope* top_scope_; + Scope* original_scope_; // for ES5 function declarations in sloppy eval FunctionState* current_function_state_; Target* target_stack_; // for break, continue statements v8::Extension* extension_; diff --git a/src/scopes.cc b/src/scopes.cc index e631332..e38e903 100644 --- a/src/scopes.cc +++ b/src/scopes.cc @@ -907,26 +907,32 @@ void Scope::Print(int n) { PrintF("%d heap slots\n", num_heap_slots_); } // Print locals. - Indent(n1, "// function var\n"); if (function_ != NULL) { + Indent(n1, "// function var:\n"); PrintVar(n1, function_->proxy()->var()); } - Indent(n1, "// temporary vars\n"); - for (int i = 0; i < temps_.length(); i++) { - PrintVar(n1, temps_[i]); + if (temps_.length() > 0) { + Indent(n1, "// temporary vars:\n"); + for (int i = 0; i < temps_.length(); i++) { + PrintVar(n1, temps_[i]); + } } - Indent(n1, "// internal vars\n"); - for (int i = 0; i < internals_.length(); i++) { - PrintVar(n1, internals_[i]); + if (internals_.length() > 0) { + Indent(n1, "// internal vars:\n"); + for (int i = 0; i < internals_.length(); i++) { + PrintVar(n1, internals_[i]); + } } - Indent(n1, "// local vars\n"); - PrintMap(n1, &variables_); + if (variables_.Start() != NULL) { + Indent(n1, "// local vars:\n"); + PrintMap(n1, &variables_); + } - Indent(n1, "// dynamic vars\n"); if (dynamics_ != NULL) { + Indent(n1, "// dynamic vars:\n"); PrintMap(n1, dynamics_->GetMap(DYNAMIC)); PrintMap(n1, dynamics_->GetMap(DYNAMIC_LOCAL)); PrintMap(n1, dynamics_->GetMap(DYNAMIC_GLOBAL)); diff --git a/test/mjsunit/regress/regress-2594.js b/test/mjsunit/regress/regress-2594.js new file mode 100644 index 0000000..60720cb --- /dev/null +++ b/test/mjsunit/regress/regress-2594.js @@ -0,0 +1,104 @@ +// Copyright 2013 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// In the assertions but the first, the ES5 spec actually requires 0, but +// that is arguably a spec bug, and other browsers return 1 like us. +// In ES6, all of those will presumably result in a ReferenceError. +// Our main concern with this test is that we do not crash, though. + +function f1() { + var XXX = 0 + try { throw 1 } catch (XXX) { + eval("var h = function() { return XXX }") + } + return h() +} +assertEquals(1, f1()) + +function f2() { + var XXX = 0 + try { throw 1 } catch (XXX) { + eval("function h(){ return XXX }") + } + return h() +} +assertEquals(1, f2()) + +function f3() { + var XXX = 0 + try { throw 1 } catch (XXX) { + try { throw 2 } catch (y) { + eval("function h(){ return XXX }") + } + } + return h() +} +assertEquals(1, f3()) + +function f4() { + var XXX = 0 + try { throw 1 } catch (XXX) { + with ({}) { + eval("function h(){ return XXX }") + } + } + return h() +} +assertEquals(1, f4()) + +function f5() { + var XXX = 0 + try { throw 1 } catch (XXX) { + eval('eval("function h(){ return XXX }")') + } + return h() +} +assertEquals(1, f5()) + +function f6() { + var XXX = 0 + try { throw 1 } catch (XXX) { + eval("var h = (function() { function g(){ return XXX } return g })()") + } + return h() +} +assertEquals(1, f6()) + +function f7() { + var XXX = 0 + try { throw 1 } catch (XXX) { + eval("function h() { var XXX=2; function g(){ return XXX } return g }") + } + return h()() +} +assertEquals(2, f7()) // ! + +var XXX = 0 +try { throw 1 } catch (XXX) { + eval("function h(){ return XXX }") +} +assertEquals(1, h()) -- 2.7.4