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

Better typings of Object.keys when keys coming from enum #30228

Closed
wants to merge 1 commit into from

Conversation

dubzzz
Copy link

@dubzzz dubzzz commented Mar 5, 2019

Fixes #13254

For the record, back in 2016, there was a comment from @ahejlsberg stating that using keys<T>(o: T): Array<keyof T> would not be safe because:

Calling Object.keys with a Base would return a (keyof Base)[] which is almost certainly wrong

In this precise case an instance of { [K in EnumOfStrings]: any } cannot define keys outside of EnumOfStrings so I think it would be OK.

The PR would at least improve the typings of objects taking their values from enums of strings.

Tested on TypeScript playground:

enum MyKeys {
    A = 'key-a',
    B = 'key-b'
}

type MyObj = { [key in MyKeys]: string };

const o: MyObj = {
    [MyKeys.A]: 'a',
    [MyKeys.B]: 'b',
//    '3': '4' // impossible to have other keys
}

function Object_keys_new<KeyType extends string>(obj: { [key in KeyType]: any }): KeyType[];
function Object_keys_new(obj: {}): string[];
function Object_keys_new(obj: {}): string[] {
    return [];
}

// const a: MyKeys[] = Object.keys(o); // today: type is string[]
const b: string[] = Object.keys({});

const c: MyKeys[] = Object_keys_new(o);
const d: string[] = Object_keys_new({});

More here

@typescript-bot
Copy link
Collaborator

It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src'. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src'.

@j-oliveras
Copy link
Contributor

Last time rejected on PR #28899 (comment) (3 month ago).

Your change will fail:

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

const a: A = {
  a: 1,
  b: 2,
  c: "c",
} as any;

for (const key of Object.keys(a)) {
  // This condition will always return 'false' since the types '"a" | "b"' and '"c"' have no overlap.
  if (key == "c") {

  }
}

key c exists but the compiler says is not valid.

@dubzzz
Copy link
Author

dubzzz commented Mar 5, 2019

@j-oliveras I'm closing the issue for the moment. I'll need to double-check if the case you explained can occur with my proposal

@dubzzz dubzzz closed this Mar 5, 2019
@dragomirtitian
Copy link
Contributor

@j-oliveras But once you start using type assertions, the type system will start to give false positives anyway. For example this raises an error, yet it is not an argument against flow analysis:

const key: "a" = "c" as any;

if (key == "c") {
}

If you stay within the type safe sandbox, when would Object.keys returning Array<keyof T> not be preferable to string[] ?

@dubzzz
Copy link
Author

dubzzz commented Mar 5, 2019

@dragomirtitian
I think the answer to your question is here: #12253 (comment)

@j-oliveras
Copy link
Contributor

@dragomirtitian I used a type assertion to simulate a object with more properties than declared.

@dragomirtitian
Copy link
Contributor

@j-oliveras @dubzzz 10x I got the point from the comment. I guess string is better than getting bad types. In your example a[key] would be number if we had Array<keyof A> which is clearly bad and would lead to it's own can of worms.

@dubzzz
Copy link
Author

dubzzz commented Mar 5, 2019

@dragomirtitian

In the case of my PR I do not exactly get where it can lead to bad types - I do not consider the as any trick as a valid candidate to prove that the PR is doing something wrong otherwise we can say that all the typings would be wrong

The PR only focuses on the very specific case of an object having keys coming from an enum of strings. In my understanding it is not feasible to have other keys than the one of the enum in the case of a type defined as being: type MyObj = { [key in MyKeys]: string }.

@dubzzz
Copy link
Author

dubzzz commented Mar 5, 2019

@RyanCavanaugh I would be interested into having your opinion on this PR

@jack-williams
Copy link
Collaborator

jack-williams commented Mar 6, 2019

@dubzzz

In my understanding it is not feasible to have other keys than the one of the enum in the case of a type defined as being:

The fundamental issue is not have keys outside of a given domain; it's that you can drop keys from within a domain. Once you accept that the type-system can freely drop keys you lose a one-to-one correspondence between type-level keys and run-time keys: the former being and under approximation of the latter.

As a real-word example of where you can get bitten by this is by libraries that use internal properties.

interface MyObj {
    /* @internal */
    [MyKeys.A]: 'a',
    
    [MyKeys.B]: 'b',
}

The corresponding public type of MyObj will not have A, but the value will. This is perfectly reasonable from the type-system POV, but it will make the keys type incorrect.

@RyanCavanaugh
Copy link
Member

In my understanding it is not feasible to have other keys than the one of the enum in the case of a type defined as being: type MyObj = { [key in MyKeys]: string }.

This is not correct, because the type is not exact.

As long as subtyping is legal and exact types don't exist, it will always be possible to have an extra key present in the runtime object whenever an object is aliased:

const f = {
    "key-a": "a" as "a",
    "key-b": "b" as "b",
    "key-c": "c"
};
const g: MyObj = f;
// h: MyKeys[], but contains illegal value "key-c"
const h = Object_keys_new(g);

@dubzzz
Copy link
Author

dubzzz commented Mar 6, 2019

@RyanCavanaugh thanks a lot for the update. I'll definitely keep this PR closed, without exact types it will not be feasible.

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.

6 participants