Closed
Bug 1225396
Opened 6 years ago
Closed 5 years ago
Fix ES6 Iterator prototype chains
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
13.44 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
20.66 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
3.50 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
1.76 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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?
![]() |
Reporter | |
Updated•6 years ago
|
Flags: needinfo?
![]() |
Reporter | |
Updated•6 years ago
|
Flags: needinfo?(jorendorff)
![]() |
Reporter | |
Comment 1•6 years ago
|
||
Ah, looks like the thing on %IteratorPrototype% is something weird: it doesn't actually have IteratorIdentity as its Symbol.iterator. :(
Assignee | ||
Comment 3•5 years ago
|
||
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)
Assignee | ||
Comment 4•5 years ago
|
||
(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 | ||
Comment 5•5 years ago
|
||
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jorendorff)
Attachment #8696595 -
Flags: review?(jorendorff)
Assignee | ||
Comment 6•5 years ago
|
||
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)
![]() |
Reporter | |
Comment 7•5 years ago
|
||
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.
![]() |
Reporter | |
Comment 8•5 years ago
|
||
And bug 1225395.
Assignee | ||
Comment 9•5 years ago
|
||
(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.
![]() |
Reporter | |
Comment 10•5 years ago
|
||
> 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.
Assignee | ||
Comment 11•5 years ago
|
||
... and remove @@iterator from %GeneratorPrototype%.
Attachment #8696601 -
Flags: review?(jorendorff)
Assignee | ||
Comment 12•5 years ago
|
||
Attachment #8696602 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•5 years ago
|
Blocks: es6
Summary: Why do we need ArrayIteratorIdentity and StringIteratorIdentity? → Fix ES6 Iterator prototype chains
![]() |
Reporter | |
Comment 16•5 years ago
|
||
Comment on attachment 8696602 [details] [diff] [review] Part 4 - Remove @@iterator workaround in Codegen.py r=me
Attachment #8696602 -
Flags: review?(bzbarsky) → review+
Updated•5 years ago
|
Attachment #8696595 -
Flags: review?(jorendorff) → review+
Comment 17•5 years ago
|
||
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+
Updated•5 years ago
|
Attachment #8696601 -
Flags: review?(jorendorff) → review+
Comment 18•5 years ago
|
||
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
Assignee | ||
Comment 19•5 years ago
|
||
(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•5 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•