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

Mapped types fixes #17336

Closed
wants to merge 4 commits into from
Closed

Mapped types fixes #17336

wants to merge 4 commits into from

Conversation

gcnew
Copy link
Contributor

@gcnew gcnew commented Jul 21, 2017

Fixes #17238

This fix changes the rule from #12351 to:

  • The operation { [P in K]: T}[X] is equivalent to an instantiation of T where X is substituted for every occurrence of P, unless K is an indexing operation itself. For example, { [P in K]: Box<T[P]> }[X] is equivalent to Box<T[X]>, however { [P in T[X]]: Box<T[P]> }[X] is not.

This PR also fixes checking of operations done on the object of type indexing, e.g. the following is now correctly identified as error:

// Note that `extra` is not present in { a:'a', b:'a' }
type T5<S extends 'a'|'b'|'extra'> = {[key in { a:'a', b:'a' }[S]]: true}[S]

Examples:

type AB = {
    a: 'a'
    b: 'a'
}

type T1<K extends keyof AB> = { [key in AB[K]]: true }
type T2<K extends 'a'|'b'> = T1<K>[K] // BUG 1: should be error for K = 'b'

type R = AB[keyof AB]; // "a"
type T3 = { [key in R]: true }
type T4<K extends 'a'|'b'> = T3[K] // error as expected

// BUG 2: 'extra' not checked in AB[S]
type T5<S extends 'a'|'b'|'extra'> = {[key in AB[S]]: true}[S]

@DanielRosenwasser
Copy link
Member

@ahejlsberg can you take a look at this PR?

@ahejlsberg
Copy link
Member

@gcnew Thanks for your contribution! You are definitely right about the missing checkSourceElement call call in checkIndexedAccessType. In fact, there should be a call for node.objectType as well.

Regarding the other change, the correct fix is to add a check in getIndexedAccessForMappedType that verifies that the index type is assignable to keyof T for the object type. This is supposed to be true for any indexed access type but ends up not being verified in the generic mapped type case.

I'm going to put up a new PR with these changes, but I will keep the tests you wrote. Thanks again!

@gcnew
Copy link
Contributor Author

gcnew commented Jul 27, 2017

Funny enough, I tried to fix checkIndexedAccessType at first, but due to my lack of knowledge and guided by the debugger, I went for getIndexedAccessType.

Now, I'm looking at another issue. Mapped types buried inside an intersection are eagerly resolved, leading to incorrect behaviour (see #17456 and gcnew@73c78e6). The problem is that if the indexing operation is postponed (as in my experiment), the index type is not properly checked.

@gcnew
Copy link
Contributor Author

gcnew commented Aug 2, 2017

Closing as the bugs have already been addressed by #17455.

@gcnew gcnew closed this Aug 2, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

Bug with mapped types and indexing
4 participants