Implement DefineOwnProperty for TypedArrays

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index f852ca0de39dee015d82e03b5f7f326910688c56..f5f58ff015b58ecfe25e698907c92da9bedf4473 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -6542,12 +6542,15 @@ Maybe<bool> JSReceiver::DefineOwnProperty(Isolate* isolate,
     return JSProxy::DefineOwnProperty(isolate, Handle<JSProxy>::cast(object),
                                       key, desc, should_throw);
   }
+  if (object->IsJSTypedArray()) {
+    return JSTypedArray::DefineOwnProperty(isolate,
+                                           Handle<JSTypedArray>::cast(object),
+                                           key, desc, should_throw);
+  }
   // TODO(neis): Special case for JSModuleNamespace?
 
   // OrdinaryDefineOwnProperty, by virtue of calling
   // DefineOwnPropertyIgnoreAttributes, can handle arguments (ES6 9.4.4.2)
-  // and IntegerIndexedExotics (ES6 9.4.5.3), with one exception:
-  // TODO(jkummerow): Setting an indexed accessor on a typed array should throw.
   return OrdinaryDefineOwnProperty(isolate, Handle<JSObject>::cast(object), key,
                                    desc, should_throw);
 }
@@ -17434,6 +17437,88 @@ Handle<Object> JSObject::PrepareElementsForSort(Handle<JSObject> object,
   return isolate->factory()->NewNumberFromUint(result);
 }
 
+static Handle<Object> CanonicalNumericIndexString(Isolate* isolate,

[Camillo Bruni, 2016/11/15 13:07:22]

nit: use an anonymous namespace for this helper

You probably want to convert this into something similar to
PropertyKeyToArrayIndex, directly returning a converted value and a boolean for
success.
This would avoid accessing undefined and the IsUndefined check further down.

Looking at the spec, I see that all the users of CanonicalNumericIndexString do
an integer conversion later-on, so you could directly fold this into here. And
return false if the input cannot be converted to a valid index.

+                                                  Handle<Object> s) {
+  DCHECK(s->IsString());
+  Handle<Object> index = String::ToNumber(Handle<String>::cast(s));
+  if (index->IsMinusZero()) return index;
+  // Avoid treating strings like "2E1" and "20" as the same key
+  if (!Object::ToString(isolate, index).ToHandleChecked()->SameValue(*s))
+    return isolate->factory()->undefined_value();

[Camillo Bruni, 2016/11/15 13:07:22]

nit: always use braces for if-bodies on separate lines.

+  return index;
+}
+
+// ES6 9.4.5.3

[Dan Ehrenberg, 2016/11/15 11:28:16]

Nit: New references to the spec should be identified by the id, e.g. here
ES#sec-integer-indexed-exotic-objects-defineownproperty-p-desc so that it's
stable across versions.

+// static
+Maybe<bool> JSTypedArray::DefineOwnProperty(Isolate* isolate,
+                                            Handle<JSTypedArray> o,
+                                            Handle<Object> key,
+                                            PropertyDescriptor* desc,
+                                            ShouldThrow should_throw) {
+  // 1. Assert: IsPropertyKey(P) is true.
+  DCHECK(key->IsName() || key->IsNumber());
+  // 2. Assert: O is an Object that has a [[ViewedArrayBuffer]] internal slot.
+  // 3. If Type(P) is String, then
+  // ToPropertyKey returns Smi's too
+  if (key->IsString() || key->IsSmi()) {
+    // 3a. Let numericIndex be ! CanonicalNumericIndexString(P)
+    Handle<Object> numeric_index =
+        key->IsSmi() ? key
+                     : CanonicalNumericIndexString(isolate,
+                                                   Handle<String>::cast(key));
+    // 3b. If numericIndex is not undefined, then
+    if (!numeric_index->IsUndefined(isolate)) {
+      // 3b i. If IsInteger(numericIndex) is false, return false.
+      // 3b ii. If numericIndex = -0, return false.
+      // 3b iii. If numericIndex < 0, return false.
+      int index = numeric_index->Number();
+      // FIXME: the standard allows up to 2^53 elements
+      if (!IsUint32Double(index))

[Camillo Bruni, 2016/11/15 13:07:22]

You may want to shuffle around the steps a bit here to make the code a bit more
streamlined (see comment above for CanonicalNumericIndexString).

+        RETURN_FAILURE(isolate, should_throw,
+                       NewTypeError(MessageTemplate::kInvalidTypedArrayIndex));
+      // 3b iv. Let length be O.[[ArrayLength]].
+      int length = o->length()->Number();

[Camillo Bruni, 2016/11/15 13:07:23]

I'd feel safer by using a uint32_t here.

+      // 3b v. If numericIndex ≥ length, return false.
+      if (index >= length)
+        RETURN_FAILURE(isolate, should_throw,
+                       NewTypeError(MessageTemplate::kInvalidTypedArrayIndex));

[Camillo Bruni, 2016/11/15 13:07:23]

nit: missing braces

+      // 3b vi. If IsAccessorDescriptor(Desc) is true, return false.
+      if (PropertyDescriptor::IsAccessorDescriptor(desc))
+        RETURN_FAILURE(isolate, should_throw,
+                       NewTypeError(MessageTemplate::kRedefineDisallowed, key));

[Camillo Bruni, 2016/11/15 13:07:22]

nit: missing braces

+      // 3b vii. If Desc has a [[Configurable]] field and if
+      // Desc.[[Configurable]] is true, return false.
+      // 3b viii. If Desc has an [[Enumerable]] field and if Desc.[[Enumerable]]
+      // is false, return false.
+      // 3b ix. If Desc has a [[Writable]] field and if Desc.[[Writable]] is
+      // false, return false.
+      if ((desc->has_configurable() && desc->configurable()) ||
+          (desc->has_enumerable() && !desc->enumerable()) ||
+          (desc->has_writable() && !desc->writable()))
+        RETURN_FAILURE(isolate, should_throw,
+                       NewTypeError(MessageTemplate::kRedefineDisallowed, key));

[Camillo Bruni, 2016/11/15 13:07:22]

nit: missing braces

+      // 3b x. If Desc has a [[Value]] field, then
+        // 3b x 1. Let value be Desc.[[Value]].
+        // 3b x 2. Return ? IntegerIndexedElementSet(O, numericIndex, value).
+      if (desc->has_value()) {
+        if (!desc->has_configurable()) desc->set_configurable(false);
+        if (!desc->has_enumerable()) desc->set_enumerable(true);
+        if (!desc->has_writable()) desc->set_writable(true);
+        Handle<Object> value = desc->value();
+        RETURN_ON_EXCEPTION_VALUE(
+            isolate,
+            SetOwnElementIgnoreAttributes(o, index, value,
+                                          desc->ToAttributes()),
+            Nothing<bool>());
+      }
+      // 3b xi. Return true.
+      return Just(true);
+    }
+  }
+  // 4. Return ! OrdinaryDefineOwnProperty(O, P, Desc).
+  return OrdinaryDefineOwnProperty(isolate, o, key, desc, should_throw);
+}
+
 
 ExternalArrayType JSTypedArray::type() {
   switch (elements()->map()->instance_type()) {