tc39 / proposal-async-iterator-helpers

Methods for working with async iterators in ECMAScript
https://tc39.es/proposal-async-iterator-helpers/
102 stars 3 forks source link

should `AsyncIterator.zip` await values yielded from sync iterators? #24

Open bakkot opened 2 months ago

bakkot commented 2 months ago

AsyncIterator.zip([asyncIterator, syncIterable]) should presumably work (once we get Iterator.zip).

for await (let val of [Promise.resolve(0)]) will await the promise, binding 0 to val. I think necessarily that means that AsyncIterator.from([Promise.resolve(0)]) must as well, such that AsyncIterator.from([Promise.resolve(0)]).forEach(console.log) will print 0. This preserves the invariant that async iterables never yield promises.

But for zip, we don't necessarily have to match that behavior, because the results are boxed, not yielded directly, so the invariant is upheld either way. Question, then: should zip await values yielded by sync iterators?

On the one hand, "it promotes to async iterator, and then zips that" is the obvious intuition, which suggests that it should await.

On the other hand, that behavior is very easy to recover if it does not await values from the zipped things (simply do the AsyncIterator.from yourself), whereas recovering the "not await" behavior is pretty annoying if it does await them.

Thoughts? @michaelficarra expressed a preference for not awaiting.

conartist6 commented 2 months ago

Strong preference for awaiting

bakkot commented 2 months ago

@conartist6 say more about why?

conartist6 commented 2 months ago

In my mind the arguments to zip are async iterators. Zip is defined to return one item from each async input iterator.

I think I would favor not awaiting if async iterators themselves weren't defined to await on both step and step.value, but they are, and they are defined to do so when coercing sync iterators which contain promises to be used as async iterators.

const iter = [Promise.resolve("awaited")];

for await(const value of iter) console.log(value);

// awaited
conartist6 commented 2 months ago

That it's a pain to sidestep this awaiting (when you want to) is no more or less true with zip than it is for any other part of the async iterable API, and the value-wrapping tools used to do so will work the same with inputs to zip as they will everywhere else

bakkot commented 2 months ago

This is the only static method other than from, so I'm not sure it makes sense to compare to the rest of the async iterator API.

conartist6 commented 2 months ago

To me that has nothing to do with belonging to a different category of methods that should have fundamentally different input types. The reasons why zip is static make sense without it defining a category of its own (in other ways than just being static).

bakkot commented 2 months ago

I think that it's pretty natural to treat the receiver and arguments differently. For example, methods on the prototype (like .map) will definitely not be awaiting the .value property of the objects returned by their super.next(), because they will assume that the receiver is already an async iterator which is maintaining the invariant that .value does not contain a Promise. Whereas here we are taking a potentially-sync iterable as input, which needs explicit handling.

This is the only place other than .from which takes an potentially-sync iterable as an argument. And from is explicitly a coercion method; I don't think it counts as precedent for whether other methods should coerce in the same way.

conartist6 commented 2 months ago

What I hear you saying is that so far the language has never anywhere broken the rule that it awaits promises in async iteration APIs.

bakkot commented 2 months ago

No? I don't know how you got that from what I said.

And in any case, the point is that we're passing a sync iterable here. We could choose to await the .values it yields, but we're not obligated to.

laverdet commented 2 months ago

I'm not going to link to it because I don't think we want the attention, but this echos the monadic argument in promises-aplus/promises-spec # (94).

The ship sailed on monadic promises a decade ago so I think the most consistent thing is for zip to unbox them. It matches the overall design philosophy of Promises A+, whether or not you agree with the philosophy. I'd bet dollars to donuts that MM & the committee will ask you to unbox as well.

bakkot commented 2 months ago

That argument applies to AsyncIterator.from, but there's no question about how that behaves. The question here is not "should promoting a sync iterator to an async iterator unwrap promises"; that's settled. The question is "should .zip work by promoting a sync iterator to an async iterator, or can it handle sync iterators explicitly".

conartist6 commented 2 months ago

It should work by promoting a sync iterator to async because there is a natural linguistic/ontological expectation that the AsyncIterator.zip([...iterables]) is a method whose inputs are async iterables.

"The definition of zip over the domain of async iterables," is how I might dissect the name. Why would one expect such a function to have special edge case behavior for input types outside of its domain?

conartist6 commented 2 months ago

Moreover if you don't define a standard method that means "perform the zip operation on async streams" it's really going to hurt, and it would hurt that that name doesn't imply that implementation

bakkot commented 2 months ago

Moreover if you don't define a standard method that means "perform the zip operation on async streams" it's really going to hurt, and it would hurt that that name doesn't imply that implementation

This method will perform the natural zip operation on async iterables either way. Again, the only question here is how to handle sync inputs. There are multiple sensible ways to generalize the natural zip operation on async iterables to handle sync inputs.

ljharb commented 2 months ago

I would expect every item to be implicitly wrapped in AsyncIterator.from.

conartist6 commented 2 months ago

This method will perform the natural zip operation on async iterables either way.

How do you figure? If adding an a-priori coercion to an async iterable changes the result, it seems clear to me that the method does not treat its arguments as async iterables.

bakkot commented 2 months ago

There will be no coercion of async iterables. We're only discussing what to do with sync iterables.

conartist6 commented 2 months ago

If the inputs are conceputally async, then sync iterables would be coerced. Do we agree on that?

The main question is whether the inputs are conceptually async or whether they are conceptually understood to be a mixture of sync and async iterables.

conartist6 commented 2 months ago

Or maybe I should phrase it as the inverse? Coercing sync iterables creates an accurate, minimal conceptual model for the method's behavior

conartist6 commented 2 months ago

I'd also remind that while it's easy to imagine this:

AsyncIterator.zip([asyncIterable, AsyncIterator.wrap(syncIterable)]);

In practice the more common place the issue will crop is in code with syntactic structure more like this:

AsyncIterator.zip(iterables);

So to be safe in such a situation you would always need to write:

AsyncIterator.zip(iterables.map(AsyncIterables.wrap));

which at least is less wordy than most efficient/correct implementation (which nobody is going to bother to write out):

AsyncIterator.zip(iterables.map(iter => {
  return iterable[Symbol.asyncIterator] ? iter : AsyncIterable.wrap(iter);
}));

Say you're an intern or junior dev and you read that last example without knowing why I wrote it, would you have any idea what I was trying to accomplish?

conartist6 commented 2 months ago

One last thought: when you're designing an API in which different behaviors might be correct depending on the intentions of the user but the intentions of the user are not possible to deduce, then the only safe API design choice is to make the behavior simple and predictable so that the user is given a chance to declare their intention with knowledge of the choice they are making

bakkot commented 2 months ago

If the inputs are conceputally async, then sync iterables would be coerced. Do we agree on that?

No. Sync iterables would be rejected. If sync iterables are accepted, then it's possible they're coerced, or it's possible they're handled explicitly, like anything else which accepts inputs of multiple types.

Say you're an intern or junior dev and you read that last example without knowing why I wrote it, would you have any idea what I was trying to accomplish?

I would guess you're trying to coerce sync iterables to async ones. Why? Well, knowing that would require knowing more details of the behavior of zip than an junior dev could reasonably be expected to know, but at least it gives you something to google. But consider the reverse, where someone has to write AsyncIterator.zip(iterables.map(x => [x])).map(y => y.map(x => x[0])) - I think that's considerably more opaque.

make the behavior simple and predictable

The behavior is simple either way. It can't be fully predictable because both behaviors are reasonable and there will certainly be some people who expect each. In such cases, a useful question is "which behavior allows those who are surprised to most easily notice and recover from getting the wrong behavior?", and I think that points in the direction of not unboxing promises, because it's much easier to recover from that if you expected it to unbox than it is to recover the unboxing behavior if you expected it not to.

conartist6 commented 2 months ago

Your example doesn't make any sense to me:

AsyncIterator.zip(iterables.map(x => [x])).map(y => y.map(x => x[0]))

a simpler way to write that would be:

Iterator.zip(iterables);
conartist6 commented 2 months ago

...also you're not mapping over the individual iterables, just the array of them, so I think you really meant

AsyncIterator.zip(iterables.map(iter => iter.map(v => [v]))).map(values => values.map(v => v[0]))

If I'm correct about what you meant here is my amended example of how I would write it:

const getIterator = obj => obj[Symbol.asyncIterator]?.() || obj[Symbol.iterator]?.();

Iterator.zip(iterables.map(getIterator));

It think that's a good bit less confusing than using an async method just to undo all the asyncness and return results synchronously. My way also has a lot less overhead.