KEMBAR78
Improve checking of complex rest parameter types by ahejlsberg · Pull Request #26676 · microsoft/TypeScript · GitHub
Skip to content

Conversation

@ahejlsberg
Copy link
Member

With this PR we properly check rest parameters of any subtype of any[], including user-defined array types and unions of tuple types. In cases where a rest parameter type is not exactly a T[] or a tuple type, we now collect the corresponding remainder of the argument list as a tuple type and then check that this tuple type is assignable to the rest parameter type.

declare const foo: (x: string, ...args: [string] | [number, boolean]) => void;
declare const t: [string] | [number, boolean];

foo("abc", "def");  // Ok
foo("abc", 10, true);  // Ok
foo("abc", ...t);  // Ok
foo("abc", 10);  // Error, [number] not assignable to [string] | [number, boolean]
foo("abc");  // Error, [] not assignable to [string] | [number, boolean]

Effectively, rest parameters typed as unions of tuple types provide a form of overloading expressed in a single function type signature. Unlike regular overloading, generic rest parameters can be used to form composed overloads:

type Func<T extends any[]> = (x: string, ...args: T) => void;

declare const f: Func<[] | [string] | [number, ...boolean[]]>;

f("abc");
f("abc", "def");
f("abc", 10);
f("abc", 10, true);
f("abc", 10, true, false);
f();  // Error
f("abc", true);  // Error

Function type relations have been improved to handle rest parameters of union types:

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

f2 = f1;  // Ok
f3 = f1;  // Ok
f4 = f1;  // Error, misaligned complex rest types
f1 = f2;  // Error
f1 = f3;  // Error
f1 = f4;  // Error, misaligned complex rest types

Note that we currently don't "normalize" misaligned rest parameters. It would certainly be possible to do so, but it is not clear that the additional complexity is merited.

Fixes #26110.
Fixes #26491.

@mattmccutchen
Copy link
Contributor

I just tested the PR (commit 349bee9) on the bad example from #26110 (comment) and I get no compile errors. Is this expected?

@mattmccutchen
Copy link
Contributor

For anyone watching, the discussion of the unsoundness continues at #26110 (comment).

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I like the change. Just a few nits in the code.

const sourceRestType = getNonArrayRestType(source);
const targetRestType = getNonArrayRestType(target);
if (sourceRestType && targetRestType && sourceCount !== targetCount) {
// We're not able to relate misaliged complex rest parameters
Copy link
Member

Choose a reason for hiding this comment

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

typo:misaligned

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix.

return type.target.hasRestElement ? type.typeArguments![type.target.typeParameters!.length - 1] : undefined;
}

function getRestArrayTypeOfTupleType(type: TupleTypeReference) {
Copy link
Member

Choose a reason for hiding this comment

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

since this is only used in getEffectiveRestType, can you move this near to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like it where it is, it is closely related to the function just above.

}
if (restType) {
const spreadType = getSpreadArgumentType(args, argCount, args.length, restType, /*context*/ undefined);
const errorNode = reportErrors ? argCount < args.length ? args[argCount] : node : undefined;
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 not a fan of x ? y : e1 : e2 : undefined patterns. Would reportErrors && (argCount < args.length ? ...) work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sort of agree, but the rewrite you suggest doesn't work because it injects a false into the union type. Ima keepit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants