770835 - Fix up array iterator semantics to match proposal

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The current behavior of the .next() method of Array iterators is fine, but there are corner cases where it can behave really strangely.

In particular, it's possible for the iterator to throw a TypeError "(void 0) is undefined" in a case where the iterator has been closed and therefore its TargetSlot has been set to undefined. It takes bizarre corner case code to trigger it, but that's really ugly.

I wrote some proposed spec language here
  http://wiki.ecmascript.org/doku.php?id=harmony:iterators
and I figure we might as well change our implementation to match.

Fully specifying something like this gets you immediately into all sorts of stupid corner cases. The proposal settles them in what I hope are unobjectionable ways; we can certainly tweak to match whatever TC39 settles on later.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

The old code clears TargetSlot when closing the iterator. This turns out to be undesirable:

- The iterator is the only thing keeping the target value gc-alive.

- The iterator is almost certainly garbage almost as soon as we return,
  so it's not going to keep anything alive anyway.

- Clearing the slot is a pointless extra write.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v2

Same code change as v1, different test. I found out that iterator-proto-surfaces.js covers the exact thing v1's Array-iterator-proto.js is testing, so I removed the latter.

Comment 3

[Jeff Walden [:Waldo]]

Looking at the review now.  A comment on the proposal semantics tho: they should use ToObject or CheckObjectCoercible more, so that undefined/null as this trigger errors.  It looks like [[ArrayIteratorPrototype]].next(), Array.prototype.iterator(), and String.prototype.iterator() all lack ToObject or CheckObjectCoercible calls.

Comment 4

[Jeff Walden [:Waldo]]

I don't understand "The iterator is the only thing keeping the target value gc-alive."  If you can't use the iterator to get a reference to the target, why is it a good thing for the iterator to keep the target alive, in this case?

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 639119 [details] [diff] [review]
v2

Review of attachment 639119 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like the reentrancy semantics this patch implements, but they *are* what's in the wiki.  :-(

::: js/src/jit-test/tests/collections/Array-iterator-semantics-1.js
@@ +5,5 @@
> +var n = 0;
> +function check() {
> +    if (n++ === 0) {
> +        assertEq(iter.next(), "x");
> +        assertThrowsValue(function () { iter.next(); }, StopIteration);

These semantics are just so wrong, if they let you rewind iterator state to previous indexes with a length getter (or valueOf/toString on the length property value).  Is there any possible way that can be avoided?

::: js/src/jit-test/tests/collections/Array-iterator-semantics-4.js
@@ +5,5 @@
> +var obj = {iterator: [].iterator, length: 2, 0: "zero", 1: "one"};
> +var log = "";
> +var iter = obj.iterator();
> +for (var x of iter)  // after this loop, iter is closed
> +    log += x + ";";

Why aren't you asserting the final value of log?

Comment 6

[André Bargull [:anba]]

No longer relevant - StopIteration based Array iterators are gone. Resolving as Won't Fix.