Array.prototype.concat definition improvement

[Ghabriel]

Fixes #19535.

The compiler found some strange compatibility issues with the proposed solution by @mhegazy, as noted here. The alternative solution I used seems to work equally well.

Notable baseline change: the testcase at tests/cases/conformance/es6/spread/iteratorSpreadInArray6.ts no longer has errors... and IMO that's correct since Array.prototype.concat is a lot more flexible now.

[msftclas]

All CLA requirements met.

[mhegazy]

first you do not need the first overload with this change. this one covers it already. second, we intentionally did not add this overload to avoid heterogeneous array creation. we think most ppl think of concat as maintaining the type of the correct array and adding elements of a different type is expected to be an error.

[Ghabriel]

Regarding the unnecessary overload: indeed, I'll remove it. Regarding the heterogeneous array creation: the problem with not adding this overload is that it can lead to unintended heterogeneous array creation, as shown in the original issue. I don't agree that "most people think of concat as maintaining the type of the array", since it creates a new array. People with this kind of "false expectation" will likely get an error when they try to use the created array, so I don't see it as a problem...

[RyanCavanaugh]

@Ghabriel can you resolve the merge conflicts please? We'd like to run this against our real world code suite and see the impact and then decide whether this is a good change in terms of possible breaks. Thanks!

[Ghabriel]

Sorry for the long wait... I've fixed the merge conflicts but got some CI issues. I'll see if I can fix them this weekend.

[Ghabriel]

@RyanCavanaugh I've fixed all merge conflicts. Sorry for the delay!

[RyanCavanaugh]

@typescript-bot test this

[typescript-bot]

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 410b5cd. You can monitor the build here. It should now contribute to this PR's status checks.

[RyanCavanaugh]

@typescript-bot test this again

[typescript-bot]

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 410b5cd. You can monitor the build here. It should now contribute to this PR's status checks.

[andersk]

This fails to disallow

const a: number[][] = [[1]].concat([[2]], [3])
// → [[1], [2], 3]

(Personally, I think we should remove that other overload concat(...items: (T | ConcatArray<T>)[]): T[]. Yeah it happens to run in JavaScript—if T is not itself an array type as in the above example. But we disallow 10 > "9" even though it happens to run in JavaScript, because it’s confusing; you should have written 10 > +"9" if that’s what you meant. We should similarly disallow [1].concat([2], 3) because you should have written [1].concat([2], [3]).)

[RyanCavanaugh]

This turns out to have some fairly gnarly breaks in RWC. We'd prefer to leave as-is for now