microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.33k stars 12.4k forks source link

Type inference for rest parameters does not work #41064

Closed ezsh closed 3 years ago

ezsh commented 3 years ago

TypeScript Version: 4.1.0-dev.20201012 Search Terms: type inference, rest parameters Consider the following function declaration:

declare function f<T>(...list: T[]): void;

Expected behavior: The function f() can receive arbitrary number of arguments of any type. For the given list of arguments type T should be set to the "least common denominator" of the argument types.

Actual behavior: TS fails to infer that common type by itself, but is happy to consume the same call when the common type is provided manually:

f("a", "b", 0); // TS thinks this call is illegal
f<string|number>("a", "b", 0); // yet it is happy to accept the same call with manual typing

The other case that works is when arguments contains the any type:

let a:any;
f("a", "b", 0, a);

Code

declare function f<T>(...list: T[]): void;

const ar = ["a", "b", 0];

f(...ar);
f<string|number>("a", "b", 0);

f("a", "b", 0);

let a:any;
f("a", "b", 0, a);
Output ```ts "use strict"; const ar = ["a", "b", 0]; f(...ar); f("a", "b", 0); f("a", "b", 0); let a; f("a", "b", 0, a); ```
Compiler Options ```json { "compilerOptions": { "noImplicitAny": true, "strictNullChecks": true, "strictFunctionTypes": true, "strictPropertyInitialization": true, "strictBindCallApply": true, "noImplicitThis": true, "noImplicitReturns": true, "alwaysStrict": true, "esModuleInterop": true, "declaration": true, "experimentalDecorators": true, "emitDecoratorMetadata": true, "moduleResolution": 2, "target": "ES2017", "jsx": "React", "module": "ESNext" } } ```

Playground Link: Provided

MartinJohns commented 3 years ago

This is intended, as mentioned in https://github.com/microsoft/TypeScript/issues/41048#issuecomment-707186879. Re-creating issues because you didn't like the response you got is icky.

ezsh commented 3 years ago

Thank you, I already know that the current behaviour is intended. I believe it is incorrect because f("a", "b", 0) and f<string|number>("a", "b", 0) are equivalent but the compiler treats them differently.

This issue is about another problem, so it is not a re-creation. BTW, clothing issues without explaining is also not the right way to communicate.

ezsh commented 3 years ago

Another illustration:

declare function f<T>(...list: T[]): T;

class A {
}

class B extends A {
}

class C extends A {
}

const t2 = f(new B(), new C(), null);

TS says type of t2 is C|null (B is missing).

RyanCavanaugh commented 3 years ago

Generic inference proceeds along the following algorithm:

This is the intended behavior because calls like this

declare function find<T>(haystack: T[], needle: T): boolean;
find([1, 2, 3], "four");

should fail; inferring number | string as the type argument here is actively working against the intent of the author. This is the intended behavior; we know this because TS used to do the other thing and we got nothing but complaints about it. So we changed this to be the intended behavior.

Now if you write

declare function find<T>(haystack: T[], needle: T): boolean;
const sn: Array<string | number> = [1, 2, 3];
find(sn, "four");

Now this is a valid call, because the candidate string | number is a valid supertype of all the candidates. This is the intended behavior because these types share a common root.

When you write something like this:

declare function f<T>(...list: T[]): void;

const ar = ["a", "b", 0];

f(...ar);

There's only one candidate presented here: string | number. There's no basis by which you could reject this; you could as easily have written

declare function f<T>(...list: T[]): void;

const ar = ["a" as string | number];

f(...ar);

which clearly must pass by your proposed logic of "they should behave the same", but the type of ar is the same in both cases.

So anyway, this is the intended behavior; you haven't found a six-year-old bug in TypeScript. You're free to disagree with that design but it's not a bug.

ezsh commented 3 years ago

Thank you for the explanation, your time, an with all due respect to the six years of the TypeScript history, but...

The system you outlined above is self-inconsistent and contradicts JavaScript semantic of the rest parameters (five years old feature). Consider the second example I gave, with three classes. In that example the types of the parameters are compatible and I would even be happy if the compiler deduces the parameter type T to be A|null. But it returns C|null. How is that possible?

Now, examples with two parameters are not very helpful, because they illustrate quite a different pattern, where two entitles are at play and there the compiler obtains the value for T from the first argument, tries to apply it to the second argument and that fails. Clear and simple. My examples contain a single entity (array) only. And from the runtime behaviour (i.e. the JavaScript run-time rules), this shall work in the very same way as it works for arrays. You don't expect us, users, to write

const array:(string|number)[] = ["a", 1];

do you?

Why then when the very same array is created implicitly TypeScript says it's impossible?

Arian94 commented 3 years ago

Thank you for the explanation, your time, an with all due respect to the six years of the TypeScript history, but...

The system you outlined above is self-inconsistent and contradicts JavaScript semantic of the rest parameters (five years old feature). Consider the second example I gave, with three classes. In that example the types of the parameters are compatible and I would even be happy if the compiler deduces the parameter type T to be A|null. But it returns C|null. How is that possible?

Now, examples with two parameters are not very helpful, because they illustrate quite a different pattern, where two entitles are at play and there the compiler obtains the value for T from the first argument, tries to apply it to the second argument and that fails. Clear and simple. My examples contain a single entity (array) only. And from the runtime behaviour (i.e. the JavaScript run-time rules), this shall work in the very same way as it works for arrays. You don't expect us, users, to write

const array:(string|number)[] = ["a", 1];

do you?

Why then when the very same array is created implicitly TypeScript says it's impossible?

TS has many issues and their developers are so full of themselves in a silly way. So don't be too positive about it.

RyanCavanaugh commented 3 years ago

@ezsh your classes are empty and that's the only reason you see that behavior. Add members to B / C and you will see the behavior you expect. This is addressed in the FAQ.

@Arian94 personal snipes aren't appreciated here

RyanCavanaugh commented 3 years ago

Regarding the second example, I'm not sure what you're arguing. "Consistency" is a proxy measure of a useful system; there are calls we want to disallow and initializations we want to allow. The rules are designed to produce desired outcomes and this does sometimes mean that for two different ways of doing the same thing, only one will be an error. Honestly it doesn't sound like TypeScript is a good fit for how you'd like a type system to behave; I'd recommend Hegel if you want a JS-based type checker that prioritizes theoretical principles above human factors.

ezsh commented 3 years ago

@RyanCavanaugh, I don't think it's that simple. Here is an example:

declare function f<T>(...list: T[]): T;

class A {
    propA: number;

    constructor() {
        this.propA = 2;
    }
}

class B extends A {
    prop: number;
    constructor() {
        super();
        this.prop = 1;
    }
}

class C extends A {
   f(){
   }
}

const t3 = f(new B(), new C());

All three classes are non-empty, TypeScript thinks the call is invalid, but there is even a common non-empty base class.

RyanCavanaugh commented 3 years ago

In a structural type system there's no theoretical reason to prefer inferring to A here over { } except as a heuristic that would just generate more perceived inconsistencies. TypeScript did in fact infer { } as the "common base type" when inference failed and, again, people complained about that endlessly because so many clearly-wrong calls were accepted under that rule.

ezsh commented 3 years ago

Regarding the second example, I'm not sure what you're arguing. "Consistency" is a proxy measure of a useful system; there are calls we want to disallow and initializations we want to allow.

I absolutely understand what type of errors/typos you want to catch.

The rules are designed to produce desired outcomes and this does sometimes mean that for two different ways of doing the same thing, only one will be an error. Honestly it doesn't sound like TypeScript is a good fit for how you'd like a type system to behave;

But it already behaves like I want it to, with array initialisation.

I'd recommend Hegel if you want a JS-based type checker that prioritizes theoretical principles above human factors.

Thank you.

ezsh commented 3 years ago

In a structural type system there's no theoretical reason to prefer inferring to A here over { } except as a heuristic that would just generate more perceived inconsistencies.

I'm arguing that the present set of decisions solves the problem at the wrong place. If you deduce T in the original example as string|number, errors can be caught where the values are used. In the very same way, if in the second example you deduce T to be B|C, that would be indistinguishable from A and would create a usable context. If I declare declare function g(...list: A[]):A;,g(new B(), new C()) will work, but if I copy a body from g to f<T>(...list: T[]):T, it results in error, while nothing changed semantically.

ezsh commented 3 years ago

Here is the sample:

function g(...list: A[]):A {
    console.log(list[0].propA);
    return list[0];
}

function h<T extends B|C>(...list: T[]):T {
    console.log(list[0].propA);
    return list[0];
}

const t3 = f(new B(), new C()); // error
const t4 = g(new B(), new C()); // OK
const t5 = h(new B(), new C()); // error
RyanCavanaugh commented 3 years ago

If you deduce T in the original example as string|number, errors can be caught where the values are used

The failure mode of this was addressed by the find example above. Generics do not always appear in the return position, and the return value is not always used in the first place. We studied hundreds of generic functions from every definition file on DefinitelyTyped and, when a type parameter appeared in two places, nearly every single usage was in a place where inferring to a union type would have been unsafe, unexpected, or undesirable.

Ultimately generic inference is an inference process that produces the type we think you wanted based on available information. Explicit type arguments exist for a reason -- there are cases where that inference is overspecific and you need to supply the "this is what I mean to have happen"

Arian94 commented 3 years ago

@ezsh your classes are empty and that's the only reason you see that behavior. Add members to B / C and you will see the behavior you expect. This is addressed in the FAQ.

@Arian94 personal snipes aren't appreciated here

There will be no snipes, if you don't hide yourself.

ezsh commented 3 years ago

I can't understand why do you keep referring to the find example when it works and fails in a different way comparing to the case I report.

Explicit type arguments exist for a reason -- there are cases where that inference is overspecific and you need to supply the "this is what I mean to have happen"

But then I have to care about this line if argument types get an update, and if the union of argument types shrinks, the compiler will not tell me that my manual type is too wide. In this particular case I would rather use an explicit array: f(...[<my arg list>]) to make it work.

We studied hundreds of generic functions from every definition file on DefinitelyTyped and, when a type parameter appeared in two places, nearly every single usage was in a place where inferring to a union type would have been unsafe, unexpected, or undesirable.

Seems like there is a difference between those functions, otherwise no study would be required. I still believe you correct the error at the wrong place. TypeScript provides ReadonlyArray already, why don't you add another array interface, like StrictlyTypedArray that implements the constrain (all elements can be converted to the type of the first one) you put unconditionally on rest parameters? It may be late to do that now, but the opposite type (UnconstrainedArray) would solve the issue and be visible in function interface? So people can declare f<T>(...list: UnconstrainedArray<T>[]):void to accept any mix of elements?

RyanCavanaugh commented 3 years ago

I can't understand why do you keep referring to the find example when it works and fails in a different way comparing to the case I report.

This is begging the question. Your assertion has been that a function f<T>(...list: T[]) should act as a singular inference candidate site so that a call like f(1, "foo") would infer as string | number, because it's "the same" as the array case. My assertion is that this is the same" as function f<T>(arg0?: T, arg1?: T, arg2?: T, arg3?: T, ... and should be processed that way. Whether or not these examples are truly isomorphic is just a question of where we park certain inconsistencies that arise from a) accepting heterogeneous arrays and b) wanting to reject bad find calls.

I still believe you correct the error at the wrong place.

We're just going to have to agree to disagree here. This is basically the same as #27859 so you can add your use case there for tracking purposes, but I don't think we're going to completely rethink inference based on this one use case.

ezsh commented 3 years ago

But heterogeneous arrays are already accepted by TypeScript, and given that if you process f<T>(...list: T[]) in the way you wrote above, f<T>(list: T[]) becomes less strict about argument types then f<T>(...list: T[]), which is somehow illogical.

typescript-bot commented 3 years ago

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.