Skip to content

polymorphic arguments validation error #20268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
amir-arad opened this issue Nov 26, 2017 · 6 comments · Fixed by #20455
Closed

polymorphic arguments validation error #20268

amir-arad opened this issue Nov 26, 2017 · 6 comments · Fixed by #20455
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue

Comments

@amir-arad
Copy link

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
Copy link
Author

currently blocks wix-incubator/wix-react-tools#188

@mhegazy
Copy link
Contributor

mhegazy commented Nov 27, 2017

Seems to have been caused by #17806. a possible solution here is to add an overload for (T | T[])[] or just make it (T | T[] | ReadonlyArray<T>)[]

@mhegazy mhegazy added the Bug A bug in TypeScript label Nov 27, 2017
@mhegazy mhegazy added this to the TypeScript 2.6.3 milestone Nov 27, 2017
@mhegazy mhegazy added the Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript label Nov 27, 2017
@mhegazy mhegazy assigned ghost Nov 27, 2017
@amir-arad
Copy link
Author

@mhegazy thanks for the workaround

@ghost
Copy link

ghost commented Nov 28, 2017

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.

@ghost ghost closed this as completed in #20455 Dec 7, 2017
@ghost ghost added the Fixed A PR has been merged for this issue label Dec 8, 2017
@mhegazy mhegazy modified the milestones: TypeScript 2.6.3, TypeScript 2.7 Jan 17, 2018
@ahejlsberg
Copy link
Member

ahejlsberg commented Feb 2, 2018

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 ahejlsberg added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Bug A bug in TypeScript labels Feb 2, 2018
@ahejlsberg
Copy link
Member

ahejlsberg commented Feb 2, 2018

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.

@ahejlsberg ahejlsberg added Bug A bug in TypeScript and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Feb 2, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue
Projects
None yet
3 participants