Fix up array iterator semantics to match proposal




JavaScript Engine
6 years ago
3 years ago


(Reporter: jorendorff, Assigned: jorendorff)


Other Branch

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [js:t])


(1 attachment, 1 obsolete attachment)



6 years ago
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
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

6 years ago
Created attachment 639051 [details] [diff] [review]

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.
Assignee: general → jorendorff
Attachment #639051 - Flags: review?(jwalden+bmo)

Comment 2

6 years ago
Created attachment 639119 [details] [diff] [review]

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.
Attachment #639051 - Attachment is obsolete: true
Attachment #639051 - Flags: review?(jwalden+bmo)
Attachment #639119 - Flags: review?(jwalden+bmo)
Whiteboard: [js:t]

Comment 3

6 years ago
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

6 years ago
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

6 years ago
Comment on attachment 639119 [details] [diff] [review]

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(, "x");
> +        assertThrowsValue(function () {; }, 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?
Attachment #639119 - Flags: review?(jwalden+bmo) → review+
No longer relevant - StopIteration based Array iterators are gone. Resolving as Won't Fix.
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.