Overloads in Array.concat now handle ReadonlyArray

[sandersn]

Previously it was union types, which is slower.

[ahejlsberg]

I'm not a fan of adding more overloads. Instead, I propose we introduce a new type InputArray<T> and use that in place of T[] | ReadonlyArray<T> in the original two overloads.

interface InputArray<T> {
    readonly length: number;
    readonly [n: number]: T;
    join(separator?: string): string;
}

/**
  * Combines two or more arrays.
  * @param items Additional items to add to the end of array1.
  */
concat(...items: InputArray<T>[]): T[];
/**
  * Combines two or more arrays.
  * @param items Additional items to add to the end of array1.
  */
concat(...items: (T | InputArray<T>)[]): T[];

This works because InputArray<T> omits the methods that cause ReadonlyArray<T> and Array<T> to become invariant in --strictFunctionTypes mode. I have kept the join method in InputArray<T> to ensure that it isn't compatible with the string type or other types that might look like arrays. Now, technically this is slightly less type-safe than using T[] | ReadonlyArray<T>, but not in any meaningful fashion.

The baselines look fine with this change and checker performance when compiling the compiler improves by as much as 10%!

[mhegazy]

sounds good to me.

[sandersn]

@DanielRosenwasser and I considered this approach but didn't use it because it didn't show as much performance improvement as the current one.
However, I double-checked with the cleaned-up perf tests and confirmed that the performance improvement for the compiler is around 10%. It barely helped the other projects, except for TFS. This includes compiler-with-unions.

[sandersn]

Performance is the same between the additional overloads and the additional interface. I would say it's down to which we think is easier to understand in quickinfo.

[sandersn]

After some discussion with @weswigham, he points out that the additional interface is more dangerous because you can satisfy it with { length: 1, join(s) { return s } }, which will fail at runtime. Almost anything works; you just need a join(string): string method.

[ahejlsberg]

@mhegazy @sandersn I looked at the original issue (#20268) in more depth and I'm now convinced that it isn't a bug. See this comment #20268 (comment).

[ahejlsberg]

Nevermind, #20268 is indeed a bug as I explain here #20268 (comment).

I think the best way to fix it is the InputArray<T> that omits indexOf and the other methods that cause invariant behavior. If we're concerned about false matches, we can add the slice method as well:

interface InputArray<T> {
    readonly length: number;
    readonly [n: number]: T;
    join(separator?: string): string;
    slice(start?: number, end?: number): T[];
}

I think it is extremely unlikely something will match by accident. And, even with the current definitions something could match by accident. That's the nature of structural typing.

[sandersn]

I don't think making the interface one method larger buys us that much safety in practice. I'd prefer to leave InputArray as it is.

[sandersn]

This is now in 2.7 as well.