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

Assignability check not enough for class that is mutually recursive with an interface? #39947

Closed
uhyo opened this issue Aug 7, 2020 · 2 comments · Fixed by #41218
Closed
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@uhyo
Copy link
Contributor

uhyo commented Aug 7, 2020

TypeScript Version: 3.9.2, 4.0.1-rc

Search Terms: class no type error interface

Code

Unbelievably this is the minimal reproduction I was able to make.

interface I<Dummy, V> {
  c: C<Dummy, V>;
}

class C<Dummy, V> {
  declare sub: I<Dummy, V>;

  declare covariance: V;
}

// should be a type error, but none emitted
const c1: C<unknown, string> = new C<unknown, number>();

Expected behavior:

new C<unknown, number>() should not be assignable to C<unknown, string> due to existence of the covariance property.
TSC should emit an error like Type 'C<unknown, number>' is not assignable to type 'C<unknown, string>'.

Actual behavior:

No error.

Playground Link: link

Related Issues: none found (sorry, hard to explain the exact problem so no good search terms came up)

@jack-williams
Copy link
Collaborator

jack-williams commented Aug 9, 2020

I believe this regressed with #36261

I think the issue is that the recursion marker for variance (emptyArray) and the recursion tracker for checkTypeRelatedTo (depth) are not aligned. When checking variance for C, the variance for I is recursively calculated, however this occurs through a call to checkTypeRelatedTo which has a fresh depth variable.

Checking the variance for I recursively checks the variance for C, which has been marked as recursive by emptyArray, and under the new logic returns Maybe. The issue here is that all the various relation checks for C made when computing variance for I get cached because measuring variance for I happens through a new call to checkTypeRelatedTo. When finally calculating variance for V in C, all the variance probes hit the cache and succeed, marking V as independent.

A diagram would be:

getVariances for C
  mark C in recursive variance calc
  check that C_Sub is related to C_Super
    check C_Sub[sub] is related to C_Super[sub]
      getVariances for I
        mark I in recursive variance calc
        ...
        check that I_Indpendent is related to I_Super (new depth = 0)
          check I_Indpendent[c] is related to I_Super[c]
            C<Dummy, V_Indpendent> is maybe related to C<Dummy, V_Super> because of recursive variance. add as assumption
        I_Indpendent is maybe related to I_Super at depth 0, cache our maybe assumptions
  ...
  check that C_Super is related to C_Sub
    ...
    check that C<Dummy, V_Super> is related to C<Dummy, V_Sub>
      cache hit (related)
  check that C_Indpendent is related to C_Super
    ...
    check that C<Dummy, V_Indpendent> is related to C<Dummy, V_Super>
      cache hit (related)

  find that C is indpendent in V.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Aug 20, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.1.0 milestone Aug 20, 2020
@RyanCavanaugh RyanCavanaugh added Rescheduled This issue was previously scheduled to an earlier milestone and removed Rescheduled This issue was previously scheduled to an earlier milestone labels Aug 31, 2020
@ahejlsberg
Copy link
Member

@jack-williams Thanks for doing the detective work! I think the fix is to not record Ternary.Maybe results in the cache when recursion is encountered during variance measurement. I will put up a PR to that effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
5 participants