725168 - Element iterators should use [[Get]] and not peculiarly ignore the prototype chain

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

This only matters in dumb corner cases, but basically:

    Array.prototype[2] = "potato";
    for (let x of [0, 1, , 3])
        print(x);

should print 0, 1, "potato", 3
rather than 0, 1, undefined, 3.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

I don't know what I was thinking. Just randomly hating on holes.

The fast path for dense arrays could be thrown away too. It really depends on how we want to implement this.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 595257 [details] [diff] [review]
v1

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

Agreed about throwing out the dense array fast path, I think.  We can pick up fast paths when they're demonstrably necessary; in the meantime, clarity and concision are more important.

::: js/src/jsiter.cpp
@@ -1153,5 @@
> -        } else {
> -            Value v = DoubleValue(i);
> -            if (!js_ValueToStringId(cx, v, &id))
> -                goto error;
> -        }

Did I review this code and not require IndexToString?  :-(

Comment 3

[Jeff Walden [:Waldo]]

Oh, I express no position on whether it is better to have for-of look up the prototype chain (as happens with this patch) or look only at own properties (as happens currently).  Definitely whatever the rationale is should be recorded in this bug, tho.

Comment 4

[Jason Orendorff [:jorendorff]]

The rationale, in broad strokes:

  - it is more like the canonical for loop that way
  - nothing in the language currently does this kind of "get if own property";
    for better or worse, everything uses [[Get]]
  - just how unusual this is shows up as code complexity
  - everybody says ignoring the prototype chain won't make for-of any faster

In any case TC39 will kick this back and forth and eventually come to some decision, and we'll implement that.

Comment 5

[Jason Orendorff [:jorendorff]]

Incidentally, removing the "simple" fast path fixes bug 726212, so the patch I check in for that bug will just be a few tests.

Comment 6

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dac3cc61a1b0

Comment 7

[Marco Bonardo [:mak]]

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

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Nice diffstat!