TypeScript icon indicating copy to clipboard operation
TypeScript copied to clipboard

Spread operator should not have worse ergonomics than `apply` - unexpected error spreading a union-of-tuples

Open callionica opened this issue 3 years ago • 14 comments

Bug Report

Attempting to spread a union-of-tuples that is otherwise compatible with the function being called results in an unexpected compiler error:

4.3.5+ "A spread argument must either have a tuple type or be passed to a rest parameter." 3.33-4.2.3 "Expected 2 arguments, but got 0 or more."

However, calling apply on the function with the same union-of-tuples does not produce an error, and neither does calling the function "memberwise" using indexes into the union-of-tuples. In both the apply case and the memberwise case, the user is getting the benefit of some level of typechecking, whereas when using the spread operator, the user is met with a hard error.

The error message is confusing, but more confusing is why there would even be an error. I am opening this issue primarily to address the error, not the message.

Since there is a very clear relationship between apply, a memberwise call, and the spread operator, it is perplexing why spread does not work in this case. I imagine that hitting this kind of error might make JS switchers to TS quite confused and possibly cause them to abandon ship. That same relationship between spread (which does not work as expected) and apply (which works adequately) also points to at least one possible path to implementing the desired behavior of successfully typechecking a spread involving a union-of-tuples.

🔎 Search Terms

spread, apply, union of tuples, A spread argument must either have a tuple type or be passed to a rest parameter

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about spread

⏯ Playground Link

Playground link with relevant code

💻 Code

// A function that takes two arguments:
function fn(a: string | number, b: string | number) {
    console.log(a, b);
}

// A union-of-2-tuples type
type T = [string, string] | [number, number];

// A union-of-tuples variable
let t : T = Math.random() > 0.5 ? ["A", "B"] : [1, 1];

// Can we call the fn member-wise with a union-of-tuples? Yes.
fn(t[0], t[1]);

// Can we call the function through apply with a union-of-tuples? Yes.
fn.apply(null, t);

// Note that in both cases above type checking is happening! 

// Can we spread the variable? No.
fn(...t); // ERROR: A spread argument must either have a tuple type or be passed to a rest parameter.

🙁 Actual behavior

In the code above, you can see that we have a variable t that is a union-of-tuples.

Each tuple in the union is type-compatible with the function fn.

You can see that we are able to call the function using the members of t without any casts nor compiler errors: fn(t[0], t[1]).

You can also see that we are able to pass t to the function in one go using apply: fn.apply(null, t). Again, this is successful and has no casts or other code artifacts.

Finally, we attempt to call the function by spreading t: fn(...t). This alone causes the compiler to produce an error.

🙂 Expected behavior

I would expect the code using the spread operator, fn(...t), to succeed, just as the apply and memberwise versions succeeded.

callionica avatar Jul 06 '22 10:07 callionica

I've implemented a version of a function call type-checking algorithm that involves spread and there weren't any particular challenges. It's a demo/POC - not a changelist - using a mini-parser with no generics or type inference, but it might be useful if anyone is going to take a look at improving TS support for spread.

callionica avatar Jul 14 '22 15:07 callionica

I'd be interested to see the PR.

A couple testcases I'd like to see, along with discussion of how they're handled:

declare function fn<T>(arg: T, cb: (n: T) => void): void;

declare const t: [number] | [string] | [boolean];

fn(...t, x => {
    const n: number | string = x;
});
type Tupleize<T> = T extends unknown ? [T] : never;
type WindowKeys = Tupleize<keyof typeof window>;
declare const c1: WindowKeys;
declare function fn<T>(arg1: T, arg2: T, arg3: T, arg4: T, arg5: T): T;
fn(...c1, ...c1, ...c1, ...c1, ...c1);

RyanCavanaugh avatar Jul 14 '22 16:07 RyanCavanaugh

These are interesting test cases, but I don't have a PR, it's a demo and it doesn't involve generics or inference. TS' spread support is currently a bit limited in non-generic, explicitly-typed code which is why I opened this issue with code that doesn't involve generics or inference.

It's not obvious to me that inference is the blocker on improvements to spread support, but I assume by your test cases that's a concern for you. In the first code you provide, I see that x is inferred as string | number | boolean and the function produces a corresponding failure on the assignment. That seems reasonable to me. Similarly, despite the size of the tuple with ~820 members on your second test case, TS seems to infer the types OK. Maybe I shouldn't trust the current inference results because of the failure on the spread itself? I had imagined that inference happened before the stage that produced the spread error, and maybe that's not true, but first results are hopeful.

Ignoring inference for a moment, the 2nd test case is clearly intended to test large unions. In that test case, WindowKeys is a union of single member tuples with ~820 members and if inference has already happened T is also a union with the same number of members. The basic algorithm for spreading would deunionize the argument union and test its single member for compatibility with the parameter union and it would do it 5 times for the whole function: so 820 x 5 typechecks of a literal against an 820 member union. It's not obvious that this is too expensive, but there are opportunities for both optimization and parallelization. For both of these, we recognize that the union consists only of tuples of the same length. Instead of deunionizing the argument, we can unionize each element of the argument tuples and typecheck that against the type of the relevant parameter. In this case, it's the same type, so I expect it'd be a pretty quick type check. So element unionization once and then 5 quick comparisons.

It's hard to describe optimizations without knowing exactly the current implementation and without measuring particular operations. Given how WindowKeys has been defined, it might be crazy cheap to get back to key typeof window which is what we're really going to end up comparing. Parallelisation is similarly possible once you recognize that if you have fixed lengths, you can do the arg to param mapping first and then parallelize the type checking.

The way I think about it is this: there's a dead simple algorithm that might be slow in the presence of large unions. Knowing if you have a large union is surely super cheap, so if you don't have the payroll budget for optimization, during compilation you can check the conditions that might blow your runtime budget and produce a compile error. In other words, error only when you see the big unions, and otherwise use the simple algorithm for most cases where there aren't large unions. That's step 1. But, yes, there are optimization possibilities for large unions.

Does TS have a defined runtime budget for particular type-checking operations?

If you want to point me at the relevant code in your codebase, I'd be interested to take a look, but it's unlikely I'll be producing a PR.

callionica avatar Jul 14 '22 20:07 callionica

Non-generic test case that fails in Typescript 4.7.4:

declare function fn(p0: number, p1: string, ...rest: string[]) : void;
declare const a0 : number;
declare const a1 : string[];
fn(a0, ...a1);

Playground 001

Expected: no error Actual: "A spread argument must either have a tuple type or be passed to a rest parameter."

Presumably TS is failing because a1 could be an empty array, leaving parameter p1 unfilled.

However, this is too strict, because the following also fails yet the code ensures parameter p1 will be filled:

declare function fn(p0: number, p1: string, ...rest: string[]) : void;
declare const a0 : number;
declare const a1 : string[];
if (a1.length >= 1) {
    fn(a0, ...a1);
}

Playground 002

If TS was able to handle to the length check, it would be acceptable to fail the case that doesn't check the length. But TS doesn't use the information that the length has been checked, so I believe it would be better to allow both cases instead of preventing both cases.

(Note that there is no compile error if p1 is made optional which is good).

callionica avatar Jul 15 '22 14:07 callionica

Another unexpected compile error:

declare function fn(p0: number, p1: string, p2?: number) : void;
declare const a0 : number;
declare const a1 : ([string] | [string, number]);
fn(a0, ...a1);

Playground 003

Expected: no error Actual: "A spread argument must either have a tuple type or be passed to a rest parameter."

It should compile because the required parameters are guaranteed to be filled.

This one is interesting because we can compare it to this case which does compile, so tuple unions occasionally work:

declare function fn(p0: number, p1?: string, p2?: number) : void;
declare const a0 : number;
declare const a1 : ([string] | []);
fn(a0, ...a1);

callionica avatar Jul 15 '22 14:07 callionica

This is a fun one because we get a new error message:

declare function fn(p0: number, p1?: string, p2?: number) : void;
declare const a0 : number;
declare const a1 : ([string] | [string, number]);
fn(a0, ...a1);

Playground 004

Expected: no error Actual:

Argument of type 'string | number' is not assignable to parameter of type 'string | undefined'.
  Type 'number' is not assignable to type 'string'.

Where does Argument of type 'string | number' come from?

...a1 is either a single argument of type string or 2 arguments of types string and number. If we combine them we could get an argument of type string and an argument of type number | undefined. I'm not seeing how string | number could be relevant here except if TS has collapsed ([string] | [string, number]) to (string | number)[], but that doesn't seem like a winning move.

Perhaps worth comparing to this one which gets us back to the classic error:

declare function fn(p0: number, p1?: string, ...rest: number[]) : void;
declare const a0 : number;
declare const a1 : ([string] | [string, number]);
declare const a2 : number;
fn(a0, ...a1, a2);

Playground 005

Expected: No error Actual: A spread argument must either have a tuple type or be passed to a rest parameter.

The code should compile because a2 is guaranteed to be in rest parameter position, so we know that it should be number and it is.

callionica avatar Jul 15 '22 14:07 callionica

You can see the spread checking demo at https://callionica.github.io/typescript/type-demo.htm

callionica avatar Jul 16 '22 11:07 callionica

Taking a glance through the TS code in checker.ts. I see that tuples (but not arrays or iterables) are expanded in getEffectiveCallArguments and later hasCorrectArity forces the spread argument returned by getSpreadArgumentIndex to start at optional/rest parameter position. (I think that code in hasCorrectArity does not apply to tuples as a result of the earlier expansion).

Looks like the spread checking code in hasCorrectArity is the cause of the errors produced in Playground 001 and Playground 002 above.

callionica avatar Jul 16 '22 16:07 callionica

It would be nice for very simple cases to not have this issue, for example:

declare function doStuff(name: string, age: number): void
declare function doStuff(employeeId: number): void

declare const args: [string, number] | [number]

// Error described in this issue:
doStuff(...args)

// No error:
if (args.length === 1) {
    doStuff(...args)
} else {
    doStuff(...args)
}

Essentially, if the tuple union can be trivially turned into a single type via a simple condition (such as length) – would it not be possible to iterate over the conditions in the first doStuff(...args) call and allow it if all pass?

blixt avatar Sep 09 '22 08:09 blixt

A quick update for v5.0.4

Original repro fails as originally described Playground 1 fails as originally described Playground 2 fails as originally described Playground 3 fails as originally described Playground 4 fails as originally described Playground 5 fails as originally described

So no change in behavior with these repros.

I'm not sure how I should understand the "Suggestion" label on this bug, but I just want to repeat that I believe that users see this behavior as a defect in TS.

callionica avatar Apr 25 '23 08:04 callionica

Not sure what's confusing about my update. I opened this issue to describe buggy behavior in July 2022 and my update is to say that the buggy behavior is still present in TypeScript as of 5.0.4 in April 2023. Hope that helps.

callionica avatar Apr 29 '23 13:04 callionica

It'd be great to have better behavior here. One time this comes up is eslint giving prefer-spread errors and then ts saying you can't spread a union of tuples.

It seems to me there should be basic support for this, even if a couple edge cases aren't immediately supported. I agree with @callionica, this feels like a defect.

jeremy-rifkin avatar Jun 18 '23 11:06 jeremy-rifkin

This is still an issue in 5.4.2. I didn't check all the playgrounds this time, but I did check the first one and that still reproduces.

callionica avatar Mar 19 '24 15:03 callionica

Related on Stack Overflow: spread argument union of tuples

starball5 avatar May 14 '24 21:05 starball5