Closed
Bug 1067049
Opened 10 years ago
Closed 8 years ago
The arguments object should support the ES6 @@iterator protocol
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: asqueella, Assigned: arai)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])
Attachments
(1 file, 3 obsolete files)
16.52 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
As far as I understand the ES6 spec (CreateUnmappedArgumentsObject / CreateMappedArgumentsObject -- http://people.mozilla.org/~jorendorff/es6-draft.html#sec-createunmappedargumentsobject ), the arguments object is supposed to have the @@iterator property: var std_iterator = Symbol.iterator; // until bug 918828 is fixed, try this instead: // var std_iterator = "@@iterator"; (function() { return arguments[std_iterator]() }) ("params") Instead the above code results in: TypeError: arguments[std_iterator] is not a function
Comment 1•10 years ago
|
||
It looks like you're right, yes. Not sure how that'll interact with the JIT optimizations around arguments. Hopefully not at all.
Blocks: es6
Updated•10 years ago
|
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Assignee | ||
Comment 3•8 years ago
|
||
Added @@iterator support to following functions: * MappedArgumentsObject::obj_resolve * MappedArgumentsObject::obj_enumerate * UnmappedArgumentsObject::obj_resolve * UnmappedArgumentsObject::obj_enumerate In newly added testcases (arguments-iterator.js), one test is disabled for now because Array.prototype[@@iterator].name is not "values" (bug 875433). Also, fixed existing 3 tests that assumed arguments is non-iterable.
Assignee: nobody → arai.unmht
Attachment #8702222 -
Flags: review?(evilpies)
Comment 4•8 years ago
|
||
Thank you for working on this! We will probably have to something like "hasOverriddenLength". I assume this will currently fail: assertEq(Symbol.iterator in arguments, true); delete arguments[Symbol.iterator]; assertEq(Symbol.iterator in arguments, false); // Resolved again here.
Assignee | ||
Comment 5•8 years ago
|
||
Thank you for pointing that out :) Changed following: * Added hasOverriddenIterator and markIteratorOverridden, as same as .length * Use 2nd lowest bit of INITIAL_LENGTH_SLOT to indicate whether [@@iterator] is overridden * Do nothing for @@iterator in obj_resolve when hasOverriddenIterator() is true * Call markIteratorOverridden in NativeDefineProperty * Added 3 more testcases for 'delete'
Attachment #8702222 -
Attachment is obsolete: true
Attachment #8702222 -
Flags: review?(evilpies)
Attachment #8702297 -
Flags: review?(evilpies)
Comment 6•8 years ago
|
||
Comment on attachment 8702297 [details] [diff] [review] Implement arguments[@@iterator]. Review of attachment 8702297 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/ecma_6/Function/arguments-iterator.js @@ +17,5 @@ > + function(a, b, c) { > + arguments[Symbol.iterator] = 10; > + delete arguments[Symbol.iterator]; > + assertEq(Symbol.iterator in arguments, false); > + }, Please also add a test here and below that uses Object.defineProperty with/without delete. ::: js/src/vm/ArgumentsObject.cpp @@ +403,5 @@ > > +static bool > +DefineArgumentsIterator(JSContext* cx, Handle<ArgumentsObject*> argsobj) > +{ > + if (argsobj->hasOverriddenIterator()) This has to move inside the callers, otherwise resolvedp is wrong. @@ +407,5 @@ > + if (argsobj->hasOverriddenIterator()) > + return true; > + > + HandlePropertyName name = cx->names().ArrayValues; > + // funName should be ignored, as ArrayValues should be cached. Why is that? Does it always become cached during initialization?
Attachment #8702297 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #6) > @@ +407,5 @@ > > + if (argsobj->hasOverriddenIterator()) > > + return true; > > + > > + HandlePropertyName name = cx->names().ArrayValues; > > + // funName should be ignored, as ArrayValues should be cached. > > Why is that? Does it always become cached during initialization? Thank you again, it turns out that it was not true. I had to revolve Array ctor. Discusses in IRC, and added ensureConstructor with JSProto_Array into DefineArgumentsIterator, to cache ArrayValues and name it correctly, and replaced getSelfHostedFunction with existingIntrinsicValue, to get cached function value directly. Also, added a testcase to check arguments[@@iteratpr].name in newGlobal().eval without any Array literal in the script, that is the case we should resolve Array ctor/proto in resolve hook. It now compares with "[Symbol.iterator]", this test will fail once Array.prototype.values is implemented, but it should be better to check this again at that point. Can you review that part?
Attachment #8702297 -
Attachment is obsolete: true
Attachment #8702699 -
Flags: review?(evilpies)
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8702699 [details] [diff] [review] Implement arguments[@@iterator]. Sorry, I'll change existingIntrinsicValue to getSelfHostedFunction again, with some more assertion inside getSelfHostedFunction. will post updated patch shortly.
Attachment #8702699 -
Flags: review?(evilpies)
Assignee | ||
Comment 9•8 years ago
|
||
Okay, the self-hosted function name issue is solved in bug 1235656, and Array.prototype[@@iterator].name is always "values", so we can pass cx->names().values in DefineArgumentsIterator, and also we can test it :) no other changes.
Attachment #8702699 -
Attachment is obsolete: true
Attachment #8703456 -
Flags: review?(evilpies)
Comment 10•8 years ago
|
||
Comment on attachment 8703456 [details] [diff] [review] Implement arguments[@@iterator]. Review of attachment 8703456 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArgumentsObject.cpp @@ +472,5 @@ > id = NameToId(cx->names().callee); > if (!HasProperty(cx, argsobj, id, &found)) > return false; > > + id = SYMBOL_TO_JSID(cx->wellKnownSymbols().iterator); I believe this needs to move before callee. In ES6 property insertion order is specified. 9.4.4.6 has length, @@iterator, callee, caller. @@ +609,5 @@ > if (!HasProperty(cx, argsobj, id, &found)) > return false; > > + id = SYMBOL_TO_JSID(cx->wellKnownSymbols().iterator); > + if (!HasProperty(cx, argsobj, id, &found)) Same here, maybe addd a simple Object.getOwnPropertyKeys(arguments) test.
Attachment #8703456 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7aa719c43c9639680f4d525b4ad7175e93b459d Bug 1067049 - Implement arguments[@@iterator]. r=evilpie
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f7aa719c43c9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Assignee | ||
Comment 13•8 years ago
|
||
Updated following documents: https://developer.mozilla.org/en-US/Firefox/Releases/46 https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments/@@iterator
You need to log in
before you can comment on or make changes to this bug.
Description
•