Skip to content

in operator shouldn't narrow {} originating in unknown#50610

Merged
ahejlsberg merged 2 commits into
mainfrom
fix50531
Sep 2, 2022
Merged

in operator shouldn't narrow {} originating in unknown#50610
ahejlsberg merged 2 commits into
mainfrom
fix50531

Conversation

@ahejlsberg

@ahejlsberg ahejlsberg commented Sep 2, 2022

Copy link
Copy Markdown
Member

Fixes #50531.

This is a targeted fix for the exact issue in #50531. It doesn't address other known issues with the in operator.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Sep 2, 2022
@ahejlsberg

ahejlsberg commented Sep 2, 2022

Copy link
Copy Markdown
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

typescript-bot commented Sep 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@typescript-bot

typescript-bot commented Sep 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

typescript-bot commented Sep 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 517777d. You can monitor the build here.

@typescript-bot

typescript-bot commented Sep 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

typescript-bot commented Sep 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 517777d. You can monitor the build here.

Update: The results are in!

@typescript-bot

typescript-bot commented Sep 2, 2022

Copy link
Copy Markdown
Contributor

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

Here they are:

Comparison Report - main..50610

Metric main 50610 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 338,741k (± 0.01%) 338,732k (± 0.01%) -9k (- 0.00%) 338,679k 338,780k
Parse Time 2.08s (± 0.72%) 2.09s (± 1.27%) +0.01s (+ 0.67%) 2.05s 2.16s
Bind Time 0.80s (± 1.52%) 0.79s (± 0.51%) -0.01s (- 1.00%) 0.78s 0.80s
Check Time 5.84s (± 0.30%) 5.83s (± 0.48%) -0.01s (- 0.24%) 5.78s 5.90s
Emit Time 6.18s (± 0.96%) 6.17s (± 0.52%) -0.01s (- 0.16%) 6.10s 6.23s
Total Time 14.90s (± 0.48%) 14.88s (± 0.26%) -0.01s (- 0.09%) 14.79s 14.98s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,975k (± 0.38%) 192,983k (± 0.39%) +8k (+ 0.00%) 192,579k 195,996k
Parse Time 0.86s (± 1.15%) 0.85s (± 0.80%) -0.00s (- 0.47%) 0.84s 0.87s
Bind Time 0.48s (± 1.03%) 0.49s (± 0.98%) +0.00s (+ 0.41%) 0.48s 0.50s
Check Time 6.69s (± 0.60%) 6.73s (± 0.41%) +0.03s (+ 0.51%) 6.67s 6.80s
Emit Time 2.41s (± 0.65%) 2.41s (± 0.62%) +0.00s (+ 0.04%) 2.37s 2.43s
Total Time 10.44s (± 0.45%) 10.47s (± 0.28%) +0.03s (+ 0.29%) 10.38s 10.53s
Monaco - node (v14.15.1, x64)
Memory used 326,527k (± 0.01%) 326,530k (± 0.01%) +3k (+ 0.00%) 326,469k 326,562k
Parse Time 1.57s (± 0.68%) 1.57s (± 0.53%) -0.00s (- 0.19%) 1.55s 1.58s
Bind Time 0.72s (± 0.50%) 0.72s (± 0.47%) -0.00s (- 0.14%) 0.72s 0.73s
Check Time 5.72s (± 0.51%) 5.73s (± 0.31%) +0.01s (+ 0.23%) 5.70s 5.77s
Emit Time 3.33s (± 0.87%) 3.30s (± 0.45%) -0.04s (- 1.14%) 3.27s 3.33s
Total Time 11.35s (± 0.36%) 11.32s (± 0.30%) -0.03s (- 0.25%) 11.25s 11.40s
TFS - node (v14.15.1, x64)
Memory used 289,647k (± 0.01%) 289,649k (± 0.01%) +2k (+ 0.00%) 289,594k 289,681k
Parse Time 1.31s (± 0.81%) 1.30s (± 0.91%) -0.00s (- 0.38%) 1.28s 1.34s
Bind Time 0.79s (± 0.51%) 0.80s (± 1.27%) +0.00s (+ 0.51%) 0.78s 0.83s
Check Time 5.36s (± 0.43%) 5.34s (± 0.42%) -0.02s (- 0.35%) 5.30s 5.39s
Emit Time 3.60s (± 1.19%) 3.56s (± 1.05%) -0.03s (- 0.97%) 3.49s 3.68s
Total Time 11.06s (± 0.52%) 11.00s (± 0.48%) -0.06s (- 0.56%) 10.88s 11.16s
material-ui - node (v14.15.1, x64)
Memory used 436,456k (± 0.00%) 436,461k (± 0.01%) +5k (+ 0.00%) 436,416k 436,518k
Parse Time 1.86s (± 0.38%) 1.87s (± 0.56%) +0.01s (+ 0.48%) 1.85s 1.89s
Bind Time 0.58s (± 0.63%) 0.58s (± 0.51%) -0.00s (- 0.51%) 0.58s 0.59s
Check Time 12.86s (± 0.51%) 12.90s (± 0.80%) +0.04s (+ 0.32%) 12.66s 13.21s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.30s (± 0.41%) 15.35s (± 0.64%) +0.05s (+ 0.33%) 15.11s 15.64s
xstate - node (v14.15.1, x64)
Memory used 546,724k (± 0.00%) 546,737k (± 0.00%) +13k (+ 0.00%) 546,714k 546,782k
Parse Time 2.60s (± 0.43%) 2.60s (± 0.52%) -0.00s (- 0.12%) 2.58s 2.64s
Bind Time 0.97s (± 0.77%) 0.98s (± 0.85%) +0.01s (+ 0.93%) 0.96s 1.00s
Check Time 1.53s (± 0.65%) 1.53s (± 0.68%) -0.00s (- 0.13%) 1.51s 1.55s
Emit Time 0.07s (± 4.13%) 0.07s (± 3.14%) -0.00s (- 1.39%) 0.07s 0.08s
Total Time 5.18s (± 0.31%) 5.18s (± 0.41%) 0.00s ( 0.00%) 5.13s 5.23s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 50610 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot

typescript-bot commented Sep 2, 2022

Copy link
Copy Markdown
Contributor

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

Everything looks good!

@typescript-bot

typescript-bot commented Sep 2, 2022

Copy link
Copy Markdown
Contributor

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

Everything looks good!

@andrewbranch andrewbranch added this to the TypeScript 4.8.3 milestone Sep 2, 2022
@andrewbranch

andrewbranch commented Sep 2, 2022

Copy link
Copy Markdown
Member

@typescript-bot cherry-pick this to release-4.8

@typescript-bot

typescript-bot commented Sep 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya @andrewbranch, I've started to run the task to cherry-pick this into release-4.8 on this PR at 517777d. You can monitor the build here.

@typescript-bot typescript-bot mentioned this pull request Sep 2, 2022
Merged
@typescript-bot

typescript-bot commented Sep 2, 2022

Copy link
Copy Markdown
Contributor

Hey @andrewbranch, I've opened #50612 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Sep 2, 2022
@ahejlsberg @typescript-bot
Cherry-pick PR microsoft#50610 into release-4.8
4f66dcc 
Component commits:
908548e 'in' operator shouldn't narrow {} originating in unknown

517777d Add regression test
@ahejlsberg ahejlsberg merged commit 854d448 into main Sep 2, 2022
@ahejlsberg ahejlsberg deleted the fix50531 branch September 2, 2022 16:30
@fatcerberus

fatcerberus commented Sep 2, 2022

Copy link
Copy Markdown

What was the cause of #50531? It’s weird that unknown -> {} got narrowed by in but a directly-typed {} didn’t.

@andrewbranch

andrewbranch commented Sep 2, 2022

Copy link
Copy Markdown
Member

It’s that weird declaredType !== type condition—we discussed rewriting that whole set of conditions for 4.9 such that in only filters object types out of unions by property when there’s more than one object-like type in the declared type. There was also some discussion about whether to do anything if the property tested by in doesn’t exist in any of the object-like types in the union. (@ahejlsberg can probably better explain where we landed.)

andrewbranch pushed a commit that referenced this pull request Sep 2, 2022
@ahejlsberg
Cherry-pick PR #50610 into release-4.8 (#50612)
4083b46 
Component commits:
908548e 'in' operator shouldn't narrow {} originating in unknown

517777d Add regression test

Co-authored-by: Anders Hejlsberg <[email protected]>
@sync-by-unito sync-by-unito Bot mentioned this pull request Sep 13, 2022
Closed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jan 12, 2023