-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
1819397
67892c3
bd712ab
88027d5
6c5383d
46f2e57
0de8923
beb241d
388ec89
8753248
e357450
29979e2
fd52884
336288a
92d0267
1c24035
608f7c3
61d2612
919fc0e
b920e9a
a8ddfc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1298,6 +1298,12 @@ export const enum SignatureCheckMode { | |
Callback = BivariantCallback | StrictCallback, | ||
} | ||
|
||
const enum TupleStructureComparisonKind { | ||
None = 0, | ||
MatchFixed = 1 << 0, | ||
MatchVariable = 1 << 1, | ||
} | ||
|
||
const enum IntersectionState { | ||
None = 0, | ||
Source = 1 << 0, // Source type is a constituent of an outer intersection | ||
|
@@ -20732,7 +20738,59 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
|
||
for (let i = 0; i < paramCount; i++) { | ||
const sourceType = i === restIndex ? getRestOrAnyTypeAtPosition(source, i) : tryGetTypeAtPosition(source, i); | ||
const targetType = i === restIndex ? getRestOrAnyTypeAtPosition(target, i) : tryGetTypeAtPosition(target, i); | ||
let targetType = i === restIndex ? getRestOrAnyTypeAtPosition(target, i) : tryGetTypeAtPosition(target, i); | ||
if (i === restIndex && targetType && sourceType && isTupleType(sourceType)) { | ||
targetType = mapType(targetType, t => { | ||
if ( | ||
!isTupleType(t) || | ||
// When both sides are tuples of the same structure, we don't want to "propagate" types from elements of variable positions | ||
// to the following positions as that would disallow signatures of the exact same structures when trailing fixed elements are involved: | ||
// | ||
// let fn: (...rest: [...string[], number]) => void = (...rest: [...string[], number]) => {}; // ok | ||
// | ||
// Since we want to allow contextual types to flow into paremeters, we don't need to differentiate between rest and variadic elements | ||
// as that doesn't affect the contextual type of the parameter | ||
isTupleTypeStructureMatching(sourceType, t, TupleStructureComparisonKind.MatchVariable) | ||
) { | ||
return t; | ||
} | ||
|
||
// We create a tuple type based on the target elements and the source's length here. | ||
// When the source signature accepts less parameters than the target signature | ||
// we only need to check the *used* elements of the target tuple, the rest is ignored by the source anyway | ||
// and thus it can be safely ignored here. | ||
// | ||
// let fn: (a: number, b: string) => void = (a: number) => {}; // ok | ||
// | ||
// In addition to that we also want to "propagate" element types of variable positions | ||
// to all following positions, as that represents possible argument types. | ||
// | ||
// function fn(...[a, b]: [...number[], string]) { | ||
// a; // number | string | ||
// b; // number | string | ||
// } | ||
// fn('str'); // valid | ||
const elementTypes: Type[] = []; | ||
const elementFlags: ElementFlags[] = []; | ||
|
||
const sourceArity = getTypeReferenceArity(sourceType); | ||
const targetArity = getTypeReferenceArity(t); | ||
|
||
for (let i = 0; i < sourceArity; i++) { | ||
if (i >= targetArity) { | ||
if (sourceType.target.elementFlags[i] & ElementFlags.Fixed) { | ||
elementTypes.push(undefinedType); | ||
elementFlags.push(sourceType.target.elementFlags[i]); | ||
} | ||
continue; | ||
} | ||
elementTypes.push(getTupleElementType(t, i)!); | ||
elementFlags.push(sourceType.target.elementFlags[i]); | ||
} | ||
|
||
return createTupleType(elementTypes, elementFlags); | ||
}); | ||
} | ||
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, | ||
|
@@ -24440,9 +24498,14 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
return undefined; | ||
} | ||
|
||
function isTupleTypeStructureMatching(t1: TupleTypeReference, t2: TupleTypeReference) { | ||
function isTupleTypeStructureMatching(t1: TupleTypeReference, t2: TupleTypeReference, tupleStructureComparisonKind: TupleStructureComparisonKind) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious if we could just accept |
||
return getTypeReferenceArity(t1) === getTypeReferenceArity(t2) && | ||
every(t1.target.elementFlags, (f, i) => (f & ElementFlags.Variable) === (t2.target.elementFlags[i] & ElementFlags.Variable)); | ||
every(t1.target.elementFlags, (f1, i) => { | ||
const f2 = t2.target.elementFlags[i]; | ||
return f1 === f2 || | ||
!!(tupleStructureComparisonKind & TupleStructureComparisonKind.MatchFixed && f1 & ElementFlags.Fixed && f2 & ElementFlags.Fixed) || | ||
!!(tupleStructureComparisonKind & TupleStructureComparisonKind.MatchVariable && f1 & ElementFlags.Variable && f2 & ElementFlags.Variable); | ||
Comment on lines
+24506
to
+24507
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why aren't these something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I guess that breaks things. |
||
}); | ||
} | ||
|
||
function isZeroBigInt({ value }: BigIntLiteralType) { | ||
|
@@ -26050,7 +26113,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
const elementFlags = target.target.elementFlags; | ||
// When source and target are tuple types with the same structure (fixed, variadic, and rest are matched | ||
// to the same kind in each position), simply infer between the element types. | ||
if (isTupleType(source) && isTupleTypeStructureMatching(source, target)) { | ||
if (isTupleType(source) && isTupleTypeStructureMatching(source, target, TupleStructureComparisonKind.MatchFixed)) { | ||
for (let i = 0; i < targetArity; i++) { | ||
inferFromTypes(getTypeArguments(source)[i], elementTypes[i]); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -6,14 +6,15 @@ genericRestParameters3.ts(18,1): error TS2345: Argument of type '[]' is not assi | |||
Source has 0 element(s) but target requires 2. | ||||
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'. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||
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'. | ||||
genericRestParameters3.ts(35,1): error TS2554: Expected 1 arguments, but got 0. | ||||
genericRestParameters3.ts(36,21): error TS2345: Argument of type 'number' is not assignable to parameter of type '(...args: CoolArray<any>) => void'. | ||||
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'. | ||||
|
@@ -69,16 +70,17 @@ genericRestParameters3.ts(59,5): error TS2345: Argument of type '["what"]' is no | |||
~~ | ||||
!!! 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): 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:
|
||||
f1 = f4; | ||||
|
||||
// Repro from #26110 | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
//// [tests/cases/compiler/restTupleUnionShorterContextualParams.ts] //// | ||
|
||
=== 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)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we use
append
rather than pushing onto an empty list directly; this is typically more efficient due to how the runtime constructs arrays.