Typescript: TS2.4 compile error with derived type assignment

[Diluka]

TypeScript Version: 2.4.1

Code

// A *self-contained* demonstration of the problem follows...
export class City extends BaseModel {
  //...
}

```ts
export abstract class BaseModel extends Model {
//...
}

```ts
export abstract class Model {
  //...
}

Expected behavior:
compile without error
Actual behavior:

error TS2345: Argument of type 'typeof City' is not assignable to parameter of type 'typeof Model'.
  Type 'City' is not assignable to type 'Model'.
    Types of property 'set' are incompatible.
      Type '{ <K extends "reload" | "isNewRecord" | "sequelize" | "aggregate" | "where" | "getDataValue" | "s...' is not assignable to type '{ <K extends "reload" | "isNewRecord" | "sequelize" | "aggregate" | "where" | "getDataValue" | "s...'. Two different types with this name exist, but they are unrelated.
        Type 'City' is not assignable to type 'Model'.

[felixfbecker]

This is related to using workarounds for static polymorphic this as described in https://github.com/Microsoft/TypeScript/issues/5863

See https://travis-ci.org/types/sequelize/builds/247636686

[sylvanaar]

You left out the code that would tell us the answer

Type '{ <K extends "reload" | "isNewRecord" | "sequelize" | "aggregate" | "where" | "getDataValue" | "s...' is not assignable to type '{ <K extends "reload" | "isNewRecord" | "sequelize" | "aggregate" | "where" | "getDataValue" | "s...'. Two different types with this name exist, but they are unrelated.

[felixfbecker]

It's in the build that I linked. Seems to be an issue with module resolution - but it worked before 2.4
Maybe correct the issue title?

[RyanCavanaugh]

You'll need to provide a repro that actually demonstrates the problem. Three empty class declarations is not sufficient.

[felixfbecker]

@RyanCavanaugh You can reproduce by cloning https://github.com/types/sequelize and running the commands from the failed build (npm install, tsc -p .). It compiles under TS 2.3 but 2.4 produces the posted error. Please let me know if there is anything I am doing wrong on my side.

[txiaocao]

oh my god....i dont know what happend.my project can not run..because of something package update typescript [email protected] by semantics version...

[kitsonk]

Almost all of these errors are actual errors in how people typed things. It may not be your code directly, but it maybe some sort of upstream system. Before 2.4, the type system was a bit lazy when it came to assessing the assignability of generics.

Likely somewhere City is being filled in a generic slot which before TypeScript didn't actually evaluate the assignability. The error Two different types with this name exist, but they are unrelated. can often be because you have duplicate versions of a package where that type comes from in two physically different locations where the TypeScript compiler is running, or actually those two types are coming from two separate definitions but are named the same. In some cases, especially with symbols and generic operators, TypeScript plays it "safe" by indicating it can't ensure that the types are the same, though they _might_ look the same.

Either way there is a problem in the code that likely needs to be addressed, because there is some sort of unsound type handling going on. Of course you can 🙈 🙉 🙊 and turn on --noStrictGenericChecks in 2.4 which should restore the behaviour of not being so strict.

[felixfbecker]

@kitsonk can you explain the wrong typing in this error?

error TS2684: The 'this' context of type 'typeof User' is not assignable to method's 'this' of type '(new () => User) & typeof Model'.
  Type 'typeof User' is not assignable to type 'typeof Model'

if User extends Model, then typeof User should be assignable to typeof Model, and it was in TS 2.3

[lumaxis]

@RyanCavanaugh I'm trying to update the Sequelize types to 2.4 but that is blocked by this issue. Does the public repo work as a repro case for you?

[RyanCavanaugh]

@lumaxis we generally don't have time to investigate large external repositories to diagnose user code unless there's a crash or other clear evidence of a TS bug. If there's something blocking you from creating a self-contained minimal repro I can help you with some tips if needed.

[mmitchellgarcia]

@RyanCavanaugh Hi Ryan - I created a self-contained repository with the mentioned packages above to demonstrate the issue. It is here: https://github.com/mmitchellgarcia/Derived-Type-Assignment-Error. If you run npm install then npm run build on that repo with Typescript 2.4.2, you'll get this issue:

src/MainModel.ts(20,19): error TS2345: Argument of type 'typeof AssociatedModel' is not assignable to parameter of type 'typeof Model'.
  Type 'AssociatedModel' is not assignable to type 'Model'.
    Types of property 'set' are incompatible.
      Type '{ <K extends "isNewRecord" | "sequelize" | "aggregate" | "where" | "getDataValue" | "setDataValue...' is not assignable to type '{ <K extends "isNewRecord" | "sequelize" | "aggregate" | "where" | "getDataValue" | "setDataValue...'. Two different types with this name exist, but they are unrelated.
        Type 'AssociatedModel' is not assignable to type 'Model'.
src/MainModel.ts(27,18): error TS2345: Argument of type 'typeof AdditionalModel' is not assignable to parameter of type 'typeof Model'.
  Type 'AdditionalModel' is not assignable to type 'Model'.
    Types of property 'set' are incompatible.
      Type '{ <K extends "isNewRecord" | "sequelize" | "aggregate" | "where" | "getDataValue" | "setDataValue...' is not assignable to type '{ <K extends "isNewRecord" | "sequelize" | "aggregate" | "where" | "getDataValue" | "setDataValue...'. Two different types with this name exist, but they are unrelated.
        Type 'AdditionalModel' is not assignable to type 'Model'.

[mhegazy]

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

[lumaxis]

@mhegazy I don't feel like this issue has been addressed, it's waiting for feedback from the team. Does @mmitchellgarcia example work as a repro for you?

[mhegazy]

Here is a simpler repro:

interface B  {
    increment(fields: Partial<B>): Promise<B>;
}

interface D  {
    increment(fields: "foo" | Partial<D>): Promise<D>;
    other: string;
}

declare var b: B;
declare var d: D;

b = d; // Error: D is not assignable to B

As work around for now, sequelize types can be changed to have increment and @decrement defined using overloads instead of union types:

  increment(fields: keyof this | (keyof this)[], options?: IncrementDecrementOptions): Promise<this>;
  increment(fields: Partial<this>, options?: IncrementDecrementOptions): Promise<this>;

[sandersn]

Here's an even smaller repro:

interface B  {
    increment(fields: B): B;
}

interface D extends B {
    increment(fields: "foo" | D): D;
    other: string;
}

[sandersn]

This error is technically correct; it's easy to construct an example that shows b = d is unsafe:

class B  {
    increment(fields: B): B {
        return fields;
    }
}

class D  {
    increment(fields: "foo" | D): D {
      if (typeof fields === "string") {
        return this
      }
      else {
        console.log(fields.other) // Runtime error if `fields: Base`
        return fields;
      }
    }
    other: string = "default";
}

var b: B = new D();
b.increment(new B()) // b is allowed here, but will be passed to D.increment

However, it seems like the unsafe relation is desired and used to work. I'll see why it stopped working.

[felixfbecker]

Yeah, that example seems like it _should_ error. But the Sequelize example that uses the polymorphic this shouldn't as far as I can see.

[sandersn]

The reason that @mhegazy's example doesn't fail in 2.3 is that "foo" is assignable to Partial<B> before 2.4. So Partial<D> | "foo" ==> Partial<B> succeeds. The weak type checking added in 2.4 makes it so that "foo" is not assignable to Partial<B> any more.

I'll take a look at the Sequelize example to see how it fails in 2.3 vs 2.4+.

[sandersn]

@felixfbecker Actually they have the same cause. this defeats the inheritance check because the compiler doesn't know what types will actually be instantiated for this and allows the inheritance. But when an actual assignment happens (or passing an argument), it can tell that the particular assignment is not safe. Compare these two examples:

Error on inheritance and assignability

interface B  {
    increment(fields: B): B;
}

interface D extends B { // error here
    increment(fields: D | "foo"): D;
    other: string;
}

declare var b: B
declare var d: D
b = d // error here

Error only on assignability

interface B  {
    increment(fields: this): this;
}

interface D extends B { // no error here
    increment(fields: this | "foo"): this;
    other: string;
}

declare var b: B
declare var d: D
b = d // error here

Notice that in the class-based example I gave above, even if you change the B and D types on increment to this, the code would still fail at runtime. The Model type hierarchy has the same error, despite using this.

I don't know Sequelize's code, but I suspect that it never does anything like

let m: Model = new MainModel();
m.set(new AssociatedModel());

It is probably careful to never mix different subclasses, so probably avoids runtime errors. The crux of the problem is that probably is not provably, and the compiler started complaining about provability in one lone place in Typescript 2.4.

[sandersn]

I tried relaxing the weak-type check for unions, although I'm not sure this is the right solution. This allows the small example to succeed but not the Sequelize example. I'll have to investigate further.

[sandersn]

I got Sequelize to work by turning off the weak-type check for unions both in covariant and contravariant relations.

After discussing it again with @mhegazy, we decided that the weak type check is working as intended, however. Weak type errors popped up lots of places in 2.4, and this error is not fundamentally different from the others. It's just obscured under several layers of complexity.

Since Sequelize likely is safe to use, even though it's unsound, I think the correct workaround is the one proposed [here -- split the offending increment and decrement methods] into overloads.