875433 - Array.prototype[@@iterator] should be the same function object as Array.prototype.values

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brandon Benvie [:benvie]]

The ES6 spec states:

> Array.prototype[@@iterator]()
> The initial value of the @@iterator property is the same function object as the initial value of the Array.prototype.values property.

> Map.prototype[@@iterator]()
> The initial value of the @@iterator property is the same function object as the initial value of the Map.prototype.entries property.

> Set.prototype[@@iterator ]()
> The initial value of the @@iterator property is the same function object as the initial value of the Set.prototype.values property.


Ignoring the fact that SpiderMonkey currently isn't using a Symbol for the @@iterator property under the assumption that the string keyed "iterator" is a temporary stand-in for it, these departures from the ES6 spec can be observed:

> Array.prototype.entries !== Array.prototype.iterator;
> Array.prototype.iterator.name !== "entries";

> Map.prototype.entries !== Map.prototype.iterator;
> Map.prototype.iterator.name !== "entries";

(Set.{keys, values, entries} is not implemented yet, bug 869996)

Comment 1

[Brandon Benvie [:benvie]]

Apologies, a correction. This should be observed for Array:

> Array.prototype.values !== Array.prototype.iterator;
> Array.prototype.iterator.name !== "values";

Comment 2

[Sankha Narayan Guria [:sankha]]

Attachment: patch for Array.prototype

This should patch should fix the Array.prototype deviations from the ES6 spec.

The rest of the problem for Map and Set should go away once the patch on the bug 869996 has been landed.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d35dc039af7

Comment 4

[Jason Orendorff [:jorendorff]]

Adding Array.prototype.values caused some surprising compatibility issues (bug 881782, bug 883914).  I think we need to do a partial backout before FF24.  I intend to do so tomorrow unless the plot develops further.

I've mentioned this to es-discuss, as well -- the method we added that seems to be causing the problems is standard-track. Presumably that will change.

Comment 5

[Jason Orendorff [:jorendorff]]

Backed out for now. No real discussion on es-discuss yet.
  https://hg.mozilla.org/integration/mozilla-inbound/rev/38f0844975f2

Comment 6

[Ed Morley [:emorley]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Backed out for now. No real discussion on es-discuss yet.
>   https://hg.mozilla.org/integration/mozilla-inbound/rev/38f0844975f2

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/38f0844975f2

Comment 7

[Daniel Holbert [:dholbert]]

[clearing milestone and in-testsuite, since this has been backed out]

Comment 8

[Jason Orendorff [:jorendorff]]

I'm sure Sencha is working on fixing Ext JS but that won't fix existing web sites without further outreach.

The attitude on es-discuss seems to be "wait and see", so this could be in limbo for a long time. Setting need-info from dherman, one of our TC39 members.

Comment 9

[Brandon Benvie [:benvie]]

Having Array.prototype.values has limited usefulness as long as you have Array.prototype.@@iterator (or just "iterator" in our case currently). Perhaps we could spin this bug off into two bugs: one for Array.prototype.values which is in limbo, and one for the rest which is not.

Comment 10

[Brandon Benvie [:benvie]]

Oh nevermind, Map/Set was done elsewhere. Ignore my last comment.

Comment 11

[Jason Orendorff [:jorendorff]]

Trying again, now that FF24 has branched.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4426d926b31

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please use hg out to check the commit message before you push. This one got a bit munged.

Comment 13

[Daniel Holbert [:dholbert]]

So, is there a plan to address the site regressions from comment 4?  Are those going to become tech evang issues?

Comment 14

[Till Schneidereit [:till]]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> So, is there a plan to address the site regressions from comment 4?  Are
> those going to become tech evang issues?

Sencha is aware of the issue and working on a fix. Once they have that, a roll out to regressing sites is hopefully going to be fairly painless, and hence quick. Hopefully.

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b4426d926b31

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22c3433bdbc9

Comment 17

[Daniel Holbert [:dholbert]]

Just for completeness, and because it confused someone on another bug, here's the m-c changeset for Comment 16's backout:
 https://hg.mozilla.org/mozilla-central/rev/22c3433bdbc9

Comment 18

[Daniel Holbert [:dholbert]]

Should this bug be closed (or morphed?) as a result of bug 881782 comment 22?

Comment 19

[Jason Orendorff [:jorendorff]]

No, keep this open.

TC39 is designing a truly gruesome workaround for this. Eventually it'll be done and we'll implement, and then we can re-land the patch here.

Comment 20

[Till Schneidereit [:till]]

We can try relanding this once bug 1054759 is fixed.

Comment 21

[Tom S]

Next time let's land this together with bug 1258163.

Comment 22

[Tom S]

Attachment: Add Array.prototype.values

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8732611 [details] [diff] [review]
Add Array.prototype.values

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

r=me. I had this in my patch for a while, but didn't realize we had _SetCanonicalName -- so I didn't expect it to work -- so I removed it. Nice!

Comment 24

[Jason Orendorff [:jorendorff]]

I'll land this when I reland @@unscopables, hopefully Monday.

Comment 25

[Tom S]

I think I forgot to add values to test_xrayToJS.xul.

Comment 26

[Jason Orendorff [:jorendorff]]

Yeah, I expected that. I'll add it before pushing to the try server.

Comment 27

[Jason Orendorff [:jorendorff]]

Attachment: Implement Array.prototype.values

Comment 28

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=068a8afc645a

Comment 29

[Bobby Holley (:bholley)]

Comment on attachment 8732889 [details] [diff] [review]
Implement Array.prototype.values

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

r=me on the xray bits. Everything else looks fine, but I don't know the spec well enough to confirm that this matches it.

Comment 30

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d098511959e1

Comment 31

[Kohei Yoshino]

I added the site-compat keyword 2 years ago but is there still any chance of breaking site compatibility here?

Comment 32

[Jason Orendorff [:jorendorff]]

Not with @@unscopables in place, no.

Comment 33

[Kohei Yoshino]

Then removing the keyword. Thanks.

Comment 34

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee890a109435989837df6bf7b860203bbe80ff4b
Bug 875433 - Implement Array.prototype.values. r=jorendorff.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee890a109435

Comment 36

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/48#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/values