Skip to content

Discriminate types with divergent read/write property types #55576

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
Merged
merged 2 commits into from
Aug 30, 2023
Merged

Discriminate types with divergent read/write property types #55576

merged 2 commits into from
Aug 30, 2023

Conversation

Andarist
Copy link
Contributor

fixes #55532 (comment)
fixes #55566

The cases related to optional members and EOPT regressed from #54777
The divergent accessors case has never worked but I feel like it should and this fixes it as well

cc @jakebailey

jakebailey
jakebailey reviewed Aug 30, 2023
View reviewed changes
src/compiler/checker.ts
else if (type !== firstType) {
if (type !== firstType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh man this looks so silly, it had to have been a pain to debug.

@jakebailey
Copy link
Member

@typescript-bot test top200
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this faster
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 30, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 30, 2023 •
edited
Loading

Heya @jakebailey, I've started to run the tsc-only perf test suite on this PR at 8a75fd7. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 30, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 30, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 30, 2023 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 30, 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/157158/artifacts?artifactName=tgz&fileId=98F844B893A60674D19DA40925D52C76DDFCB7F360CC0A18D8BC56AB16E6098802&fileName=/typescript-5.3.0-insiders.20230830.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
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 (v16.17.1, x64)
Memory used 300,273k (± 0.01%) 300,267k (± 0.00%) ~ 300,251k 300,285k p=0.521 n=6
Parse Time 3.00s (± 0.34%) 3.00s (± 0.27%) ~ 2.99s 3.01s p=0.932 n=6
Bind Time 0.93s (± 0.00%) 0.93s (± 0.00%) ~ 0.93s 0.93s p=1.000 n=6
Check Time 9.34s (± 0.32%) 9.31s (± 0.31%) ~ 9.28s 9.35s p=0.172 n=6
Emit Time 7.63s (± 0.35%) 7.63s (± 0.25%) ~ 7.60s 7.65s p=0.871 n=6
Total Time 20.90s (± 0.20%) 20.87s (± 0.24%) ~ 20.81s 20.94s p=0.228 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 194,453k (± 0.66%) 193,927k (± 0.03%) ~ 193,856k 193,966k p=0.471 n=6
Parse Time 1.58s (± 0.00%) 1.58s (± 0.26%) ~ 1.58s 1.59s p=0.405 n=6
Bind Time 0.79s (± 0.65%) 0.80s (± 0.51%) ~ 0.79s 0.80s p=0.112 n=6
Check Time 9.93s (± 0.33%) 9.95s (± 0.59%) ~ 9.87s 10.02s p=0.748 n=6
Emit Time 2.74s (± 0.27%) 2.74s (± 0.27%) ~ 2.73s 2.75s p=1.000 n=6
Total Time 15.04s (± 0.27%) 15.06s (± 0.41%) ~ 14.99s 15.14s p=0.630 n=6
Monaco - node (v16.17.1, x64)
Memory used 347,200k (± 0.00%) 347,181k (± 0.01%) ~ 347,123k 347,230k p=0.229 n=6
Parse Time 2.69s (± 0.20%) 2.68s (± 0.44%) ~ 2.67s 2.70s p=0.498 n=6
Bind Time 0.99s (± 0.00%) 0.99s (± 0.41%) ~ 0.98s 0.99s p=0.405 n=6
Check Time 7.94s (± 0.37%) 7.93s (± 0.53%) ~ 7.86s 7.97s p=0.629 n=6
Emit Time 4.26s (± 0.39%) 4.26s (± 0.27%) ~ 4.24s 4.27s p=0.804 n=6
Total Time 15.88s (± 0.18%) 15.86s (± 0.24%) ~ 15.81s 15.91s p=0.377 n=6
TFS - node (v16.17.1, x64)
Memory used 301,182k (± 0.01%) 301,170k (± 0.01%) ~ 301,144k 301,199k p=0.378 n=6
Parse Time 2.17s (± 0.45%) 2.18s (± 0.56%) ~ 2.15s 2.18s p=0.242 n=6
Bind Time 1.11s (± 1.23%) 1.11s (± 0.73%) ~ 1.11s 1.13s p=0.599 n=6
Check Time 7.23s (± 0.19%) 7.23s (± 0.45%) ~ 7.20s 7.29s p=0.745 n=6
Emit Time 3.99s (± 0.29%) 3.98s (± 0.41%) ~ 3.96s 4.00s p=0.142 n=6
Total Time 14.49s (± 0.15%) 14.50s (± 0.24%) ~ 14.46s 14.54s p=0.809 n=6
material-ui - node (v16.17.1, x64)
Memory used 479,465k (± 0.00%) 479,466k (± 0.00%) ~ 479,451k 479,475k p=0.470 n=6
Parse Time 3.15s (± 0.17%) 3.15s (± 0.26%) ~ 3.14s 3.16s p=0.859 n=6
Bind Time 0.91s (± 0.00%) 0.91s (± 0.00%) ~ 0.91s 0.91s p=1.000 n=6
Check Time 17.85s (± 0.36%) 17.75s (± 0.40%) -0.11s (- 0.60%) 17.63s 17.81s p=0.025 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 21.90s (± 0.29%) 21.80s (± 0.30%) -0.10s (- 0.46%) 21.70s 21.87s p=0.024 n=6
xstate - node (v16.17.1, x64)
Memory used 542,815k (± 0.00%) 542,829k (± 0.01%) ~ 542,769k 542,888k p=1.000 n=6
Parse Time 3.69s (± 0.22%) 3.70s (± 0.15%) ~ 3.69s 3.70s p=0.441 n=6
Bind Time 1.43s (± 3.05%) 1.40s (± 4.31%) ~ 1.34s 1.45s p=0.662 n=6
Check Time 3.20s (± 2.28%) 3.24s (± 2.88%) ~ 3.14s 3.35s p=0.747 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 6.19%) ~ 0.08s 0.09s p=0.174 n=6
Total Time 8.40s (± 0.35%) 8.41s (± 0.51%) ~ 8.37s 8.48s p=0.935 n=6
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

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

Otherwise...

Something interesting changed - please have a look.

Details

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

  • [NEW] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55576/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • [MISSING] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)

@jakebailey
Copy link
Member

Apparently I did not fix the dang error deltas.

@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/55576/merge:

Everything looks good!

@jakebailey
Copy link
Member

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 30, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/55576/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!

@jakebailey jakebailey merged commit f424a6b into microsoft:main Aug 30, 2023
@Andarist Andarist deleted the fix/discriminate-divergent-types branch August 31, 2023 07:19
@Andarist Andarist mentioned this pull request Aug 31, 2023
Merged
snovader pushed a commit to EG-A-S/TypeScript that referenced this pull request Sep 23, 2023
@Andarist @snovader
Discriminate types with divergent read/write property types (microsof…
6a011b5 
…t#55576)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakebailey jakebailey jakebailey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@Andarist @jakebailey @typescript-bot