Skip to content

Properly handle null and undefined in getCommonSupertype - #50021

Merged
Anders Hejlsberg (ahejlsberg) merged 3 commits into
mainfrom
fix49938
Jul 24, 2022
Merged

Properly handle null and undefined in getCommonSupertype#50021
Anders Hejlsberg (ahejlsberg) merged 3 commits into
mainfrom
fix49938

Conversation

@ahejlsberg

Copy link
Copy Markdown
Member

This is an alternative fix for #49938 that supercedes #49981.

Jake Bailey (@jakebailey) I found it easier to put up a new PR. I've copied the tests from #49981. This PR more uniformly lifts undefined and null from the inference candidates and then adds them back later, plus it's a bit better at avoiding duplicate work. For example, it handles this case that otherwise is an error:

letv=null!;declarefunctionfoo<T>(x: T,y: T): T;constx=foo(vas'a'|undefined,vas'b');

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) test this

@typescript-bot

TypeScript Bot (typescript-bot) commented Jul 23, 2022

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the extended test suite on this PR at d4a239c. You can monitor the build here.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Jul 23, 2022

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Jul 23, 2022

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the abridged perf test suite on this PR at d4a239c. You can monitor the build here.

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Jul 23, 2022

Copy link
Copy Markdown
Contributor

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

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Jul 23, 2022

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the tarball bundle task on this PR at d4a239c. You can monitor the build here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..50021

Metricmain50021DeltaBestWorst
Angular - node (v14.15.1, x64)
Memory used335,938k (± 0.01%)335,962k (± 0.01%)+24k (+ 0.01%)335,871k336,005k
Parse Time2.06s (± 0.65%)2.06s (± 0.46%)-0.01s (- 0.39%)2.04s2.08s
Bind Time0.90s (± 1.30%)0.90s (± 0.72%)-0.00s (- 0.11%)0.89s0.91s
Check Time5.82s (± 0.41%)5.83s (± 0.41%)+0.01s (+ 0.19%)5.77s5.89s
Emit Time6.38s (± 0.63%)6.38s (± 0.63%)-0.01s (- 0.08%)6.27s6.43s
Total Time15.17s (± 0.37%)15.16s (± 0.43%)-0.01s (- 0.05%)14.98s15.24s
Compiler-Unions - node (v14.15.1, x64)
Memory used193,129k (± 0.01%)193,453k (± 0.38%)+324k (+ 0.17%)193,001k196,437k
Parse Time0.85s (± 0.68%)0.84s (± 0.43%)-0.00s (- 0.35%)0.84s0.85s
Bind Time0.57s (± 1.01%)0.57s (± 1.13%)+0.00s (+ 0.35%)0.56s0.59s
Check Time6.71s (± 0.80%)6.73s (± 0.58%)+0.03s (+ 0.39%)6.64s6.83s
Emit Time2.49s (± 0.87%)2.49s (± 0.82%)+0.01s (+ 0.28%)2.46s2.55s
Total Time10.61s (± 0.54%)10.64s (± 0.38%)+0.03s (+ 0.29%)10.54s10.74s
Monaco - node (v14.15.1, x64)
Memory used325,664k (± 0.01%)325,647k (± 0.01%)-17k (- 0.01%)325,601k325,702k
Parse Time1.58s (± 0.48%)1.59s (± 0.70%)+0.01s (+ 0.63%)1.56s1.61s
Bind Time0.78s (± 0.71%)0.79s (± 1.06%)+0.00s (+ 0.38%)0.77s0.81s
Check Time5.71s (± 0.46%)5.67s (± 0.36%)-0.03s (- 0.56%)5.64s5.73s
Emit Time3.36s (± 0.58%)3.34s (± 0.75%)-0.02s (- 0.60%)3.28s3.40s
Total Time11.42s (± 0.36%)11.38s (± 0.34%)-0.04s (- 0.32%)11.34s11.51s
TFS - node (v14.15.1, x64)
Memory used288,822k (± 0.01%)288,845k (± 0.01%)+23k (+ 0.01%)288,795k288,931k
Parse Time1.33s (± 1.04%)1.33s (± 1.76%)+0.00s (+ 0.23%)1.29s1.39s
Bind Time0.77s (± 4.36%)0.77s (± 4.05%)-0.00s (- 0.00%)0.73s0.85s
Check Time5.36s (± 0.26%)5.33s (± 0.50%)-0.03s (- 0.54%)5.27s5.39s
Emit Time3.54s (± 1.81%)3.62s (± 1.92%)+0.08s (+ 2.18%)3.46s3.73s
Total Time10.99s (± 0.57%)11.04s (± 0.73%)+0.05s (+ 0.48%)10.82s11.17s
material-ui - node (v14.15.1, x64)
Memory used446,711k (± 0.01%)446,730k (± 0.01%)+19k (+ 0.00%)446,661k446,786k
Parse Time1.88s (± 0.62%)1.88s (± 0.64%)-0.00s (- 0.05%)1.85s1.91s
Bind Time0.72s (± 1.23%)0.72s (± 0.97%)-0.01s (- 0.83%)0.71s0.74s
Check Time13.18s (± 0.51%)13.24s (± 0.58%)+0.07s (+ 0.51%)13.11s13.44s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.78s (± 0.47%)15.85s (± 0.52%)+0.06s (+ 0.39%)15.70s16.04s
xstate - node (v14.15.1, x64)
Memory used541,295k (± 0.01%)541,308k (± 0.00%)+13k (+ 0.00%)541,260k541,341k
Parse Time2.60s (± 0.29%)2.61s (± 0.32%)+0.01s (+ 0.35%)2.58s2.62s
Bind Time1.15s (± 1.24%)1.14s (± 1.57%)-0.01s (- 0.61%)1.10s1.20s
Check Time1.56s (± 0.49%)1.56s (± 0.72%)-0.00s (- 0.06%)1.53s1.58s
Emit Time0.07s (± 4.66%)0.07s (± 4.13%)-0.00s (- 1.37%)0.07s0.08s
Total Time5.38s (± 0.36%)5.38s (± 0.41%)+0.01s (+ 0.11%)5.35s5.46s
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)
BenchmarkNameIterations
Current5002110
Baselinemain10

Developer Information:

Download Benchmark

@typescript-bot

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@ahejlsberg), I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg)
Great news! no new errors were found between main..refs/pull/50021/merge

@typescript-bot

TypeScript Bot (typescript-bot) commented Jul 23, 2022

Copy link
Copy Markdown
Contributor

Hey Anders Hejlsberg (@ahejlsberg), 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/130297/artifacts?artifactName=tgz&fileId=5B6A9298038EC2E4EEE17420D8BA1C609540E927E60F49CC71CBE61DF68A3F3902&fileName=/typescript-4.8.0-insiders.20220723.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/pr-build@4.8.0-pr-50021-12".;

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

Tests and performance all look good. No new errors, but one error went away in RWC because of better inference. I think this is a good fix.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

Latest commit adds more tests that now succeed but previously failed.

@jakebaileyJake Bailey (jakebailey) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great. I should have known that literals needed the same treatment!

(filterType is another helper I need to commit to memory as existing.)

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Uncommitted BugPR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ahejlsberg@typescript-bot@jakebailey