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

TS 3.7rc regression on intersected union types #34762

Closed
andrewgreenh opened this issue Oct 28, 2019 · 9 comments · Fixed by #34789
Closed

TS 3.7rc regression on intersected union types #34762

andrewgreenh opened this issue Oct 28, 2019 · 9 comments · Fixed by #34789
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@andrewgreenh
Copy link

TypeScript Version: 3.6.3 VS 3.7-beta/nightly (from playground)

Search Terms: discriminated union, intersection type, tag in intersection, extract by tag

Code

We tried the new TS 3.7rc in our project and encountered a problem with our generated types. The types are generated by GraphQLCodegen.
Our code compiles without errors or warning in TS 3.6.3 but has compile errors in 3.7rc and in the nightly build of the playground.
We reduced the code to get the following situation in the playground:

http://www.typescriptlang.org/play/?ts=3.7-Beta#code/DYUwLgBAngXBCiAPMAnAhgYzAHgEoHtQAaCAbwgH0KwoAHEAOzQFsQB+OAclxAHMBLfAwKhOEAL4A+CAF4IDAK7BgENAGdVDKAFgAUKEgAvWdAB0KPoIZ6bugPR2IPNZH4bejEOjAgAJhAAzFHxmCABxdFoACwBFYD0aeidLIRNSPQhKajpGFnY4ACIeASECgG4MiH5fOBcUfgZeCt1MplYOCDqGpr1xZsSQZJLhQkG5dJasgbb8iCKUkdByyura1G7mzIthuGKrZr6EnIgAQV9mBpExsimcmY6Cs4vFkHKqms71xolmo6Sr2SVAA+EAAFOQqNM8g8npdRgUJBAAGQQAAK-AwAGtsLCXiQCtUCpIAJTAsEQ7L0e6FPZCK4I8TE5FojHY2l4ubbKxE366IA

The error seems to occur, when I try to use the Extract type on a field that is added in an intersection type.

Expected behavior:
No errors in TS 3.7

@andrewbranch
Copy link
Member

Minimal repro:

type U = { kind?: 'A', a: string } | { kind?: 'B' } & { b: string };
declare let x: Extract<U, { kind?: 'A' }>
x.a;

Notable criteria:

  1. If kind is not optional, no error
  2. If the non-matching union constituent is not an intersection, no error

@andrewbranch
Copy link
Member

While instantiating this usage of Extract, the checker basically comes to this:

type Source = { kind?: 'B' } & { b: string };
type Target = { kind?: 'A' }
// Source is assignable to Target if any constituents of Source are.
// 1. Is `{ kind?: 'B' }` assignable to `{ kind?: 'A' }`? Nope.
// 2. Is `{ b : string }` assignable to `{ kind?: 'B' }`?
// Actually yes, because `kind` is optional in Target, and we don’t care that
// these have no overlapping members because we’re comparing an intersection
// constituent—that check will be performed at the top level.

Not sure yet how this was avoided previously / what introduced this regression, because the fix wasn’t obvious while discovering the problem. Going back to 3.6 to see what happened there.

@andrewbranch
Copy link
Member

Caused by #33213

@sandersn sandersn added the Bug A bug in TypeScript label Oct 28, 2019
@sandersn sandersn added this to the TypeScript 3.8.0 milestone Oct 28, 2019
@AnyhowStep
Copy link
Contributor

AnyhowStep commented Oct 28, 2019

Seems like a pretty serious bug. Should it really be part of 3.8, and not 3.7?

I use intersection types and optional properties together very often. This bug is bad enough that I can't upgrade to TS 3.7

@andrewgreenh
Copy link
Author

I'd guess that every user of GraphQL codegen should be affected when they are using Extract in the __typename as this is generated in ts as an optional property.

@sandersn
Copy link
Member

@andreasgruenh as a workaround, could graphql stop generating an optional discriminant?

@andrewbranch
Copy link
Member

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
@andrewgreenh
Copy link
Author

Yes, that's true, you can add nonOptionalTypename: true to the codegen.yml
For new projects this might be a solution. When using this flag in our codebase, 110 new errors appear, because various places assume the typename to be optional :/

@andrewgreenh
Copy link
Author

I think I found a workaround by writing a special mapped type ExtractByOptionalTag which can be used for our typename usecase.

Code:

type U = ({ kind?: "A" } & { a: string }) | ({ kind?: "B" } & { b: string });
declare let x: ExtractByOptionalTag<"kind", U, "A">;
x.a

type SingleRequired<T extends Object, TKey extends keyof T> = T &
    Required<Pick<T, TKey>>;

type SingleOptional<T extends Object, TKey extends keyof T> = Omit<T, TKey> &
    Partial<Pick<T, TKey>>;

type ExtractByOptionalTag<
    TTagKey extends string,
    TObject extends { [key in TTagKey]?: string },
    TTagValue extends TObject[TTagKey]
    > = SingleOptional<
        Extract<SingleRequired<TObject, TTagKey>, { [key in TTagKey]: TTagValue }>,
        TTagKey
    >;

Playground link: http://www.typescriptlang.org/play/?ts=3.7-Beta#code/C4TwDgpgBAqlC8UAUBvKBrAlgOwCYH4AuKAIgEESoBfKAMijQENiBnYAJxwHNqBKKAD7I0WPEVIAhSjXpoARqw7c+AbgCwAKFwQAxgBtG7aHojAoAD2IBRcx0Y7gEkAHkwwTAHtsjPQBVGXAA8JKK4JAA0sJHkJAB86hrmAHSMmpqgkFAAytwmAEoQAI4ArphGuIG+UBC2EHgsUM5yAFa6wJG+ANIQINW19Rg9HgBmUL6xCGN0mlCzUAUlZRAVAAqYOuiVHd0gsfFpGhnQOdhcJq7uXj6VfcB1uA1NrQ7bPbf3DehDo+OTzgC2mGAWzGOwmtBmcxWhnc1zWGxBXR6ewS6XA0BsdgcTgunm8fgCgUhs18-i4O3eAzYnFO4WJYyebUpDwYUAA2l9ejgxmSdgBdcTU5RUOkaOY8gIANR8xWgNTuA18jIcbNJAX59ImiBOZwguKueiJYvFc0x7HswJ1+SKpXKlWV7Ql5ORkTQHLe3LVzpAfOIXulell1FiopNcy9O01CSAA

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 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
None yet
Development

Successfully merging a pull request may close this issue.

4 participants