Add Array.prototype.includes

Add Array.prototype.includes

Requires adding a SameValueZero implementation.

LOG=Y
BUG=v8:3575
R=dslomov@chromium.org, arv@chromium.org
TEST=added to test262

[domenic (use chromium.org), 2014-12-02 01:18:58]

For tests I would like guidance as to how we should proceed. The existing
test262-format tests are at
https://github.com/tc39/Array.prototype.includes/tree/master/test.

1) Port the tests to mjsunit and commit them.

2) Add the ability to pull test262-format tests from arbitrary repos (in a
sibling folder, e.g. test262-other) and have it pull from
tc39/Array.prototype.includes

3) Add the ability to run checked-in test262 tests (in a sibling folder, e.g.
test262-not-upstreamed-yet) and check in the tests. Doing so will probably mean
modifying the copyright header of the tests to make the presubmit happy.

4) Get the tests merged into tc39/test262 proper, and add some way of running a
subset of test262 tests during development. This will probably be blocked on
other test262-related changes, e.g. the fact that we are using the old
deprecated Python runner and not the new Node.js-based runner.

I am happy to do any of these, and I have a spike of 3) at
https://github.com/domenic/v8-git-mirror/tree/test262-not-upstreamed-yet but ran
into the copyright header issue and thought I'd better ask before moving
further.

[caitp (gmail), 2014-12-02 02:03:00]

On 2014/12/02 01:18:58, domenic wrote:
> For tests I would like guidance as to how we should proceed. The existing
> test262-format tests are at
> https://github.com/tc39/Array.prototype.includes/tree/master/test.
> 
> 1) Port the tests to mjsunit and commit them.
> 
> 2) Add the ability to pull test262-format tests from arbitrary repos (in a
> sibling folder, e.g. test262-other) and have it pull from
> tc39/Array.prototype.includes
> 
> 3) Add the ability to run checked-in test262 tests (in a sibling folder, e.g.
> test262-not-upstreamed-yet) and check in the tests. Doing so will probably
mean
> modifying the copyright header of the tests to make the presubmit happy.
> 
> 4) Get the tests merged into tc39/test262 proper, and add some way of running
a
> subset of test262 tests during development. This will probably be blocked on
> other test262-related changes, e.g. the fact that we are using the old
> deprecated Python runner and not the new Node.js-based runner.
> 
> I am happy to do any of these, and I have a spike of 3) at
> https://github.com/domenic/v8-git-mirror/tree/test262-not-upstreamed-yet but
ran
> into the copyright header issue and thought I'd better ask before moving
> further.

It looks like the test262-es6 runner isn't able to pass in flags for unshipped
features (or maybe it can and it's just not happening?), so that seems like a
blocker for running these from test262 directly.

Would it be terrible to add v8-specific tests for this feature and remove them
once it's handled by test262 proper? mjsunit would simplify adding the needed
flags.

[domenic (use chromium.org), 2014-12-02 02:29:17]

> It looks like the test262-es6 runner isn't able to pass in flags for unshipped
features (or maybe it can and it's just not happening?), so that seems like a
blocker for running these from test262 directly.

Sure, but that's not hard to fix. We could have a manifest file mapping test262
tests to the flags necessary.

On 2014/12/02 02:03:00, caitp wrote:

> Would it be terrible to add v8-specific tests for this feature and remove them
> once it's handled by test262 proper? mjsunit would simplify adding the needed
> flags.

Yep, I'd be happy to do that; that's option 1.

[Dmitry Lomov (no reviews), 2014-12-02 12:22:40]

On 2014/12/02 02:29:17, domenic wrote:
> > It looks like the test262-es6 runner isn't able to pass in flags for
unshipped
> features (or maybe it can and it's just not happening?), so that seems like a
> blocker for running these from test262 directly.
> 
> Sure, but that's not hard to fix. We could have a manifest file mapping
test262
> tests to the flags necessary.
> 
> On 2014/12/02 02:03:00, caitp wrote:
> 
> > Would it be terrible to add v8-specific tests for this feature and remove
them
> > once it's handled by test262 proper? mjsunit would simplify adding the
needed
> > flags.
> 
> Yep, I'd be happy to do that; that's option 1.

I'd prefer tests in V8 repo, until we figure out our relationship with test262
:)

[Dmitry Lomov (no reviews), 2014-12-02 12:25:26]

As a ES7 feature, this needs intent-to-implement (and separate flag)

https://codereview.chromium.org/771863002/diff/1/src/harmony-array.js
File src/harmony-array.js (right):

https://codereview.chromium.org/771863002/diff/1/src/harmony-array.js#newcode133
src/harmony-array.js:133: function ArrayIncludes(searchElement, fromIndex) {
Since it is not a ES6 feature, Array.prototype.includes should be under a
separate flag.

[domenic (use chromium.org), 2014-12-02 15:17:40]

> I'd prefer tests in V8 repo, until we figure out our relationship with test262
:)

OK. Would you prefer option 1, option 2, or option 3?

> As a ES7 feature, this needs intent-to-implement (and separate flag)

Already sent a while back:

https://groups.google.com/forum/#!topic/v8-dev/Th-67YERn1I

Questions about flags included there. It seems we have since defined "harmony"
to mean "ES6" (in contrary to its TC39-understood-definition), so ideas for new
flag names?

[arv (Not doing code reviews), 2014-12-02 15:19:02]

On 2014/12/02 at 15:17:40, d wrote:
> > I'd prefer tests in V8 repo, until we figure out our relationship with
test262
> :)
> 
> OK. Would you prefer option 1, option 2, or option 3?
> 
> > As a ES7 feature, this needs intent-to-implement (and separate flag)
> 
> Already sent a while back:
> 
> https://groups.google.com/forum/#!topic/v8-dev/Th-67YERn1I
> 
> Questions about flags included there. It seems we have since defined "harmony"
to mean "ES6" (in contrary to its TC39-understood-definition), so ideas for new
flag names?

harmony is everything harmonious... no need to come up with a new name.

[domenic (use chromium.org), 2014-12-02 16:20:09]

Arv clarified that we probably want a new flag but it would still start with
"harmony". We settled on --harmony_array_includes for now.

Pretty sure I did all the right steps for introducing a new flag (and it works,
testing against the test262 tests when running with --harmony_array_includes).

[arv (Not doing code reviews), 2014-12-02 16:26:48]

Impl looks good. Were you planning on moving the tests over or not? As is now,
no tests are covering this.

[domenic (use chromium.org), 2014-12-02 16:39:04]

On 2014/12/02 16:26:48, arv wrote:
> Impl looks good. Were you planning on moving the tests over or not? As is now,
> no tests are covering this.

I'm still waiting to figure out which of the four options from my OP is desired.
As of now Dmitry indicated the tests should be part of V8, so that rules out
option 4; still unclear which of options 1, 2, or 3 would be preferred.

[Dmitry Lomov (no reviews), 2014-12-02 17:52:56]

On 2014/12/02 16:39:04, domenic wrote:
> On 2014/12/02 16:26:48, arv wrote:
> > Impl looks good. Were you planning on moving the tests over or not? As is
now,
> > no tests are covering this.
> 
> I'm still waiting to figure out which of the four options from my OP is
desired.
> As of now Dmitry indicated the tests should be part of V8, so that rules out
> option 4; still unclear which of options 1, 2, or 3 would be preferred.

For now, I'd prefer option 1.

[domenic (use chromium.org), 2014-12-04 20:42:08]

On 2014/12/02 at 17:52:56, dslomov wrote:

> For now, I'd prefer option 1.

Tests uploaded!

[caitp (gmail), 2014-12-04 20:53:02]

caitpotter88@gmail.com changed reviewers:
+ caitpotter88@gmail.com

[caitp (gmail), 2014-12-04 20:53:02]

These are really just nitpicky, but

Other tests that are often present:

Test that an exception during a getter results propagates the exception

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
File test/mjsunit/harmony/array-includes-to-object.js (right):

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-includes-to-object.js:21: })();
Usually these tests include `assertThrows(function() {
Array.prototype.includes.call(undefined, whatever); }, TypeError);` and similar

[arv (Not doing code reviews), 2014-12-04 21:04:01]

Maybe some tests on holey arrays with things on the prototype?

Maybe some tests of typed arrays?

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
File test/mjsunit/harmony/array-includes-to-object.js (right):

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-includes-to-object.js:12: // Array.prototype.includes
should use ToObject on this, so that when called with a number, it picks up
numeric properties from Number.prototype
Can you add a test that this inside a getter has the right type?

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
File test/mjsunit/harmony/array-includes.js (right):

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-includes.js:11: //
Nit. We don't use this kind of formatting in other places. Since this is
generated maybe do:

// Array.prototype.includes sees a new element added by a getter that is hit
during iteration 
(function() {

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-includes.js:27: 
We use 2 newlines between the test "blocks"

[domenic (use chromium.org), 2014-12-04 21:57:19]

caitp:

> Test that an exception during a getter results propagates the exception

Already there; search for "should terminate if getting an index throws an
exception"

arv:

added the tests and addressed the nits, I think!

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
File test/mjsunit/harmony/array-includes-to-object.js (right):

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-includes-to-object.js:12: // Array.prototype.includes
should use ToObject on this, so that when called with a number, it picks up
numeric properties from Number.prototype
On 2014/12/04 21:04:00, arv wrote:
> Can you add a test that this inside a getter has the right type?

Done.

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-includes-to-object.js:21: })();
On 2014/12/04 20:53:02, caitp wrote:
> Usually these tests include `assertThrows(function() {
> Array.prototype.includes.call(undefined, whatever); }, TypeError);` and
similar

Done.

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
File test/mjsunit/harmony/array-includes.js (right):

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-includes.js:11: //
On 2014/12/04 21:04:00, arv wrote:
> Nit. We don't use this kind of formatting in other places. Since this is
> generated maybe do:

Hmm OK I was copying
https://code.google.com/p/chromium/codesearch#chromium/src/v8/test/mjsunit/ha...
will fix

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-includes.js:11: //
On 2014/12/04 21:04:00, arv wrote:
> Nit. We don't use this kind of formatting in other places. Since this is
> generated maybe do:
> 
> // Array.prototype.includes sees a new element added by a getter that is hit
> during iteration 
> (function() {

Done.

https://codereview.chromium.org/771863002/diff/60001/test/mjsunit/harmony/arr...
test/mjsunit/harmony/array-includes.js:27: 
On 2014/12/04 21:04:01, arv wrote:
> We use 2 newlines between the test "blocks"

Done.

[Dmitry Lomov (no reviews), 2014-12-09 21:13:03]

lgtm, thanks for porting tests.

[arv (Not doing code reviews), 2014-12-09 21:38:48]

https://codereview.chromium.org/771863002/diff/100001/test/mjsunit/harmony/ar...
File test/mjsunit/harmony/array-includes.js (right):

https://codereview.chromium.org/771863002/diff/100001/test/mjsunit/harmony/ar...
test/mjsunit/harmony/array-includes.js:121: }, MjsUnitAssertionError);
I think assertUnreachable throws an MjsUnitAssertionError. Maybe throw a
different error in valueOf?

https://codereview.chromium.org/771863002/diff/100001/test/mjsunit/harmony/ar...
test/mjsunit/harmony/array-includes.js:136: throw new
MjsUnitAssertionError("This error should be re-thrown");
same?

[domenic_domenicdenicola.com, 2014-12-09 22:15:55]

domenic@domenicdenicola.com changed reviewers:
+ domenic@domenicdenicola.com

[domenic_domenicdenicola.com, 2014-12-09 22:15:55]

The CQ bit was checked by domenic@domenicdenicola.com

[domenic_domenicdenicola.com, 2014-12-09 22:15:56]

Fixed to use dummy `function Test262Error() {}`s in the relevant tests; thanks
arv!

[commit-bot: I haz the power, 2014-12-09 22:16:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/771863002/120001

[commit-bot: I haz the power, 2014-12-09 22:17:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-09 22:17:49]

Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1651)
  v8_win_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/1867)

[domenic_domenicdenicola.com, 2014-12-09 22:41:06]

The CQ bit was checked by domenic@domenicdenicola.com

[domenic_domenicdenicola.com, 2014-12-09 22:41:07]

Forgot to rebase!

[commit-bot: I haz the power, 2014-12-09 22:41:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/771863002/140001

[commit-bot: I haz the power, 2014-12-10 08:58:14]

Committed patchset #8 (id:140001)