Implement DefineOwnProperty for TypedArrays

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index f852ca0de39dee015d82e03b5f7f326910688c56..3ed69ba44b97a7f53f1bdcf9c558fe623310c99f 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,82 @@ Handle<Object> JSObject::PrepareElementsForSort(Handle<JSObject> object,
   return isolate->factory()->NewNumberFromUint(result);
 }
 
+static bool IsInteger(Handle<Object> number) {
+  if (!number->IsNumber() || number->IsNaN() ||
+      !std::isfinite(number->Number()))
+      return false;
+  double d = number->Number();
+  if (std::floor(std::abs(d)) != std::abs(d)) return false;

[caitp, 2016/10/20 15:39:21]

Why the std::floor() and std::abs() and stuff? I think we can get away with
something like this:

```
if (!number->IsNumber()) return false;

double index = number->Value();

return std::isfinite(index) && FastD2I(index) == index;
```

And, you could also get the `-0` and negative number checks for free by adding a
`&& !std::signbit(index)` to that line at the end.

+  return true;
+}
+
+static Handle<Object> CanonicalNumericIndexString(Isolate* isolate,
+                                                  Handle<Object> s) {
+  DCHECK(s->IsString());
+  Handle<Object> index = String::ToNumber(Handle<String>::cast(s));
+  if (index->IsMinusZero()) return index;
+  if (!Object::ToString(isolate, index).ToHandleChecked()->SameValue(*s))

[caitp, 2016/10/20 15:39:21]

I would leave a comment saying that we need this to avoid treating strings like
"2E1" and "20" as the same key, so that it doesn't look like we're just
implementing the spec verbatim "just because".

I wonder if there's a faster way to implement this, though. If the string
contains an non-digit characters, ToString() should always return a different
value AFAIK (except in the case of NaN, which would return false anyways?)

I'm not sure of all of the situations we'd have to account for, so I'm fine with
leaving it as is (with the comment).

+    return isolate->factory()->undefined_value();
+  return index;
+}
+
+// ES6 9.4.5.3
+// 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
+  if (key->IsString()) {
+    // 3a. Let numericIndex be ! CanonicalNumericIndexString(P)
+    Handle<Object> numeric_index =
+      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.
+      if (!IsInteger(numeric_index) || numeric_index->IsMinusZero() ||
+          numeric_index->Number() < 0)
+        return Just(false);
+      // 3b iv. Let length be O.[[ArrayLength]].
+      int length = o->length()->Number();
+      // 3b v. If numericIndex ≥ length, return false.
+      if (numeric_index->Number() >= length) return Just(false);
+      // 3b vi. If IsAccessorDescriptor(Desc) is true, return false.
+      if (PropertyDescriptor::IsAccessorDescriptor(desc)) return Just(false);
+      // 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 Just(false);
+      // 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).
+        // Performed by OrdinaryDefineOwnProperty below.
+        // OrdinaryDefineOwnProperty, by virtue of calling
+        // DefineOwnPropertyIgnoreAttributes, can handle IntegerIndexedExotics,
+        // with one exception:
+        // TODO(jkummerow): Setting an indexed accessor on a typed array should
+        // throw.
+      // 3b xi. Return true.
+      if (!desc->has_value()) {
+        return Just(true);
+      }

[caitp, 2016/10/20 15:39:21]

It looks like we're missing the Integer IntegerIndexedElementSet case. Some of
the validation steps in that algorithm can be skipped here because they're
already checked above (eg index < length, index != -0, IsInteger(index), etc).

Do you need help with this part?

+    }
+  }
+  // 4. Return ! OrdinaryDefineOwnProperty(O, P, Desc).
+  return OrdinaryDefineOwnProperty(isolate, o, key, desc, should_throw);

[caitp, 2016/10/20 15:39:21]

The ToPropertyKey implementation in v8 can return Smi numbers, but per spec
those should be converted to strings.

You should add a path which handles the Smi case (which you only need to
validate by testing if it's negative or within the range of the typed array).

+}
+
 
 ExternalArrayType JSTypedArray::type() {
   switch (elements()->map()->instance_type()) {