super.property store

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 1f4881037bdd80375a9a503981714dedc880a72a..01a23424f300f16a10256537cab590f3fda164a2 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -291,7 +291,7 @@ MaybeHandle<Object> Object::GetPropertyWithAccessor(Handle<Object> receiver,
   if (structure->IsAccessorInfo()) {
     Handle<AccessorInfo> info = Handle<AccessorInfo>::cast(structure);
     if (!info->IsCompatibleReceiver(*receiver)) {
-      Handle<Object> args[2] = { name, receiver };
+      Handle<Object> args[] = {name, receiver};
       THROW_NEW_ERROR(isolate,
                       NewTypeError("incompatible_method_receiver",
                                    HandleVector(args, arraysize(args))),
@@ -356,7 +356,7 @@ MaybeHandle<Object> Object::SetPropertyWithAccessor(
     // api style callbacks
     ExecutableAccessorInfo* info = ExecutableAccessorInfo::cast(*structure);
     if (!info->IsCompatibleReceiver(*receiver)) {
-      Handle<Object> args[2] = { name, receiver };
+      Handle<Object> args[] = {name, receiver};
       THROW_NEW_ERROR(isolate,
                       NewTypeError("incompatible_method_receiver",
                                    HandleVector(args, arraysize(args))),
@@ -383,10 +383,11 @@ MaybeHandle<Object> Object::SetPropertyWithAccessor(
           receiver, Handle<JSReceiver>::cast(setter), value);
     } else {
       if (is_sloppy(language_mode)) return value;
-      Handle<Object> args[2] = { name, holder };
-      THROW_NEW_ERROR(
-          isolate, NewTypeError("no_setter_in_callback", HandleVector(args, 2)),
-          Object);
+      Handle<Object> args[] = {name, holder};
+      THROW_NEW_ERROR(isolate,
+                      NewTypeError("no_setter_in_callback",
+                                   HandleVector(args, arraysize(args))),
+                      Object);
     }
   }
 
@@ -755,13 +756,12 @@ MaybeHandle<Object> Object::SetElementWithReceiver(
   Handle<JSObject> target = Handle<JSObject>::cast(receiver);
   ElementsAccessor* accessor = target->GetElementsAccessor();
   PropertyAttributes attrs = accessor->GetAttributes(receiver, target, index);
-  if ((attrs & READ_ONLY) != 0) {
-    return WriteToReadOnlyElement(isolate, receiver, index, value,
-                                  language_mode);
+  if (attrs == ABSENT) {
+    return JSObject::SetElement(target, index, value, NONE, language_mode,
+                                false);
   }
-  PropertyAttributes new_attrs = attrs != ABSENT ? attrs : NONE;
-  return JSObject::SetElement(target, index, value, new_attrs, language_mode,
-                              false);
+  return JSObject::SetElement(target, index, value, attrs, language_mode, false,
+                              DEFINE_PROPERTY);
 }
 
 
@@ -3137,18 +3137,46 @@ MaybeHandle<Object> Object::SetProperty(LookupIterator* it,
   // the property did not exist yet on the global object itself, we have to
   // throw a reference error in strict mode.
   if (it->GetReceiver()->IsJSGlobalObject() && is_strict(language_mode)) {
-    Handle<Object> args[1] = {it->name()};
-    THROW_NEW_ERROR(it->isolate(),
-                    NewReferenceError("not_defined", HandleVector(args, 1)),
-                    Object);
+    Handle<Object> args[] = {it->name()};
+    THROW_NEW_ERROR(
+        it->isolate(),
+        NewReferenceError("not_defined", HandleVector(args, arraysize(args))),
+        Object);
   }
 
+  // super.property = value
+  // No property was found starting from the prototype of [[HomeObject]].
   if (data_store_mode == SUPER_PROPERTY) {

[Toon Verwaest, 2015/02/18 13:54:08]

Given that this flag just switches half of the implementation of this method,
what about removing the flag and instead extracting the code above into a
separate helper function used by both SetProperty and SetSuperProperty?

The code below is a bit scary. Can you please replace it with switch
(it->state()) { ... } without "default:" to ensure all possible LookupIterator
states are handled? That would make it obvious what e.g., has to happen in the
ACCESS_CHECK case.

     LookupIterator own_lookup(it->GetReceiver(), it->name(),
                               LookupIterator::OWN);
+    if (!own_lookup.IsFound()) {
+      return AddDataProperty(&own_lookup, value, NONE, language_mode,
+                             store_mode);
+    }
+
+    PropertyDetails details = own_lookup.property_details();
 
-    return JSObject::SetProperty(&own_lookup, value, language_mode, store_mode,
-                                 NORMAL_PROPERTY);
+    // Otherwise reconfigure property with descriptor {value: value}.
+    if (details.IsConfigurable()) {
+      return JSObject::SetOwnPropertyIgnoreAttributes(
+          Handle<JSObject>::cast(it->GetReceiver()), it->name(), value,
+          details.attributes());
+    }
+
+    if (details.IsReadOnly()) {
+      return WriteToReadOnlyProperty(&own_lookup, value, language_mode);
+    }
+
+    if (details.kind() == kAccessor) {
+      if (is_sloppy(language_mode)) return value;
+      Handle<Object> args[] = {it->name()};
+      THROW_NEW_ERROR(it->isolate(),
+                      NewTypeError("redefine_disallowed",

[arv (Not doing code reviews), 2015/02/18 01:00:04]

Maybe I should just use strict_read_only_property here as well?

[adamk, 2015/02/18 01:26:05]

Or just make a new RedefineNonconfigurableProperty() method and factor the logic
out into that?

[arv (Not doing code reviews), 2015/02/18 17:46:14]

Done.

+                                   HandleVector(args, arraysize(args))),
+                      Object);
+    }
+
+    return SetDataProperty(&own_lookup, value);
   }
 
   return AddDataProperty(it, value, NONE, language_mode, store_mode);
@@ -3157,12 +3185,18 @@ MaybeHandle<Object> Object::SetProperty(LookupIterator* it,
 
 MaybeHandle<Object> Object::WriteToReadOnlyProperty(
     LookupIterator* it, Handle<Object> value, LanguageMode language_mode) {
-  if (is_sloppy(language_mode)) return value;
+  return WriteToReadOnlyProperty(it->isolate(), it->GetReceiver(), it->name(),
+                                 value, language_mode);
+}
+
 
-  Handle<Object> args[] = {it->name(), it->GetReceiver()};
-  THROW_NEW_ERROR(it->isolate(),
-                  NewTypeError("strict_read_only_property",
-                               HandleVector(args, arraysize(args))),
+MaybeHandle<Object> Object::WriteToReadOnlyProperty(
+    Isolate* isolate, Handle<Object> receiver, Handle<Object> name,
+    Handle<Object> value, LanguageMode language_mode) {
+  if (is_sloppy(language_mode)) return value;
+  Handle<Object> args[] = {name, receiver};
+  THROW_NEW_ERROR(isolate, NewTypeError("strict_read_only_property",
+                                        HandleVector(args, arraysize(args))),
                   Object);
 }
 
@@ -3172,13 +3206,9 @@ MaybeHandle<Object> Object::WriteToReadOnlyElement(Isolate* isolate,
                                                    uint32_t index,
                                                    Handle<Object> value,
                                                    LanguageMode language_mode) {
-  if (is_sloppy(language_mode)) return value;
-
-  Handle<Object> args[] = {isolate->factory()->NewNumberFromUint(index),
-                           receiver};
-  THROW_NEW_ERROR(isolate, NewTypeError("strict_read_only_property",
-                                        HandleVector(args, arraysize(args))),
-                  Object);
+  return WriteToReadOnlyProperty(isolate, receiver,
+                                 isolate->factory()->NewNumberFromUint(index),
+                                 value, language_mode);
 }
 
 
@@ -3245,7 +3275,7 @@ MaybeHandle<Object> Object::AddDataProperty(LookupIterator* it,
   if (it->state() != LookupIterator::TRANSITION) {
     if (is_sloppy(language_mode)) return value;
 
-    Handle<Object> args[1] = {it->name()};
+    Handle<Object> args[] = {it->name()};
     THROW_NEW_ERROR(it->isolate(),
                     NewTypeError("object_not_extensible",
                                  HandleVector(args, arraysize(args))),
@@ -3791,11 +3821,8 @@ MaybeHandle<Object> JSProxy::SetPropertyViaPrototypesWithHandler(
     DCHECK(writable->IsBoolean());
     *done = writable->IsFalse();
     if (!*done) return isolate->factory()->the_hole_value();
-    if (is_sloppy(language_mode)) return value;
-    Handle<Object> args[] = { name, receiver };
-    THROW_NEW_ERROR(isolate, NewTypeError("strict_read_only_property",
-                                          HandleVector(args, arraysize(args))),
-                    Object);
+    return WriteToReadOnlyProperty(isolate, receiver, name, value,
+                                   language_mode);
   }
 
   // We have an AccessorDescriptor.
@@ -5154,9 +5181,10 @@ MaybeHandle<Object> JSObject::DeleteElement(Handle<JSObject> object,
     if (is_strict(language_mode)) {
       // Deleting a non-configurable property in strict mode.
       Handle<Object> name = factory->NewNumberFromUint(index);
-      Handle<Object> args[2] = { name, object };
-      THROW_NEW_ERROR(isolate, NewTypeError("strict_delete_property",
-                                            HandleVector(args, 2)),
+      Handle<Object> args[] = {name, object};
+      THROW_NEW_ERROR(isolate,
+                      NewTypeError("strict_delete_property",
+                                   HandleVector(args, arraysize(args))),
                       Object);
     }
     return factory->false_value();
@@ -5287,7 +5315,7 @@ MaybeHandle<Object> JSObject::DeleteProperty(Handle<JSObject> object,
         if (!it.IsConfigurable()) {
           // Fail if the property is not configurable.
           if (is_strict(language_mode)) {
-            Handle<Object> args[2] = {name, object};
+            Handle<Object> args[] = {name, object};
             THROW_NEW_ERROR(it.isolate(),
                             NewTypeError("strict_delete_property",
                                          HandleVector(args, arraysize(args))),
@@ -12645,10 +12673,11 @@ MaybeHandle<Object> JSObject::SetElementWithCallback(
     } else {
       if (is_sloppy(language_mode)) return value;
       Handle<Object> key(isolate->factory()->NewNumberFromUint(index));
-      Handle<Object> args[2] = { key, holder };
-      THROW_NEW_ERROR(
-          isolate, NewTypeError("no_setter_in_callback", HandleVector(args, 2)),
-          Object);
+      Handle<Object> args[] = {key, holder};
+      THROW_NEW_ERROR(isolate,
+                      NewTypeError("no_setter_in_callback",
+                                   HandleVector(args, arraysize(args))),
+                      Object);
     }
   }
 
@@ -12835,24 +12864,32 @@ MaybeHandle<Object> JSObject::SetDictionaryElement(
     if (details.type() == ACCESSOR_CONSTANT && set_mode == SET_PROPERTY) {
       return SetElementWithCallback(object, element, index, value, object,
                                     language_mode);
+    } else if (set_mode == DEFINE_PROPERTY && !details.IsConfigurable() &&
+               details.kind() == kAccessor) {
+      if (is_sloppy(language_mode)) {
+        return isolate->factory()->undefined_value();
+      }
+      Handle<Object> number = isolate->factory()->NewNumberFromUint(index);
+      Handle<Object> args[] = {number};
+      THROW_NEW_ERROR(isolate,
+                      NewTypeError("redefine_disallowed",
+                                   HandleVector(args, arraysize(args))),
+                      Object);
+
+    } else if ((set_mode == DEFINE_PROPERTY && !details.IsConfigurable() &&
+                details.IsReadOnly() && details.kind() == kData) ||

[adamk, 2015/02/18 01:26:05]

I take it we don't need this !IsTheHole() check in the DEFINE_PROPERTY case
simply because we don't exercise that code path?

[Toon Verwaest, 2015/02/18 13:54:08]

details.kind() == kData isn't necessary since DEFINE + !Configurable + Accessor
is already handled above.

[Toon Verwaest, 2015/02/18 13:54:08]

We could add a DCHECK(element->IsTheHole()) inside the branch...

[arv (Not doing code reviews), 2015/02/18 17:46:14]

Done.

[arv (Not doing code reviews), 2015/02/18 17:46:14]

Done.

+               (set_mode == SET_PROPERTY && details.IsReadOnly() &&
+                !element->IsTheHole())) {
+      // If a value has not been initialized we allow writing to it even if it
+      // is read-only (a declared const that has not been initialized).
+      return WriteToReadOnlyProperty(
+          isolate, object, isolate->factory()->NewNumberFromUint(index),
+          isolate->factory()->undefined_value(), language_mode);
     } else {
       dictionary->UpdateMaxNumberKey(index);
-      // If a value has not been initialized we allow writing to it even if it
-      // is read-only (a declared const that has not been initialized).  If a
-      // value is being defined we skip attribute checks completely.
       if (set_mode == DEFINE_PROPERTY) {
         details = PropertyDetails(attributes, DATA, details.dictionary_index());
         dictionary->DetailsAtPut(entry, details);
-      } else if (details.IsReadOnly() && !element->IsTheHole()) {
-        if (is_sloppy(language_mode)) {
-          return isolate->factory()->undefined_value();
-        } else {
-          Handle<Object> number = isolate->factory()->NewNumberFromUint(index);
-          Handle<Object> args[2] = { number, object };
-          THROW_NEW_ERROR(isolate, NewTypeError("strict_read_only_property",
-                                                HandleVector(args, 2)),
-                          Object);
-        }
       }
       // Elements of the arguments object in slow mode might be slow aliases.
       if (is_arguments && element->IsAliasedArgumentsEntry()) {
@@ -12885,9 +12922,10 @@ MaybeHandle<Object> JSObject::SetDictionaryElement(
       } else {
         Handle<Object> number = isolate->factory()->NewNumberFromUint(index);
         Handle<String> name = isolate->factory()->NumberToString(number);
-        Handle<Object> args[1] = { name };
-        THROW_NEW_ERROR(isolate, NewTypeError("object_not_extensible",
-                                              HandleVector(args, 1)),
+        Handle<Object> args[] = {name};
+        THROW_NEW_ERROR(isolate,
+                        NewTypeError("object_not_extensible",
+                                     HandleVector(args, arraysize(args))),
                         Object);
       }
     }
@@ -13597,7 +13635,7 @@ bool JSArray::WouldChangeReadOnlyLength(Handle<JSArray> array,
 MaybeHandle<Object> JSArray::ReadOnlyLengthError(Handle<JSArray> array) {
   Isolate* isolate = array->GetIsolate();
   Handle<Name> length = isolate->factory()->length_string();
-  Handle<Object> args[2] = { length, array };
+  Handle<Object> args[] = {length, array};
   THROW_NEW_ERROR(isolate, NewTypeError("strict_read_only_property",
                                         HandleVector(args, arraysize(args))),
                   Object);