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

Defer indexed access type resolution #17521

Merged
merged 6 commits into from Aug 7, 2017
Merged

Conversation

ahejlsberg
Copy link
Member

With this PR we defer resolution of indexed access types in situations where we were previously too eager.

Fixes #17456.

@gcnew
Copy link
Contributor

gcnew commented Jul 31, 2017

The immediate issue is resolved, however I don't think that's the complete fix. Currently, indexing errors are not properly reported:

type E<T> = { true: 'true' }[ObjectHasKey<T, '1'>] // should error, `false` not handled
type ErrorExample = E<[string]> // any :/

Here's the complete set of tests I'm using:

type StringContains<S extends string, L extends string> = (
    { [K in S]: 'true' } &
    { [key: string]: 'false'}
  )[L]

type ObjectHasKey<O, L extends string> = StringContains<keyof O, L>

type A<T> = ObjectHasKey<T, '0'>

type B = ObjectHasKey<[string, number], '1'> // "true"
type C = ObjectHasKey<[string, number], '2'> // "false"
type D = A<[string]> // "true"

// should be an error, `false` not handled
type E<T> = { true: 'true' }[ObjectHasKey<T, '1'>]

type Juxtapose<T> = ({ true: 'otherwise' } & { [k: string]: 'true' })[ObjectHasKey<T, '1'>]

// "otherwise" is missing
type DeepError<T> = { true: 'true' }[Juxtapose<T>]

// should be OK
type DeepOK<T> = { true: 'true', otherwise: 'false' }[Juxtapose<T>]

@ahejlsberg
Copy link
Member Author

@gcnew With latest commits we can now properly reason about higher-order types of the form (T & { [x: string]: D })[K]. That's probably as far as we can go along this path, we're definitely at the outer edges of what this was designed for. 😃

@gcnew
Copy link
Contributor

gcnew commented Aug 2, 2017

Thank you!

I'm definitely aware we are on the edge here. I've been pondering whether it is more correct to leave the behaviour as is in master, or fix it. On the one hand, the & operator distributes the & operation on colliding properties, which means that { true: true } & { [key: string]: string } should transform to { true: true & string } & { [key: string]: string }. The possible property types thus are (true & string) | string, reducible to just string (the status quo). On the other hand, that's not a faithful representation of what might happen at runtime. Expression level indexing also picks the type of the property if such a property exists.

All in all, I'm leaning towards the second option, the one implemented by this PR. It's unfortunate that & has the strange semantics it has and the complications they carry with them.

@KiaraGrouwstra
Copy link
Contributor

{ true: true } & { [key: string]: string } should transform to { true: true & string } & { [key: string]: string }

Would this PR break trying to use property access to grab just true from this?

@gcnew
Copy link
Contributor

gcnew commented Aug 7, 2017

@tycho01 As implemented in this PR, indexing will return just true if the key exists.

I was making an argument that the & operator is somewhat inconsistent and was wondering whether the current (the one in master) behaviour is more correct given the semantics & has.

@KiaraGrouwstra
Copy link
Contributor

@gcnew: Yeah, I'll admit it's hard to foresee. Hopefully the test harness will help find out...

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned in person that using TypeFlags.Index requires a bit of implicit knowledge. Specifically, such types will only even exist when a lookup type can't operate eagerly. In other words, in keyof T, T must be a type variable to even have something with TypeFlags.Index. An assertion about that somewhere would be helpful, but a long-term way of documenting that would potentially go farther.

Otherwise, the changes seem good to me. I don't see the above being a blocker.

@ahejlsberg
Copy link
Member Author

@gcnew @tycho01 The changes related to transforming a mapped type (T & U & { [x: string]: D })[K] into (T & U)[K] | D more accurately reflect what actually happens when type checking a member access. Given a member access obj.x, where obj is an intersection type, an index signature in the type of obj comes into effect only if none of the constituent types have a member x.

@ahejlsberg ahejlsberg merged commit 313c93c into master Aug 7, 2017
@ahejlsberg ahejlsberg deleted the deferLookupTypeResolution branch August 7, 2017 15:25
@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.

None yet

5 participants