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

Omit in 3.5 breaking Discriminated Unions #31501

Closed
aczekajski opened this issue May 21, 2019 · 9 comments
Closed

Omit in 3.5 breaking Discriminated Unions #31501

aczekajski opened this issue May 21, 2019 · 9 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@aczekajski
Copy link

TypeScript Version: 3.5.0-rc, 3.5.0-dev.20190521

Search Terms: discriminated union omit, tagged union omit

Code

// discriminated union
type Union =
    | { type: 'A', a: 'a', c: string }
    | { type: 'B', b: 'b', c: string };

// various Omit definitions
type Omit<T, K extends keyof any> = Pick<T, Exclude<keyof T, K>>; // new omit copied from TS 3.5
type OmitBetter<T, K extends keyof any> = T extends any ? Pick<T, Exclude<keyof T, K>> : never;
type OmitBetterStrict<T, K extends keyof T> = T extends any ? Pick<T, Exclude<keyof T, K>> : never;

// Omit use-cases with Discriminated Union (DU)
type UnionMod = Omit<Union, 'whatever'>; // DU gets broken
type UnionMod2 = OmitBetter<Union, 'a'>; // DU remains unchanged
type UnionMod3 = OmitBetter<Union, 'a'>; // DU is still a DU but without field 'a'
type UnionMod4 = OmitBetterStrict<Union, 'c'>; // DU is still a DU but without common field 'c'
type UnionMod5 = OmitBetterStrict<Union, 'a'>; // error, because 'a' is not a common field of DU

// regular Omit use-cases (they all work properly)
type Regular = Omit<{ readonly a: 'a'; b?: 'b'; c: 'c'; }, 'whatever' | 'c'>;
type Regular2 = OmitBetter<{ readonly a: 'a'; b?: 'b'; c: 'c'; }, 'whatever' | 'c'>;
type Regular3 = OmitBetterStrict<{ readonly a: 'a'; b?: 'b'; c: 'c'; }, 'c'>;

Expected behavior:
Omit type should properly omit fields from objects but discriminated unions should still be discriminated unions. The expected behaviour is observed when using Omit from type-zoo package. The same approach was also proposed by the Microsoft member here: #28791 (comment).

Actual behavior:
Built-in Omit type which is being introduced in TypeScript 3.5 "merges" discriminated unions. When omitting field c from

{ type: 'A', a: 'a', c: string } |
{ type: 'B', b: 'b', c: string }

the resulting type should be equivalent to

{ type: 'A', a: 'a' } |
{ type: 'B', b: 'b' }

but with Omit@3.5, we get

{ type: 'A' | 'B' }

which is no longer a discriminated union.

According to Design Meeting Notes, 4/15/2019 (#30947), the resolution about making Omit stricter was based on "not worth breaking half the usages of Omit in the wild". When looking at "type Omit" search results, some of them also use the conditional type trick to distribute Omit over discriminated union. Following the same "not breaking the usages in the wild" principle, it might be a better idea to change the built-in Omit into the version with T extends any ? ... : never. Especially since it does not break the regular use-cases.

Playground Link: Link

@dragomirtitian
Copy link
Contributor

Pick isn't distributed either. Making all library types distributed can have severe performance implications. This was also discussed here: #28339

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jun 13, 2019
@RyanCavanaugh
Copy link
Member

All possible definitions of Omit have certain trade-offs; we've chosen one we think is the best general fit and can't really change it at this point without inducing a large set of hard-to-pinpoint breaks.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@tantaman
Copy link

tantaman commented Mar 26, 2022

A workaround for this is to use mapped types.

E.g.,

type RemoveNameField<Type> = {
  [Property in keyof Type as Exclude<Property, "name">]: Type[Property];
};

instead of Omit<Type, 'name'>

TS Playground example based on #39556

@micdah
Copy link

micdah commented Oct 17, 2022

Found another workaround by using the fact that T extends K is applied per-member of a union, so we can actually define a re-usable OmitUnion<T, K> type which can omit any number of members

interface Dog { type: 'dog', name: string, barks: boolean }
interface Cat { type: 'cat', name: string, meows: boolean }
type Pet = Dog | Cat

type PetWithoutName = Omit<Pet, 'name'>

// Cannot because PetWithoutName is reduced to {type: 'dog' | 'cat'}
let unnamedPet:PetWithoutName = { type: 'dog', barks: true }


type OmitUnion<T, K extends keyof any> = T extends any ? Omit<T, K> : never
// Uses the fact that `T extends K` is applied to _each union member_ of T in-turn

type PetWithoutName2 = OmitUnion<Pet, 'name'>
let unnamedPet2:PetWithoutName2 = { type: 'dog', barks: true }

So instead of ending up with the type { type: 'dog' | 'cat' } when applying Omit<Pet, 'name'> we instead want Omit<Dog, 'name'> | Omit<Cat, 'name'> which we can get by using T extends any which ensures we apply Omit<T, K>on each member of the union, rather than the entire union.

It's kind of a hack - but I like that it enables us to have a generic re-usable OmitUnion<T, K>

@unional
Copy link
Contributor

unional commented Oct 17, 2022

@micdah
Copy link

micdah commented Oct 17, 2022

@micdah yes, that is the way to get around it in type-zoo and type-plus

https://github.com/unional/type-plus/blob/main/ts/object/omit.ts

https://github.com/pelotom/type-zoo/blob/master/types/index.d.ts#L26

#28339 (comment)

Cool was not aware of those - but nice seeing multiple people coming up with the same workaround 👍

@IchordeDionysos
Copy link

IchordeDionysos commented May 5, 2024

This is a dangerous API design.

If someone designs an interface (as a non-discriminate union), dependents use it with Omit.
Then later, the interface changes and becomes a discriminate union.

There is a good chance that this will lead to software bugs, given the nature of the discriminate union is not propagated down to dependents (e.g. special handling of the discriminate union type for all cases).

People aren't aware of these specifics, and keeping this in mind, figuring out all of its consequences is a hellscape of complexity.

Problematic example code:

Before:

export interface Item {
  identifier: LearningPlanItemIdentifier;
  type: 'topic';
  item: TopicIdentifier;
}

After:

export type Item = ItemTopic | ItemUserGeneratedTopic;

export interface ItemTopic {
  identifier: LearningPlanItemIdentifier;
  type: 'topic';
  item: TopicIdentifier;
}

export interface ItemUserGeneratedTopic {
  identifier: LearningPlanItemIdentifier;
  type: 'userGeneratedTopic';
  item: UserGeneratedTopicIdentifier;
}

// Results in a type error
const full: Item = {
  identifier: new LearningPlanItemIdentifier('foo'),
  type: 'topic',
  item: new UserGeneratedTopicIdentifier('sdf'),
};

// Does NOT result in a type error and in software bugs.
const partial: Omit<Item, 'identifier'> = {
  type: 'topic',
  item: new UserGeneratedTopicIdentifier('sdf'),
};

@RyanCavanaugh
Copy link
Member

@microsoft microsoft locked as resolved and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

8 participants