1225396 - Fix ES6 Iterator prototype chains

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Seems like IteratorIdentity as inherited from %IteratorPrototype% should do the job fine, no?  These are not in the spec, precisely for that reason I assume.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ah, looks like the thing on %IteratorPrototype% is something weird: it doesn't actually have IteratorIdentity as its Symbol.iterator.  :(

Comment 2

[Jason Orendorff [:jorendorff]]

Exactly. :-|

Comment 3

[Jan de Mooij [:jandem]]

Jason, can we disentangle Iterator.prototype and %IteratorPrototype% by using a different object for Iterator.prototype (say, LegacyIteratorPrototype) with LegacyIteratorPrototype[Symbol.iterator] == LegacyIteratorShim? Then we can fix %IteratorPrototype%[Symbol.iterator] to do the right thing...

Comment 4

[Jan de Mooij [:jandem]]

(This is causing some failures on kangax's ES6 table; if my suggestion in comment 3 works I can take a stab at this.)

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Rename *Iterator stuff to *LegacyIterator

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Clean up Iterator prototypes

This patch:

* Adds a new %IteratorPrototype% object, different from Iterator.prototype.
* Makes Array/String/Map/Set iterators inherit from it.
* Removes the Symbol.iterator properties from these iterators; it's now inherited from %IteratorPrototype%.
* Changes %MapIteratorPrototype% and %SetIteratorPrototype% to be plain objects instead of MapIterator/SetIterator.
* Fixes tests, adds some new tests.

Sorry, this is a bunch of slightly different changes, but it's all related and nothing here is very complicated I think.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Jan, I expect that you can also drop the hunk of code at http://hg.mozilla.org/mozilla-central/file/d08afef8b42d/dom/bindings/Codegen.py#l2272 now, right?  And in general it sounds like your patches fix bug 1091945 and bug 1201089.

Comment 8

[Boris Zbarsky [:bzbarsky]]

And bug 1225395.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Jan, I expect that you can also drop the hunk of code at
> http://hg.mozilla.org/mozilla-central/file/d08afef8b42d/dom/bindings/Codegen.
> py#l2272 now, right?

Ah thanks! I wasn't aware of that code, I'll look into it.

> And in general it sounds like your patches fix bug 1091945 and bug 1201089.

Hm reading those bugs I realized I also should fix the generator prototype chain while I'm at it. I'll work on another patch for that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Ah thanks! I wasn't aware of that code, I'll look into it.

So the reason I think you can get rid of it is that http://hg.mozilla.org/mozilla-central/file/d08afef8b42d/dom/bindings/Codegen.py#l554 puts the return value of JS_GetIteratorPrototype on the proto chain of the things that the @@iterator is being added to in the hunk I link to in comment 7.  Looks like your patch 2 changes that to return the real %IteratorPrototype%, so things should be good.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Make %GeneratorPrototype% inherit from %IteratorPrototype%

... and remove @@iterator from %GeneratorPrototype%.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Remove @@iterator workaround in Codegen.py

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8696602 [details] [diff] [review]
Part 4 - Remove @@iterator workaround in Codegen.py

r=me

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8696598 [details] [diff] [review]
Part 2 - Clean up Iterator prototypes

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

::: js/src/jit-test/tests/for-of/string-iterator-surfaces.js
@@ +52,5 @@
>  // Test StringIterator.prototype surface
>  var iter = ""[Symbol.iterator]();
>  var iterProto = Object.getPrototypeOf(iter);
>  
> +// StringIterator.prototype inherits from %IteratorPrototype. Check it's the

missing the trailing % here

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a87be824cc8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0eb91b63bf26
https://hg.mozilla.org/integration/mozilla-inbound/rev/de72e2291ae8
https://hg.mozilla.org/integration/mozilla-inbound/rev/151ce2b0e3f6

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> This is causing some failures on kangax's ES6 table

I verified these patches fix the "{Array,String,Map,Set} iterator prototype chain" tests.

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a87be824cc8f
https://hg.mozilla.org/mozilla-central/rev/0eb91b63bf26
https://hg.mozilla.org/mozilla-central/rev/de72e2291ae8
https://hg.mozilla.org/mozilla-central/rev/151ce2b0e3f6