885553 - Implement ES6 Array.prototype.find and Array.prototype.findIndex

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brandon Benvie [:benvie]]

These are basically the same as `Array.prototype.some` (returns on first truthy result from callback) except they return different things:

* Array.prototype.find(predicate, thisArg = undefined)
  Returns the value or undefined if not found.

* Array.prototype.findIndex(predicate, thisArg = undefined)
  Returns the index or -1 if not found.


See ES6 spec (May 2013 revision) sections 15.4.3.24 and 15.4.3.25.

Also see: https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-03/mar-14.md#410-array-extras

Comment 1

[Brandon Benvie [:benvie]]

Sorry, that's ES6 spec sections 15.4.3.23 and 15.4.3.24.

Comment 2

[Till Schneidereit [:till]]

Attachment: Implement ES6 Array.prototype.find and Array.prototype.findIndex

Implements both methods, but lacking tests

Comment 3

[André Bargull [:anba]]

Step 9.d.i of Array.prototype.find is not properly implemented due to possible side-effects in `Get(O, pk)`. For example `Array.prototype.find.call({get 0(){ print("get zero") }, length: 1}, ()=>true)` should print "get zero" only once.

Comment 4

[Till Schneidereit [:till]]

Attachment: Implement ES6 Array.prototype.find and Array.prototype.findIndex. v2

Now with tests. I heavily cribbed from the Array.prototype.some tests in js1_6/Array/regress-290592.js, but added tests for array-likes and removed the method-as-callback tests, as I didn't see how they could be interesting.

Comment 5

[Till Schneidereit [:till]]

Attachment: Implement ES6 Array.prototype.find and Array.prototype.findIndex

@André, good catch! Updated with a fix and test for that case.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 765712 [details] [diff] [review]
Implement ES6 Array.prototype.find and Array.prototype.findIndex

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

::: js/src/builtin/Array.js
@@ +397,5 @@
> +    /* Steps 1-2. */
> +    var O = ToObject(this);
> +
> +    /* Steps 3-5. */
> +    var len = TO_UINT32(O.length);

Per spec this should be ToInteger.  Please add a test that distinguishes these, something like should do it:

var obj = { length: -4294967295, 0: 42 };
assertEq(Array.prototype.find.apply(obj, () => true), undefined);

@@ +410,5 @@
> +    var T = arguments.length > 1 ? arguments[1] : undefined;
> +
> +    /* Steps 8-9. */
> +    /* Steps a (implicit), and e. */
> +    for (var k = 0; k < len; k++) {

The spec algorithm is in terms of mathematically precise numbers.  This is in terms of IEEE-754 numbers.  So if someone does something like this:

var obj = { 18014398509481984: true, length: 18014398509481988 };
Array.prototype.find.apply(obj, () => true);

our implementation will hang forever.  Let's at least put a comment by this pointing out the issue, even if we're not going to deal with it now.

@@ +430,5 @@
> +    /* Steps 1-2. */
> +    var O = ToObject(this);
> +
> +    /* Steps 3-5. */
> +    var len = TO_UINT32(O.length);

ToInteger, and same variety of test please.

::: js/src/tests/ecma_6/Array/find_findindex.js
@@ +31,5 @@
> +var obj;
> +var strings = ['hello', 'Array', 'WORLD'];
> +var mixed   = [0, '1', 2];
> +var sparsestrings = new Array();
> +sparsestrings[2] = 'sparse';

Please also test an array that isIndexed() -- Object.defineProperty(arr, someIndex, { get: function() { return 42; } }) should do the trick of setting one up.

@@ +37,5 @@
> +
> +// find and findIndex have 1 required argument
> +
> +expect = 1;
> +actual = Array.prototype.find.length;

var-declare expect/actual, please, somewhere.

@@ +108,5 @@
> +catch(e)
> +{
> +  actual = dumpError(e);
> +}
> +reportCompare(expect, actual, 'mixed: findIndex finds first string element');

reportCompare compares by loose equality, so checking for 1 and '1' is going to admit more cases than you want.  I don't really care what you use, so long as you don't admit extra cases, but I'd have used assertEq for this stuff, because it uses a SameValue check to determine equality -- and that has a same-type requirement in it (and same-signed-zero and NaNs-are-equal bits, but those aren't relevant here).  Anyway, use *something* that won't let both cases through, please.

Comment 7

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a839287871f9
and, for tests I forgot to qref:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc67ab8bef36

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a839287871f9
https://hg.mozilla.org/mozilla-central/rev/dc67ab8bef36

Comment 9

[Jeremie Patonnier :Jeremie]

Follow up for documentation on https://bugzilla.mozilla.org/show_bug.cgi?id=891779