Skip to content

Handle generic mapped types in getTypeOfPropertyOfContextualType. #27586

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 3 commits into from
Mar 12, 2019

Conversation

mattmccutchen
Copy link
Contributor

Fixes #24694.

@yortus
Copy link
Contributor

yortus commented Oct 11, 2018

Thanks @mattmccutchen, I hope this gets merged. I added a bunch of test cases in #24694 (comment) that I think should be resolved by this.

@yortus
Copy link
Contributor

yortus commented Oct 11, 2018

@mattmccutchen I checked out your branch to see the effect on my repro cases, and got the following:

type TakeString = (s: string) => any;

// Various functions accepting an object whose properties are TakeString functions.
// Note these all use mapped types.
declare function mapped1<T extends {[P in string]: TakeString}>(obj: T): void;
declare function mapped2<T extends {[P in keyof T]: TakeString}>(obj: T): void;
declare function mapped3<T extends {[P in keyof any]: TakeString}>(obj: T): void;
declare function mapped4<T>(obj: T & {[P in keyof T]: TakeString}): void;
declare function mapped5<T, K extends keyof T>(obj: T & {[P in K]: TakeString}): void;
declare function mapped6<K extends string>(obj: {[P in K]: TakeString}): void;
declare function mapped7<K extends keyof any>(obj: {[P in K]: TakeString}): void;
declare function mapped8<K extends 'foo'>(obj: {[P in K]: TakeString}): void;
declare function mapped9<K extends 'foo'|'bar'>(obj: {[P in K]: TakeString}): void;

//                          CURRENT MASTER                                  THIS PR
//                          ==============                                  =======
mapped1({foo: s => 42});    // OK: 's' is contextually typed as 'string'    ✓ OK (unchanged)
mapped2({foo: s => 42});    // ERROR: 's' implicitly has an 'any' type      ? ERROR: s still 'any'
mapped3({foo: s => 42});    // OK: 's' is contextually typed as 'string'    ✓ OK (unchanged)
mapped4({foo: s => 42});    // ERROR: 's' implicitly has an 'any' type      ? ERROR: s still 'any'
mapped5({foo: s => 42});    // OK: 's' is contextually typed as 'string'    ✓ OK (unchanged)
mapped6({foo: s => 42});    // ERROR: 's' implicitly has an 'any' type      ✓ OK: s now 'string'
mapped7({foo: s => 42});    // OK: 's' is contextually typed as 'string'    ✓ OK (unchanged)
mapped8({foo: s => 42});    // ERROR: 's' implicitly has an 'any' type      ✓ OK: s now 'string'
mapped9({foo: s => 42});    // OK: 's' is contextually typed as 'string'    ✓ OK (unchanged)

So mapped6 and mapped8 now get contextual typing, which is great.

However mapped2 and mapped4 still don't work. Any idea why? Are these constructions just wrongly put together, or is this some other compiler pathway that doesn't do contextual typing?

@mattmccutchen
Copy link
Contributor Author

mattmccutchen commented Oct 11, 2018

@yortus Under the current compiler design, mapped2 and mapped4 have a chicken-and-egg problem: before we can confirm that the foo property is a member of the mapped type to get its contextual type, we need to know what T is, and to determine T, we have to determine the type of the entire argument. This doesn't explain why mapped5 through mapped9 work with or without the PR. Other weird things are going on, and I want to try to understand them.

You can give the compiler a hint about the contextual type by unioning in a type with an index signature:

declare function mapped2b<T extends {[n: string]: TakeString} | {[P in keyof T]: TakeString}>(obj: T): void;
declare function mapped4b<T>(obj: T & ({[n: string]: TakeString} | {[P in keyof T]: TakeString})): void;
mapped2b({foo: s => 42});  // OK
mapped4b({foo: s => 42});  // OK

@yortus
Copy link
Contributor

yortus commented Oct 11, 2018

Haha, and I thought I had tried every way to express it 😄. There is definitely an art to constructing some types.

@mattmccutchen
Copy link
Contributor Author

@yortus Now I think I really figured out what is going on in your test cases. (Good test cases!) TypeScript has a rule that if you try to access the members of a generic mapped type {[P in K]: X} (by dot notation, element access, etc.), the members are based on the base constraint of K; for example, if the constraint of K is "foo", you are supposed to get a foo property, and if the constraint is string, you are supposed to get a string index signature. (Of course, this is unsound because K can be a subtype of its constraint, meaning the mapped type actually has fewer members, but that's beside the present point.) This rule should apply to cases 5-9, but it currently has a bug (#27819) that affects cases 6 and 8. Although the main purpose of this PR is to handle the kind of example in #24694, the PR has the side effect of patching up cases 6 and 8 because on the first pass of inference (where context-sensitive functions are excluded), we get an inference K = "foo", and then on the second pass of inference, the code in this PR instantiates the parameter type of mapped6 or mapped8 based on this inference for K.

In cases 2 and 4, the first pass makes no inference at all (because the inference for T would contain excluded context-sensitive functions and such inferences are not allowed), so the PR doesn't do anything. When we fall back to try to get the members of {[P in keyof T]: TakeString} directly, we take the constraint of T, which is {[P in keyof T]: TakeString}, and trigger a recursion limiter that leaves us with no members. The end result is that we have no contextual type for foo.

I realize now that if I could make resolveMappedTypeMembers use the contextual mapper, that would likely achieve the same end result as the current PR without having to introduce a separate code path in getTypeOfPropertyOfContextualType. However, there's caching going on in resolveMappedTypeMembers, so I'm afraid of breaking things in bizarre ways if I try to use the contextual mapper there. With the current approach, any bizarreness is at least contained to the contextual typing process.

The changes to existing baselines look acceptable to me.

Fixes microsoft#24694.
@yortus
Copy link
Contributor

yortus commented Oct 12, 2018

OK, so there are several separate issues brought up by the those test cases. In any case, contextual typing works much better in our project with the fix presented here. I do hope it gets merged.

I appreciate the time you have put into sleuthing through the compiler internals to figure this out.

@weswigham weswigham merged commit 6c6f216 into microsoft:master Mar 12, 2019
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.

3 participants