Implement DefineOwnProperty for TypedArrays

src/objects.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/objects.h"
 
 #include <cmath>
 #include <iomanip>
 #include <memory>
 #include <sstream>
@@ skipping 6524 matching lines @@
                                           PropertyDescriptor* desc,
                                           ShouldThrow should_throw) {
   if (object->IsJSArray()) {
     return JSArray::DefineOwnProperty(isolate, Handle<JSArray>::cast(object),
                                       key, desc, should_throw);
   }
   if (object->IsJSProxy()) {
     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);
 }
 
 
 // static
 Maybe<bool> JSReceiver::OrdinaryDefineOwnProperty(Isolate* isolate,
                                                   Handle<JSObject> object,
                                                   Handle<Object> key,
                                                   PropertyDescriptor* desc,
@@ skipping 10866 matching lines @@
     }
     while (holes < limit) {
       elements->set_the_hole(holes);
       holes++;
     }
   }
 
   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()) {
 #define INSTANCE_TYPE_TO_ARRAY_TYPE(Type, type, TYPE, ctype, size)            \
     case FIXED_##TYPE##_ARRAY_TYPE:                                           \
       return kExternal##Type##Array;
 
     TYPED_ARRAYS(INSTANCE_TYPE_TO_ARRAY_TYPE)
 #undef INSTANCE_TYPE_TO_ARRAY_TYPE
 
@@ skipping 2766 matching lines @@
         ns, Accessors::ModuleNamespaceEntryInfo(isolate, name, attr))
         .Check();
   }
   JSObject::PreventExtensions(ns, THROW_ON_ERROR).ToChecked();
 
   return ns;
 }
 
 }  // namespace internal
 }  // namespace v8