10BC0 `Merge`: Ensure idempotency so `Merge<A, A>` returns `A` by taiyakihitotsu · Pull Request #1336 · sindresorhus/type-fest · GitHub
[go: up one dir, main page]
Skip to content

Merge: Ensure idempotency so Merge<A, A> returns A #1336

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.

Sign up for GitHub

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

Jump to bottom
Open
taiyakihitotsu wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Merge: Ensure idempotency so Merge<A, A> returns A #1336

taiyakihitotsu wants to merge 16 commits into from
+19 −4

Conversation

@taiyakihitotsu
Copy link
Contributor
@taiyakihitotsu taiyakihitotsu commented Jan 27, 2026 •
edited
Loading

Current Status

See: #1336 (comment) -> #1336 (comment)
Below the first my comment is wrong because my misunderstanding about branded type and intersection of objects.

Mainly changes FixedLengthArray

Background

Object merged via Simplify and & (e.g., {a: 0, b: 0}) and those merged only via & (e.g., {a: 0} & {b: 0}) shouldn't be treated as the same type, to consider Branded Type or the fact that intersection types are handled as distinct constructs.

In the previous (meaning before this PR, fa55f48) FixedLengthArray definition, the internal objects are directly merged only via & which broke the idempotency of Merge.

So the previous status:

  • General Rule: Merge<A, A> should be identical to A itself, despite a lack of test cases.
  • But, Merge<A, A> did not result in exactly A when FixedLengthArray was involved.

Major Changes

  • New Test Cases: Added tests in test-d/is-equal.ts and test-d/merge.ts to ensure Merge idempotency and strict handling of Branded Types.
  • FixedLengthArray Fix: Updated the definition to use Simplify for internal object merging.

Now Merge<FixedLengthArray<A, L>, FixedLengthArray<A, L>> is equivalent to FixedLengthArray<A, L>.

// Idempotency
expectType<BarWithIndexSignatureOverwrite>({} as Merge<BarWithIndexSignatureOverwrite, BarWithIndexSignatureOverwrite>);
expectType<FixedLengthArray<string, 3>>({} as Merge<FixedLengthArray<string, 3>, FixedLengthArray<string, 3>>);

Minor Changes

Refactoring source/is-equal.d.ts

@som-sm
Copy link
Collaborator
som-sm commented Jan 27, 2026 •
edited
Loading

@taiyakihitotsu I don't get this, if the problem is with Merge, then shouldn't Merge be fixed, and not FixedLengthTuple?

If I understand correctly, this is the problem, right?

// Idempotency
type Test1 = {a: string} & {b: string};
expectType<Test1>({} as Merge<Test1, Test1>);

The above test still fails. Wrapping FixedLengthTuple with Simplify doesn't fix the root cause.

@som-sm
Copy link
Collaborator
som-sm commented Jan 27, 2026

Probably Merge needs something like If<IsEqual<Destination, Source>, Destination, ...>.

taiyakihitotsu and others added 2 commits January 27, 2026 19:54
@taiyakihitotsu
fix: IsEqual, intersection objects are equivalent to merged objects
d513e9d
@taiyakihitotsu @som-sm
fix: Merge, return arg if both are equal
02b412b 
Co-authored-by: Som Shekhar Mukherjee <49264891+som-sm@users.noreply.github.com>
@taiyakihitotsu taiyakihitotsu force-pushed the fix/fixed-length-array/20260127 branch from c5a99cf to 02b412b Compare January 27, 2026 11:00
@taiyakihitotsu
Copy link
Contributor Author
taiyakihitotsu commented Jan 27, 2026 •
edited
Loading

@som-sm

Thanks!
It's my fault about branded type I wrote as the 1st comment.

I re-read the official doc of intersection, I finally fix my misunderstanding.
https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html#intersection-types

As the above doc and you mentioned, {a: 0} & {b: 0} should be equivalent to {a: 0, b: 0} on IsEqual,
but the current definition (this PR) doesn't satisfy it.

I've added new 2 commits.

  • Fix IsEqual. The previous definition didn't get that {a: 0} & {b: 0} should be equal to {a: 0, b: 0}.
  • Fix Merge. I addressed your suggestion, and added you as co-author.

So finally this PR fixes:

  • source/is-equal.d.ts: {a: 0} & {b: 0} should be equal to {a: 0, b: 0}, (and a bit of refactor)
  • source/merge.d.ts: return Destination directly if Destination and Source are equal via fixed IsEqual
  • source/fixed-array-length.d.ts: just Simplify wrapping
  • test/merge.ts: added test case to ensure Merge<A, A> should be equal to A
  • test/is-equal.ts: added test case to ensure IsEqual<{a: 0} & {b: 0}, {a: 0, b: 0}> is true

@taiyakihitotsu
Copy link
Contributor Author
taiyakihitotsu commented Jan 27, 2026 •
edited
Loading

I've added this change, b101b8e, but the following errors occur:

Merge defined with Simplify

export type Merge<Destination, Source> = If<IsEqual<Destination, Source>, Simplify<Destination>, _Merge<Destination, Source>>;

This test fails via npm test.

type TestIntersectionObject = {a: string} & {b: string};

expectType<TestIntersectionObject>({} as Merge<TestIntersectionObject, TestIntersectionObject>);

Output:

test-d/merge.ts:203:0
  ✖  203:0  Parameter type TestIntersectionObject is not identical to argument type { a: string; b: string; }

Merge defined without Simplify

export type Merge<Destination, Source> = If<IsEqual<Destination, Source>, Destination, _Merge<Destination, Source>>;
  test-d/merge.ts:202:0
  ✖  202:0  Parameter type TestGeneralObject is not identical to argument type TestIntersectionObject.

type TestGeneralObject = {a: string; b: string};

type TestIntersectionObject = {a: string} & {b: string};
type IDMergeIntersection = Merge<TestIntersectionObject, TestIntersectionObject>;

expectType<TestGeneralObject>({} as IDMergeIntersection);

New assignable tests I added are passed in both cases.

@som-sm
What do you think?
I think it cannot maintain equivalence between {a: 0} & {b: 0} and {a: 0, b: 0} outside of the context of Merge or IsEqual.
(Assignable tests are passed though.)
To ensure predictability for the users, which structure do you think is better to return in this case?

@som-sm
Copy link
Collaborator
som-sm commented Jan 27, 2026

To ensure predictability for the users, which structure do you think is better to return in this case?

The non-simplified version, i.e., If<IsEqual<Destination, Source>, Destination, ...>;


I think it cannot maintain equivalence between {a: 0} & {b: 0} and {a: 0, b: 0} outside of the context of Merge or IsEqual.

That’s right, expectType doesn’t consider {a: 0} & {b: 0} equal to {a: 0; b: 0}. But, I guess that shouldn't be a problem here, because if you use the non-simplified approach, then the following test should pass.

type TestIntersectionObject = {a: string} & {b: string};

expectType<TestIntersectionObject>({} as Merge<TestIntersectionObject, TestIntersectionObject>);

Also, it looks like we're mixing two different problems over here.

  1. IsEqual<{a: 0; b: 0}, {a: 0} & {b: 0}> currently returns false, but instead should return true.
  2. Merge<A, A> should return A w/o any modification/simplification.

So, please open a separate PR for the IsEqual fix.

Also, inside Merge, I think we need to let the types distribute first before checking for equality (IsEqual<Destination, Source>). So, the following tests should pass:

type TestIntersectionObject = {a: string} & {b: string};

expectType<TestIntersectionObject | {a: string; b: string; c: string}>({} as Merge<TestIntersectionObject | {c: string}, TestIntersectionObject>);
expectType<TestIntersectionObject | {a: string; b: string; c: string}>({} as Merge<TestIntersectionObject, TestIntersectionObject | {c: string}>);
  • source/fixed-array-length.d.ts: just Simplify wrapping

Please remove this, it's not required.

@taiyakihitotsu taiyakihitotsu mentioned this pull request Jan 27, 2026
Draft
@taiyakihitotsu
Copy link
Contributor Author
taiyakihitotsu commented Jan 27, 2026

@som-sm

Thanks a lot!
I open the separeted PR for IsEqual: #1338

Should I close this PR to separate the changes of Merge as well?
Should we keep using this PR even after this PR above?

I'll respond to your other comments later.

@som-sm
Copy link
Collaborator
som-sm commented Jan 27, 2026

Should I close this PR to separate the changes of Merge as well?
Should we keep using this PR even after this PR above?

Use this PR for the Merge changes, i.e., Merge<A, A> should return A, remove the IsEqual changes from this PR.

taiyakihitotsu and others added 3 commits January 27, 2026 22:48
@taiyakihitotsu
revert: IsEqual
51877be
@taiyakihitotsu
revert: FixedLengthArray
08465ff
@taiyakihitotsu @som-sm
fix: remove Simplify from Merge, ensure union distribution
fc17bb6 
Co-authored-by: Som Shekhar Mukherjee <49264891+som-sm@users.noreply.github.com>
@taiyakihitotsu
Copy link
Contributor Author
taiyakihitotsu commented Jan 27, 2026

@som-sm

The non-simplified version, i.e., If<IsEqual<Destination, Source>, Destination, ...>;

Thanks, I've removed Simplify.

That’s right, expectType doesn’t consider {a: 0} & {b: 0} equal to {a: 0; b: 0}. But, I guess that shouldn't be a problem here, because if you use the non-simplified approach, then the following test should pass.

I got it!
I added the test and it is passed.

Also, it looks like we're mixing two different problems over here.

I separated the IsEqual changes into a different PR (#1338) and kept this one for Merge.
Since FixedLengthArray is no longer changed, I've updated the PR title.

Also, inside Merge, I think we need to let the types distribute first before checking for equality (IsEqual<Destination, Source>). So, the following tests should pass:

Addressed. I separated Merge into two parts:

  • exported Merge: responsible for union distribution and the If logic described above.
  • local _Merge: uses the Simplify codebase from the previous Merge implementation.

@taiyakihitotsu taiyakihitotsu changed the title Fix: keep idempotency of FixedLengthArray in Merge Fix: keep idempotency of Merge Jan 27, 2026
som-sm
som-sm requested changes Jan 28, 2026
View reviewed changes
som-sm
som-sm requested changes Jan 28, 2026
View reviewed changes
@taiyakihitotsu taiyakihitotsu requested a review from som-sm January 28, 2026 05:58
@taiyakihitotsu
Copy link
Contributor Author
taiyakihitotsu commented Jan 28, 2026

@som-sm

Thanks for your quick review!
I've addressed both.

@som-sm som-sm changed the title Fix: keep idempotency of Merge Merge: Ensure idempotency so Merge<A, A> returns A Jan 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@som-sm som-sm som-sm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@taiyakihitotsu @som-sm
0