From a13e2298e20976fc2e2a76eaba97a81f6e88aab7 Mon Sep 17 00:00:00 2001 From: "arv@chromium.org" Date: Tue, 28 Oct 2014 12:23:26 +0000 Subject: [PATCH] Allow duplicate property names in classes ES6 no longer makes duplicate properties an error. However, we continue to treat duplicate properties in strict mode object literals as errors. With this change we allow duplicate properties in class bodies. We continue to flag duplicate constructors as an error as required by ES6. BUG=v8:3570 LOG=Y R=marja@chromium.org Review URL: https://codereview.chromium.org/677953004 Cr-Commit-Position: refs/heads/master@{#24933} git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@24933 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/messages.js | 3 ++- src/preparser.h | 54 +++++++++++++++++++++++++++------------------ test/cctest/test-parsing.cc | 9 +++----- 3 files changed, 37 insertions(+), 29 deletions(-) diff --git a/src/messages.js b/src/messages.js index 6578e8d..513ffdc 100644 --- a/src/messages.js +++ b/src/messages.js @@ -176,7 +176,8 @@ var kMessages = { module_export_undefined: ["Export '", "%0", "' is not defined in module"], unexpected_super: ["'super' keyword unexpected here"], extends_value_not_a_function: ["Class extends value ", "%0", " is not a function or null"], - prototype_parent_not_an_object: ["Class extends value does not have valid prototype property ", "%0"] + prototype_parent_not_an_object: ["Class extends value does not have valid prototype property ", "%0"], + duplicate_constructor: ["A class may only have one constructor"] }; diff --git a/src/preparser.h b/src/preparser.h index a53abfe..2db88e7 100644 --- a/src/preparser.h +++ b/src/preparser.h @@ -476,6 +476,7 @@ class ParserBase : public Traits { ExpressionT ParseObjectLiteral(bool* ok); ObjectLiteralPropertyT ParsePropertyDefinition(ObjectLiteralChecker* checker, bool in_class, bool is_static, + bool* has_seen_constructor, bool* ok); typename Traits::Type::ExpressionList ParseArguments(bool* ok); ExpressionT ParseAssignmentExpression(bool accept_IN, bool* ok); @@ -1183,10 +1184,7 @@ class PreParserTraits { return false; } - bool IsConstructorProperty(PreParserExpression property) { return false; } - static PreParserExpression GetPropertyValue(PreParserExpression property) { - UNREACHABLE(); return PreParserExpression::Default(); } @@ -1925,7 +1923,9 @@ typename ParserBase::IdentifierT ParserBase::ParsePropertyName( template typename ParserBase::ObjectLiteralPropertyT ParserBase< Traits>::ParsePropertyDefinition(ObjectLiteralChecker* checker, - bool in_class, bool is_static, bool* ok) { + bool in_class, bool is_static, + bool* has_seen_constructor, bool* ok) { + DCHECK(!in_class || is_static || has_seen_constructor != NULL); ExpressionT value = this->EmptyExpression(); bool is_get = false; bool is_set = false; @@ -1942,8 +1942,10 @@ typename ParserBase::ObjectLiteralPropertyT ParserBase< if (!in_class && !is_generator && peek() == Token::COLON) { // PropertyDefinition : PropertyName ':' AssignmentExpression - checker->CheckProperty(name_token, kValueProperty, - CHECK_OK_CUSTOM(EmptyObjectLiteralProperty)); + if (checker != NULL) { + checker->CheckProperty(name_token, kValueProperty, + CHECK_OK_CUSTOM(EmptyObjectLiteralProperty)); + } Consume(Token::COLON); value = this->ParseAssignmentExpression( true, CHECK_OK_CUSTOM(EmptyObjectLiteralProperty)); @@ -1968,11 +1970,20 @@ typename ParserBase::ObjectLiteralPropertyT ParserBase< return this->EmptyObjectLiteralProperty(); } + if (*has_seen_constructor) { + ReportMessageAt(scanner()->location(), "duplicate_constructor"); + *ok = false; + return this->EmptyObjectLiteralProperty(); + } + + *has_seen_constructor = true; kind = FunctionKind::kNormalFunction; } - checker->CheckProperty(name_token, kValueProperty, - CHECK_OK_CUSTOM(EmptyObjectLiteralProperty)); + if (checker != NULL) { + checker->CheckProperty(name_token, kValueProperty, + CHECK_OK_CUSTOM(EmptyObjectLiteralProperty)); + } value = this->ParseFunctionLiteral( name, scanner()->location(), @@ -1983,7 +1994,7 @@ typename ParserBase::ObjectLiteralPropertyT ParserBase< } else if (in_class && name_is_static && !is_static) { // static MethodDefinition - return ParsePropertyDefinition(checker, true, true, ok); + return ParsePropertyDefinition(checker, true, true, NULL, ok); } else if (is_get || is_set) { // Accessor @@ -1998,16 +2009,15 @@ typename ParserBase::ObjectLiteralPropertyT ParserBase< *ok = false; return this->EmptyObjectLiteralProperty(); } else if (in_class && !is_static && this->IsConstructor(name)) { - // ES6, spec draft rev 27, treats static get constructor as an error too. - // https://bugs.ecmascript.org/show_bug.cgi?id=3223 - // TODO(arv): Update when bug is resolved. ReportMessageAt(scanner()->location(), "constructor_special_method"); *ok = false; return this->EmptyObjectLiteralProperty(); } - checker->CheckProperty(name_token, - is_get ? kGetterProperty : kSetterProperty, - CHECK_OK_CUSTOM(EmptyObjectLiteralProperty)); + if (checker != NULL) { + checker->CheckProperty(name_token, + is_get ? kGetterProperty : kSetterProperty, + CHECK_OK_CUSTOM(EmptyObjectLiteralProperty)); + } typename Traits::Type::FunctionLiteral value = this->ParseFunctionLiteral( name, scanner()->location(), @@ -2061,8 +2071,8 @@ typename ParserBase::ExpressionT ParserBase::ParseObjectLiteral( const bool in_class = false; const bool is_static = false; - ObjectLiteralPropertyT property = - this->ParsePropertyDefinition(&checker, in_class, is_static, CHECK_OK); + ObjectLiteralPropertyT property = this->ParsePropertyDefinition( + &checker, in_class, is_static, NULL, CHECK_OK); // Mark top-level object literals that contain function literals and // pretenure the literal so it can be added as a constant function @@ -2744,22 +2754,22 @@ typename ParserBase::ExpressionT ParserBase::ParseClassLiteral( scope_->SetStrictMode(STRICT); scope_->SetScopeName(name); - ObjectLiteralChecker checker(this, STRICT); typename Traits::Type::PropertyList properties = this->NewPropertyList(4, zone_); ExpressionT constructor = this->EmptyExpression(); + bool has_seen_constructor = false; Expect(Token::LBRACE, CHECK_OK); while (peek() != Token::RBRACE) { if (Check(Token::SEMICOLON)) continue; if (fni_ != NULL) fni_->Enter(); - const bool in_class = true; const bool is_static = false; - ObjectLiteralPropertyT property = - this->ParsePropertyDefinition(&checker, in_class, is_static, CHECK_OK); + bool old_has_seen_constructor = has_seen_constructor; + ObjectLiteralPropertyT property = this->ParsePropertyDefinition( + NULL, in_class, is_static, &has_seen_constructor, CHECK_OK); - if (this->IsConstructorProperty(property)) { + if (has_seen_constructor != old_has_seen_constructor) { constructor = this->GetPropertyValue(property); } else { properties->Add(property, zone()); diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc index 4028167..55c5b6c 100644 --- a/test/cctest/test-parsing.cc +++ b/test/cctest/test-parsing.cc @@ -4041,9 +4041,6 @@ TEST(ClassConstructorNoErrors) { TEST(ClassMultipleConstructorErrors) { - // We currently do not allow any duplicate properties in class bodies. This - // test ensures that when we change that we still throw on duplicate - // constructors. const char* context_data[][2] = {{"class C {", "}"}, {"(class {", "});"}, {NULL, NULL}}; @@ -4061,9 +4058,7 @@ TEST(ClassMultipleConstructorErrors) { } -// TODO(arv): We should allow duplicate property names. -// https://code.google.com/p/v8/issues/detail?id=3570 -DISABLED_TEST(ClassMultiplePropertyNamesNoErrors) { +TEST(ClassMultiplePropertyNamesNoErrors) { const char* context_data[][2] = {{"class C {", "}"}, {"(class {", "});"}, {NULL, NULL}}; @@ -4072,6 +4067,8 @@ DISABLED_TEST(ClassMultiplePropertyNamesNoErrors) { "constructor() {}; static constructor() {}", "m() {}; static m() {}", "m() {}; m() {}", + "static m() {}; static m() {}", + "get m() {}; set m(_) {}; get m() {}; set m(_) {};", NULL}; static const ParserFlag always_flags[] = { -- 2.7.4