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

Generic type alias produces erroneously compatible types #31006

Closed
jcalz opened this issue Apr 18, 2019 · 8 comments
Closed

Generic type alias produces erroneously compatible types #31006

jcalz opened this issue Apr 18, 2019 · 8 comments
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@jcalz
Copy link
Contributor

jcalz commented Apr 18, 2019

TypeScript Version: 3.5.0-dev.20190417

Search Terms:
generics, resolved, nested generics, specified, assignable, type safety, type alias, compatible

Code

interface A { a: number; }
interface B { b: string; }

type Demo<T> = <K extends keyof T> (key: K, val: T[K]) => void

// Demo<A> should not be assignable to Demo<B>, but it is?!
declare const implDemoA: Demo<A>;
const implDemoB: Demo<B> = implDemoA; // no error!? 😕

Expected behavior:
There should be an error that a type Demo<A> is not assignable to Demo<B>.

Actual behavior:
It compiles with no error.

I guess the compiler "forgets" what concrete type T is, since it doesn't appear directly in the resulting type. If one manually substitutes A and B for T into the definition of Demo<T> to make DemoA and DemoB, everything behaves as expected:

// Note that we can manually make a concrete type DemoA and DemoB:
type DemoA = <K extends keyof A>(key: K, val: A[K]) => void;
type DemoB = <K extends keyof B>(key: K, val: B[K]) => void;

type MutuallyAssignable<T extends U, U extends V, V = T> = true;
// the compiler agrees that DemoA and Demo<A> are mutually assignable
declare const testAWitness: MutuallyAssignable<Demo<A>, DemoA>; // okay
// the compiler agrees that DemoB and Demo<B> are mutually assignable
declare const testBWitness: MutuallyAssignable<Demo<B>, DemoB>; // okay

// And the compiler *correctly* sees that DemoA is not assignable to DemoB
declare const implDemoAConcrete: DemoA;
const implDemoBConcrete: DemoB = implDemoAConcrete; // error as expected
//    ~~~~~~~~~~~~~~~~~ <-- Type 'DemoA' is not assignable to type 'DemoB'.

This comes from a question on Stack Overflow.

Playground Link:
👩‍💻🔗

Related Issues:
I'm not finding anything I am confident is related: 🔍👀∅⁉

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 18, 2019

Behavior dates back to TS 2.1 when the syntax was first legal.

@RyanCavanaugh
Copy link
Member

And it's not like we don't know

type Params<T> = T extends (arg: infer A, arg2: infer B) => any ? [A, B] : never;
type DA = Params<Demo<A>>; // ['a', number]
type DB = Params<Demo<B>>; // ['b', string]
declare let da: DA, db: DB;
da = db; // Error

🤷‍♀️

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Apr 18, 2019
@ahejlsberg
Copy link
Member

The key issue here is that we erase function type parameters (i.e. replace them with any) when we relate signatures that are each instantiations of the same underlying signature. Extract from signaturesRelatedTo:

if (getObjectFlags(source) & ObjectFlags.Instantiated && getObjectFlags(target) & ObjectFlags.Instantiated && source.symbol === target.symbol) {
    // We have instantiations of the same anonymous type (which typically will be the type of a
    // method). Simply do a pairwise comparison of the signatures in the two signature lists instead
    // of the much more expensive N * M comparison matrix we explore below. We erase type parameters
    // as they are known to always be the same.
    for (let i = 0; i < targetSignatures.length; i++) {
        const related = signatureRelatedTo(sourceSignatures[i], targetSignatures[i], /*erase*/ true, reportErrors);
        if (!related) {
            return Ternary.False;
        }
        result &= related;
    }
}

This obviously looses some information, but that's generally fine as long as the type parameters have no constraints, non-generic constraints, or constraints that reference other type parameters in the same type parameter list, because they're then known to be identical in the two signatures we're relating. However, when type parameters have constraints that reference outer type parameters (as is the case in the Demo<T> type above) we have an issue. In this particular example we end up getting the variance probing wrong and then fall back to a structural comparison that gets it even more wrong (because we resolve T[any] for a non-generic T to just any).

I think the solution is to apply getBaseSignature to each of the signatures before we relate them. That instantiates type parameters to their constraints instead of any. Doing so reveals another issue (we're not relating callbacks with type predicates correctly and Array<T> uses that in its filter method) but I think that's fixable too.

@weswigham
Copy link
Member

I think we should proooobably also move the sourceSignatures.length === 1 && targetSignatures.length === 1 case to the top.

@ahejlsberg ahejlsberg added Bug A bug in TypeScript Fixed A PR has been merged for this issue and removed Needs Investigation This issue needs a team member to investigate its status. labels Apr 19, 2019
@ahejlsberg ahejlsberg added this to the TypeScript 3.5.0 milestone Apr 19, 2019
@ahejlsberg
Copy link
Member

@weswigham No, the current ordering is good. We definitely want to first recognize instantiations of the same signature because that tells us that function type parameters (if any) are identical, allowing us to erase them to their constraints.

@ahejlsberg
Copy link
Member

The Definitely Typed valiation suites turned up several packages that are affected by this (errors and performance regressions). We need some more time to analyze and decide whether the cure is worse than the disease.

@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Aug 31, 2020
@DetachHead
Copy link
Contributor

it looks like this was fixed years ago lol
playground

@RyanCavanaugh
Copy link
Member

Indeed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants