Repository navigation
Possible regression in overloads with optional this types in callbacks #53080
Description
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.
Activity
jakebailey commentedon Mar 3, 2023
FYI Mateusz Burzyński (@Andarist)
Andarist commentedon Mar 3, 2023
Jake Bailey (@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 commentedon Mar 3, 2023
Chris Krycho (@chriskrycho) Did you bisect to the PR in particular, or do you only know the nightly revisions?
Andarist commentedon Mar 3, 2023
I bisected it just now and the culprit is this one: #52387
chriskrycho commentedon Mar 3, 2023
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 commentedon Mar 3, 2023
If you replace
neverwith[]within yourPartialParamsthen it starts to work again: TS playgroundI think this is the correct solution here. With this change the
checkCandidatehere changes like this: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 notnever).chriskrycho commentedon Mar 4, 2023
Thanks!
Andarist commentedon Mar 4, 2023
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 commentedon Mar 4, 2023
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 commentedon Mar 5, 2023
And this is why we're merging all of this ASAP 😄