Confusing type error message in concat

[elbarzil]

Typechecking this code:

var x: string[] = ["A","B","C"];
x = x.concat([1,2,3])

will throw up with

Argument of type 'number[]' is not assignable to parameter of type 'string'.

This happens even without the x = assignment, and that shows that it would make more sense if the error was either of:

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

or

Argument of type 'number[]' is not assignable to parameter of type 'string[]'.

[ahejlsberg]

Agreed. The Array<T> interface declares concat as two overloads:

interface Array<T> {
    // ...
    concat<U extends T[]>(...items: U[]): T[];
    concat(...items: T[]): T[];
}

Those declarations were written before we permitted union types and used a generic constraint to work around some other historical issues. We need to update the declaration to this instead:

interface Array<T> {
    // ...
    concat(...items: (T | T[])[]): T[];
}

This will also allow us to correctly check an argument list that mixes single elements and arrays, such as numberArray.concat(1, [2, 3, 4], 5, [6]). (Now, technically arrays can be nested to any level in the argument list, but declaring that would require an intermediate recursive generic type and would adversely affect type inference, so I would just keep it simple like the above.)

[SlurpTheo]

This is a good/simple correction that needs to happen (I figured I must have had a head cold Friday when the playground kept complaining to me about const x = [1].concat(2, [3, 4])).

How about just "deprecating" Array<T>::concat() in TypeScript though? We have Array<T>::push() aided by SpreadElementExpression-awesomeness. What besides number[] could a code reviewer expect x to be here?

const x = [1];
x.push(2, ...[3, 4]);

Clear. Now, as for:

var x = [1];
x = x.concat(2, [3, 4]);

-OR- (my historical, personal problem with concat):

var x = [1];
x.concat(2, [3, 4]);  // forgot that I have to assign the expression result back over x

Ick.

... I don't like concat (from Node.js v4.2.6 REPL):

>
> [1].concat(2, [3])
[ 1, 2, 3 ]
>
> [1].concat(2, [3], [[4]])
[ 1, 2, 3, [ 4 ] ]
>

[elbarzil]

@ahejlsberg, there is a subtle point there with what concat is doing, where something like:

var a: number[][] = [[1],[2]];
a.concat([9]);

should be a type error because JS would add just 9. Hopefully the order of the T and T[] would make that work both for type errors and for the above (being an error too). And there's no deeper nesting of concat arguments, IIRC... (Otherwise this could be a problem in a way that I ran into with flatten in Typed Racket, BTW.)

@SlurpTheo: I don't know how things are done in JS VMs, but a spread that gets translated to apply might be either more limited or less efficient since it passes a huge number of arguments instead of keeping them in the original array. That's with the obvious difference of concat being functional and push modifying the array.

[LPGhatguy]

I'll take a crack at this issue!

[LPGhatguy]

@ahejlsberg would removing the generic variant of concat be a breaking change?

[SlurpTheo]

So, the issue isn't simply that TypeScript isn't accepting enough input scenarios; it's that TypeScript as-is today is already type-incorrect when calling Array<T>.concat() for a T which is an array of more than one dimension... yikes!

const x: number[][] = [[1]];
const y: number[][] = x.concat([2]);

Execution-result in JavaScript (from Node.js v4.2.6 REPL):

>
> x = [[1]]
[ [ 1 ] ]
> x.concat([2])
[ [ 1 ], 2 ]
>

[DanielRosenwasser]

Hmm, the proposed fix doesn't actually address that either.

[LPGhatguy]

Is there a solution to that problem that wouldn't introduce extra typing complexity? JS's handling of Array.concat with arrays-of-arrays is a mucky edge case!

[DanielRosenwasser]

I discussed something with @RyanCavanaugh offline. I was wondering whether @sandersn's this-typing could help us here at all when combined with type parameter constraints.

[sandersn]

What would concat's this be if not Array? My first guess would be
concat<U>(this:U, ...args:U|U[])

[DanielRosenwasser]

I had something like

interface Array<T> {
    concat<U, ArrayElement extends Array<U>>(this: ArrayElement[], ...args: (ArrayElement | ArrayElement[])[]): ArrayElement[];
}

which partially almost works kinda-sorta.

[sandersn]

I don't think that buys us anything beyond Ander's first proposal. It just binds U locally instead of inheriting it from the containing class.

[DanielRosenwasser]

Thanks @LPGhatguy!

[SlurpTheo]

If T is number[] why should someone be allowed to pass in something of type number[] instead of requiring them to pass number[][]? The return type (if allowed to pass number[]) really should not be number[][] as the type of the array created by JavaScript is actually (number | number[])[].

Why not be more restrictive/"limiting"/safe?

interface Array<T> {
    // ...
    concat(...items: T[][]): T[];
}

Has there been talk of creating a ~"use strict"; ("use explicit;"?) directive/flag to opt-in to only allowing type-safe/good programming and dump (more) historical JavaScript baggage? For example, I'd totally sign-up for more Ridiculousness Blocking™ right at the point of TypeScript compilation/Visual Studio IDE squiggly-feedback:

let testStr = "ABCDEFG";
testStr[1] = "6";      // Ick, the 2nd character isn't updated

const testNumber = 1234;
testNumber[2];         // Ick, this returns undefined

let myNum = 1;
let myAnyString: any = "hi";
myNum += myAnyString;  // Ick, my "number" variable contains "1hi"

Not really sure how any such directive/flag would effect a definition file change like this, but seems similarish in my mind.

[LPGhatguy]

@SlurpTheo I feel that would be the job of a linter like tslint, not the base compiler.

[aluanhaddad]

@SlurpTheo I prefer concat because it does not modify the original array. At any rate, the two are not equivalent.