microsoft / TypeScript

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

Comparison targets are reversed (regression) #31251

Open falsandtru opened 5 years ago

falsandtru commented 5 years ago

The similar problem #30118 had been fixed, but appeared again.

TypeScript Version: 3.4.0-dev.20190504

Search Terms:

Code

export abstract class Supervisor<N extends string, P = unknown, R = unknown> {
  private static instances_: Set<Supervisor<string, unknown, unknown>>;
  private static get instances(): typeof Supervisor.instances_ {
    return this.hasOwnProperty('instances_')
      ? this.instances_
      : this.instances_ = new Set();
  }
  constructor() {
    void (this.constructor as typeof Supervisor).instances.add(this);
  }
  public abstract call(name: N | ('' extends N ? undefined : never), param: P, timeout?: number): Promise<R>;
}

Expected behavior:

pass

Actual behavior:

Argument of type 'this' is not assignable to parameter of type 'Supervisor<string, unknown, unknown, unknown>'.
  Type 'Supervisor<N, P, R, S>' is not assignable to type 'Supervisor<string, unknown, unknown, unknown>'.
    Type 'string' is not assignable to type 'N'.
      'string' is assignable to the constraint of type 'N', but 'N' could be instantiated with a different subtype of constraint 'string'.ts(2345)

Playground Link:

Related Issues:

falsandtru commented 5 years ago

Note that a regression test for #30118 is already added but another case, posted here and there, is not covered by it.

jack-williams commented 5 years ago

Can't reproduce on 3.5.0-dev - is this behaviour in a release branch or at some intermediate point?

falsandtru commented 5 years ago

Can you try with 3.5.0-dev.20190504?

falsandtru commented 5 years ago

ping @weswigham

jack-williams commented 5 years ago

Just tried off master and I do get the error you link.

I presume you are running strictFunctionTypes?

This error looks correct to me [EDIT: for this specific example, but there may be something wrong for other examples. See next comment in thread]. The property instances is a set of Supervisor<string, unknown, unknown>, therefore allowing the method call to accept arguments of type string in the name position. The this instance is generic in the name parameter to call and may be instantiated with something more specific like "a", so it's not safe to assign this to Supervisor<string, unknown, unknown>.

A smaller example:

declare let withString: Supervisor<string>;
declare let withA: Supervisor<"a">;
withA.call("a", 'param'); // ok
withA.call("a string" as string, 'param'); // not ok
withString = withA; // correctly an error, otherwise we could make an illegal call through the alias
withString.call("a string" as string, 'param'); // no error
jack-williams commented 5 years ago

If you change call to be:

  public abstract call(name: ('' extends N ? undefined : never), param: P, timeout?: number): Promise<R>;

you still get the error, which is probably incorrect and due to an invariant variance calculation which has been turned on again for the conditional type extends type. Small repro:

interface A<T> {
    x: number extends T ? 1 : 1;
}

declare let a: A<number>;
declare let b: A<3>;

a = b; // error
b = a; // error

@weswigham Is it worth using your new unmeasurable markers for this?

// Two conditional types 'T1 extends U1 ? X1 : Y1' and 'T2 extends U2 ? X2 : Y2' are related if
// one of T1 and T2 is related to the other, U1 and U2 are identical types, X1 is related to X2,
// and Y1 is related to Y2.
const u1 = instantiateType((<ConditionalType>source).extendsType, reportUnmeasurableMarkers);
const u2 = instantiateType((<ConditionalType>target).extendsType, reportUnmeasurableMarkers);
    if (isTypeIdenticalTo(u1, u2) &&
    ...
falsandtru commented 5 years ago

I presume you are running strictFunctionTypes?

No: https://github.com/falsandtru/spica/blob/master/tsconfig.json

declare let withString: Supervisor; declare let withA: Supervisor<"a">; withA.call("a", 'param'); // ok withA.call("a string" as string, 'param'); // not ok withString = withA; // correctly an error, otherwise we could make an illegal call through the alias withString.call("a string" as string, 'param'); // no error

TypeScript originally hasn't supported this problem as Arrays also have the same problem. Therefore type checking is wrong or wrongly too strict in this case. So the error message may be correct but TypeScript shouldn't check it.

interface A { x: number extends T ? 1 : 1; }

declare let a: A; declare let b: A<3>;

a = b; // error b = a; // error

Does it also fix this issue?

jack-williams commented 5 years ago

Yep a couple of things wrong in my comments. Corrections:

Does it also fix this issue?

The minor suggestion I posted does fix my small repro and your example - though I'm not sure if it's the correct thing to do overall.

falsandtru commented 5 years ago

@ahejlsberg Take a look?

falsandtru commented 5 years ago

@ahejlsberg Can you also fix this regression?