Fix several issues with ES6 redeclaration checks

R=ulan@chromium.org
BUG=v8:3426
LOG=Y

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

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@22298 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/parser.cc

@@ -1709,15 +1709,14 @@ void Parser::Declare(Declaration* declaration, bool resolve, bool* ok) {
       // Declare the name.
       var = declaration_scope->DeclareLocal(
           name, mode, declaration->initialization(), proxy->interface());
-    } else if ((mode != VAR || var->mode() != VAR) &&
-               (!declaration_scope->is_global_scope() ||
-                IsLexicalVariableMode(mode) ||
-                IsLexicalVariableMode(var->mode()))) {
+    } else if (IsLexicalVariableMode(mode) || IsLexicalVariableMode(var->mode())
+               || ((mode == CONST_LEGACY || var->mode() == CONST_LEGACY) &&
+                   !declaration_scope->is_global_scope())) {
       // The name was declared in this scope before; check for conflicting
       // re-declarations. We have a conflict if either of the declarations is
       // not a var (in the global scope, we also have to ignore legacy const for
       // compatibility). There is similar code in runtime.cc in the Declare
-      // functions. The function CheckNonConflictingScope checks for conflicting
+      // functions. The function CheckConflictingVarDeclarations checks for
       // var and let bindings from different scopes whereas this is a check for
       // conflicting declarations within the same scope. This check also covers
       // the special case
@@ -1900,11 +1899,12 @@ Statement* Parser::ParseFunctionDeclaration(
   // Even if we're not at the top-level of the global or a function
   // scope, we treat it as such and introduce the function with its
   // initial value upon entering the corresponding scope.
-  // In extended mode, a function behaves as a lexical binding, except in the
-  // global scope.
+  // In ES6, a function behaves as a lexical binding, except in the
+  // global scope, or the initial scope of eval or another function.
   VariableMode mode =
-      allow_harmony_scoping() &&
-      strict_mode() == STRICT && !scope_->is_global_scope() ? LET : VAR;
+      allow_harmony_scoping() && strict_mode() == STRICT &&
+      !(scope_->is_global_scope() || scope_->is_eval_scope() ||
+          scope_->is_function_scope()) ? LET : VAR;
   VariableProxy* proxy = NewUnresolved(name, mode, Interface::NewValue());
   Declaration* declaration =
       factory()->NewFunctionDeclaration(proxy, mode, fun, scope_, pos);
@@ -2213,9 +2213,8 @@ Block* Parser::ParseVariableDeclarations(
     // executed.
     //
     // Executing the variable declaration statement will always
-    // guarantee to give the global object a "local" variable; a
-    // variable defined in the global object and not in any
-    // prototype. This way, global variable declarations can shadow
+    // guarantee to give the global object an own property.
+    // This way, global variable declarations can shadow
     // properties in the prototype chain, but only after the variable
     // declaration statement has been executed. This is important in
     // browsers where the global object (window) has lots of
@@ -3577,10 +3576,10 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     }
     ast_properties = *factory()->visitor()->ast_properties();
     dont_optimize_reason = factory()->visitor()->dont_optimize_reason();
-  }
 
-  if (allow_harmony_scoping() && strict_mode() == STRICT) {
-    CheckConflictingVarDeclarations(scope, CHECK_OK);
+    if (allow_harmony_scoping() && strict_mode() == STRICT) {
+      CheckConflictingVarDeclarations(scope, CHECK_OK);
+    }
   }
 
   FunctionLiteral::IsGeneratorFlag generator = is_generator

test/mjsunit/harmony/block-conflicts.js

@@ -39,9 +39,18 @@ function CheckException(e) {
 }
 
 
+function TestGlobal(s,e) {
+  try {
+    return eval(s + e);
+  } catch (x) {
+    return CheckException(x);
+  }
+}
+
+
 function TestFunction(s,e) {
   try {
-    return eval("(function(){" + s + ";return " + e + "})")();
+    return eval("(function(){" + s + " return " + e + "})")();
   } catch (x) {
     return CheckException(x);
   }
@@ -50,7 +59,7 @@ function TestFunction(s,e) {
 
 function TestBlock(s,e) {
   try {
-    return eval("(function(){ if (true) { " + s + "; }; return " + e + "})")();
+    return eval("(function(){ {" + s + "} return " + e + "})")();
   } catch (x) {
     return CheckException(x);
   }
@@ -59,76 +68,123 @@ function TestBlock(s,e) {
 function TestAll(expected,s,opt_e) {
   var e = "";
   var msg = s;
-  if (opt_e) { e = opt_e; msg += "; " + opt_e; }
-  assertEquals(expected, TestFunction(s,e), "function:'" + msg + "'");
-  assertEquals(expected, TestBlock(s,e), "block:'" + msg + "'");
+  if (opt_e) { e = opt_e; msg += opt_e; }
+  assertEquals(expected === 'LocalConflict' ? 'NoConflict' : expected,
+      TestGlobal(s,e), "global:'" + msg + "'");
+  assertEquals(expected === 'LocalConflict' ? 'NoConflict' : expected,
+      TestFunction(s,e), "function:'" + msg + "'");
+  assertEquals(expected === 'LocalConflict' ? 'Conflict' : expected,
+      TestBlock(s,e), "block:'" + msg + "'");
 }
 
 
 function TestConflict(s) {
   TestAll('Conflict', s);
-  TestAll('Conflict', 'eval("' + s + '")');
+  TestAll('Conflict', 'eval("' + s + '");');
 }
 
-
 function TestNoConflict(s) {
   TestAll('NoConflict', s, "'NoConflict'");
-  TestAll('NoConflict', 'eval("' + s + '")', "'NoConflict'");
+  TestAll('NoConflict', 'eval("' + s + '");', "'NoConflict'");
 }
 
-var letbinds = [ "let x",
-                 "let x = 0",
-                 "let x = undefined",
-                 "function x() { }",
-                 "let x = function() {}",
-                 "let x, y",
-                 "let y, x",
-                 "const x = 0",
-                 "const x = undefined",
-                 "const x = function() {}",
-                 "const x = 2, y = 3",
-                 "const y = 4, x = 5",
+function TestLocalConflict(s) {
+  TestAll('LocalConflict', s, "'NoConflict'");
+  TestAll('NoConflict', 'eval("' + s + '");', "'NoConflict'");
+}
+
+var letbinds = [ "let x;",
+                 "let x = 0;",
+                 "let x = undefined;",
+                 "let x = function() {};",
+                 "let x, y;",
+                 "let y, x;",
+                 "const x = 0;",
+                 "const x = undefined;",
+                 "const x = function() {};",
+                 "const x = 2, y = 3;",
+                 "const y = 4, x = 5;",
                  ];
-var varbinds = [ "var x",
-                 "var x = 0",
-                 "var x = undefined",
-                 "var x = function() {}",
-                 "var x, y",
-                 "var y, x",
+var varbinds = [ "var x;",
+                 "var x = 0;",
+                 "var x = undefined;",
+                 "var x = function() {};",
+                 "var x, y;",
+                 "var y, x;",
                  ];
-
+var funbind = "function x() {}";
 
 for (var l = 0; l < letbinds.length; ++l) {
   // Test conflicting let/var bindings.
   for (var v = 0; v < varbinds.length; ++v) {
     // Same level.
-    TestConflict(letbinds[l] +'; ' + varbinds[v]);
-    TestConflict(varbinds[v] +'; ' + letbinds[l]);
+    TestConflict(letbinds[l] + varbinds[v]);
+    TestConflict(varbinds[v] + letbinds[l]);
     // Different level.
-    TestConflict(letbinds[l] +'; {' + varbinds[v] + '; }');
-    TestConflict('{ ' + varbinds[v] +'; }' + letbinds[l]);
+    TestConflict(letbinds[l] + '{' + varbinds[v] + '}');
+    TestConflict('{' + varbinds[v] +'}' + letbinds[l]);
+    TestNoConflict(varbinds[v] + '{' + letbinds[l] + '}');
+    TestNoConflict('{' + letbinds[l] + '}' + varbinds[v]);
+    // For loop.
+    TestConflict('for (' + letbinds[l] + '0;) {' + varbinds[v] + '}');
+    TestNoConflict('for (' + varbinds[v] + '0;) {' + letbinds[l] + '}');
   }
 
   // Test conflicting let/let bindings.
   for (var k = 0; k < letbinds.length; ++k) {
     // Same level.
-    TestConflict(letbinds[l] +'; ' + letbinds[k]);
-    TestConflict(letbinds[k] +'; ' + letbinds[l]);
+    TestConflict(letbinds[l] + letbinds[k]);
+    TestConflict(letbinds[k] + letbinds[l]);
     // Different level.
-    TestNoConflict(letbinds[l] +'; { ' + letbinds[k] + '; }');
-    TestNoConflict('{ ' + letbinds[k] +'; } ' + letbinds[l]);
+    TestNoConflict(letbinds[l] + '{ ' + letbinds[k] + '}');
+    TestNoConflict('{' + letbinds[k] +'} ' + letbinds[l]);
+    // For loop.
+    TestNoConflict('for (' + letbinds[l] + '0;) {' + letbinds[k] + '}');
+    TestNoConflict('for (' + letbinds[k] + '0;) {' + letbinds[l] + '}');
   }
 
+  // Test conflicting function/let bindings.
+  // Same level.
+  TestConflict(letbinds[l] + funbind);
+  TestConflict(funbind + letbinds[l]);
+  // Different level.
+  TestNoConflict(letbinds[l] + '{' + funbind + '}');
+  TestNoConflict('{' + funbind + '}' + letbinds[l]);
+  TestNoConflict(funbind + '{' + letbinds[l] + '}');
+  TestNoConflict('{' + letbinds[l] + '}' + funbind);
+  // For loop.
+  TestNoConflict('for (' + letbinds[l] + '0;) {' + funbind + '}');
+
   // Test conflicting parameter/let bindings.
-  TestConflict('(function (x) { ' + letbinds[l] + '; })()');
+  TestConflict('(function(x) {' + letbinds[l] + '})();');
+}
+
+// Test conflicting function/var bindings.
+for (var v = 0; v < varbinds.length; ++v) {
+  // Same level.
+  TestLocalConflict(varbinds[v] + funbind);
+  TestLocalConflict(funbind + varbinds[v]);
+  // Different level.
+  TestLocalConflict(funbind + '{' + varbinds[v] + '}');
+  TestLocalConflict('{' + varbinds[v] +'}' + funbind);
+  TestNoConflict(varbinds[v] + '{' + funbind + '}');
+  TestNoConflict('{' + funbind + '}' + varbinds[v]);
+  // For loop.
+  TestNoConflict('for (' + varbinds[v] + '0;) {' + funbind + '}');
 }
 
 // Test conflicting catch/var bindings.
 for (var v = 0; v < varbinds.length; ++v) {
-  TestConflict('try {} catch (x) { ' + varbinds[v] + '; }');
+  TestConflict('try {} catch(x) {' + varbinds[v] + '}');
 }
 
 // Test conflicting parameter/var bindings.
 for (var v = 0; v < varbinds.length; ++v) {
-  TestNoConflict('(function (x) { ' + varbinds[v] + '; })()');
+  TestNoConflict('(function (x) {' + varbinds[v] + '})();');
 }
+
+// Test conflicting catch/function bindings.
+TestNoConflict('try {} catch(x) {' + funbind + '}');
+
+// Test conflicting parameter/function bindings.
+TestNoConflict('(function (x) {' + funbind + '})();');

test/mjsunit/harmony/block-let-declaration.js

@@ -56,11 +56,11 @@ if (true) {
 // an exception in eval code during parsing, before even compiling or executing
 // the code. Thus the generated function is not called here.
 function TestLocalThrows(str, expect) {
-  assertThrows("(function(){ 'use strict'; " + str + "})", expect);
+  assertThrows("(function(arg){ 'use strict'; " + str + "})", expect);
 }
 
 function TestLocalDoesNotThrow(str) {
-  assertDoesNotThrow("(function(){ 'use strict'; " + str + "})()");
+  assertDoesNotThrow("(function(arg){ 'use strict'; " + str + "})()");
 }
 
 // Test let declarations in statement positions.
@@ -108,6 +108,28 @@ TestLocalDoesNotThrow("for (;false;) var x;");
 TestLocalDoesNotThrow("switch (true) { case true: var x; }");
 TestLocalDoesNotThrow("switch (true) { default: var x; }");
 
+// Test that redeclarations of functions are only allowed in outermost scope.
+TestLocalThrows("{ let f; var f; }");
+TestLocalThrows("{ var f; let f; }");
+TestLocalThrows("{ function f() {} let f; }");
+TestLocalThrows("{ let f; function f() {} }");
+TestLocalThrows("{ function f() {} var f; }");
+TestLocalThrows("{ var f; function f() {} }");
+TestLocalThrows("{ function f() {} function f() {} }");
+TestLocalThrows("function f() {} let f;");
+TestLocalThrows("let f; function f() {}");
+TestLocalDoesNotThrow("function arg() {}");
+TestLocalDoesNotThrow("function f() {} var f;");
+TestLocalDoesNotThrow("var f; function f() {}");
+TestLocalDoesNotThrow("function f() {} function f() {}");
+
+function g(f) {
+  function f() { return 1 }
+  return f()
+}
+assertEquals(1, g(function() { return 2 }))
+
+
 // Test function declarations in source element and
 // sloppy statement positions.
 function f() {

test/mjsunit/harmony/regress/regress-3426.js

+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-scoping
+
+assertThrows("(function() { 'use strict'; { let f; var f; } })", SyntaxError);

tools/generate-runtime-tests.py

@@ -51,7 +51,7 @@ EXPECTED_FUNCTION_COUNT = 417
 EXPECTED_FUZZABLE_COUNT = 332
 EXPECTED_CCTEST_COUNT = 6
 EXPECTED_UNKNOWN_COUNT = 4
-EXPECTED_BUILTINS_COUNT = 810
+EXPECTED_BUILTINS_COUNT = 815
 
 
 # Don't call these at all.