From d434d3158ca44a1a4ba2e28a753bd6b14a8bea33 Mon Sep 17 00:00:00 2001 From: "keuchel@chromium.org" Date: Thu, 1 Sep 2011 12:31:18 +0000 Subject: [PATCH] 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 | 9 +- src/ast.h | 12 ++- src/ia32/full-codegen-ia32.cc | 9 +- src/mips/full-codegen-mips.cc | 9 +- src/parser.cc | 67 ++++++++++++-- src/parser.h | 11 +++ src/scopes.cc | 26 +++++- src/scopes.h | 6 +- src/x64/full-codegen-x64.cc | 9 +- test/mjsunit/harmony/block-conflicts.js | 126 ++++++++++++++++++++++++++ test/mjsunit/harmony/block-let-declaration.js | 2 - 11 files changed, 264 insertions(+), 22 deletions(-) create mode 100644 test/mjsunit/harmony/block-conflicts.js diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc index c18b9c3..1342188 100644 --- a/src/arm/full-codegen-arm.cc +++ b/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 diff --git a/src/ast.h b/src/ast.h index 67902cd..e36eb9b 100644 --- a/src/ast.h +++ b/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_; }; diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index 36baa1d..edeb028 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/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 diff --git a/src/mips/full-codegen-mips.cc b/src/mips/full-codegen-mips.cc index 2bfa6aa..48c176a 100644 --- a/src/mips/full-codegen-mips.cc +++ b/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 diff --git a/src/parser.cc b/src/parser.cc index cf907ce..d685b8a 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -647,6 +647,11 @@ FunctionLiteral* Parser::DoParseProgram(Handle 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 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 c_string = name->ToCString(DISALLOW_NULLS); + const char* elms[2] = { "Variable", *c_string }; + Vector 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 type_string = @@ -1379,8 +1402,10 @@ VariableProxy* Parser::Declare(Handle 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 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 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 name = decl->proxy()->name(); + SmartPointer c_string = name->ToCString(DISALLOW_NULLS); + const char* elms[2] = { "Variable", *c_string }; + Vector 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 Parser::ParseIdentifierNameOrGetOrSet(bool* is_get, diff --git a/src/parser.h b/src/parser.h index 686dac8..381ff27 100644 --- a/src/parser.h +++ b/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 name, Variable::Mode mode, FunctionLiteral* fun, diff --git a/src/scopes.cc b/src/scopes.cc index ddde48a..8d5a4a8 100644 --- a/src/scopes.cc +++ b/src/scopes.cc @@ -383,11 +383,11 @@ Variable* Scope::DeclareFunctionVar(Handle name) { } -void Scope::DeclareParameter(Handle name) { +void Scope::DeclareParameter(Handle 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 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 void Scope::CollectUsedVariables(List* locals) { // Collect variables in this scope. diff --git a/src/scopes.h b/src/scopes.h index c2c4179..4af19d3 100644 --- a/src/scopes.h +++ b/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 name); + void DeclareParameter(Handle 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. diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc index 058bd89..55a3905 100644 --- a/src/x64/full-codegen-x64.cc +++ b/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 diff --git a/test/mjsunit/harmony/block-conflicts.js b/test/mjsunit/harmony/block-conflicts.js new file mode 100644 index 0000000..8d3de6f --- /dev/null +++ b/test/mjsunit/harmony/block-conflicts.js @@ -0,0 +1,126 @@ +// 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] + '; })()'); +} diff --git a/test/mjsunit/harmony/block-let-declaration.js b/test/mjsunit/harmony/block-let-declaration.js index 19c943f..49b6348 100644 --- a/test/mjsunit/harmony/block-let-declaration.js +++ b/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;"); -- 2.7.4