Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync iterator’s and array-like objects’ values are not awaited #9

Closed
js-choi opened this issue Sep 19, 2021 · 4 comments · Fixed by #11
Closed

Sync iterator’s and array-like objects’ values are not awaited #9

js-choi opened this issue Sep 19, 2021 · 4 comments · Fixed by #11
Labels
bug Something isn't working

Comments

@js-choi
Copy link
Collaborator

js-choi commented Sep 19, 2021

On 64a4921, @zloirock left this comment:

this fallback will not properly work with sync iterators.

await Array.fromAsync([Promise.resolve(1), 2, Promise.resolve(3)]); // => [Promise.resolve(1), 2, Promise.resolve(3)]

Something like that could be simpler.

Let usingAsyncIterator be ? GetMethod(asyncItems, @@asyncIterator).
Let usingIterator be undefined.
If usingAsyncIterator is undefined,
  Let usingIterator be ? GetMethod(asyncItems, @@Iterator).
  If usingIterator is undefined,
    Let usingIterator be ? %Array.prototype.values%.
...
If usingAsyncIterator is not undefined,
  Let iteratorRecord be ? GetIterator(asyncItems, async, usingAsyncIterator).
Else,
  Let syncIteratorRecord be ? GetIterator(asyncItems, sync, usingIterator).
  Let iteratorRecord be ? CreateAsyncFromSyncIterator(syncIteratorRecord).

The problem is that the current specification will not properly await each value yielded from a synchronous iterator. (We do want await to be called on each value from a synchronous iterator—this is what for await does.)

This problem is due to how the spec currently incorrectly creates an synchronous iterator. Step 3.e.iii assigns iteratorRecord to GetIterator(asyncItems, async, usingAsyncOrSyncIterator), which results in a synchronous iterator that does not call await on each of its yielded values. What we need to do is copy GetIterator when it is called with an async hint. We need to call CreateAsyncFromSyncIterator on the created sync iterator, which in turn will create an Async-from-Sync Iterator object, whose next method will call await on each of its values. (Thanks @bakkot for the help).

I will fix this later.

@js-choi js-choi added the bug Something isn't working label Sep 19, 2021
@zloirock
Copy link
Contributor

The same problem is with non-iterable array-like objects. The simplest solution above -)

@js-choi
Copy link
Collaborator Author

js-choi commented Sep 24, 2021

I’m a little confused. How does the current algorithm not work with non-iterable array-like objects?

If asyncItems is a non-iterable array-like object, then GetMethod(asyncItems, @@asyncIterator) and GetMethod(asyncItems, @@iterator) would be both undefined, which makes usingAsyncOrSyncIterator undefined, which means step 3.e is skipped and step 4.f etc. are executed.

@zloirock
Copy link
Contributor

zloirock commented Sep 24, 2021

The current algorithm doesn't await the value (async-from-sync iterator behavior) and the result of the callback.

await Array.fromAsync({ 0: Promise.resolve(1), 1: 2, 2: Promise.resolve(3), length: 3 }); // => [Promise.resolve(1), 2, Promise.resolve(3)]

await Array.fromAsync({ 0: 1, 1: 2, 2: 3, length: 3 }, async (it) => it); // => [Promise.resolve(1), Promise.resolve(2), Promise.resolve(3)]

@js-choi
Copy link
Collaborator Author

js-choi commented Sep 24, 2021

Aha, that’s what you mean. I understand now—thank you for explaining. I will fix this when I can later.

@js-choi js-choi changed the title Sync iterator’s values are not awaited Sync iterator’s and array-like objects’ values are not awaited Sep 30, 2021
js-choi added a commit that referenced this issue Sep 30, 2021
js-choi added a commit that referenced this issue Sep 30, 2021
js-choi added a commit that referenced this issue Sep 30, 2021
js-choi added a commit that referenced this issue Sep 30, 2021
js-choi added a commit that referenced this issue Sep 30, 2021
js-choi added a commit that referenced this issue Sep 30, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants