Possible regression in overloads with optional this types in callbacks

[chriskrycho]

Bug Report

I believe #52838 #52387 is the cause of a “regression” in Ember’s type test suite, but I also am very willing to believe that this is a bug in how Ember’s types are written which this change caught, but I wanted to flag it up.

Here's the type test (source):

expectTypeOf(
  bind(foo, function (_foo: number, _bar: boolean, _baz?: string): number {
    expectTypeOf(this).toEqualTypeOf<Foo>();
    return 1;
  })
).toEqualTypeOf<(foo: number, bar: boolean, baz?: string) => number | void>();

Here's the set of overloads, including the implementation overload (source):

export function bind<
  T,
  F extends (this: T, ...args: any[]) => any,
  A extends PartialParams<Parameters<F>>
>(
  target: T,
  method: F,
  ...args: A
): (...args: RemainingParams<A, Parameters<F>>) => ReturnType<F> | void;
export function bind<F extends AnyFn, A extends PartialParams<Parameters<F>>>(
  method: F,
  ...args: A
): (...args: RemainingParams<A, Parameters<F>>) => ReturnType<F> | void;
export function bind<
  T,
  U extends keyof T,
  A extends T[U] extends AnyFn ? PartialParams<Parameters<T[U]>> : []
>(
  target: T,
  method: U,
  ...args: A
): T[U] extends AnyFn
  ? (...args: RemainingParams<A, Parameters<T[U]>>) => ReturnType<T[U]> | void
  : never;
export function bind(...curried: any[]): any {
  // ...
}

Prior to this change, this type checked. After this change, it reports:

No overload matches this call.
  Argument of type '[]' is not assignable to parameter of type 'never'.
  Overload 2 of 3, '(method: AnyFn, ...args: never): (...args: never) => any', gave the following error.
    Argument of type 'Foo' is not assignable to parameter of type 'AnyFn'.
  Overload 3 of 3, '(target: Foo, method: "test"): (_foo: number, _bar: boolean, _baz?: string | undefined) => number | void', gave the following error.
    Argument of type '(this: Foo, _foo: number, _bar: boolean, _baz?: string | undefined) => number' is not assignable to parameter of type '"test"'.ts(2769)
index.ts(295, 17): The call would have succeeded against this implementation, but implementation signatures of overloads are not externally visible.

Prior to this change, it was resolving against the first overload:

export function bind<
  T,
  F extends (this: T, ...args: any[]) => any,
  A extends PartialParams<Parameters<F>>
>(
  target: T,
  method: F,
  ...args: A
): (...args: RemainingParams<A, Parameters<F>>) => ReturnType<F> | void;

I have tried adding an overload which has the same form but does not explicitly include the this type on the F constraint, but at that point I'm in overload-ordering hell.

🔎 Search Terms

• overloads
• this type

🕗 Version & Regression Information

This changed between versions 5.1.0-dev.20230301 and 5.1.0-dev.20230302.

⏯ Playground Link

Playground link with relevant code

💻 Code

import { expectTypeOf } from 'expect-type';

// ----- TEST CODE ----- //
class Foo {
  literallyAnythingToAvoidAnEmptyClass() {}
}

let foo = new Foo();

expectTypeOf(
  bind(foo, function (_foo: number, _bar: boolean, _baz?: string): number {
    expectTypeOf(this).toEqualTypeOf<Foo>();
    return 1;
  })
).toEqualTypeOf<(foo: number, bar: boolean, baz?: string) => number | void>();

// ----- OVERLOAD DEFINITIONS ----- //

export function bind<
  T,
  F extends (this: T, ...args: any[]) => any,
  A extends PartialParams<Parameters<F>>
>(
  target: T,
  method: F,
  ...args: A
): (...args: RemainingParams<A, Parameters<F>>) => ReturnType<F> | void;
export function bind<F extends AnyFn, A extends PartialParams<Parameters<F>>>(
  method: F,
  ...args: A
): (...args: RemainingParams<A, Parameters<F>>) => ReturnType<F> | void;
export function bind<
  T,
  U extends keyof T,
  A extends T[U] extends AnyFn ? PartialParams<Parameters<T[U]>> : []
>(
  target: T,
  method: U,
  ...args: A
): T[U] extends AnyFn
  ? (...args: RemainingParams<A, Parameters<T[U]>>) => ReturnType<T[U]> | void
  : never;
// overload implementation
export function bind(...curried: any[]): any {
  // ...
}


// ----- TYPE UTILITIES ----- //

export type AnyFn = (...args: any[]) => any;

type PartialParams<P extends any[]> = P extends [infer First, ...infer Rest]
  ? [] | [First] | [First, ...PartialParams<Rest>]
  : // This is necessary to handle optional tuple values
  Required<P> extends [infer First, ...infer Rest]
  ? [] | [First | undefined] | [First | undefined, ...PartialParams<Partial<Rest>>]
  : never;

type RemainingParams<PartialParams extends any[], All extends any[]> = PartialParams extends [
  infer First,
  ...infer Rest
]
  ? All extends [infer AllFirst, ...infer AllRest]
    ? First extends AllFirst
      ? RemainingParams<Rest, AllRest>
      : never
    : // This is necessary to handle optional tuple values
    Required<All> extends [infer AllFirst, ...infer AllRest]
    ? First extends AllFirst | undefined
      ? Partial<RemainingParams<Rest, AllRest>>
      : never
    : never
  : PartialParams extends []
  ? All
  : never;

🙁 Actual behavior

Wellllll the overload isn't working anymore.

🙂 Expected behavior

I think I expected this overload to keep working.

[jakebailey]

FYI @Andarist

[Andarist]

@jakebailey thanks for the ping, I'll try to investigate this to understand what has happened here. This regression is definitely surprising since there is no spread involved here.

[jakebailey]

@chriskrycho Did you bisect to the PR in particular, or do you only know the nightly revisions?

[Andarist]

I bisected it just now and the culprit is this one: #52387

[chriskrycho]

Ah, I only knew nightly revisions—I missed 52387 in my history review (PR history vs. Git history fail), and hadn't yet gotten down to bisecting individual commits. Sorry for the misdirection!

[Andarist]

If you replace never with [] within your PartialParams then it starts to work again: TS playground

I think this is the correct solution here. With this change the checkCandidate here changes like this:

-type CheckCandidate = (target: Foo, method: (this: Foo, ...args: any[]) => any, ...args: never): (...args: never) => any
+type CheckCandidate = (target: Foo, method: (this: Foo, ...args: any[]) => any): (...args: any[]) => any

And that new one makes more sense here. At this point we already know that there are no extra arguments and when spreading [] represents that (and not never).

[chriskrycho]

Thanks!

[Andarist]

While this got fixed in Ember types... I wonder how many more such reports we will get. Hopefully not many, I think this case was slightly off from the beginning but it was also relatively easy to make such a mistake. The time will tell.

[chriskrycho]

I agree that the original implementation was wrong, but unfortunately I suspect it’s semi-common, as the contributor who landed it in our types drew heavily on a Stack Overflow answer. I reviewed it when it landed, but did not notice the underlying issue originally or even when re-evaluating it yesterday. It’s quite subtle and it required basically doing the full “type check” pass mentally to see the issue.

[jakebailey]

While this got fixed in Ember types... I wonder how many more such reports we will get. Hopefully not many, I think this case was slightly off from the beginning but it was also relatively easy to make such a mistake. The time will tell.

And this is why we're merging all of this ASAP 😄