Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

What happens if you pass a sync iterator (not iterable) to AsyncIterator.from? #237

Closed
bakkot opened this issue Sep 19, 2022 · 9 comments · Fixed by #233
Closed

What happens if you pass a sync iterator (not iterable) to AsyncIterator.from? #237

bakkot opened this issue Sep 19, 2022 · 9 comments · Fixed by #233

Comments

@bakkot
Copy link
Collaborator

bakkot commented Sep 19, 2022

If you have an object { next() {...} }, and it doesn't have a Symbol.iterator or Symbol.asyncIterator method, there's no way to tell if that's sync or async.

If you pass such a thing to AsyncIterator.from, the result is supposed to be an async iterator. Right now, any time a sync iterable is passed to something which expects an async iterable, and that sync iterator yields Promises, the spec will lift from Iterator<Promise<T>> to AsyncIterator<T>, by awaiting the .value property (in %AsyncFromSyncIteratorPrototype%.next).

But here there's no way to tell if the passed thing is sync or async. So should values from the .value property be awaited or not?

That is:

let iter = { next(){ return { done: false, value: Promise.resolve(0); } } };
let aIter = AsyncIterator.from(iter);
let { value } = await aIter.next();
value // 0, or Promise.resolve(0)?
@devsnek
Copy link
Member

devsnek commented Sep 19, 2022

we should assume its an async iterator, because you called AsyncIterator.from

@bakkot
Copy link
Collaborator Author

bakkot commented Sep 19, 2022

I feel like it's not totally unreasonable to call AsyncIterator.from because you had a sync iterator and you wanted an async iterator.

@devsnek
Copy link
Member

devsnek commented Sep 19, 2022

at the risk of sounding like a shitpost, AsyncIterator.from(Iterator.from(x))?

@bakkot
Copy link
Collaborator Author

bakkot commented Sep 19, 2022

That does happen to work, but only because Iterator.from(x) returns an iterable. But it's not something people are going to try without being told to try it, I would think.

@bergus
Copy link

bergus commented Sep 19, 2022

I had absolutely expected AsyncIterator.from to have the same behaviour as CreateAsyncFromSyncIterator. Why is there even a WrapForValidAsyncIteratorPrototype separate from the AsyncFromSyncIteratorPrototype?

@bakkot
Copy link
Collaborator Author

bakkot commented Sep 19, 2022

The primary purpose of AsyncIterator.from is to ensure that the result inherits from AsyncIterator.prototype, in the same way that Iterator.from ensures the result inherits from Iterator.prototype.

@michaelficarra
Copy link
Member

Also of note: if the iterator has a Symbol.iterator (possibly from its prototype being Iterator.prototype), its iterable-ness will take precedence over its iterator-ness. I think it would be really strange if the result differed for those cases.

@bakkot
Copy link
Collaborator Author

bakkot commented Sep 20, 2022

I am tempted to resolve this by saying the from methods only take iterables, not iterators. That makes them marginally less useful for wrapping manually produced iterators; on the other hand, it's quite unusual to vend iterators, as opposed to iterables, so I don't think that would actually come up much.

@michaelficarra
Copy link
Member

I've updated #233 to address this. It aligns flatMap with the respective from function.

Iterator.prototype.flatMap and Iterator.from:

  • non-Object: throw
  • has Symbol.iterator: call it, get sync iterator, iterate
  • has callable "next": assume sync iterator, iterate
  • otherwise: throw

AsyncIterator.prototype.flatMap and AsyncIterator.from:

  • non-Object: throw
  • has Symbol.asyncIterator: call it, get async iterator, async iterate
  • has Symbol.iterator: call it, get sync iterator, lift to async iterator, async iterate
  • has callable "next": assume async iterator, async iterate
  • otherwise: throw

Unfortunately, this does mean that async flatMap will treat sync iterators differently if they have a Symbol.iterator, indicating their sync-ness. I think this is our best option.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants