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

3.6.4 Partial field matching does not work for conditional type #33944

Closed
crisslion opened this issue Oct 11, 2019 · 7 comments · Fixed by #34789
Closed

3.6.4 Partial field matching does not work for conditional type #33944

crisslion opened this issue Oct 11, 2019 · 7 comments · Fixed by #34789
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@crisslion
Copy link

TypeScript Version:

3.6.4

Search Terms:
Partial field conditional matching
3.6.4 conditional partial
Code

export type QueryType = (
  { __typename?: 'TypeOne' } & { a: boolean }
) | (
  { __typename?: 'TypeTwo' } & { a: boolean }
) | (
  { __typename?: 'TypeThree' } & { a: boolean }
);

type PickFromUnion<Union, Typename> =
  Union extends { __typename?: Typename } ? Union : never;

type FirstType = PickFromUnion<QueryType, 'TypeOne'>;

Expected behavior:
FirstType would be of type { __typename?: 'TypeOne' } & { a: boolean }
Actual behavior:
FirstType is of type QueryType

Playground Link:
3.6.3 where it works
http://www.typescriptlang.org/play/?ts=3.6.3#code/KYDwDg9gTgLgBDAnmYcCKBXYVEBVmoC8cAFAFBxwDecA+rUigHYCGAtsAPwBccA5PhQB5JsD5wAvnABk1OC14AjCBAA2wFk0lkAlHAA+pCnPqNgrDj36DguAO4RxU2TQVxlajVom6DRyjSmBBZcvAIEuAAWUMBikjJybh7qmto6ANxkZGZwAAoAlgDGANYAYlAQbACqTPkQTAA8NXVMADRwNiEAfHCExs31cKAw5gAmAM4mDMHsoR0zHPGccANavKIAbtiZ2QRwpflQ4zA2vXlFZRXVtfUNmNh4BO3hwqJ8XelAA

3.7 where it does not work (3.6.4 not provided in playground)
http://www.typescriptlang.org/play/?ts=3.7-Beta#code/KYDwDg9gTgLgBDAnmYcCKBXYVEBVmoC8cAFAFBxwDecA+rUigHYCGAtsAPwBccA5PhQB5JsD5wAvnABk1OC14AjCBAA2wFk0lkAlHAA+pCnPqNgrDj36DguAO4RxU2TQVxlajVom6DRyjSmBBZcvAIEuAAWUMBikjJybh7qmto6ANxkZGZwAAoAlgDGANYAYlAQbACqTPkQTAA8NXVMADRwNiEAfHCExs31cKAw5gAmAM4mDMHsoR0zHPGccANavKIAbtiZ2QRwpflQ4zA2vXlFZRXVtfUNmNh4BO3hwqJ8XelAA

@crisslion crisslion changed the title 3.6.4 Partial field does not work for conditional type 3.6.4 Partial field matching does not work for conditional type Oct 11, 2019
@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Oct 17, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.7.1 milestone Oct 17, 2019
@ahejlsberg
Copy link
Member

This was broken by #33213. Simple repro:

declare let x1: { __typename?: 'TypeTwo' } & { a: boolean };
let y1: { __typename?: 'TypeOne' } & { a: boolean} = x1;  // No error!

Before #33213 this was an error, after it isn't.

@ahejlsberg ahejlsberg assigned sandersn and unassigned ahejlsberg Oct 21, 2019
@ahejlsberg ahejlsberg added Bug A bug in TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Oct 21, 2019
@sandersn sandersn added this to Not started in Rolling Work Tracking Oct 21, 2019
@sandersn
Copy link
Member

This change happened in 3.6.4, so I'm going to move this into 3.8.

@sandersn
Copy link
Member

@crisslion In the meantime, you can work around this by making the discriminant required.

Actually, why was the discriminant optional in the first place?

@sandersn
Copy link
Member

For the minimal repro:

  1. Is { t?: "Two" } & { a } assignable to each type in { t?: "One" } & { a }?
  2. Some type in { t?: "Two" } & { a } must be assignable to { t?: "One" } and some type in { t?: "Two" } & { a } must be assignable to { a }.
  3. { a } is assignable to { t?: "One" } (because we are checking an intersection, weak type checks are turned off) and { a } is assignable to { a }.

So { t?: "Two" } & { a } is assignable to each type in { t?: "One" } & { a }.

@crisslion
Copy link
Author

@sandersn We are generating typescript types from graphQL types so we are using __typename to pick specific type from union. __typename is optional because then don't have to make it optional/omit it when we are using the type.

I guess more precise data type we want to pick from is:

export type QueryType = (
  { __typename?: 'TypeOne' } & { baz: boolean }
) | (
  { __typename?: 'TypeTwo' } & { foo: boolean }
) | (
  { __typename?: 'TypeThree' } & { bar: boolean }
);

So only convenient way to select the right type is using type name.
At the moment as a workaround we reverted to previous version we had 3.6.2

@sandersn
Copy link
Member

__typename is optional because then don't have to make it optional/omit it when we are using the type.

I don't understand this part. Can you show me an example of how you omit it when using the type? How would it look if the property weren't optional?

@sandersn sandersn moved this from Not started to In Progress in Rolling Work Tracking Oct 22, 2019
@sandersn sandersn moved this from In Progress to In Review in Rolling Work Tracking Oct 22, 2019
sandersn added a commit that referenced this issue Oct 28, 2019
isIntersectionConstituent controls whether relation checking performs
excess property and common property checks. It is possible to fail a
relation check with excess property checks turned on, cache the result,
and then skip a relation check with excess property checks that would
have succeeded. #33133 provides an example of such a program.

Fixes #33133 the right way, so I reverted the fix at #33213
Fixes #34762 (by reverting #33213)
Fixes #33944 -- I added the test from #34646
@sandersn sandersn added the Fix Available A PR has been opened for this issue label Oct 28, 2019
@sandersn
Copy link
Member

Fix is available at #34789

Rolling Work Tracking automation moved this from In Review to Done Oct 29, 2019
sandersn added a commit that referenced this issue Oct 29, 2019
* Add isIntersectionConstituent to relation key

isIntersectionConstituent controls whether relation checking performs
excess property and common property checks. It is possible to fail a
relation check with excess property checks turned on, cache the result,
and then skip a relation check with excess property checks that would
have succeeded. #33133 provides an example of such a program.

Fixes #33133 the right way, so I reverted the fix at #33213
Fixes #34762 (by reverting #33213)
Fixes #33944 -- I added the test from #34646

* Update comments in test
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this issue Oct 29, 2019
Component commits:
2e0b451 Add isIntersectionConstituent to relation key
isIntersectionConstituent controls whether relation checking performs
excess property and common property checks. It is possible to fail a
relation check with excess property checks turned on, cache the result,
and then skip a relation check with excess property checks that would
have succeeded. microsoft#33133 provides an example of such a program.

Fixes microsoft#33133 the right way, so I reverted the fix at microsoft#33213
Fixes microsoft#34762 (by reverting microsoft#33213)
Fixes microsoft#33944 -- I added the test from microsoft#34646

14d7a44 Merge branch 'master' into add-isIntersectionConstituent-to-relation-key

ea80362 Update comments in test

0764275 Merge branch 'master' into add-isIntersectionConstituent-to-relation-key
@sandersn sandersn moved this from Done to In Review in Rolling Work Tracking Oct 29, 2019
@sandersn sandersn moved this from In Review to Done in Rolling Work Tracking Oct 29, 2019
sandersn pushed a commit that referenced this issue Oct 29, 2019
Component commits:
2e0b451 Add isIntersectionConstituent to relation key
isIntersectionConstituent controls whether relation checking performs
excess property and common property checks. It is possible to fail a
relation check with excess property checks turned on, cache the result,
and then skip a relation check with excess property checks that would
have succeeded. #33133 provides an example of such a program.

Fixes #33133 the right way, so I reverted the fix at #33213
Fixes #34762 (by reverting #33213)
Fixes #33944 -- I added the test from #34646

14d7a44 Merge branch 'master' into add-isIntersectionConstituent-to-relation-key

ea80362 Update comments in test

0764275 Merge branch 'master' into add-isIntersectionConstituent-to-relation-key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
4 participants