Implement Array.prototype.includes

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: till, Assigned: ziyunfei)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla36
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS] , URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

3 years ago
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]
(Assignee)

Comment 3

3 years ago
Created attachment 8491350 [details] [diff] [review]
bug1069063.patch
Assignee: nobody → 446240525
Attachment #8491350 - Flags: review?(till)
(Assignee)

Comment 4

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

Comment 5

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

Comment 8

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

Comment 9

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

Comment 10

3 years ago
Created attachment 8491980 [details] [diff] [review]
bug-1069063-v2.patch
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.
(Reporter)

Comment 12

3 years ago
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)

Comment 13

3 years ago
`Array.prototype.contains.length` should be 1, did I miss that?
(Reporter)

Comment 14

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

Comment 15

3 years ago
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+

Comment 16

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

Comment 17

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

Comment 18

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

Comment 19

3 years ago
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.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c902ebaa8520 for https://tbpl.mozilla.org/php/getParsedLog.php?id=48543781&tree=Mozilla-Inbound and to let you guys figure out whether https://tbpl.mozilla.org/php/getParsedLog.php?id=48543667&tree=Mozilla-Inbound was from this or from bug 1062860.
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.
(Reporter)

Updated

3 years ago
Blocks: 1070767
(Reporter)

Comment 22

3 years ago
And re-landed. Thanks, Bobby!

https://hg.mozilla.org/integration/mozilla-inbound/rev/178df0290ee1
(Assignee)

Comment 23

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

Comment 24

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

Comment 25

3 years ago
Urgh, thanks for noticing!

Follow-up pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/15aebfcdbde5
(Assignee)

Comment 26

3 years ago
https://hg.mozilla.org/mozilla-central/rev/2c92e43e29d8
https://hg.mozilla.org/mozilla-central/rev/15aebfcdbde5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(Assignee)

Comment 27

3 years ago
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/contains
Keywords: dev-doc-needed → dev-doc-complete
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.
(Assignee)

Updated

3 years ago
Blocks: 1021376
No longer blocks: 694100

Comment 29

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

Updated

3 years ago
Status: RESOLVED → REOPENED
Component: JavaScript Engine → JavaScript: Standard Library
Keywords: dev-doc-complete → dev-doc-needed
Resolution: FIXED → ---
Summary: Implement Array.prototype.contains → Implement Array.prototype.includes
Whiteboard: [DocArea=JS] → [DocArea=JS]
Target Milestone: mozilla35 → mozilla36
(Assignee)

Comment 30

3 years ago
Created attachment 8525790 [details] [diff] [review]
rename `contains` to `includes`
Attachment #8491980 - Attachment is obsolete: true
Attachment #8525790 - Flags: review?(till)
(Reporter)

Comment 31

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

Comment 32

3 years ago
Created attachment 8526368 [details] [diff] [review]
Fix up test_xrayToJS.xul

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+
(Reporter)

Comment 33

3 years ago
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)

Comment 34

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

Comment 35

3 years ago
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+
(Assignee)

Comment 36

3 years ago
Created attachment 8526539 [details] [diff] [review]
remove step 8 and fix steps' numbering
Attachment #8526368 - Attachment is obsolete: true
Attachment #8526539 - Flags: review+
(Assignee)

Comment 37

3 years ago
Got an unrelated failure.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=119d4fb5b66e
Keywords: dev-doc-needed → checkin-needed, dev-doc-complete
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e692d870578
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e692d870578
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 40

3 years ago
Followup to actually make this Nightly-only as stated on dev-platform:
https://hg.mozilla.org/integration/mozilla-inbound/rev/160459853756
https://hg.mozilla.org/mozilla-central/rev/160459853756

Updated

2 years ago
Keywords: dev-doc-complete → dev-doc-needed
Ms2ger, this looks documented to me...
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.