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

KeysOfUnion: Fix assignability with keyof #1009

Merged

Conversation

som-sm
Copy link
Collaborator

@som-sm som-sm commented Dec 16, 2024

Fixes #1008

To make keyof T assignable to KeysOfUnion<T>, we can simply add an union with keyof ObjectType to the existing type, as shown below.

type KeysOfUnion<ObjectType> =
 | keyof ObjectType
 | ObjectType extends unknown ? keyof ObjectType : never;

However, I couldn’t find a straightforward way to prevent KeysOfUnion<T> from being assignable to keyof T. So, I reworked the implementation to use UnionToIntersection, which resolves both issues.


There's another hack I figured out while experimenting, this also solves both the issues, but it's very hacky:

type KeysOfUnion<ObjectType> =
  | keyof ObjectType // Convinces TS that keyof T is part of KeysOfUnion<T>
  | ObjectType extends unknown ? keyof ObjectType: never
  | Extract<Uppercase<string>, Lowercase<ObjectType & string>>; // Convinces TS that KeysOfUnion<T> is broader than keyof T

// keyof T should be assignable to KeysOfUnion<T>
type Assignability2<T, K extends KeysOfUnion<T>> = unknown;
type Test2<T> = Assignability2<T, keyof T>; // doesn't error now

// KeysOfUnion<T> should not be assignable to keyof T
type Assignability1<T, K extends keyof T> = unknown;
type Test1<T> = Assignability1<T, KeysOfUnion<T>>; // does error now

Playground

? keyof ObjectType
: never;
export type KeysOfUnion<ObjectType> =
// Hack to fix https://github.com/sindresorhus/type-fest/issues/1008
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way not hack?

Copy link
Collaborator Author

@som-sm som-sm Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be, but I couldn't figure out anything else.

Do you have any ideas or suggestions?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to do this for now until we come up with a better solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree!
And this is probably a bug, refer this comment.

Copy link
Collaborator Author

@som-sm som-sm Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, removed intersection with KeysOfUnions as it's not required after this fix, refer db0ce32.

expectType<'a' | 'b'>(actual3);

// KeysOfUnion<T> should NOT be assignable to keyof T
type Assignability1<T, _K extends keyof T> = unknown;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add test:

type Assignability1<T extends UnknownRecord, K extends keyof T> = unknown;
type Test1<T extends UnknownRecord> = Assignability1<T, KeysOfUnion<T>>; // should error

Copy link
Collaborator Author

@som-sm som-sm Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Emiyaaaaa The test you suggested doesn’t seem to produce an error:

type Assignability5<T extends UnknownRecord, _K extends keyof T> = unknown;
type Test5<T extends UnknownRecord> = Assignability5<T, KeysOfUnion<T>>; // Doesn't error

However, adding slightly different constraints does produce the expected error:

type Assignability5<T extends Record<string, unknown>, _K extends keyof T> = unknown;
// @ts-expect-error
type Test5<T extends Record<string, unknown>> = Assignability5<T, KeysOfUnion<T>>; // Does error
type Assignability5<T extends object, _K extends keyof T> = unknown;
// @ts-expect-error
type Test5<T extends object> = Assignability5<T, KeysOfUnion<T>>; // Does error

Unsure what's happening here, any ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have idea right now, maybe it's a bug of Typescript I'm not sure. I'll try to look into this interesting issue later

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tsd has some flaws with type checking, try assertions using expect-type instead: https://github.com/mmkal/expect-type

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sindresorhus These tests don't use either tsd or expect-type, they are just simple generics, like this:

type Assignability5<T extends Record<string, unknown>, _K extends keyof T> = unknown;
// @ts-expect-error
type Test5<T extends Record<string, unknown>> = Assignability5<T, KeysOfUnion<T>>; // Does error

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, maybe just add your tests, and then add type Assignability5<T extends UnknownRecord, _K extends keyof T> = unknown; type Test5<T extends UnknownRecord> = Assignability5<T, KeysOfUnion<T>>; // Doesn't error commented out with a comment that says that they don't produce error even though they should.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup makes sense, added all the above mentioned cases and some more, refer 2629884.

@som-sm som-sm force-pushed the fix/keys-of-union-assignability branch from 75f2018 to db0ce32 Compare January 20, 2025 11:06
@sindresorhus sindresorhus merged commit 4789c7c into sindresorhus:main Jan 20, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assignability b/w generic keyof T and KeysOfUnion<T>
3 participants