Skip to content

Distribute 'keyof' union types#23626

Closed
ahejlsberg wants to merge 4 commits intomasterfrom
distributeKeyofUnion
Closed

Distribute 'keyof' union types#23626
ahejlsberg wants to merge 4 commits intomasterfrom
distributeKeyofUnion

Conversation

@ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Apr 23, 2018

With this PR we transform keyof applied to a union type to an intersection of keyof applied to each union constituent. In other words, we rewrite types of the form keyof (A | B) to keyof A & keyof B.

Some examples:

type A = { a: string, c: string };
type B = { b: string, c: string };

type T1 = keyof (A | B);  // "c"
type T2<T> = keyof (T | B);  // (keyof T & "b") | (keyof T & "c")
type T3<U> = keyof (A | U);  // ("a" | keyof U) | ("c" & keyof U)
type T4<T, U> = keyof (T | U);  // keyof T & keyof U
type T5 = T2<A>;  // "c"
type T6 = T3<B>;  // "c"
type T7 = T4<A, B>;  // "c"

This PR is the mirror image of #22300 and finally brings symmetry to the type eqivalences:

keyof (A & B) <==> keyof A | keyof B
keyof (A | B) <==> keyof A & keyof B

Why it took so long to figure out I don't know!

Fixes #23618.

@ahejlsberg
Merge branch 'master' into distributeKeyofUnion
faf44b2 
# Conflicts:
#	src/compiler/checker.ts
#	tests/baselines/reference/keyofAndIndexedAccessErrors.errors.txt
#	tests/baselines/reference/keyofAndIndexedAccessErrors.symbols
weswigham
weswigham reviewed Apr 24, 2018
View reviewed changes
tests/baselines/reference/computedPropertyNames28_ES5.types
var obj = {
>obj : { [x: string]: () => void; }
>{ [(super(), "prop")]() { } } : { [x: string]: () => void; }
>obj : { [(super(), "prop")](): void; }
Copy link
Member

@weswigham weswigham Apr 24, 2018

Choose a reason for hiding this comment

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

So, I don't know if it's related to this PR, but this change caught my eye:

{ [(super(), "prop")](): void; }

isn't a valid type. Somewhere we're reusing the name's expression tree when building the computed property name type and shouldn't be. :(

Copy link
Member Author

@ahejlsberg ahejlsberg Apr 24, 2018

Choose a reason for hiding this comment

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

This is actually a baseline change from #23592. I somehow messed up a merge and ended up with a lot of apparent changes in this PR.

Anyways, what's not valid about the type? It's a method with a computed property name that uses the comma operator.

Copy link
Member

@weswigham weswigham Apr 24, 2018

Choose a reason for hiding this comment

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

That's supposed to be type output - if someone wrote type Foo = { [(super(), "prop")](): void; }, we'd issue an error.

@ahejlsberg ahejlsberg mentioned this pull request Apr 24, 2018
Merged
@ahejlsberg
Copy link
Member Author

ahejlsberg commented Apr 24, 2018

I somehow ended up with a bad merge on this branch so I'm abandoning this PR in favor of #23645.

@ahejlsberg ahejlsberg closed this Apr 24, 2018
@ahejlsberg ahejlsberg deleted the distributeKeyofUnion branch April 24, 2018 04:15
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham weswigham weswigham approved these changes

@sandersn sandersn sandersn approved these changes

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

+1 more reviewer

@mhegazy mhegazy mhegazy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ahejlsberg @sandersn @weswigham @mhegazy