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

"Indexed access types which immediately index into a mapped object type" infering improvement is unsound #48730

Closed
jfet97 opened this issue Apr 16, 2022 · 4 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@jfet97
Copy link
Contributor

jfet97 commented Apr 16, 2022

Bug Report

πŸ•— Version & Regression Information

When did you start seeing this bug occur? From 4.6, in which indexed access inference was improved.

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

interface TypeMap {
    "number": number;
    "string": string;
    "boolean": boolean;
}

type UnionRecord<P extends keyof TypeMap> = { [K in P]:
    {
        kind: K;
        v: TypeMap[K];
        f: (p: TypeMap[K]) => void;
    }
}[P];

function processRecords<K extends keyof TypeMap>(record1: UnionRecord<K>, record2: UnionRecord<K>) {
    record1.f(record2.v); // <-- this is bad
}

processRecords({
    kind: "string",
    v: "hello!",
    f: val => {
        console.log(val.toUpperCase());
    }
}, {
    kind: "number",
    v: 123,
    f: val => {
        console.log(val.toExponential(3));
    }
})

πŸ™ Actual behavior

The code typechecks, but it breaks at runtime: TypeError: val.toUpperCase is not a function.

πŸ™‚ Expected behavior

I think record1.f(record2.v); shouldn't be allowed.

πŸ—’ Notes

I understand that the type parameter K isn't behaving like an existential type, so the correct way to type processRecords may be:

function processRecords<
    K1 extends keyof TypeMap,
    K2 extends keyof TypeMap>(record1: UnionRecord<K1>, record2: UnionRecord<K2>) {
        record1.f(record2.v); // <-- now this is an error
}

Playground

I think that the main problem here is that the original definition:

function processRecords<K extends keyof TypeMap>(record1: UnionRecord<K>, record2: UnionRecord<K>) {
    record1.f(record2.v);
}

may suggest that the two nodes are related, because this whole feature was added to express correlation between things. But that may not be the case.

And what about a more general one?

function processRecords<K extends keyof TypeMap>(records: UnionRecord<K>[]) {
    records.forEach(r => r.f(r.v))
}

Playground

How could I express the fact that each record is related only to itself? I don't even know if its possible to properly type it, I think variadic type arguments would be needed because I need a variable number of Ks, each of which unrelated to others. Not sure. The above typing lets me do bad things as you can see in the playground.

I tried the following, but it is all broken:

type MapKeyofTypeMapToUnionRecord<K extends (keyof TypeMap)[]> = 
    K extends [infer H extends keyof TypeMap, ...infer REST extends (keyof TypeMap)[]] ? 
        [UnionRecord<H>, ...MapKeyofTypemapToUnionRecord<REST>] :
        []

function processRecords<K extends (keyof TypeMap)[] | []>(records: MapKeyofTypeMapToUnionRecord<K>) {
   records.forEach(r => r.f(r.v)) // broken
}

Playground

@jfet97 jfet97 changed the title Indexed access types which immediately index into a mapped object type improvement seems to be unsound "Indexed access types which immediately index into a mapped object type" infering improvement seems to be unsound Apr 16, 2022
@whzx5byb
Copy link

I agree that processRecords is unsound. It is only sound when K is not a sub union of the constraint, but only "one of" the constraint union. This concept is described in #27808.

@jcalz
Copy link
Contributor

jcalz commented Apr 17, 2022

I think that TS has always had unsoundness on writes to generic indices:

function set<T, K extends keyof T>(obj: T, k: K, val: T[K]) {
    obj[k] = val; // no compiler error, but unsound
}

const obj = { a: 0, b: "" }
set(obj, Math.random() < 2 ? "a" : "b", "");
obj.a.toFixed() // runtime error

Playground link

And presumably this is just another instance of that unsoundness?

I guess I was naively assuming this wouldn't be the case, since one of the points of #30581 was to encode such correlations correctly (not a trivial task, as described in this comment). I'm not sure it's worth the refactoring involved in turning something into a distributive object type form as described in #47109 if you get behavior similar to using a type assertion.

@jfet97 jfet97 changed the title "Indexed access types which immediately index into a mapped object type" infering improvement seems to be unsound "Indexed access types which immediately index into a mapped object type" infering improvement is unsound Apr 17, 2022
@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Apr 18, 2022
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 18, 2022

This was an intentional trade-off. If you're using the mapped type lookup pattern, you should use multiple type parameters if the value can appear in multiple places

function processRecords<K1 extends keyof TypeMap, K2 extends keyof TypeMap>(record1: UnionRecord<K1>, record2: UnionRecord<K2>) {
    // Errors as hoped
    record1.f(record2.v);
}

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants