Spreads instantiated with array subtypes types exhibit bad behavior

[DanielRosenwasser]

Found by playing around with @mattmccutchen's example at #26013 (comment):

interface CoolArray<E> extends Array<E> { 
    hello: number;
}

declare function foo<T extends any[]>(cb: (...args: T) => void): void;

foo<CoolArray<any>>();    // error
foo<CoolArray<any>>(100); // error
foo<CoolArray<any>>(foo); // no error!!

Expected: All of these calls produce an error and have a good error message.
Actual: The first two have bad errors, the last one is considered okay.

[DanielRosenwasser]

It might be difficult, but I think ideally the user would get an error message on the type argument during instantiation; something along the lines of

'{0}' is used as a spread parameter type and cannot be instantiated with this type.
  Only arrays and tuple types may be used as spread parameter types.

[ahejlsberg]

The tricky issue here is that constraints can only limit us to subtypes of arrays, but not specifically to instantiations of Array<T> or a tuple type. It would effectively require a new type of constraint to completely tighten this down. Erroring during instantiation is really a non-option because we simply aren't equipped to pinpoint the source location that caused the error. A core assumption is that instantiation is error free and that any errors are caught during constraint checking.

[RyanCavanaugh]

The last call should not be an error

[ahejlsberg]

Here's a more obvious example of the issue:

function bar<T extends any[]>(...args: T): T {
    return args;
}

let a = bar(10, 20);  // Ok: [10, 20]
let b = bar<CoolArray<number>>(10, 20);  // Bad: CoolArray<number>

We'll never infer anything but instantiations of Array<T> or tuple types, but it is possible to explicitly specify a subtype. However, JavaScript always creates an array instance for a rest parameter, so there is no way args could actually be such a subtype.

[DanielRosenwasser]

@RyanCavanaugh why do you think so? foo shouldn't have a parameter list that can be inferred to a non-array type.

[RyanCavanaugh]

@DanielRosenwasser consider this code, which is representative of many functional libraries:

function fn<T extends any[]>(cb: (...args: T) => void, arr: T): T {
    cb.call(null, arr);
    return arr;
}

type MyTaggedArray = Array<number> & { "I promise it's sorted" };
const arr = [] as MyTaggedArray;
const arr2 = fn(x => x, arr);

There is nothing wrong with this code!

The error in Anders' example should be that the [10, 20] argument list is not assignable to CoolArray, not that the declaration of bar is somehow intrinsically wrong.

[ahejlsberg]

To recap, the problem in this issue as well as #26491 is that we have no expressible constraint that limits type parameters to only be instantiated with Array<T> or tuple types. What we can do however is detect situations where a spread parameter isn't exactly of one of those types and substitute never[] for that spread parameter type. This at least produces the expected errors in the examples above, although the appearance of a never type might be surprising.

[mattmccutchen]

If we change the rest parameter to never[], then call sites that pass zero arguments to the rest parameter will go unnoticed, right? ISTM it would be better to change the rest parameter to never.

[ahejlsberg]

@mattmccutchen It's more like if we have a union of all tuple types, we should substitute a tuple type with all never elements of a length that matches the shortest of those tuple types. Otherwise, we should substitute never[].

[mattmccutchen]

@ahejlsberg To emphasize the point, if we substitute never[], then the following code compiles (because the empty array is of type never[]) but accesses a nonexistent property at runtime:

interface CoolArray<E> extends Array<E> { 
    hello: number;
}
function bar<T extends any[]>(...args: T): T {
    return args;
}
console.log(bar<CoolArray<number>>().hello);

If we substitute never, then we get an error that the empty array isn't assignable to never. If you're concerned that we should only substitute a valid array or tuple type, then [never] should be necessary and sufficient to ensure that the call always generates a compile error (unless a never value or, alas, an any value is already in scope).

[mattmccutchen]

On second thought, changing a generic rest parameter to something never-y only when it is instantiated with something that is definitely not an array or tuple type does not fix the entire problem. Consider:

interface CoolArray<E> extends Array<E> { 
    hello: number;
}
function bar<T extends any[]>(...args: T): T {
    return args;
}
function baz<T extends any[]>(array: T): T { 
    return bar(...array);
}
declare const ca: CoolArray<number>;
console.log(baz(ca).hello);

The sound (but maybe annoying) approach would be to define a helper type alias Spread<T> that represents the type you get at runtime by spreading a T and then require that a generic rest parameter be of type Spread<T> for some T. Interestingly, this would also fix #26013 because a rest parameter of any[] is compatible with Spread<T> for every T according to the parameter list comparison rules, so we could allow the compatibility even if T is unknown. The definition of Spread<T> is approximately:

type Spread<T extends any[]> = Pick<T, keyof T & number | keyof any[]>;

but ideally we'd like it to always return a real array or tuple type even if T wasn't one.

[ahejlsberg]

I think I have the right solution figured out.

In cases where a rest parameter type is not an Array<T> or a tuple type, we need to collect the corresponding remainder of the argument list as a tuple type and then check that it is assignable to the rest parameter type. We already do that when the rest parameter type is a type parameter, we just need to do it in more cases.

In the CoolArray<E> case this means you'll consistently get an error because no tuple type is assignable to CoolArray<E>. In cases where the rest parameter type is a union of tuple types (as in #26491), you'll get exactly the expected behavior: Either success because the tuple collected from the remainder of the argument list matches, or an assignability error that elaborates why that tuple is not assignable to the union.

I'll be putting up a PR with this behavior.

[mattmccutchen]

@ahejlsberg Isn't it a little early to add the "Fixed" label?

[ahejlsberg]

@mattmccutchen Unless we introduce a whole new kind of constraint I'm not sure we can do any better than #26676, and even then it would be a breaking change since it would be stricter than the assignable to any[] requirement we have now. I think scenarios such as the following need to work:

function call<T extends unknown[], U>(f: (...args: T) => U, ...args: T) {
    return f(...args);
}

which means spreading a T needs to be of type T. As I see it, the only way to tighten it all down is to catch it early by having a constraint that only permits Array<T> or tuples. But given how close we are with the current solution and how hard you have to work to expose the unsoundness, I think we're fine.

[mattmccutchen]

@ahejlsberg For completeness, I want to point out that there is another solution without introducing a new kind of constraint and even without breaking the specific example you gave. (Some other code might break but would be straightforward to fix manually; in principle, the change could be put behind a new strict flag.) Define a built-in type alias Spread<T> that gives the type that results from spreading a T (like I proposed above), and when a rest parameter is declared as some type expression T, actually use Spread<T> as its type. Then the example works because the declared rest parameters ...args: T are both interpreted as ...args: Spread<T>. When call spreads its args, the result is Spread<Spread<T>>; the compiler would have to be able to reason that that equals Spread<T>, which is what f wants. But in my unsound example, bar would fail to compile because its parameter is implicitly changed to Spread<T>, while the return type is still T.

I'm guessing you're not going to want to do this, but I still wanted to point out that it would be possible.

[RyanCavanaugh]

@mmitche FWIW a lot of us use "Open + Fixed" to mean "Has an open PR to address the issue" for tracking purposes