Skip to content

Functions operating on union types with a generic type parameter do not compile return type #25915

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

Closed
benbayard opened this issue Jul 25, 2018 · 4 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@benbayard
Copy link

TypeScript Version:
originally seen in 2.9.2
reproduced in 3.1.0-dev.20180724

Search Terms:

Code

export interface Program {
    type: 'Program'
    body: AST[]
}

export interface MemberExpression {
    type: 'MemberExpression'
    object: AST
    property: AST
}

export interface FunctionDeclaration {
    type: 'FunctionDeclaration'
    body: AST
}

export type AST =
    | Program
    | MemberExpression
    | FunctionDeclaration

type ASTKeys<T extends AST> = {
    [K in keyof T]: T[K] extends AST ? K : T[K] extends AST[] ? K : never
}[keyof T]
export type PickASTKeys<T extends AST> = Pick<T, ASTKeys<T>>

function pickASTProperties<T extends AST>(ast: AST): PickASTKeys<T> {
    switch (ast.type) {
        case 'FunctionDeclaration': return {body: ast.body}
        case 'Program': return { body: ast.body }
        case 'MemberExpression': return { object: ast.object, property: ast.property }
    }
}

Expected behavior:
This code should compile. The union type should be narrowed down by the switch statement and the returned type should behave correctly. Interestingly, this compilation error can be worked around. If you supply a lookup that looks like the below and update the function declaration, the expected behavior does occur:

export interface Lookup {
    Program: PickASTKeys<Program>;
    MemberExpression: PickASTKeys<MemberExpression>;
    FunctionDeclaration: PickASTKeys<FunctionDeclaration>;
}

function pickASTProperties<T extends AST>(ast: AST): Lookup[T['type']] {/*... same code*/}

However, this workaround only works if the PickASTKeys is there, if I do not include the PickASTKeys in the lookup and put that in the return type, I again get compilation errors. I have included that example in the playground list

Actual behavior:
3 errors are received:

index.ts(33,7): error TS2322: Type '{ body: AST; }' is not assignable to type 'Pick<T, { [K in keyof T]: T[K] extends AST ? K : T[K] extends AST[] ? K : never; }[keyof T]>'.
index.ts(35,7): error TS2322: Type '{ body: AST[]; }' is not assignable to type 'Pick<T, { [K in keyof T]: T[K] extends AST ? K : T[K] extends AST[] ? K : never; }[keyof T]>'.
index.ts(37,7): error TS2322: Type '{ object: AST; property: AST; }' is not assignable to type 'Pick<T, { [K in keyof T]: T[K] extends AST ? K : T[K] extends AST[] ? K : never; }[keyof T]>'.

Playground Link:
Link with failing compiler
Link with workaround that does not compile
Link with workaround
Related Issues:
I honestly looked, but the namespace for this type of issue is fairly overloaded. I would not find anything within a reasonable timeframe, but I might have been looking for the wrong keywords.

@mattmccutchen
Copy link
Contributor

The original code definitely can't work: there is no relationship between T and the argument. If you change the argument to type T like this, then narrowing no longer works; there's an existing suggestion #20375 about this. Even if that were fixed, the compiler can't rule out that elsewhere in the program, you define a subclass of Program with additional AST keys and call pickASTProperties with T set to that subclass. So I don't see any way this code can work. (The workaround works due to the current very unsound assignability rule for indexed access types that looks at the constraint.) What were you trying to achieve?

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Jul 25, 2018
@benbayard
Copy link
Author

Hey @mattmccutchen ! Thank you for the great insight. My follow up is: do we expect the workaround I have in place to stop working in the future?

I believe the issue with the latter half of your comment has to do with variance? I feel that this space in TS has a lot of open issues, so I'm not sure what I can add here. However, I do understand how complex that can make the compiler and how few bugs there are.

I was trying to achieve this:
For every property an interface has that can be walked, I return it.

I explored a few options for this:

  1. Ensuring I return a Tuple for each property
  2. Ensuring I return all the keys themselves as a tuple (safer type)
  3. The above approach

This above approach works great, originally I had this returning a string index signature and having each case have an inner function that correctly typechecks the object.

(Also, yes I noticed the lack of narrowing in changing the argument type, interesting to see the suggestions in #20375 thank you!)

@mattmccutchen
Copy link
Contributor

I don't have a definite answer about the workaround. I expect that the return type will continue to simplify as you expect at call sites that set T to a particular alternative of AST, such as Program. As I mentioned, the body of pickASTProperties is currently relying on the rule that a type is assignable to an indexed access type that the compiler can't simplify (here Lookup[T['type']]) if the type is assignable to the "constraint" of the indexed access type (here Lookup[AST['type']], which simplifies to Lookup['Program' | 'MemberExpression' | 'FunctionDeclaration'], which simplifies to PickASTKeys<'Program'> | PickASTKeys<'MemberExpression'> | PickASTKeys<'FunctionDeclaration'>). Based on my own limited experience, I believe this rule is harmful and would like to see it changed; I would guess the TypeScript maintainers won't change it, but I could be wrong. Note that with the current rule, the compiler is only checking that what you return is valid for at least one alternative of AST and not that it matches the argument. For example, this compiles.

@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

4 participants