microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.07k stars 12.49k forks source link

"Spread argument must either have a tuple type" err even when spread arg has a tuple type #49700

Closed wbt closed 2 years ago

wbt commented 2 years ago

Bug Report

🔎 Search Terms

tuple type union 2556

🕗 Version & Regression Information

⏯ Playground Link

Playground link with relevant code

💻 Code

const demonstrate = function<B extends boolean>(
    useStringVersion: B,
    paramsTuple : B extends true ? readonly [string] : readonly [number, number]
) {
    //Errors on each function call:
    //A spread argument must either have a tuple type or be passed to a rest parameter. ts(2556)
    //The error doesn't make sense because paramsTuple is a tuple.
    //Note that `this` within the called function should not be overridden.
    //The function definitions are in an external library, do not take a rest parameter, and cannot be modified.
    if(useStringVersion) {
        return stringFunction(...paramsTuple);
    } else {
        return numbersFunction(...paramsTuple);
    }
    //This is intended as a minimal example from a motivating case with more potential functions
    //and a more complex selection process for which one is called, but that can't get into a
    //working state without significant functionality reduction unless this simpler example can also work.
    //Writing function overrides also introduces undesired maintainability-reducing repetition 
    //& brittleness which would require active maintenance when the external library is updated, 
    //instead of the benefits of passing on a call directly.
}
const numbersFunction = function(low: number, high: number) { /*...*/ }
const stringFunction = function(myString: string) { /*...*/ }

🙁 Actual behavior

Each of the return lines throw an error:

A spread argument must either have a tuple type or be passed to a rest parameter. ts(2556)

The error doesn't make sense because paramsTuple is a tuple.

Though it is the same error code reported in #43787, I don’t think it’s a duplicate of that because in this case, for any given call to the demonstrate function, the paramsTuple is one kind of tuple. It seems like this should work.

The same issue is observed when using the Parameters approach to generating the tuple types (playground link here).

🙂 Expected behavior

No errors and no need to rewrite valid JavaScript beyond adding type annotations; Typescript can validate proper typing.

The motivating use case here is part of a generic caching queue for calling functions from an externally defined set across a noisy & narrow network pipe. Each function in the set might require a different tuple of parameters, but the types are well defined and the right tuple type corresponding to which function is being called is always paired with the function identifier. Without type annotations, the code works just fine as JavaScript.

RyanCavanaugh commented 2 years ago

A conditional type isn't a tuple type; the error means what it says and means it correctly.

This is a correct error; you could legally call

demonstrate<boolean>(true, [1, 2]);
wbt commented 2 years ago

When both sides of the conditional are tuple types, it seems like the conditional should itself be at least considered a tuple type.

callionica commented 2 years ago

@wbt It's easy to miss a couple of subtleties with your example. I hope you don't mind if I highlight them:

  1. We normally think of booleans as only having one of two values, but we can actually think of it as having one of three states when we think of the type: (1) true, (2) false, (3) true | false (The third state, true | false, can be shortened to boolean).
  2. The constraint T extends boolean means that T can be in any of those 3 states (it doesn't limit T to being only true or false) because T extends T is always true.
  3. When you pass a union type through a generic argument, an interesting thing happens: type X<A | B> is treated like X<A> | X<B>
  4. If you look at paramsTuple : B extends true ? readonly [string] : readonly [number, number] knowing that B is a generic parameter, you can see that paramsTuple ends up with the type readonly [string] | readonly [number, number] when B is boolean (aka true | false).

Hopefully that explains what's going on in your code.

wbt commented 2 years ago

Thanks for your patient explanation @callionica!

However I think there might be some over-focus on an artifact of the simplification to the example above, and my most recent comment is about an even more basic point: Regardless of what B is, paramsTuple should still be a Tuple type, and the error message saying it's not a Tuple type doesn't make sense.

For example, consider this variant of the same code. The second parameter is of type B extends true ? 'alpha' : 'bravo' but because both 'alpha' and 'bravo' (i.e. both sides of the conditional) are string the parameter value can be used anywhere that requires a string type. The expected behavior is the same: when paramsTuple is going to be a tuple no matter what, TypeScript saying it's not a tuple creates frustrating confusion.

callionica commented 2 years ago

I agree that the error message is confusing and could be improved with a better message even if the behaviour stays the same.

I agree it would also be desirable to be able to spread a union of tuples.

Here's a simplified repro of the underlying problem, showing a case where currently good code is getting an error from the compiler, as well as the case where an error message would be improved:

type T = [number, number] | [string, string];

function takesNumbers(n1: number, n2: number) {
}

const t : T = Math.random() > 0.5 ? [1,2] : ["a", "b"];

// ACTUAL: Produces an error saying "spread argument must .. have a tuple type .."
// DESIRED: This should produce an error, but not because spread is invalid on this type
// It should fail because takesNumbers cannot take arguments of string, string
takesNumbers(...t); 

function takesEither(n1: string, n2: string) : void
function takesEither(n1: number, n2: number) : void
function takesEither(n1: number | string, n2: number | string) : void {
}

// ACTUAL: Produces an error saying "spread argument must .. have a tuple type .."
// DESIRED: This should not produce an error because takesEither can handle any result of spreading the tuple
takesEither(...t);

Playground

fatcerberus commented 2 years ago

Regardless of the suitability of the error message, the error is correct. When B = boolean, then typeof paramsTuple = [string] | [number, number], meaning it's unsound to use ...paramsTuple in either call (making this typesafe would require extends oneof constraints). It also means B can be a union, and a union of tuple types is not itself a tuple type (in the same sense that "foo" is a unit type while "foo" | "bar" is not; admittedly this is squabbling over semantics though)

callionica commented 2 years ago

I think the point that OP was making is that spreading a union of tuples can and should be supported, which in the original code would produce different errors and in other code would eliminate errors. I produced a new repro, in my previous comment, that hopefully eliminates some distractions from the original.

RyanCavanaugh commented 2 years ago

Repeated overload resolution using every possible constituent of a union is going to be combinatorially explosive in many real-world situations. This isn't something we could plausibly do.

wbt commented 2 years ago

TS resolves B extends true ? 'alpha' : 'bravo' as being validly assignable to string - why is it that different and more feasible than resolving a conditional on Tuple types as being a Tuple?

RyanCavanaugh commented 2 years ago

Because the very next thing that happens after we determine that it's a tuple type is to look inside the tuple and apply its contents to the function overload algorithm.

fatcerberus commented 2 years ago

Because the very next thing that happens after we determine that it's a tuple type is to look inside the tuple and apply its contents to the function overload algorithm.

Ah ha! So my comparison to unit types was appropriate after all (i.e. that a union of unit types is not itself a unit type, and this is the same capacity in which TS cares about something “being a tuple type” here)

callionica commented 2 years ago

It is surprising to me that the reason for not making a fix in this area is a complexity concern (about overloads specifically?) given that TS appears to already handle type checking for union-of-tuple types for variable assignment and for calling single argument functions. In both those cases TS produces pretty reasonable error messages when the types don't match. It feels like there is a fairly straightforward mapping between a function that takes separate arguments and a function that takes a single tuple argument (and overload sets is just an extension of this mapping to a single union-of-tuples argument), so it's not obvious to me why the line is drawn here. If overloads is the step too far, would it not be preferable to detect the overloads (at presumably minimal cost) and only fail those cases? But still, the mapping between overload sets, functions with multiple arguments, and a function with a single union-of-tuples argument (which TS handles) is so strong in my mind, that I must be missing something.

callionica commented 2 years ago

Here's another repro showing the strangeness of the spread failure (no overloads involved). We can get type checking if we use apply, but we just get a failure if we use .... It's a strange inconsistency.

// 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.

Playground

typescript-bot commented 2 years ago

This issue has been marked as 'Question' and has seen no recent activity. It has been automatically closed for house-keeping purposes. If you're still waiting on a response, questions are usually better suited to stackoverflow or the TypeScript Discord community.

wbt commented 2 years ago

I don't think the tag or closure is really appropriate here.

RyanCavanaugh commented 2 years ago

It is surprising to me that the reason for not making a fix in this area is a complexity concern...

I think this is a misapprehension. The problem is that to handle this "correctly" (i.e. with no visible discontinuities in behavior, which then become a future source of complaint because it only moves the problem one step over rather than solving it completely), the only correct algorithm is necessarily O(n ^ m), because the only correct thing to do is to multiply all out all the possible tuple values into an argument list, and resolve that against the overload list. The list of all possible tuple inhabitants could easily be enormous, literally billions.

Pretty much every operation needs to be done in terms of the category of inputs, not through combinatorial explosion of every possible inhabitant of the category. Otherwise typechecking never finishes.

RyanCavanaugh commented 2 years ago

I don't think the tag or closure is really appropriate here.

Your complaint seems to be that:

If you want me to change the label to "Not a defect" I can do that, but open issues represent identifiable next steps and this has neither.

callionica commented 2 years ago

I'm still confused by the complexity argument, partly because "the only correct thing to do is to multiply all out all the possible tuple values into an argument list" doesn't seem correct to me, whereas I agree with "Pretty much every operation needs to be done in terms of the category of inputs, not through combinatorial explosion of every possible inhabitant of the category. Otherwise typechecking never finishes." But surely we must be talking at cross-purposes.

In the interests of clarity, I have opened a new issue #49802 to address my core issue that spread fails where apply and memberwise calls succeed.

imdavidmin commented 2 years ago

How should this scenario be handled presently then? I don't think writing all overloads out by hand would be a clever way? I do see the current workaround suggested by #49802 using Function.apply(null, args), and would think this is a very archaic way to involve setting this when that's not relevant, just to satisfy Typescript.

wbt commented 2 years ago

The error message can and should be improved, especially because other conditionals with the same type on each branch are usable as that type. Also, just for completeness, it doesn't have to be a conditional: a simple union of two Tuple types still produces this message that it's not a tuple; same if selected from a map.

I also don't agree with the argument that "because the worst case is slow, it's definitely better to not handle more common reasonable cases and instead require hacky workarounds or disabling of type-checking altogether." Handling simpler common cases even without handling the most complex version of a situation is still a (potentially-large) step forward.

mbrittsess commented 2 years ago

I think this is the same bug, but I got hit by it for an even simpler case, and I'm very frustrated at how long it took to figure out why it was happening (especially since it happened inconsistently, as in several places TS was able to infer a more specific version of the type, so the error didn't occur).

declare function setColor ( gray :number ) :void;
declare function setColor ( red :number, green :number, blue :number ) :void;

declare const SomeColor :[number]|[number,number,number];

// Error: "A spread argument must either have a tuple type or be passed to a rest parameter."
setColor( ...SomeColor );

To make matters even more unintuitive, while the solution is to perform a cast:

setColor( ...(SomeColor as [number,number,number]) );

In my real-world example (which I will show a simplified version of), this is even more unintuitive because it sometimes works and sometimes doesn't, again based on some unexpected narrowing inferences. Like so:

declare function setColor ( gray :number ) :void;
declare function setColor ( r :number, g :number, b :number ) :void;

// Later maintainers are intended to be able to change this value, I'd like them to get automatic errors immediately if they define the wrong number of elements
const SomeColor :[number]|[number,number,number] = [ 0.2 * 255 ]; 

function foo ( ) :void {
    // Error is resolved here
    setColor( ...(SomeColor as [number,number,number]) );
};

// But trying the same solution here gives error:
// "Conversion of type '[number]' to type '[number, number, number]' may be a mistake because..."
setColor( ...(SomeColor as [number,number,number]) );

If it's still decided that TypeScript isn't going to make this feature work, then at least some much more explanatory error messages would be greatly appreciated.

zhuzeyu22 commented 1 year ago

Look at this Proxy

const target = {
  message1: "hello",
  message2: "everyone",
};

const handler3 = {
  get(target, prop, receiver) {
    if (prop === "message2") {
      return "world";
    }
    return Reflect.get(...arguments);
  },
};

const proxy3 = new Proxy(target, handler3);

console.log(proxy3.message1); // hello
console.log(proxy3.message2); // world

I got a error

11:26 - error TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.

11       return Reflect.get(...arguments);
controversial commented 1 year ago

In the following snippet:

const directionVec = ({ horizontal: [1, 0], vertical: [0, 1] } as const)[direction];
//    ^ readonly [1, 0] | readonly [0, 1]
gl.uniform2i(this.blurProgram.uniforms.direction, ...directionVec);

typechecking fails with “spread argument must either have a tuple type ...”

However, if I coerce directionVce to the (wider) [number, number], typechecking passes:

const directionVec: readonly [number, number] = ({ horizontal: [1, 0], vertical: [0, 1] } as const)[direction];
//    ^ [number, number]
gl.uniform2i(this.blurProgram.uniforms.direction, ...directionVec);

It seems like TypeScript should be at least able to handle the case where a union has a homogenous tuple type?

pencilcheck commented 10 months ago

I want to ignore this error, as it doesn't make any sense.

If I convert to ".apply" format it told me that it prefer spread argument so it is a circular error....