1067049 - The arguments object should support the ES6 @@iterator protocol

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nickolay_Ponomarev]

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

[Till Schneidereit [:till]]

It looks like you're right, yes. Not sure how that'll interact with the JIT optimizations around arguments. Hopefully not at all.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Implement arguments[@@iterator].

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.

Comment 4

[Tom S [:evilpie]]

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.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Implement arguments[@@iterator].

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'

Comment 6

[Tom S [:evilpie]]

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?

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Implement arguments[@@iterator].

(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?

Comment 8

[Tooru Fujisawa [:arai]]

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.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Implement arguments[@@iterator].

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.

Comment 10

[Tom S [:evilpie]]

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.

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7aa719c43c9639680f4d525b4ad7175e93b459d
Bug 1067049 - Implement arguments[@@iterator]. r=evilpie

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f7aa719c43c9

Comment 13

[Tooru Fujisawa [:arai]]

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

Comment 14

[Florian Scholz (Open Web Docs)]

Thanks :arai!