v4.0.x -> v4.1.1-rc change in overload selection

[tchetwin]

TypeScript Version: 4.1.0-dev.20201029, 4.1.1-rc

Search Terms: overload

Code

import { promisify } from "util";

// These are components of @types/xml2js@0.4.5
export interface convertableToString {
    toString(): string;
}
declare class Parser {
    parseString(str: convertableToString, cb?: Function): void;
}

const xmlParser = new Parser();
const parseString = xmlParser.parseString.bind(xmlParser);
const parseStringP = promisify(parseString); // <--- interested in overload selection from this line

parseStringP("abc");
//           ^--- error surfaces here

Expected behavior:

No error. This overload is selected from promisify:

(alias) promisify<convertableToString, unknown>(fn: (arg1: convertableToString, callback: (err: any, result: unknown) => void) => void): (arg1: convertableToString) => Promise<...> (+13 overloads)

Actual behavior:

Expected 0 arguments, but got 1. (2554) (on the argument to parseStringP). This overload is selected from promisify:

(alias) promisify<unknown>(fn: (callback: (err: any, result: unknown) => void) => void): () => Promise<unknown> (+13 overloads)

Notes:

Overloads from unpkg:

interface CustomPromisifyLegacy<TCustom extends Function> extends Function {
    __promisify__: TCustom;
}
interface CustomPromisifySymbol<TCustom extends Function> extends Function {
    [promisify.custom]: TCustom;
}
type CustomPromisify<TCustom extends Function> = CustomPromisifySymbol<TCustom> | CustomPromisifyLegacy<TCustom>;

function promisify<TCustom extends Function>(fn: CustomPromisify<TCustom>): TCustom;
function promisify<TResult>(fn: (callback: (err: any, result: TResult) => void) => void): () => Promise<TResult>;
function promisify(fn: (callback: (err?: any) => void) => void): () => Promise<void>;
function promisify<T1, TResult>(fn: (arg1: T1, callback: (err: any, result: TResult) => void) => void): (arg1: T1) => Promise<TResult>;
function promisify<T1>(fn: (arg1: T1, callback: (err?: any) => void) => void): (arg1: T1) => Promise<void>;
function promisify<T1, T2, TResult>(fn: (arg1: T1, arg2: T2, callback: (err: any, result: TResult) => void) => void): (arg1: T1, arg2: T2) => Promise<TResult>;
function promisify<T1, T2>(fn: (arg1: T1, arg2: T2, callback: (err?: any) => void) => void): (arg1: T1, arg2: T2) => Promise<void>;
// removed overloads with 3, 4 & 5 template parameters for brevity
function promisify(fn: Function): Function;

Playground Links:

4.1.0-beta ✔️
4.1.0-dev.20201028 ✔️
4.1.0-dev.20201029 ❌
4.1.1-rc ❌
4.2.0-dev.20201112 ❌

Related Issues:

#41099 - Similar looking but the version introducing the regression doesn't align.
#41359 (comment) - Previously reported in this Issue

[robpalme]

@DanielRosenwasser This is the late-breaking 4.1 compatibility break I mentioned earlier. It didn't really hamper the upgrade because it was easy to circumvent.

[DanielRosenwasser]

@RyanCavanaugh let's discuss about this briefly before the release. It sounds like it's not a blocker, but I can't make sense of the change here and it affects the inference of Node.js's promisify.

[DanielRosenwasser]

@ahejlsberg too - unclear if #41099 is related.

[DanielRosenwasser]

@ahejlsberg I believe this regression came from #41308

Here's an easier diff view https://github.com/microsoft/TypeScript/pull/41308/files?w=1

[DanielRosenwasser]

Okay, here is the minimal repro:

declare function promisify<TResult>(fn: (callback: (err: any, result: TResult) => void) => void): () => Promise<TResult>;
declare function promisify<T1, TResult>(fn: (arg1: T1, callback: (err: any, result: TResult) => void) => void): (arg1: T1) => Promise<TResult>;

declare function parseString(str: any, cb?: Function): void;

const parseStringP = promisify(parseString);

let x = parseStringP("abc");

This works in TypeScript 4.0, but not in 4.1 or after that PR. If you swap the overloads, then things work again.

---

Edit: slightly better repro here

declare function parseString(str: any, cb?: (...args: any) => any): void;

const parseStringP = promisify(parseString);

let x = parseStringP("abc");

const arg1UnknownError: (arg: string) => Promise<number> = promisify((arg: string, cb: (err: unknown, result: number) => void): void => { });

declare function promisify<TResult>(fn: (callback: (err: any, result: TResult) => void) => void): () => Promise<TResult>;
declare function promisify<T1, TResult>(fn: (arg1: T1, callback: (err: any, result: TResult) => void) => void): (arg1: T1) => Promise<TResult>;

[DanielRosenwasser]

I tried swapping each of the corresponding void/non-void callback operators, let's see if it can just be a DefinitelyTyped fix: DefinitelyTyped/DefinitelyTyped#49640

[ahejlsberg]

I took a look. Short story, the current behavior is correct and the previous behavior wasn't. But it is incredibly subtle. It comes down to relating the types

• source (str: convertableToString, cb?: Function | undefined) => void and
• target (cb: (err: any, result: unknown) => void) => void

during overload resolution. First subtlety is that convertableToString is effectively equivalent to {} because every object has a toString(): string method. So, the cb and str parameters end up being compatible, and since target only supplies one parameter there is no need to relate the cb? parameter to anything. Overload resolution therefore succeeds and picks an overload that subsequently causes an error.

I didn't succeed before because we'd relate type Function | undefined to any for the second parameter instead of not relating at all. The intent here was to have it always succeed (because anything is assignable to any). However, the first pass of overload resolution uses the subtype relation, and in that relation any is a top type and not assignable to Function | undefined. So, overload resolution would (erroneously) skip the overload.

Like I said, subtle!

The real issue is that the overloads aren't selective enough and/or listed in the wrong order.

[DanielRosenwasser]

I don't think there's an obvious way to reorder promisify in a way that doesn't introduce a new break. Here's a few test cases included:

declare function parseString(str: any, cb?: (...args: any) => any): void;

const parseStringP = promisify(parseString);

let x = parseStringP("abc");

const arg1UnknownError: (arg: string) => Promise<number> = promisify((arg: string, cb: (err: unknown, result: number) => void): void => { });
const arg1AnyError: (arg: string) => Promise<number> = promisify((arg: string, cb: (err: any, result: number) => void): void => { });

const arg0: () => Promise<number> = promisify((cb: (err: Error | null, result: number) => void): void => { });
const arg0NoResult: () => Promise<any> = promisify((cb: (err: Error | null) => void): void => { });
const arg1: (arg: string) => Promise<number> = promisify((arg: string, cb: (err: Error | null, result: number) => void): void => { });
const arg1UnknownError2: (arg: string) => Promise<number> = promisify((arg: string, cb: (err: Error | null, result: number) => void): void => { });
const arg1NoResult: (arg: string) => Promise<any> = promisify((arg: string, cb: (err: Error | null) => void): void => { });
const cbOptionalError: () => Promise<void | {}> = promisify((cb: (err?: Error | null) => void): void => { cb(); }); // tslint:disable-line void-return

declare function promisify<TResult>(fn: (callback: (err: any, result: TResult) => void) => void): () => Promise<TResult>;
declare function promisify(fn: (callback: (err?: any) => void) => void): () => Promise<void>;
declare function promisify<T1, TResult>(fn: (arg1: T1, callback: (err: any, result: TResult) => void) => void): (arg1: T1) => Promise<TResult>;
declare function promisify<T1>(fn: (arg1: T1, callback: (err?: any) => void) => void): (arg1: T1) => Promise<void>;
declare function promisify<T1, T2, TResult>(fn: (arg1: T1, arg2: T2, callback: (err: any, result: TResult) => void) => void): (arg1: T1, arg2: T2) => Promise<TResult>;
declare function promisify<T1, T2>(fn: (arg1: T1, arg2: T2, callback: (err?: any) => void) => void): (arg1: T1, arg2: T2) => Promise<void>;

I could imagine trying a migration to rest parameter types, but I'd rather not do that for this change.

[DanielRosenwasser]

@tchetwin @robpalme you mentioned that this was something you managed to get around - do you remember what you specifically did?

[tchetwin]

@tchetwin @robpalme you mentioned that this was something you managed to get around - do you remember what you specifically did?

By as ... casting the argument to promisify to force selection of the correct overload.

This occurred in precisely one place and it happened to be in the shared build tooling itself rather than the application population.

[AlCalzone]

I could imagine trying a migration to rest parameter types, but I'd rather not do that for this change.

How do you like this?

/**
 * Returns the last item's type in a tuple
 */
export type Last<T extends unknown[]> = T extends []
	? never
	: T extends [...infer _, infer R] ? R
	: T extends [...infer _, (infer R)?] ? (R | undefined)
	: never;

/** Returns the type of the last argument of a function */
export type LastArgument<T extends (...args: any[]) => any> = Last<Parameters<T>>;

/** Returns the "return" type of a callback-style API */
export type CallbackAPIReturnType<
	T extends (...args: any[]) => any,
	TCb extends (...args: any[]) => any = LastArgument<T>,
	TCbArgs = Parameters<Exclude<TCb, undefined>>,
	> =
	TCbArgs extends [(Error | null | undefined)?] ? void :
	TCbArgs extends [Error | null | undefined, infer U] ? U :
	TCbArgs extends any[] ? TCbArgs[1] :
	never;

/**
 * Returns a promisified function signature for the given callback-style function.
 */
export type Promisify<T extends (...args: any[]) => any,
	TReturn = CallbackAPIReturnType<T>,
	TArgs extends any[] = Lead<Parameters<T>>,
	> = (...args: TArgs) => Promise<TReturn>;

/**
 * Returns all but the last item's type in a tuple/array
 */
export type Lead<T extends unknown[]> = T extends []
	? []
	: T extends [...infer L, any?] ? L
	: [];

declare function promisify<T extends (...args: any[]) => any>(fn: T): Promisify<T>;

results for your test cases:

declare function parseString(str: any, cb?: (...args: any[]) => any): void;

const parseStringP = promisify(parseString);
let x = parseStringP("abc");
// has type  (str: any) => Promise<any>

// expected: (arg: string) => Promise<number>
// actual:   (arg: string) => Promise<number> ✔
const arg1UnknownError = promisify((arg: string, cb: (err: unknown, result: number) => void): void => { });

// expected: (arg: string) => Promise<number>
// actual:   (arg: string) => Promise<number> ✔
const arg1AnyError = promisify((arg: string, cb: (err: any, result: number) => void): void => { });

// expected: () => Promise<number>
// actual:   () => Promise<number> ✔
const arg0 = promisify((cb: (err: Error | null, result: number) => void): void => { });

// expected: () => Promise<any>
// actual:   () => Promise<void> --> I'd argue this is correct
const arg0NoResult = promisify((cb: (err: Error | null) => void): void => { });

// expected: (arg: string) => Promise<number>
// actual:   (arg: string) => Promise<number> ✔
const arg1 = promisify((arg: string, cb: (err: Error | null, result: number) => void): void => { });

// expected: (arg: string) => Promise<number>
// actual:   (arg: string) => Promise<number> ✔
const arg1UnknownError2 = promisify((arg: string, cb: (err: Error | null, result: number) => void): void => { });

// expected: (arg: string) => Promise<any>
// actual:   (arg: string) => Promise<void> --> I'd argue this is correct
const arg1NoResult = promisify((arg: string, cb: (err: Error | null) => void): void => { });

// expected: () => Promise<void | {}>
// actual:   () => Promise<void> --> where should the {} come from?
const cbOptionalError = promisify((cb: (err?: Error | null) => void): void => { cb(); }); // tslint:disable-line void-return

[DanielRosenwasser]

Well those seem way better. I don't know how I feel about those helper types, but if you wanted to put up your own PR to @types/node that might be a worthwhile way to gauge any other breaks.

[clonardo]

is there a way to switch back to the old behavior via a compilerOptions flag or something? TypeScript 4.1.2 is now shown on npm as a stable release, and this is a breaking change in my application.

[DanielRosenwasser]

It is a stable release, and we make breaks across minor versions. 4.1.2 is the first stable minor version for 4.1.x.

I don't know whether we realistically can add a flag. What kind of things did you hit in the upgrade?

[clonardo]

It is a stable release, and we make breaks across minor versions. 4.1.2 is the first stable minor version for 4.1.x.

I don't know whether we realistically can add a flag. What kind of things did you hit in the upgrade?

This app had a number of places where it was creating a new Promise() via the constructor and resolving it with resolve(). I had to change the promise constructors to new Promise<void>. The app was being built with noImplicitAny:false and noImplicitReturns:false . What made life a little more difficult is that VSCode's TypeScript language service (in the current stable release) wasn't letting me select 4.1.x for pre-build checking, so I had to go back to a find + replace + fix, transpile, wait and cross fingers-type workflow. After doing this work to update a pretty big monorepo, it's fine now, I think.

[DanielRosenwasser]

Sorry to hear that - that actually sounds more like a separate known break, relating to the issue of how resolve is typed.

VSCode's TypeScript language service (in the current stable release) wasn't letting me select 4.1.x for pre-build checking

Hmm - you should be able to select a workspace version of TypeScript. I suspect if that doesn't work, the issues there might relate to the monorepo setup.

Again, sorry that this hit you in a surprising way.

[clonardo]

Sorry to hear that - that actually sounds more like a separate known break, relating to the issue of how resolve is typed.

VSCode's TypeScript language service (in the current stable release) wasn't letting me select 4.1.x for pre-build checking

Hmm - you should be able to select a workspace version of TypeScript. I suspect if that doesn't work, the issues there might relate to the monorepo setup.

Again, sorry that this hit you in a surprising way.

Thanks, I didn't realize about the change to resolve typings.

To check the VSCode issue, I brought up another (non-mono) repo and installed typescript@latest. Still no good, even after closing + reopening VSCode, and restarting the TypeScript language service. I'm on VSCode 1.51.1 on Windows 10. Now that I know where the risks are, I'm not too worried about this.

[yw662]

Can confirm in 4.1.2 where I have to explicitly write like new Promise<void>(res=>res()) instead of just new Promise(res=>res()) where former version of typescript infers a Promise<unknown>.

[yw662]

VSCode's TypeScript language service (in the current stable release) wasn't letting me select 4.1.x for pre-build checking,

But you can change the typescript version VSCode use. Just click the ts version in bottom right corner and vscode will write your option in ./.vscode/sth.

[tchetwin]

@yw662, as per Daniel above:

that actually sounds more like a separate known break, relating to the issue of how resolve is typed.

The need to introduce an explicit void type parameter for Promise constructors resolving with no arguments is unrelated to this Issue thread.

[AlCalzone]

Well those seem way better. I don't know how I feel about those helper types, but if you wanted to put up your own PR to @types/node that might be a worthwhile way to gauge any other breaks.

PR is up at DefinitelyTyped/DefinitelyTyped#49699 and seems to be working. What needs to be done to gauge the impact of this? @DanielRosenwasser