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

Trouble implementing AsyncIterator interface in 3.6.2 #33239

Closed
brainkim opened this issue Sep 4, 2019 · 11 comments · Fixed by #33354
Closed

Trouble implementing AsyncIterator interface in 3.6.2 #33239

brainkim opened this issue Sep 4, 2019 · 11 comments · Fixed by #33354
Assignees
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript

Comments

@brainkim
Copy link

brainkim commented Sep 4, 2019

TypeScript Version: 3.6.2

Search Terms:
async iterator iterable next

Code:
filename: constant.ts

class ConstantIterator<T> implements AsyncIterator<T, undefined, T | undefined> {
  constructor(private constant: T) {
  }
  async next(value?: T): Promise<IteratorResult<T>> {
    if (value != null) {
      throw new Error("ConstantIterator.prototype.next may not take any values");
    }
    return {value: this.constant, done: false};
  }
}

Expected behavior:
Code compiles.
Actual behavior:

constant.ts(4,9): error TS2416: Property 'next' in type 'ConstantIterator<T>' is not assignable to the same property in base type 'AsyncIterator<T, undefined, T>'.
  Type '(value?: T) => Promise<{ value: T; done: boolean; }>' is not assignable to type '(...args: [] | [T | PromiseLike<T>]) => Promise<IteratorResult<T, undefined>>'.
    Types of parameters 'value' and 'args' are incompatible.
      Type '[] | [T | PromiseLike<T>]' is not assignable to type '[T?]'.
        Type '[T | PromiseLike<T>]' is not assignable to type '[T?]'.
          Types of property '0' are incompatible.
            Type 'T | PromiseLike<T>' is not assignable to type 'T'.
              'T | PromiseLike<T>' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint '{}'.
                Type 'PromiseLike<T>' is not assignable to type 'T'.
                  'PromiseLike<T>' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint '{}'.

Note that the synchronous version of ConstantIterator does not throw the same error. It’s the type union of T | PromiseLike<T> which seems to cause the error.

Playground Link:
Playground has yet to be updated for 3.6.

Related Issues:
#30790 Strongly typed iterator PR.
#32682 Other (async) iterator usability issues.
#33131 Possible change to type inference in 3.6.

@ilogico
Copy link

ilogico commented Sep 5, 2019

The error message has a lot of text, but it's clear:

  • async functions must return promises: Promise<IteratorResult<T>> and not IteratorResult<T>
  • the next method must accept not only T | undefined, but also PromiseLike<T | undefined>

@brainkim
Copy link
Author

brainkim commented Sep 5, 2019

@ilogico Woops, I forgot to change the return type when I was testing that this error does not happen for sync iterators.

class ConstantIterator<T> implements AsyncIterator<T, undefined, T | undefined> {
  constructor(private constant: T) {
  }
  async next(value?: T): Promise<IteratorResult<T>> {
    if (value != null) {
      throw new Error("ConstantIterator.prototype.next may not take any values");
    }
    return {value: this.constant, done: false};
  }
}

try running tsc version 3.6.2 on the above code.

the next method must accept not only T | undefined, but also PromiseLike<T | undefined>

Is this an actual requirement in the async iterator spec? Can you point to where this is stated? As far as I can tell I have never defined an async iterator which expected a promise as the result of a yield.

@fatcerberus
Copy link

fatcerberus commented Sep 5, 2019

Based on a quick test in Node.js, there is no special handling for passing promises to .next():

> async function* g() { console.log(yield); }
undefined
> let iter = g();
undefined
> iter.next(Promise.resolve(812))
Promise { <pending> }
> iter.next(Promise.resolve(812))
Promise { 812 }
Promise { { value: undefined, done: true } }

The promise is not unwrapped by yield (unless explicitly awaited).

@brainkim
Copy link
Author

brainkim commented Sep 5, 2019

Yeah I initially thought it was an assignability error related to the weird tuple args, but I think it’s probably the fact that TNext should never be unwrapped as a Promise. Likely just a misunderstanding of async iterators?

@sandersn sandersn added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 5, 2019
@brainkim
Copy link
Author

brainkim commented Sep 6, 2019

@sandersn Can you explain the addition of the “Working as Intended” label? Here’s the current AsyncIterator interface defined by typescript:

interface AsyncIterator<T, TReturn = any, TNext = undefined> {
    // NOTE: 'next' is defined using a tuple to ensure we report the correct assignability errors in all places.
    next(...args: [] | [TNext | PromiseLike<TNext>]): Promise<IteratorResult<T, TReturn>>;
    return?(value?: TReturn | PromiseLike<TReturn>): Promise<IteratorResult<T, TReturn>>;
    throw?(e?: any): Promise<IteratorResult<T, TReturn>>;
}

Requiring the next method to have a first argument of type TNext | PromiseLike<TNext> implies that all well-formed async iterators unwrap the value passed to next, and this is a requirement which even async generators do not adhere to. It is strictly incorrect and I can point to the exact parts of the spec which prove this.

NOTE 1
Arguments may be passed to the next function but their interpretation and validity is dependent upon the target AsyncIterator. The for-await-of statement and other common users of AsyncIterators do not pass any arguments, so AsyncIterator objects that expect to be used in such a manner must be prepared to deal with being called with no arguments.

  • 25.5.1.2 AsyncGenerator.prototype.next
  • 25.5.3.6 AsyncGeneratorEnqueue
  • 25.5.3.5 AsyncGeneratorResumeNext
    These are all the definitions which relate to the TNext type parameter as it relates to async generators. The method AsyncGenerator.prototype.next calls AsyncGeneratorEnqueue with TNext as a normal completion, AsyncGeneratorEnqueue adds the completion to a queue without reading or awaiting it, and AsyncGeneratorResumeNext pulls the completion from the queue, and if the completion is not abrupt (which it won’t be because a next call never enqueues an abrupt completion), it resumes the generator with the completion as the value of yield. See step 18 of AsyncGeneratorResumeNext:
  1. Resume the suspended evaluation of genContext using completion as the result of the operation that suspended it. Let result be the completion record returned by the resumed computation.

At no point in any of these steps is the TNext value awaited, and this is proven to be true in the comment above.

The correct definition of AsyncIterator should probably be:

interface AsyncIterator<T, TReturn = any, TNext = undefined> {
    // NOTE: 'next' is defined using a tuple to ensure we report the correct assignability errors in all places.
    next(...args: [] | [TNext]): Promise<IteratorResult<T, TReturn>>;
    return?(value?: TReturn): Promise<IteratorResult<T, TReturn>>;
    throw?(e?: any): Promise<IteratorResult<T, TReturn>>;
}

In other words, the same as Iterator except the return types are all promises.

You could make an argument that the value passed to return should be TReturn | PromiseLike<TReturn> based on step 10 of AsyncGeneratorResumeNext and 25.5.3.5.1 AsyncGeneratorResumeNext Return Processor Fulfilled Functions which does state that promises passed to AsyncGenerator.prototype.return are unwrapped, but the AsyncIterator interface itself as defined by the spec does not contain any directives that all async iterators should unwrap the value passed to return.

In any case, the value passed to next is not required to be unwrapped anywhere in the spec for both async iterators and async generators.

@ilogico
Copy link

ilogico commented Sep 6, 2019

I agree that requiring the next method to accept PromiseLike<TNext> is incorrect.
If some iterators allow it, they should declare it explicitly.

@fatcerberus
Copy link

To be 100% clear: if a Promise<string> is passed to next, then inferring TNext = string is wrong; the correct inference is Promise<string>, because that’s what will be observed by the generator.

@sandersn
Copy link
Member

sandersn commented Sep 6, 2019

@brainkim I understood your comment

Likely just a misunderstanding of async iterators?

To refer to your misunderstanding. But it sounds like it's a misunderstanding by the author of lib.d.ts?

@sandersn sandersn added Bug A bug in TypeScript Needs Investigation This issue needs a team member to investigate its status. and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Sep 6, 2019
@sandersn sandersn added this to the TypeScript 3.7.0 milestone Sep 6, 2019
@brainkim
Copy link
Author

brainkim commented Sep 6, 2019

@sandersn Misunderstandings all around! Yes, I think lib.d.ts was written with a slight misunderstanding of async iterators.

@brainkim
Copy link
Author

Would it be possible to fix this in a 3.6 (3.6.4?) release? Currently assigning/implementing async iterators is really broken on 3.6 and I don’t like idea of telling people you have to skip a version of typescript just to use them.

I would also be willing to volunteer time to get this and other (async) iterator issues fixed! Let me know if you’re willing to accept non-microsoft contributions for these issues.

@rbuckton
Copy link
Member

You are correct that next and return do not await the values passes back to the async generator. I am looking into fixing the types now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants