Add back the duplicate property checker

We're not quite ready to make this change.

BUG=v8:3498
LOG=Y
R=rossberg@chromium.org

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

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

src/preparser.h

@@ -130,6 +130,7 @@ class ParserBase : public Traits {
   };
 
   class CheckpointBase;
+  class ObjectLiteralChecker;
 
   // ---------------------------------------------------------------------------
   // FunctionState and BlockState together implement the parser's scope stack.
@@ -473,7 +474,8 @@ class ParserBase : public Traits {
   ExpressionT ParseExpression(bool accept_IN, bool* ok);
   ExpressionT ParseArrayLiteral(bool* ok);
   ExpressionT ParseObjectLiteral(bool* ok);
-  ObjectLiteralPropertyT ParsePropertyDefinition(bool* ok);
+  ObjectLiteralPropertyT ParsePropertyDefinition(ObjectLiteralChecker* checker,
+                                                 bool* ok);
   typename Traits::Type::ExpressionList ParseArguments(bool* ok);
   ExpressionT ParseAssignmentExpression(bool accept_IN, bool* ok);
   ExpressionT ParseYieldExpression(bool* ok);
@@ -496,6 +498,60 @@ class ParserBase : public Traits {
       ExpressionT expression,
       Scanner::Location location, const char* message, bool* ok);
 
+  // Used to detect duplicates in object literals. Each of the values
+  // kGetterProperty, kSetterProperty and kValueProperty represents
+  // a type of object literal property. When parsing a property, its
+  // type value is stored in the DuplicateFinder for the property name.
+  // Values are chosen so that having intersection bits means the there is
+  // an incompatibility.
+  // I.e., you can add a getter to a property that already has a setter, since
+  // kGetterProperty and kSetterProperty doesn't intersect, but not if it
+  // already has a getter or a value. Adding the getter to an existing
+  // setter will store the value (kGetterProperty | kSetterProperty), which
+  // is incompatible with adding any further properties.
+  enum PropertyKind {
+    kNone = 0,
+    // Bit patterns representing different object literal property types.
+    kGetterProperty = 1,
+    kSetterProperty = 2,
+    kValueProperty = 7,
+    // Helper constants.
+    kValueFlag = 4
+  };
+
+  // Validation per ECMA 262 - 11.1.5 "Object Initializer".
+  class ObjectLiteralChecker {
+   public:
+    ObjectLiteralChecker(ParserBase* parser, StrictMode strict_mode)
+        : parser_(parser),
+          finder_(scanner()->unicode_cache()),
+          strict_mode_(strict_mode) {}
+
+    void CheckProperty(Token::Value property, PropertyKind type, bool* ok);
+
+   private:
+    ParserBase* parser() const { return parser_; }
+    Scanner* scanner() const { return parser_->scanner(); }
+
+    // Checks the type of conflict based on values coming from PropertyType.
+    bool HasConflict(PropertyKind type1, PropertyKind type2) {
+      return (type1 & type2) != 0;
+    }
+    bool IsDataDataConflict(PropertyKind type1, PropertyKind type2) {
+      return ((type1 & type2) & kValueFlag) != 0;
+    }
+    bool IsDataAccessorConflict(PropertyKind type1, PropertyKind type2) {
+      return ((type1 ^ type2) & kValueFlag) != 0;
+    }
+    bool IsAccessorAccessorConflict(PropertyKind type1, PropertyKind type2) {
+      return ((type1 | type2) & kValueFlag) == 0;
+    }
+
+    ParserBase* parser_;
+    DuplicateFinder finder_;
+    StrictMode strict_mode_;
+  };
+
   // If true, the next (and immediately following) function literal is
   // preceded by a parenthesis.
   // Heuristically that means that the function will be called immediately,
@@ -1793,8 +1849,8 @@ typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseArrayLiteral(
 
 
 template <class Traits>
-typename ParserBase<Traits>::ObjectLiteralPropertyT
-ParserBase<Traits>::ParsePropertyDefinition(bool* ok) {
+typename ParserBase<Traits>::ObjectLiteralPropertyT ParserBase<
+    Traits>::ParsePropertyDefinition(ObjectLiteralChecker* checker, bool* ok) {
   LiteralT key = this->EmptyLiteral();
   Token::Value next = peek();
   int next_pos = peek_position();
@@ -1846,6 +1902,10 @@ ParserBase<Traits>::ParsePropertyDefinition(bool* ok) {
             name = ParseIdentifierName(
                 CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
         }
+        // Validate the property.
+        PropertyKind type = is_getter ? kGetterProperty : kSetterProperty;
+        checker->CheckProperty(next, type,
+                               CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
         typename Traits::Type::FunctionLiteral value =
             this->ParseFunctionLiteral(
                 name, scanner()->location(),
@@ -1863,6 +1923,10 @@ ParserBase<Traits>::ParsePropertyDefinition(bool* ok) {
     }
   }
 
+  // Validate the property
+  checker->CheckProperty(next, kValueProperty,
+                         CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
+
   Expect(Token::COLON, CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
   ExpressionT value = this->ParseAssignmentExpression(
       true, CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
@@ -1883,12 +1947,15 @@ typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseObjectLiteral(
   int number_of_boilerplate_properties = 0;
   bool has_function = false;
 
+  ObjectLiteralChecker checker(this, strict_mode());
+
   Expect(Token::LBRACE, CHECK_OK);
 
   while (peek() != Token::RBRACE) {
     if (fni_ != NULL) fni_->Enter();
 
-    ObjectLiteralPropertyT property = this->ParsePropertyDefinition(CHECK_OK);
+    ObjectLiteralPropertyT property =
+        this->ParsePropertyDefinition(&checker, CHECK_OK);
 
     // Mark top-level object literals that contain function literals and
     // pretenure the literal so it can be added as a constant function
@@ -2572,6 +2639,32 @@ ParserBase<Traits>::CheckAndRewriteReferenceExpression(
 #undef CHECK_OK_CUSTOM
 
 
+template <typename Traits>
+void ParserBase<Traits>::ObjectLiteralChecker::CheckProperty(
+    Token::Value property, PropertyKind type, bool* ok) {
+  int old;
+  if (property == Token::NUMBER) {
+    old = scanner()->FindNumber(&finder_, type);
+  } else {
+    old = scanner()->FindSymbol(&finder_, type);
+  }
+  PropertyKind old_type = static_cast<PropertyKind>(old);
+  if (HasConflict(old_type, type)) {
+    if (IsDataDataConflict(old_type, type)) {
+      // Both are data properties.
+      if (strict_mode_ == SLOPPY) return;
+      parser()->ReportMessage("strict_duplicate_property");
+    } else if (IsDataAccessorConflict(old_type, type)) {
+      // Both a data and an accessor property with the same name.
+      parser()->ReportMessage("accessor_data_property");
+    } else {
+      DCHECK(IsAccessorAccessorConflict(old_type, type));
+      // Both accessors of the same type.
+      parser()->ReportMessage("accessor_get_set");
+    }
+    *ok = false;
+  }
+}
 } }  // v8::internal
 
 #endif  // V8_PREPARSER_H

test/cctest/test-parsing.cc

@@ -2486,7 +2486,7 @@ TEST(StrictObjectLiteralChecking) {
     { NULL, NULL }
   };
 
-  // ES6 allows duplicate properties even in strict mode.
+  // These are only errors in strict mode.
   const char* statement_data[] = {
     "foo: 1, foo: 2",
     "\"foo\": 1, \"foo\": 2",
@@ -2499,7 +2499,7 @@ TEST(StrictObjectLiteralChecking) {
   };
 
   RunParserSyncTest(non_strict_context_data, statement_data, kSuccess);
-  RunParserSyncTest(strict_context_data, statement_data, kSuccess);
+  RunParserSyncTest(strict_context_data, statement_data, kError);
 }
 
 
@@ -2511,17 +2511,23 @@ TEST(ErrorsObjectLiteralChecking) {
   };
 
   const char* statement_data[] = {
-    ",",
-    // 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\"",
-    NULL
-  };
+      ",", "foo: 1, get foo() {}", "foo: 1, set foo(v) {}",
+      "\"foo\": 1, get \"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\"(v) {}",
+      "\"foo\": 1, get foo() {}", "\"foo\": 1, set foo(v) {}",
+      "1: 1, get \"1\"() {}", "1: 1, set \"1\"() {}",
+      "\"1\": 1, get 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\"", NULL};
 
   RunParserSyncTest(context_data, statement_data, kError);
 }
@@ -2565,24 +2571,6 @@ TEST(NoErrorsObjectLiteralChecking) {
     "super: 6",
     "eval: 7",
     "arguments: 8",
-    // Duplicate property names are allowed in ES6.
-    "foo: 1, get foo() {}",
-    "foo: 1, set foo(v) {}",
-    "\"foo\": 1, get \"foo\"() {}",
-    "\"foo\": 1, set \"foo\"(v) {}",
-    "1: 1, get 1() {}",
-    "1: 1, set 1(v) {}",
-    // 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\"(v) {}",
-    "\"foo\": 1, get foo() {}",
-    "\"foo\": 1, set foo(v) {}",
-    "1: 1, get \"1\"() {}",
-    "1: 1, set \"1\"(v) {}",
-    "\"1\": 1, get 1() {}",
-    "\"1\": 1, set 1(v) {}",
     NULL
   };
 

test/mjsunit/strict-mode.js

@@ -166,6 +166,17 @@ assertThrows('\
     "use strict";\
   }', SyntaxError);
 
+// Duplicate data properties.
+CheckStrictMode("var x = { dupe : 1, nondupe: 3, dupe : 2 };", SyntaxError);
+CheckStrictMode("var x = { '1234' : 1, '2345' : 2, '1234' : 3 };", SyntaxError);
+CheckStrictMode("var x = { '1234' : 1, '2345' : 2, 1234 : 3 };", SyntaxError);
+CheckStrictMode("var x = { 3.14 : 1, 2.71 : 2, 3.14 : 3 };", SyntaxError);
+CheckStrictMode("var x = { 3.14 : 1, '3.14' : 2 };", SyntaxError);
+CheckStrictMode("var x = { \
+  123: 1, \
+  123.00000000000000000000000000000000000000000000000000000000000000000001: 2 \
+}", SyntaxError);
+
 // Non-conflicting data properties.
 (function StrictModeNonDuplicate() {
   "use strict";
@@ -177,52 +188,37 @@ assertThrows('\
   };
 })();
 
-// Duplicate properties are no longer errors in ES6.
-(function Duplicates() {
-  "use strict";
-
-  ({ dupe : 1, nondupe: 3, dupe : 2 });
-  ({ '1234' : 1, '2345' : 2, '1234' : 3 });
-  ({ '1234' : 1, '2345' : 2, 1234 : 3 });
-  ({ 3.14 : 1, 2.71 : 2, 3.14 : 3 });
-  ({ 3.14 : 1, '3.14' : 2 });
-  ({
-    123: 1,
-    123.00000000000000000000000000000000000000000000000000000000000000000001: 2
-  });
-
-  // Two getters
-  ({ get foo() { }, get foo() { } });
-  ({ get foo(){}, get 'foo'(){}});
-  ({ get 12(){}, get '12'(){}});
-
-  // Two setters
-  ({ set foo(v) { }, set foo(v) { } });
-  ({ set foo(v) { }, set 'foo'(v) { } });
-  ({ set 13(v) { }, set '13'(v) { } });
-
-  // Setter and data
-  ({ foo: 'data', set foo(v) { } });
-  ({ set foo(v) { }, foo: 'data' });
-  ({ foo: 'data', set 'foo'(v) { } });
-  ({ set foo(v) { }, 'foo': 'data' });
-  ({ 'foo': 'data', set foo(v) { } });
-  ({ set 'foo'(v) { }, foo: 'data' });
-  ({ 'foo': 'data', set 'foo'(v) { } });
-  ({ set 'foo'(v) { }, 'foo': 'data' });
-  ({ 12: 1, set '12'(v){}});
-  ({ 12: 1, set 12(v){}});
-  ({ '12': 1, set '12'(v){}});
-  ({ '12': 1, set 12(v){}});
-
-  // Getter and data
-  ({ foo: 'data', get foo() { } });
-  ({ get foo() { }, foo: 'data' });
-  ({ 'foo': 'data', get foo() { } });
-  ({ get 'foo'() { }, 'foo': 'data' });
-  ({ '12': 1, get '12'(){}});
-  ({ '12': 1, get 12(){}});
-})();
+// Two getters (non-strict)
+assertThrows("var x = { get foo() { }, get foo() { } };", SyntaxError);
+assertThrows("var x = { get foo(){}, get 'foo'(){}};", SyntaxError);
+assertThrows("var x = { get 12(){}, get '12'(){}};", SyntaxError);
+
+// Two setters (non-strict)
+assertThrows("var x = { set foo(v) { }, set foo(v) { } };", SyntaxError);
+assertThrows("var x = { set foo(v) { }, set 'foo'(v) { } };", SyntaxError);
+assertThrows("var x = { set 13(v) { }, set '13'(v) { } };", SyntaxError);
+
+// Setter and data (non-strict)
+assertThrows("var x = { foo: 'data', set foo(v) { } };", SyntaxError);
+assertThrows("var x = { set foo(v) { }, foo: 'data' };", SyntaxError);
+assertThrows("var x = { foo: 'data', set 'foo'(v) { } };", SyntaxError);
+assertThrows("var x = { set foo(v) { }, 'foo': 'data' };", SyntaxError);
+assertThrows("var x = { 'foo': 'data', set foo(v) { } };", SyntaxError);
+assertThrows("var x = { set 'foo'(v) { }, foo: 'data' };", SyntaxError);
+assertThrows("var x = { 'foo': 'data', set 'foo'(v) { } };", SyntaxError);
+assertThrows("var x = { set 'foo'(v) { }, 'foo': 'data' };", SyntaxError);
+assertThrows("var x = { 12: 1, set '12'(v){}};", SyntaxError);
+assertThrows("var x = { 12: 1, set 12(v){}};", SyntaxError);
+assertThrows("var x = { '12': 1, set '12'(v){}};", SyntaxError);
+assertThrows("var x = { '12': 1, set 12(v){}};", SyntaxError);
+
+// Getter and data (non-strict)
+assertThrows("var x = { foo: 'data', get foo() { } };", SyntaxError);
+assertThrows("var x = { get foo() { }, foo: 'data' };", SyntaxError);
+assertThrows("var x = { 'foo': 'data', get foo() { } };", SyntaxError);
+assertThrows("var x = { get 'foo'() { }, 'foo': 'data' };", SyntaxError);
+assertThrows("var x = { '12': 1, get '12'(){}};", SyntaxError);
+assertThrows("var x = { '12': 1, get 12(){}};", SyntaxError);
 
 // Assignment to eval or arguments
 CheckStrictMode("function strict() { eval = undefined; }", SyntaxError);

test/test262/test262.status

@@ -77,17 +77,6 @@
   'S8.5_A2.1': [PASS, FAIL_OK],
   'S8.5_A2.2': [PASS, FAIL_OK],
 
-  # ES6 allows duplicate properties
-  '11.1.5-4-4-a-1-s': [FAIL],
-  '11.1.5_4-4-b-1': [FAIL],
-  '11.1.5_4-4-b-2': [FAIL],
-  '11.1.5_4-4-c-1': [FAIL],
-  '11.1.5_4-4-c-2': [FAIL],
-  '11.1.5_4-4-d-1': [FAIL],
-  '11.1.5_4-4-d-2': [FAIL],
-  '11.1.5_4-4-d-3': [FAIL],
-  '11.1.5_4-4-d-4': [FAIL],
-
   ############################ INVALID TESTS #############################
 
   # The reference value calculated by Test262 is incorrect if you run these

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

@@ -26,20 +26,20 @@ Make sure that we correctly identify parse errors in object literals
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS ({a:1, get a(){}}), true is true
-PASS ({a:1, set a(v){}}), true is true
-PASS ({get a(){}, a:1}), true is true
-PASS ({set a(v){}, a:1}), true is true
-PASS ({get a(){}, get a(){}}), true is true
-PASS ({set a(v){}, set a(v){}}), true is true
-PASS ({set a(v){}, get a(){}, set a(v){}}), true is true
-PASS (function(){({a:1, get a(){}})}), true is true
-PASS (function(){({a:1, set a(v){}})}), true is true
-PASS (function(){({get a(){}, a:1})}), true is true
-PASS (function(){({set a(v){}, a:1})}), true is true
-PASS (function(){({get a(){}, get a(){}})}), true is true
-PASS (function(){({set a(v){}, set a(v){}})}), true is true
-PASS (function(){({set a(v){}, get a(){}, set a(v){}})}), true is true
+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(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(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(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(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(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(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(v){}}), true is true
 PASS ({set a(v){}, get a(){}}), true is true
@@ -48,6 +48,5 @@ 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

@@ -23,20 +23,20 @@
 
 description("Make sure that we correctly identify parse errors in object literals");
 
-shouldBeTrue("({a:1, get a(){}}), true");
-shouldBeTrue("({a:1, set a(v){}}), true");
-shouldBeTrue("({get a(){}, a:1}), true");
-shouldBeTrue("({set a(v){}, a:1}), true");
-shouldBeTrue("({get a(){}, get a(){}}), true");
-shouldBeTrue("({set a(v){}, set a(v){}}), true");
-shouldBeTrue("({set a(v){}, get a(){}, set a(v){}}), true");
-shouldBeTrue("(function(){({a:1, get a(){}})}), true");
-shouldBeTrue("(function(){({a:1, set a(v){}})}), true");
-shouldBeTrue("(function(){({get a(){}, a:1})}), true");
-shouldBeTrue("(function(){({set a(v){}, a:1})}), true");
-shouldBeTrue("(function(){({get a(){}, get a(){}})}), true");
-shouldBeTrue("(function(){({set a(v){}, set a(v){}})}), true");
-shouldBeTrue("(function(){({set a(v){}, get a(){}, set a(v){}})}), true");
+shouldThrow("({a:1, get a(){}})");
+shouldThrow("({a:1, set a(v){}})");
+shouldThrow("({get a(){}, a:1})");
+shouldThrow("({set a(v){}, a:1})");
+shouldThrow("({get a(){}, get 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(v){}})})");
+shouldThrow("(function(){({get a(){}, a:1})})");
+shouldThrow("(function(){({set a(v){}, a:1})})");
+shouldThrow("(function(){({get a(){}, get 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(v){}}), true");
 shouldBeTrue("({set a(v){}, get a(){}}), true");