Array Iterator prototype should not have a constructor.

Array Iterator prototype should not have a constructor.

BUG=v8:3293
LOG=Y
R=verwaest@chromium.org

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

[arv (Not doing code reviews), 2014-04-25 16:43:06]

ws

[arv (Not doing code reviews), 2014-04-25 16:45:30]

https://codereview.chromium.org/258793005/diff/40001/src/array-iterator.js
File src/array-iterator.js (left):

https://codereview.chromium.org/258793005/diff/40001/src/array-iterator.js#ol...
src/array-iterator.js:102: %FunctionSetReadOnlyPrototype(ArrayIterator);
This is not in the spec and now there is no way to get to the ArrayIterator so
it is not observable anyway.

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

https://codereview.chromium.org/258793005/diff/40001/src/array-iterator.js#ne...
src/array-iterator.js:111: delete ArrayIterator.prototype.constructor;
I don't see where the constructor is added to this?

[arv (Not doing code reviews), 2014-04-28 21:11:10]

Replace prototype instead of deleting the constructor

[arv (Not doing code reviews), 2014-04-28 21:12:54]

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

https://codereview.chromium.org/258793005/diff/40001/src/array-iterator.js#ne...
src/array-iterator.js:111: delete ArrayIterator.prototype.constructor;
On 2014/04/25 16:45:30, arv wrote:
> I don't see where the constructor is added to this?

Doh! The constructor is set by default for all function prototypes. I ended up
replacing the prototype with a new fresh object instead of deleting the
prototype.

[arv (Not doing code reviews), 2014-05-06 20:50:45]

Toon, would you mind taking a look or suggest a better reviewer?

[Toon Verwaest, 2014-05-06 23:23:19]

LGTM for this CL

However, as far as I can tell, it seems like Array iterators in V8 aren't spec
conforming in the following way. (Correct me if I'm reading this wrongly.)

3. If O does not have all of the internal slots of an Array Iterator Instance
(22.1.5.3), throw a 
TypeError exception. (I presume internal slots are only read from the direct
receiver?)

var a = [1,2,3];
var i = a.values();
var o = {__proto__: i};
console.log(o.next()); // 1
console.log(o.next()); // 2
console.log(o.next()); // 3
console.log(i.next()); // 1
console.log(i.next()); // 2
console.log(i.next()); // 3

I also realized that the current ecmascript 6 spec is very different from
http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts. Which is
supposed to be newer? I hope/presume the spec, because we are horribly
incompatible with the spec-draft page.

As a side note: The spec is also a bit redundant. Before calling
CreateArrayIterator it always does ToObject(this), passing it as the "array"
argument. The first thing CreateArrayIterator does is doing ToObject(array).

Regards,
Toon

[Toon Verwaest, 2014-05-06 23:23:19]

LGTM for this CL

However, as far as I can tell, it seems like Array iterators in V8 aren't spec
conforming in the following way. (Correct me if I'm reading this wrongly.)

3. If O does not have all of the internal slots of an Array Iterator Instance
(22.1.5.3), throw a 
TypeError exception. (I presume internal slots are only read from the direct
receiver?)

var a = [1,2,3];
var i = a.values();
var o = {__proto__: i};
console.log(o.next()); // 1
console.log(o.next()); // 2
console.log(o.next()); // 3
console.log(i.next()); // 1
console.log(i.next()); // 2
console.log(i.next()); // 3

I also realized that the current ecmascript 6 spec is very different from
http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts. Which is
supposed to be newer? I hope/presume the spec, because we are horribly
incompatible with the spec-draft page.

As a side note: The spec is also a bit redundant. Before calling
CreateArrayIterator it always does ToObject(this), passing it as the "array"
argument. The first thing CreateArrayIterator does is doing ToObject(array).

Regards,
Toon

[arv (Not doing code reviews), 2014-05-07 15:12:47]

On 2014/05/06 23:23:19, Toon Verwaest wrote:
> LGTM for this CL
> 
> However, as far as I can tell, it seems like Array iterators in V8 aren't spec
> conforming in the following way. (Correct me if I'm reading this wrongly.)
> 
> 3. If O does not have all of the internal slots of an Array Iterator Instance
> (22.1.5.3), throw a 
> TypeError exception. (I presume internal slots are only read from the direct
> receiver?)
> 
> var a = [1,2,3];
> var i = a.values();
> var o = {__proto__: i};
> console.log(o.next()); // 1
> console.log(o.next()); // 2
> console.log(o.next()); // 3
> console.log(i.next()); // 1
> console.log(i.next()); // 2
> console.log(i.next()); // 3

That is a good point. GLOBAL_PRIVATE is not suitable for this. I'm pretty sure
we have a bunch of similar bugs in Promise as well. I'll investigate and file
bugs.

> I also realized that the current ecmascript 6 spec is very different from
> http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts. Which is
> supposed to be newer? I hope/presume the spec, because we are horribly
> incompatible with the spec-draft page.

The newest draft at
http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts is Rev 24
which is also what the current online HTML version is,
http://people.mozilla.org/~jorendorff/es6-draft.html

> As a side note: The spec is also a bit redundant. Before calling
> CreateArrayIterator it always does ToObject(this), passing it as the "array"
> argument. The first thing CreateArrayIterator does is doing ToObject(array).

Filed bug: https://bugs.ecmascript.org/show_bug.cgi?id=2828

This extra ToObject is not observable so I'll remove it from this file in a
follow up CL.

> 
> Regards,
> Toon

[arv (Not doing code reviews), 2014-05-07 17:03:20]

On 2014/05/07 15:12:47, arv wrote:
> On 2014/05/06 23:23:19, Toon Verwaest wrote:
> > LGTM for this CL
> > 
> > However, as far as I can tell, it seems like Array iterators in V8 aren't
spec
> > conforming in the following way. (Correct me if I'm reading this wrongly.)
> > 
> > 3. If O does not have all of the internal slots of an Array Iterator
Instance
> > (22.1.5.3), throw a 
> > TypeError exception. (I presume internal slots are only read from the direct
> > receiver?)
> > 
> > var a = [1,2,3];
> > var i = a.values();
> > var o = {__proto__: i};
> > console.log(o.next()); // 1
> > console.log(o.next()); // 2
> > console.log(o.next()); // 3
> > console.log(i.next()); // 1
> > console.log(i.next()); // 2
> > console.log(i.next()); // 3
> 
> That is a good point. GLOBAL_PRIVATE is not suitable for this. I'm pretty sure
> we have a bunch of similar bugs in Promise as well. I'll investigate and file
> bugs.

https://codereview.chromium.org/268363011/

We only use GET_PRIVATE in Promise.js and messages.js.

The promise does a HasLocalProperty.

messages.js uses private symbols for CallSite to store state. These are non
standard but it is clearly questionable if we should   allow non own properties
here? All the functions work with {__proto__: callSiteObject}. Passing a non
CallSite object throws illegal access in some cases but not all.

Filed https://code.google.com/p/v8/issues/detail?id=3321

> > As a side note: The spec is also a bit redundant. Before calling
> > CreateArrayIterator it always does ToObject(this), passing it as the "array"
> > argument. The first thing CreateArrayIterator does is doing ToObject(array).
> 
> Filed bug: https://bugs.ecmascript.org/show_bug.cgi?id=2828
> 
> This extra ToObject is not observable so I'll remove it from this file in a
> follow up CL.

Our code only calls ToObject once.

> 
> > 
> > Regards,
> > Toon

[Toon Verwaest, 2014-05-09 16:37:10]

Committed patchset #4 manually as r21234 (presubmit successful).