Excess spread arguments are not errored

[kitsonk]

TypeScript Version: 2.6.1

Code

It appears that extra arguments that are spread arguments are always accepted, which can lead to unintentional errors in the code. For example the following:

declare const foo: (value: string) => string;

foo('bar', 'qat'); // Expected 1 arguments, but got 2.

foo('bar', ...['qat']); // No error

This is related to #4130 but @sandersn suggested a separate issue, as this subset of the larger issue is likely addressable, where as determining if a spread argument might result in longer arguments is difficult, versus cases where it is easily statically identifiable (this case).

Expected behavior:

That an extra spread argument errors.

Actual behavior:

An excess spread argument does not error.

[sandersn]

When I tried to fix this, I found the test that explains why it works this way. It's for functions that use arguments:

function mixed(fixed: number) {
    Debug.assert(fixed > 0);
    for (var i = fixed; i < arguments.length; i++) {
        console.log(arguments[i]);
    }
}
mixed(2, 'hello', 'world', 'goodbye', 'goodbye') // ok ... I guess?

I guess this makes sense, but I think the right thing to do is only allow calls like this for functions that use arguments in the body and disallow it for all other calls.

[mhegazy]

is that in a .js file?

[sandersn]

The test is a .ts file; I haven't tracked the comment in the test back to a discussion on github yet. Maybe restricting that usage to javascript would be an acceptable solution too.

[mhegazy]

The call is an error in TS if you do not have the spread. i.e. mixed(2, 'hello', 'world', 'goodbye', 'goodbye'). the function should have been declred as:

function mixed(fixed: number, ...args: string[]) {
    Debug.assert(fixed > 0);
    for (var i = fixed; i < arguments.length; i++) {
        console.log(arguments[i]);
    }
}

or

function mixed(fixed: number, ...args: string[]);
function mixed(fixed: number) {
 Debug.assert(fixed > 0);
    for (var i = fixed; i < arguments.length; i++) {
        console.log(arguments[i]);
    }
}

[sandersn]

Ah, I see. Yes, the argument has to be a spread, which is what @kitsonk actually has in his code base. I mistakenly converted it to individual arguments.

Here's the actual test case from cases/conformance/expressions/functionCalls/callWithSpread2.ts:

declare function normal(s: string): void;
declare numbers: number[];
normal("i", ...numbers);

I think this should be disallowed. The question is whether it should be allowed if this is the definition of normal:

function normal(s: string) {
  console.log(s);
  console.log(arguments[1]);
}

[mhegazy]

That looks wrong.. i would be interested to see if any RWC/user code test fails because of that.

[kitsonk]

function normal(s: string) {
  console.log(s);
  console.log(arguments[1]);
}

That is exactly the sort of JS bs TypeScript should help us fix as well... I suspect it will break some user code because if you are porting some code over, and TypeScript didn't force you to change it, you certainly wouldn't fix it... I guess it is arguable if it should be strict or not, since it would be a breaking change, but it seems like forcing someone to add an argument (or a spread argument) to strongly type their arguments is worth it for the sanity of code.

[sandersn]

@kitsonk that’s a good argument. Unfortunately I didn’t record my previous reasoning in the PR #15849. I think it should be fine to make this an error, although I doubt our test suites will catch anything since this has only been legal since May.

[sandersn]

Nope, no failures from RWC or user code.

[sandersn]

Here's a counter-example that I thought about and then decided should still be illegal:

function f() { return arguments[0] }
declare var ns: number[];
f(...ns)

By analogy with what we allow in Javascript:

function j() { return arguments[0] }
j(1,2,3)

However, if you're using Typescript, or a version of Javascript that supports ... spread arguments, you also have ... rest parameters, and should always write f as function f(...args) { return args[0] }.

[sandersn]

Fix is up at #20071