Last Comment Bug 885553 - Implement ES6 Array.prototype.find and Array.prototype.findIndex
: Implement ES6 Array.prototype.find and Array.prototype.findIndex
Status: RESOLVED FIXED
: doc-bug-filed
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Till Schneidereit [:till]
:
Mentors:
Depends on: 896999 890070 891779 897784 903755
Blocks: es6
  Show dependency treegraph
 
Reported: 2013-06-20 15:12 PDT by Brandon Benvie [:benvie]
Modified: 2013-10-28 03:58 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement ES6 Array.prototype.find and Array.prototype.findIndex (3.70 KB, patch)
2013-06-20 15:42 PDT, Till Schneidereit [:till]
no flags Details | Diff | Review
Implement ES6 Array.prototype.find and Array.prototype.findIndex. v2 (7.42 KB, patch)
2013-06-20 16:44 PDT, Till Schneidereit [:till]
no flags Details | Diff | Review
Implement ES6 Array.prototype.find and Array.prototype.findIndex (7.69 KB, patch)
2013-06-20 17:31 PDT, Till Schneidereit [:till]
jwalden+bmo: review+
Details | Diff | Review

Description Brandon Benvie [:benvie] 2013-06-20 15:12:49 PDT
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] 2013-06-20 15:14:27 PDT
Sorry, that's ES6 spec sections 15.4.3.23 and 15.4.3.24.
Comment 2 Till Schneidereit [:till] 2013-06-20 15:42:55 PDT
Created attachment 765647 [details] [diff] [review]
Implement ES6 Array.prototype.find and Array.prototype.findIndex

Implements both methods, but lacking tests
Comment 3 André Bargull 2013-06-20 16:40:03 PDT
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] 2013-06-20 16:44:00 PDT
Created attachment 765687 [details] [diff] [review]
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] 2013-06-20 17:31:20 PDT
Created attachment 765712 [details] [diff] [review]
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] (remove +bmo to email) 2013-06-25 15:35:54 PDT
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 9 Jeremie Patonnier :Jeremie 2013-07-10 01:53:52 PDT
Follow up for documentation on https://bugzilla.mozilla.org/show_bug.cgi?id=891779

Note You need to log in before you can comment on or make changes to this bug.