Skip to content

Always compare rest types as mutable #55793

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Always compare rest types as mutable #55793

wants to merge 2 commits into from

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Sep 20, 2023 •
edited
Loading

Fixes #53255

...args are always safely mutable within the function body as their contents are copied. See also #53398, though this PR does not fix them inside of function bodies.

This PR ignores the mutability of rest params when relating signatures, stripping readonly-ness via getMutableArrayOrTupleType.

It's probable that this will cause a perf hit; a cache will probably fix that but I'm sending it without one for now just to test.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Sep 20, 2023
@jakebailey
Copy link
Member Author

@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 Sep 20, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 20, 2023 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 20, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 20, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 20, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 20, 2023 •
edited
Loading

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/157821/artifacts?artifactName=tgz&fileId=0952797A78A9CAAB929E28E81A449B883F795DF55524EE10C419E9F7BBD92EB302&fileName=/typescript-5.3.0-insiders.20230920.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/55793/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Unknown failure"
  • 2 instances of "Package install failed"

Otherwise...

Everything looks good!

@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 294,909k (± 0.01%) 294,910k (± 0.01%) ~ 294,880k 294,944k p=0.936 n=6
Parse Time 2.63s (± 0.20%) 2.64s (± 0.28%) ~ 2.63s 2.65s p=0.069 n=6
Bind Time 0.85s (± 0.96%) 0.85s (± 0.96%) ~ 0.83s 0.85s p=1.000 n=6
Check Time 8.00s (± 0.25%) 8.01s (± 0.29%) ~ 7.98s 8.04s p=0.288 n=6
Emit Time 7.01s (± 0.21%) 7.02s (± 0.32%) ~ 7.00s 7.06s p=0.315 n=6
Total Time 18.48s (± 0.09%) 18.53s (± 0.17%) +0.04s (+ 0.23%) 18.49s 18.57s p=0.019 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 191,612k (± 1.22%) 194,418k (± 1.52%) ~ 190,598k 196,351k p=0.575 n=6
Parse Time 1.35s (± 0.40%) 1.35s (± 1.01%) ~ 1.33s 1.36s p=0.341 n=6
Bind Time 0.73s (± 0.00%) 0.73s (± 0.00%) ~ 0.73s 0.73s p=1.000 n=6
Check Time 9.12s (± 0.57%) 9.07s (± 0.27%) ~ 9.04s 9.11s p=0.147 n=6
Emit Time 2.59s (± 0.40%) 2.61s (± 0.80%) ~ 2.58s 2.63s p=0.190 n=6
Total Time 13.79s (± 0.39%) 13.76s (± 0.36%) ~ 13.69s 13.83s p=0.259 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,175k (± 0.01%) 347,170k (± 0.00%) ~ 347,155k 347,199k p=1.000 n=6
Parse Time 2.47s (± 0.21%) 2.47s (± 0.34%) ~ 2.45s 2.47s p=0.923 n=6
Bind Time 0.94s (± 0.80%) 0.94s (± 0.55%) ~ 0.93s 0.94s p=0.784 n=6
Check Time 6.89s (± 0.75%) 6.85s (± 0.65%) ~ 6.79s 6.92s p=0.173 n=6
Emit Time 4.04s (± 0.26%) 4.05s (± 0.49%) ~ 4.03s 4.08s p=0.217 n=6
Total Time 14.32s (± 0.30%) 14.30s (± 0.31%) ~ 14.24s 14.35s p=0.375 n=6
TFS - node (v18.15.0, x64)
Memory used 302,459k (± 0.00%) 302,462k (± 0.00%) ~ 302,451k 302,471k p=0.873 n=6
Parse Time 2.01s (± 0.70%) 2.01s (± 0.51%) ~ 1.99s 2.02s p=0.738 n=6
Bind Time 1.00s (± 0.81%) 1.00s (± 0.41%) ~ 0.99s 1.00s p=0.218 n=6
Check Time 6.22s (± 0.28%) 6.23s (± 0.37%) ~ 6.20s 6.26s p=0.808 n=6
Emit Time 3.51s (± 0.35%) 3.51s (± 0.59%) ~ 3.49s 3.55s p=0.408 n=6
Total Time 12.74s (± 0.20%) 12.75s (± 0.37%) ~ 12.70s 12.80s p=0.936 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,427k (± 0.00%) 470,431k (± 0.01%) ~ 470,385k 470,469k p=0.630 n=6
Parse Time 2.58s (± 0.80%) 2.60s (± 0.47%) ~ 2.58s 2.61s p=0.370 n=6
Bind Time 1.00s (± 1.17%) 1.00s (± 0.52%) ~ 0.99s 1.00s p=0.542 n=6
Check Time 16.57s (± 0.39%) 16.63s (± 0.70%) ~ 16.48s 16.75s p=0.575 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.16s (± 0.24%) 20.22s (± 0.55%) ~ 20.08s 20.35s p=0.520 n=6
xstate - node (v18.15.0, x64)
Memory used 512,200k (± 0.01%) 512,254k (± 0.01%) ~ 512,182k 512,341k p=0.230 n=6
Parse Time 3.26s (± 0.32%) 3.25s (± 0.56%) ~ 3.23s 3.27s p=0.142 n=6
Bind Time 1.54s (± 0.35%) 1.55s (± 0.33%) +0.01s (+ 0.54%) 1.55s 1.56s p=0.038 n=6
Check Time 2.77s (± 0.78%) 2.78s (± 0.80%) ~ 2.76s 2.82s p=0.669 n=6
Emit Time 0.08s (± 6.19%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=0.174 n=6
Total Time 7.66s (± 0.28%) 7.66s (± 0.29%) ~ 7.62s 7.68s p=0.935 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,354ms (± 0.81%) 2,366ms (± 1.48%) ~ 2,332ms 2,405ms p=0.689 n=6
Req 2 - geterr 5,363ms (± 1.28%) 5,346ms (± 1.33%) ~ 5,270ms 5,415ms p=1.000 n=6
Req 3 - references 329ms (± 1.36%) 329ms (± 1.67%) ~ 325ms 338ms p=0.871 n=6
Req 4 - navto 276ms (± 0.60%) 276ms (± 0.77%) ~ 274ms 280ms p=0.622 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 84ms (± 7.78%) 83ms (± 9.48%) ~ 75ms 90ms p=1.000 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,474ms (± 0.67%) 2,462ms (± 0.70%) ~ 2,441ms 2,490ms p=0.336 n=6
Req 2 - geterr 4,105ms (± 1.75%) 4,111ms (± 1.78%) ~ 4,052ms 4,206ms p=0.378 n=6
Req 3 - references 338ms (± 1.36%) 337ms (± 1.42%) ~ 331ms 343ms p=0.684 n=6
Req 4 - navto 283ms (± 0.39%) 283ms (± 0.32%) ~ 282ms 284ms p=0.932 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 80ms (± 7.41%) 78ms (± 6.74%) ~ 73ms 88ms p=0.273 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,598ms (± 0.63%) 2,600ms (± 0.26%) ~ 2,594ms 2,613ms p=0.936 n=6
Req 2 - geterr 1,717ms (± 0.99%) 1,703ms (± 2.54%) ~ 1,648ms 1,769ms p=0.297 n=6
Req 3 - references 114ms (± 9.45%) 115ms (± 8.54%) ~ 106ms 128ms p=1.000 n=6
Req 4 - navto 367ms (± 0.56%) 364ms (± 0.37%) ~ 363ms 366ms p=0.054 n=6
Req 5 - completionInfo count 2,071 (± 0.00%) 2,071 (± 0.00%) ~ 2,071 2,071 p=1.000 n=6
Req 5 - completionInfo 303ms (± 2.65%) 307ms (± 2.39%) ~ 298ms 317ms p=0.336 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 151.89ms (± 0.18%) 151.98ms (± 0.19%) +0.09ms (+ 0.06%) 150.88ms 155.30ms p=0.002 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 226.77ms (± 0.15%) 226.51ms (± 0.15%) -0.26ms (- 0.12%) 225.28ms 230.80ms p=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 228.53ms (± 0.15%) 228.44ms (± 0.15%) -0.09ms (- 0.04%) 226.61ms 231.61ms p=0.006 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 227.84ms (± 0.14%) 227.88ms (± 0.16%) ~ 226.38ms 233.42ms p=0.558 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, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@jakebailey
Copy link
Member Author

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 20, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@jakebailey jakebailey marked this pull request as draft September 20, 2023 16:48
@jakebailey
Copy link
Member Author

On second thought, I think this is wrong and getMutableArrayOrTupleType works for tuples, not for arrays; it converts arrays to variadics and I mistakenly disabled that.

@jakebailey
Copy link
Member Author

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 20, 2023 •
edited
Loading

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

Update: The results are in!

@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.

@jakebailey jakebailey closed this Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weswigham weswigham Awaiting requested review from weswigham

@gabritto gabritto Awaiting requested review from gabritto

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@jakebailey jakebailey

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parameters created from tuple are treated as mutable instead of immutable, unless written as "...args"
2 participants
@jakebailey @typescript-bot