Skip to content
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

Fix Callback argument type not inferred for union of interfaces #59309 #59672

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+669 −20
Open

Fix Callback argument type not inferred for union of interfaces #59309 #59672

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
25b5914
contextual type
MichalMarsalek Aug 18, 2024
effe3a0
Merge branch 'main' into contextual-type
MichalMarsalek Aug 18, 2024
ed2bfa4
fix getContextualSignature
MichalMarsalek Aug 18, 2024
2e530af
format
MichalMarsalek Aug 18, 2024
ac64f64
simplify getContextualSignature
MichalMarsalek Aug 19, 2024
6eaf5e3
sort signatures by getMinArgumentCount
MichalMarsalek Aug 19, 2024
f69d6e1
use test file options
MichalMarsalek Aug 19, 2024
8b0b855
special case async function
MichalMarsalek Aug 19, 2024
8a3b095
fix logic for async functions
MichalMarsalek Aug 19, 2024
dcb8f5d
special case generators
MichalMarsalek Aug 19, 2024
4fd6bbd
update tests
MichalMarsalek Aug 19, 2024
381e4e0
update tests
MichalMarsalek Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32423,22 +32423,23 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return getContextualCallSignature(type, node);
}
let signatureList: Signature[] | undefined;
const types = (type as UnionType).types;
const types = (type as UnionType).types
.map(type => ({ type, signature: getContextualCallSignature(type, node) }))
MichalMarsalek marked this conversation as resolved.
Show resolved Hide resolved
.filter(type => type.signature)
MichalMarsalek marked this conversation as resolved.
Show resolved Hide resolved
MichalMarsalek marked this conversation as resolved.
Show resolved Hide resolved
MichalMarsalek marked this conversation as resolved.
Show resolved Hide resolved
.sort((t1, t2) => t1.signature!.parameters.length - t2.signature!.parameters.length);
Copy link
Contributor

@Andarist Andarist Aug 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to account for rest parameters here so it would be more appropriate to use getParameterCount here over directly checking the .parameters.length. Please also consider cases like

((a: string, b?: string) => void) | ((a: string, ...rest: string[]) => void)

Both of those signatures have the same parameter count but perhaps you'd like to specifically sort one of them as the first one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I think that actually what I need is getMinArgumentCount.
For the case ((a: string, b?: string) => void) | ((a: string, ...rest: string[]) => void) I don't think it matters which one is first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it will make any difference but to exercise even more funky test cases you could add test cases with signatures of this shape:

((a: string, ...rest: [string, number?] | [number, boolean?]) => void) | ((a: string, ...rest: [string, number] | [number, boolean]) => void)

Note that I didn't think through what types you should use in those rest parameters, just that they can be unions of tuples and that one of the signatures might have some of the elements there required and some might have them optional etc.

for (const current of types) {
const signature = getContextualCallSignature(current, node);
if (signature) {
if (!signatureList) {
// This signature will contribute to contextual union signature
signatureList = [signature];
}
else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ false, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesIdentical)) {
// Signatures aren't identical, do not use
return undefined;
}
else {
// Use this signature for contextual union signature
signatureList.push(signature);
}
const signature = current.signature!;
if (!signatureList) {
// This signature will contribute to contextual union signature
signatureList = [signature];
}
else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ true, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesSubtypeOf)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are loosening here how parameter types are meant to be compared I'd probably expect here:

Suggested change
else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ true, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesSubtypeOf)) {
else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ true, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesAssignable)) {

Unless there is already a reason why the subtype relationship was used here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure when each relation should be used. I used compareTypesSubtypeOf because that's what is used in a call to compareSignaturesIdentical with partialMatch === true in the function findMatchingSignatures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findMatchingSignatures gets only called by getUnionSignatures so I agree that this probably makes sense

// Signatures aren't identical, do not use
return undefined;
}
else {
// Use this signature for contextual union signature
signatureList.push(signature);
}
}
// Result is union of signatures collected (return type is union of return types of this signature set)
Expand Down
87 changes: 87 additions & 0 deletions tests/baselines/reference/contextualSignatureWithExtraParameters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
//// [tests/cases/compiler/contextualSignatureWithExtraParameters.ts] ////

//// [contextualSignatureWithExtraParameters.ts]
// https://github.com/microsoft/TypeScript/issues/59309
function f1(
cb: ((item: number) => void) | ((item: number, extra: string) => void),
) {}

f1((item) => {});

function f2<T>(
arr: T[],
cb: ((item: T) => void) | ((item: T, extra: unknown) => void),
) {}

f2([1, 2, 3], (item) => {});

export interface AsyncResultCallback<T, E = Error> {
(err?: E | null, result?: T): void;
}

export interface AsyncResultIterator<T, R, E = Error> {
(item: T, callback: AsyncResultCallback<R, E>): void;
}
export interface AsyncResultIteratorPromise<T, R> {
(item: T): Promise<R>;
}

declare function mapLimit<T, R, E = Error>(
arr: T[],
limit: number,
iterator: AsyncResultIteratorPromise<T, R> | AsyncResultIterator<T, R, E>,
): Promise<R[]>;

mapLimit([1,2,3], 3, async (n) => {
return n ** 2;
});

//// [contextualSignatureWithExtraParameters.js]
"use strict";
var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
return new (P || (P = Promise))(function (resolve, reject) {
function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); }
step((generator = generator.apply(thisArg, _arguments || [])).next());
});
};
var __generator = (this && this.__generator) || function (thisArg, body) {
var _ = { label: 0, sent: function() { if (t[0] & 1) throw t[1]; return t[1]; }, trys: [], ops: [] }, f, y, t, g = Object.create((typeof Iterator === "function" ? Iterator : Object).prototype);
return g.next = verb(0), g["throw"] = verb(1), g["return"] = verb(2), typeof Symbol === "function" && (g[Symbol.iterator] = function() { return this; }), g;
function verb(n) { return function (v) { return step([n, v]); }; }
function step(op) {
if (f) throw new TypeError("Generator is already executing.");
while (g && (g = 0, op[0] && (_ = 0)), _) try {
if (f = 1, y && (t = op[0] & 2 ? y["return"] : op[0] ? y["throw"] || ((t = y["return"]) && t.call(y), 0) : y.next) && !(t = t.call(y, op[1])).done) return t;
if (y = 0, t) op = [op[0] & 2, t.value];
switch (op[0]) {
case 0: case 1: t = op; break;
case 4: _.label++; return { value: op[1], done: false };
case 5: _.label++; y = op[1]; op = [0]; continue;
case 7: op = _.ops.pop(); _.trys.pop(); continue;
default:
if (!(t = _.trys, t = t.length > 0 && t[t.length - 1]) && (op[0] === 6 || op[0] === 2)) { _ = 0; continue; }
if (op[0] === 3 && (!t || (op[1] > t[0] && op[1] < t[3]))) { _.label = op[1]; break; }
if (op[0] === 6 && _.label < t[1]) { _.label = t[1]; t = op; break; }
if (t && _.label < t[2]) { _.label = t[2]; _.ops.push(op); break; }
if (t[2]) _.ops.pop();
_.trys.pop(); continue;
}
op = body.call(thisArg, _);
} catch (e) { op = [6, e]; y = 0; } finally { f = t = 0; }
if (op[0] & 5) throw op[1]; return { value: op[0] ? op[1] : void 0, done: true };
}
};
Object.defineProperty(exports, "__esModule", { value: true });
// https://github.com/microsoft/TypeScript/issues/59309
function f1(cb) { }
f1(function (item) { });
function f2(arr, cb) { }
f2([1, 2, 3], function (item) { });
mapLimit([1, 2, 3], 3, function (n) { return __awaiter(void 0, void 0, void 0, function () {
return __generator(this, function (_a) {
return [2 /*return*/, Math.pow(n, 2)];
});
}); });
117 changes: 117 additions & 0 deletions tests/baselines/reference/contextualSignatureWithExtraParameters.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
//// [tests/cases/compiler/contextualSignatureWithExtraParameters.ts] ////

=== contextualSignatureWithExtraParameters.ts ===
// https://github.com/microsoft/TypeScript/issues/59309
function f1(
>f1 : Symbol(f1, Decl(contextualSignatureWithExtraParameters.ts, 0, 0))

cb: ((item: number) => void) | ((item: number, extra: string) => void),
>cb : Symbol(cb, Decl(contextualSignatureWithExtraParameters.ts, 1, 12))
>item : Symbol(item, Decl(contextualSignatureWithExtraParameters.ts, 2, 10))
>item : Symbol(item, Decl(contextualSignatureWithExtraParameters.ts, 2, 37))
>extra : Symbol(extra, Decl(contextualSignatureWithExtraParameters.ts, 2, 50))

) {}

f1((item) => {});
>f1 : Symbol(f1, Decl(contextualSignatureWithExtraParameters.ts, 0, 0))
>item : Symbol(item, Decl(contextualSignatureWithExtraParameters.ts, 5, 6))

function f2<T>(
>f2 : Symbol(f2, Decl(contextualSignatureWithExtraParameters.ts, 5, 19))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 7, 12))

arr: T[],
>arr : Symbol(arr, Decl(contextualSignatureWithExtraParameters.ts, 7, 15))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 7, 12))

cb: ((item: T) => void) | ((item: T, extra: unknown) => void),
>cb : Symbol(cb, Decl(contextualSignatureWithExtraParameters.ts, 8, 13))
>item : Symbol(item, Decl(contextualSignatureWithExtraParameters.ts, 9, 10))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 7, 12))
>item : Symbol(item, Decl(contextualSignatureWithExtraParameters.ts, 9, 32))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 7, 12))
>extra : Symbol(extra, Decl(contextualSignatureWithExtraParameters.ts, 9, 40))

) {}

f2([1, 2, 3], (item) => {});
>f2 : Symbol(f2, Decl(contextualSignatureWithExtraParameters.ts, 5, 19))
>item : Symbol(item, Decl(contextualSignatureWithExtraParameters.ts, 12, 15))

export interface AsyncResultCallback<T, E = Error> {
>AsyncResultCallback : Symbol(AsyncResultCallback, Decl(contextualSignatureWithExtraParameters.ts, 12, 28))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 14, 37))
>E : Symbol(E, Decl(contextualSignatureWithExtraParameters.ts, 14, 39))
>Error : Symbol(Error, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))

(err?: E | null, result?: T): void;
>err : Symbol(err, Decl(contextualSignatureWithExtraParameters.ts, 15, 5))
>E : Symbol(E, Decl(contextualSignatureWithExtraParameters.ts, 14, 39))
>result : Symbol(result, Decl(contextualSignatureWithExtraParameters.ts, 15, 20))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 14, 37))
}

export interface AsyncResultIterator<T, R, E = Error> {
>AsyncResultIterator : Symbol(AsyncResultIterator, Decl(contextualSignatureWithExtraParameters.ts, 16, 1))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 18, 37))
>R : Symbol(R, Decl(contextualSignatureWithExtraParameters.ts, 18, 39))
>E : Symbol(E, Decl(contextualSignatureWithExtraParameters.ts, 18, 42))
>Error : Symbol(Error, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))

(item: T, callback: AsyncResultCallback<R, E>): void;
>item : Symbol(item, Decl(contextualSignatureWithExtraParameters.ts, 19, 5))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 18, 37))
>callback : Symbol(callback, Decl(contextualSignatureWithExtraParameters.ts, 19, 13))
>AsyncResultCallback : Symbol(AsyncResultCallback, Decl(contextualSignatureWithExtraParameters.ts, 12, 28))
>R : Symbol(R, Decl(contextualSignatureWithExtraParameters.ts, 18, 39))
>E : Symbol(E, Decl(contextualSignatureWithExtraParameters.ts, 18, 42))
}
export interface AsyncResultIteratorPromise<T, R> {
>AsyncResultIteratorPromise : Symbol(AsyncResultIteratorPromise, Decl(contextualSignatureWithExtraParameters.ts, 20, 1))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 21, 44))
>R : Symbol(R, Decl(contextualSignatureWithExtraParameters.ts, 21, 46))

(item: T): Promise<R>;
>item : Symbol(item, Decl(contextualSignatureWithExtraParameters.ts, 22, 5))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 21, 44))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --))
>R : Symbol(R, Decl(contextualSignatureWithExtraParameters.ts, 21, 46))
}

declare function mapLimit<T, R, E = Error>(
>mapLimit : Symbol(mapLimit, Decl(contextualSignatureWithExtraParameters.ts, 23, 1))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 25, 26))
>R : Symbol(R, Decl(contextualSignatureWithExtraParameters.ts, 25, 28))
>E : Symbol(E, Decl(contextualSignatureWithExtraParameters.ts, 25, 31))
>Error : Symbol(Error, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))

arr: T[],
>arr : Symbol(arr, Decl(contextualSignatureWithExtraParameters.ts, 25, 43))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 25, 26))

limit: number,
>limit : Symbol(limit, Decl(contextualSignatureWithExtraParameters.ts, 26, 13))

iterator: AsyncResultIteratorPromise<T, R> | AsyncResultIterator<T, R, E>,
>iterator : Symbol(iterator, Decl(contextualSignatureWithExtraParameters.ts, 27, 18))
>AsyncResultIteratorPromise : Symbol(AsyncResultIteratorPromise, Decl(contextualSignatureWithExtraParameters.ts, 20, 1))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 25, 26))
>R : Symbol(R, Decl(contextualSignatureWithExtraParameters.ts, 25, 28))
>AsyncResultIterator : Symbol(AsyncResultIterator, Decl(contextualSignatureWithExtraParameters.ts, 16, 1))
>T : Symbol(T, Decl(contextualSignatureWithExtraParameters.ts, 25, 26))
>R : Symbol(R, Decl(contextualSignatureWithExtraParameters.ts, 25, 28))
>E : Symbol(E, Decl(contextualSignatureWithExtraParameters.ts, 25, 31))

): Promise<R[]>;
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --))
>R : Symbol(R, Decl(contextualSignatureWithExtraParameters.ts, 25, 28))

mapLimit([1,2,3], 3, async (n) => {
>mapLimit : Symbol(mapLimit, Decl(contextualSignatureWithExtraParameters.ts, 23, 1))
>n : Symbol(n, Decl(contextualSignatureWithExtraParameters.ts, 31, 28))

return n ** 2;
>n : Symbol(n, Decl(contextualSignatureWithExtraParameters.ts, 31, 28))

});
136 changes: 136 additions & 0 deletions tests/baselines/reference/contextualSignatureWithExtraParameters.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
//// [tests/cases/compiler/contextualSignatureWithExtraParameters.ts] ////

=== contextualSignatureWithExtraParameters.ts ===
// https://github.com/microsoft/TypeScript/issues/59309
function f1(
>f1 : (cb: ((item: number) => void) | ((item: number, extra: string) => void)) => void
> : ^ ^^ ^^^^^^^^^

cb: ((item: number) => void) | ((item: number, extra: string) => void),
>cb : ((item: number) => void) | ((item: number, extra: string) => void)
> : ^^ ^^ ^^^^^ ^^^^^^ ^^ ^^ ^^ ^^^^^ ^
>item : number
> : ^^^^^^
>item : number
> : ^^^^^^
>extra : string
> : ^^^^^^

) {}

f1((item) => {});
>f1((item) => {}) : void
> : ^^^^
>f1 : (cb: ((item: number) => void) | ((item: number, extra: string) => void)) => void
> : ^ ^^ ^^^^^^^^^
>(item) => {} : (item: number) => void
> : ^ ^^^^^^^^^^^^^^^^^
>item : number
> : ^^^^^^

function f2<T>(
>f2 : <T>(arr: T[], cb: ((item: T) => void) | ((item: T, extra: unknown) => void)) => void
> : ^ ^^ ^^ ^^ ^^ ^^^^^^^^^

arr: T[],
>arr : T[]
> : ^^^

cb: ((item: T) => void) | ((item: T, extra: unknown) => void),
>cb : ((item: T) => void) | ((item: T, extra: unknown) => void)
> : ^^ ^^ ^^^^^ ^^^^^^ ^^ ^^ ^^ ^^^^^ ^
>item : T
> : ^
>item : T
> : ^
>extra : unknown
> : ^^^^^^^

) {}

f2([1, 2, 3], (item) => {});
>f2([1, 2, 3], (item) => {}) : void
> : ^^^^
>f2 : <T>(arr: T[], cb: ((item: T) => void) | ((item: T, extra: unknown) => void)) => void
> : ^ ^^ ^^ ^^ ^^ ^^^^^^^^^
>[1, 2, 3] : number[]
> : ^^^^^^^^
>1 : 1
> : ^
>2 : 2
> : ^
>3 : 3
> : ^
>(item) => {} : (item: number) => void
> : ^ ^^^^^^^^^^^^^^^^^
>item : number
> : ^^^^^^

export interface AsyncResultCallback<T, E = Error> {
(err?: E | null, result?: T): void;
>err : E
> : ^
>result : T
> : ^
}

export interface AsyncResultIterator<T, R, E = Error> {
(item: T, callback: AsyncResultCallback<R, E>): void;
>item : T
> : ^
>callback : AsyncResultCallback<R, E>
> : ^^^^^^^^^^^^^^^^^^^^^^^^^
}
export interface AsyncResultIteratorPromise<T, R> {
(item: T): Promise<R>;
>item : T
> : ^
}

declare function mapLimit<T, R, E = Error>(
>mapLimit : <T, R, E = Error>(arr: T[], limit: number, iterator: AsyncResultIteratorPromise<T, R> | AsyncResultIterator<T, R, E>) => Promise<R[]>
> : ^ ^^ ^^ ^^^^^^^^^^ ^^ ^^ ^^ ^^ ^^ ^^^^^

arr: T[],
>arr : T[]
> : ^^^

limit: number,
>limit : number
> : ^^^^^^

iterator: AsyncResultIteratorPromise<T, R> | AsyncResultIterator<T, R, E>,
>iterator : AsyncResultIteratorPromise<T, R> | AsyncResultIterator<T, R, E>
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

): Promise<R[]>;

mapLimit([1,2,3], 3, async (n) => {
>mapLimit([1,2,3], 3, async (n) => { return n ** 2;}) : Promise<number[]>
> : ^^^^^^^^^^^^^^^^^
>mapLimit : <T, R, E = Error>(arr: T[], limit: number, iterator: AsyncResultIteratorPromise<T, R> | AsyncResultIterator<T, R, E>) => Promise<R[]>
> : ^ ^^ ^^ ^^^^^^^^^^ ^^ ^^ ^^ ^^ ^^ ^^^^^
>[1,2,3] : number[]
> : ^^^^^^^^
>1 : 1
> : ^
>2 : 2
> : ^
>3 : 3
> : ^
>3 : 3
> : ^
>async (n) => { return n ** 2;} : (n: number) => Promise<number>
> : ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>n : number
> : ^^^^^^

return n ** 2;
>n ** 2 : number
> : ^^^^^^
>n : number
> : ^^^^^^
>2 : 2
> : ^

});
Loading