Removal of Object type-check from GetIterator() algorithm

[caitp]

The If Type(iterator) is not Object, throw a TypeError exception. step is generally not implemented by engines, see https://jsfiddle.net/gj3uaw1t/ for an example. V8 does implement this step for yield*, but not other uses of GetIterator().

It would be cool if we could remove this from the spec and have common behaviour among implementations (it seems a recent version of SpiderMonkey has added the typecheck, breaking compatibility between FF and Chrome/Safari in this very unlikely situation).

[ljharb]

Wouldn't it be better for engines to implement what's in the spec? Is there a good reason it can't be implemented?

[caitp]

@ljharb it can be, but:

1. the typecheck isn't really useful, since non-Objects can still have iterator methods on their prototype chain
2. the typecheck takes up extra code (e.g. in https://codereview.chromium.org/2557593004/patch/20001/30028 7 bytes of additional bytecode to account for something that doesn't happen in practice, and even if it does happen, it's okay). It would be nice to be able to remove that gunk since it doesn't really do anything useful

[ljharb]

The usefulness I see is that it effectively prevents anyone from returning a primitive as an iterator, thus discouraging patching of built in prototypes - if the user wants to patch the built in prototype, they'd have to pass Object(4) or similar to be able to use it as the iteration result.

[caitp]

In @domenic's words, these type checks aren't very common to other parts of the language, so maybe it's a bit weird that GetIterator() behaves this way. Since it's unlikely that anyone will create an iterator method that returns a non-undefined, non-null, non-Object, maybe it's not the right place to discourage manipulation of builtin prototypes.

I mostly just want to avoid growing bytecode, since we want to keep this stuff as small as possible, and since most implementations don't implement it as spec'd in the first place, it's not breaking anything to change this.

[littledan]

There are other unexpected type checks in iteration whose motivation is hard to discern, for example if a .return() method exists, there is a type check that it returned an object. cc @GeorgNeis

[domenic]

In general these type checks are very surprising to me, given that we allow primitives in most of JS, e.g. by using GetV().

[littledan]

IIRC @allenwb had a consistency argument. Was it something like, that we would have these object checks in other cases where we use the object, so they should be extended to cases where we don't?

[domenic]

I'd be interested to hear such an argument. To me, consistency cuts in favor of removing the checks, given the widespread use of GetV(), including how it is used by Invoke(). We certainly have no precedent of saying "methods must be invoked on objects, not primitives" and I'd say we in fact have the opposite precedent.

For example, 1 can serve as a thenable, if Number.prototype has been patched with a then method.

[caitp]

IIRC @allenwb had a consistency argument. Was it something like, that we would have these object checks in other cases where we use the object, so they should be extended to cases where we don't?

there is no point where GetIterator() is used that its return value isn't immediately used as an object (either by IteratorStep() or IteratorClose(), which both perform GetMethod() or Invoke(), which defer to GetV as said above))

[ljharb]

@caitp this seems like it should be a Needs-Consensus PR, and then we can discuss it in the meeting. Any interest in filing it?

[caitp]

I likely won’t have time to put that together