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

@@ -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<Statement*>* body = NULL;
   int materialized_literal_count = -1;
   int expected_property_count = -1;

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_;

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));

test/mjsunit/regress/regress-2594.js

+// 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())