1078975 - Implement %TypedArray%.prototype.{find, findIndex}

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Comment 1

[ziyunfei]

Attachment: bug-1078975.patch

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8500844 [details] [diff] [review]
bug-1078975.patch

Review of attachment 8500844 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

I have one request for an additional test, though: the spec states that the implementation has to make sure that ArrayBuffer detaching in the predicate doesn't cause issues. You can test that using the shell-only `neuter` function. ("detach" used to be called "neuter" in earlier spec drafts.) Make sure you guard that test with a `if (typeof neuter === "function")` test. Please ask for re-review if that test doesn't just pass as expected.

This change also requires a review from an xpconnect peer for the added methods on TypedArrayPrototype. r?gabor for that.

::: js/src/builtin/TypedArray.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* ES6 draft rev27 (2014/08/24) 22.2.3.10 %TypedArray%.prototype.find(predicate [,thisArg]). */
> +function TypedArrayFind(predicate, thisArg = undefined) {
> +    /* This function is not generic. */

Nit: //-style comments here and everywhere below.

@@ +8,5 @@
> +    if (!IsTypedArray(this))
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "%TypedArray%", "find", "value");
> +
> +    /* Steps 1-2. */
> +    var O = this;

Waldo and I disagree on whether this should be called `O` to align with the spec, or `obj` for readability. I don't care strongly, so let's go with Waldo's opinion and rename this to `obj`.

@@ +26,5 @@
> +    /* Steps 8-9. */
> +    /* Steps a (implicit), and g. */
> +    for (var k = 0; k < len; k++) {
> +        /* Steps a-c. */
> +        var kValue = O[k];

Nit, you can inline the O[k] in the callFunction below, and change the steps comment accordingly.

@@ +43,5 @@
> +    if (!IsTypedArray(this))
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "%TypedArray%", "findIndex", "value");
> +
> +    /* Steps 1-2. */
> +    var O = this;

Same as above.

::: js/src/vm/SelfHosting.cpp
@@ +771,5 @@
>  {
>      return js::TypeDescrIsSizedArrayType(cx, argc, vp);
>  }
>  
> +/*

Uber-nit: DocComment-style /**-start of the comment, please.

Comment 3

[ziyunfei]

Comment on attachment 8500844 [details] [diff] [review]
bug-1078975.patch

Review of attachment 8500844 [details] [diff] [review]:
-----------------------------------------------------------------

>Make sure you guard that test with a `if (typeof neuter === "function")` test. Please ask for re-review if that test doesn't just pass as expected.
Till, I don't know exactly what is the expected behavior for this situation.

js> var arr = new Int8Array(10)
js> neuter(arr.buffer, "same-data")
js> arr.find(v=>true) // undefined

I think this should throw per spec 22.2.3.9 step 6 http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.prototype.filter

js> var arr = new Int8Array([1,2,3,4])
js> arr.find(v => {
js>   print(v)  // 1, 2, 3
js>   if (v === 3) neuter(arr.buffer, "same-data")
js>   if (v === 4) return true
js> })  // undefined

Does this should throw in the fourth loop.

::: js/src/builtin/TypedArray.js
@@ +20,5 @@
> +    if (!IsCallable(predicate))
> +        ThrowError(JSMSG_NOT_FUNCTION, DecompileArg(0, predicate));
> +
> +    /* Step 7. */
> +    var T = thisArg;

If we rename `O` to `obj`, then maybe we should also rename `T` to `thisArg` below, and make step 7 here implicit.

@@ +26,5 @@
> +    /* Steps 8-9. */
> +    /* Steps a (implicit), and g. */
> +    for (var k = 0; k < len; k++) {
> +        /* Steps a-c. */
> +        var kValue = O[k];

I think we cannot inline O[k] here, since if we do that, step b "Let kValue be Get(O, Pk)" will be run twice at line 32 and line 33.

Comment 4

[Till Schneidereit [:till]]

(In reply to ziyunfei from comment #3)
> >Make sure you guard that test with a `if (typeof neuter === "function")` test. Please ask for re-review if that test doesn't just pass as expected.
> Till, I don't know exactly what is the expected behavior for this situation.
> 
> js> var arr = new Int8Array(10)
> js> neuter(arr.buffer, "same-data")
> js> arr.find(v=>true) // undefined
> 
> I think this should throw per spec 22.2.3.9 step 6
> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.
> prototype.filter

Correct, both cases you mention should throw. That is a relatively recent change in the spec, though, and it's not yet implemented. For now, please write the tests to the current behavior; they'll just turn red once we change the behavior, at which point we'll fix them.

Always test for before and after neutering, like so:

var arr = new Int8Array([1,2,3]);
assertEq(arr.find(v => v === 2), true);
neuter(arr.buffer, "change-data");
assertEq(arr.find(v => v === 2), false);

Oh, and please use "change-data", as that is equivalent to what structured cloning does (used for sending across channels, e.g. from a DOM Worker to the main thread).

Please request another review for those tests from me. You can carry Gabor's r+, though.


> If we rename `O` to `obj`, then maybe we should also rename `T` to `thisArg`
> below, and make step 7 here implicit.

I guess that makes sense, yes.

> I think we cannot inline O[k] here, since if we do that, step b "Let kValue
> be Get(O, Pk)" will be run twice at line 32 and line 33.

Oh, of course. Sorry for the noise.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8500844 [details] [diff] [review]
bug-1078975.patch

Review of attachment 8500844 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TypedArray.js
@@ +5,5 @@
> +/* ES6 draft rev27 (2014/08/24) 22.2.3.10 %TypedArray%.prototype.find(predicate [,thisArg]). */
> +function TypedArrayFind(predicate, thisArg = undefined) {
> +    /* This function is not generic. */
> +    if (!IsTypedArray(this))
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "%TypedArray%", "find", "value");

So IsTypedArray does approximately an exact-class check.  I suspect we'll have bugs with this, for the case where find/findIndex are called across global objects.  How does this shell testcase behave?

var t = new (newGlobal().Uint8Array)(8);
assertEq(newGlobal().Uint8Array.prototype.find.call(t, 0), 0);

I'd bet it throws.

This may mean we need, in effect, a self-hosted version of CallNonGenericMethod.  If so, please file a followup bug.  If I'm just misreading, please add a testcase.

::: js/src/tests/ecma_6/TypedArray/find-and-findIndex.js
@@ +30,5 @@
> +                throw new Error("length accessor called");
> +            }
> +        });
> +        assertEq(arr[method].length, 1);
> +        assertEq(arr[method](v => v === 3), 3);

There should be a test searching for +0 and a test searching for -0.  And the array should include NaN, and finding NaN should be tested.

@@ +32,5 @@
> +        });
> +        assertEq(arr[method].length, 1);
> +        assertEq(arr[method](v => v === 3), 3);
> +        assertEq(arr[method](v => v === 6), method === "find" ? undefined : -1);
> +        [undefined, null, true, 1, "foo", [], {}].forEach(thisArg =>

Add in a symbol as well -- poke jorendorff for the preferred way to do it, with symbols only being enabled in nightlies right now.

::: js/src/vm/TypedArrayObject.cpp
@@ +759,4 @@
>      JS_FN("subarray", TypedArrayObject::subarray, 2, 0),
>      JS_FN("set", TypedArrayObject::set, 2, 0),
>      JS_FN("copyWithin", TypedArrayObject::copyWithin, 2, 0),
> +    JS_SELF_HOSTED_FN("@@iterator", "ArrayValues", 0, 0),

FYI, we have a debugger test or two that depends on the ordering of values in this array.  :-(  And maybe on the elements of it, can't remember offhand now.  We should probably make the test not so dependent on this.

Comment 6

[Tom S [:evilpie]]

It would be great if we could get that landed, because this patch contains all of the infrastructure code for the rest of the %TypedArray methods. I want to start opening good-first-bugs for those.

Comment 7

[Tom S [:evilpie]]

Comment on attachment 8500844 [details] [diff] [review]
bug-1078975.patch

Review of attachment 8500844 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TypedArray.js
@@ +8,5 @@
> +    if (!IsTypedArray(this))
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "%TypedArray%", "find", "value");
> +
> +    /* Steps 1-2. */
> +    var O = this;

We already use `O` in Array.js

::: js/src/vm/SelfHosting.cpp
@@ +776,5 @@
> + * Returns the value of [[ArrayLength]] internal slot of typed arrays.
> + */
> +bool
> +js::intrinsic_TypedArrayLength(JSContext *cx, unsigned argc, Value *vp)
> +{

If we are going to add an intrinsic for this, we should make sure it gets ion inlined.

Comment 8

[Tom S [:evilpie]]

Attachment: bug-1078975.patch

I tried to address all of the feedback. I am going to land this patch tomorrow when it looks good on try, so that we can't start opening bugs for the other methods. I already filed a few follow ups.

Comment 9

[Tom S [:evilpie]]

Attachment: v2

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8524971 [details] [diff] [review]
v2

Review of attachment 8524971 [details] [diff] [review]:
-----------------------------------------------------------------

This mostly looks good. Still giving f+ instead of r+ because I have a few open questions. If these can all be answered satisfactorily, I'll change to r+. (I know you only asked for f+, but a new review seems in order given the issues that were unearthed since my last one.)

::: js/src/builtin/TypedArray.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// ES6 draft rev27 (2014/08/24) 22.2.3.10 %TypedArray%.prototype.find(predicate [,thisArg]).

There's since been a new draft, please update the references to that.

@@ +5,5 @@
> +// ES6 draft rev27 (2014/08/24) 22.2.3.10 %TypedArray%.prototype.find(predicate [,thisArg]).
> +function TypedArrayFind(predicate, thisArg = undefined) {
> +    // This function is not generic.
> +    if (!IsObject(this) || !IsTypedArray(this))
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "%TypedArray%", "find", "value");

In other places, we at least do something like `typeof this` instead of "value" to get a slightly better message. Please do that here and below.

@@ +8,5 @@
> +    if (!IsObject(this) || !IsTypedArray(this))
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "%TypedArray%", "find", "value");
> +
> +    // Steps 1-2.
> +    var O = this;

Waldo and me disagree on this, but I think it's nicer to stick with the spec naming here like you do. Everybody should employ fonts that make the difference between O and 0 entirely obvious, anyway.

::: js/src/tests/ecma_6/TypedArray/find-and-findIndex.js
@@ +44,5 @@
> +
> +        assertThrowsInstanceOf(() => arr[method](), TypeError);
> +        assertThrowsInstanceOf(() => arr[method](1), TypeError);
> +
> +        // ToDo neutering

What's the argument for being able to land without this? It seems to me like we absolutely have to land bug 1101256 before this one, no?

If I'm mistaken about that, at least mention the bug number here.

::: js/src/vm/SelfHosting.cpp
@@ +805,5 @@
> +    MOZ_ASSERT(args.length() == 1);
> +    MOZ_ASSERT(args[0].isObject());
> +
> +    RootedObject obj(cx, &args[0].toObject());
> +    args.rval().setBoolean(IsAnyTypedArray(obj));

Do you have any plans to work on bug 1081435? Would be nice not to delay this too much, as it's a clear correctness issue. At least for now, would `IsAnyTypedObject(CheckedUnwrap(obj))` work to fix Waldo's test case from comment 5?

::: js/src/vm/TypedArrayObject.cpp
@@ +767,5 @@
>      JS_FN("subarray", TypedArrayObject::subarray, 2, 0),
>      JS_FN("set", TypedArrayObject::set, 2, 0),
>      JS_FN("copyWithin", TypedArrayObject::copyWithin, 2, 0),
> +    JS_SELF_HOSTED_FN("find", "TypedArrayFind", 2, 0),
> +    JS_SELF_HOSTED_FN("findIndex", "TypedArrayFindIndex", 2, 0),

I think length should be 1 for both of these.

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +178,5 @@
>      gPrototypeProperties[c] = ["constructor", "BYTES_PER_ELEMENT"];
>    }
>    gPrototypeProperties['TypedArray'] =
>      ["length", "buffer", "byteLength", "byteOffset", kIteratorSymbol, "subarray",
> +     "set", "copyWithin", "find", "findIndex"];

Don't forget the r=gabor in the final commit message.

Comment 11

[Tom S [:evilpie]]

>What's the argument for being able to land without this? It seems to me like we absolutely have to land >bug 1101256 before this one, no?
I find that this neutering check is a totally weird decision. You can still neuter the typedarray while invoking the callback and we have to handle that gracefully anyway. Unless of course "A TypeError exception is also immediately thrown if the this value is detached or if its [[ViewedArrayBuffer]] is." is supposed to cover the whole execution time of this function?

>I think length should be 1 for both of these.
Good catch.

Comment 12

[Till Schneidereit [:till]]

(In reply to Tom Schuster [:evilpie] from comment #11)
> >What's the argument for being able to land without this? It seems to me like we absolutely have to land >bug 1101256 before this one, no?
> I find that this neutering check is a totally weird decision. You can still
> neuter the typedarray while invoking the callback and we have to handle that
> gracefully anyway. Unless of course "A TypeError exception is also
> immediately thrown if the this value is detached or if its
> [[ViewedArrayBuffer]] is." is supposed to cover the whole execution time of
> this function?

That's a good point. I guess IntegerIndexedElementGet[1] should catch that in the first iteration. There is a difference in when the error would be thrown, though. Say `callback` isn't a function or coercing `thisArg` to Object throws. Presumably, these errors are meant to be thrown after the TypeError for a detached buffer.


[1] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-integerindexedelementget

Comment 13

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b23f3ddef1c

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b23f3ddef1c

Comment 15

[Florian Scholz (Open Web Docs)]

Release notes
https://developer.mozilla.org/en-US/Firefox/Releases/37#JavaScript

Reference pages (based on Array#find and Array#findIndex)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/find
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/findIndex

Feel free to improve these pages (code samples? is this polyfillable?).

Comment 16

[Tom S [:evilpie]]

Apparently I forgot to actually commit the test.
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d9f3f8aaf1

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed the test out for jsreftest orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3f2dfbff62

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4810903&repo=mozilla-inbound

Comment 18

[Tom S [:evilpie]]

Removed the neutering, we haven't been doing this in any other typed array tests either.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3927eb71bba8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d70de8e8bcf

Comment 19

[Jeff Walden [:Waldo]]

(In reply to Tom Schuster [:evilpie] from comment #18)
> Removed the neutering, we haven't been doing this in any other typed array
> tests either.

Uh....  This is a serious mistake.  Every new typed array method should include tests for behaviors in the presence of neutering.  We definitely need to fix this.  Please file a new bug, and I'll look into it.  (I had some concerns about ES6 language for these methods at one point, actually, and the testing may well fold nicely into that concern.)

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d70de8e8bcf