Closed Bug 1225396 Opened 4 years ago Closed 4 years ago

Fix ES6 Iterator prototype chains

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: bzbarsky, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.
Flags: needinfo?
Flags: needinfo?
Flags: needinfo?(jorendorff)
Ah, looks like the thing on %IteratorPrototype% is something weird: it doesn't actually have IteratorIdentity as its Symbol.iterator.  :(
Depends on: 1201089
Exactly. :-|
Flags: needinfo?(jorendorff)
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...
Flags: needinfo?(jorendorff)
(This is causing some failures on kangax's ES6 table; if my suggestion in comment 3 works I can take a stab at this.)
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jorendorff)
Attachment #8696595 - Flags: review?(jorendorff)
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.
Attachment #8696598 - Flags: review?(jorendorff)
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.
(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.
> 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.
... and remove @@iterator from %GeneratorPrototype%.
Attachment #8696601 - Flags: review?(jorendorff)
Duplicate of this bug: 1201089
Duplicate of this bug: 1225395
Duplicate of this bug: 1091945
Blocks: es6
Summary: Why do we need ArrayIteratorIdentity and StringIteratorIdentity? → Fix ES6 Iterator prototype chains
Comment on attachment 8696602 [details] [diff] [review]
Part 4 - Remove @@iterator workaround in Codegen.py

r=me
Attachment #8696602 - Flags: review?(bzbarsky) → review+
Attachment #8696595 - Flags: review?(jorendorff) → review+
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
Attachment #8696598 - Flags: review?(jorendorff) → review+
Attachment #8696601 - Flags: review?(jorendorff) → review+
(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.
Depends on: 1232676
Depends on: 1233115
Depends on: 1233152
No longer depends on: 1232676
No longer depends on: 1233115
No longer depends on: 1233152
Depends on: 1236476
Depends on: 1236473
No longer depends on: 1236473
Blocks: 1232229
Depends on: 1252903
No longer depends on: 1252903
You need to log in before you can comment on or make changes to this bug.