Detect conflicting variable bindings in harmony mode.

BUG=
TEST=mjsunit/harmony/block-conflicts.js

Review URL: http://codereview.chromium.org/7756014

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

src/arm/full-codegen-arm.cc

@@ -2167,8 +2167,13 @@ void FullCodeGenerator::EmitResolvePossiblyDirectEval(ResolveEvalFlag flag,
   int receiver_offset = 2 + info_->scope()->num_parameters();
   __ ldr(r1, MemOperand(fp, receiver_offset * kPointerSize));
   __ push(r1);
-  // Push the strict mode flag.
-  __ mov(r1, Operand(Smi::FromInt(strict_mode_flag())));
+  // Push the strict mode flag. In harmony mode every eval call
+  // is a strict mode eval call.
+  StrictModeFlag strict_mode = strict_mode_flag();
+  if (FLAG_harmony_block_scoping) {
+    strict_mode = kStrictMode;
+  }
+  __ mov(r1, Operand(Smi::FromInt(strict_mode)));
   __ push(r1);
 
   __ CallRuntime(flag == SKIP_CONTEXT_LOOKUP

src/ast.h

@@ -408,10 +408,14 @@ class Block: public BreakableStatement {
 
 class Declaration: public AstNode {
  public:
-  Declaration(VariableProxy* proxy, Variable::Mode mode, FunctionLiteral* fun)
+  Declaration(VariableProxy* proxy,
+              Variable::Mode mode,
+              FunctionLiteral* fun,
+              Scope* scope)
       : proxy_(proxy),
         mode_(mode),
-        fun_(fun) {
+        fun_(fun),
+        scope_(scope) {
     ASSERT(mode == Variable::VAR ||
            mode == Variable::CONST ||
            mode == Variable::LET);
@@ -425,11 +429,15 @@ class Declaration: public AstNode {
   Variable::Mode mode() const { return mode_; }
   FunctionLiteral* fun() const { return fun_; }  // may be NULL
   virtual bool IsInlineable() const;
+  Scope* scope() const { return scope_; }
 
  private:
   VariableProxy* proxy_;
   Variable::Mode mode_;
   FunctionLiteral* fun_;
+
+  // Nested scope from which the declaration originated.
+  Scope* scope_;
 };
 
 

src/ia32/full-codegen-ia32.cc

@@ -2157,8 +2157,13 @@ void FullCodeGenerator::EmitResolvePossiblyDirectEval(ResolveEvalFlag flag,
   // Push the receiver of the enclosing function.
   __ push(Operand(ebp, (2 + info_->scope()->num_parameters()) * kPointerSize));
 
-  // Push the strict mode flag.
-  __ push(Immediate(Smi::FromInt(strict_mode_flag())));
+  // Push the strict mode flag. In harmony mode every eval call
+  // is a strict mode eval call.
+  StrictModeFlag strict_mode = strict_mode_flag();
+  if (FLAG_harmony_block_scoping) {
+    strict_mode = kStrictMode;
+  }
+  __ push(Immediate(Smi::FromInt(strict_mode)));
 
   __ CallRuntime(flag == SKIP_CONTEXT_LOOKUP
                  ? Runtime::kResolvePossiblyDirectEvalNoLookup

src/mips/full-codegen-mips.cc

@@ -2176,8 +2176,13 @@ void FullCodeGenerator::EmitResolvePossiblyDirectEval(ResolveEvalFlag flag,
   int receiver_offset = 2 + info_->scope()->num_parameters();
   __ lw(a1, MemOperand(fp, receiver_offset * kPointerSize));
   __ push(a1);
-  // Push the strict mode flag.
-  __ li(a1, Operand(Smi::FromInt(strict_mode_flag())));
+  // Push the strict mode flag. In harmony mode every eval call
+  // is a strict mode eval call.
+  StrictModeFlag strict_mode = strict_mode_flag();
+  if (FLAG_harmony_block_scoping) {
+    strict_mode = kStrictMode;
+  }
+  __ li(a1, Operand(Smi::FromInt(strict_mode)));
   __ push(a1);
 
   __ CallRuntime(flag == SKIP_CONTEXT_LOOKUP

src/parser.cc

@@ -647,6 +647,11 @@ FunctionLiteral* Parser::DoParseProgram(Handle<String> source,
     if (ok && top_scope_->is_strict_mode()) {
       CheckOctalLiteral(beg_loc, scanner().location().end_pos, &ok);
     }
+
+    if (ok && harmony_block_scoping_) {
+      CheckConflictingVarDeclarations(scope, &ok);
+    }
+
     if (ok) {
       result = new(zone()) FunctionLiteral(
           isolate(),
@@ -1343,14 +1348,32 @@ VariableProxy* Parser::Declare(Handle<String> name,
       // Declare the name.
       var = declaration_scope->DeclareLocal(name, mode);
     } else {
-      // The name was declared before; check for conflicting re-declarations.
-      // We have a conflict if either of the declarations is not a var. There
-      // is similar code in runtime.cc in the Declare functions.
+      // 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. There is similar code in runtime.cc in the Declare
+      // functions. The function CheckNonConflictingScope checks for conflicting
+      // var and let bindings from different scopes whereas this is a check for
+      // conflicting declarations within the same scope. This check also covers
+      //
+      // function () { let x; { var x; } }
+      //
+      // because the var declaration is hoisted to the function scope where 'x'
+      // is already bound.
       if ((mode != Variable::VAR) || (var->mode() != Variable::VAR)) {
         // We only have vars, consts and lets in declarations.
         ASSERT(var->mode() == Variable::VAR ||
                var->mode() == Variable::CONST ||
                var->mode() == Variable::LET);
+        if (harmony_block_scoping_) {
+          // In harmony mode we treat re-declarations as early errors. See
+          // ES5 16 for a definition of early errors.
+          SmartPointer<char> c_string = name->ToCString(DISALLOW_NULLS);
+          const char* elms[2] = { "Variable", *c_string };
+          Vector<const char*> args(elms, 2);
+          ReportMessage("redeclaration", args);
+          *ok = false;
+          return NULL;
+        }
         const char* type = (var->mode() == Variable::VAR) ? "var" :
                            (var->mode() == Variable::CONST) ? "const" : "let";
         Handle<String> type_string =
@@ -1379,8 +1402,10 @@ VariableProxy* Parser::Declare(Handle<String> name,
   // semantic issue as long as we keep the source order, but it may be
   // a performance issue since it may lead to repeated
   // Runtime::DeclareContextSlot() calls.
-  VariableProxy* proxy = declaration_scope->NewUnresolved(name, false);
-  declaration_scope->AddDeclaration(new(zone()) Declaration(proxy, mode, fun));
+  VariableProxy* proxy = declaration_scope->NewUnresolved(
+      name, false, scanner().location().beg_pos);
+  declaration_scope->AddDeclaration(
+      new(zone()) Declaration(proxy, mode, fun, top_scope_));
 
   // For global const variables we bind the proxy to a variable.
   if (mode == Variable::CONST && declaration_scope->is_global_scope()) {
@@ -2207,7 +2232,9 @@ TryStatement* Parser::ParseTryStatement(bool* ok) {
       if (top_scope_->is_strict_mode()) {
         catch_scope->EnableStrictMode();
       }
-      catch_variable = catch_scope->DeclareLocal(name, Variable::VAR);
+      Variable::Mode mode = harmony_block_scoping_
+          ? Variable::LET : Variable::VAR;
+      catch_variable = catch_scope->DeclareLocal(name, mode);
       catch_block = new(zone()) Block(isolate(), NULL, 2, false);
 
       Scope* saved_scope = top_scope_;
@@ -3736,7 +3763,10 @@ FunctionLiteral* Parser::ParseFunctionLiteral(Handle<String> function_name,
         reserved_loc = scanner().location();
       }
 
-      top_scope_->DeclareParameter(param_name);
+      top_scope_->DeclareParameter(param_name,
+                                   harmony_block_scoping_
+                                   ? Variable::LET
+                                   : Variable::VAR);
       num_parameters++;
       if (num_parameters > kMaxNumFunctionParameters) {
         ReportMessageAt(scanner().location(), "too_many_parameters",
@@ -3863,6 +3893,10 @@ FunctionLiteral* Parser::ParseFunctionLiteral(Handle<String> function_name,
     }
   }
 
+  if (harmony_block_scoping_) {
+    CheckConflictingVarDeclarations(scope, CHECK_OK);
+  }
+
   FunctionLiteral* function_literal =
       new(zone()) FunctionLiteral(isolate(),
                                   function_name,
@@ -4069,6 +4103,25 @@ void Parser::CheckOctalLiteral(int beg_pos, int end_pos, bool* ok) {
 }
 
 
+void Parser::CheckConflictingVarDeclarations(Scope* scope, bool* ok) {
+  Declaration* decl = scope->CheckConflictingVarDeclarations();
+  if (decl != NULL) {
+    // In harmony mode we treat conflicting variable bindinds as early
+    // errors. See ES5 16 for a definition of early errors.
+    Handle<String> name = decl->proxy()->name();
+    SmartPointer<char> c_string = name->ToCString(DISALLOW_NULLS);
+    const char* elms[2] = { "Variable", *c_string };
+    Vector<const char*> args(elms, 2);
+    int position = decl->proxy()->position();
+    Scanner::Location location = position == RelocInfo::kNoPosition
+        ? Scanner::Location::invalid()
+        : Scanner::Location(position, position + 1);
+    ReportMessageAt(location, "redeclaration", args);
+    *ok = false;
+  }
+}
+
+
 // This function reads an identifier name and determines whether or not it
 // is 'get' or 'set'.
 Handle<String> Parser::ParseIdentifierNameOrGetOrSet(bool* is_get,

src/parser.h

@@ -645,6 +645,17 @@ class Parser {
   // Strict mode octal literal validation.
   void CheckOctalLiteral(int beg_pos, int end_pos, bool* ok);
 
+  // For harmony block scoping mode: Check if the scope has conflicting var/let
+  // declarations from different scopes. It covers for example
+  //
+  // function f() { { { var x; } let x; } }
+  // function g() { { var x; let x; } }
+  //
+  // The var declarations are hoisted to the function scope, but originate from
+  // a scope where the name has also been let bound or the var declaration is
+  // hoisted over such a scope.
+  void CheckConflictingVarDeclarations(Scope* scope, bool* ok);
+
   // Parser support
   VariableProxy* Declare(Handle<String> name, Variable::Mode mode,
                          FunctionLiteral* fun,

src/scopes.cc

@@ -383,11 +383,11 @@ Variable* Scope::DeclareFunctionVar(Handle<String> name) {
 }
 
 
-void Scope::DeclareParameter(Handle<String> name) {
+void Scope::DeclareParameter(Handle<String> name, Variable::Mode mode) {
   ASSERT(!already_resolved());
   ASSERT(is_function_scope());
   Variable* var =
-      variables_.Declare(this, name, Variable::VAR, true, Variable::NORMAL);
+      variables_.Declare(this, name, mode, true, Variable::NORMAL);
   params_.Add(var);
 }
 
@@ -467,6 +467,28 @@ void Scope::VisitIllegalRedeclaration(AstVisitor* visitor) {
 }
 
 
+Declaration* Scope::CheckConflictingVarDeclarations() {
+  int length = decls_.length();
+  for (int i = 0; i < length; i++) {
+    Declaration* decl = decls_[i];
+    if (decl->mode() != Variable::VAR) continue;
+    Handle<String> name = decl->proxy()->name();
+    bool cond = true;
+    for (Scope* scope = decl->scope(); cond ; scope = scope->outer_scope_) {
+      // There is a conflict if there exists a non-VAR binding.
+      Variable* other_var = scope->variables_.Lookup(name);
+      if (other_var != NULL && other_var->mode() != Variable::VAR) {
+        return decl;
+      }
+
+      // Include declaration scope in the iteration but stop after.
+      if (!scope->is_block_scope() && !scope->is_catch_scope()) cond = false;
+    }
+  }
+  return NULL;
+}
+
+
 template<class Allocator>
 void Scope::CollectUsedVariables(List<Variable*, Allocator>* locals) {
   // Collect variables in this scope.

src/scopes.h

@@ -130,7 +130,7 @@ class Scope: public ZoneObject {
   // Declare a parameter in this scope.  When there are duplicated
   // parameters the rightmost one 'wins'.  However, the implementation
   // expects all parameters to be declared and from left to right.
-  void DeclareParameter(Handle<String> name);
+  void DeclareParameter(Handle<String> name, Variable::Mode mode);
 
   // Declare a local variable in this scope. If the variable has been
   // declared before, the previously declared variable is returned.
@@ -182,6 +182,10 @@ class Scope: public ZoneObject {
   // Check if the scope has (at least) one illegal redeclaration.
   bool HasIllegalRedeclaration() const { return illegal_redecl_ != NULL; }
 
+  // For harmony block scoping mode: Check if the scope has conflicting var
+  // declarations, i.e. a var declaration that has been hoisted from a nested
+  // scope over a let binding of the same name.
+  Declaration* CheckConflictingVarDeclarations();
 
   // ---------------------------------------------------------------------------
   // Scope-specific info.

src/x64/full-codegen-x64.cc

@@ -2069,8 +2069,13 @@ void FullCodeGenerator::EmitResolvePossiblyDirectEval(ResolveEvalFlag flag,
   // Push the receiver of the enclosing function and do runtime call.
   __ push(Operand(rbp, (2 + info_->scope()->num_parameters()) * kPointerSize));
 
-  // Push the strict mode flag.
-  __ Push(Smi::FromInt(strict_mode_flag()));
+  // Push the strict mode flag. In harmony mode every eval call
+  // is a strict mode eval call.
+  StrictModeFlag strict_mode = strict_mode_flag();
+  if (FLAG_harmony_block_scoping) {
+    strict_mode = kStrictMode;
+  }
+  __ Push(Smi::FromInt(strict_mode));
 
   __ CallRuntime(flag == SKIP_CONTEXT_LOOKUP
                  ? Runtime::kResolvePossiblyDirectEvalNoLookup

test/mjsunit/harmony/block-conflicts.js

+// Copyright 2011 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.
+
+// Flags: --harmony-block-scoping
+
+// Test for conflicting variable bindings.
+
+function CheckException(e) {
+  var string = e.toString();
+  assertTrue(string.indexOf("has already been declared") >= 0 ||
+             string.indexOf("redeclaration") >= 0);  return 'Conflict';
+}
+
+
+function TestFunction(s,e) {
+  try {
+    return eval("(function(){" + s + ";return " + e + "})")();
+  } catch (x) {
+    return CheckException(x);
+  }
+}
+
+
+function TestBlock(s,e) {
+  try {
+    return eval("(function(){ if (true) { " + s + "; }; return " + e + "})")();
+  } catch (x) {
+    return CheckException(x);
+  }
+}
+
+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 + "'");
+}
+
+
+function TestConflict(s) {
+  TestAll('Conflict', s);
+  TestAll('Conflict', 'eval("' + s + '")');
+}
+
+
+function TestNoConflict(s) {
+  TestAll('NoConflict', 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",
+                 ];
+var varbinds = [ "var x",
+                 "var x = 0",
+                 "var x = undefined",
+                 "var x = function() {}",
+                 "var x, y",
+                 "var y, 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]);
+    // Different level.
+    TestConflict(letbinds[l] +'; {' + varbinds[v] + '; }');
+    TestConflict('{ ' + varbinds[v] +'; }' + 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]);
+    // Different level.
+    TestNoConflict(letbinds[l] +'; { ' + letbinds[k] + '; }');
+    TestNoConflict('{ ' + letbinds[k] +'; } ' + letbinds[l]);
+  }
+
+  // Test conflicting parameter/let bindings.
+  TestConflict('(function (x) { ' + letbinds[l] + '; })()');
+}
+
+// Test conflicting catch/var bindings.
+for (var v = 0; v < varbinds.length; ++v) {
+  TestConflict('try {} catch (x) { ' + varbinds[v] + '; }');
+}
+
+// Test conflicting parameter/var bindings.
+for (var v = 0; v < varbinds.length; ++v) {
+  TestConflict('(function (x) { ' + varbinds[v] + '; })()');
+}

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

@@ -57,11 +57,9 @@ function TestLocalDoesNotThrow(str) {
 
 // Unprotected statement
 TestLocalThrows("if (true) let x;", SyntaxError);
-TestLocalThrows("with ({}) let x;", SyntaxError);
 TestLocalThrows("do let x; while (false)", SyntaxError);
 TestLocalThrows("while (false) let x;", SyntaxError);
 
 TestLocalDoesNotThrow("if (true) var x;");
-TestLocalDoesNotThrow("with ({}) var x;");
 TestLocalDoesNotThrow("do var x; while (false)");
 TestLocalDoesNotThrow("while (false) var x;");