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

Improve type checking and inference for Generators and Async Generators #30790

Merged
merged 16 commits into from
Jul 4, 2019

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Apr 6, 2019

This PR changes the definitions for IteratorResult, Generator, and AsyncGenerator and makes a number of improvements in how we check and infer types for yield and return inside of the body of a generator or async generator.

Type Definition Changes:

// es2015.iterable.d.ts
interface IteratorYieldResult<TYield> {
    done: false;
    value: TYield;
}

interface IteratorReturnResult<TReturn> {
    done: true;
    value: TReturn;
}

type IteratorResult<T, TReturn = any> = 
    | IteratorYieldResult<T>
    | IteratorReturnResult<TReturn>;

interface Iterator<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]): IteratorResult<T, TReturn>;
    return?(value?: TReturn): IteratorResult<T, TReturn>;
    throw?(e?: any): IteratorResult<T, TReturn>;
}

// es2015.generator.d.ts
interface Generator<T = unknown, TReturn = any, TNext = unknown> 
    extends Iterator<T, TReturn, TNext> {
    // NOTE: 'next' is defined using a tuple to ensure we report the correct assignability errors
    // in all places.
    next(...args: [] | [TNext]): IteratorResult<T, TReturn>;
    return(value: TReturn): IteratorResult<T, TReturn>;
    throw(e: any): IteratorResult<T, TReturn>;
    [Symbol.iterator](): Generator<T, TReturn, TNext>;
}

// es2018.asynciterable.d.ts
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>>;
}

// es2018.asyncgenerator.d.ts
interface AsyncGenerator<T = unknown, TReturn = any, TNext = unknown> 
    extends AsyncIterator<T, TReturn, TNext> {
    // 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>>;
    [Symbol.asyncIterator](): AsyncGenerator<T, TReturn, TNext>;
}

Type Checker Changes

With these new definitions, a generator can now distinguish between the following three types:

  • yielded type - The type of the expression provided to yield (i.e. TYield above)
  • returned type - The type of the expression provided to return (i.e. TReturn above)
  • next type - The type of the value accepted in a call to next() on the generator (i.e. TNext above).

Inside of the type checker we have also made the following changes to improve checking and inference when inside the body of a generator or async generator:

  • In a generator with an explicit return type annotation:
    • We will now correctly check the expression provided to return against the returned type of the generator's return type annotation (i.e. the TReturn type in the Generator definition above).
    • We will now correctly check, and provide a type for, the result of a yield expression based on the next type of the generator's return type annotation (i.e. the TNext type in the Generator definition above).
  • When inferring the return type of a generator without a return type annotation:
    • We will now infer the yielded type and return type as separate types rather than using a union of both types.
    • We will infer the next type as an intersection of the contextual types of each yield expression.
    • We no longer treat function*f(){} as an implicit-any error, and will instead give it a return type of Generator<never, void, unknown>. This aligns with our behavior for function f(){} where we infer a return type of void.

Here are several examples of the new behavior:

function* f(): Generator</*TYield*/ number, /*TReturn*/ string, /*TNext*/ number> {
  const x = yield 1; // ok, '1' is assignable to 'number'
  x; // 'x' has type 'number'

  const y: string = yield 2; // error, 'number' (the type of TNext) is not assignable to 'string'

  return ""; // ok, '""' is assignable to 'string'
}

function* g() {
  let i = 1;
  let s = "";
  // NOTE: for 'TNext' we will infer the intersection of A & B as that is the only type that
  // satisfies *both* assignments.
  const a: A = yield i;
  const b: B = yield s;
  return true;
}

g; // 'g' has type '() => Generator<string | number, boolean, A & B>'

Notes

  • For these definitions TReturn has a default of any. This is necessary for backwards compatibility when assigning a { done: boolean, value: number } type to an IteratorResult<number>, which is often the case when the source type is result of widening an object literal's type when there is no contextual type.
  • For existing code using weakly-typed iterators (i.e. { next(): { done: boolean, value: T } }) to still be considered asssignable, this PR depends on Relate source types covered by a target discriminated union #30779 (Relate source types covered by a target discriminated union).

Fixes #13847 (types will be checked correctly if return type is specified as Generator<T, void>)
Fixes #26959
Fixes #2983
Fixes #21527
Fixes #31214

@rbuckton
Copy link
Member Author

rbuckton commented Apr 6, 2019

@typescript-bot test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 6, 2019

Heya @rbuckton, I've started to run the Definitely Typed test suite on this PR at 03afdb4. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 6, 2019

Heya @rbuckton, I've started to run the extended test suite on this PR at 03afdb4. You can monitor the build here. It should now contribute to this PR's status checks.

@yortus
Copy link
Contributor

yortus commented Apr 6, 2019

It's great that yield and return types are now treated separately.

However it looks like the typing for yield/next types won't be able to model uses like co (I described that use-case a bit in #2983). However I don't know if anyone still uses that library or indeed how people use generators these days. Just curious what kind of usage patterns you came across in deciding to do it this way.

If there are still co-style usecases out there (i.e. where the type of each yield expression is a function of that yield's operand, and unrelated to the other yields), what will they have to do to pass these new type-checking rules? Explicit casts?

@rbuckton
Copy link
Member Author

rbuckton commented Apr 6, 2019

Unfortunately, casts would be necessary. The problems is that you would need a way to relate the yielded type to the next type:

function* f(): /* some type to relate 'y' to 'x' */ {
  let x = yield y;
}

Our compiler does not currently have a mechanism to handle this scenario.

@yortus
Copy link
Contributor

yortus commented Apr 6, 2019

Well a type-level function from the yielded type to the next type would be simple enough to express in TypeScript. It's just a generic type. For co it would be something like:

type Next<TYielded> =
    TYielded extends Promise<infer U> ? U :
    TYielded extends Array<Promise<infer U>> ? U[] :
    TYielded;

But the type function itself would need be to be passed as a type parameter to Generator<>, which I guess is the bit that tsc currently doesn't support.

@xaviergonz
Copy link

xaviergonz commented Apr 6, 2019

Just curious, will this proposal support emulating async/await?
This is, something like

// yields must be done over promises
asAsyncAwaitish(
  function* g(target: number) { // function generator works similar to async
    const a = yield Promise.resolve(3); // a is number (3?), similar to await
    const b = yield Promise.resolve("hi") // b is string ("hi"?), similar to await
    return a === target; // final returned type is a boolean
  }
) // typed as (target: number) => Promise<boolean>

if so, what would the Generator type look like?

Basically I'm just wondering if properly typing flows from mobx (https://mobx.js.org/best/actions.html - flow section) would be possible after this is in.

@rbuckton
Copy link
Member Author

rbuckton commented Apr 7, 2019

@xaviergonz: That is essentially what @yortus was discussing earlier. No, that is not supported at this time.

@treybrisbane
Copy link

+1 for the usecase @yortus mentioned (in my case, it's more around monadic operations as mentioned here, but effectively the same thing).

Even without that, this PR will be a huge improvement to generator support, and I'm glad to see them getting some love! :D

@rbuckton
Copy link
Member Author

rbuckton commented Apr 7, 2019

@yortus: Yes, we would probably need some mechanism for higher-order generics which would allow us to do something like this:

interface InteractiveGenerator<TNext<TYield>, TReturn> {
    next(value?: TNext): IteratorResult<TYield, TReturn>;
    return(value: TReturn): IteratorResult<TYield, TReturn>;
    throw(err: any): IteratorResult<TYield, TReturn>;
}

type Await<T> = T extends PromiseLike<infer U> ? U : T;

function* f(): InteractiveGenerator<Await, void> {
  const x = yield Promise.resolve(1);
  // TYield: 'Promise<number>'
  // TNext: 'Await<TYield>' -> 'Await<Promise<number>>' -> 'number'
  // x: 'number'
}

If we do add a feature for higher-order generics in the future, I imagine this would be feasible to implement over a separate interface like InteractiveGenerator above. However, that is out of scope for this PR.

@rbuckton rbuckton force-pushed the strictGeneratorTypes branch 2 times, most recently from 0e23eac to 8204898 Compare April 7, 2019 05:38
@rbuckton
Copy link
Member Author

rbuckton commented Apr 7, 2019

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 7, 2019

Heya @rbuckton, I've started to run the extended test suite on this PR at 8204898. You can monitor the build here. It should now contribute to this PR's status checks.

@yortus
Copy link
Contributor

yortus commented Apr 7, 2019

I just remembered #1213, which tracks higher-order generics. So if that issue was resolved, then usecases like co, mobx, and redux-saga could be fully typed (in some other PR in the future obviously).

@rbuckton
Copy link
Member Author

rbuckton commented Apr 7, 2019

Looks like the RWC errors fall into two categories:

  • Conflicts with IteratorResult due to the older version of node definitions used in the RWC tests.
  • A missing definition for Generator since it is defined in es2015.generator and not es2015.iterable.

@rbuckton
Copy link
Member Author

rbuckton commented Apr 7, 2019

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 7, 2019

Heya @rbuckton, I've started to run the Definitely Typed test suite on this PR at 8204898. You can monitor the build here. It should now contribute to this PR's status checks.

@rbuckton
Copy link
Member Author

rbuckton commented Apr 7, 2019

DT Failures fall into one of three categories:

  • Tests that don't distinguish between done: true and done: false in IteratorResult (adone, ckeditor).
  • An assignment compatibility error in the tests for 'co'.
  • Identifier collisions for IteratorResult in node v7, v8, and v9.

The tests for 'adone' and 'ckeditor' can be easily fixed though I need to take a closer look at the 'co' tests. The errors in Node were expected, and we've already addressed them using <reference lib= entries for Node v10 and v11, so we would likely need to make the same change in the earlier node library versions.

@paldepind
Copy link

@yortus

However it looks like the typing for yield/next types won't be able to model uses like co (I described that use-case a bit in #2983). However I don't know if anyone still uses that library or indeed how people use generators these days.

redux-saga is a very popular library that uses generators in a way similar to co and which would benefit from what you're describing.

@Igorbek
Copy link
Contributor

Igorbek commented Apr 9, 2019

This will be a great addition!
I have a few questions,

  • why does Generator's throw accept e: any but AsyncGenerator's one accepts e: unknown?
  • next accepts optional parameter, but yeild's result (next) doesn't contain | undefined implicitly, right?

@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Apr 9, 2019

  • why does Generator's throw accept e: any but AsyncGenerator's one accepts e: unknown?

throw(e: unknown) seems like a better choice than throw(e: any). With unknown, the AsyncGenerator implementation must narrow the type of e before it can use it (e.g. using instanceof Error or a type guard).

I assume Generator must have throw(e: any) because changing it to unknown would break backwards compatibility (not an expert on this, so correct me if I'm wrong! 😉). Since AsyncGenerator is a new interface, TypeScript can use the more appropriate unknown type instead.

  • next accepts optional parameter, but yield's result (next) doesn't contain | undefined implicitly, right?

Correct.

This is because the (async) generator only starts executing after the first next() call (see MDN). As such, the argument to the first next() call will be ignored by the (async) generator. However, for all future next() calls, you should pass an argument to next().

There's no way to encode this behavior at the type level. To know whether value is optional or not, the type checker would need to know whether next() has been called before, but that's very hard (if not impossible) to know at compile time. Therefore, marking value as always optional is an approximation: it does not give false negatives (for programs that do not pass a value to the first next() call), but it can give false positives (for programs that do not pass a value to subsequent next() calls).

TNext does not need to include undefined. If you use the generator correctly (and TypeScript assumes you do), then you'll always pass a value of type TNext to all next() calls (except for the first call).

@rbuckton
Copy link
Member Author

rbuckton commented Jul 4, 2019

We already addressed this issue in @types/node@9.6.48 with the addition of a "typesVersions" entry in the package.json.

buschtoens added a commit to ember-decorators/ember-decorators that referenced this pull request Jul 4, 2019
@buschtoens
Copy link

@rbuckton Awesome! Thank you so much — That fixed it! You just saved me hours of research. ❤

@Andarist
Copy link
Contributor

Andarist commented Jul 7, 2019

From what I have understand here - are you saying that improving types around co-like libraries is not on the schedule of 3.6? I was kinda hoping that advertised "improved generators support" was all over that. I might be biased (redux-saga maintainer here) but implementing all sort of interpreters with generators is a really cool usage and was hoping that upcoming changes would finally help express those patterns in TS.

@treybrisbane
Copy link

@Andarist It sounds like your use case is the same as what @yortus mentioned above. If so, you're correct in that this PR does not address that. Such a use case requires type-level higher-order functions, the implementation of which would be a much more fundamental change to the type system, and is very much beyond the scope of this work.

Unfortunately, there do not seem to be any plans (that I can find) to add such a feature to the language. :(

@JoshuaKGoldberg
Copy link
Contributor

This appears to be the root cause of palantir/tslint#4784.

@rbuckton rbuckton deleted the strictGeneratorTypes branch July 27, 2019 19:32
josemarluedke pushed a commit to josemarluedke/ember-decorators that referenced this pull request Aug 6, 2019
@treybrisbane treybrisbane mentioned this pull request May 31, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet