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

Distributed keyof over union leads to erroneous indexed access #49000

Open
jfet97 opened this issue May 6, 2022 · 11 comments
Open

Distributed keyof over union leads to erroneous indexed access #49000

jfet97 opened this issue May 6, 2022 · 11 comments
Labels
Bug A bug in TypeScript Help Wanted You can do this
Milestone

Comments

@jfet97
Copy link
Contributor

jfet97 commented May 6, 2022

Bug Report

πŸ•— Version & Regression Information

This is the behavior in every version I tried

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

type KeyOf<T> = T extends unknown ? keyof T : never

type Wrong<T> = { [K in KeyOf<T>]: T[K] }

πŸ™ Actual behavior

It is accepted by the type checker, but it shouldn't be allowed because T could be a union of objects with unrelated properties. The computed type doesn't even have any sense:

type test= Wrong<{ prop0: string } | { prop1: number; prop2: boolean } | { prop3: string[] }>
//   { prop0: unknown; prop1: unknown; prop2: unknown; prop3: unknown; }

πŸ™‚ Expected behavior

K shouldn't be allowed to index type T.

@MartinJohns
Copy link
Contributor

This is working as intended. The conditional type KeyOf<T> is distributive. See the documentation: https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#distributive-conditional-types

@jfet97
Copy link
Contributor Author

jfet97 commented May 6, 2022

@MartinJohns I know that KeyOf distributes over union. The problem is that its result is "prop0" | "prop1" | "prop2" | "prop3" and TS let me index the union { prop0: string } | { prop1: number; prop2: boolean } | { prop3: string[] } with that. This shouldn't be allowed.

@MartinJohns
Copy link
Contributor

MartinJohns commented May 6, 2022

But why shouldn't it be allowed? It's a legit operation. The result is the union of each type.

Reading the properties on that type is fine. But yeah, writing to the properties results in unsound behaviour. But that's basically the same as when you assign a variable to any and write to it.

@jfet97
Copy link
Contributor Author

jfet97 commented May 6, 2022

It is not a legit operation. For example you cannot index the first element, { prop0: string }, with the key "prop3". The rule is keyof (A | B) = keyof A & keyof B.

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug Bug A bug in TypeScript Help Wanted You can do this and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels May 6, 2022
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone May 6, 2022
@MartinJohns
Copy link
Contributor

Guess I was wrong. :-D To me it looks legit, but I get what you refer to.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented May 6, 2022

I'm not going to promise that fixing this won't create worse problems and ultimately lead to us declaring this "design limitation", but I agree that there should be an error here

type KeyOf<T> = T extends unknown ? keyof T : never;

type Wrong<T> = { [K in KeyOf<T>]: T[K] }
//                                 ^^^^

because the in-principle equivalent substitution of

type S = { prop0: string } | { prop1: number; prop2: boolean } | { prop3: string[] };
type Temp = KeyOf<S>;
type GoodWrong = { [K in Temp]: S[K] };

produces an error

@jfet97
Copy link
Contributor Author

jfet97 commented May 6, 2022

type KeyOf<T> = [T] extends [unknown] ? keyof T : never;

@RyanCavanaugh Maybe it's just an oversight but the problem is with type KeyOf<T> = T extends unknown ? keyof T : never;, not with type KeyOf<T> = [T] extends [unknown] ? keyof T : never;. If you prevent distribution over union, KeyOf behaves correctly like the keyof type operator.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented May 6, 2022

Oops, that was a bad copy/paste. Edited to fix.

@bentongxyz
Copy link
Contributor

Hi there, I'd like to take a shot on this issue!

@jfet97
Copy link
Contributor Author

jfet97 commented May 9, 2022

@RyanCavanaugh
Minor suggestion: if the compiler isn't always able to prevent T[K] when K is not assignable to keyof T, could make more sense if the default resulting type was never instead of unknown?

@bentongxyz
Copy link
Contributor

Hi @RyanCavanaugh, @jfet97, @MartinJohns,

I created a PR that seem to fix this error.

The problem (paste again below) ...

type KeyOf<T> = T extends unknown ? keyof T : never;
type Wrong<T> = { [K in KeyOf<T>]: T[K] }
//                                 ^^^^ `K` should not be allowed to index `T`

... seems to be that in structuredTypeRelatedTo (checker.ts L19684-L19691), KeyOf<T> is found to be related to T, because keyof T inside T extends unknown ? keyof T : never returns IndexType.

It seems to be a legit operation if e.g. T extends **object** ? keyof T : never instead of T extends [some union]/ unknown ? keyof T : never.

So my working solution is to guard against return IndexType if the above conditions are met.

I place the logic at getTrueTypeFromConditionalType.

However, I am quite new to this and I am not quite sure:

  1. that getTrueTypeFromConditionalType is the right place to place the logic; and
  2. what is the right type to return in place of IndexType (I am returning UnknownType)

Please do give me a few pointers whether I am approaching this correctly.

Please do correct me if I am wrong!

Thank you for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Help Wanted You can do this
Projects
None yet
4 participants