Skip to content

Fix check in getDiscriminantPropertyAccess #55464

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 23, 2023
Merged

Fix check in getDiscriminantPropertyAccess #55464

merged 2 commits into from
Aug 23, 2023

Conversation

ahejlsberg
Copy link
Member

Fixes #55425.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 22, 2023
@ahejlsberg
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot run dt
@typescript-bot perf test faster
@typescript-bot test top100

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 22, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 22, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 22, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 22, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@ahejlsberg
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,334k (± 0.00%) 300,324k (± 0.00%) ~ 300,315k 300,335k p=0.199 n=6
Parse Time 3.03s (± 0.27%) 3.02s (± 0.33%) ~ 3.00s 3.03s p=0.177 n=6
Bind Time 0.93s (± 0.00%) 0.93s (± 0.00%) ~ 0.93s 0.93s p=1.000 n=6
Check Time 9.48s (± 0.32%) 9.47s (± 0.20%) ~ 9.45s 9.50s p=0.744 n=6
Emit Time 7.63s (± 0.20%) 7.62s (± 0.22%) ~ 7.60s 7.64s p=0.138 n=6
Total Time 21.07s (± 0.23%) 21.04s (± 0.16%) ~ 21.00s 21.09s p=0.293 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 194,289k (± 0.51%) 193,883k (± 0.01%) ~ 193,839k 193,920k p=0.936 n=6
Parse Time 1.55s (± 0.79%) 1.56s (± 1.32%) ~ 1.55s 1.59s p=0.461 n=6
Bind Time 0.79s (± 0.00%) 0.79s (± 0.00%) ~ 0.79s 0.79s p=1.000 n=6
Check Time 9.95s (± 0.34%) 9.92s (± 0.30%) ~ 9.89s 9.97s p=0.256 n=6
Emit Time 2.75s (± 0.64%) 2.74s (± 0.33%) ~ 2.73s 2.75s p=0.673 n=6
Total Time 15.04s (± 0.23%) 15.02s (± 0.37%) ~ 14.96s 15.10s p=0.469 n=6
Monaco - node (v16.17.1, x64)
Memory used 347,123k (± 0.01%) 347,126k (± 0.01%) ~ 347,082k 347,165k p=0.630 n=6
Parse Time 2.69s (± 0.20%) 2.68s (± 0.30%) ~ 2.67s 2.69s p=0.859 n=6
Bind Time 0.99s (± 0.00%) 0.99s (± 0.52%) ~ 0.98s 0.99s p=0.174 n=6
Check Time 7.93s (± 0.36%) 7.93s (± 0.31%) ~ 7.90s 7.96s p=0.871 n=6
Emit Time 4.25s (± 0.28%) 4.26s (± 0.38%) ~ 4.23s 4.27s p=0.621 n=6
Total Time 15.85s (± 0.20%) 15.86s (± 0.23%) ~ 15.81s 15.90s p=1.000 n=6
TFS - node (v16.17.1, x64)
Memory used 301,139k (± 0.01%) 301,140k (± 0.01%) ~ 301,114k 301,162k p=0.873 n=6
Parse Time 2.18s (± 0.63%) 2.17s (± 0.61%) ~ 2.16s 2.19s p=0.550 n=6
Bind Time 1.11s (± 0.00%) 1.11s (± 0.37%) ~ 1.10s 1.11s p=0.405 n=6
Check Time 7.24s (± 0.24%) 7.25s (± 0.14%) ~ 7.23s 7.26s p=0.459 n=6
Emit Time 3.97s (± 0.37%) 3.98s (± 0.47%) ~ 3.95s 4.00s p=0.808 n=6
Total Time 14.50s (± 0.09%) 14.50s (± 0.22%) ~ 14.46s 14.55s p=1.000 n=6
material-ui - node (v16.17.1, x64)
Memory used 479,471k (± 0.00%) 479,474k (± 0.00%) ~ 479,460k 479,501k p=0.936 n=6
Parse Time 3.15s (± 0.24%) 3.15s (± 0.20%) ~ 3.14s 3.16s p=0.718 n=6
Bind Time 0.91s (± 0.00%) 0.91s (± 0.00%) ~ 0.91s 0.91s p=1.000 n=6
Check Time 18.00s (± 0.52%) 17.89s (± 0.21%) ~ 17.85s 17.94s p=0.078 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 22.06s (± 0.40%) 21.95s (± 0.18%) ~ 21.90s 22.00s p=0.054 n=6
xstate - node (v16.17.1, x64)
Memory used 542,823k (± 0.01%) 542,808k (± 0.01%) ~ 542,757k 542,873k p=0.689 n=6
Parse Time 3.70s (± 0.11%) 3.70s (± 0.30%) ~ 3.69s 3.72s p=0.445 n=6
Bind Time 1.40s (± 5.10%) 1.34s (± 0.00%) ~ 1.34s 1.34s p=0.654 n=6
Check Time 3.27s (± 2.58%) 3.35s (± 0.70%) ~ 3.32s 3.38s p=0.295 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=1.000 n=6
Total Time 8.45s (± 0.23%) 8.47s (± 0.15%) ~ 8.46s 8.49s p=0.170 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

@ahejlsberg Here are the results of running the user test suite comparing main and refs/pull/55464/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-55464/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55464/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55464/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)

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

@ahejlsberg
Copy link
Member Author

Tests and perf look good. This one is ready to merge.

@ahejlsberg ahejlsberg merged commit fcaa900 into main Aug 23, 2023
@ahejlsberg ahejlsberg deleted the fix55425 branch August 23, 2023 18:43
snovader pushed a commit to EG-A-S/TypeScript that referenced this pull request Sep 23, 2023
@ahejlsberg @snovader
Fix check in getDiscriminantPropertyAccess (microsoft#55464)
cb2f733
@RyanCavanaugh RyanCavanaugh mentioned this pull request Oct 18, 2023
Closed
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

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

Assignees

@ahejlsberg ahejlsberg

Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discriminated union narrowing breaks when discriminating from a wider union type
3 participants
@ahejlsberg @typescript-bot @jakebailey