Align %TypedArray% behavior with recent spec adjustments

https://bugs.webkit.org/show_bug.cgi?id=218776 Reviewed by Yusuke Suzuki. JSTests: * stress/reflect-set.js: * stress/typedarray-functions-with-neutered.js: * stress/typedarray-includes.js: * stress/typedarray-indexOf.js: * stress/typedarray-join.js: Added. * stress/typedarray-lastIndexOf.js: Update tests. * test262/expectations.yaml: Mark a handful of test cases as temporarily failing. These will disappear in a future test262 update. Source/JavaScriptCore: The recent spec changes for typed arrays with detached buffers had certain ripple effects, namely the following two PRs which will be presented in next week's TC39 meeting. Since no controversy is expected, this patch addresses them now, though test262 adjustments are forthcoming. 1. tc39/ecma262#2210 It is correct that `ta[i] = n` doesn't throw when `ta` has a detached buffer or `i` is otherwise OOB, but by not throwing, Reflect.set(ta, i, n) is obliged to return true. 2. tc39/ecma262#2221 Until now, %TypedArray%.prototype.{includes, indexOf, join, lastIndexOf} lacked a rigorous specification; in particular, each has a parameter that may detach the buffer upon valueOf or toString, and the expected behavior was not made clear. It seems most sensible to do what the corresponding Array methods do upon `array.length = 0`: make use of the cached length but don't access indices, such that indexOf/lastIndexOf return -1 while includes/join act as if the elements were all `undefined`. * runtime/JSGenericTypedArrayViewInlines.h: (JSC::JSGenericTypedArrayView<Adaptor>::put): (JSC::JSGenericTypedArrayView<Adaptor>::defineOwnProperty): (JSC::JSGenericTypedArrayView<Adaptor>::putByIndex): * runtime/JSGenericTypedArrayViewPrototypeFunctions.h: (JSC::genericTypedArrayViewProtoFuncIncludes): (JSC::genericTypedArrayViewProtoFuncIndexOf): (JSC::genericTypedArrayViewProtoFuncJoin): (JSC::genericTypedArrayViewProtoFuncLastIndexOf): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@269670 268f45cc-cd09-0410-ab3c-d52691b4dbfc
caiolima · Nov 11, 2020 · 5d29948 · 5d29948
1 parent 1a7def2
commit 5d29948
Show file tree

Hide file tree

Showing 11 changed files with 132 additions and 28 deletions.
diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
@@ -1,3 +1,22 @@
+2020-11-10  Ross Kirsling  <[email protected]>
+
+        Align %TypedArray% behavior with recent spec adjustments
+        https://bugs.webkit.org/show_bug.cgi?id=218776
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/reflect-set.js:
+        * stress/typedarray-functions-with-neutered.js:
+        * stress/typedarray-includes.js:
+        * stress/typedarray-indexOf.js:
+        * stress/typedarray-join.js: Added.
+        * stress/typedarray-lastIndexOf.js:
+        Update tests.
+
+        * test262/expectations.yaml:
+        Mark a handful of test cases as temporarily failing.
+        These will disappear in a future test262 update.
+
 2020-11-10  Michael Catanzaro  <[email protected]>
 
         stress/intl-datetimeformat-formatrange.js and stress/intl-datetimeformat-formatrange-relevant-extensions.js fail with ICU 65.1

diff --git a/JSTests/stress/reflect-set.js b/JSTests/stress/reflect-set.js
@@ -691,6 +691,11 @@ var symbol = Symbol();
         shouldBe(Reflect.set(object, 0, 42, receiver), true);
         shouldBe(Reflect.get(object, 0), 42);
         shouldBe(receiver.hasOwnProperty(0), false);
+
+        var object = new TypedArray(1);
+        transferArrayBuffer(object.buffer);
+        shouldBe(Reflect.set(object, 0, 42), true);
+        shouldBe(Reflect.get(object, 0), undefined);
     };
 })());
 

diff --git a/JSTests/stress/typedarray-functions-with-neutered.js b/JSTests/stress/typedarray-functions-with-neutered.js
@@ -78,10 +78,6 @@ for (var i = 0; i < 1000; i++)
 prototypeFunctions = [
     { func:proto.copyWithin, args:["prim", "prim", "prim"] },
     { func:proto.fill, args:["prim", "prim", "prim"] },
-    { func:proto.indexOf, args:["na", "prim"] },
-    { func:proto.includes, args:["na", "prim"] },
-    { func:proto.join, args:["prim"] },
-    { func:proto.lastIndexOf, args:["na", "prim"] },
     { func:proto.set, args:["array", "prim"] },
     { func:proto.slice, args:["prim", "prim"] },
     { func:proto.sort, args:["func"] },

diff --git a/JSTests/stress/typedarray-includes.js b/JSTests/stress/typedarray-includes.js
@@ -1,10 +1,16 @@
 load("resources/typedarray-test-helper-functions.js", "caller relative");
 
 for (constructor of typedArrays) {
-    let a = new constructor(10);
+    a = new constructor(10);
     passed = true;
     result = a.includes({ valueOf() { passed = false; return 1; } });
     shouldBeTrue("passed");
     shouldBeFalse("result");
+
+    shouldBeTrue("a.includes(undefined, { valueOf() { transferArrayBuffer(a.buffer); return 0; } })");
+    shouldThrow("a.includes(undefined)");
+
+    a = new constructor(1);
+    shouldBeFalse("a.includes(null, { valueOf() { transferArrayBuffer(a.buffer); return 0; } })");
 }
 finishJSTest();
diff --git a/JSTests/stress/typedarray-indexOf.js b/JSTests/stress/typedarray-indexOf.js
@@ -44,6 +44,9 @@ for (constructor of typedArrays) {
     shouldBe("a.indexOf(undefined)", "-1");
     shouldBe("a.indexOf({1: ''})", "-1");
     shouldBe("a.indexOf(\"\")", "-1");
+
+    shouldBe("a.indexOf(undefined, { valueOf() { transferArrayBuffer(a.buffer); return 0; } })", "-1");
+    shouldThrow("a.indexOf(undefined)");
 }
 
 

diff --git a/JSTests/stress/typedarray-join.js b/JSTests/stress/typedarray-join.js
@@ -0,0 +1,33 @@
+const typedArrays = [Int8Array, Uint8Array, Uint8ClampedArray, Int16Array, Uint16Array, Int32Array, Uint32Array, Float32Array, Float64Array];
+
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error(`expected ${expected} but got ${actual}`);
+}
+
+function shouldThrowTypeError(func) {
+    let error;
+    try {
+        func();
+    } catch (e) {
+        error = e;
+    }
+
+    if (!(error instanceof TypeError))
+        throw new Error('Expected TypeError!');
+}
+
+function test() {
+    for (const constructor of typedArrays) {
+        const ta = new constructor([1,2,3]);
+        shouldBe(ta.join(), '1,2,3');
+        shouldBe(ta.join(','), '1,2,3');
+        shouldBe(ta.join('--'), '1--2--3');
+
+        shouldBe(ta.join({ toString() { transferArrayBuffer(ta.buffer); return ','; } }), ',,');
+        shouldThrowTypeError(() => ta.join());
+    }
+}
+
+for (let i = 0; i < 1e3; i++)
+    test();
diff --git a/JSTests/stress/typedarray-lastIndexOf.js b/JSTests/stress/typedarray-lastIndexOf.js
@@ -41,6 +41,9 @@ for (constructor of typedArrays) {
     shouldBe("a.lastIndexOf(undefined)", "-1");
     shouldBe("a.lastIndexOf({1: ''})", "-1");
     shouldBe("a.lastIndexOf(\"\")", "-1");
+
+    shouldBe("a.lastIndexOf(undefined, { valueOf() { transferArrayBuffer(a.buffer); return 0; } })", "-1");
+    shouldThrow("a.lastIndexOf(undefined)");
 }
 
 

diff --git a/JSTests/test262/expectations.yaml b/JSTests/test262/expectations.yaml
@@ -838,6 +838,9 @@ test/built-ins/TypedArray/prototype/findIndex/predicate-may-detach-buffer.js:
 test/built-ins/TypedArray/prototype/forEach/callbackfn-detachbuffer.js:
   default: 'Test262Error: Expected a TypeError but got a Test262Error (Testing with Float64Array.)'
   strict mode: 'Test262Error: Expected a TypeError but got a Test262Error (Testing with Float64Array.)'
+test/built-ins/TypedArray/prototype/includes/detached-buffer-tointeger.js:
+  default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all (Testing with Float64Array.)'
+  strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all (Testing with Float64Array.)'
 test/built-ins/TypedArray/prototype/map/callbackfn-detachbuffer.js:
   default: 'Test262Error: Expected a TypeError but got a Test262Error (Testing with Float64Array.)'
   strict mode: 'Test262Error: Expected a TypeError but got a Test262Error (Testing with Float64Array.)'
@@ -934,9 +937,18 @@ test/built-ins/TypedArrayConstructors/internals/Set/detached-buffer-realm.js:
 test/built-ins/TypedArrayConstructors/internals/Set/detached-buffer.js:
   default: 'Test262Error: `sample[0] = 1` is false Expected SameValue(«1», «false») to be true (Testing with Float64Array.)'
   strict mode: 'Test262Error: `sample[0] = 1` is false Expected SameValue(«1», «false») to be true (Testing with Float64Array.)'
+test/built-ins/TypedArrayConstructors/internals/Set/key-is-minus-zero.js:
+  default: 'Test262Error: Reflect.set(sample, "-0", 1) must return false Expected SameValue(«true», «false») to be true (Testing with Float64Array.)'
+  strict mode: 'Test262Error: Reflect.set(sample, "-0", 1) must return false Expected SameValue(«true», «false») to be true (Testing with Float64Array.)'
+test/built-ins/TypedArrayConstructors/internals/Set/key-is-not-integer.js:
+  default: 'Test262Error: Reflect.set(sample, "1.1", 1) must return false Expected SameValue(«true», «false») to be true (Testing with Float64Array.)'
+  strict mode: 'Test262Error: Reflect.set(sample, "1.1", 1) must return false Expected SameValue(«true», «false») to be true (Testing with Float64Array.)'
+test/built-ins/TypedArrayConstructors/internals/Set/key-is-out-of-bounds.js:
+  default: 'Test262Error: Reflect.set(sample, "-1", 1) must return false Expected SameValue(«true», «false») to be true (Testing with Float64Array.)'
+  strict mode: 'Test262Error: Reflect.set(sample, "-1", 1) must return false Expected SameValue(«true», «false») to be true (Testing with Float64Array.)'
 test/built-ins/TypedArrayConstructors/internals/Set/tonumber-value-detached-buffer.js:
-  default: 'Test262Error: Expected SameValue(«undefined», «false») to be true (Testing with Float64Array.)'
-  strict mode: 'Test262Error: Expected SameValue(«undefined», «false») to be true (Testing with Float64Array.)'
+  default: 'Test262Error: Expected SameValue(«true», «false») to be true (Testing with Float64Array.)'
+  strict mode: 'Test262Error: Expected SameValue(«true», «false») to be true (Testing with Float64Array.)'
 test/intl402/DateTimeFormat/prototype/formatRange/en-US.js:
   default: 'Test262Error: Expected SameValue(«1/3/2019 – 1/5/2019», «1/3/2019 – 1/5/2019») to be true'
   strict mode: 'Test262Error: Expected SameValue(«1/3/2019 – 1/5/2019», «1/3/2019 – 1/5/2019») to be true'
@@ -2538,12 +2550,6 @@ test/language/statements/class/elements/nested-direct-eval-err-contains-argument
 test/language/statements/class/elements/nested-private-direct-eval-err-contains-arguments.js:
   default: 'Test262Error: Expected a SyntaxError but got a ReferenceError'
   strict mode: 'Test262Error: Expected a SyntaxError but got a ReferenceError'
-test/language/statements/class/elements/privatefieldget-primitive-receiver.js:
-  default: 'Bad exit code: 5'
-  strict mode: 'Bad exit code: 5'
-test/language/statements/class/elements/privatefieldput-primitive-receiver.js:
-  default: 'Bad exit code: 5'
-  strict mode: 'Bad exit code: 5'
 test/language/statements/class/gen-method-static/forbidden-ext/b2/cls-decl-gen-meth-static-forbidden-ext-indirect-access-own-prop-caller-get.js:
   default: 'TypeError: Function.caller used to retrieve generator body'
 test/language/statements/class/gen-method-static/forbidden-ext/b2/cls-decl-gen-meth-static-forbidden-ext-indirect-access-own-prop-caller-value.js:

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,35 @@
+2020-11-10  Ross Kirsling  <[email protected]>
+
+        Align %TypedArray% behavior with recent spec adjustments
+        https://bugs.webkit.org/show_bug.cgi?id=218776
+
+        Reviewed by Yusuke Suzuki.
+
+        The recent spec changes for typed arrays with detached buffers had certain ripple effects,
+        namely the following two PRs which will be presented in next week's TC39 meeting.
+        Since no controversy is expected, this patch addresses them now, though test262 adjustments are forthcoming.
+
+        1. https://github.com/tc39/ecma262/pull/2210
+           It is correct that `ta[i] = n` doesn't throw when `ta` has a detached buffer or `i` is otherwise OOB,
+           but by not throwing, Reflect.set(ta, i, n) is obliged to return true.
+
+        2. https://github.com/tc39/ecma262/pull/2221
+           Until now, %TypedArray%.prototype.{includes, indexOf, join, lastIndexOf} lacked a rigorous specification;
+           in particular, each has a parameter that may detach the buffer upon valueOf or toString, and the expected
+           behavior was not made clear. It seems most sensible to do what the corresponding Array methods do upon
+           `array.length = 0`: make use of the cached length but don't access indices, such that indexOf/lastIndexOf
+           return -1 while includes/join act as if the elements were all `undefined`.
+
+        * runtime/JSGenericTypedArrayViewInlines.h:
+        (JSC::JSGenericTypedArrayView<Adaptor>::put):
+        (JSC::JSGenericTypedArrayView<Adaptor>::defineOwnProperty):
+        (JSC::JSGenericTypedArrayView<Adaptor>::putByIndex):
+        * runtime/JSGenericTypedArrayViewPrototypeFunctions.h:
+        (JSC::genericTypedArrayViewProtoFuncIncludes):
+        (JSC::genericTypedArrayViewProtoFuncIndexOf):
+        (JSC::genericTypedArrayViewProtoFuncJoin):
+        (JSC::genericTypedArrayViewProtoFuncLastIndexOf):
+
 2020-11-10  John Wilander  <[email protected]>
 
         PCM: Change from ad-click-attribution to private-click-measurement (in all forms, including .well-known URL)

diff --git a/Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h b/Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h
@@ -365,7 +365,7 @@ bool JSGenericTypedArrayView<Adaptor>::put(
     if (isCanonicalNumericIndexString(propertyName)) {
         // Cases like '-0', '1.1', etc. are still obliged to give the RHS a chance to throw.
         toNativeFromValue<Adaptor>(globalObject, value);
-        return false;
+        return true;
     }
 
     return Base::put(thisObject, globalObject, propertyName, value, slot);
@@ -403,7 +403,7 @@ bool JSGenericTypedArrayView<Adaptor>::defineOwnProperty(
             return throwTypeErrorIfNeeded("Attempting to store non-writable property on a typed array at index: ");
 
         if (descriptor.value())
-            RELEASE_AND_RETURN(scope, thisObject->putByIndex(thisObject, globalObject, index.value(), descriptor.value(), shouldThrow));
+            RELEASE_AND_RETURN(scope, thisObject->setIndex(globalObject, index.value(), descriptor.value()));
 
         return true;
     }
@@ -449,7 +449,8 @@ bool JSGenericTypedArrayView<Adaptor>::putByIndex(
     JSCell* cell, JSGlobalObject* globalObject, unsigned propertyName, JSValue value, bool)
 {
     JSGenericTypedArrayView* thisObject = jsCast<JSGenericTypedArrayView*>(cell);
-    return thisObject->setIndex(globalObject, propertyName, value);
+    thisObject->setIndex(globalObject, propertyName, value);
+    return true;
 }
 
 template<typename Adaptor>

diff --git a/Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h b/Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h
@@ -198,7 +198,7 @@ ALWAYS_INLINE EncodedJSValue genericTypedArrayViewProtoFuncIncludes(VM& vm, JSGl
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
     if (thisObject->isDetached())
-        return throwVMTypeError(globalObject, scope, typedArrayBufferHasBeenDetachedErrorMessage);
+        return JSValue::encode(jsBoolean(valueToFind.isUndefined()));
 
     typename ViewClass::ElementType* array = thisObject->typedVector();
     auto targetOption = ViewClass::toAdaptorNativeFromValueWithoutCoercion(valueToFind);
@@ -243,7 +243,7 @@ ALWAYS_INLINE EncodedJSValue genericTypedArrayViewProtoFuncIndexOf(VM& vm, JSGlo
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
     if (thisObject->isDetached())
-        return throwVMTypeError(globalObject, scope, typedArrayBufferHasBeenDetachedErrorMessage);
+        return JSValue::encode(jsNumber(-1));
 
     typename ViewClass::ElementType* array = thisObject->typedVector();
     auto targetOption = ViewClass::toAdaptorNativeFromValueWithoutCoercion(valueToFind);
@@ -269,16 +269,18 @@ ALWAYS_INLINE EncodedJSValue genericTypedArrayViewProtoFuncJoin(VM& vm, JSGlobal
     if (thisObject->isDetached())
         return throwVMTypeError(globalObject, scope, typedArrayBufferHasBeenDetachedErrorMessage);
 
-    // 22.2.3.14
+    unsigned length = thisObject->length();
     auto joinWithSeparator = [&] (StringView separator) -> EncodedJSValue {
-        ViewClass* thisObject = jsCast<ViewClass*>(callFrame->thisValue());
-        unsigned length = thisObject->length();
-
         JSStringJoiner joiner(globalObject, separator, length);
         RETURN_IF_EXCEPTION(scope, encodedJSValue());
-        for (unsigned i = 0; i < length; i++) {
-            joiner.append(globalObject, thisObject->getIndexQuickly(i));
-            RETURN_IF_EXCEPTION(scope, encodedJSValue());
+        if (!thisObject->isDetached()) {
+            for (unsigned i = 0; i < length; i++) {
+                joiner.append(globalObject, thisObject->getIndexQuickly(i));
+                RETURN_IF_EXCEPTION(scope, encodedJSValue());
+            }
+        } else {
+            for (unsigned i = 0; i < length; i++)
+                joiner.appendEmptyString();
         }
         RELEASE_AND_RETURN(scope, JSValue::encode(joiner.join(globalObject)));
     };
@@ -292,8 +294,6 @@ ALWAYS_INLINE EncodedJSValue genericTypedArrayViewProtoFuncJoin(VM& vm, JSGlobal
     JSString* separatorString = separatorValue.toString(globalObject);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
-    if (thisObject->isDetached())
-        return throwVMTypeError(globalObject, scope, typedArrayBufferHasBeenDetachedErrorMessage);
     auto viewWithString = separatorString->viewWithUnderlyingString(globalObject);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
     return joinWithSeparator(viewWithString.view);
@@ -331,7 +331,7 @@ ALWAYS_INLINE EncodedJSValue genericTypedArrayViewProtoFuncLastIndexOf(VM& vm, J
     }
 
     if (thisObject->isDetached())
-        return throwVMTypeError(globalObject, scope, typedArrayBufferHasBeenDetachedErrorMessage);
+        return JSValue::encode(jsNumber(-1));
 
     auto targetOption = ViewClass::toAdaptorNativeFromValueWithoutCoercion(valueToFind);
     if (!targetOption)