Skip to content

Improve signature assignability for argument lists of different lengths #49218

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Improve signature assignability for argument lists of different lengths #49218

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1819397
Fixed an issue with shorter param list not being assignable to rest p…
Andarist May 23, 2022
67892c3
Merge branch 'main' into fix/rest-tuple-union-shorter-contextual-params
Andarist Sep 25, 2022
bd712ab
Merge remote-tracking branch 'origin/main' into fix/rest-tuple-union-…
Andarist Dec 28, 2022
88027d5
Fixed the contextual params assignability with target params declared…
Andarist Dec 28, 2022
6c5383d
Fill the shorter target tuple with undefined when comparing signatures
Andarist Dec 28, 2022
46f2e57
Add an additional test case for mixed length tuples in the target's rest
Andarist Dec 28, 2022
0de8923
Add tests for mixed-length tuples used as rest
Andarist Dec 28, 2022
beb241d
add tests from #45972
Andarist Dec 28, 2022
388ec89
Allow rest in source
Andarist Dec 29, 2022
8753248
Merge remote-tracking branch 'origin/main' into fix/rest-tuple-union-…
Andarist Jan 15, 2023
e357450
Fixed cases involving generics and add comments
Andarist Jan 16, 2023
29979e2
Merge remote-tracking branch 'origin/main' into fix/rest-tuple-union-…
Andarist Jun 13, 2023
fd52884
Merge remote-tracking branch 'origin/main' into fix/rest-tuple-union-…
Andarist Jul 10, 2023
336288a
fix extra cases
Andarist Jul 10, 2023
92d0267
Fixed tupel structure matching in the signature-related codepath
Andarist Jul 10, 2023
1c24035
use conditional undefined instead of any
Andarist Jul 10, 2023
608f7c3
Merge remote-tracking branch 'origin/main' into fix/rest-tuple-union-…
Andarist Jan 6, 2024
61d2612
Merge remote-tracking branch 'origin/main' into fix/rest-tuple-union-…
Andarist Feb 19, 2024
919fc0e
add extra test case
Andarist Feb 19, 2024
b920e9a
Merge branch 'main' into pr-49218
jakebailey Apr 19, 2024
a8ddfc3
Update baselines
jakebailey Apr 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
24 changes: 23 additions & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19381,7 +19381,29 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

for (let i = 0; i < paramCount; i++) {
const sourceType = i === restIndex ? getRestTypeAtPosition(source, i) : tryGetTypeAtPosition(source, i);
const targetType = i === restIndex ? getRestTypeAtPosition(target, i) : tryGetTypeAtPosition(target, i);
let targetType = i === restIndex ? getRestTypeAtPosition(target, i) : tryGetTypeAtPosition(target, i);
if (i === restIndex && targetType && sourceType && isTupleType(sourceType) && !sourceType.target.hasRestElement) {
Copy link
Member

@gabritto gabritto Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You're intentionally not fixing the case where sourceType is a union, right? I think that's ok because I think the code would become a lot more complicated, but I wanted to ask.
Same for when targetType is an array I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're intentionally not fixing the case where sourceType is a union, right? I think that's ok because I think the code would become a lot more complicated, but I wanted to ask.

Yeeah. It's likely that this could be fixed by wrapping the body of this loop, executing it for each union member, and checking if any of the iterations return ~ Ternary.False. Maybe that could be explored in a follow-up PR - but for the time being, I think that it's best to improve this incrementally. It's way easier to follow this diff here than it would be to follow it if I would try to handle union sources as well.

Same for when targetType is an array I guess.

Hmm, I think this is actually already covered - it's just that for this case we don't need to do anything special, so we forward the type so it can be checked by the latter logic as-is. Maybe I'm missing something though - do you have any particular test case in mind? The main purpose behind the added code is to "adjust" the target tuple types - their lengths etc, as without that we simply fail on required elements checks etc (but when comparing signatures we can actually often ignore extra positions that are not utilized by the source signature).

targetType = mapType(targetType, t => {
if (!isTupleType(t)) {
return t;
}

const typeArguments = getTypeArguments(t);
const elementTypes: Type[] = [];

for (let i = 0; i < getTypeReferenceArity(sourceType); i++) {
elementTypes.push(
i < typeArguments.length
Copy link
Member

Choose a reason for hiding this comment

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

Why are using sourceType's element flags when we're pushing the target type elements here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an excellent question. At first, I wasn't sure if this is correct - it has been a while since I worked on this. I rechecked that and this is intentional. The implied source tuple type, which represents parameters of the source signature, might only have fixed elements - while the target tuple type might be declared using rest. For example:

const f1: (...args: [number, string, ...boolean[]] | [string, number, ...boolean[]]) => void = (a, b, c) => {};

Here, we want to compare those contravariant tuples:

type Target = [number, string, boolean] | [string, number, boolean]
type Source = [a: string | number, b: string | number, c: boolean]

However, at the moment - I'm not entirely sure if all "transitions" that can happen here are OK. I need to analyze this again and think if it's always OK to, for example, create an optional element from some required elements.

Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to review this again, and I'm trying to think if using the source tuple type's element flags to construct the new target tuple types is ok. I was trying to think through the case where the source element is optional, so the new target element will be too. I think this makes sense and is OK to do when we're comparing function signatures strictly, i.e. when we'll compare parameter types contravariantly. However, if we're comparing method signatures, we'll end up comparing the parameter types also covariantly, and in that case I don't know if it makes sense for the new target type element to be marked as optional for that comparison.

A perhaps convoluted example I came up with to try and show this:

// Counter-example to making target element optional when source element is optional?
type NowSource<T extends boolean[]> = [...rest: [boolean, ...T, n?: number]];
type NowTarget<T extends boolean[]> = [...rest: [boolean, ...T, number | string]];

class A {
  method<T extends boolean[]>(...args: NowTarget<T>): void {}
}

class B extends A {
  method<T extends boolean[]>(...args: NowSource<T>): void {} // Errors on main, no error on PR
}

declare let func: <T extends boolean[]>(...args: NowTarget<T>) => void;
declare let gunc: <T extends boolean[]>(...args: NowSource<T>) => void;

func = gunc; // Errors on main and PR, ok

? t.target.elementFlags[i] & ElementFlags.Required
? typeArguments[i]
: getElementTypeOfSliceOfTupleType(t, i)!
: undefinedType
);
}

return createTupleType(elementTypes, elementTypes.map(() => ElementFlags.Required));
});
}
if (sourceType && targetType) {
// In order to ensure that any generic type Foo<T> is at least co-variant with respect to T no matter
// how Foo uses T, we need to relate parameters bi-variantly (given that parameters are input positions,
Expand Down
26 changes: 14 additions & 12 deletions tests/baselines/reference/genericRestParameters3.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@ tests/cases/conformance/types/rest/genericRestParameters3.ts(18,1): error TS2345
Source has 0 element(s) but target requires 2.
tests/cases/conformance/types/rest/genericRestParameters3.ts(23,1): error TS2322: Type '(x: string, y: string) => void' is not assignable to type '(x: string, ...args: [string] | [number, boolean]) => void'.
Types of parameters 'y' and 'args' are incompatible.
Type '[string] | [number, boolean]' is not assignable to type '[y: string]'.
Type '[number, boolean]' is not assignable to type '[y: string]'.
Source has 2 element(s) but target allows only 1.
Type '[string] | [number]' is not assignable to type '[y: string]'.
Type '[number]' is not assignable to type '[y: string]'.
Type 'number' is not assignable to type 'string'.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To the best of my understanding, this change is correct and welcome under the implemented changes.

tests/cases/conformance/types/rest/genericRestParameters3.ts(24,1): error TS2322: Type '(x: string, y: number, z: boolean) => void' is not assignable to type '(x: string, ...args: [string] | [number, boolean]) => void'.
Types of parameters 'y' and 'args' are incompatible.
Type '[string] | [number, boolean]' is not assignable to type '[y: number, z: boolean]'.
Type '[string]' is not assignable to type '[y: number, z: boolean]'.
Source has 1 element(s) but target requires 2.
Type '[number, boolean] | [string, undefined]' is not assignable to type '[y: number, z: boolean]'.
Type '[string, undefined]' is not assignable to type '[y: number, z: boolean]'.
Type at position 0 in source is not compatible with type at position 0 in target.
Type 'string' is not assignable to type 'number'.
tests/cases/conformance/types/rest/genericRestParameters3.ts(35,1): error TS2554: Expected 1 arguments, but got 0.
tests/cases/conformance/types/rest/genericRestParameters3.ts(36,21): error TS2345: Argument of type 'number' is not assignable to parameter of type '(...args: CoolArray<any>) => void'.
tests/cases/conformance/types/rest/genericRestParameters3.ts(37,21): error TS2345: Argument of type '<T extends any[]>(cb: (...args: T) => void) => void' is not assignable to parameter of type '(...args: CoolArray<any>) => void'.
Expand Down Expand Up @@ -69,16 +70,17 @@ tests/cases/conformance/types/rest/genericRestParameters3.ts(59,5): error TS2345
~~
!!! error TS2322: Type '(x: string, y: string) => void' is not assignable to type '(x: string, ...args: [string] | [number, boolean]) => void'.
!!! error TS2322: Types of parameters 'y' and 'args' are incompatible.
!!! error TS2322: Type '[string] | [number, boolean]' is not assignable to type '[y: string]'.
!!! error TS2322: Type '[number, boolean]' is not assignable to type '[y: string]'.
!!! error TS2322: Source has 2 element(s) but target allows only 1.
!!! error TS2322: Type '[string] | [number]' is not assignable to type '[y: string]'.
!!! error TS2322: Type '[number]' is not assignable to type '[y: string]'.
!!! error TS2322: Type 'number' is not assignable to type 'string'.
Comment on lines +73 to +75
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a change of the error for this case:

declare let f1: (x: string, ...args: [string] | [number, boolean]) => void;
declare let f2: (x: string, y: string) => void;

f1 = f2

With the current changes in this PR, I create a slice from the target tuple and I cap it to the source's length~. I do that for similar reasons that I've outlined in the other comment. The source signature can freely ignore the "extra" arguments so we don't even need to check them here (we'd have to ignore them through some other mechanism anyway)

f1 = f3; // Error
~~
!!! error TS2322: Type '(x: string, y: number, z: boolean) => void' is not assignable to type '(x: string, ...args: [string] | [number, boolean]) => void'.
!!! error TS2322: Types of parameters 'y' and 'args' are incompatible.
!!! error TS2322: Type '[string] | [number, boolean]' is not assignable to type '[y: number, z: boolean]'.
!!! error TS2322: Type '[string]' is not assignable to type '[y: number, z: boolean]'.
!!! error TS2322: Source has 1 element(s) but target requires 2.
!!! error TS2322: Type '[number, boolean] | [string, undefined]' is not assignable to type '[y: number, z: boolean]'.
!!! error TS2322: Type '[string, undefined]' is not assignable to type '[y: number, z: boolean]'.
!!! error TS2322: Type at position 0 in source is not compatible with type at position 0 in target.
!!! error TS2322: Type 'string' is not assignable to type 'number'.
Comment on lines +80 to +83
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error change comes from the fact that I fill the slice of the target type with undefineds here (when it's shorter than the source's slice):
https://github.dev/microsoft/TypeScript/blob/6c5383d701417e8a98398b4171af0f201ee593c5/src/compiler/checker.ts#L19394-L19402

I think this is a good change. This will allow more patterns to be assignable, especially with target signatures using unions of tuples for their rest parameter. After all, the implementation is free to ignore any of the provided arguments etc. This is also already just fine in TS:

declare let f1: (a: string) => void
declare let f2: (a: string, b: number) => void

f2 = f1

And since this is OK I think that this one should be too:

declare let f1: (x: string, ...args: [string] | [number, boolean]) => void;

// Type '(a: string, b: string | number, c: boolean | undefined) => void' is not assignable to type '(x: string, ...args: [string] | [number, boolean]) => void'.
//   Types of parameters 'b' and 'args' are incompatible.
//     Type '[string] | [number, boolean]' is not assignable to type '[b: string | number, c: boolean | undefined]'.
//       Type '[string]' is not assignable to type '[b: string | number, c: boolean | undefined]'.
//         Source has 1 element(s) but target requires 2.
f1 = (a, b, c) => {}

So with this change that caused this particular error here to be reported differently we now allow the example above and I added a test case for it:

TypeScript/tests/cases/compiler/restTupleUnionWithRestContextualParams.ts

Line 6 in 46f2e57

const f2: (x: string, ...args: [string] | [number, boolean]) => void = (a, b, c) => {};

f1 = f4;

// Repro from #26110
Expand Down
31 changes: 31 additions & 0 deletions tests/baselines/reference/restTupleUnionShorterContextualParams.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
=== tests/cases/compiler/restTupleUnionShorterContextualParams.ts ===
// repro #48663

// showcase how those transitive assignments are OK
const f1: (x: string | number) => void = x => {};
>f1 : Symbol(f1, Decl(restTupleUnionShorterContextualParams.ts, 3, 5))
>x : Symbol(x, Decl(restTupleUnionShorterContextualParams.ts, 3, 11))
>x : Symbol(x, Decl(restTupleUnionShorterContextualParams.ts, 3, 40))

const f2: (x: string | number, y: string | number) => void = f1;
>f2 : Symbol(f2, Decl(restTupleUnionShorterContextualParams.ts, 4, 5))
>x : Symbol(x, Decl(restTupleUnionShorterContextualParams.ts, 4, 11))
>y : Symbol(y, Decl(restTupleUnionShorterContextualParams.ts, 4, 30))
>f1 : Symbol(f1, Decl(restTupleUnionShorterContextualParams.ts, 3, 5))

const f3: (...args: [number, string] | [string, number]) => void = f2;
>f3 : Symbol(f3, Decl(restTupleUnionShorterContextualParams.ts, 5, 5))
>args : Symbol(args, Decl(restTupleUnionShorterContextualParams.ts, 5, 11))
>f2 : Symbol(f2, Decl(restTupleUnionShorterContextualParams.ts, 4, 5))

// by extension those should be OK too
const f4: (...args: [number, string] | [string, number]) => void = (item) => {}
>f4 : Symbol(f4, Decl(restTupleUnionShorterContextualParams.ts, 8, 5))
>args : Symbol(args, Decl(restTupleUnionShorterContextualParams.ts, 8, 11))
>item : Symbol(item, Decl(restTupleUnionShorterContextualParams.ts, 8, 68))

const f5: (...args: [number, string] | [string, number]) => void = (item: number | string) => {}
>f5 : Symbol(f5, Decl(restTupleUnionShorterContextualParams.ts, 9, 5))
>args : Symbol(args, Decl(restTupleUnionShorterContextualParams.ts, 9, 11))
>item : Symbol(item, Decl(restTupleUnionShorterContextualParams.ts, 9, 68))

34 changes: 34 additions & 0 deletions tests/baselines/reference/restTupleUnionShorterContextualParams.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
=== tests/cases/compiler/restTupleUnionShorterContextualParams.ts ===
// repro #48663

// showcase how those transitive assignments are OK
const f1: (x: string | number) => void = x => {};
>f1 : (x: string | number) => void
>x : string | number
>x => {} : (x: string | number) => void
>x : string | number

const f2: (x: string | number, y: string | number) => void = f1;
>f2 : (x: string | number, y: string | number) => void
>x : string | number
>y : string | number
>f1 : (x: string | number) => void

const f3: (...args: [number, string] | [string, number]) => void = f2;
>f3 : (...args: [number, string] | [string, number]) => void
>args : [number, string] | [string, number]
>f2 : (x: string | number, y: string | number) => void

// by extension those should be OK too
const f4: (...args: [number, string] | [string, number]) => void = (item) => {}
>f4 : (...args: [number, string] | [string, number]) => void
>args : [number, string] | [string, number]
>(item) => {} : (item: string | number) => void
>item : string | number

const f5: (...args: [number, string] | [string, number]) => void = (item: number | string) => {}
>f5 : (...args: [number, string] | [string, number]) => void
>args : [number, string] | [string, number]
>(item: number | string) => {} : (item: number | string) => void
>item : string | number

16 changes: 16 additions & 0 deletions tests/baselines/reference/restTupleUnionWithRestContextualParams.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
=== tests/cases/compiler/restTupleUnionWithRestContextualParams.ts ===
const f1: (...args: [number, string, ...boolean[]] | [string, number, ...boolean[]]) => void = (a, b, c) => {};
>f1 : Symbol(f1, Decl(restTupleUnionWithRestContextualParams.ts, 0, 5))
>args : Symbol(args, Decl(restTupleUnionWithRestContextualParams.ts, 0, 11))
>a : Symbol(a, Decl(restTupleUnionWithRestContextualParams.ts, 0, 96))
>b : Symbol(b, Decl(restTupleUnionWithRestContextualParams.ts, 0, 98))
>c : Symbol(c, Decl(restTupleUnionWithRestContextualParams.ts, 0, 101))

const f2: (x: string, ...args: [string] | [number, boolean]) => void = (a, b, c) => {};
>f2 : Symbol(f2, Decl(restTupleUnionWithRestContextualParams.ts, 2, 5))
>x : Symbol(x, Decl(restTupleUnionWithRestContextualParams.ts, 2, 11))
>args : Symbol(args, Decl(restTupleUnionWithRestContextualParams.ts, 2, 21))
>a : Symbol(a, Decl(restTupleUnionWithRestContextualParams.ts, 2, 72))
>b : Symbol(b, Decl(restTupleUnionWithRestContextualParams.ts, 2, 74))
>c : Symbol(c, Decl(restTupleUnionWithRestContextualParams.ts, 2, 77))

18 changes: 18 additions & 0 deletions tests/baselines/reference/restTupleUnionWithRestContextualParams.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
=== tests/cases/compiler/restTupleUnionWithRestContextualParams.ts ===
const f1: (...args: [number, string, ...boolean[]] | [string, number, ...boolean[]]) => void = (a, b, c) => {};
>f1 : (...args: [number, string, ...boolean[]] | [string, number, ...boolean[]]) => void
>args : [number, string, ...boolean[]] | [string, number, ...boolean[]]
>(a, b, c) => {} : (a: string | number, b: string | number, c: boolean) => void
>a : string | number
>b : string | number
>c : boolean

const f2: (x: string, ...args: [string] | [number, boolean]) => void = (a, b, c) => {};
>f2 : (x: string, ...args: [string] | [number, boolean]) => void
>x : string
>args : [string] | [number, boolean]
>(a, b, c) => {} : (a: string, b: string | number, c: boolean | undefined) => void
>a : string
>b : string | number
>c : boolean | undefined

13 changes: 13 additions & 0 deletions tests/cases/compiler/restTupleUnionShorterContextualParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// @strict: true
// @noEmit: true

// repro #48663

// showcase how those transitive assignments are OK
const f1: (x: string | number) => void = x => {};
const f2: (x: string | number, y: string | number) => void = f1;
const f3: (...args: [number, string] | [string, number]) => void = f2;

// by extension those should be OK too
const f4: (...args: [number, string] | [string, number]) => void = (item) => {}
const f5: (...args: [number, string] | [string, number]) => void = (item: number | string) => {}
6 changes: 6 additions & 0 deletions tests/cases/compiler/restTupleUnionWithRestContextualParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// @strict: true
// @noEmit: true

const f1: (...args: [number, string, ...boolean[]] | [string, number, ...boolean[]]) => void = (a, b, c) => {};

const f2: (x: string, ...args: [string] | [number, boolean]) => void = (a, b, c) => {};