Skip to content

Infer object type from string literal to keyof T #19227

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

Merged
merged 4 commits into from
Nov 16, 2017

Conversation

sandersn
Copy link
Member

Fixes #18715
Supercedes #19159

  1. When inferring from a string literal [union] type to a index type, infer a object type with properties whose names match the constituents of the union type. The type of all the properties is {}.
  2. Infer index types contravariantly.

For example, when inferring "foo" | "bar" to keyof T, the inference algorithm now infers { foo: {}, bar: {} } to T. When inferring string to keyof T, the inference algorithm now infers { [s: string]: {} } to T.

Note that I don't have a test for the change to contravariant inference. I'd appreciate any help coming up with a test that fails without the change and passes with it.

1. When inferring from a string literal [union] type to a index type,
infer a object type with properties whose names match the constituents of the
union type. The type of all the properties is `{}`.
2. Infer index types contravariantly.

For example, when inferring `"foo" | "bar"` to `keyof T`, the inference
algorithm now infers `{ foo: {}, bar: {} }` to `T`. When inferring
`string` to `keyof T`, the inference algorithm now infers `{ [s:
string]: {} }` to `T`.
@sandersn sandersn requested a review from ahejlsberg October 16, 2017 20:39
@DanielRosenwasser
Copy link
Member

Let's try to chat about this some time this week. There are definitely some scenarios I'd like to see if we can make better with this.

CC @HerringtonDarkholme @ktsn

@sandersn
Copy link
Member Author

Good idea — without some additional motivation, @ahejlsberg and I were not sure whether fixing this bug was worth the extra code. I just wrote it because I got started on the problem and wanted to see it through.

@sandersn
Copy link
Member Author

Here's a simplified version of the vue d.ts that would benefit from this inference:

declare function before<T>(propertyNames: T[]): Record<T, any>;
let v1 = before(['foo', 'bar'])
v1.foo = v1.bar // ok, two properties exist, both are type any
v1.foo.who = 'knows'

declare function after<T>(propertyNames: (keyof T)[]): T;
let v2 = after(['foo', 'bar'])
let v3 = after<{ foo: number, bar: string }>(['foo', 'bar'])

v2.foo = v2.bar // ok, two properties exist, both are type {}
v3.foo = v3.bar // error, two properties exist, but their types differ

v2.foo.who = 'knows' // error, foo: {} and has no property `who`

Notice that the version with inference to keyof allows users to specify the type parameter explicitly and give types to their properties. This is not possible with the current version of vue.d.ts.

However, in the inferred case, vue.d.ts actually needs the resulting properties to be of type any (or maybe { [s: string]: any }). Since they are read from in other parts of the Vue object, {} doesn't suffice.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 9, 2017

@sandersn is this ready to go?

@sandersn
Copy link
Member Author

sandersn commented Nov 9, 2017

No, @DanielRosenwasser @ahejlsberg and I need to decide whether to infer {} or any, and whether the feature is valuable enough to matter. We can discuss in person tomorrow I think.

@sandersn sandersn merged commit c698a2b into master Nov 16, 2017
@sandersn sandersn deleted the infer-object-type-from-string-literal-to-keyof-T branch November 16, 2017 23:07
@HerringtonDarkholme
Copy link
Contributor

Sorry for the late reply. Quite busy with my day jobs these days.

Inferring from keyof is amazing! I can see how Vue's typing can be greatly simplified by this great feature.

Currently keyof T will infer the property value type to {} rather than any. It would be a little bit inconvenient in our case. Is there any chance to improve it?

@sandersn
Copy link
Member Author

Nice catch @HerringtonDarkholme! Looks like I forgot to change it to any before merging. I’ll send out another PR.

@sandersn
Copy link
Member Author

OK, the fix is at #21271.

@gcnew
Copy link
Contributor

gcnew commented Jan 18, 2018

Will it trigger a noImplicitAny error? If not, this feature will be dangerous.

@HerringtonDarkholme
Copy link
Contributor

@gcnew Yes, I also think unconditional any might be undesirable.

However, if we do trigger a noImplicitAny, how can we turn off it, other than turning off the flag itself? @ts-ignore is also bad IMHO.

declare function inference1<T>(name: keyof T): T;
inference('foo') // implicit any

For trivial code like above users can provide type annotation in the function call. But real world usage might have keyof deeply nested in other types (e.g. Vue's extend function). It is very unwieldy to annotate it.

Another expedient is generating different type under different flags, it is not ideal. But it might be the most convenient inference for users. And we do have flag sensitive inference like narrowing implicit any.

What's your opinion?

@gcnew
Copy link
Contributor

gcnew commented Jan 19, 2018

Yes, I agree that inferring {} is kind of limiting. However, I think we should stick to it and maybe add improvements later. Making the compiler loose might soon be regretted and there will be no way back. As you know, we are already suffering from such decisions, let's not add more.

I'm not a huge fan of altering the types based on flags. I've seen code that compiles fine with the strictest flags but not without them.

Unfortunately, I don't understand the use-cases very well and I'm unable to see what other options there are or the current limitations.

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants