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

lib: Improve Object.values and Object.entries #5663

Closed
wants to merge 1 commit into from
Closed

lib: Improve Object.values and Object.entries #5663

wants to merge 1 commit into from

Conversation

zertosh
Copy link
Member

@zertosh zertosh commented Jan 15, 2018

Use $Keys and $Values to better reflect that actual types of Object.values and Object.entries. Fixes #2804.

@vkurchatkin
Copy link
Contributor

This is really unsound in general.

@zertosh
Copy link
Member Author

zertosh commented Jan 15, 2018

@vkurchatkin how so?

@vkurchatkin
Copy link
Contributor

Because $Keys and $Values return statically known properties, while an object might have more properties at runtime.

@TrySound
Copy link
Contributor

@vkurchatkin Which ones? Prototype is not handled.

The Object.keys() method returns an array of a given object's own enumerable properties, in the same order as that provided by a for...in loop (the difference being that a for-in loop enumerates properties in the prototype chain as well).

@samwgoldman
Copy link
Member

Vladimir is right. There are two issues.

Width subtyping:

var o: { p: number, q: string } = { p: 0, q: "", foo: "foo" };
var keys: "p" | "q" = Object.keys(o); // lies
var vals: number | string = Object.values(o); // lies

Proto vs. own

var o: { p: number, q: string } = Object.create({ p: 0, q: "" });
var keys: Array<"p" | "q"> = Object.keys(o); // lies
var vals: Array<number | string)> = Object.values(o); // lies

Exact object types are great for the former case, because they by definition exclude values having more properties. For the latter case, we really need to provide better mechanisms to specify object layout as part of types, since so many APIs depend on it. It's on our radar.

Closing this because the implementation is unsound in a way that I'm not comfortable with. I think if we can continue wearing the hair shirt for a bit longer we can come up with some useful+principled solutions to make idiomatic code pass with simple types.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants