Arguments object has @@iterator

Arguments object has @@iterator

R=arv@chromium.org, verwaest@chromium.org, rossberg@chromium.org
BUG=v8:3391
LOG=N
TEST=mjsunit/harmony/arguments-iterator.js

Committed: https://code.google.com/p/v8/source/detail?r=23341

[wingo, 2014-06-17 11:31:55]

PTAL.  This deviates from the spec in two ways:

 1) The @@iterator property appears after the other properties, in iteration
order

 2) The @@iterator property is an accessor property, not a data property.

I think we can probably argue for inclusion of these changes in the spec.

[wingo, 2014-06-25 07:23:17]

Ping :)

[arv (Not doing code reviews), 2014-06-25 15:55:05]

FYI.

https://codereview.chromium.org/342453002/diff/1/src/array-iterator.js
File src/array-iterator.js (right):

https://codereview.chromium.org/342453002/diff/1/src/array-iterator.js#newcod...
src/array-iterator.js:152: get: ArgumentsIteratorGetter,
It is not clear to me why this cannot be a data property?

With the current code, what does the descriptor look like for?

Object.getOwnPropertyDescriptor(arguments, Symbol.iterator)

[wingo, 2014-06-25 16:16:51]

On 2014/06/25 15:55:05, arv wrote:
> src/array-iterator.js:152: get: ArgumentsIteratorGetter,
> It is not clear to me why this cannot be a data property?

It would be a per-object memory penalty, unless we can move the property into
the map somehow.  If it is in the object, it complicates matters for the
materializer, and is a not insignificant change implementation-wise.

> With the current code, what does the descriptor look like for?
> 
> Object.getOwnPropertyDescriptor(arguments, Symbol.iterator)

{get: function ArgumentsIteratorGetter() { [native code] }, set: function
ArgumentsIteratorSetter() { [native code] }, enumerable: false, configurable:
true}

[rossberg, 2014-06-26 15:51:53]

I agree that implementing it as a data property would be rather painful. I think
it's fine to do this as an accessor for the time being. We can try to lobby for
making that legal in the spec, although I kind of doubt that will be successful.
(If you ask me, arguments iterators should be removed from the spec altogether.)

https://codereview.chromium.org/342453002/diff/1/test/mjsunit/harmony/argumen...
File test/mjsunit/harmony/arguments-iterator.js (right):

https://codereview.chromium.org/342453002/diff/1/test/mjsunit/harmony/argumen...
test/mjsunit/harmony/arguments-iterator.js:39: assertIteratorResult(void 0,
true, iterator.next());
Nit: no reason to use void 0 instead of undefined in tests.

https://codereview.chromium.org/342453002/diff/1/test/mjsunit/harmony/argumen...
test/mjsunit/harmony/arguments-iterator.js:120: function TestIndirectValues4(a,
b, c) {
It would be good to also test that

- changing an arguments slot is reflected in the iterator
- changing a sloppy arguments slot through an aliased parameter variable is
reflected in the iterator
- changing a parameter variable does not effect strict arguments iteration

[wingo, 2014-08-12 16:11:37]

Updated to add requested tests, and to make @@iterator a data property using API
accessors instead of a JS accessor property.  It still appears in the wrong
insertion order though.  Depends on the API symbol accessor patch.

[arv (Not doing code reviews), 2014-08-12 16:31:53]

Andreas is out this week. Toon, can you take a look?

LGTM except for the test change.

This is good stuff.

https://codereview.chromium.org/342453002/diff/20001/test/mjsunit/es6/argumen...
File test/mjsunit/es6/arguments-iterator.js (right):

https://codereview.chromium.org/342453002/diff/20001/test/mjsunit/es6/argumen...
test/mjsunit/es6/arguments-iterator.js:11:
assertTrue(arguments.hasOwnProperty(Symbol.iterator));
Can you change this to?

var descr = Object.getOwnPropertyDescriptor(arguments, Symbol.iterator);
assertTrue(descr.writable);
assertTrue(descr.configurable);
assertFalse(descr.enumerable);
assertEquals(Array.prototype.values, descr.value);

The current test does not cover whether this is a data property or an accessor.

[Toon Verwaest, 2014-08-18 13:47:25]

https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184
src/accessors.cc:184: MaybeHandle<Object> maybe = Object::SetDataProperty(&it,
value);
This doesn't seem right to me, if only because the setter could have been called
through a derived dictionary-mode object for which the iterator will stop on the
dictionary mode object; and HasProperty will be false.

What's the expected behavior exactly? Can't you just use SetProperty(&it,
value), or SetOwnPropertyIgnoreAttributes?

[arv (Not doing code reviews), 2014-08-18 14:16:56]

https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184
src/accessors.cc:184: MaybeHandle<Object> maybe = Object::SetDataProperty(&it,
value);
On 2014/08/18 13:47:25, Toon Verwaest wrote:
> This doesn't seem right to me, if only because the setter could have been
called
> through a derived dictionary-mode object for which the iterator will stop on
the
> dictionary mode object; and HasProperty will be false.
> 
> What's the expected behavior exactly? Can't you just use SetProperty(&it,
> value), or SetOwnPropertyIgnoreAttributes?

The intent is to replace the existing interceptor with a plain old data
property.

[wingo, 2014-08-20 15:46:57]

https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184
src/accessors.cc:184: MaybeHandle<Object> maybe = Object::SetDataProperty(&it,
value);
On 2014/08/18 13:47:25, Toon Verwaest wrote:
> What's the expected behavior exactly? Can't you just use SetProperty(&it,
> value), or SetOwnPropertyIgnoreAttributes?

As Erik says the intent is that arguments[@@iterator] is just a data property. 
It has to be implemented using API accessor callbacks because we know what the
value should be for it, and we don't want to bloat the accessor object with a
different word.  But if someone does arguments[@@iterator] = 10, it should
replace the API accessors with a normal data property.

[wingo, 2014-08-20 15:55:39]

Um,

On 2014/08/20 15:46:57, wingo wrote:
> It has to be implemented using API accessor callbacks because we know what the
> value should be for it, and we don't want to bloat the accessor object with a
> different word.

Thinko -- I meant to say "we don't want to bloat the arguments object with an
additional word"

[Toon Verwaest, 2014-08-21 11:07:20]

https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184
src/accessors.cc:184: MaybeHandle<Object> maybe = Object::SetDataProperty(&it,
value);
To be clear what I meant, what about:

o = {__proto__:arguments};
// Make o dict-mode.
%OptimizeObjectForAddingMultipleProperties(o, 0);
o[@@iterator] = 10;

The setter will be called with a slow-mode o, for which (without calling
Next()), IsFound() is true while HasProperty is false.

This sucks about ExecutableAccessorInfo: it looks like a data property but
behaves like an accessor. That's why SetPropertyOnInstanceIfInherited is used as
a hack; which by itself isn't fully correct with regard to non-extensible
objects in strict-mode.

Or even worse, what about:
String.prototype.__proto__ = arguments;
"bla"[@@iterator] = 10;

https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode191
src/accessors.cc:191: return MakeAccessor(isolate,
isolate->factory()->length_string(),
Ok, in that case this is part of what confused me. This shouldn't be
length-string, but rather @@iterator, right?

[wingo, 2014-08-21 12:56:50]

On 2014/08/21 11:07:20, Toon Verwaest wrote:
> https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc
> File src/accessors.cc (right):
> 
>
https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184
> src/accessors.cc:184: MaybeHandle<Object> maybe = Object::SetDataProperty(&it,
> value);
> To be clear what I meant, what about:
> 
> o = {__proto__:arguments};
> // Make o dict-mode.
> %OptimizeObjectForAddingMultipleProperties(o, 0);
> o[@@iterator] = 10;
> 
> The setter will be called with a slow-mode o, for which (without calling
> Next()), IsFound() is true while HasProperty is false.

Yes, I see what you mean.  Thanks for catching this.

> Or even worse, what about:
> String.prototype.__proto__ = arguments;
> "bla"[@@iterator] = 10;

In this case String.prototype already has an @@iterator; but yeah, I know what
you mean.

>
https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode191
> src/accessors.cc:191: return MakeAccessor(isolate,
> isolate->factory()->length_string(),
> Ok, in that case this is part of what confused me. This shouldn't be
> length-string, but rather @@iterator, right?

Good catch, will fix.  (Makes you wonder what this is for.)

[wingo, 2014-08-21 15:33:04]

Updated patch fixes { __proto__=arguments }[Symbol.iterator] = 10 case.  It's
pretty gross and I probably misunderstood things.  PTAL.

[Toon Verwaest, 2014-08-22 11:01:02]

https://codereview.chromium.org/342453002/diff/70008/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/342453002/diff/70008/src/accessors.cc#newcode180
src/accessors.cc:180: LookupIterator it(object, Utils::OpenHandle(*name));
What about just

if (SetPropertyOnInstanceIfInherited(isolate, info, name, value)) {
  return;
}

LookupIterator it(object, name);
CHECK(it.HasProperty());
DCHECK(it.HolderisReceiverOrHiddenPrototype());
Object::SetDataProperty(&it, value);

[wingo, 2014-08-25 08:42:30]

https://codereview.chromium.org/342453002/diff/70008/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/342453002/diff/70008/src/accessors.cc#newcode180
src/accessors.cc:180: LookupIterator it(object, Utils::OpenHandle(*name));
On 2014/08/22 11:01:02, Toon Verwaest wrote:
> What about just
> 
> if (SetPropertyOnInstanceIfInherited(isolate, info, name, value)) {
>   return;
> }
> 
> LookupIterator it(object, name);
> CHECK(it.HasProperty());
> DCHECK(it.HolderisReceiverOrHiddenPrototype());
> Object::SetDataProperty(&it, value);

Done, works fine; thanks for the tip.  The difference is that setting the
property on a frozen object doesn't throw, because of the strict-mode thing.  I
changed the test case accordingly.  WDYT?

[Toon Verwaest, 2014-08-25 08:51:31]

lgtm, thanks!
(Note that you'll have to remove the .Check() for SetDataProperty since I
changed the return-type from MaybeHandle to Handle on Friday after seeing this
CL :)).

[wingo, 2014-08-25 09:12:34]

Committed patchset #7 manually as 23341 (presubmit successful).