Skip to content

Conversation

@taiyakihitotsu
Copy link
Contributor

@taiyakihitotsu taiyakihitotsu commented Sep 24, 2025

This PR fixes #1224.
(I’m opening this because I previously sent a message about the issue, but it seems that no one has started working on it.)

Approach

The issue can be broken down into two separate bugs:

  • PickDeep fails to pick values when the object’s properties are of a union type. (2025/09/29 ... ok)
  • PickDeep doesn't properly merge results when the path is a union type. (2025/09/29 ... it has a bug) (2025/10/01 ... ok)

The first was fixed by applying distribution over the union type in the object.

For the second issue, the fix involves evaluating each path in the union individually, obtaining the result as a union of objects, and then merging them using MergeNarrow. (2025/10/01 ... See)

related issue

I'm not sure whether the expected value shown in #1223 is truly correct according to the PickDeep specification, so I’ve decided to leave that issue for now.

(In the current commit, the result for that case would be {arr: []}. I think this is caused by MergeNarrow.) (2025/10/01 ... See)

If I should include a fix for #1223 in this PR as well, please let me know.

note

I rewrote the tests to be more strict, using the following pattern:

expectType<true>({} as IsEqual<Actual, Expected>)

This is because some previous tests using expectType<T>(value) were passing unintentionally.

@som-sm
Copy link
Collaborator

som-sm commented Sep 27, 2025

@taiyakihitotsu Thanks for the PR! I’m a bit occupied with festivities, I’ll try to review it sometime next week.

@taiyakihitotsu
Copy link
Contributor Author

I've added a fix commit for #1223.

@sindresorhus
Thanks!

@som-sm
Understood, I’ll wait. Enjoy!

@taiyakihitotsu
Copy link
Contributor Author

I found a bug in this case and will fix it as well.

type unionKeyUnionObject_Actual = PickDeep<{obj: {a: string; b: number; c: boolean} | {d: 0}}, `obj.${'b' | 'c' | 'd'}`>; // {obj: {b: number; d: 0; c: boolean}}

@taiyakihitotsu
Copy link
Contributor Author

And LastOfUnion is also defined in

type LastOfUnion<T> =

I'll move it to internal if ok to combine them into one PR.

@taiyakihitotsu
Copy link
Contributor Author

I’ve got an idea for the fix, but not sure when I can get back to it, so I leave this as a draft for now.
Maybe I’ll pick it up again in a week or so.

@taiyakihitotsu taiyakihitotsu marked this pull request as draft September 28, 2025 16:04
@taiyakihitotsu taiyakihitotsu marked this pull request as ready for review October 1, 2025 10:56
@taiyakihitotsu
Copy link
Contributor Author

taiyakihitotsu commented Oct 1, 2025

@som-sm

I fixed the issues (#1224, #1223) and a bug in the comment I posted earlier.
Sorry about the size of the diff; I wrote it nearly the whole structure.

diff

I defined a converter that transforms template literals like a.${'b' | 'c'} | 'x' into a tree structure like {a: {b: ''; c: ''}; x: ''}.
This makes it easier to get a union type using keyof at the same path level, which helps preserve the shape when merging.

(When the implementation of Path relies on Union type distribution, the original structure can be lost. For example, in an extreme case like {a: 0, b: 0} | {a: 1, b: 1}, if they are not merged at the same point in time, it may end up being split into {a: 0} | {a: 1} | {b: 0} | {b: 1}.)

I've also defined some internal helper functions for certain getter functions, which ignore the distinction between string and number types for numeric keys.
(Since the path does not differentiate between the two, there's no meaningful reason for the source object to do so either.)

IsNever bug

Outdated. see this
(Check the edit history if you want to see)

move to internal

It seems reasonable to mark some of the functions as internal.
(Example: IsKeyOf, LastOfUnion, _IsEqual (rename to -> InternalIsEqual if IsNever uses it for a fix)
Would it be acceptable to include that change in this PR as well?

@som-sm
Copy link
Collaborator

som-sm commented Oct 1, 2025

The current IsNever implementation has a bug:

Is there a simpler reproduction of where IsNever fails?

@taiyakihitotsu
Copy link
Contributor Author

taiyakihitotsu commented Oct 1, 2025

This comment is outdated, see this new comment


@som-sm

It may only fail when using recursion with LastOfUnion<T> and IsNever. This is the same issue we’re currently facing with IsEqual.

Current IsEqual

Fixed

Failed

  • never returned LastOfUnion.

Internal _IsEqual

type _IsEqual<A, B> =
	(<G>() => G extends A & G | G ? 1 : 2) extends
	(<G>() => G extends B & G | G ? 1 : 2)
		? true
		: false;

This has bugs above the fixed part.
But it can detect well of never of LastOfUnion if defining like:

export type CurrentIsNever<T> = _IsEqual<T, never>

whole code

I'm researching it in source/is-equal.d.ts.

export type IsEqual<A, B> =
	[A, B] extends [infer AA, infer BB]
		? [AA] extends [never]
			? [BB] extends [never]
				? true
				: false
			: [BB] extends [never]
				? false
				: _IsEqual<AA, BB>
		: false;

import type {UnionToIntersection} from './union-to-intersection.d.ts'
type LastOfUnion<T> = UnionToIntersection<T extends any ? () => T : never> extends () => (infer R)	? R : never;

export type PreviousIsNever<T> = [T] extends [never] ? true : false;
export type IsNever<T> = IsEqual<T, never>
export type CurrentIsNever<T> = _IsEqual<T, never>

type PreviousUnionRecursion<T, R = {}> = LastOfUnion<T> extends infer L ? PreviousIsNever<L> extends false ? PreviousUnionRecursion<Exclude<T, L>, R | L> : R : never;
type UnionRecursion<T, R = {}> = LastOfUnion<T> extends infer L ? IsNever<L> extends false ? UnionRecursion<Exclude<T, L>, R | L> : R : never;
type CurrentUnionRecursion<T, R = {}> = LastOfUnion<T> extends infer L ? CurrentIsNever<L> extends false ? CurrentUnionRecursion<Exclude<T, L>, R | L> : R : never;

// @ts-expect-error
type PrevTest = PreviousUnionRecursion<string | number>;
// Type instantiation is excessively deep and possibly infinite.

// @ts-expect-error
type FixedTest = UnionRecursion<string | number>; 
// Type instantiation is excessively deep and possibly infinite.

type CurrentFixedTest = CurrentUnionRecursion<string | number>; // string | number | {}

// This version fails the `equalWrappedTupleIntersectionToBeNeverAndNeverExpanded` test in `test-d/is-equal.ts`.
type _IsEqual<A, B> =
	(<G>() => G extends A & G | G ? 1 : 2) extends
	(<G>() => G extends B & G | G ? 1 : 2)
		? true
		: false;

I have two plans.

One is to redefine IsNever as shown above in CurrentIsNever, and at the same time make _IsEqual internal and rename like InternalIsEqual. This solution has already been discovered, is simple to implement, and maintains consistency in terms of never checking. However, it sacrifices some theoretical rigor.
(Users may select IsEqual<L, never> instead of IsNever<L> when using LastOfUnion<T> extends infer L, then it fails.)

The other plan is to fix IsEqual and then redefine IsNever using the revised IsEqual. So far, this approach has not been successful.

Either way, I feel it's better to make the PR separate.

@taiyakihitotsu
Copy link
Contributor Author

taiyakihitotsu commented Oct 1, 2025

Ah, maybe this isn't IsEqual's job — it might be LastOfUnion's responsibility.

type LastOfUnion<T> =
  UnionToIntersection<
    T extends infer A
      ? () => A
      : never> extends () => (infer R)
    ? R extends T ? R : never
    : never;

This fixes all above cases.
I will research a lot.

This works well only in the test cases above, but it cannot replace the existing code.

@taiyakihitotsu
Copy link
Contributor Author

taiyakihitotsu commented Oct 1, 2025

@som-sm

type LastOfUnion<T> =
  UnionToIntersection<
    T extends infer A
      ? () => A
      : never> extends () => (infer R)
    ? R
    : never;

type a = LastOfUnion<never>; // unknown

OK I see that this returns unknown, and _IsEqual<never, unknown> returns true unintentionally.
So LastOfUnion<U> extends infer L and L extends Something works as expected, IsNever using _IsEqual works fine.
But the current IsEqual returns false for L even if it's expected to be never (L is unknown in this case though).

The current (at 590efae8223ed1ac61855840dff114360b7c6807) definition of IsEqual and IsNever aren't incorrect.
It's not a bug in IsNever.

type test = LastOfUnion<Exclude<string, string>> extends infer U ? PreviousIsNever<U> : 'false';
// ^ false

export type PreviousIsNever<T> = [T] extends [never] ? true : false;

Apologies for my misunderstanding.

In any case, LastOfUnion is already used in source/union-to-tuple.d.ts, and this commit defines it redundantly.
So I want to fix and move it to internal.

Should I include it in this PR? Or not?

@sindresorhus
Copy link
Owner

Should I include it in this PR? Or not?

In this PR, yes.

@taiyakihitotsu
Copy link
Contributor Author

@sindresorhus @som-sm

Thanks for your response!
I've added a commit moving LastOfUnion to source/internal/type.d.ts.

I wrote a comment and provided two examples of how to use it for recursion.

And I made a small change to UnionToTuple, hiding the second argument.

The latest status of this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PickDeep does not pick types unioned to object or arrays

3 participants