Skip to content
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

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

Open
callionica opened this issue Jul 6, 2022 · 14 comments
Labels
Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript

Comments

@callionica
Copy link

callionica commented Jul 6, 2022

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
Copy link
Author

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.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Experimentation Needed Someone needs to try this out to see what happens labels Jul 14, 2022
@RyanCavanaugh
Copy link
Member

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);

@callionica
Copy link
Author

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
Copy link
Author

callionica commented Jul 15, 2022

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
Copy link
Author

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
Copy link
Author

callionica commented Jul 15, 2022

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
Copy link
Author

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

@callionica
Copy link
Author

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.

@blixt
Copy link

blixt commented Sep 9, 2022

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?

@callionica
Copy link
Author

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
Copy link
Author

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.

@jeremy-rifkin
Copy link

jeremy-rifkin commented Jun 18, 2023

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.

@callionica
Copy link
Author

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.

@starball5
Copy link

Related on Stack Overflow: spread argument union of tuples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants