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

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: benvie, Assigned: till)

Tracking

(Blocks: 1 bug, {doc-bug-filed})

Trunk
mozilla25
doc-bug-filed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Sorry, that's ES6 spec sections 15.4.3.23 and 15.4.3.24.
(Assignee)

Updated

4 years ago
Assignee: general → tschneidereit
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 765647 [details] [diff] [review]
Implement ES6 Array.prototype.find and Array.prototype.findIndex

Implements both methods, but lacking tests

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
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.
Attachment #765687 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Attachment #765647 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
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.
Attachment #765712 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Attachment #765687 - Attachment is obsolete: true
Attachment #765687 - Flags: review?(jwalden+bmo)
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.
Attachment #765712 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 7

4 years ago
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

4 years ago
https://hg.mozilla.org/mozilla-central/rev/a839287871f9
https://hg.mozilla.org/mozilla-central/rev/dc67ab8bef36
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Keywords: dev-doc-needed

Updated

4 years ago
Depends on: 890070
Depends on: 891779
Follow up for documentation on https://bugzilla.mozilla.org/show_bug.cgi?id=891779
Keywords: dev-doc-needed → doc-bug-filed

Updated

4 years ago
Blocks: 897784
(Assignee)

Updated

4 years ago
No longer blocks: 897784
Depends on: 897784
(Assignee)

Updated

4 years ago
Depends on: 903755
Depends on: 896999
You need to log in before you can comment on or make changes to this bug.