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

Allow variance annotations on generic references #56418

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

JoshuaKGoldberg
Copy link
Contributor

Fixes #56390

Adds ObjectFlags.Reference to the allowed object flags, to encompass generic interfaces. Also adds tests to ensure that multi-layered type references to object or primitive (number) shapes still error only when they should.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Nov 15, 2023
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review November 15, 2023 23:21
@jakebailey
Copy link
Member

@typescript-bot test top200
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot perf test this
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 15, 2023

Heya @jakebailey, I've started to run the regular perf test suite on this PR at 79e0c3a. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 15, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 79e0c3a. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 15, 2023

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 79e0c3a. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 15, 2023

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 79e0c3a. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 15, 2023

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 79e0c3a. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 15, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/158624/artifacts?artifactName=tgz&fileId=1332F4A856A6ED1D3815FCF1733754B86EC8BA9B54E73DADAEEB2B182714BA4502&fileName=/typescript-5.4.0-insiders.20231115.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/56418/merge:

There were infrastructure failures potentially unrelated to your change:

  • 2 instances of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,199k (± 0.01%) 295,160k (± 0.01%) ~ 295,102k 295,179k p=0.128 n=6
Parse Time 2.64s (± 0.20%) 2.63s (± 0.44%) -0.01s (- 0.44%) 2.61s 2.64s p=0.039 n=6
Bind Time 0.82s (± 0.00%) 0.82s (± 0.63%) ~ 0.82s 0.83s p=0.174 n=6
Check Time 8.04s (± 0.37%) 8.05s (± 0.31%) ~ 8.02s 8.09s p=0.809 n=6
Emit Time 7.09s (± 0.30%) 7.08s (± 0.26%) ~ 7.05s 7.10s p=0.293 n=6
Total Time 18.59s (± 0.15%) 18.58s (± 0.21%) ~ 18.55s 18.65s p=0.418 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,620k (± 1.63%) 191,664k (± 1.23%) ~ 190,663k 196,486k p=0.149 n=6
Parse Time 1.36s (± 1.14%) 1.36s (± 0.93%) ~ 1.35s 1.38s p=0.928 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.57%) ~ 0.72s 0.73s p=0.405 n=6
Check Time 9.18s (± 0.11%) 9.15s (± 0.34%) -0.04s (- 0.42%) 9.11s 9.19s p=0.043 n=6
Emit Time 2.64s (± 0.72%) 2.63s (± 0.59%) ~ 2.62s 2.66s p=0.448 n=6
Total Time 13.90s (± 0.24%) 13.86s (± 0.33%) ~ 13.81s 13.92s p=0.127 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,355k (± 0.00%) 347,343k (± 0.01%) ~ 347,298k 347,386k p=0.575 n=6
Parse Time 2.46s (± 0.36%) 2.45s (± 0.49%) ~ 2.43s 2.46s p=0.437 n=6
Bind Time 0.93s (± 0.44%) 0.93s (± 0.56%) ~ 0.92s 0.93s p=0.595 n=6
Check Time 6.92s (± 0.50%) 6.91s (± 0.52%) ~ 6.86s 6.96s p=0.629 n=6
Emit Time 4.05s (± 0.58%) 4.05s (± 0.35%) ~ 4.03s 4.07s p=1.000 n=6
Total Time 14.36s (± 0.32%) 14.34s (± 0.17%) ~ 14.31s 14.37s p=0.333 n=6
TFS - node (v18.15.0, x64)
Memory used 302,622k (± 0.00%) 302,628k (± 0.00%) ~ 302,606k 302,641k p=0.423 n=6
Parse Time 2.00s (± 0.60%) 2.00s (± 0.97%) ~ 1.98s 2.03s p=0.806 n=6
Bind Time 1.00s (± 0.98%) 1.00s (± 0.98%) ~ 0.99s 1.02s p=1.000 n=6
Check Time 6.25s (± 0.48%) 6.26s (± 0.33%) ~ 6.24s 6.29s p=0.516 n=6
Emit Time 3.56s (± 0.25%) 3.57s (± 0.62%) ~ 3.54s 3.60s p=0.368 n=6
Total Time 12.82s (± 0.32%) 12.84s (± 0.28%) ~ 12.80s 12.89s p=0.377 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,564k (± 0.00%) 470,544k (± 0.01%) ~ 470,509k 470,585k p=0.109 n=6
Parse Time 2.57s (± 0.45%) 2.57s (± 0.74%) ~ 2.55s 2.60s p=1.000 n=6
Bind Time 0.99s (± 0.64%) 0.99s (± 0.64%) ~ 0.98s 1.00s p=1.000 n=6
Check Time 16.73s (± 0.33%) 16.66s (± 0.36%) ~ 16.60s 16.77s p=0.062 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.29s (± 0.33%) 20.22s (± 0.26%) -0.07s (- 0.36%) 20.18s 20.32s p=0.044 n=6
xstate - node (v18.15.0, x64)
Memory used 512,822k (± 0.01%) 512,814k (± 0.01%) ~ 512,742k 512,908k p=0.936 n=6
Parse Time 3.28s (± 0.19%) 3.27s (± 0.17%) -0.02s (- 0.46%) 3.26s 3.27s p=0.007 n=6
Bind Time 1.54s (± 0.58%) 1.54s (± 0.49%) ~ 1.53s 1.55s p=0.798 n=6
Check Time 2.86s (± 0.76%) 2.87s (± 0.52%) ~ 2.85s 2.89s p=0.373 n=6
Emit Time 0.08s (± 4.99%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=0.405 n=6
Total Time 7.76s (± 0.26%) 7.75s (± 0.11%) ~ 7.74s 7.76s p=0.607 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,375ms (± 0.80%) 2,359ms (± 0.44%) ~ 2,346ms 2,374ms p=0.092 n=6
Req 2 - geterr 5,341ms (± 1.05%) 5,406ms (± 1.39%) +65ms (+ 1.22%) 5,321ms 5,483ms p=0.031 n=6
Req 3 - references 325ms (± 0.75%) 327ms (± 1.68%) ~ 323ms 337ms p=0.627 n=6
Req 4 - navto 279ms (± 0.73%) 276ms (± 1.19%) ~ 273ms 279ms p=0.085 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 81ms (± 6.09%) 84ms (± 7.22%) ~ 75ms 90ms p=0.367 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,492ms (± 0.94%) 2,484ms (± 0.78%) ~ 2,454ms 2,513ms p=0.336 n=6
Req 2 - geterr 4,081ms (± 1.88%) 4,080ms (± 1.83%) ~ 4,015ms 4,178ms p=1.000 n=6
Req 3 - references 340ms (± 1.53%) 340ms (± 1.64%) ~ 332ms 344ms p=0.511 n=6
Req 4 - navto 284ms (± 0.53%) 283ms (± 0.36%) ~ 282ms 285ms p=0.741 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 86ms (± 6.89%) 86ms (± 6.89%) ~ 78ms 90ms p=1.000 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,593ms (± 0.28%) 2,598ms (± 0.36%) ~ 2,593ms 2,617ms p=0.688 n=6
Req 2 - geterr 1,691ms (± 1.91%) 1,716ms (± 2.40%) ~ 1,668ms 1,769ms p=0.298 n=6
Req 3 - references 115ms (± 8.88%) 114ms (± 8.87%) ~ 101ms 123ms p=0.466 n=6
Req 4 - navto 368ms (± 0.42%) 368ms (± 0.37%) ~ 365ms 369ms p=0.432 n=6
Req 5 - completionInfo count 2,073 (± 0.00%) 2,073 (± 0.00%) ~ 2,073 2,073 p=1.000 n=6
Req 5 - completionInfo 307ms (± 2.35%) 308ms (± 1.73%) ~ 301ms 316ms p=0.810 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v18.15.0, x64)
Execution time 152.68ms (± 0.18%) 152.66ms (± 0.17%) ~ 151.64ms 155.25ms p=0.718 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 228.13ms (± 0.16%) 228.02ms (± 0.17%) -0.12ms (- 0.05%) 226.73ms 235.05ms p=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 229.55ms (± 0.18%) 229.53ms (± 0.17%) ~ 228.02ms 233.30ms p=0.861 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 229.32ms (± 0.19%) 229.47ms (± 0.16%) +0.15ms (+ 0.07%) 227.89ms 232.26ms p=0.000 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/56418/merge:

Everything looks good!

@bgenia
Copy link

bgenia commented Nov 16, 2023

Why does assignability change here if I replace interface with a type alias?

namespace Interface {

    interface InvariantArray<in out T> extends Array<T> { }

    declare let arr: InvariantArray<unknown>
    declare let brr: InvariantArray<number>

    arr = brr // Variance annotations forbid this
    brr = arr

}

namespace TypeAlias {

    type InvariantArray<in out T> = Array<T>

    declare let arr: InvariantArray<unknown>
    declare let brr: InvariantArray<number>

    arr = brr // For some reason this still works?
    brr = arr

}

Playground

@fatcerberus
Copy link

@bgenia Array is special-cased in the compiler to force covariance. The first version works because you're using a newly minted subtype, but that will also likely stop working too once #55880 gets merged.

@gabritto gabritto merged commit 41259d5 into microsoft:main Dec 11, 2023
19 checks passed
c0sta pushed a commit to c0sta/TypeScript that referenced this pull request Dec 20, 2023
@JoshuaKGoldberg JoshuaKGoldberg deleted the variance-allow-reference branch March 2, 2024 14:15
Andarist added a commit to Andarist/TypeScript that referenced this pull request Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Variance annotations are not allowed in type aliases for interfaces
6 participants