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

Mapped Types should distribute over union types #28339

Closed
Jessidhia opened this issue Nov 5, 2018 · 33 comments
Closed

Mapped Types should distribute over union types #28339

Jessidhia opened this issue Nov 5, 2018 · 33 comments

Comments

@Jessidhia
Copy link

@Jessidhia Jessidhia commented Nov 5, 2018

Search Terms

Mapped Type Union

Suggestion

This might be a bug or an intentional consequence of the original design, but mapped types do not distribute over union types. The example below demonstrates what happens:

type Foo = { x: false; foo?: string }
type Bar = { x: true; foo: string }

type FooOrBar = Foo | Bar

type MappedFooOrBar = Pick<FooOrBar, keyof FooOrBar>

// the type of MappedFooOrBar will be { x: boolean; foo: string | undefined }
// note how foo is now required even with x = false
// and also how you're now allowed to set foo = undefined even with x = true

Use Cases

Higher order components that receive components whose props are union types.

This usually is not a problem but it is a problem if you are going to use Pick (or Omit) on their props. This is already a problem with react-redux's connect for example.

Examples

type Foo = { x: false; foo?: string }
type Bar = { x: true; foo: string }

type FooOrBar = Foo | Bar

// input is a union type so this should distribute
type MappedFooOrBar = Pick<FooOrBar, keyof FooOrBar>
// should be equivalent to a MappedFooOrBar that distributes over its input
type MappedFooBar = Pick<Foo, keyof FooOrBar> | Pick<Bar, keyof FooOrBar>

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript / JavaScript code
    • It probably is breaking but is a "good break" as far as I can tell.
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. new expression-level syntax)
@Jessidhia Jessidhia changed the title Mapped Types don't distribute over union types Mapped Types should distribute over union types Nov 5, 2018
@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 13, 2019

It looks like only Pick does not distribute over union types. Partial is working fine.

@jack-williams

This comment has been minimized.

Copy link
Collaborator

@jack-williams jack-williams commented Feb 14, 2019

The difference seems natural if you look at the definitions of Partial and Pick, to me at least.

type Partial<T> = {
    [P in keyof T]?: T[P];
};

type Pick<T, K extends keyof T> = {
    [P in K]: T[P];
};

Specifically the constraint part which is essentially the input to the map.

// Partial: [P in keyof T], could be read as a function T => [P in keyof T]
// Pick:    [P in K],       could be read as a function K => [P in K]

Partial is a parametric function on objects T, so applying it uniformly to object types in a union makes sense. Pick is a function on keys; the notion there was ever union of object types has been lost.

@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 14, 2019

@jack-williams

The difference seems natural if you look at the definitions of Partial and Pick, to me at least.

That's kind of my issue here: Should I actually have to look at the implementation and then infer different behavior? Or wouldn't it be nicer if they behave the same since they're both classified as mapped types?

Do you have an example where the current behavior of Pick for unions is useful?

Is this maybe more of an issue with keyof for unions? In a perfect world the keyof { kind: 1; error: string } | { kind: 2; payload: any } would be ('kind' | 'error') | ('kind' | 'payload'). Typescript would not flatten this union and keep the information that 'kind' is the discriminant. Maybe this touches to much internals which is why I would prefer the standard lib Pick to use conditionals to distribute over union types.

At least the title should be changed from "Mapped types" to "Mapped types that are a function of keys". Otherwise it is misleading that all mapped types do not distribute over unions.

@dragomirtitian

This comment has been minimized.

Copy link
Contributor

@dragomirtitian dragomirtitian commented Feb 14, 2019

@eps1lon

The difference seems natural if you look at the definitions of Partial and Pick, to me at least.

That's kind of my issue here: Should I actually have to look at the implementation and then infer different behavior? Or wouldn't it be nicer if they behave the same since they're both classified as mapped types?

Distribution does not happen in all cases for conditional types either. If the type parameter is not used nakedly it will not distribute so you have to know a bit about the type to know if it distributes over unions.

I think we can easily create a distributive version of Pick with what the language currently offers. I don't think it is of as general a use as the original version, but it would look something like:

type UnionKeys<T> = T extends any ? keyof T : never
type DistributivePick<T, K extends UnionKeys<T>> = T extends any ? Pick<T, Extract<keyof T, K>> : never;

type Union = {
    kind: "A",
    payload: number
} | {
    kind: "B",
    payload: string
} | {
    kind: "C",
}

type JustKind = DistributivePick<Union, 'kind'> // Pick<{ kind: "A"; payload: number; }, "kind"> | Pick<{ kind: "B"; payload: string; }, "kind"> | Pick<{ kind: "C"; payload: string; }, "kind">
type JustPayload = DistributivePick<Union, 'payload'> //  Pick<{ kind: "A"; payload: number; }, "payload"> | Pick<{ kind: "B"; payload: string; }, "payload"> | Pick<{ kind: "C"; }, never>
@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 14, 2019

@dragomirtitian Thanks for code. The union keys trick is new to me.

I don't think it is of as general a use as the original version

Do you have a use case were you require Pick to behave in its current way for union types?

@dragomirtitian

This comment has been minimized.

Copy link
Contributor

@dragomirtitian dragomirtitian commented Feb 14, 2019

@eps1lon I am not 100% sure that i have used Pick with unions. I usually use Pick on non-union types.

The problem is that I do suspect changing it now would break some code somewhere. Also the distributive version is considerably more complex, it will slow down compilation and I think in most cases Pick is not used over unions so the people who want it can use custom type such as the one above.

Distributive omit below, for completeness :)

type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>
type DistributiveOmit<T, K extends UnionKeys<T>> = T extends any ? Omit<T, Extract<keyof T, K>> : never;
@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 14, 2019

Well many libraries use Pick (see original Post: react higher-order components in particular). In those cases Pick should be agnostic to the type of the type.

I usually use Pick on non-union types.

Can you show me some examples where you use Pick? I'm biased towards react and we use Pick mostly in HOCs which is why the current behavior is so frustrating.

it will slow down compilation

Is there some accepted method to benchmark tsc compilation? I'm not familiar with all the intricacies of the language server. Can I just repeatedly run the cli or do I need to reset some things in between?

@jack-williams

This comment has been minimized.

Copy link
Collaborator

@jack-williams jack-williams commented Feb 14, 2019

@eps1lon

That's kind of my issue here: Should I actually have to look at the implementation and then infer different behavior? Or wouldn't it be nicer if they behave the same since they're both classified as mapped types?

Yes, I probably agree that the difference between homomorphic (Partial) and non-homomorphic (Pick) mapped types should be better documented. This does not mean the correct solution is to remove the differences.

Fundamentally I think they are mapping over different things and it doesn't really make sense to force them to behave the same. If you look at the implementation of Pick I'm not even sure how you would implement it as a mapped type that distributes over unions; the object type does not appear in the constraint type ([K in …]).

Do you have an example where the current behavior of Pick for unions is useful?

They are useful to me in so far as they work as I expect them to. This isn't a very satisfactory answer, but I've never felt the need for them to distribute.

In a perfect world the keyof { kind: 1; error: string } | { kind: 2; payload: any } would be ('kind' | 'error') | ('kind' | 'payload').

This will fundamentally break union types because they should be associative. If it doesn't break the correctness of the typechecker, I suspect it will at least cause a non-trivial perf hit.

The crux of the issue, IMO, is that distributing non-homomorphic mapped types over unions is incredibly shaky. They are functions that depend on keys, and the keys change if you view as a union or if you distribute.

As an example, consider the distributive implementation of Pick.

type JustPayload = DistributivePick<Union, 'payload'> //  Pick<{ kind: "A"; payload: number; }, "payload"> | Pick<{ kind: "B"; payload: string; }, "payload"> | Pick<{ kind: "C"; }, never>

For JustPayload, distributing payload to the branch with kind "C" produces an empty object type {}, and consequently the overall type of JustPayload is essentially {}. I'm not sure this is what someone would want or expect.

@dragomirtitian

This comment has been minimized.

Copy link
Contributor

@dragomirtitian dragomirtitian commented Feb 14, 2019

@jack-williams

JustPayload can be useful with the apropriate type guards (an in type guard should narrow the {} away). But I do agree variations in behavior may be required on what you are trying to achieve.

@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 14, 2019

For JustPayload, distributing payload to the branch with kind "C" produces an empty object type {}, and consequently the overall type of JustPayload is essentially {}.

What do you mean by that? I can still branch depending on the shape if I have a union like Foo | {}: https://www.typescriptlang.org/play/index.html#src=type%20Foo%20%3D%20%7B%20payload%3A%20string%20%7D%20%7C%20%7B%7D%0D%0A%0D%0Afunction%20getPayload(foo%3A%20Foo)%3A%20string%20%7C%20undefined%20%20%7B%0D%0A%20%20%20%20if%20(%22payload%22%20in%20foo)%20%7B%0D%0A%20%20%20%20%20%20%20%20return%20foo.payload%3B%0D%0A%20%20%20%20%7D%0D%0A%20%20%20%20return%20undefined%3B%0D%0A%7D

@jack-williams

This comment has been minimized.

Copy link
Collaborator

@jack-williams jack-williams commented Feb 14, 2019

The in operator is terribly unsound. I can do things like:

const x: JustPayload = 3;

If you are using the in operator, why not do that before applying Pick to get rid of the unions?

@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 14, 2019

The in operator is terribly unsound. I can do things like:

const x: JustPayload = 3;

If you are using the in operator, why not do that before applying Pick to get rid of the unions?

I just wanted to investigate what you mean by

and consequently the overall type of JustPayload is essentially {}

I still have a union type information.

const x: JustPayload = 3;

This is fine by me. After all 3 is assignable to {}.

@jack-williams

This comment has been minimized.

Copy link
Collaborator

@jack-williams jack-williams commented Feb 14, 2019

3 is assignable to {}, but would someone expect 3 to be assignable to the result of picking fields from an object using Pick? That doesn't seem right to me.

The problem is that semantically JustPayload isn't a union type, it's {}.

type Check = {} extends JustPayload ? (JustPayload extends {} ? "They are the same" : "no") : "no";

You are relying on the type-checker to not perform subtype reduction on the union.

@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 14, 2019

The problem is that semantically JustPayload isn't a union type, it's {}

@jack-williams What do you mean by that? The playground tells me it's still { payload: string } | {}. I kind still use type guards to narrow it down. Your statement sounds like any type information is lost and it's "just {}".

All of this sounds more of an issue with {}. I would still have the same issue with non union types if I omit every property.

I think think we argue with a different premise: You assume that you know the exact shape of a given T from where you pick. This assumption is valid for app code. In library code we can't make that assumption. It might be that we omit every property, might be we pick none.

@VincentLanglet

This comment has been minimized.

Copy link

@VincentLanglet VincentLanglet commented Feb 22, 2019

What is the difference between Something<T> and T extends any ? Something<T> : never ?

For example, I don't understand why

type DistributiveOmit<T, K extends UnionKeys<T>> = T extends any ? Omit<T, Extract<keyof T, K>> : never;

Is not the same than

type DistributiveOmit<T, K extends UnionKeys<T>> = Omit<T, Extract<keyof T, K>>;

And why

type UnionKeys<T> = T extends any ? keyof T : never

Is not the same than

type UnionKeys<T> = keyof T

@dragomirtitian Btw, i think you wanted to write type DistributiveOmit<T, K extends UnionKeys<T>> and not type DistributiveOmit<T, K extends keyof UnionKeys<T>>

@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 22, 2019

@VincentLanglet See #28483 (comment) for additional resources. Combined with the comments in this thread it hopefully gives you a decent explanation what is happening.

@VincentLanglet

This comment has been minimized.

Copy link

@VincentLanglet VincentLanglet commented Feb 22, 2019

@dragomirtitian

This comment has been minimized.

Copy link
Contributor

@dragomirtitian dragomirtitian commented Feb 22, 2019

@VincentLanglet 10x for the catch, fixed.

@RyanCavanaugh RyanCavanaugh added this to Next Meeting in Suggestion Backlog Triage Feb 25, 2019
@RyanCavanaugh

This comment has been minimized.

Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Feb 25, 2019

Recommend using

type Pick2<T, K extends keyof T> = T extends unknown ? Pick<T, K> : never;
type MappedFooOrBar = Pick2<FooOrBar, keyof FooOrBar>

declare function fn(x: MappedFooOrBar): void;
// Error
fn({ x: true, foo: undefined });
// OK
fn({ x: false });

We're still noodling on whether this is a safe change to make

@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 26, 2019

We're still noodling on whether this is a safe change to make

@RyanCavanaugh Would it help if I checked the DefinitelyTyped repo with the new version and report back with any breakage? Or is the main concern performance of the type checker?

@dragomirtitian

This comment has been minimized.

Copy link
Contributor

@dragomirtitian dragomirtitian commented Feb 26, 2019

@RyanCavanaugh @eps1lon
I can already see the questions: "Since I can pick on union members why can't I specify keys that in only in some members ?"

type Foo = { x: false; foo?: string, bar: string }
type Bar = { x: true; foo: string }

type FooOrBar = Foo | Bar

type Pick2<T, K extends keyof T> = T extends unknown ? Pick<T, K> : never;
type MappedFooOrBar = Pick2<FooOrBar, "x" | "bar"> // error here

I fear that since Pick2 only accepts common keys of the union in K this will create a lot of confusion (Not that I mind answering the SO questions 😛)

@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 26, 2019

@RyanCavanaugh @eps1lon
I can already see the questions: "Since I can pick on union members why can't I specify keys that in only in some members ?"

@dragomirtitian
IMO I wouldn't constrain the keys to begin with. If the key doesn't exist it won't be picked. That being sad: You already proposed the solution to this previously:

type UnionKeys<T> = T extends unknown ? keyof T : never;
type Pick2<T, K extends UnionKeys<T>> = T extends unknown ? Pick<T, K> : never;

type Foo = { x: false; foo?: string, bar: string }
type Bar = { x: true; foo: string }

type FooOrBar = Foo | Bar


type MappedFooOrBar = Pick2<FooOrBar, "x" | "bar"> // error here

I'm still a little bit confused why you argue against it? Is there something this would break? Are you concerned about perf? Just playing devils advocate?

@dragomirtitian

This comment has been minimized.

Copy link
Contributor

@dragomirtitian dragomirtitian commented Feb 26, 2019

@eps1lon A bit of devils advocate (we should consider all sides before making a decision), mostly perf concern.

I would restrict K. Part of the selling point of Typescript is catching missing/misspelled properties. If K is unrestricted you might pass in a misspelled key or a key may get removed or renamed and no errors will occur on the picks.

UnionKeys will probably be expensive if applied to all picks. Just as an alternative workaround we could use something like StrictUnion (link) this will let us use @RyanCavanaugh Pick version but let us pick any union key, although it does change the behavior of the union a bit.

type UnionKeys<T> = T extends any ? keyof T : never;
type StrictUnionHelper<T, TAll> = T extends any ? T & Partial<Record<Exclude<UnionKeys<TAll>, keyof T>, never>> : never;
type StrictUnion<T> = StrictUnionHelper<T, T>

type Foo = { x: false; foo?: string, bar: string }
type Bar = { x: true; foo: string }

type FooOrBar = Foo | Bar

type Pick2<T, K extends keyof T> = T extends unknown ? Pick<T, K> : never;
type MappedFooOrBar = Pick2<StrictUnion<FooOrBar>, "x" | "bar"> // ok now, but will be type equivalent to 
type MappedFooOrBarExpanded = {
    x: false;
    bar: string;
} | {
    x: true;
    bar?: undefined;
}

But the confusing behavior would still be there initially and it would be the kind of thing you "have to know" about,

@dragomirtitian

This comment has been minimized.

Copy link
Contributor

@dragomirtitian dragomirtitian commented Feb 26, 2019

@eps1lon @RyanCavanaugh

Just one more detail. Under Pick2, the usual definition of Omit, the one that is generally used will yield some unexpected results:

type Foo = { x: false; foo?: string, bar: string }
type Bar = { x: true; foo: string }

type FooOrBar = Foo | Bar

type Pick2<T, K extends keyof T> = T extends unknown ? Pick<T, K> : never;
type Omit2<T, K extends keyof T> = Pick2<T, Exclude<keyof T, K>>
type MappedFooOrBar = Omit2<FooOrBar, "foo"> // omitting foo, but actually omitting bar too 
type MappedFooOrBarExpanded =  { // Where's my bar ? 
    x: false;
} | {
    x: true;
}
@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 26, 2019

@dragomirtitian
Omit2 would need the same treatment as Pick:

type Omit2<T, K extends keyof T> = T extends unknown ? Pick<T, Exclude<keyof T, K>> : never
//                                                     ^^^^ old Pick is enough

Your example doesn't really show anything. What you probably meant to do is assign some object to MappedFooOrBar.

@jack-williams

This comment has been minimized.

Copy link
Collaborator

@jack-williams jack-williams commented Feb 27, 2019

I've slightly lost track of where this proposal is going so I'm recapping, mostly for my own benefit. There seem to be three options:

  1. The initial proposal of have having the Pick mapped type innately distribute. I'm not sure this is even possible given how the type is defined.

The other two options seem to be oriented around redefining the library definition of Pick.

  1. Redefine a 'safe' distributive Pick that still enforces the keyof constraint on the union.
  2. Redefine distributive Pick that lets you pick from a key that only exists in some (or none) of the types.

Option (3) will systematically produce unions with {} in; this seems like pathological behaviour to me. Getting any use out of a union with {} relies on avoiding subtype reduction, and depends upon the unsound in operator. The reasoning is happening at the level of syntax, rather than the model of types.

Options (2) and (3) introduce generic conditional types into the definition. This will further complicate type relations between generic Pick types. Before it only relied upon generic mapped types, now it needs to reason at the level of generic conditional types too. The latter are much harder to get right and still have edge cases. I wouldn't be surprised if #27697 meant that a lot of Pick2 types people think should be assignable to each other are not.

There is also the issue of making users have to juggle the syntax of T extends unknown ? … to understand what should be a simple Pick type. This feels like quite a high cost to me.

@eps1lon

This comment has been minimized.

Copy link

@eps1lon eps1lon commented Feb 27, 2019

Option (3) will systematically produce unions with {} in; this seems like pathological behaviour to me. Getting any use out of a union with {} relies on avoiding subtype reduction, and depends upon the unsound in operator. The reasoning is happening at the level of syntax, rather than the model of types.

Isn't that already the case for non-union types and Pick currently?

type PickedFoo = Omit<{foo: string}, 'foo'>
    ^^^ $ExpectType {} 

I'm not sure I understand how a {} type would be problematic at runtime. Could you add some code that would cause trouble at runtime with a distributive Pick that the current Pick would not also cause with non-union types?

@jack-williams

This comment has been minimized.

Copy link
Collaborator

@jack-williams jack-williams commented Feb 27, 2019

For Omit yes; for Pick no.

type PickedFoo = Pick<{foo: string}, 'foo'> // { foo: string }

I'm not sure I understand how a {} type would be problematic at runtime. Could you add some code that would cause trouble at runtime with a distributive Pick that the current Pick would not also cause with non-union types?

My concern is not that much about runtime; it's more the meaning of the types produced. A union of object types with {}, such as {} | { foo: string } | { bar: number }, really means the same thing as just {}. Sometimes TypeScript will keep the syntax of the type around, and you can narrow using in, but sometimes TypeScript will reduce the type to its true meaning. For example:

function foo(both: { a: string } | {}) {
	if ("a" in both) {
		both // narrowed: { a: string};
	}
	// both2: {} because of subtype reduction
	const both2 = Math.random() > 0.5 ? both : both;
	if ("a" in both2) {
		both2 // never;
	}

}

In the first if the union type is not reduced, so narrowing by in works. However, when TypeScript constructs a union type for both2 it will apply subtype reduction that reduces { a: string } | {} to {}; now narrowing by in does not work.

The difference here, to me, is largely a syntactic artefact.

A Pick type that by default does not put constraints on the keys is going to be more likely to produce union types with {}. These union types might work as a user expects in some cases, but if union reduction is applied then it will break their code. TypeScript now has to be more careful about applying union reductions that should be semantically fine, but in practice could break a lot of user code.

My comments here are largely technical. I think there is a good argument to be data-driven about this and see what happens in the wild. Anecdotally, I've never had a problem with Pick so I don't find the motivation that strong---but I'm a single data point.

@zpdDG4gta8XKpMCd

This comment has been minimized.

Copy link

@zpdDG4gta8XKpMCd zpdDG4gta8XKpMCd commented Mar 1, 2019

how can distribution be pre-picked once to work everywhere for one way over another?

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Apr 1, 2019

Rough notes from discussion in #30696:

  • We definitely can't change the behavior now
  • Desirability of distributivity/mapping is entirely scenario-dependent; current defaults exist to a) do the right thing "most" of the time and b) allow opting in to the other behavior
    • The defaults here only seem "bad" if you ignore all the cases where they work like you expect them to
@typescript-bot

This comment has been minimized.

Copy link
Collaborator

@typescript-bot typescript-bot commented Apr 4, 2019

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

@davidgomes

This comment has been minimized.

Copy link

@davidgomes davidgomes commented Dec 5, 2019

Rough notes from discussion in #30696:

  • We definitely can't change the behavior now

  • Desirability of distributivity/mapping is entirely scenario-dependent; current defaults exist to a) do the right thing "most" of the time and b) allow opting in to the other behavior

    • The defaults here only seem "bad" if you ignore all the cases where they work like you expect them to

@RyanCavanaugh I mostly see your point here that the existing Pick implementation works for most cases and that users can opt in to the more complex version of Pick if they wish to.

However, I'd love to understand better why you "definitely can't change the behavior". Are there any examples you can provide that would justify that? Is it possible that changing the Pick implementation would break current uses of Pick? If so, how?

I'm just trying to learn more about this issue, and would appreciate any help.

@AnyhowStep

This comment has been minimized.

Copy link
Contributor

@AnyhowStep AnyhowStep commented Jan 3, 2020

Conditional types incur additional costs.
Using it when you don't need it is a waste.

Each use adds up.


Also, it's possible for people to want the current behavior when passing in a union type.

I'm on mobile but Pick<{ a:1,b:2 }|{ b:3,c:4 }, "b"> should be { b:2|3 }. I don't see it as too farfetched for there to exist people relying on this behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
You can’t perform that action at this time.