Skip to content
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

Comparison targets are reversed (regression) #31251

Open
falsandtru opened this issue May 4, 2019 · 10 comments
Open

Comparison targets are reversed (regression) #31251

falsandtru opened this issue May 4, 2019 · 10 comments
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Milestone

Comments

@falsandtru
Copy link
Contributor

falsandtru commented May 4, 2019

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

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

@jack-williams
Copy link
Collaborator

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

@falsandtru
Copy link
Contributor Author

falsandtru commented May 4, 2019

Can you try with 3.5.0-dev.20190504?

@falsandtru
Copy link
Contributor Author

ping @weswigham

@jack-williams
Copy link
Collaborator

jack-williams commented May 6, 2019

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

jack-williams commented May 6, 2019

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

falsandtru commented May 6, 2019

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

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

  • strictFunctionTypes is irrelevant here because call is a method.
  • Similarly, my example:
withString = withA; // correctly an error, otherwise we could make an illegal call through the alias

is not relevant because call is a method. It would be an error if call was an arrow function, but it's not and so should not error for your code.

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.

@weswigham weswigham added the Needs Investigation This issue needs a team member to investigate its status. label May 8, 2019
@falsandtru
Copy link
Contributor Author

@ahejlsberg Take a look?

@falsandtru
Copy link
Contributor Author

@ahejlsberg Can you also fix this regression?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
7 participants