From 2fda95eb8032783a1a7264220df031dd60760d0d Mon Sep 17 00:00:00 2001 From: "rossberg@chromium.org" Date: Wed, 2 Apr 2014 12:38:01 +0000 Subject: [PATCH] Make stray 'return' an early error As required by the spec, and implemented by other browsers. (Plus minor clean-up for redeclaration TypeErrors.) R=marja@chromium.org BUG= LOG=Y Review URL: https://codereview.chromium.org/220473014 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@20434 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/messages.js | 3 +- src/parser.cc | 61 +++++++++------------- src/parser.h | 3 +- src/preparser.cc | 2 +- src/preparser.h | 2 +- src/runtime.cc | 19 +++---- test/cctest/test-parsing.cc | 7 +-- test/mjsunit/debug-stepout-scope-part2.js | 2 +- test/mjsunit/debug-stepout-scope-part3.js | 2 +- test/mjsunit/debug-stepout-scope-part4.js | 2 +- test/mjsunit/debug-stepout-scope-part5.js | 2 +- test/mjsunit/debug-stepout-scope-part6.js | 2 +- test/mjsunit/delay-syntax-error.js | 11 +--- test/webkit/fast/js/arguments-expected.txt | 2 +- 14 files changed, 48 insertions(+), 72 deletions(-) diff --git a/src/messages.js b/src/messages.js index fc043f20df..ff108d6c87 100644 --- a/src/messages.js +++ b/src/messages.js @@ -47,7 +47,8 @@ var kMessages = { incompatible_method_receiver: ["Method ", "%0", " called on incompatible receiver ", "%1"], multiple_defaults_in_switch: ["More than one default clause in switch statement"], newline_after_throw: ["Illegal newline after throw"], - redeclaration: ["%0", " '", "%1", "' has already been declared"], + label_redeclaration: ["Label '", "%0", "' has already been declared"], + var_redeclaration: ["Identifier '", "%0", "' has already been declared"], no_catch_or_finally: ["Missing catch or finally after try"], unknown_label: ["Undefined label '", "%0", "'"], uncaught_exception: ["Uncaught ", "%0"], diff --git a/src/parser.cc b/src/parser.cc index 38a4fe56ba..5fce1dd94a 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -590,8 +590,7 @@ Expression* ParserTraits::BuildUnaryExpression( } -Expression* ParserTraits::NewThrowReferenceError( - const char* message, int pos) { +Expression* ParserTraits::NewThrowReferenceError(const char* message, int pos) { return NewThrowError( parser_->isolate()->factory()->MakeReferenceError_string(), message, HandleVector(NULL, 0), pos); @@ -609,11 +608,9 @@ Expression* ParserTraits::NewThrowSyntaxError( Expression* ParserTraits::NewThrowTypeError( - const char* message, Handle arg1, Handle arg2, int pos) { - ASSERT(!arg1.is_null() && !arg2.is_null()); - Handle elements[] = { arg1, arg2 }; - Vector< Handle > arguments = - HandleVector(elements, ARRAY_SIZE(elements)); + const char* message, Handle arg, int pos) { + int argc = arg.is_null() ? 0 : 1; + Vector< Handle > arguments = HandleVector(&arg, argc); return NewThrowError( parser_->isolate()->factory()->MakeTypeError_string(), message, arguments, pos); @@ -1754,18 +1751,14 @@ void Parser::Declare(Declaration* declaration, bool resolve, bool* ok) { // In harmony we treat re-declarations as early errors. See // ES5 16 for a definition of early errors. SmartArrayPointer c_string = name->ToCString(DISALLOW_NULLS); - const char* elms[2] = { "Variable", c_string.get() }; - Vector args(elms, 2); - ReportMessage("redeclaration", args); + const char* elms[1] = { c_string.get() }; + Vector args(elms, 1); + ReportMessage("var_redeclaration", args); *ok = false; return; } - Handle message_string = - isolate()->factory()->InternalizeOneByteString( - STATIC_ASCII_VECTOR("Variable")); - Expression* expression = - NewThrowTypeError("redeclaration", - message_string, name, declaration->position()); + Expression* expression = NewThrowTypeError( + "var_redeclaration", name, declaration->position()); declaration_scope->SetIllegalRedeclaration(expression); } } @@ -2374,9 +2367,9 @@ Statement* Parser::ParseExpressionOrLabelledStatement(ZoneStringList* labels, // make later anyway so we should go back and fix this then. if (ContainsLabel(labels, label) || TargetStackContainsLabel(label)) { SmartArrayPointer c_string = label->ToCString(DISALLOW_NULLS); - const char* elms[2] = { "Label", c_string.get() }; - Vector args(elms, 2); - ReportMessage("redeclaration", args); + const char* elms[1] = { c_string.get() }; + Vector args(elms, 1); + ReportMessage("label_redeclaration", args); *ok = false; return NULL; } @@ -2521,7 +2514,7 @@ Statement* Parser::ParseReturnStatement(bool* ok) { // reporting any errors on it, because of the way errors are // reported (underlining). Expect(Token::RETURN, CHECK_OK); - int pos = position(); + Scanner::Location loc = scanner()->location(); Token::Value tok = peek(); Statement* result; @@ -2539,23 +2532,17 @@ Statement* Parser::ParseReturnStatement(bool* ok) { Expression* generator = factory()->NewVariableProxy( function_state_->generator_object_variable()); Expression* yield = factory()->NewYield( - generator, return_value, Yield::FINAL, pos); - result = factory()->NewExpressionStatement(yield, pos); + generator, return_value, Yield::FINAL, loc.beg_pos); + result = factory()->NewExpressionStatement(yield, loc.beg_pos); } else { - result = factory()->NewReturnStatement(return_value, pos); + result = factory()->NewReturnStatement(return_value, loc.beg_pos); } - // An ECMAScript program is considered syntactically incorrect if it - // contains a return statement that is not within the body of a - // function. See ECMA-262, section 12.9, page 67. - // - // To be consistent with KJS we report the syntax error at runtime. - Scope* declaration_scope = scope_->DeclarationScope(); - if (declaration_scope->is_global_scope() || - declaration_scope->is_eval_scope()) { - Expression* throw_error = - NewThrowSyntaxError("illegal_return", Handle::null(), pos); - return factory()->NewExpressionStatement(throw_error, pos); + Scope* decl_scope = scope_->DeclarationScope(); + if (decl_scope->is_global_scope() || decl_scope->is_eval_scope()) { + ReportMessageAt(loc, "illegal_return"); + *ok = false; + return NULL; } return result; } @@ -3669,13 +3656,13 @@ void Parser::CheckConflictingVarDeclarations(Scope* scope, bool* ok) { // errors. See ES5 16 for a definition of early errors. Handle name = decl->proxy()->name(); SmartArrayPointer c_string = name->ToCString(DISALLOW_NULLS); - const char* elms[2] = { "Variable", c_string.get() }; - Vector args(elms, 2); + const char* elms[1] = { c_string.get() }; + Vector args(elms, 1); int position = decl->proxy()->position(); Scanner::Location location = position == RelocInfo::kNoPosition ? Scanner::Location::invalid() : Scanner::Location(position, position + 1); - ParserTraits::ReportMessageAt(location, "redeclaration", args); + ParserTraits::ReportMessageAt(location, "var_redeclaration", args); *ok = false; } } diff --git a/src/parser.h b/src/parser.h index 6a164c7f6f..cf7d2a5905 100644 --- a/src/parser.h +++ b/src/parser.h @@ -538,8 +538,7 @@ class ParserTraits { // Generate AST node that throws a TypeError with the given // type. Both arguments must be non-null (in the handle sense). - Expression* NewThrowTypeError( - const char* type, Handle arg1, Handle arg2, int pos); + Expression* NewThrowTypeError(const char* type, Handle arg, int pos); // Generic AST generator for throwing errors from compiled code. Expression* NewThrowError( diff --git a/src/preparser.cc b/src/preparser.cc index a6811e9b97..21e2b7786f 100644 --- a/src/preparser.cc +++ b/src/preparser.cc @@ -582,7 +582,7 @@ PreParser::Statement PreParser::ParseReturnStatement(bool* ok) { // ReturnStatement :: // 'return' [no line terminator] Expression? ';' - // Consume the return token. It is necessary to do the before + // Consume the return token. It is necessary to do before // reporting any errors on it, because of the way errors are // reported (underlining). Expect(Token::RETURN, CHECK_OK); diff --git a/src/preparser.h b/src/preparser.h index 3781631cca..4d4e10402e 100644 --- a/src/preparser.h +++ b/src/preparser.h @@ -922,7 +922,7 @@ class PreParserTraits { return PreParserExpression::Default(); } PreParserExpression NewThrowTypeError( - const char* type, Handle arg1, Handle arg2, int pos) { + const char* type, Handle arg, int pos) { return PreParserExpression::Default(); } diff --git a/src/runtime.cc b/src/runtime.cc index 67810ef152..768c992258 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -2117,15 +2117,11 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_SetAccessorProperty) { } -static Failure* ThrowRedeclarationError(Isolate* isolate, - const char* type, - Handle name) { +static Failure* ThrowRedeclarationError(Isolate* isolate, Handle name) { HandleScope scope(isolate); - Handle type_handle = - isolate->factory()->NewStringFromAscii(CStrVector(type)); - Handle args[2] = { type_handle, name }; - Handle error = - isolate->factory()->NewTypeError("redeclaration", HandleVector(args, 2)); + Handle args[1] = { name }; + Handle error = isolate->factory()->NewTypeError( + "var_redeclaration", HandleVector(args, 1)); return isolate->Throw(*error); } @@ -2202,7 +2198,7 @@ RUNTIME_FUNCTION(MaybeObject*, RuntimeHidden_DeclareGlobals) { if (lookup.IsFound() && lookup.IsDontDelete()) { if (lookup.IsReadOnly() || lookup.IsDontEnum() || lookup.IsPropertyCallbacks()) { - return ThrowRedeclarationError(isolate, "function", name); + return ThrowRedeclarationError(isolate, name); } // If the existing property is not configurable, keep its attributes. attr = lookup.GetAttributes(); @@ -2254,8 +2250,7 @@ RUNTIME_FUNCTION(MaybeObject*, RuntimeHidden_DeclareContextSlot) { if (((attributes & READ_ONLY) != 0) || (mode == READ_ONLY)) { // Functions are not read-only. ASSERT(mode != READ_ONLY || initial_value->IsTheHole()); - const char* type = ((attributes & READ_ONLY) != 0) ? "const" : "var"; - return ThrowRedeclarationError(isolate, type, name); + return ThrowRedeclarationError(isolate, name); } // Initialize it if necessary. @@ -2309,7 +2304,7 @@ RUNTIME_FUNCTION(MaybeObject*, RuntimeHidden_DeclareContextSlot) { LookupResult lookup(isolate); object->Lookup(*name, &lookup); if (lookup.IsPropertyCallbacks()) { - return ThrowRedeclarationError(isolate, "const", name); + return ThrowRedeclarationError(isolate, name); } } if (object->IsJSGlobalObject()) { diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc index ccb5e8c992..e9429bf385 100644 --- a/test/cctest/test-parsing.cc +++ b/test/cctest/test-parsing.cc @@ -1480,9 +1480,10 @@ TEST(ParserSync) { "break", "break label", "break\nlabel", - "return", - "return 12", - "return\n12", + // TODO(marja): activate once parsing 'return' is merged into ParserBase. + // "return", + // "return 12", + // "return\n12", "with ({}) ;", "with ({}) {}", "with ({}) 12", diff --git a/test/mjsunit/debug-stepout-scope-part2.js b/test/mjsunit/debug-stepout-scope-part2.js index 121c7b74df..69cee994aa 100644 --- a/test/mjsunit/debug-stepout-scope-part2.js +++ b/test/mjsunit/debug-stepout-scope-part2.js @@ -54,7 +54,7 @@ Debug.setListener(listener); var q = 42; var prefixes = [ "debugger; ", - "if (false) { try { throw 0; } catch(x) { return x; } }; debugger; " ]; + "if (false) { try { throw 0; } catch(x) { this.x = x; } }; debugger; " ]; var bodies = [ "1", "1 ", "1;", diff --git a/test/mjsunit/debug-stepout-scope-part3.js b/test/mjsunit/debug-stepout-scope-part3.js index 16b085e541..319f87991f 100644 --- a/test/mjsunit/debug-stepout-scope-part3.js +++ b/test/mjsunit/debug-stepout-scope-part3.js @@ -55,7 +55,7 @@ Debug.setListener(listener); var q = 42; var prefixes = [ "debugger; ", - "if (false) { try { throw 0; } catch(x) { return x; } }; debugger; " ]; + "if (false) { try { throw 0; } catch(x) { this.x = x; } }; debugger; " ]; var with_bodies = [ "with ({}) {}", "with ({x:1}) x", "with ({x:1}) x = 1", diff --git a/test/mjsunit/debug-stepout-scope-part4.js b/test/mjsunit/debug-stepout-scope-part4.js index 48f43477d7..eb9c82f8c7 100644 --- a/test/mjsunit/debug-stepout-scope-part4.js +++ b/test/mjsunit/debug-stepout-scope-part4.js @@ -55,7 +55,7 @@ Debug.setListener(listener); var q = 42; var prefixes = [ "debugger; ", - "if (false) { try { throw 0; } catch(x) { return x; } }; debugger; " ]; + "if (false) { try { throw 0; } catch(x) { this.x = x; } }; debugger; " ]; var bodies = [ "1", "1 ", "1;", diff --git a/test/mjsunit/debug-stepout-scope-part5.js b/test/mjsunit/debug-stepout-scope-part5.js index f060ec3889..250bee4d81 100644 --- a/test/mjsunit/debug-stepout-scope-part5.js +++ b/test/mjsunit/debug-stepout-scope-part5.js @@ -54,7 +54,7 @@ Debug.setListener(listener); var q = 42; var prefixes = [ "debugger; ", - "if (false) { try { throw 0; } catch(x) { return x; } }; debugger; " ]; + "if (false) { try { throw 0; } catch(x) { this.x = x; } }; debugger; " ]; var with_bodies = [ "with ({}) {}", "with ({x:1}) x", "with ({x:1}) x = 1", diff --git a/test/mjsunit/debug-stepout-scope-part6.js b/test/mjsunit/debug-stepout-scope-part6.js index f7c8df0bc8..2d8357fea4 100644 --- a/test/mjsunit/debug-stepout-scope-part6.js +++ b/test/mjsunit/debug-stepout-scope-part6.js @@ -54,7 +54,7 @@ Debug.setListener(listener); var q = 42; var prefixes = [ "debugger; ", - "if (false) { try { throw 0; } catch(x) { return x; } }; debugger; " ]; + "if (false) { try { throw 0; } catch(x) { this.x = x; } }; debugger; " ]; var bodies = [ "1", "1 ", "1;", diff --git a/test/mjsunit/delay-syntax-error.js b/test/mjsunit/delay-syntax-error.js index 64cc1429bb..20b2affacc 100644 --- a/test/mjsunit/delay-syntax-error.js +++ b/test/mjsunit/delay-syntax-error.js @@ -25,18 +25,11 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -// To be compatible with JSC syntax errors for illegal returns should be delayed -// to runtime. -// Invalid continue and break statements are caught at compile time. - -// Do not throw syntax errors for illegal return at compile time. -assertDoesNotThrow("if (false) return;"); - -// Throw syntax errors for illegal break and continue at compile time. +// Throw syntax errors for illegal return, break and continue at compile time. +assertThrows("if (false) return;"); assertThrows("if (false) break;"); assertThrows("if (false) continue;"); -// Throw syntax errors for illegal return, break and continue at runtime. assertThrows("return;"); assertThrows("break;"); assertThrows("continue;"); diff --git a/test/webkit/fast/js/arguments-expected.txt b/test/webkit/fast/js/arguments-expected.txt index ce1b383f59..f5bbb72936 100644 --- a/test/webkit/fast/js/arguments-expected.txt +++ b/test/webkit/fast/js/arguments-expected.txt @@ -157,7 +157,7 @@ PASS access_after_delete_extra_5(1, 2, 3, 4, 5) is 5 PASS argumentsParam(true) is true PASS argumentsFunctionConstructorParam(true) is true PASS argumentsVarUndefined() is '[object Arguments]' -FAIL argumentsConstUndefined() should be [object Arguments]. Threw exception TypeError: Variable 'arguments' has already been declared +FAIL argumentsConstUndefined() should be [object Arguments]. Threw exception TypeError: Identifier 'arguments' has already been declared PASS argumentCalleeInException() is argumentCalleeInException PASS shadowedArgumentsApply([true]) is true PASS shadowedArgumentsLength([]) is 0