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

Object.values and Object.entries are unsound and inconsistent with Object.keys. #38520

Open
MicahZoltu opened this issue May 13, 2020 · 28 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@MicahZoltu
Copy link
Contributor

Search Terms:
Object.values Object.entries sound soundness unsound inconsistent Object.keys

Code
Proposed change: MicahZoltu@603c363

Related Issues:
#12207
#12253

Back in November 2016, a PR (#12207) was submitted to make the types of Object.entries and Object.values generic. This PR was reviewed and accepted, and as a note it was recommended that Object.keys be updated similarly. The author then submitted a PR to update Object.keys (#12253) but @ahejlsberg made the very valid point that this change was unsound and the PR was closed. The author of both PRs then suggested that perhaps #12207 should be reverted, but this revert never happened.

This issue is to discuss options for rectifying this situation and I propose we make Object.entries and Object.values consistent with the sound behavior of Object.keys. The major issue here is that this IS a breaking change since people may be relying on the currently unsound behavior of Object.values and Object.entries. However, I think having TypeScript be inconsistent on this front indefinitely is not good, and I don't think changing Object.keys to be unsound is the right solution, so at the least this change should be merged into TS 4.x.

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels May 14, 2020
@RyanCavanaugh
Copy link
Member

It's an unfortunate situation but I can't imagine the upside to moving things in a direction that so many people would simply consider worse, even if it's more correct. I don't know if you've seen it, but we basically get a bug report or PR to change Object.keys to keyof T at least once a week.

@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented May 14, 2020

Yeah, I have seen many of the Object.keys reports and it is a common question in TS Discord, people asking why Object.keys isn't generic. What bugs me the most at the moment is the inconsistency between Object.keys and Object.values/Object.entries. I would be willing to defer to the TS dev team's judgement on whether to go with generics or not and I can submit a PR to "fix" either.

Personally, I think that the sound solution is preferable, but I understand that often the TS dev team makes soundness sacrifices for usability reasons. The main reason I didn't lead with a PR to "fix" Object.keys is because my searching suggests that many have tried this in the past and it is consistently rejected, which is why I thought perhaps going the other direction would get more traction.

@susisu
Copy link

susisu commented Jul 19, 2020

Hi,

I agree with you that Object.values and Object.entries become less confusing if they are soundly typed and consistent with Object.keys.
However, I think there are a few things to consider before changing them.

First, when an object is used as a record (or "dictionary"), the current typing of Object.values and Object.entries are useful.

const r: Record<string, number> = {
  foo: 0,
  bar: 1,
};
const vs = Object.values(r); // : number[]
const es = Object.entries(r); // : [string, number][]

If they are typed as vs: unknown[] and es: [string, unknown][], it becomes much inconvenient.
On the other hand, there is nothing wrong with Object.keys being soundly typed in this situation.

Second, the object spread operator has the same kind of unsoundness.

const a = { foo: 0, bar: 1 }; // : { foo: number, bar: number }
const b = { foo: 2, bar: "bar" };
const c: { foo: number } = b;
const d = { ...a, ...c }; // : { foo: number, bar: number }

As in the above code, the object spread operator ...obj works the same as the current Object.entries.
So I think changing the typing of Object.entries will introduce another kind of inconsistency.

What do you think?

@amakhrov
Copy link

Aha, I found it finally!
I'm having hard enough time justifying it to myself and explaining to my team mates why Object.keys returns string[]. And once I manage that, I have zero explanation why the same doesn't apply to Object.values. Even though the same code example that demonstrates soundness of Object.keys demonstrates the unsoundness of Object.values (or vice versa - depending on your view point).

I personally do not agree the behavior of Object.keys is sound - after all there are quite a lot of other ways to break static type guarantees (@susisu just demonstrated one of them in the comment above).
But no matter what, I believe consistency is even more important.

@devinrhode2
Copy link

I think if Typescript is about improving developer experience, Object.keys should return (keyof T)[], it's easy enough to typecast it to make it be keyof T but, there should at least be something like Object.typeCastedKeys or something of that ilk.

@devinrhode2
Copy link

If we are concerned about actual runtime type checking, TS team should develop a tool to actually check runtime types (during development only)

@samwightt
Copy link

+1 for moving Object.entries and Object.values to the current Object.keys behavior. Out of the two, it's the more sound definition and prevents more unexpected type errors. It also aligns more with how Typescript's type system works, and forces more people to become aware of object type compatibility.

Moving Object.keys to keyof T provides a false sense of safety, but also doesn't align with how Typescript's type system works. Typescript object types declare subsets of every object, not a strict definition of the object's shape. It's important for users to understand that any arbitrary object can be a member of an object type, so long as it has the specified fields, and that when iterating over the object's keys or values, it's possible for there to be an infinite number of keys in addition to the specified ones. Accessing properties on an object is safe, but iterating over object properties may produce an unexpected and unsafe result.

Typescript is not a language for improving your editor's autocomplete, it's a language for preventing type errors. Given how Typescript's type system works, it's safer to use an unknown values type and a string[] key type because those are correct. Again, any arbitrary object can be assigned to another type because of how type compatibility works. The alternative not only gives users a false sense of security, but also encourages misconceptions about how Typescript's type system works.

@mantljosh
Copy link

I don't know if you've seen it, but we basically get a bug report or PR to change Object.keys to keyof T at least once a week.

IMO that should be setting off huge alarm bells that the current typing of Object.keys is a problem. At a minimum as const should work for objects/records in a similar way as they do for arrays.

const arr = ['a', 'b'] as const
arr.map(x => console.log(x)) // x is correctly typed as: 'a' | 'b'

const obj = { foo: 'a', bar: 'b'} as const
Object.keys(obj).map(key => console.log(key)) // key is just string

@samwightt
Copy link

IMO that should be setting off huge alarm bells that the current typing of Object.keys is a problem. At a minimum as const should work for objects/records in a similar way as they do for arrays.

That is still not type safe. TypeScript has no way to guarantee the fields you can iterate over when you’re iterating over all of an object’s keys. Const assertions prevent type widening, but it’s still possible to set arbitrary fields on them by asserting any or something like that. Convenience is not the same thing as type safety.

@amakhrov
Copy link

asserting any kills any type safety. it's not limited to Object.keys. you can shoot yourself in foot with any in every possible way.

@devinrhode2
Copy link

devinrhode2 commented Oct 2, 2021

Honestly, I don't understand this Object.keys situation. I don't recall seeing one good example of how typescript will mislead developers by using keyof T.

I do have an npm package which gives you a well-typed alias. I have a stack overflow answer that details an npm package I created, object-typed: https://stackoverflow.com/a/65117465/565877

@markdascher
Copy link

This confused me too. Since it isn't mentioned here yet, I'll just say the reason for Object.keys() returning string is because types aren't exact. Exact types are a prerequisite for making Object.keys() return keyof T in a sound way. See #12936.

@mdbetancourt
Copy link

mdbetancourt commented Jan 10, 2022

@ahejlsberg said

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.

what about allow something like (keyof Base | string)[]? so everyone is happy (safety and autocomplete) (maybe typescript keeping some information about unions)

@devinrhode2
Copy link

@mdbetancourt I like the effort, but, try putting that into typescript playground:

type myKeys = keyof Base | string

string is wider than keyof Base, and will "swallow" the other type, reducing it to just string.

@mdbetancourt
Copy link

mdbetancourt commented Jan 11, 2022

@devinrhode2

@mdbetancourt I like the effort, but, try putting that into typescript playground:

type myKeys = keyof Base | string

string is wider than keyof Base, and will "swallow" the other type, reducing it to just string.

yup i know :) that's the part of what about allow and maybe typescript keeping some information about unions instead of "swallow" the other type.

related to: #46859 and #46548

@mkantor
Copy link
Contributor

mkantor commented Feb 14, 2022

@devinrhode2

I don't recall seeing one good example of how typescript will mislead developers by using keyof T.

Here's an example. That code will always throw a runtime error (lookupTable[key] is undefined). Without a type assertion the bug is caught by the type checker.

@sylann
Copy link

sylann commented Sep 8, 2022

IMO that should be setting off huge alarm bells that the current typing of Object.keys is a problem. At a minimum as const should work for objects/records in a similar way as they do for arrays.

That is still not type safe. TypeScript has no way to guarantee the fields you can iterate over when you’re iterating over all of an object’s keys. Const assertions prevent type widening, but it’s still possible to set arbitrary fields on them by asserting any or something like that. Convenience is not the same thing as type safety.

Frankly, arguing about the lack of type-safety is laughable considering the current state of Typescript.
There are plenty of pretty common pitfalls that are well beyond typescript's "type-safety".

A simple example that comes to mind :

const arr = ["a", "b"]
type V = typeof arr[number] // string, should be string | undefined
const x = arr[2] // string, should be string | undefined

I agree with @mantljosh that we should consider cases where objects are frozen in priority.

I very often stumble upon the return type of Object.keysbecause I'm often working with simple tables of correspondence.
It is convenient to be able to group an array by id for example.
I also often try to work with unions of known string literals.

I understand there are limitations in the design, and not everything can be done.
But could someone provide a sound example where Object.keys returning (keyof T)[] is actually dangerous?
And by "sound", I mean a snippet of code that is likely to be written, not some theoretical edge-case.

EDIT:

@devinrhode2

I don't recall seeing one good example of how typescript will mislead developers by using keyof T.

Here's an example. That code will always throw a runtime error (lookupTable[key] is undefined). Without a type assertion the bug is caught by the type checker.

Yes, but with a frozen object
your example is perfectly fine.

@amakhrov
Copy link

amakhrov commented Sep 9, 2022

@romain20100

A simple example that comes to mind :

At least noUncheckedIndexedAccess fixes the second part of your example

Yes, but with a frozen object your example is perfectly fine.

It's not the same as the original example (originally lookupTable was not supposed to be of type X).

@mkantor

Here's an example. That code will always throw a runtime error (lookupTable[key] is undefined).

Honestly, this example is manifestation of a different problem (untyped object literals + non-exact types). This problem can occur in different forms. That's why it's a good practice to ensure object literals are explicitly typed. NgRx even has an eslint rule for this (only for one specific case of the problem, though): https://ngrx.io/guide/eslint-plugin/rules/on-function-explicit-return-type

@mkantor
Copy link
Contributor

mkantor commented Sep 11, 2022

@amakhrov it's not just about literals. The same problem occurs any time you have a subtype with additional properties (e.g. here's an example with classes).

But you're right that the lack of exactness is implicated here—TypeScript types only ever describe a subset of the properties which will exist at runtime, so the callback given to Object.keys receives arguments that may be wider than keyof T. I'm hopeful that if the JavaScript "Record & Tuple" proposal is implemented it'll provide some avenue for handling exactness, but time will tell.

@vegerot
Copy link

vegerot commented Oct 11, 2022

An even more minimal example is

type X = {a: unknown}

const f = (x: X) => Object.keys(x) // should expect ['a']

f({a: 1, b: 2}) // but you actually get `['a', 'b']`

This reminds me of variance issues with generics as well

@dpchamps
Copy link

dpchamps commented Oct 11, 2022

I wrote out this appeal just over a year ago, which unfortunately was closed: #45835

I feel that we should still consider it, the crux being:

Within a structural type-system and in the absence of Exact Types, it would appear that most Object reflection methods must be typed imprecisely wide, including Object.entries and Object.values.

But this kind of feels like it violates Typescripts third non-goal: Precise types for object methods are incredibly useful, which is clear by the number of requests, questions and PRs around Object.keys type signature.

Given the state of things it feels like one of two things should happen:

  1. Remove these two type signatures, and revisit when Exact Types land.
  2. Add a more precise Object.keys type definition override, and maintain at least the same level of unsoundness that already exists.

Sample Code

Something like this maintains parity with the status-quo:

interface ObjectConstructor {
    keys<T extends Record<string, unknown>>(o: T): (keyof T)[];
}

Which is not a valid program in the SO example, but demonstrates the same level of unsoundness that already exists

The first part is just a bit of context for why I think one of the two above things should happen
`@jcalz` has enumerated a non-exhaustive list [here](https://github.com//issues/45390#issuecomment-895661910), the issue template links to the [SO Answer](https://stackoverflow.com/questions/55012174/why-doesnt-object-keys-return-a-keyof-type-in-typescript). I'm aware of these resources, I know that this comes up quite frequently, but it would be greatly appreciated if you would hear me out 🙏

It's instructive to go way back to 2016 with this comment in a follow-up PR to #12207 -- which will be relevant in a few seconds.

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.

@RyanCavanaugh sort of codifies these hesitations in the SO answer. The crux of this argument is that a more-precisely typed Object.keys is potentially unsound. We get a first hand demonstration of this by observing some well-typed program that would produce runtime errors.

But TS is already in this state where very similar code, is A) well-typed, but B) will produce runtime errors. This comes down to how Object.values and Object.entries are typed.

See two examples here, using Object.values:

Note: this was known way back in 2016, where @ahejlsberg called this out in the same comment:

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

@probablykasper
Copy link

probablykasper commented Jan 21, 2023

Both object keys and values are already unsound like this:

const x = {
	name: 'test',
	0: true,
}
const y: { name: string } = x
const unsound: Record<string, string> = y

So if the above is possible, wouldn't it be consistent for Object.keys() and Object.values() to be generic?

@guillaumebrunerie
Copy link

@probablykasper This example only shows that Record is unsound, not "object keys and values" in general. I assume that Record being unsound is a design decision, because if you think about it there should be no way to make any element of type Record<string, string> (except with black magic like proxies). It has in any case nothing to do with Object.keys and Object.values.

@probablykasper
Copy link

Ah ok, thank you for explaining

@babur001
Copy link

babur001 commented Jun 3, 2023

Any solutions provided so far?

@babur001
Copy link

babur001 commented Jun 7, 2023

If adding an extra package (maybe already been installed) to your prject is not a problem, you can have a solution with rambda toPairs:

import { toPairs } from "rambda"

toPairs({a: 1, b: 2, c: 3}); //=> [['a', 1], ['b', 2], ['c', 3]]
Screenshot 2023-06-07 at 10 05 08

All Keys types are have been stored!

Docs:

https://ramdajs.com/docs/#toPairs

https://github.com/ramda/ramda/blob/v0.29.0/source/toPairsIn.js

@devinrhode2
Copy link

ts-extras is also a great option with it's objectEntries and objectKeys utils.

Lesser known, Froebel is a lodash like library that is written from the ground up to have excellent typing.

@craigphicks
Copy link

craigphicks commented Jul 23, 2023

[OP] This issue is to discuss options for rectifying this situation and I propose we make Object.entries and Object.values consistent with the sound behavior of Object.keys.

An argument against the assertion that "consistency dictates that Object.entries and Object.values have similar type behavior with the behavior of Object.keys", can be made as follows -

  • prop keys have a very limited range to begin with (<any), whereas prop values can take on any type.
  • prop keys are not as often or as deeply carried in variables into the general program flow as are prop values.
  • most of the time a key being a string type is not a problem, but values having any type would require frequent manual (and therefore error prone) casting to prevent type checking from silently and unexpectedly disappearing.

In short, key types and value types are different animals with different needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests