Last Comment Bug 1069063 - Implement Array.prototype.includes
: Implement Array.prototype.includes
Status: RESOLVED FIXED
[DocArea=JS]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript: Standard Library (show other bugs)
: Trunk
: All All
-- normal with 1 vote (vote)
: mozilla36
Assigned To: ziyunfei
:
: Jason Orendorff [:jorendorff]
Mentors:
https://github.com/domenic/Array.prot...
Depends on: 1075059
Blocks: es7 1070767
  Show dependency treegraph
 
Reported: 2014-09-17 18:13 PDT by Till Schneidereit [till]
Modified: 2015-01-19 13:13 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug1069063.patch (5.59 KB, patch)
2014-09-18 02:23 PDT, ziyunfei
no flags Details | Diff | Splinter Review
bug-1069063-v2.patch (5.54 KB, patch)
2014-09-18 20:40 PDT, ziyunfei
till: review+
Details | Diff | Splinter Review
rename `contains` to `includes` (15.07 KB, patch)
2014-11-19 19:57 PST, ziyunfei
till: review+
Details | Diff | Splinter Review
Fix up test_xrayToJS.xul (16.87 KB, patch)
2014-11-20 14:59 PST, ziyunfei
446240525: review+
till: feedback+
Details | Diff | Splinter Review
remove step 8 and fix steps' numbering (16.81 KB, patch)
2014-11-20 20:34 PST, ziyunfei
446240525: review+
Details | Diff | Splinter Review

Description User image Till Schneidereit [till] 2014-09-17 18:13:03 PDT
Not yet in the spec draft, but got approval at the last F2F. Spec at
https://github.com/domenic/Array.prototype.contains/blob/master/spec.md

Implementation should be very straight-forward, essentially duplicating most of indexOf[1]. Tests could also essentially duplicate the ones for indexOf, with the exception of having to add tests for the behavior around NaNs.


[1]: http://mxr.mozilla.org/mozilla-central/source/js/src/builtin/Array.js?rev=bb579e3de64b#6
Comment 1 User image Till Schneidereit [till] 2014-09-17 18:14:45 PDT
Oh, I forgot: we don't have an intrinsic for SameValueZero. The best course of action here would be to self-host that, too, and add it to builtins/Utilities.js. Just inlining the check would work, too, though.
Comment 2 User image Domenic Denicola 2014-09-17 18:15:13 PDT
Pretty darn comprehensive test262 tests are already written: https://github.com/tc39/test262/pull/95

Reference implementation at https://github.com/domenic/Array.prototype.contains/blob/master/reference-implementation/index.js
Comment 3 User image ziyunfei 2014-09-18 02:23:53 PDT
Created attachment 8491350 [details] [diff] [review]
bug1069063.patch
Comment 4 User image ziyunfei 2014-09-18 02:40:43 PDT
Comment on attachment 8491350 [details] [diff] [review]
bug1069063.patch

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

Do we need to move Array/contains.js to tests/ecma_7
Comment 5 User image Till Schneidereit [till] 2014-09-18 07:13:53 PDT
Comment on attachment 8491350 [details] [diff] [review]
bug1069063.patch

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

This looks great, thanks!

Before landing it though, we should figure out how to deal with the test262 tests Domenic mentioned in comment 2. I posted to the JS internals mailing list about this[1], so let's see what other people think.

For now, can you import the tests locally by putting them into a subfolder of tests/test262 and make sure that they all pass?

[1] https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine.internals/VmjEa2ATcNo

::: js/src/builtin/Array.js
@@ +602,5 @@
> +        return false;
> +
> +    // Step 9.
> +    var k;
> +    if (n >= 0)

Nit: We add braces for all branches if at least one requires them. Since the `else` branch does, please add them here.

@@ +619,5 @@
> +        // Steps a-b.
> +        var elementK = O[k];
> +
> +        // Step c.
> +        if (SameValueZero(searchElement, elementK))

I wouldn't mind if you inlined elementK here, and made steps a-c one thing.

::: js/src/builtin/Utilities.js
@@ +155,5 @@
>      // Math.pow(2, 53) - 1 = 0x1fffffffffffff
>      return v < 0x1fffffffffffff ? v : 0x1fffffffffffff;
>  }
>  
> +/* Spec: ECMAScript Draft, 6 edition Aug 24, 2014, 7.2.4 */

Nits: use line comment, change "6 edition" to "6th edition", and add a "." at the end.

(Yes, this file is really inconsistent about comment formatting, but this is as close to the usual style in self-hosting as it gets.)

@@ +157,5 @@
>  }
>  
> +/* Spec: ECMAScript Draft, 6 edition Aug 24, 2014, 7.2.4 */
> +function SameValueZero(x, y) {
> +    return x !== x && y !== y || x === y

Neat solution. Only small quibble: please switch the order of the || operands around; the NaN case should be far less likely. I know that just affects cases where `x === y` actually holds, which is one at most per call of `[].contains`, but still.

Also, nit: semicolon missing.
Comment 6 User image Jason Orendorff [:jorendorff] 2014-09-18 12:33:57 PDT
> +function SameValueZero(x, y) {
> +    return x !== x && y !== y || x === y

Also, please parenthesize around this, and add the optional semicolon:

    return x === y || (x !== x && y !== y);
Comment 7 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-09-18 13:20:12 PDT
(In reply to Till Schneidereit [:till] from comment #5)
> Before landing it though, we should figure out how to deal with the test262
> tests Domenic mentioned in comment 2. I posted to the JS internals mailing
> list about this[1], so let's see what other people think.
> 
> For now, can you import the tests locally by putting them into a subfolder
> of tests/test262 and make sure that they all pass?

tests/test262 should largely be considered read-only.  It's generally a pain to have an import-folder that also has modifications made to it, that must be preserved across updates.  I'd really, really, rather you not change tests/test262 at all.  (The only case we've ever done it was to modify existing tests that a change on our end deliberately broke.  Those changes are recorded in the old-but-currently-dysfunctional update-test262.sh script.  It's on my todo list to update that to work again, soon.)
Comment 8 User image Till Schneidereit [till] 2014-09-18 15:47:13 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> tests/test262 should largely be considered read-only.  It's generally a pain
> to have an import-folder that also has modifications made to it, that must
> be preserved across updates.  I'd really, really, rather you not change
> tests/test262 at all.

Sorry, I didn't mean to imply that the tests should be *landed* there. I meant putting them there in a WIP patch for testing purposes to make sure the implementation passes them. Putting them under the test262 folder makes them pick up the harness stuff required, so it's easiest. I really didn't make that very clear.
Comment 9 User image ziyunfei 2014-09-18 20:39:05 PDT
(In reply to Till Schneidereit [:till] from comment #5)
> For now, can you import the tests locally by putting them into a subfolder
> of tests/test262 and make sure that they all pass?

Some tests failed, seems like due to our jstests.py doesn't support "negative:" flag?
https://github.com/tc39/test262/issues/94
Comment 10 User image ziyunfei 2014-09-18 20:40:49 PDT
Created attachment 8491980 [details] [diff] [review]
bug-1069063-v2.patch
Comment 11 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-09-18 21:06:50 PDT
Yes, we don't support @negative (or negative: if they changed the format on us, maybe).  Thus far we've worked around this by just skipping such tests in js/src/tests/jstests.list.  Long run we won't, but it was a quick and dirty way to get many tests imported, even if not all could run.
Comment 12 User image Till Schneidereit [till] 2014-09-19 11:50:20 PDT
Comment on attachment 8491980 [details] [diff] [review]
bug-1069063-v2.patch

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

Great, thanks. The patch is entirely ok, but I'll still cancel review instead r+'ing for now: we need to figure out what to do with the test262 tests, and I have to talk to other people about whether we want to make this Nightly-only for now. My guess would be "yes". I'll get back to you once I have an answer to these questions.
Comment 13 User image John-David Dalton 2014-09-19 16:43:54 PDT
`Array.prototype.contains.length` should be 1, did I miss that?
Comment 14 User image Till Schneidereit [till] 2014-09-20 07:46:04 PDT
(In reply to John-David Dalton from comment #13)
> `Array.prototype.contains.length` should be 1, did I miss that?

I'm not sure I follow. The proposed spec says "The length property of the contains method is 1", and that's what we implement. Check line 12 in the test file. Perhaps the line "JS_SELF_HOSTED_FN("contains", "ArrayContains", 2,0)" is what's confusing here? That's just an artifact of how our function definition works: the number of arguments at that point has to include default arguments, so it's not necessarily what'll be reflected by Function#length.
Comment 15 User image Till Schneidereit [till] 2014-09-20 07:52:29 PDT
Comment on attachment 8491980 [details] [diff] [review]
bug-1069063-v2.patch

(In reply to Till Schneidereit [:till] from comment #12)
> we need to figure out what to do with the test262
> tests

Waldo agrees that we should just leave them out for now. We know that we pass the ones our harness would support (and have no reason whatsoever to assume we wouldn't pass the others), so getting them once they land in test262 proper is enough.

> I have to talk to other people about whether we want to make this
> Nightly-only for now. My guess would be "yes". I'll get back to you once I
> have an answer to these questions.

Waldo agrees again: this should be Nightly-only for now. I'll add the required #ifdef to jsarray.cpp and feature test to contains.js before landing this.
Comment 16 User image John-David Dalton 2014-09-20 09:38:44 PDT
> I'm not sure I follow. The proposed spec says "The length property of the contains method is 1", and that's what we implement.

No worries, the default param threw me off.
Comment 17 User image Till Schneidereit [till] 2014-09-20 16:54:25 PDT
Oh, same issue here as in bug 1062860: please confirm that you're ok with the test's license being changed to https://creativecommons.org/publicdomain/zero/1.0/ so I can land this. (Except if you're explicitly not ok with that for some reason, of course. In that case, please just say that.)
Comment 18 User image ziyunfei 2014-09-20 18:32:18 PDT
(In reply to Till Schneidereit [:till] from comment #17)
> Oh, same issue here as in bug 1062860: please confirm that you're ok with
> the test's license being changed to
> https://creativecommons.org/publicdomain/zero/1.0/ so I can land this.
> (Except if you're explicitly not ok with that for some reason, of course. In
> that case, please just say that.)

Of course I'm ok with that!
Comment 19 User image Till Schneidereit [till] 2014-09-20 19:22:35 PDT
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2c92e43e29d8

(In reply to ziyunfei from comment #18)
> Of course I'm ok with that!

Great, thanks for the quick confirmation.
Comment 21 User image Bobby Holley (:bholley) (busy with Stylo) 2014-09-21 11:24:44 PDT
securityAudit=bholley. Please update the test and make sure to make it conditional on the right things so that the test doesn't go orange when merged to aurora/beta/release.
Comment 22 User image Till Schneidereit [till] 2014-09-21 15:55:17 PDT
And re-landed. Thanks, Bobby!

https://hg.mozilla.org/integration/mozilla-inbound/rev/178df0290ee1
Comment 23 User image ziyunfei 2014-09-21 18:54:31 PDT
(In reply to Till Schneidereit [:till] from comment #22)
> And re-landed. Thanks, Bobby!
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/178df0290ee1

Till, it seems this changeset used the legacy code from patch v1.
Comment 24 User image ziyunfei 2014-09-21 19:11:52 PDT
(In reply to ziyunfei from comment #23)
> (In reply to Till Schneidereit [:till] from comment #22)
> > And re-landed. Thanks, Bobby!
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/178df0290ee1
> 
> Till, it seems this changeset used the legacy code from patch v1.

s/legacy code/obsolete code/

Sorry for my bad english.
Comment 25 User image Till Schneidereit [till] 2014-09-22 10:25:25 PDT
Urgh, thanks for noticing!

Follow-up pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/15aebfcdbde5
Comment 28 User image Jason Orendorff [:jorendorff] 2014-10-01 06:29:14 PDT
Unfortunately, we have to back this out for now because of bug 1075059. There is some hope that we'll be able to re-land it sometime down the road, when MooTools is less widely (or at least less *prominently*) used on the Web.

It's also possible that TC39 will pick a different name for the method, and then we'd be able to land it right away. We'll see.
Comment 29 User image Domenic Denicola 2014-11-19 17:20:40 PST
At the November TC39 meeting we agreed to rename to Array.prototype.includes to dodge this issue. (String.prototype.contains also needs to be renamed, as IE had compat problems with it.) Anyone interested in re-landing under the new name? :D

Spec updated at https://github.com/domenic/Array.prototype.includes
Comment 30 User image ziyunfei 2014-11-19 19:57:31 PST
Created attachment 8525790 [details] [diff] [review]
rename `contains` to `includes`
Comment 31 User image Till Schneidereit [till] 2014-11-20 05:00:37 PST
Comment on attachment 8525790 [details] [diff] [review]
rename `contains` to `includes`

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

Great, thank you for doing this!

Domenic, requesting feedback from you to verify that the rename is the only change here. If it is, just give an f+ and we'll land this.
Comment 33 User image Till Schneidereit [till] 2014-11-20 15:17:54 PST
Comment on attachment 8526368 [details] [diff] [review]
Fix up test_xrayToJS.xul

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

We still want Domenic's feedback here.
Comment 34 User image Domenic Denicola 2014-11-20 15:23:04 PST
Yep, that was the only change. Although in the process of code review over on V8 we found a couple redundant algorithm steps that you might be interested in:

https://github.com/tc39/Array.prototype.includes/commit/4550016b71f13dd5097e2c4ac9d2962a494f41a4?short_path=958e727#diff-958e7270f96f5407d7d980f500805b1b

They do not affect observable behavior though.
Comment 35 User image Till Schneidereit [till] 2014-11-20 15:31:53 PST
Comment on attachment 8526368 [details] [diff] [review]
Fix up test_xrayToJS.xul

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

Great, thanks for the feedback, Domenic.

::: js/src/builtin/Array.js
@@ +596,5 @@
> +
> +    // Steps 6-7.
> +    var n = ToInteger(fromIndex);
> +
> +    // Step 8.

As Domenic points out, this step is redundant (if it's removed, the loop in step 11 will have 0 iterations in the affected cases). Please remove it and fix the following steps' numbering.

@@ +608,5 @@
> +    }
> +    // Step 10.
> +    else {
> +        // Step a.
> +        k = len + n;

We already do this the way the spec does it now, too :)
Comment 36 User image ziyunfei 2014-11-20 20:34:51 PST
Created attachment 8526539 [details] [diff] [review]
remove step 8 and fix steps' numbering
Comment 37 User image ziyunfei 2014-11-20 20:36:11 PST
Got an unrelated failure.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=119d4fb5b66e
Comment 39 User image Carsten Book [:Tomcat] 2014-11-21 04:01:26 PST
https://hg.mozilla.org/mozilla-central/rev/9e692d870578
Comment 40 User image Till Schneidereit [till] 2014-11-21 07:43:44 PST
Followup to actually make this Nightly-only as stated on dev-platform:
https://hg.mozilla.org/integration/mozilla-inbound/rev/160459853756
Comment 41 User image Wes Kocher (:KWierso) 2014-11-21 16:39:03 PST
https://hg.mozilla.org/mozilla-central/rev/160459853756
Comment 42 User image Florian Scholz [:fscholz] (MDN) 2015-01-19 13:13:16 PST
Ms2ger, this looks documented to me...
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes

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