Normative: Use GetMethod instead of GetV to get iterator next

[dead-claudia]

This is purely for consistency. Currently, only the first throws (tested in Node 10), but with this patch, the second would also throw:

[] = {[Symbol.iterator]: () => ({get next() { throw "foo" }})};
[] = {[Symbol.iterator]: () => ({next: 0})};

In every other case, this has no effect since they all call IteratorNext before any other ECMAScript-visible behavior, and I feel this better aligns intuitively with what the spec implies.

[domenic]

This is not "editorial" if it changes behavior of implementations. As such it would require testing against engines, test262 tests, and (assuming engines follow the current spec) sign-off on changing it.

[dead-claudia]

Okay. I'll fix the commit message, then. I misunderstood the distinction.

…

On Thu, Aug 16, 2018, 11:17 Domenic Denicola ***@***.***> wrote: This is not "editorial" if it changes behavior of implementations. As such it would require testing against engines, test262 tests, and (assuming engines follow the current spec) sign-off on changing it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AERrBB1MqC_lEn1nEcbSbCZhjUlcEkYNks5uRY0PgaJpZM4V_P48> .

[dead-claudia]

@domenic Done.

[dead-claudia]

So Node/Chrome, Firefox, and Safari all have the same behavior. I haven't yet tested on Edge (don't have a VM ready), but it appears this would in fact be a theoretically breaking change.

[ljharb]

With a file of:

[] = {[Symbol.iterator]: () => ({get next() { throw new Error("foo") }})};

[] = {[Symbol.iterator]: () => ({next: 0})};

I get these results:

#### ch


#### v8
Error: foo

#### sm
Error: foo

#### jsc
Error: foo

When I comment out the first line, no engines throw.

[ljharb]

@isiahmeadows the committee discussed this and had the following feedback:

1. It'd be great to see more of a motivating use case here for the normative change
2. before merging a breaking change like this, we'd like to see at least one implementation of it (ie, one browser) both to signal web compatibility, and to indicate implementor buy-in.

[ljharb]

@isiahmeadows are you still interested in pursuing this change? if so, the two points in #1288 (comment) need to be addressed - if not, this should be closed.

[dead-claudia]

@ljharb I do, but it's low priority. I'll close this for now, and I'll open a new issue and/or PR if/once I get the time to do it and address the feedback question. (I do also want to run it by V8 first to get their thoughts on it.)

[bakkot]

@isiahmeadows I'm going to close this per your previous comment.