UnionToTuple: Fix behavior when a union member is a supertype of another; Add ExcludeExactly type#1349
Conversation
BTW, this is not just about property modifiers on objects. This would happen anytime type Union1 = {a: string; b: string} | {a: string};
type Tuple1 = UnionToTuple<Union1>;
//=> [{a: string}]
type Union2 = {a: 1} | {b: 1} | {[x: string]: number};
type Tuple2 = UnionToTuple<Union2>;
//=> [{[x: string]: number}]Note: In both the above examples, for the error to be noticeable, it's important that import type {UnionToTuple} from 'type-fest';
type Union1 = {a: string} | {a: string; b: string};
type Tuple1 = UnionToTuple<Union1>;
//=> [{a: string}, {a: string; b: string}]
type Union2 = {[x: string]: number} | {a: 1} | {b: 1};
type Tuple2 = UnionToTuple<Union2>;
//=> [{[x: string]: number}, {a: 1}, {b: 1}] |
In order to make the test reliable, we need a union that contains two members type Union = {a: string} | {readonly a: string}In the above union, both the object types are supertypes of each other because the type T1 = {a: string} extends {readonly a: string} ? true : false;
//=> true
type T2 = {readonly a: string} extends {a: string} ? true : false;
//=> trueHence, the above union is a test case that is always guaranteed to fail: type Union1 = {readonly a: string} | {a: string};
type Tuple1 = UnionToTuple<Union1>;
//=> [{a: string}]
type Union2 = {a: string} | {readonly a: string};
type Tuple2 = UnionToTuple<Union2>;
//=> [{readonly a: string}] |
|
Thank you for fixing and refining the documentation!
Thanks for your information! // Super type cases.
type UnionSuperType0 = {a: string; b: string} | {a: string};
expectType<UnionSuperType0>({} as UnionToTuple<UnionSuperType0>[number]);
type UnionSuperType1 = {a: 1} | {b: 1} | {[x: string]: number};
expectType<UnionSuperType1>({} as UnionToTuple<UnionSuperType1>[number]);
type T1 = {a: string} extends {readonly a: string} ? true : false;
//=> true
type T2 = {readonly a: string} extends {a: string} ? true : false;
//=> trueI believe this test case aims to prove that the order of the union does not affect the result of If my understanding is correct, we still cannot predict the order of union member picking as we might hope. I’ve dug into Taking this into account, I believe your proposed test case can be fully covered by the following cases. // `LastOfUnion` distinguishes between different modifiers.
type UnionType = {a: 0} | {b: 0} | {a?: 0} | {readonly a?: 0} | {readonly a: 0};
expectType<true>({} as LastOfUnion<UnionType> extends UnionType ? true : false);
expectType<false>({} as UnionType extends LastOfUnion<UnionType> ? true : false);What are your thoughts on this?
|
These test cases are not reliable because they may pass even with the existing incorrect implementation. For example, in the first example, if the first type picked up from the union is The error only happens if the first type picked up is
You didn’t get me. The order is not predictable. That’s clear, that’s the whole point. What I meant is that we need a test case that "always" fails with the existing implementation, regardless of the order in which things happen. Therefore, I suggested a test case that's "guaranteed" to fail with the existing implementation. type Union = {readonly a: string} | {a: string};
expectType<Union>({} as UnionToTuple<Union>[number]);In this case, with the existing implementation, the output of
The proposed test case is "already" fully covered. In the above test case the only necessary members are Again, this problem has nothing to do with object property modifiers. The issue can occur even without property modifiers, as illustrated in the earlier examples. But, to ensure our test case is not flaky, we need a case where |
Just to clarify, here I was only trying to emphasize that this problem is not limited to object property modifiers. I did "not" mean that this should be used as a test case. It should not be, since the behavior here is not predictable. |
test-d/exclude-exactly.ts
Outdated
| expectType<never>({} as ExcludeExactly<unknown, unknown>); | ||
| expectType<never>({} as ExcludeExactly<unknown, any>); | ||
| expectType<never>({} as ExcludeExactly<any, any>); | ||
| expectType<never>({} as ExcludeExactly<any, unknown>); |
There was a problem hiding this comment.
//
unknownandanyexclude themselves.
Why? Is this a requirement or a limitation of the existing implementation?
If this type does an equality check (instead of an extends check), then IMO the ideal behaviour should be:
expectType<unknown>({} as ExcludeExactly<unknown, any>);
expectType<any>({} as ExcludeExactly<any, unknown>);There was a problem hiding this comment.
//
unknownandanyexclude themselves.Why? Is this a requirement or a limitation of the existing implementation?
ExcludeExactly<A, B> would be expected to return
If unknown or any, never).
There was a problem hiding this comment.
expectType<any>({} as ExcludeExactly<any, unknown>);In this case, returning any might actually be fine, as you suggested.
There was a problem hiding this comment.
ExcludeExactly<A, B>would be expected to return$(A \cup B) \setminus B$ .
Isn't
If ExcludeExactly<A, B> is doing just Exclude type already provides that behavior.
Also, if ExcludeExactly<A, B> is ExcludeExactly<1, number> return never, instead of 1?
Wasn't the whole point of building ExcludeExactly that it shouldn't behave like
Currently, this type is not consistent in its behavior. Sometimes it behaves like ExcludeExactly<1, any>), but sometimes it does not (like ExcludeExactly<1, number>).
There was a problem hiding this comment.
Sorry, completely my bad...
Firstly, not
Secondly, I confused some cases: if A is 1, number, 1 | number, and if B is number or unknown.
1 | number means number, so ExcludeExactly properly removes it with ExcludeExactly<1 | number, number>.
This shouldn't mean ExcludeExactly<1, number> returns never.
For the same reason, ExcludeExactly<A, unknown> or , any> shouldn't return never.
You're right, and I think it should use your definition of ExcludeExactly.
I've addressed it, moving SimpleIsEqual to internal and adding test cases for it (4baac7e and 0699dc0).
test-d/exclude-exactly.ts
Outdated
| // `unknown` and `any` exclude other types. | ||
| expectType<never>({} as ExcludeExactly<string | number, unknown>); | ||
| expectType<never>({} as ExcludeExactly<string | number, any>); |
There was a problem hiding this comment.
Similar question here.
Should be this IMO:
expectType<string | number>({} as ExcludeExactly<string | number, unknown>);
expectType<string | number>({} as ExcludeExactly<string | number, any>);There was a problem hiding this comment.
There was a problem hiding this comment.
Is this implementation simpler/better? It passes all the existing tests and also works as suggested with any and unknown.
export type ExcludeExactly<Union, Delete> =
IfNotAnyOrNever<
Union,
_ExcludeExactly<Union, Delete>,
// If `Union` is `any`, then if `Delete` is `any`, return `never`, else return `Union`.
If<IsAny<Delete>, never, Union>,
// If `Union` is `never`, then if `Delete` is `never`, return `never`, else return `Union`.
If<IsNever<Delete>, never, Union>
>;
type _ExcludeExactly<Union, Delete> =
IfNotAnyOrNever<Delete,
Union extends unknown // For distributing `Union`
? [Delete extends unknown // For distributing `Delete`
? If<SimpleIsEqual<Union, Delete>, true, never>
: never] extends [never] ? Union : never
: never,
// If `Delete` is `any` or `never`, then return `Union`,
// because `Union` cannot be `any` or `never` here.
Union, Union
>;
type SimpleIsEqual<A, B> =
(<G>() => G extends A & G | G ? 1 : 2) extends
(<G>() => G extends B & G | G ? 1 : 2)
? true
: false;There was a problem hiding this comment.
Your definition doesn't use LastOfUnion and is clearer about the comparison part, since the SimpleIsEqual pattern is well-known, and all tests pass.
But SimpleIsEqual is already defined in is-equal.d.ts, so should we move it to internal, or even export it? Is that ok?
(I vaguely think it is acceptable because this comparison construct is useful to define some type functions.)
And in this situation, LastOfUnion is still worth exporting as a standalone utility.
There was a problem hiding this comment.
|
Thanks for your clarification and review! What you wanted to say was that we shouldn't focus on the unpredictable union order itself, but on the test cases that must fail with the old The case below makes your point clear to me. type OldUnionToTuple<T, L = LastOfUnion<T>> =
IsNever<T> extends false
? Exclude<T, L> extends infer E // Improve performance.
? [...UnionToTuple<E>, L]
: never // Unreachable.
: [];
type Union1 = {a: string; b: string} | {a: string}
type Tuple1 = OldUnionToTuple<Union1>;
//=> [{a: string}]
type Union2 = {a: string} | {a: string; b: string}
type Tuple2 = OldUnionToTuple<Union2>;
//=> [{a: string}, {a: string, b: string}](But, extremely, I think, we shouldn't expect that writing Now I've addressed your feedback by adding reversed test cases in And add 2 cases more:
Do these new test cases cover your concerns? |
Again, there's some confusion, I "never" meant we should treat
|
Co-authored-by: Som Shekhar Mukherjee <49264891+som-sm@users.noreply.github.com>
|
Do these tests fill the requirements? type DifferentModifiers = {a: 0} | {readonly a: 0};
expectType<UnionToTuple<DifferentModifiers>[number]>({} as DifferentModifiers);
expectType<UnionToTuple<DifferentModifiers>['length']>(2);
These tests were added and passed by this PR's // Different modifiers cases.
type DifferentModifiers = {a: 0} | {readonly a: 0};
expectType<UnionToTuple<DifferentModifiers>[number]>({} as DifferentModifiers);
expectType<UnionToTuple<DifferentModifiers>['length']>(2);
type ReversedDifferentModifiers = {readonly a: 0} | {a: 0};
expectType<UnionToTuple<ReversedDifferentModifiers>[number]>({} as ReversedDifferentModifiers);
expectType<UnionToTuple<ReversedDifferentModifiers>['length']>(2); |
98b5a03 to
01cbb49
Compare
|
Regarding your point about the deficiency in the The structure of these tests is identical to the length check I added for In these tests, I needed a If we go with my definition (of course it should be fixed about What are your thoughts on this? After I've re-read your comment, I've realized this is what you pointed out now.
|
test-d/union-to-tuple.ts
Outdated
| type UnionSuperType0 = {a: string; b: string} | {a: string}; | ||
| expectType<UnionSuperType0>({} as UnionToTuple<UnionSuperType0>[number]); | ||
| expectType<UnionToTuple<UnionSuperType0>['length']>(2); | ||
|
|
||
| type ReversedUnionSuperType0 = {a: string} | {a: string; b: string}; | ||
| expectType<ReversedUnionSuperType0>({} as UnionToTuple<ReversedUnionSuperType0>[number]); | ||
| expectType<UnionToTuple<ReversedUnionSuperType0>['length']>(2); | ||
|
|
||
| type UnionSuperType1 = {a: 1} | {[x: string]: number}; | ||
| expectType<UnionSuperType1>({} as UnionToTuple<UnionSuperType1>[number]); | ||
| expectType<UnionToTuple<UnionSuperType1>['length']>(2); | ||
|
|
||
| type ReversedUnionSuperType1 = {[x: string]: number} | {a: 1}; | ||
| expectType<ReversedUnionSuperType1>({} as UnionToTuple<ReversedUnionSuperType1>[number]); | ||
| expectType<UnionToTuple<ReversedUnionSuperType1>['length']>(2); |
There was a problem hiding this comment.
As mentioned previously, these cases are flaky, and all of them "can" pass with the existing incorrect implementation of UnionToTuple on main. Please remove them.
There was a problem hiding this comment.
I am not sure where the confusion is, I thought we were clear on this.
Can you tell me why are these cases required when we already have this:
// Different modifiers cases.
type DifferentModifiers = {a: 0} | {readonly a: 0};
expectType<UnionToTuple<DifferentModifiers>[number]>({} as DifferentModifiers);There was a problem hiding this comment.
Replaced with super type cases, in exclude-exactly, union-to-tuple, last-of-union.
(And add comment about the union test's limitation.)
There was a problem hiding this comment.
// TypeScript doesn't distinguish the order of a union type, but this is the only way we can write the tests.
type SuperTypeCase = {a: 0} | {a: 0; b: 0};
expectType<UnionToTuple<SuperTypeCase>[number]>({} as SuperTypeCase);
expectType<UnionToTuple<SuperTypeCase>['length']>(2);
type SuperTypeCaseReversed = {a: 0; b: 0} | {a: 0};
expectType<UnionToTuple<SuperTypeCaseReversed>[number]>({} as SuperTypeCaseReversed);
expectType<UnionToTuple<SuperTypeCaseReversed>['length']>(2);If Exclude happens to produce the same result as ExcludeExactly, it's probably impossible to write a test case that only ExcludeExactly would pass.
Given that, I think it's better to keep this case rather than remove it.
What do you think?
There was a problem hiding this comment.
Can you tell me why are these cases required when we already have this:
Sorry, I misread your comment. Please ignore my previous replies.
There was a problem hiding this comment.
Sorry, I misread your comment. Please ignore my previous replies.
So, we're clear on this, right? That we don't need all these cases.
There was a problem hiding this comment.
So, we're clear on this, right? That we don't need all these cases.
Currently, I've left DifferentModifierUnion test and removed UnionSuperType0 ~ ReversedUnionSuperType1 in test-d/union-to-tuple.ts.
If that's what you were referring to, then yes.
…ete unused tests
a9015cc to
e4bd1e8
Compare
5fe4d9e to
e4bd1e8
Compare
|
I've removed any changes about the identical union case tests and |
som-sm
left a comment
There was a problem hiding this comment.
Added comments.
I've also made some improvements in ExcludeExactly tests, please review them once.
source/union-to-tuple.d.ts
Outdated
| export type UnionToTuple<T, L = LastOfUnion<T>> = | ||
| IsNever<T> extends false | ||
| ? [...UnionToTuple<Exclude<T, L>>, L] | ||
| ? ExcludeExactly<T, L> extends infer E // Improve performance. |
There was a problem hiding this comment.
Can there be a test that validates this perf improvement?
There was a problem hiding this comment.
Can there be a test that validates this perf improvement?
Currently, no.
I haven’t started working on this yet...
(This is the comment about this gc issue I posted firstly.)
I made a branch to reproduce this.
This commit, taiyakihitotsu@e8f0967, fixes the issue.
And npm test will crash if this is reverted.
The error logs don't explicitly point to UnionToTuple as the cause, and it’s not a standard TS2589 error.
We might need to implement granular tests for individual type definitions within the package, rather than just running a full type-check on test files.
Without these, it’s difficult to pinpoint exactly where the performance bottleneck or crash is originating.
Also, if we do implement such tests, we’ll need to separately discuss where to set the thresholds (e.g., memory limits or complexity scores).
Thank you! |
There was a problem hiding this comment.
Please create another PR for the purpose of exposing LastOfUnion and move all these changes there.
There was a problem hiding this comment.
Keep this PR only for fixing UnionToTuple and adding new ExcludeExactly type.
There was a problem hiding this comment.
Yes.
I've opened LastOfUnion PR, and updated this PR's title.
Note:
test-d/last-of-union.ts in #1368 uses the old (my defined) ExcludeExactly which is written with LastOfUnion, because LastOfUnion should also be guaranteed to keep its union members.
There was a problem hiding this comment.
Here is the expected workflow:
- Merge
LastOfUnion(PR AddUnionMembertype #1368) - Merge
UnionToTuple(here) - Discuss about
UniqueUnionDeepin (fix:IsEqual,{a: t, b: s}and{a: t} & {b: s}are equal #1338) - (Maybe) Merge
IsEqual(PR fix:IsEqual,{a: t, b: s}and{a: t} & {b: s}are equal #1338) - (And This PR
pick-deep: Fix the case when the key or element is an union type (#1224) #1241 is also related. This has conflicts and I've set it aside currently as a draft.)
How does this look?
There was a problem hiding this comment.
This PR uses the old (my defined)
ExcludeExactlywhich is written withLastOfUnion, becauseLastOfUnionshould also be guaranteed to keep its union members.
Didn't get this. Can't see ExcludeExactly using LastOfUnion.
There was a problem hiding this comment.
Sorry for the confusion, I meant this part here: https://github.com/sindresorhus/type-fest/pull/1368/changes#diff-6ce133a34fd79b9e6b57c337b69241c8a393399e40f7e37e64bba9e421d7c2b0R31
I've edited the comment to clear it.
UnionToTuple, add: LastOfUnion, ExcludeExactly.UnionToTuple, add: ExcludeExactly.
source/union-to-tuple.d.ts
Outdated
| Return a member of a union type. Order is not guaranteed. | ||
| Returns `never` when the input is `never`. | ||
|
|
||
| @see https://github.com/microsoft/TypeScript/issues/13298#issuecomment-468375328 | ||
|
|
||
| Use-cases: | ||
| - Implementing recursive type functions that accept a union type. | ||
| - Reducing a union one member at a time, for example when building tuples. | ||
|
|
||
| It can detect a termination case using {@link IsNever `IsNever`}. | ||
|
|
||
| @example | ||
| ``` | ||
| import type {LastOfUnion, ExcludeExactly, IsNever} from 'type-fest'; | ||
|
|
||
| export type UnionToTuple<T, L = LastOfUnion<T>> = | ||
| IsNever<T> extends false | ||
| ? [...UnionToTuple<ExcludeExactly<T, L>>, L] | ||
| : []; | ||
| ``` | ||
|
|
||
| @example | ||
| ``` | ||
| import type {LastOfUnion} from 'type-fest'; | ||
|
|
||
| type Last = LastOfUnion<1 | 2 | 3>; | ||
| //=> 3 | ||
|
|
||
| type LastNever = LastOfUnion<never>; | ||
| //=> never | ||
| ``` | ||
|
|
||
| @category Type | ||
| */ | ||
| type LastOfUnion<T> = | ||
| UnionToIntersection<T extends any ? () => T : never> extends () => (infer R) | ||
| ? R | ||
| : never; | ||
| true extends IsNever<T> | ||
| ? never | ||
| : UnionToIntersection<T extends any ? () => T : never> extends () => (infer R) | ||
| ? R | ||
| : never; |
There was a problem hiding this comment.
Why are these changes are still here?
There was a problem hiding this comment.
I'll remove this part after #1368 is merged.
Setting this to draft just in case.
There was a problem hiding this comment.
Once #1368 is merged, union-to-tuple.d.ts here can import LastOfUnion (which might be renamed to UnionMember) instead of defining it locally.
This will allow us to finally remove this redundant code.
export type UnionToTuple<T, L = LastOfUnion<T>> =There was a problem hiding this comment.
@taiyakihitotsu But that's not necessary, let's remove these changes and finalise this first.
There was a problem hiding this comment.
@taiyakihitotsu But that's not necessary, let's remove these changes and finalise this first.
I have removed it in 2d728ee, let's see if it breaks anything.
There was a problem hiding this comment.
I see what you mean now after looking at your commit 2d728ee.
Thank you!
There was a problem hiding this comment.
Everything seems to be working fine. Let's remove this PR from draft?
UnionToTuple, add: ExcludeExactly.UnionToTuple: Fix behavior when a union member is a supertype of another; Add ExcludeExactly type
The current (7c82a21)
UnionToTuplefails in the following case:This is
npm testerror:Add
ExcludeExactlyThis is caused by built-in
Exclude, used inUnionToTuple.This doesn't distinguish between
readonlymodifiers of objects.This is a spec: https://www.typescriptlang.org/docs/handbook/utility-types.html#excludeuniontype-excludedmembers
And I think it's not a bug of
type-fest'sExcludeStrict, because it also refers to assignability here: https://github.com/sindresorhus/type-fest/blob/main/source/extends-strict.d.ts#L5Both are based on assignability, but we need stricter version of
Excludeand this PR defines it asExcludeExactly.Add
LastOfUnionAnd to define it, we need another local type
LastOfUnion, using a well-known definition, defined already inIsNever.This is useful to recurse union-types, so I think this should be exported as a global.
(And we can evade a duplicated definition potentially).
And the current
LastOfUnionused inIsNever, this test case doesn't pass:because$A \cup B$ -> $B$ , so $\emptyset$ -> $\emptyset$ intuitively ($\bot$ -> $\bot$ ).
LastOfUnion<never>returnsunknown.Ideally,
LastOfUnionreduces a type, e.g.,This PR's
LastOfUnionhandles this.Fix
UnionToTupleHere is the definition:
There're 2 diff.
ExcludeExactlyinstead ofExcludeorExcludeStrict.ExcludeExactly<T, L>isn't expanded directly. It stores it toinfer Eto improve memory performance. (See the context: fix:IsEqual,{a: t, b: s}and{a: t} & {b: s}are equal #1338 (comment))This PR includes the above 3 changes.
Refers to: https://github.com/sindresorhus/type-fest/blob/main/.github/contributing.md
I think this is a "connected" case, because
UnionToTupleneedsExcludeExactlyExcludeExactlyneedsLastOfUnion(This PR can be split into three parts, though it would require careful consideration of the merge order:
LastOfUnion->ExcludeExactly->UnionToTuple)NOTE:
This is separate from #1338.