Object.keys() types refinement, and Object.entries() types bugfix

[ethanresnick]

Per @mhegazy's suggestion in #12207, this pr applies the logic from that PR to Object.keys.

It also fixes a bug in that PR (namely, supporting Object.entries([/* some array */])) and adds a test case for that. Finally, it requires that the keys in Object.keys and Object.entries entries be strings, even though the keyof operator can return string | number. (That part of the PR is now covered by #12425.)

[msftclas]

Hi @ethanresnick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[mhegazy]

key of T & string is just string. so this does not help you much.

keys<T>(o: Array<T>): string[];
keys<T>(o: T): (keyof T)[];

[ethanresnick]

@mhegazy Edit: sorry, I misread your comment.

key of T & string is often ("key1" & string) | ("key2" & string) | ... | ("keyn" & string), not just string.

The intersection is needed because sometimes keyof T is string | number, and we want to exclude number as a possible type for the entry keys. See this test case: https://github.com/Microsoft/TypeScript/pull/12253/files#diff-3ed8d911aae864ffc1d88e62bcb8dc47R15

[ethanresnick]

These intersections do make the intellisense popups uglier, but they're required for correctness.

Could the solution to that ugliness be for the compiler to do some work simplifying intersections (and unions)? E.g., 'a' & string could just be simplified to to 'a' (or, more generally, type & subType could just be simplified to subType). Any efforts in that direction could also help with the bug I brought up in #11426. Would this be that hard? I'd kind of expect simplifying logical formulations to be a somewhat solved problem with theorem provers etc, though I haven't looked into it, so maybe that's incredibly naive. Or maybe it's way too slow to apply here in the general case. Still, specific simplification rules might be a good idea.

[mhegazy]

i would rather leave it string[] then. adding the & string removes any type safety the literal types give you.

[ethanresnick]

adding the & string removes any type safety the literal types give you.

I don't really understand that. Why would you lose type safety? The & string version still only allows certain strings, rather than all strings, which is more safe.

Let me give the real code example that motivated these two PRs:

const loggers = {
  info: ...,
  warn: ...,
  error: ...
}

Object.entries(loggers).forEach(([name, logger]) => {
  // In here, I want typeof name to be "info" | "warn" | "error", so that 
  // the line below will fail if I add a key to loggers by mistake that
  // doesn't have a corresponding method on console. (This in fact
  // happened: I originally had a debug logger, but only later realized
  // that there is no console.debug() on the node console object.)

  logger.log = console[name].bind(console);
})

To make that code work, string[] isn't enough for typeof name. The change merged in #12207 meanwhile, works perfectly, but fails in the somewhat obscure case that motivated the & string addition in this PR. Using & string, the type of name is inferred as ("info" & string) | ("warn" & string) | ("error" & string). That should, if I understand correctly, be equivalent to "info" | "warn" | "error". However, I instead get an error. That seems like a separate bug, though (opened issue #12289 for it), also tied to simplification.

Going forward... if #12289 were addressed, I think the & string would be the right type signature for Object.keys/entries. But, if #12289 can't be easily addressed, maybe it's better to ignore case that & string was meant to solve, rather than giving up on having key-named literals entirely. If we did that, the signature would be:

keys<T>(o: Array<T>): string[];
keys<T extends { [key: string]: any }>(o: T): (keyof T)[];
keys(o: any): string[];

Then, Object.keys(loggers) would correctly be ("info" | "warn" | "error")[], but let y: { [x: string]: any } = {}; Object.keys(y) would be (string | number)[] rather than string[]. Frankly, that sounds like a net win.

[ethanresnick]

@mhegazy I've updated this PR to replace keyof T & string with simply keyof T, as the string intersections are no longer necessary now that keyof always returns a string (from #12425). So this should be good to go.

Cc @ahejlsberg: you expressed skepticism about typing Object.keys with keyof in #12289 (comment). But if/now that for in is typed with keyof (#12314), it seems like this makes sense, no?

[ahejlsberg]

@ethanresnick My reservations about Object.keys returning (keyof T)[] still hold. It makes sense only in the domain of type variables (i.e. T and keyof T). Once you move to the instantiated type world it degenerates because an object can (and often does) have more properties at run-time than are statically known at compile time. For example, imagine a type Base with just a few properties and a family of types derived from that. Calling Object.keys with a Base would return a (keyof Base)[] which is almost certainly wrong because an actual instance would be a derived object with more keys. It completely degenerates for type {} which could be any object but would return never[] for its keys.

BTW, I have the same reservations about Object.entries which I notice is now typed this way.

Note that the changes we made to for-in specifically apply only when objects have a type parameter type. In for (let x in obj) ... where obj is of type T, if T is a type parameter we infer keyof T for x, but otherwise we infer type string. To do the same for Object.keys would require some way to constrain a type argument to only be type parameter, which currently isn't possible.

[ethanresnick]

@ahejlsberg What you're saying makes sense, assuming I'm understanding it correctly, makes sense. I'll close this PR then. Feel free to revert #12207 as well.

[Deilan]

@ahejlsberg And what about using it on Readonly<T> type?

[dyst5422]

When an object has an index though, shouldn't it return the index type?

const a: {[key: number]: any} = {};
Object.keys(a) // should be a number

Basically with an index, we are saying that the index HAS to be a certain type, so the above concern about dynamic keys doesn't follow.

...unless javascript casts to strings in Object.keys...

[NN---]

@dyst5422 Keys are returner as strings.
As well in for..in you get strings.

var a={1:2};
alert(typeof Object.keys(a)[0])

[ccorcos]

@ahejlsberg I'm not sure I understand your reasoning. If you're object has type {[key: string]: number} then Object.keys will return string[]. But if you object is a literal type {a: 1, b: 2} then the return type will be Array<"a" | "b">. That makes sense doesnt it?

[pelotom]

@ccorcos no, because lots of objects with other extraneous keys can have type { a: 1; b: 2 }. For example,

declare const x: { a: 1, b: 2, c: 3 }
const y: { a: 1; b: 2 } = x

for (const k in Object.keys(y)) {
  // what should be the type of k?
}

k: 'a' | 'b' is wrong, because k will take on the value 'c' as well!

[ccorcos]

I see. Sounds like that should be a type error then. haha

[aaronbeall]

I understand the reasoning here and it makes sense, but --strict just made this a lot more annoying because now with strictFunctionTypes you can no longer do this:

interface Foo { foo: string; }
declare const foo: Foo

Object.keys(foo).forEach((key: keyof Foo) => { }) // Error

Error: Argument of type '(key: "foo") => void' is not assignable to parameter of type '(value: string, index: number, array: string[]) => void'.

Which is a pain because with noImplicitAny you can't use string as a lookup:

Object.keys(foo).forEach(key => {
  foo[key] // Error: Element implicitly has an 'any' type because type 'Foo' has no index signature.
})

You have to assert key as keyof T to use it as a lookup.

That's pretty annoying. Is there no way we could allow key to be keyof T? (For cases you're sure T only actually has keys of T)? Since AFAIK there's no way to actually check at runtime that key is in keyof T there seems to be no safe way to ensure that's true, so an assertion seems necessary (without bringing in a runtime type system of some kind).

[chriskrycho]

Yeah, the only other workaround also ends up being kind of gross to write. You can write it, but it's gross:

(Object.keys(foo) as (keyof Foo)[]).forEach((key) => { ... })

I just found my way here after writing up an issue around that b/c of the really poor ergonomics associated with it. Perhaps this could work if the exact types proposal landed? 🤔

[pelotom]

I'm not sure why this is worth debating here. It's really easy to write your own function which does this "at your own risk" if you want it:

export function keys<O>(o: O) {
  return Object.keys(o) as (keyof O)[];
}

[chriskrycho]

FWIW, my comment wasn't offered as "debate." I was mostly thinking out loud if there were ways to get both what many users will expect and satisfy the point made up-thread last year. 🙂

[domoritz]

I believe the updated version for 2.9 is

export const keys = Object.keys as <T>(o: T) => (Extract<keyof T, string>)[];