The arguments object should support the ES6 @@iterator protocol

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: asqueella, Assigned: arai)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla46
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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
It looks like you're right, yes. Not sure how that'll interact with the JIT optimizations around arguments. Hopefully not at all.
Blocks: 694100
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Duplicate of this bug: 1232929
(Assignee)

Comment 3

3 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)
(Assignee)

Updated

3 years ago
See Also: → bug 875433
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

3 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 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

3 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

3 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)

Updated

3 years ago
Depends on: 1235656
(Assignee)

Comment 9

3 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 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+

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7aa719c43c9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Thanks :arai!
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1263811
You need to log in before you can comment on or make changes to this bug.