Closed Bug 1069063 Opened 10 years ago Closed 10 years ago

Implement Array.prototype.includes

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: till, Assigned: 446240525)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS] )

Attachments

(1 file, 4 obsolete files)

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
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.
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
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Attached patch bug1069063.patch (obsolete) — Splinter Review
Assignee: nobody → 446240525
Attachment #8491350 - Flags: review?(till)
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 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.
Attachment #8491350 - Flags: review?(till)
> +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);
(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.)
(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.
(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
Attached patch bug-1069063-v2.patch (obsolete) — Splinter Review
Attachment #8491350 - Attachment is obsolete: true
Attachment #8491980 - Flags: review?(till)
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 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.
Attachment #8491980 - Flags: review?(till)
`Array.prototype.contains.length` should be 1, did I miss that?
(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 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.
Attachment #8491980 - Flags: review+
> 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.
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.)
Status: NEW → ASSIGNED
Flags: needinfo?(446240525)
(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!
Flags: needinfo?(446240525)
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.
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.
Blocks: 1070767
(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.
(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.
Urgh, thanks for noticing!

Follow-up pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/15aebfcdbde5
https://hg.mozilla.org/mozilla-central/rev/2c92e43e29d8
https://hg.mozilla.org/mozilla-central/rev/15aebfcdbde5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1075059
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.
Blocks: es7
No longer blocks: es6
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
Status: RESOLVED → REOPENED
Component: JavaScript Engine → JavaScript: Standard Library
Resolution: FIXED → ---
Summary: Implement Array.prototype.contains → Implement Array.prototype.includes
Whiteboard: [DocArea=JS] → [DocArea=JS]
Target Milestone: mozilla35 → mozilla36
Attached patch rename `contains` to `includes` (obsolete) — Splinter Review
Attachment #8491980 - Attachment is obsolete: true
Attachment #8525790 - Flags: review?(till)
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.
Attachment #8525790 - Flags: review?(till)
Attachment #8525790 - Flags: review+
Attachment #8525790 - Flags: feedback?(d)
Attached patch Fix up test_xrayToJS.xul (obsolete) — Splinter Review
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7a4033ae52f4
Attachment #8525790 - Attachment is obsolete: true
Attachment #8525790 - Flags: feedback?(d)
Attachment #8526368 - Flags: review+
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.
Attachment #8526368 - Flags: feedback?(d)
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 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 :)
Attachment #8526368 - Flags: feedback?(d) → feedback+
Attachment #8526368 - Attachment is obsolete: true
Attachment #8526539 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9e692d870578
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Followup to actually make this Nightly-only as stated on dev-platform:
https://hg.mozilla.org/integration/mozilla-inbound/rev/160459853756
You need to log in before you can comment on or make changes to this bug.