microsoft / TypeScript

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

polymorphic arguments validation error #20268

Closed amir-arad closed 6 years ago

amir-arad commented 7 years ago

TypeScript Version: 2.7.0-dev.20171126 (Introduced in 2.6)

Code

type Processor<T extends object> = <T1 extends T>(subj: T1) => T1
function doStuff<T extends object, T1 extends T>(parentProcessors: Array<Processor<T>>, childProcessors : Array<Processor<T1>>) {
    childProcessors.concat(parentProcessors);
}

Expected behavior: pass validation (polymorphism etc.)

Actual behavior: Error:(3, 28) TS2345:Argument of type 'Processor[]' is not assignable to parameter of type 'Processor | ReadonlyArray<Processor>'. Type 'Processor[]' is not assignable to type 'ReadonlyArray<Processor>'. Types of property 'indexOf' are incompatible. Type '(searchElement: Processor, fromIndex?: number | undefined) => number' is not assignable to type '(searchElement: Processor, fromIndex?: number | undefined) => number'. Types of parameters 'searchElement' and 'searchElement' are incompatible. Types of parameters 'subj' and 'subj' are incompatible. Type 'T1' is not assignable to type 'T1'. Two different types with this name exist, but they are unrelated. Type 'T' is not assignable to type 'T1'. Type 'object' is not assignable to type 'T1'.

amir-arad commented 7 years ago

currently blocks wix/wix-react-tools#188

mhegazy commented 7 years ago

Seems to have been caused by https://github.com/Microsoft/TypeScript/pull/17806. a possible solution here is to add an overload for (T | T[])[] or just make it (T | T[] | ReadonlyArray<T>)[]

amir-arad commented 7 years ago

@mhegazy thanks for the workaround

ghost commented 7 years ago

This error appears to have to do with --strictFunctionTypes making an array not usable as a ReadonlyArray of a subtype -- despite the fact that reading out a subtype does work.

type Processor<T extends object> = <T1 extends T>(subj: T1) => T1
function doStuff<T extends object, T1 extends T>(parentProcessors: Array<Processor<T>>, childProcessors : Array<Processor<T1>>) {
    const x: ReadonlyArray<Processor<T1>> = parentProcessors; // This is an error...
    const foo: Processor<T1> = parentProcessors[0]; // But this does work?
    childProcessors.concat(x);
}
src/a.ts(12,11): error TS2322: Type 'Processor<T>[]' is not assignable to type 'ReadonlyArray<Processor<T1>>'.
  Types of property 'includes' are incompatible.
    Type '(searchElement: Processor<T>, fromIndex?: number | undefined) => boolean' is not assignable to type '(searchElement: Processor<T1>, fromIndex?: number | undefined) => boolean'.
      Types of parameters 'searchElement' and 'searchElement' are incompatible.
        Types of parameters 'subj' and 'subj' are incompatible.
          Type 'T1' is not assignable to type 'T1'. Two different types with this name exist, but they are unrelated.
            Type 'T' is not assignable to type 'T1'.
              Type 'object' is not assignable to type 'T1'.

If we remove includes, indexOf, and lastIndexOf from ReadonlyArray there is no error.

ahejlsberg commented 6 years ago

This actually isn't a bug. But it is complicated.

First, imagine that Processor<T> is written as just

type Processor<T extends object> = (subj: T) => T;

This function is invariant for T because it uses T in both input and output positions. Given a types A and B where one is a subtype of the other, neither Processor<A> nor Processor<B> is assignable to the other, and indeed we error if you try.

Now, changing Processor<T> to the original declaration

type Processor<T extends object> = <T1 extends T>(subj: T1) => T1;

This shouldn't really change matters. The function is still invariant for T. However, the type checker has limited capabilities when it comes to checking assignability of function types with generic signatures that differ only in their constraints, so we consider Processor<A> and Processor<B> assignable to each other when really they shouldn't be. But, for some reason, we do end up realizing that Array<Processor<A>> is not assignable to ReadonlyArray<Processor<B>> and that Array<Processor<B>> is not assignable to ReadonlyArray<Processor<A>>. And that's what happens in the original example (where T and T1 in doStuff are like A and B). I'm not 100% clear on how we get there, but it definitely should be an error. The issue here is actually that other cases should also be errors.

My conclusion is that the error in the original example is correct, and that the "fix" in #20455 isn't actually a fix, but rather a change that causes us to not error when we still should.

I think the correct course of action is to reverse #20455 and leave things the way they were.

ahejlsberg commented 6 years ago

I was right that it is complicated. But I was wrong about it not being a bug. It is.

The original Processor<T> is naturally contra-variant, not invariant as I was arguing above. Intuitively, it is a function that always returns the same type as you pass it, but what you can pass it is constrained, and the constraint acts in a contra-variant manner (i.e. a Processor<A> can safely be treated as a Processor<B>, but not vice versa).

The actual issue is a combination of two changes: #15104 (covariant checking for callback parameters) and #17806 (arguments to concat should be ReadonlyArray<T>). Because of #17806 we end up in a structural comparison of Array and ReadonlyArray when checking a call to concat. During this structural comparison we treat the searchElement parameter of the indexOf method as a callback parameter and only perform a co-variant comparison, and this comparison fails.

It is actually correct that there is an error. What's wrong is the way indexOf is declared to take T in an input (contra-variant) position, which technically makes Array<T> and ReadonlyArray<T> invariant. That's usually masked by the fact that we always compare methods bivariantly, but in this scenario we don't because of #15104. We've previously discussed changing indexOf (and the others like it) to be properly co-variant, but we'd need the ability to specify super constraints or simply resort to type any, and we didn't want to go there.