Throw syntax error when a getter/setter has the wrong number of params

We used to allow any number of parameters in getters and setters to
match JSC. This is a violation of ES5.1 and both SpiderMonkey and
Chakra throw on these syntax errors.

BUG=v8:3371
LOG=Y
R=marja@chromium.org

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

Patch from Erik Arvidsson <arv@chromium.org>.

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

src/ast.h

@@ -2304,6 +2304,12 @@ class FunctionLiteral V8_FINAL : public Expression {
     kNotGenerator
   };
 
+  enum ArityRestriction {
+    NORMAL_ARITY,
+    GETTER_ARITY,
+    SETTER_ARITY
+  };
+
   DECLARE_NODE_TYPE(FunctionLiteral)
 
   Handle<String> name() const { return raw_name_->string(); }

src/parser.cc

@@ -746,10 +746,12 @@ FunctionLiteral* ParserTraits::ParseFunctionLiteral(
     bool is_generator,
     int function_token_position,
     FunctionLiteral::FunctionType type,
+    FunctionLiteral::ArityRestriction arity_restriction,
     bool* ok) {
   return parser_->ParseFunctionLiteral(name, function_name_location,
                                        name_is_strict_reserved, is_generator,
-                                       function_token_position, type, ok);
+                                       function_token_position, type,
+                                       arity_restriction, ok);
 }
 
 
@@ -1020,6 +1022,7 @@ FunctionLiteral* Parser::ParseLazy(Utf16CharacterStream* source) {
                                   shared_info->is_generator(),
                                   RelocInfo::kNoPosition,
                                   function_type,
+                                  FunctionLiteral::NORMAL_ARITY,
                                   &ok);
     // Make sure the results agree.
     ASSERT(ok == (result != NULL));
@@ -1871,6 +1874,7 @@ Statement* Parser::ParseFunctionDeclaration(ZoneList<const AstString*>* names,
                                               is_generator,
                                               pos,
                                               FunctionLiteral::DECLARATION,
+                                              FunctionLiteral::NORMAL_ARITY,
                                               CHECK_OK);
   // 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
@@ -3303,9 +3307,16 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     bool is_generator,
     int function_token_pos,
     FunctionLiteral::FunctionType function_type,
+    FunctionLiteral::ArityRestriction arity_restriction,
     bool* ok) {
   // Function ::
   //   '(' FormalParameterList? ')' '{' FunctionBody '}'
+  //
+  // Getter ::
+  //   '(' ')' '{' FunctionBody '}'
+  //
+  // Setter ::
+  //   '(' PropertySetParameterList ')' '{' FunctionBody '}'
 
   int pos = function_token_pos == RelocInfo::kNoPosition
       ? peek_position() : function_token_pos;
@@ -3400,7 +3411,9 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     Scanner::Location dupe_error_loc = Scanner::Location::invalid();
     Scanner::Location reserved_loc = Scanner::Location::invalid();
 
-    bool done = (peek() == Token::RPAREN);
+    bool done = arity_restriction == FunctionLiteral::GETTER_ARITY ||
+        (peek() == Token::RPAREN &&
+         arity_restriction != FunctionLiteral::SETTER_ARITY);
     while (!done) {
       bool is_strict_reserved = false;
       const AstString* param_name =
@@ -3425,6 +3438,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
         *ok = false;
         return NULL;
       }
+      if (arity_restriction == FunctionLiteral::SETTER_ARITY) break;
       done = (peek() == Token::RPAREN);
       if (!done) Expect(Token::COMMA, CHECK_OK);
     }

src/parser.h

@@ -582,6 +582,7 @@ class ParserTraits {
       bool is_generator,
       int function_token_position,
       FunctionLiteral::FunctionType type,
+      FunctionLiteral::ArityRestriction arity_restriction,
       bool* ok);
 
  private:
@@ -739,6 +740,7 @@ class Parser : public ParserBase<ParserTraits> {
       bool is_generator,
       int function_token_position,
       FunctionLiteral::FunctionType type,
+      FunctionLiteral::ArityRestriction arity_restriction,
       bool* ok);
 
   // Magical syntax support.

src/preparser.cc

@@ -95,10 +95,11 @@ PreParserExpression PreParserTraits::ParseFunctionLiteral(
     bool is_generator,
     int function_token_position,
     FunctionLiteral::FunctionType type,
+    FunctionLiteral::ArityRestriction arity_restriction,
     bool* ok) {
   return pre_parser_->ParseFunctionLiteral(
       name, function_name_location, name_is_strict_reserved, is_generator,
-      function_token_position, type, ok);
+      function_token_position, type, arity_restriction, ok);
 }
 
 
@@ -320,6 +321,7 @@ PreParser::Statement PreParser::ParseFunctionDeclaration(bool* ok) {
                        is_generator,
                        pos,
                        FunctionLiteral::DECLARATION,
+                       FunctionLiteral::NORMAL_ARITY,
                        CHECK_OK);
   return Statement::FunctionDeclaration();
 }
@@ -799,6 +801,7 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
     bool is_generator,
     int function_token_pos,
     FunctionLiteral::FunctionType function_type,
+    FunctionLiteral::ArityRestriction arity_restriction,
     bool* ok) {
   // Function ::
   //   '(' FormalParameterList? ')' '{' FunctionBody '}'
@@ -812,7 +815,6 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
   //    '(' (Identifier)*[','] ')'
   Expect(Token::LPAREN, CHECK_OK);
   int start_position = position();
-  bool done = (peek() == Token::RPAREN);
   DuplicateFinder duplicate_finder(scanner()->unicode_cache());
   // We don't yet know if the function will be strict, so we cannot yet produce
   // errors for parameter names or duplicates. However, we remember the
@@ -820,6 +822,10 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
   Scanner::Location eval_args_error_loc = Scanner::Location::invalid();
   Scanner::Location dupe_error_loc = Scanner::Location::invalid();
   Scanner::Location reserved_error_loc = Scanner::Location::invalid();
+
+  bool done = arity_restriction == FunctionLiteral::GETTER_ARITY ||
+      (peek() == Token::RPAREN &&
+       arity_restriction != FunctionLiteral::SETTER_ARITY);
   while (!done) {
     bool is_strict_reserved = false;
     Identifier param_name =
@@ -837,10 +843,9 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
       dupe_error_loc = scanner()->location();
     }
 
+    if (arity_restriction == FunctionLiteral::SETTER_ARITY) break;
     done = (peek() == Token::RPAREN);
-    if (!done) {
-      Expect(Token::COMMA, CHECK_OK);
-    }
+    if (!done) Expect(Token::COMMA, CHECK_OK);
   }
   Expect(Token::RPAREN, CHECK_OK);
 

src/preparser.h

@@ -1047,6 +1047,7 @@ class PreParserTraits {
       bool is_generator,
       int function_token_position,
       FunctionLiteral::FunctionType type,
+      FunctionLiteral::ArityRestriction arity_restriction,
       bool* ok);
 
  private:
@@ -1176,6 +1177,7 @@ class PreParser : public ParserBase<PreParserTraits> {
       bool is_generator,
       int function_token_pos,
       FunctionLiteral::FunctionType function_type,
+      FunctionLiteral::ArityRestriction arity_restriction,
       bool* ok);
   void ParseLazyFunctionLiteralBody(bool* ok);
 
@@ -1556,9 +1558,9 @@ typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseObjectLiteral(
                   false,  // reserved words are allowed here
                   false,  // not a generator
                   RelocInfo::kNoPosition, FunctionLiteral::ANONYMOUS_EXPRESSION,
+                  is_getter ? FunctionLiteral::GETTER_ARITY
+                            : FunctionLiteral::SETTER_ARITY,
                   CHECK_OK);
-          // Allow any number of parameters for compatibilty with JSC.
-          // Specification only allows zero parameters for get and one for set.
           typename Traits::Type::ObjectLiteralProperty property =
               factory()->NewObjectLiteralProperty(is_getter, value, next_pos);
           if (this->IsBoilerplateProperty(property)) {
@@ -2060,6 +2062,7 @@ ParserBase<Traits>::ParseMemberExpression(bool* ok) {
                                         is_generator,
                                         function_token_position,
                                         function_type,
+                                        FunctionLiteral::NORMAL_ARITY,
                                         CHECK_OK);
   } else {
     result = ParsePrimaryExpression(CHECK_OK);

test/cctest/test-parsing.cc

@@ -2233,22 +2233,27 @@ TEST(ErrorsObjectLiteralChecking) {
 
   const char* statement_data[] = {
     "foo: 1, get foo() {}",
-    "foo: 1, set foo() {}",
+    "foo: 1, set foo(v) {}",
     "\"foo\": 1, get \"foo\"() {}",
-    "\"foo\": 1, set \"foo\"() {}",
+    "\"foo\": 1, set \"foo\"(v) {}",
     "1: 1, get 1() {}",
     "1: 1, set 1() {}",
     // It's counter-intuitive, but these collide too (even in classic
     // mode). Note that we can have "foo" and foo as properties in classic mode,
     // but we cannot have "foo" and get foo, or foo and get "foo".
     "foo: 1, get \"foo\"() {}",
-    "foo: 1, set \"foo\"() {}",
+    "foo: 1, set \"foo\"(v) {}",
     "\"foo\": 1, get foo() {}",
-    "\"foo\": 1, set foo() {}",
+    "\"foo\": 1, set foo(v) {}",
     "1: 1, get \"1\"() {}",
     "1: 1, set \"1\"() {}",
     "\"1\": 1, get 1() {}"
-    "\"1\": 1, set 1() {}"
+    "\"1\": 1, set 1(v) {}"
+    // Wrong number of parameters
+    "get bar(x) {}",
+    "get bar(x, y) {}",
+    "set bar() {}",
+    "set bar(x, y) {}",
     // Parsing FunctionLiteral for getter or setter fails
     "get foo( +",
     "get foo() \"error\"",
@@ -2272,25 +2277,22 @@ TEST(NoErrorsObjectLiteralChecking) {
     "1: 1, 2: 2",
     // Syntax: IdentifierName ':' AssignmentExpression
     "foo: bar = 5 + baz",
-    // Syntax: 'get' (IdentifierName | String | Number) FunctionLiteral
+    // Syntax: 'get' PropertyName '(' ')' '{' FunctionBody '}'
     "get foo() {}",
     "get \"foo\"() {}",
     "get 1() {}",
-    // Syntax: 'set' (IdentifierName | String | Number) FunctionLiteral
-    "set foo() {}",
-    "set \"foo\"() {}",
-    "set 1() {}",
+    // Syntax: 'set' PropertyName '(' PropertySetParameterList ')'
+    //     '{' FunctionBody '}'
+    "set foo(v) {}",
+    "set \"foo\"(v) {}",
+    "set 1(v) {}",
     // Non-colliding getters and setters -> no errors
     "foo: 1, get bar() {}",
-    "foo: 1, set bar(b) {}",
+    "foo: 1, set bar(v) {}",
     "\"foo\": 1, get \"bar\"() {}",
-    "\"foo\": 1, set \"bar\"() {}",
+    "\"foo\": 1, set \"bar\"(v) {}",
     "1: 1, get 2() {}",
-    "1: 1, set 2() {}",
-    // Weird number of parameters -> no errors
-    "get bar() {}, set bar() {}",
-    "get bar(x) {}, set bar(x) {}",
-    "get bar(x, y) {}, set bar(x, y) {}",
+    "1: 1, set 2(v) {}",
     // Keywords, future reserved and strict future reserved are also allowed as
     // property names.
     "if: 4",

test/mozilla/mozilla.status

@@ -718,6 +718,13 @@
   'js1_5/extensions/regress-361964': [FAIL_OK],
   'js1_5/extensions/regress-363988': [FAIL_OK],
   'js1_5/extensions/regress-365869': [FAIL_OK],
+
+  # Uses non ES5 compatible syntax for setter
+  'js1_5/extensions/regress-367501-01': [FAIL_OK],
+  'js1_5/extensions/regress-367501-02': [FAIL_OK],
+  'js1_5/extensions/regress-367501-03': [FAIL_OK],
+  'js1_5/extensions/regress-367501-04': [FAIL_OK],
+
   'js1_5/extensions/regress-367630': [FAIL_OK],
   'js1_5/extensions/regress-367923': [FAIL_OK],
   'js1_5/extensions/regress-368859': [FAIL_OK],

test/preparser/duplicate-property.pyt

@@ -81,7 +81,7 @@ def PropertyTest(name, propa, propb, allow_strict = True):
     """, replacement, None)
 
   StrictTest("$name-nested-set", """
-      var o = {set $id1(){}, o: {set $id2(){} } };
+      var o = {set $id1(v){}, o: {set $id2(v){} } };
     """, replacement, None)
 
 

test/webkit/for-in-cached-expected.txt

@@ -34,8 +34,8 @@ PASS forIn3({ __proto__: { __proto__: { y3 : 2 } } }) is ['x', 'y3']
 PASS forIn4(objectWithArrayAsProto) is []
 PASS forIn4(objectWithArrayAsProto) is ['0']
 PASS forIn5({get foo() { return 'called getter'} }) is ['foo', 'called getter']
-PASS forIn5({set foo() { } }) is ['foo', undefined]
-PASS forIn5({get foo() { return 'called getter'}, set foo() { }}) is ['foo', 'called getter']
+PASS forIn5({set foo(v) { } }) is ['foo', undefined]
+PASS forIn5({get foo() { return 'called getter'}, set foo(v) { }}) is ['foo', 'called getter']
 PASS successfullyParsed is true
 
 TEST COMPLETE

test/webkit/for-in-cached.js

@@ -84,8 +84,8 @@ function forIn5(o) {
 }
 
 shouldBe("forIn5({get foo() { return 'called getter'} })", "['foo', 'called getter']");
-shouldBe("forIn5({set foo() { } })", "['foo', undefined]");
-shouldBe("forIn5({get foo() { return 'called getter'}, set foo() { }})", "['foo', 'called getter']");
+shouldBe("forIn5({set foo(v) { } })", "['foo', undefined]");
+shouldBe("forIn5({get foo() { return 'called getter'}, set foo(v) { }})", "['foo', 'called getter']");
 
 function cacheClearing() {
     for(var j=0; j < 10; j++){

test/webkit/object-literal-direct-put-expected.txt

@@ -28,11 +28,11 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
 
 PASS ({a:true}).a is true
 PASS ({__proto__: {a:false}, a:true}).a is true
-PASS ({__proto__: {set a() {throw 'Should not call setter'; }}, a:true}).a is true
+PASS ({__proto__: {set a(v) {throw 'Should not call setter'; }}, a:true}).a is true
 PASS ({__proto__: {get a() {throw 'Should not reach getter'; }}, a:true}).a is true
 PASS ({__proto__: {get a() {throw 'Should not reach getter'; }, b:true}, a:true}).b is true
 PASS ({__proto__: {__proto__: {a:false}}, a:true}).a is true
-PASS ({__proto__: {__proto__: {set a() {throw 'Should not call setter'; }}}, a:true}).a is true
+PASS ({__proto__: {__proto__: {set a(v) {throw 'Should not call setter'; }}}, a:true}).a is true
 PASS ({__proto__: {__proto__: {get a() {throw 'Should not reach getter'; }}}, a:true}).a is true
 PASS ({__proto__: {__proto__: {get a() {throw 'Should not reach getter'; }, b:true}}, a:true}).b is true
 PASS successfullyParsed is true

test/webkit/object-literal-direct-put.js

@@ -25,11 +25,11 @@ description("This test ensures that properties on an object literal are put dire
 
 shouldBeTrue("({a:true}).a");
 shouldBeTrue("({__proto__: {a:false}, a:true}).a");
-shouldBeTrue("({__proto__: {set a() {throw 'Should not call setter'; }}, a:true}).a");
+shouldBeTrue("({__proto__: {set a(v) {throw 'Should not call setter'; }}, a:true}).a");
 shouldBeTrue("({__proto__: {get a() {throw 'Should not reach getter'; }}, a:true}).a");
 shouldBeTrue("({__proto__: {get a() {throw 'Should not reach getter'; }, b:true}, a:true}).b");
 
 shouldBeTrue("({__proto__: {__proto__: {a:false}}, a:true}).a");
-shouldBeTrue("({__proto__: {__proto__: {set a() {throw 'Should not call setter'; }}}, a:true}).a");
+shouldBeTrue("({__proto__: {__proto__: {set a(v) {throw 'Should not call setter'; }}}, a:true}).a");
 shouldBeTrue("({__proto__: {__proto__: {get a() {throw 'Should not reach getter'; }}}, a:true}).a");
 shouldBeTrue("({__proto__: {__proto__: {get a() {throw 'Should not reach getter'; }, b:true}}, a:true}).b");

test/webkit/object-literal-syntax-expected.txt

@@ -27,25 +27,25 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
 
 
 PASS ({a:1, get a(){}}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
-PASS ({a:1, set a(){}}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
+PASS ({a:1, set a(v){}}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
 PASS ({get a(){}, a:1}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
-PASS ({set a(){}, a:1}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
+PASS ({set a(v){}, a:1}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
 PASS ({get a(){}, get a(){}}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
-PASS ({set a(){}, set a(){}}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
-PASS ({set a(){}, get a(){}, set a(){}}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
+PASS ({set a(v){}, set a(v){}}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
+PASS ({set a(v){}, get a(){}, set a(v){}}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
 PASS (function(){({a:1, get a(){}})}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
-PASS (function(){({a:1, set a(){}})}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
+PASS (function(){({a:1, set a(v){}})}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
 PASS (function(){({get a(){}, a:1})}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
-PASS (function(){({set a(){}, a:1})}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
+PASS (function(){({set a(v){}, a:1})}) threw exception SyntaxError: Object literal may not have data and accessor property with the same name.
 PASS (function(){({get a(){}, get a(){}})}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
-PASS (function(){({set a(){}, set a(){}})}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
-PASS (function(){({set a(){}, get a(){}, set a(){}})}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
+PASS (function(){({set a(v){}, set a(v){}})}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
+PASS (function(){({set a(v){}, get a(){}, set a(v){}})}) threw exception SyntaxError: Object literal may not have multiple get/set accessors with the same name.
 PASS ({a:1, a:1, a:1}), true is true
-PASS ({get a(){}, set a(){}}), true is true
-PASS ({set a(){}, get a(){}}), true is true
+PASS ({get a(){}, set a(v){}}), true is true
+PASS ({set a(v){}, get a(){}}), true is true
 PASS (function(){({a:1, a:1, a:1})}), true is true
-PASS (function(){({get a(){}, set a(){}})}), true is true
-PASS (function(){({set a(){}, get a(){}})}), true is true
+PASS (function(){({get a(){}, set a(v){}})}), true is true
+PASS (function(){({set a(v){}, get a(){}})}), true is true
 PASS successfullyParsed is true
 
 TEST COMPLETE

test/webkit/object-literal-syntax.js

@@ -24,22 +24,22 @@
 description("Make sure that we correctly identify parse errors in object literals");
 
 shouldThrow("({a:1, get a(){}})");
-shouldThrow("({a:1, set a(){}})");
+shouldThrow("({a:1, set a(v){}})");
 shouldThrow("({get a(){}, a:1})");
-shouldThrow("({set a(){}, a:1})");
+shouldThrow("({set a(v){}, a:1})");
 shouldThrow("({get a(){}, get a(){}})");
-shouldThrow("({set a(){}, set a(){}})");
-shouldThrow("({set a(){}, get a(){}, set a(){}})");
+shouldThrow("({set a(v){}, set a(v){}})");
+shouldThrow("({set a(v){}, get a(){}, set a(v){}})");
 shouldThrow("(function(){({a:1, get a(){}})})");
-shouldThrow("(function(){({a:1, set a(){}})})");
+shouldThrow("(function(){({a:1, set a(v){}})})");
 shouldThrow("(function(){({get a(){}, a:1})})");
-shouldThrow("(function(){({set a(){}, a:1})})");
+shouldThrow("(function(){({set a(v){}, a:1})})");
 shouldThrow("(function(){({get a(){}, get a(){}})})");
-shouldThrow("(function(){({set a(){}, set a(){}})})");
-shouldThrow("(function(){({set a(){}, get a(){}, set a(){}})})");
+shouldThrow("(function(){({set a(v){}, set a(v){}})})");
+shouldThrow("(function(){({set a(v){}, get a(){}, set a(v){}})})");
 shouldBeTrue("({a:1, a:1, a:1}), true");
-shouldBeTrue("({get a(){}, set a(){}}), true");
-shouldBeTrue("({set a(){}, get a(){}}), true");
+shouldBeTrue("({get a(){}, set a(v){}}), true");
+shouldBeTrue("({set a(v){}, get a(){}}), true");
 shouldBeTrue("(function(){({a:1, a:1, a:1})}), true");
-shouldBeTrue("(function(){({get a(){}, set a(){}})}), true");
-shouldBeTrue("(function(){({set a(){}, get a(){}})}), true");
+shouldBeTrue("(function(){({get a(){}, set a(v){}})}), true");
+shouldBeTrue("(function(){({set a(v){}, get a(){}})}), true");