fix(lib/core.js): remove required @@asyncIterator method from $AsyncIterator

[jedwards1211]

AsyncIterators are not required to be AsyncIterables, AFAIK

[nmote]

Could you provide some docs (e.g. MDN or the ECMAScript spec) to back up this change?

[jedwards1211]

https://tc39.github.io/proposal-async-iteration/#sec-control-abstraction-objects-patch makes clear that AsyncIterable and AsyncIterator are separate interfaces, and the only required property on an AsyncIterator is next().

[facebook-github-bot]

@nmote has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[jedwards1211]

However, confusingly, the docs also state:

11.1.2The %AsyncIteratorPrototype% Object
The value of the [[Prototype]] internal slot of the %AsyncIteratorPrototype% object is the intrinsic object %ObjectPrototype%. The %AsyncIteratorPrototype% object is an ordinary object. The initial value of the [[Extensible]] internal slot of the %AsyncIteratorPrototype% object is true.

NOTE
All objects defined in this specification that implement the AsyncIterator interface also inherit from %AsyncIteratorPrototype%. ECMAScript code may also define objects that inherit from %AsyncIteratorPrototype%.The %AsyncIteratorPrototype% object provides a place where additional methods that are applicable to all async iterator objects may be added.

11.1.2.1%AsyncIteratorPrototype% [ @@asyncIterator ] ( )
The following steps are taken:

Return the this value.
The value of the name property of this function is "[Symbol.asyncIterator]".

I don't fully understand this, but I think it means that iterators created by, for instance, calling a generator function, will have an @@asyncIterator property returning themselves, although user-created iterator objects don't have to have this property?

[nmote]

Ah, good catch. It looks like the same applies to #6076: https://www.ecma-international.org/ecma-262/6.0/#sec-%iteratorprototype%-@@iterator

Should these both be closed, then?

[jedwards1211]

@nmote I doubt it, I think %IteratorPrototype% etc. is only about internal details of spec implementations. If a [Symbol.iterator]() returns a plain old object with just { next(): { ... } }, one can iterate over it with for..of with no problems.

[jedwards1211]

@nmote after all https://www.ecma-international.org/ecma-262/6.0/#sec-iterator-interface doesn't say anything about @@iterator being required.

[jedwards1211]

MDN explicitly illustrates that the Iterable/Iterator interfaces can but do not have to be mixed:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols

It is not possible to know reflectively whether a particular object implements the iterator protocol, however it is easy to create an object that satisfies both the iterator and iterable protocols (as shown in the example below). Doing so allows an iterator to be consumed by the various syntaxes expecting iterables. Thus it is rarely desirable to implement the iterator protocol without also implementing iterable.

var myIterator = {
    next: function() {
        // ...
    },
    [Symbol.iterator]: function() { return this }
};

[jedwards1211]

Another example shows an iterator object without a Symbol.iterator method on it:

var someString = new String('hi');           // need to construct a String object explicitly to avoid auto-boxing

someString[Symbol.iterator] = function() {
  return { // this is the iterator object, returning a single element, the string "bye"
    next: function() {
      if (this._first) {
        this._first = false;
        return { value: 'bye', done: false };
      } else {
        return { done: true };
      }
    },
    _first: true
  };
};

[nmote]

Okay, I've spent a bit more time digging into this. Let's just centralize the discussion about both PRs here, since they are basically the same conceptually.

You are correct that the Iterator interface does not require the @@iterator property to exist. However, as you've noted, the spec says that "All objects defined in this specification that implement the Iterator interface also inherit from %IteratorPrototype%" which does include the @@iterator property.

So, at the very least, if we are to make this change, we should also create something like an IterableIterator which satisfies the current interface, and all builtin functions that currently return an Iterator should be changed to return an IterableIterator.

My main concern is that even if this change is made, it would still result in significant rollout pain. If you'd like to make it, I can try it out on our internal codebases to get a sense for what that pain would be (as long as you understand that I'm not promising that it will actually land). For an example of the kind of pain I'm talking about, some functions are currently annotated as returning an Iterator, and then their results are used in ForOf loops (which require an Iterable). This is safe, currently, given the current Iterator definition, but all of these type annotations would need to be updated as part of the rollout.

I'm not opposed in principle to creating a rollout burden, but in this case I see a pretty concrete downside. I don't really see a huge upside to this change. Sure, if I were starting from scratch, I would agree that this is better, but the current behavior isn't unsafe in any way, and I'm not sure it's causing any serious problems. I'm worried that the cost/benefit analysis doesn't work out in favor of making this change.

What do you think?

[jedwards1211]

@nmote I see what you mean. Yeah, considering that not many other people seem to be running into this issue, sounds like it's probably not a net benefit to flow users overall. I've only had flow errors from this in a few places myself and it's easy to suppress the errors or just add a Symbol.iterator method to them.

It's weird to me that people in the JS world are so apt to mix iterators and iterables. In Java at least, iterables are ideally supposed to always create a fresh iterator -- one that starts from the beginning of the collection or whatever it's iterating -- and an Iterator/Iterable that returns itself would violate this principle since the iterator could be halfway done. I'm not a fan of mixing the two. I guess JS folks decided this start-from-the-beginning behavior of iterables wasn't as important (probably because generator functions do create a start-from-the-beginning Iterator/Iterable each time they're called).

[nmote]

It's weird to me that people in the JS world are so apt to mix iterators and iterables.

Agreed, but here we are. 🤷‍♂️

[jedwards1211]

@nmote FWIW, TypeScript's AsyncIterator interface doesn't have a Symbol.asyncIterator property: https://github.com/microsoft/TypeScript/blob/master/lib/lib.es2018.asynciterable.d.ts#L32-L37

They have a separate AsyncIterableIterator interface combining the two.