Unify (Pre)Parser::ParseObjectLiteral and add tests.

Notes:
- The regexp in the ParseObjectLiteralComment was wrong, made it less wrong (
it's still wrong since trailing commas are not required / allowed).
- Change in logic: In case we have "get somekeyword() { }", the "somekeyword"
was not logged as a symbol by PreParser and not expected in the preparser data
by Parser. This is unnecessary complication; in other contexts where keywords
are allowed as identifiers, they are logged as symbols (see
ParseIdentifierName).

BUG=v8:3126
LOG=N
R=ulan@chromium.org

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

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

src/parser.cc

@@ -3492,10 +3492,11 @@ Handle<FixedArray> CompileTimeValue::GetElements(Handle<FixedArray> value) {
 
 Expression* Parser::ParseObjectLiteral(bool* ok) {
   // ObjectLiteral ::
-  //   '{' (
-  //       ((IdentifierName | String | Number) ':' AssignmentExpression)
-  //     | (('get' | 'set') (IdentifierName | String | Number) FunctionLiteral)
-  //    )*[','] '}'
+  // '{' ((
+  //       ((IdentifierName | String | Number) ':' AssignmentExpression) |
+  //       (('get' | 'set') (IdentifierName | String | Number) FunctionLiteral)
+  //      ) ',')* '}'
+  // (Except that trailing comma is not required and not allowed.)
 
   int pos = peek_position();
   ZoneList<ObjectLiteral::Property*>* properties =
@@ -3529,14 +3530,12 @@ Expression* Parser::ParseObjectLiteral(bool* ok) {
           // { ... , get foo() { ... }, ... , set foo(v) { ... v ... } , ... }
           // We have already read the "get" or "set" keyword.
           Token::Value next = Next();
-          bool is_keyword = Token::IsKeyword(next);
           if (next != i::Token::IDENTIFIER &&
               next != i::Token::FUTURE_RESERVED_WORD &&
               next != i::Token::FUTURE_STRICT_RESERVED_WORD &&
               next != i::Token::NUMBER &&
               next != i::Token::STRING &&
-              !is_keyword) {
-            // Unexpected token.
+              !Token::IsKeyword(next)) {
             ReportUnexpectedToken(next);
             *ok = false;
             return NULL;
@@ -3544,9 +3543,7 @@ Expression* Parser::ParseObjectLiteral(bool* ok) {
           // Validate the property.
           PropertyKind type = is_getter ? kGetterProperty : kSetterProperty;
           checker.CheckProperty(next, type, CHECK_OK);
-          Handle<String> name = is_keyword
-              ? isolate_->factory()->InternalizeUtf8String(Token::String(next))
-              : GetSymbol();
+          Handle<String> name = GetSymbol();
           FunctionLiteral* value =
               ParseFunctionLiteral(name,
                                    scanner()->location(),
@@ -3571,8 +3568,8 @@ Expression* Parser::ParseObjectLiteral(bool* ok) {
           }
           continue;  // restart the while
         }
-        // Failed to parse as get/set property, so it's just a property
-        // called "get" or "set".
+        // Failed to parse as get/set property, so it's just a normal property
+        // (which might be called "get" or "set" or something else).
         key = factory()->NewLiteral(id, next_pos);
         break;
       }
@@ -3604,7 +3601,6 @@ Expression* Parser::ParseObjectLiteral(bool* ok) {
           Handle<String> string = GetSymbol();
           key = factory()->NewLiteral(string, next_pos);
         } else {
-          // Unexpected token.
           Token::Value next = Next();
           ReportUnexpectedToken(next);
           *ok = false;

src/preparser.cc

@@ -1124,10 +1124,11 @@ PreParser::Expression PreParser::ParseMemberExpressionContinuation(
 
 PreParser::Expression PreParser::ParseObjectLiteral(bool* ok) {
   // ObjectLiteral ::
-  //   '{' (
-  //       ((IdentifierName | String | Number) ':' AssignmentExpression)
-  //     | (('get' | 'set') (IdentifierName | String | Number) FunctionLiteral)
-  //    )*[','] '}'
+  // '{' ((
+  //       ((IdentifierName | String | Number) ':' AssignmentExpression) |
+  //       (('get' | 'set') (IdentifierName | String | Number) FunctionLiteral)
+  //      ) ',')* '}'
+  // (Except that trailing comma is not required and not allowed.)
 
   ObjectLiteralChecker checker(this, scope_->language_mode());
 
@@ -1142,23 +1143,22 @@ PreParser::Expression PreParser::ParseObjectLiteral(bool* ok) {
         bool is_setter = false;
         ParseIdentifierNameOrGetOrSet(&is_getter, &is_setter, CHECK_OK);
         if ((is_getter || is_setter) && peek() != Token::COLON) {
-            Token::Value name = Next();
-            bool is_keyword = Token::IsKeyword(name);
-            if (name != Token::IDENTIFIER &&
-                name != Token::FUTURE_RESERVED_WORD &&
-                name != Token::FUTURE_STRICT_RESERVED_WORD &&
-                name != Token::NUMBER &&
-                name != Token::STRING &&
-                !is_keyword) {
+            Token::Value next = Next();
+            if (next != Token::IDENTIFIER &&
+                next != Token::FUTURE_RESERVED_WORD &&
+                next != Token::FUTURE_STRICT_RESERVED_WORD &&
+                next != Token::NUMBER &&
+                next != Token::STRING &&
+                !Token::IsKeyword(next)) {
+              ReportUnexpectedToken(next);
               *ok = false;
               return Expression::Default();
             }
-            if (!is_keyword) {
-              LogSymbol();
-            }
+            // Validate the property
             PropertyKind type = is_getter ? kGetterProperty : kSetterProperty;
-            checker.CheckProperty(name, type, CHECK_OK);
-            ParseFunctionLiteral(Identifier::Default(),
+            checker.CheckProperty(next, type, CHECK_OK);
+            PreParserIdentifier name = GetSymbol(scanner());
+            ParseFunctionLiteral(name,
                                  scanner()->location(),
                                  false,  // reserved words are allowed here
                                  false,  // not a generator
@@ -1168,30 +1168,30 @@ PreParser::Expression PreParser::ParseObjectLiteral(bool* ok) {
             }
             continue;  // restart the while
         }
-        checker.CheckProperty(next, kValueProperty, CHECK_OK);
         break;
       }
       case Token::STRING:
         Consume(next);
-        checker.CheckProperty(next, kValueProperty, CHECK_OK);
         LogSymbol();
         break;
       case Token::NUMBER:
         Consume(next);
-        checker.CheckProperty(next, kValueProperty, CHECK_OK);
         break;
       default:
         if (Token::IsKeyword(next)) {
           Consume(next);
-          checker.CheckProperty(next, kValueProperty, CHECK_OK);
           LogSymbol();
         } else {
-          // Unexpected token.
+          Token::Value next = Next();
+          ReportUnexpectedToken(next);
           *ok = false;
           return Expression::Default();
         }
     }
 
+    // Validate the property
+    checker.CheckProperty(next, kValueProperty, CHECK_OK);
+
     Expect(Token::COLON, CHECK_OK);
     ParseAssignmentExpression(true, CHECK_OK);
 

test/cctest/test-parsing.cc

@@ -1962,7 +1962,6 @@ TEST(DontRegressPreParserDataSizes) {
   // These tests make sure that PreParser doesn't start producing less data.
 
   v8::V8::Initialize();
-
   int marker;
   CcTest::i_isolate()->stack_guard()->SetStackLimit(
       reinterpret_cast<uintptr_t>(&marker) - 128 * 1024);
@@ -1972,9 +1971,18 @@ TEST(DontRegressPreParserDataSizes) {
     int symbols;
     int functions;
   } test_cases[] = {
-    // Labels, variables and functions are recorded as symbols.
+    // Labels and variables are recorded as symbols.
     {"{label: 42}", 1, 0}, {"{label: 42; label2: 43}", 2, 0},
     {"var x = 42;", 1, 0}, {"var x = 42, y = 43;", 2, 0},
+    {"var x = {y: 1};", 2, 0},
+    {"var x = {}; x.y = 1", 2, 0},
+    // "get" is recorded as a symbol too.
+    {"var x = {get foo(){} };", 3, 1},
+    // When keywords are used as identifiers, they're logged as symbols, too:
+    {"var x = {if: 1};", 2, 0},
+    {"var x = {}; x.if = 1", 2, 0},
+    {"var x = {get if(){} };", 3, 1},
+    // Functions
     {"function foo() {}", 1, 1}, {"function foo() {} function bar() {}", 2, 2},
     // Labels, variables and functions insize lazy functions are not recorded.
     {"function lazy() { var a, b, c; }", 1, 1},
@@ -2202,3 +2210,111 @@ TEST(ErrorsNewExpression) {
 
   RunParserSyncTest(context_data, statement_data, kError);
 }
+
+
+TEST(StrictObjectLiteralChecking) {
+  const char* strict_context_data[][2] = {
+    {"\"use strict\"; var myobject = {", "};"},
+    { NULL, NULL }
+  };
+  const char* non_strict_context_data[][2] = {
+    {"var myobject = {", "};"},
+    { NULL, NULL }
+  };
+
+  // These are only errors in strict mode.
+  const char* statement_data[] = {
+    "foo: 1, foo: 2",
+    "\"foo\": 1, \"foo\": 2",
+    "foo: 1, \"foo\": 2",
+    "1: 1, 1: 2",
+    "1: 1, \"1\": 2",
+    "get: 1, get: 2",  // Not a getter for real, just a property called get.
+    "set: 1, set: 2",  // Not a setter for real, just a property called set.
+    NULL
+  };
+
+  RunParserSyncTest(non_strict_context_data, statement_data, kSuccess);
+  RunParserSyncTest(strict_context_data, statement_data, kError);
+}
+
+
+TEST(ErrorsObjectLiteralChecking) {
+  const char* context_data[][2] = {
+    {"\"use strict\"; var myobject = {", "};"},
+    {"var myobject = {", "};"},
+    { NULL, NULL }
+  };
+
+  const char* statement_data[] = {
+    "foo: 1, get foo() {}",
+    "foo: 1, set foo() {}",
+    "\"foo\": 1, get \"foo\"() {}",
+    "\"foo\": 1, set \"foo\"() {}",
+    "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, get foo() {}",
+    "\"foo\": 1, set foo() {}",
+    "1: 1, get \"1\"() {}",
+    "1: 1, set \"1\"() {}",
+    "\"1\": 1, get 1() {}"
+    "\"1\": 1, set 1() {}"
+    // Parsing FunctionLiteral for getter or setter fails
+    "get foo( +",
+    "get foo() \"error\"",
+    NULL
+  };
+
+  RunParserSyncTest(context_data, statement_data, kError);
+}
+
+
+TEST(NoErrorsObjectLiteralChecking) {
+  const char* context_data[][2] = {
+    {"var myobject = {", "};"},
+    {"\"use strict\"; var myobject = {", "};"},
+    { NULL, NULL }
+  };
+
+  const char* statement_data[] = {
+    "foo: 1, bar: 2",
+    "\"foo\": 1, \"bar\": 2",
+    "1: 1, 2: 2",
+    // Syntax: IdentifierName ':' AssignmentExpression
+    "foo: bar = 5 + baz",
+    // Syntax: 'get' (IdentifierName | String | Number) FunctionLiteral
+    "get foo() {}",
+    "get \"foo\"() {}",
+    "get 1() {}",
+    // Syntax: 'set' (IdentifierName | String | Number) FunctionLiteral
+    "set foo() {}",
+    "set \"foo\"() {}",
+    "set 1() {}",
+    // Non-colliding getters and setters -> no errors
+    "foo: 1, get bar() {}",
+    "foo: 1, set bar(b) {}",
+    "\"foo\": 1, get \"bar\"() {}",
+    "\"foo\": 1, set \"bar\"() {}",
+    "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) {}",
+    // Keywords, future reserved and strict future reserved are also allowed as
+    // property names.
+    "if: 4",
+    "interface: 5",
+    "super: 6",
+    "eval: 7",
+    "arguments: 8",
+    NULL
+  };
+
+  RunParserSyncTest(context_data, statement_data, kSuccess);
+}