-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Error when spreading a union of tuples in a call #42508
Comments
I think I've seen either same, or very similar problem in my code once I've upgraded from TS 3.9 to TS 4.0 / TS 4.1 (both versions report the same issue with my code). This was not an issue before the upgrade, so the last known working version (for me at least) is 3.9.7. My code looks something like this (it's a React app, the snippet comes from a wrapper component that attaches its own additional handler on the passed type BaseComponentProps = React.ComponentProps<typeof BaseComponent>;
const handleClick = (...args: Parameters<NonNullable<BaseComponentProps['onChange']>>) => {
onChangeAdditionalHandler(...args);
onChange?.(...args);
}; With this code I get the following error on the
even though the args is properly inferred as:
(the union type comes from how React typings are created for the base component) This can be simplified and reproduced with the following code: const testFunction = (thisVal: () => void) => {
console.log(thisVal);
};
const testCase = () => {
type MyArgs =
| [ handler: () => void ]
| [ handler: () => void ];
const args = [ () => undefined ] as MyArgs;
testFunction(...args);
}; It looks like union types freak out the arguments spreading, even though the applied types are basically the same thing. Even the tuples have the same length, so it shouldn't be a problem. |
Related to #42891; I'll look at both at the same time. |
Previously, only calls with tuples were allowed, not unions of tuples. For example, this already works: ``` f(string, [number, string], string) ==> f(string, number, string, string) ``` But this does not: ``` f(string, [string] | [number, number]) ==> (f, string, string | number, number | undefined) ``` This PR allows union types like these *as the last argument*. It's possible to allow them anywhere, but quite a bit more complicated. And the code is already complicated enough: getEffectiveCallArguments now needs to return the minimum number of arguments as well as the argument list (which is the maximum number of arguments). Also, this transformation should not happen when the tuple union argument is passed to a tuple union rest parameter: `[number] | [string]` is assignable to `[number] | [string]` but the transformed `number | string` is not. Checking for this requires passing around even more information. Bottom line: I'm not sure the complexity is worth the new code. However, if this is a good idea, there are 3 things that need to be cleaned up: 1. More precise error messages--maybe. Right now error messages are reported in terms of the maximum number of arguments: 2. Allow tuples with trailing `...T` types. Not done or tested, but straightfoward. 3. Find a more elegant way to return the minimum number of arguments. Fixes microsoft#42508 Supercedes microsoft#43882, but does less than half of that PR, in less than half the code.
I tried again with a smaller fix, although I'm still not sure it's a good enough fix. |
After discussing this fix in the design meeting, we decided to not take #55185 either. It's too ad-hoc for the complexity of its code. If/when we try to reform overload resolution's interaction with type parameter inference and contextual typing, we should address this bug at the same time. Specifically, this example needs the ability to roll back inferences, same as overload type parameter inference. Then the fix here would be to make tuples generate multiple argument lists instead of trying to produce a single argument list. Each argument list can be applied against each overload (or a single signature if there's only one.) |
I believe I have another case for this issue. function target(arg1: string, ...numsOrStrs: number[] | string[]): void {}
function wrapper(...args: Parameters<typeof target>): ReturnType<typeof target> {
return target(...args); // TS2556 A spread argument must either have a tuple type or be passed to a rest parameter.
} In this case Either playground that has a little extra |
Why not just use this? const t3 = [10, 10] as [number, number, number?]
new Date().setHours(...t3) |
Bug Report
🔎 Search Terms
destructure array typescript union
🕗 Version & Regression Information
I used the playground to test different versions and it looks like this has always occured.
⏯ Playground Link
Playground link with relevant code
💻 Code
🙁 Actual behavior
Error: Expected 1-4 arguments, but got 0 or more.
The array type passed as a length of 2 or 3 so the message "got 0 arguments" is wrong.
🙂 Expected behavior
I expect the compiler should be able to recognize that the array type can only have length 2 or 3 and would not throw an error.
The text was updated successfully, but these errors were encountered: