894658 - Implement ES6 Array.prototype.{keys, entries}

Status: RESOLVED FIXED

Description

[Brandon Benvie [:benvie]]

For ES6, all builtin iterables should have the three iterator-creating methods (keys, values, entries). `Array#values` was added during bug 875433, but `Array#entries` and `Array#keys` remain unimplemented.

See ES6 spec (July 2013 revision):

* Array.prototype.entries (section 15.4.3.25)
  An iterator that produces a key/value pair for each index in the array.
> [...['a', 'b', 'c'].entries()] // produces....
> [['0', 'a'], ['1', 'b'], ['2', 'c']]

* Array.prototype.keys (section 15.4.3.26)
  An iterator that produces an array of keys for to the indices in array.
> [...['a', 'b', 'c'].keys()] // produces....
> ['0', '1', '2']

Array#entries, along with bug 894637, are required for the following to work as desired:
> new Map(['a', 'b', 'c'])

Comment 1

[Till Schneidereit [:till]]

Given how much trouble Array#values is causing, we should probably implement these sooner rather than later.

@sankha, you wouldn't by any chance be interested in working on this? What with having done all the other stuff in this area.

Comment 2

[Sankha Narayan Guria [:sankha]]

Thanks Till. I will look into this.

Comment 3

[Sankha Narayan Guria [:sankha]]

This is a WIP patch. I will be writing the tests.

I am having problem in the Array.prototype.entries. It fails to compile with what I have written so I have commented it out. Can you help me out with this?

All the current jit-tests are passing with this.

Comment 4

[Brandon Benvie [:benvie]]

Comment on attachment 777235 [details] [diff] [review]
WIP Patch v1

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

::: js/src/jsiter.cpp
@@ +961,5 @@
> +            }
> +            break;
> +
> +        case ElementIteratorObject::Keys:
> +            args.rval().setInt32(i);

This should be a string.

@@ +966,5 @@
> +            break;
> +
> +        case ElementIteratorObject::Entries:
> +            /*Value pair[2];
> +            pair[0].setInt32(i);

This should be a string.

Comment 5

[Brandon Benvie [:benvie]]

Ignore my last comment, the last TC39 meeting has agreed to change those to be numbers.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 777235 [details] [diff] [review]
WIP Patch v1

Don't put JS_ArrayEntries/Keys/Values in the API. I don't think anybody will use it; let's just define those functions in jsarray.cpp.

Otherwise this seems fine. Alternatively these could be self-hosted. I don't know which is better.

Comment 7

[Sankha Narayan Guria [:sankha]]

Addressed the previous feedback and moved the keys() and entries() out of the public JSAPI. I have added tests as well.

But will this patch have any issues with the patch in bug 919948?

Comment 8

[Andy Wingo [:wingo]]

This patch should be reimplemented on top of bug 919948, I think.  Sorry for the churn :/

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 816599 [details] [diff] [review]
patch v2

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

Andy is right, this does need to be reimplemented, but the patch should actually be much much easier now!

In jsarray.cpp:
>    JS_SELF_HOSTED_FN("@@iterator",  "ArrayValues",      0,0),

All we need, I think, is to add some lines in this array for "keys", "values", and "entries". Plus tests, of course.

Clearing the r? flag.

Comment 10

[Sankha Narayan Guria [:sankha]]

Should I add Array#values in this bug? It had caused a lot of trouble in bug 875433.

Comment 11

[Jason Orendorff [:jorendorff]]

Er, no, definitely not. Good catch.

Comment 12

[Sankha Narayan Guria [:sankha]]

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 822893 [details] [diff] [review]
patch v3

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

Thanks. This needs more tests.

Put the tests you've written into js/src/jit-test/tests/for-of, and check out the existing tests js/src/jit-test/tests/for-of/array-iterator-*.js.

You can add test coverage just by modifying some of those existing test files to try .entries and .keys too.

It's not necessary to modify them all, just a few here and there. Particularly array-iterator-generic.js.

Comment 14

[Sankha Narayan Guria [:sankha]]

Modified existing tests to include .keys() and .entries().

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 825176 [details] [diff] [review]
patch v4

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

Great! Thanks again. r=me with these tweaks.

::: js/src/jit-test/tests/for-of/array-iterator-changing.js
@@ +12,5 @@
>  assertIteratorNext(it, 2000);
>  assertIteratorDone(it, undefined);
> +
> +// test that .keys() and .entries() have the same behaviour
> +

To really test the same behavior, this needs to set arr = [0, 1, 2]; at the beginning of the second test.

::: js/src/jit-test/tests/for-of/array-iterator-surfaces-2.js
@@ +27,5 @@
>  check(Array.prototype[std_iterator].call({}));
> +check([].keys());
> +check(Array.prototype[std_iterator].call({}));
> +check([].entries());
> +check(Array.prototype[std_iterator].call({}));

I think you meant Array.prototype.keys.call({}) on line 29
and Array.prototype.entries.call({}) on line 31.

Comment 16

[Sankha Narayan Guria [:sankha]]

Made required changes.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31e1a183f9c1

Comment 18

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/31e1a183f9c1

Comment 19

[Chris Peterson [:cpeterson]]

JS feature added in Firefox 28

Comment 20

[Hallvord R. M. Steen [:hallvors]]

This feature breaks Twitter: https://bugzilla.mozilla.org/show_bug.cgi?id=924386#c19
We can (and will) evangelise that - I wonder if other stuff will break, though..

Comment 21

[Florian Scholz [:fscholz] (MDN)]

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

New pages contributed by https://developer.mozilla.org/en-US/profiles/vladikoff:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/entries
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/keys

We still need Array Iterator docs, but not tracking that here.
As always: Feel free to review and improve these docs in the MDN wiki.