Strict mode object literal validation

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index 2637281f080ed9f7f32fba93701c4da068a4f34e..0027152099c6d0f4e02580a2697e45bc4873b27d 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -3012,6 +3012,120 @@ Handle<Object> Parser::GetBoilerplateValue(Expression* expression) {
   return Factory::undefined_value();
 }
 
+// Defined in ast.cc
+bool IsEqualString(void* first, void* second);

[Martin Maly, 2011/01/21 00:41:06]

These 2 are defined in ast.cc, ideally I'd want to move them to some shared
location. Is there a good place? This declaration feels suboptimal.

+bool IsEqualSmi(void* first, void* second);
+
+
+// Validation per 11.1.5 Object Initialiser
+class ObjectLiteralPropertyChecker {
+ public:
+  ObjectLiteralPropertyChecker(Parser* parser, bool strict) :
+    props(&IsEqualString),
+    elems(&IsEqualSmi),
+    parser_(parser),
+    strict_(strict) {
+  }
+
+  void CheckProperty(
+    ObjectLiteral::Property* property,
+    Scanner::Location loc,
+    bool* ok);
+
+ private:
+  enum PropertyKind {
+    GetAccessor = 0x01,

[Lasse Reichstein, 2011/01/21 12:01:13]

Add a "k" in front of constants, i.e., kGetAccessor, kSetAccessor, kAccessor,
kData.

[Martin Maly, 2011/01/21 20:00:37]

Done.

+    SetAccessor = 0x02,
+    Accessor = GetAccessor | SetAccessor,
+    Data = 0x04
+  };
+
+  HashMap props;

[Lasse Reichstein, 2011/01/21 12:01:13]

Properties should be at the end of the class, after methods (including static
methods).

[Martin Maly, 2011/01/21 20:00:37]

Done.

+  HashMap elems;
+  Parser* parser_;
+  bool strict_;
+
+  static intptr_t GetPropertyKind(ObjectLiteral::Property* property) {
+    switch (property->kind()) {
+      default:

[Lasse Reichstein, 2011/01/21 12:01:13]

Move the default to the end instead.
I just think it reads better :)

[Martin Maly, 2011/01/21 20:00:37]

Done.

+        return Data;
+      case ObjectLiteral::Property::GETTER:
+        return GetAccessor;
+      case ObjectLiteral::Property::SETTER:
+        return SetAccessor;
+    }
+  }
+};
+
+
+void ObjectLiteralPropertyChecker::CheckProperty(

[Martin Maly, 2011/01/21 00:41:06]

Similar code exists in ast.cc and is executed when emitting object literal. This
is too late because the object literal errors must be early errors.

+    ObjectLiteral::Property* property,
+    Scanner::Location loc,
+    bool* ok) {
+
+  ASSERT(property != NULL);
+
+  Literal *lit = property->key();
+  Handle<Object> handle = lit->handle();
+
+  uint32_t hash;
+  HashMap* map;
+  void* key;
+
+  if (handle->IsSymbol()) {
+    Handle<String> name(String::cast(*handle));
+    ASSERT(!name->AsArrayIndex(&hash));

[Lasse Reichstein, 2011/01/21 12:01:13]

I'm a little worried that we might have a symbol that is also a valid
ArrayIndex.
We might not, but I haven't been able to convince myself that it can't happen
yet.

[Martin Maly, 2011/01/21 20:00:37]

I am less concerned since the same checks are performed in ast.cc 
ObjectLiteral::CalculateEmitStore which has been around for a while.

[Lasse Reichstein, 2011/01/24 07:57:01]

Damnit. I fixed one such place already, but it seems there are more. It's good
that you found it :)
It's NOT safe. The ASSERT gets hit by, e.g.:
var y = {get "1234"() {return 42; }};

[Martin Maly, 2011/01/24 18:09:09]

Great catch. I fixed both locations and added more tests.

+    key = handle.location();
+    hash = name->Hash();
+    map = &props;
+  } else if (handle->ToArrayIndex(&hash)) {
+    key = handle.location();
+    map = &elems;
+  } else {
+    ASSERT(handle->IsNumber());
+    double num = handle->Number();
+    char arr[100];
+    Vector<char> buffer(arr, ARRAY_SIZE(arr));
+    const char* str = DoubleToCString(num, buffer);
+    Handle<String> name = Factory::NewStringFromAscii(CStrVector(str));
+    key = name.location();
+    hash = name->Hash();
+    map = &props;
+  }
+
+  // Lookup property previously defined, if any.
+  HashMap::Entry* entry = map->Lookup(key, hash, true);
+  intptr_t prev = reinterpret_cast<intptr_t> (entry->value);
+  intptr_t curr = GetPropertyKind(property);
+
+  // Duplicate data properties are illegal in strict mode.
+  if (strict_ && (curr & prev & Data) != 0) {
+    parser_->ReportMessageAt(loc, "strict_duplicate_property",
+                            Vector<const char*>::empty());
+    *ok = false;
+    return;
+  }
+  // Data property conflicting with an accessor.

[Martin Maly, 2011/01/21 00:41:06]

Most of these checks should execute even outside of strict mode. Only duplicate
data properties are strict mode check.

[Lasse Reichstein, 2011/01/21 12:01:13]

We should be careful when adding restrictions to non-strict code that isn't
there already. There is a potential to break existing code on the web.

We should at least check whether other significant browsers already fail on the
code, and not be the first to break the code.

[MarkM, 2011/01/21 17:37:55]

On WebKit nightly Version 5.0.1 (5533.17.8, r75891) non-strict

    ({x: 33, get x(){}});  
    SyntaxError: Parse error

On Minefield/FF 4.0b10pre (2011-01-21) non-strict

    ({x: 33, get x(){}});
    SyntaxError: property name x appears more than once in object literal

[Martin Maly, 2011/01/21 20:00:37]

Thank you, Mark, for doing the research. The shipping versions of Firefox,
Safari do not report error, however. Since their future versions are
implementing this I would recommend we keep the full checks in place as
implemented, although there may be good reason (like fast Chrome release cycle)
that may lead us to wait with full check until some future date and leave them
all under strict mode for now.

+  if (((curr & Data) && (prev & Accessor)) ||
+      ((prev & Data) && (curr & Accessor))) {
+    parser_->ReportMessageAt(loc, "accessor_data_property",
+                            Vector<const char*>::empty());
+    *ok = false;
+    return;
+  }
+  // Two accessors of the same type conflicting
+  if ((curr & prev & Accessor) != 0) {
+    parser_->ReportMessageAt(loc, "accessor_get_set",
+                            Vector<const char*>::empty());
+    *ok = false;
+    return;
+  }
+
+  // Update map
+  entry->value = reinterpret_cast<void*> (prev | curr);
+  *ok = true;
+}
+
 
 void Parser::BuildObjectLiteralConstantProperties(
     ZoneList<ObjectLiteral::Property*>* properties,
@@ -3117,12 +3231,20 @@ Expression* Parser::ParseObjectLiteral(bool* ok) {
       new ZoneList<ObjectLiteral::Property*>(4);
   int number_of_boilerplate_properties = 0;
 
+  ObjectLiteralPropertyChecker checker(this, temp_scope_->StrictMode());
+
   Expect(Token::LBRACE, CHECK_OK);
+  Scanner::Location loc = scanner().location();
+
   while (peek() != Token::RBRACE) {
     if (fni_ != NULL) fni_->Enter();
 
     Literal* key = NULL;
     Token::Value next = peek();
+
+    // Location of the property name token
+    Scanner::Location loc = scanner().peek_location();
+
     switch (next) {
       case Token::IDENTIFIER: {
         bool is_getter = false;
@@ -3132,11 +3254,15 @@ Expression* Parser::ParseObjectLiteral(bool* ok) {
         if (fni_ != NULL) fni_->PushLiteralName(id);
 
         if ((is_getter || is_setter) && peek() != Token::COLON) {
+            // Update loc to point to the identifier
+            loc = scanner().peek_location();
             ObjectLiteral::Property* property =
                 ParseObjectLiteralGetSet(is_getter, CHECK_OK);
             if (IsBoilerplateProperty(property)) {
               number_of_boilerplate_properties++;
             }
+            // Validate the property.
+            checker.CheckProperty(property, loc, CHECK_OK);
             properties->Add(property);
             if (peek() != Token::RBRACE) Expect(Token::COMMA, CHECK_OK);
 
@@ -3193,6 +3319,8 @@ Expression* Parser::ParseObjectLiteral(bool* ok) {
 
     // Count CONSTANT or COMPUTED properties to maintain the enumeration order.
     if (IsBoilerplateProperty(property)) number_of_boilerplate_properties++;
+    // Validate the property
+    checker.CheckProperty(property, loc, CHECK_OK);
     properties->Add(property);
 
     // TODO(1240767): Consider allowing trailing comma.
@@ -3204,6 +3332,7 @@ Expression* Parser::ParseObjectLiteral(bool* ok) {
     }
   }
   Expect(Token::RBRACE, CHECK_OK);
+
   // Computation of literal_index must happen before pre parse bailout.
   int literal_index = temp_scope_->NextMaterializedLiteralIndex();