redux-orm broken by #43624

[sandersn]

The nightly dtslint run for 4/28 fails on redux-orm, with new errors on examples like:

export default class Model<MClass extends typeof AnyModel = typeof AnyModel, Fields extends ModelFieldMap = any> {
    // ...
    readonly ref: Ref<this>;
    // ...
}
export class AnyModel extends Model {}
export type Ref<M extends AnyModel> = {
    [K in keyof RefFields<M>]: ModelFields<M>[K] extends AnyModel ? IdType<ModelFields<M>[K]> : RefFields<M>[K];
};

ERROR: 118:23  expect  TypeScript@4.3 compile error: 
Type 'this' does not satisfy the constraint 'AnyModel'.
  Type 'Model<MClass, Fields>' is not assignable to type 'Model<typeof AnyModel, any>'.
    Type 'typeof AnyModel' is not assignable to type 'MClass'.
      'typeof AnyModel' is assignable to the constraint of type 'MClass', but 'MClass' could be instantiated with a different subtype of constraint 'typeof AnyModel'.

Almost certainly a result of https://github.com/microsoft/TypeScript/pull/43624, but it could be #42449 or #43835, since they went in on the same day.

If this is an intended result of that PR, can you fix up redux-orm? A naive change to Ref<M extends Model> doesn't work.

[weswigham]

I mean, the difference is that we're relating models by variance now, whereas before it was structural. Technically it's a regression, but I assume the actual issue is that the variance of the first type parameter is incorrectly measured as contravariant.

[sandersn]

So this is a case of fixing a bug, which exposes another bug? =(

[weswigham]

So, I minified this down and came to the conclusion... that the error is actually, sort-of, technically correct. The Model class should actually be invariant on its MClass type parameter (and any time we measure otherwise, strange edgecase of our typesystem is taking place) - it has a getClass function which returns (a conditional whose result changes based on a covariant derivative of) that type variable, and a set function which takes (a conditional whose result changes based on a contravariant derivative of) that type variable. Those combine to make this not assignable to Model<typeof Model> (the constraint of Ref) because this is some-arbitrary-subtype of Model, which therefore might resolve to differing branches in either of those two other members.

Here's a greatly simplified comparison, take this:

type ModelId<M extends ModelSub> = M; // just validates the input matches the `Model` type to issue an error
export declare class Model<MClass extends typeof ModelSub = typeof ModelSub> {
    class: MClass;
    readonly ref: ModelId<this>;
    set<K>(value: K extends ModelId<this>['class'] ? number : string): void;
}
export declare class ModelSub extends Model {}

and compare to

type ModelId<M extends Model> = M; // just validates the input matches the `Model` type to issue an error
export declare class Model<MClass extends typeof Model = typeof Model> {
    class: MClass;
    readonly ref: ModelId<this>;
    set<K>(value: K extends ModelId<this>['class'] ? number : string): void;
}

(so, the same, but with SubModel eliminated as a redundant, empty type). TS prior to #43624, we only issued an error on the inline'd (second) version, whereas now we appropriately issue an error on both! So at a minimum, we're certainly more consistent now. The question then becomes: "Is the error in the second version (and now in both versions) correct?" which, as I said above, in this simplified example, it's pretty easy to tell it could be construed to be - class is a covariant MClass usage, while set has an identity-requiring one (via the index on this inside a conditional extends).

Prior to the change, what'd happen is that the conditional in the set signature would be resolved on one side of the comparison (since the empty subtype has fully resolved type arguments, so there's no generics and no reason to defer the conditional) to a union of the possible outcomes, while on the source side, signature relating would erase the checkType to any, making the extends type irrelevant in the comparison, and allowing the default constraint to allow the conditionals to be assigned across, even though the branches might not overlap perfectly. Whereas, without the empty subtype (in all versions of TS, old and new), we launch into a variance measurement process for the Model type, which determines Model to be invariant on MClass (variance measurement will relate two generic conditionals within set, whose extends clauses don't match, and thus always fail assignment). Now, since the comparison can sometimes maybe work out because the extends clause is disregarded when the checkType is any, as when relating signatures, structurally, it really should be Unmeasurable or at least Unreliable and... drum roll... we have an extremely old outstanding PR that already adds that (which makes sense, since I was able to rephrase the root issue into something that behaved the same in old TS, too), and the PR very recently became unblocked: https://github.com/microsoft/TypeScript/pull/31277

Now... while that gets back the old no-error behavior for redux-orm (and then some), we could pick a bone about weather we're doing the right thing for signature relationships here, too, since as I said, the error (current master behavior) is actually pretty reasonable from the real structure - signature relating is just hiding it when a structural comparison is performed. (By erasing the signature-local type parameter, and thus erasing the entire "conditional" part of the conditional.) It would almost make sense to, rather than erase the parameters to any, instantiate them into a well-known type-parameter-assignable-to-all-type-parameters (almost like a reverse wildcardType!) so conditionals remain deferred... however the knock on effects of doing so for other signature relation comparisons are not immediately obvious.

TL;DR: Three step process where our behavior flip-flops in the middle.

1. For now, keep master as is, accepting new errors as better. These errors only appear when variance relationships are used, however.
2. Merge #31277 to remove the errors, making no errors appear when either structural or variance based checks occur, but
3. Since I feel the errors here are actually more correct (even if we're not getting them for all the right reasons), and we should follow up on #31277 by fixing structural signature relationship checking to not un-defer conditionals (and thus greatly weaken structural assignability checks involving conditionals in signatures) quite so haphazardly. Then we'll finally get the current behavior (an error) whenever a structural or variance based comparison is used.

If you agree the new errors are better, we can put off 2 and 3 for awhile if need be (though we should get them both done rapidly so we don't visibly flip-flop behavior). If you disagree, we can scratch 3, and we should probably work on merging #31277 sooner, since that'll remove the errors from the variance based checks, which effectively regains the old behavior. In both cases, #31277 is the next step here.

[sandersn]

Your recommended steps sound good. (2) and (3) are now combined in #43887, right? Let's do the following:

1. Keep master as is until we create the RC.
2. In the meantime, see if there's an easy-[ish] workaround for redux-orm. If not, add it to https://github.com/microsoft/DefinitelyTyped-tools/blob/master/packages/dtslint-runner/expectedFailures.txt and wait for an owner of the types to fix it. It's little used so that may never happen.
3. Get @ahejlsberg and @jack-williams to review #43887 and merge it for 4.4.