Skip to content

Ensure parameters are in scope when converting parameter/return types to type nodes - #49627

Merged
Jake Bailey (jakebailey) merged 4 commits into
microsoft:mainfrom
jakebailey:fix-48783
Feb 18, 2023
Merged

Ensure parameters are in scope when converting parameter/return types to type nodes#49627
Jake Bailey (jakebailey) merged 4 commits into
microsoft:mainfrom
jakebailey:fix-48783

Conversation

@jakebailey

@jakebaileyJake Bailey (jakebailey) commented Jun 22, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#48783

@typescript-botTypeScript Bot (typescript-bot) added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jun 22, 2022
jakebailey
Jake Bailey (jakebailey) commented Jun 22, 2022
View reviewed changes
Comment threadtests/baselines/reference/jsDeclarationsParameterTagReusesInputNodeInEmit1.js Outdated
Comment threadtests/baselines/reference/jsDeclarationsImportAliasExposedWithinNamespaceCjs.js Outdated
Comment threadtests/baselines/reference/jsDeclarationsFunctions.js Outdated
@jakebailey
Jake Bailey (jakebailey) marked this pull request as draft June 22, 2022 03:46
@jakebailey

Jake Bailey (jakebailey) commented Jun 22, 2022

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) user test this inline
TypeScript Bot (@typescript-bot) run dt
TypeScript Bot (@typescript-bot) perf test this

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the perf test suite on this PR at 70dba24. You can monitor the build here.

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at 70dba24. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the extended test suite on this PR at 70dba24. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2022

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), 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

TypeScript Bot (typescript-bot) commented Jun 22, 2022

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey)
Great news! no new errors were found between main..refs/pull/49627/merge

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2022

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..49627

Metricmain49627DeltaBestWorst
Angular - node (v10.16.3, x64)
Memory used359,540k (± 0.02%)359,610k (± 0.02%)+70k (+ 0.02%)359,432k359,750k
Parse Time2.11s (± 0.61%)2.11s (± 0.56%)-0.01s (- 0.33%)2.08s2.13s
Bind Time0.90s (± 0.72%)0.90s (± 0.64%)-0.00s (- 0.22%)0.89s0.91s
Check Time5.95s (± 0.44%)6.00s (± 0.74%)+0.04s (+ 0.72%)5.91s6.09s
Emit Time6.13s (± 0.59%)6.10s (± 0.93%)-0.03s (- 0.49%)5.99s6.24s
Total Time15.10s (± 0.33%)15.10s (± 0.65%)+0.00s (+ 0.01%)14.89s15.32s
Compiler-Unions - node (v10.16.3, x64)
Memory used206,478k (± 0.04%)206,407k (± 0.04%)-71k (- 0.03%)206,217k206,585k
Parse Time0.84s (± 0.70%)0.85s (± 0.53%)+0.00s (+ 0.47%)0.84s0.86s
Bind Time0.53s (± 1.51%)0.53s (± 1.38%)+0.00s (+ 0.00%)0.51s0.55s
Check Time8.12s (± 0.59%)8.09s (± 0.64%)-0.03s (- 0.38%)7.95s8.19s
Emit Time2.50s (± 0.64%)2.52s (± 1.06%)+0.02s (+ 0.92%)2.45s2.57s
Total Time11.99s (± 0.51%)11.98s (± 0.58%)-0.01s (- 0.05%)11.81s12.08s
Monaco - node (v10.16.3, x64)
Memory used343,932k (± 0.01%)343,925k (± 0.01%)-7k (- 0.00%)343,837k344,056k
Parse Time1.60s (± 0.39%)1.60s (± 0.55%)+0.00s (+ 0.06%)1.58s1.62s
Bind Time0.77s (± 0.64%)0.78s (± 1.56%)+0.01s (+ 0.90%)0.76s0.81s
Check Time5.96s (± 0.39%)5.94s (± 0.69%)-0.02s (- 0.34%)5.85s6.04s
Emit Time3.26s (± 0.85%)3.25s (± 0.69%)-0.00s (- 0.12%)3.20s3.29s
Total Time11.59s (± 0.42%)11.57s (± 0.44%)-0.02s (- 0.14%)11.50s11.70s
TFS - node (v10.16.3, x64)
Memory used305,066k (± 0.02%)305,121k (± 0.02%)+55k (+ 0.02%)305,014k305,309k
Parse Time1.29s (± 0.58%)1.28s (± 0.69%)-0.00s (- 0.23%)1.27s1.31s
Bind Time0.73s (± 0.71%)0.72s (± 0.80%)-0.01s (- 0.82%)0.71s0.73s
Check Time5.41s (± 0.53%)5.40s (± 0.63%)-0.02s (- 0.30%)5.33s5.47s
Emit Time3.43s (± 1.39%)3.42s (± 1.05%)-0.02s (- 0.50%)3.36s3.54s
Total Time10.86s (± 0.68%)10.82s (± 0.52%)-0.04s (- 0.41%)10.70s10.94s
material-ui - node (v10.16.3, x64)
Memory used469,051k (± 0.01%)469,049k (± 0.01%)-2k (- 0.00%)468,972k469,115k
Parse Time1.85s (± 0.53%)1.85s (± 0.48%)-0.00s (- 0.16%)1.83s1.86s
Bind Time0.70s (± 1.26%)0.70s (± 1.11%)-0.00s (- 0.14%)0.68s0.72s
Check Time14.55s (± 0.51%)14.56s (± 0.75%)+0.01s (+ 0.08%)14.34s14.93s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time17.09s (± 0.49%)17.10s (± 0.67%)+0.01s (+ 0.05%)16.89s17.50s
xstate - node (v10.16.3, x64)
Memory used584,361k (± 1.66%)581,002k (± 1.26%)-3,360k (- 0.57%)577,535k610,524k
Parse Time2.63s (± 0.21%)2.61s (± 0.34%)-0.01s (- 0.57%)2.59s2.63s
Bind Time1.04s (± 0.35%)1.03s (± 0.68%)-0.01s (- 1.44%)1.01s1.04s
Check Time1.54s (± 0.54%)1.54s (± 0.94%)-0.00s (- 0.06%)1.52s1.58s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.28s (± 0.19%)5.26s (± 0.20%)-0.03s (- 0.47%)5.23s5.28s
Angular - node (v12.1.0, x64)
Memory used337,149k (± 0.02%)337,041k (± 0.09%)-108k (- 0.03%)335,790k337,322k
Parse Time2.10s (± 0.54%)2.09s (± 0.71%)-0.01s (- 0.33%)2.06s2.12s
Bind Time0.86s (± 0.52%)0.86s (± 0.55%)+0.00s (+ 0.23%)0.85s0.87s
Check Time5.80s (± 0.36%)5.79s (± 0.54%)-0.01s (- 0.14%)5.73s5.88s
Emit Time6.35s (± 0.47%)6.36s (± 0.82%)+0.01s (+ 0.17%)6.25s6.47s
Total Time15.11s (± 0.36%)15.11s (± 0.54%)-0.00s (- 0.02%)14.92s15.29s
Compiler-Unions - node (v12.1.0, x64)
Memory used193,935k (± 0.11%)193,887k (± 0.15%)-48k (- 0.02%)193,098k194,399k
Parse Time0.85s (± 1.09%)0.84s (± 0.68%)-0.01s (- 0.94%)0.83s0.85s
Bind Time0.55s (± 1.22%)0.54s (± 1.37%)-0.00s (- 0.73%)0.53s0.57s
Check Time7.58s (± 0.49%)7.54s (± 0.73%)-0.05s (- 0.61%)7.42s7.66s
Emit Time2.53s (± 0.90%)2.54s (± 1.16%)+0.01s (+ 0.55%)2.47s2.61s
Total Time11.50s (± 0.44%)11.46s (± 0.53%)-0.04s (- 0.33%)11.33s11.65s
Monaco - node (v12.1.0, x64)
Memory used326,846k (± 0.02%)326,870k (± 0.02%)+25k (+ 0.01%)326,767k327,074k
Parse Time1.58s (± 1.03%)1.57s (± 0.80%)-0.01s (- 0.95%)1.54s1.59s
Bind Time0.76s (± 0.63%)0.76s (± 0.63%)+0.00s (+ 0.00%)0.75s0.77s
Check Time5.80s (± 0.58%)5.77s (± 0.50%)-0.03s (- 0.50%)5.72s5.83s
Emit Time3.31s (± 0.93%)3.30s (± 0.84%)-0.01s (- 0.24%)3.25s3.36s
Total Time11.45s (± 0.42%)11.40s (± 0.44%)-0.05s (- 0.45%)11.31s11.51s
TFS - node (v12.1.0, x64)
Memory used289,723k (± 0.02%)289,712k (± 0.02%)-10k (- 0.00%)289,624k289,876k
Parse Time1.32s (± 0.83%)1.31s (± 1.04%)-0.01s (- 1.13%)1.28s1.34s
Bind Time0.72s (± 0.80%)0.73s (± 1.52%)+0.00s (+ 0.55%)0.71s0.76s
Check Time5.34s (± 0.50%)5.31s (± 0.38%)-0.03s (- 0.58%)5.27s5.36s
Emit Time3.54s (± 0.86%)3.54s (± 1.65%)+0.01s (+ 0.14%)3.43s3.65s
Total Time10.93s (± 0.45%)10.89s (± 0.62%)-0.04s (- 0.38%)10.76s11.06s
material-ui - node (v12.1.0, x64)
Memory used448,154k (± 0.01%)448,053k (± 0.06%)-101k (- 0.02%)447,093k448,277k
Parse Time1.84s (± 0.63%)1.85s (± 0.45%)+0.00s (+ 0.05%)1.83s1.86s
Bind Time0.68s (± 0.65%)0.68s (± 0.49%)-0.00s (- 0.29%)0.67s0.69s
Check Time13.02s (± 0.52%)13.05s (± 0.44%)+0.03s (+ 0.23%)12.90s13.17s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.55s (± 0.48%)15.57s (± 0.37%)+0.03s (+ 0.19%)15.44s15.69s
xstate - node (v12.1.0, x64)
Memory used546,454k (± 1.31%)546,544k (± 1.31%)+89k (+ 0.02%)543,207k575,562k
Parse Time2.58s (± 0.55%)2.57s (± 0.59%)-0.01s (- 0.39%)2.54s2.61s
Bind Time1.06s (± 1.08%)1.05s (± 1.50%)-0.01s (- 0.85%)1.01s1.09s
Check Time1.49s (± 0.73%)1.48s (± 0.52%)-0.01s (- 0.47%)1.46s1.49s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.20s (± 0.32%)5.18s (± 0.42%)-0.03s (- 0.50%)5.13s5.24s
Angular - node (v14.15.1, x64)
Memory used335,317k (± 0.01%)335,342k (± 0.01%)+24k (+ 0.01%)335,296k335,389k
Parse Time2.09s (± 0.68%)2.08s (± 0.69%)-0.01s (- 0.53%)2.06s2.12s
Bind Time0.91s (± 0.52%)0.91s (± 0.82%)0.00s ( 0.00%)0.89s0.93s
Check Time5.75s (± 0.40%)5.76s (± 0.44%)+0.01s (+ 0.21%)5.72s5.81s
Emit Time6.42s (± 0.72%)6.41s (± 0.75%)-0.01s (- 0.19%)6.34s6.54s
Total Time15.17s (± 0.36%)15.15s (± 0.55%)-0.01s (- 0.08%)15.01s15.38s
Compiler-Unions - node (v14.15.1, x64)
Memory used192,554k (± 0.13%)192,995k (± 0.38%)+442k (+ 0.23%)192,626k195,955k
Parse Time0.86s (± 0.69%)0.85s (± 0.78%)-0.00s (- 0.35%)0.84s0.87s
Bind Time0.58s (± 1.29%)0.57s (± 0.39%)-0.01s (- 1.04%)0.57s0.58s
Check Time7.73s (± 0.61%)7.65s (± 0.78%)-0.08s (- 1.05%)7.53s7.76s
Emit Time2.52s (± 0.55%)2.53s (± 1.09%)+0.01s (+ 0.56%)2.47s2.59s
Total Time11.68s (± 0.47%)11.60s (± 0.71%)-0.08s (- 0.66%)11.42s11.78s
Monaco - node (v14.15.1, x64)
Memory used325,607k (± 0.00%)325,609k (± 0.01%)+2k (+ 0.00%)325,576k325,649k
Parse Time1.59s (± 0.78%)1.58s (± 0.57%)-0.01s (- 0.57%)1.56s1.60s
Bind Time0.80s (± 0.60%)0.79s (± 0.75%)-0.00s (- 0.38%)0.78s0.81s
Check Time5.72s (± 0.69%)5.70s (± 0.35%)-0.01s (- 0.23%)5.65s5.75s
Emit Time3.37s (± 0.73%)3.37s (± 0.64%)+0.01s (+ 0.18%)3.34s3.43s
Total Time11.46s (± 0.52%)11.44s (± 0.29%)-0.02s (- 0.17%)11.35s11.54s
TFS - node (v14.15.1, x64)
Memory used288,754k (± 0.01%)288,752k (± 0.01%)-2k (- 0.00%)288,693k288,829k
Parse Time1.34s (± 1.52%)1.32s (± 0.61%)-0.02s (- 1.79%)1.30s1.34s
Bind Time0.75s (± 0.94%)0.75s (± 0.53%)0.00s ( 0.00%)0.74s0.76s
Check Time5.33s (± 0.38%)5.32s (± 0.64%)-0.01s (- 0.19%)5.24s5.42s
Emit Time3.59s (± 2.15%)3.54s (± 2.17%)-0.05s (- 1.36%)3.42s3.72s
Total Time11.02s (± 0.93%)10.93s (± 0.98%)-0.08s (- 0.76%)10.76s11.21s
material-ui - node (v14.15.1, x64)
Memory used446,332k (± 0.00%)446,225k (± 0.05%)-108k (- 0.02%)445,273k446,378k
Parse Time1.89s (± 0.50%)1.89s (± 0.62%)-0.01s (- 0.37%)1.86s1.91s
Bind Time0.72s (± 1.23%)0.72s (± 0.92%)-0.00s (- 0.55%)0.71s0.73s
Check Time13.17s (± 0.59%)13.23s (± 0.81%)+0.05s (+ 0.40%)13.04s13.50s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.79s (± 0.49%)15.84s (± 0.68%)+0.05s (+ 0.31%)15.65s16.14s
xstate - node (v14.15.1, x64)
Memory used541,028k (± 0.00%)541,036k (± 0.00%)+8k (+ 0.00%)540,961k541,072k
Parse Time2.63s (± 0.49%)2.63s (± 0.64%)-0.00s (- 0.00%)2.61s2.68s
Bind Time1.17s (± 0.81%)1.16s (± 0.91%)-0.01s (- 0.60%)1.14s1.18s
Check Time1.53s (± 0.52%)1.54s (± 0.54%)+0.00s (+ 0.26%)1.52s1.56s
Emit Time0.07s (± 4.92%)0.07s (± 4.66%)-0.00s (- 1.35%)0.07s0.08s
Total Time5.41s (± 0.30%)5.40s (± 0.51%)-0.01s (- 0.17%)5.36s5.49s
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 (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v10.16.3, x64)
  • xstate - node (v12.1.0, x64)
  • xstate - node (v14.15.1, x64)
BenchmarkNameIterations
Current4962710
Baselinemain10

Developer Information:

Download Benchmark

jakebailey
Jake Bailey (jakebailey) commented Jun 22, 2022
View reviewed changes
Comment threadtests/baselines/reference/blockScopedVariablesUseBeforeDef.types Outdated
@jakebailey
Jake Bailey (jakebailey) marked this pull request as ready for review June 22, 2022 18:38
@jakebailey

Jake Bailey (jakebailey) commented Jun 22, 2022

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at 6714b2b. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@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/128615/artifacts?artifactName=tgz&fileId=60C8B4A5ED06713FB6C66B5745EA2EB6737C9B19A5285140AFCDFBFCEAB33D7102&fileName=/typescript-4.8.0-insiders.20220622.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-49627-10".;

@sandersn

Nathan Shively-Sanders (sandersn) commented Jun 23, 2022

Copy link
Copy Markdown
Member

I haven't reviewed the code yet, but I am weirded out by the whitespace changes -- which are admittedly the same as in #37444. But why insert 3 more spaces in, say, left and right below? It doesn't correspond to initial 4-space indent -- top is indented 8 spaces -- and left and right are only preceded by one space in the source.

module A{classPoint{constructor(publicx: number,publicy: number){}}exportvarUnitSquare : {top: {left: Point,right: Point},bottom: {left: Point,right: Point}}=null;}

@jakebailey

Jake Bailey (jakebailey) commented Jun 24, 2022

Copy link
Copy Markdown
MemberAuthor

I think before it was creating an all new type node and in doing so put the code onto one line. But after, it's reusing the node, and so it has the spacing as the original source. The baselines just remove the newlines so it looks awkwardly spaced.

I'd test on the above pack-this, but it looks like typescript-staging is down :(

jakebailey
Jake Bailey (jakebailey) commented Jul 18, 2022
View reviewed changes
Comment threadtests/baselines/reference/declarationEmitDistributiveConditionalWithInfer.js Outdated
jakebailey
Jake Bailey (jakebailey) commented Jul 19, 2022
View reviewed changes
Comment threadsrc/compiler/checker.ts Outdated
weswigham
Wesley Wigham (weswigham) reviewed Jul 19, 2022
View reviewed changes
Comment threadsrc/compiler/checker.ts Outdated
@jakebailey
Jake Bailey (jakebailey) marked this pull request as draft July 19, 2022 22:14
weswigham
Wesley Wigham (weswigham) reviewed Aug 2, 2022
View reviewed changes
Comment threadsrc/compiler/transformers/declarations.ts Outdated
jakebailey
Jake Bailey (jakebailey) commented Aug 2, 2022
View reviewed changes
Comment threadtests/baselines/reference/leaveOptionalParameterAsWritten.js
}
//// [c.d.ts]
declare type Foo = teams.calling.Foo;
export declare const bar: (p?: Foo) => void;

@jakebaileyJake Bailey (jakebailey)Aug 2, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is a case that I'm pretty sure only worked because of a fluke. It's desirable, but when we get the type from the annotation, we get the interface type Foo in another file (the symbol is correctly resolved), but then this gets handed down and the info about it being an alias is completely lost. I think this only worked due to assumptions about the enclosing declaration that now no longer hold.

@jakebailey

Jake Bailey (jakebailey) commented Aug 2, 2022

Copy link
Copy Markdown
MemberAuthor

I've gotten this PR pretty far but I'm going to stop working on it for a while because it's getting too frustrating. It seems like this part of the code is really fragile when it comes to what it considers to be reusable in ways that I can't seem to keep in my head at once. If fix one thing and break something I fixed earlier, because I forgot what I did and ended up undoing it, etc.

The whole enclosing declaration thing in general weirds me out; it doesn't seem consistent at all which enclosing declaration is used in what scenario, and yet the types baseline code just always uses the parent node, which would lead me to believe that it doesn't need to even exist at all, either, but I'm sure I'm missing something.

@jakebailey

Jake Bailey (jakebailey) commented Feb 16, 2023

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) test top100
TypeScript Bot (@typescript-bot) user test this
TypeScript Bot (@typescript-bot) run dt
TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 16, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the extended test suite on this PR at c215aa8. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 16, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 16, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 16, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 16, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 16, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@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/146654/artifacts?artifactName=tgz&fileId=C55F8F23AFED2FCC7B8CA8743191E9878D1B6CB3EE1F76089175519B4C81E4BA02&fileName=/typescript-5.0.0-insiders.20230216.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@5.0.0-pr-49627-28".;

@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 16, 2023

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing main and refs/pull/49627/merge:

Everything looks good!

@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 16, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), 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

TypeScript Bot (typescript-bot) commented Feb 16, 2023

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing main and refs/pull/49627/merge:

Everything looks good!

@jakebailey

Jake Bailey (jakebailey) commented Feb 17, 2023

Copy link
Copy Markdown
MemberAuthor

Forgive the force push; this had so many commits that I wanted to still be able to see the test diffs.

I've updated the logic to share the same node within the same chain instead of repeating it multiple times; I think it's about as fast as it can get.

I also dropped the added property on the context; it turns out that wasn't safe because it's technically possible that another part of the code temporarily changes the enclosing declaration, but we could then accidentally inject new variables into a different chain. Now, we just walk up the ancestors to find an existing one, creating one if needed.

jakebailey
Jake Bailey (jakebailey) commented Feb 17, 2023
View reviewed changes
Comment threadtests/baselines/reference/declarationEmitGlobalThisPreserved.js Outdated
export type AsFunctionType = (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN;


//// [DtsFileErrors]

@jakebaileyJake Bailey (jakebailey)Feb 17, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Note the the errors going away here; I constructed these tests such that they will emit a d.ts error when they're wrong, rather than us having to figure out if the output looks right or not.

weswigham
Wesley Wigham (weswigham) reviewed Feb 17, 2023
View reviewed changes
Comment threadsrc/compiler/checker.ts Outdated
weswigham
Wesley Wigham (weswigham) reviewed Feb 17, 2023
View reviewed changes
Comment threadsrc/compiler/checker.ts Outdated
@jakebailey

Jake Bailey (jakebailey) commented Feb 18, 2023

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) perf test faster

@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 18, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the abridged perf test suite on this PR at 27c0c92. You can monitor the build here.

Update: The results are in!

sandersn
Nathan Shively-Sanders (sandersn) reviewed Feb 18, 2023
View reviewed changes

@sandersnNathan Shively-Sanders (sandersn) 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.

I don't think I understand the overall flow of creating a signature declaration enough to properly sign off, but I did have a couple of questions after reading the code.

Comment threadsrc/compiler/checker.ts
&& signature.declaration
&& signature.declaration !== context.enclosingDeclaration
&& !isInJSFile(signature.declaration)
&& some(expandedParams)

@sandersnNathan Shively-Sanders (sandersn)Feb 18, 2023

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.

getExpandedParameters looks like it always returns an array of length 1, at least unless the rest type is somehow a 0-constituent union, but I think those are always impossible.

If the check is there just because the types could allow an empty array, maybe it should be an assert in the body of the if instead.

@jakebaileyJake Bailey (jakebailey)Feb 18, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Check above, and you'll see:

constexpandedParams=getExpandedParameters(signature,/*skipUnionExpanding*/true)[0];

@jakebaileyJake Bailey (jakebailey)Feb 18, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

(Later I might see if we can avoid constructing a bunch of random arrays we don't need...)

Comment threadsrc/compiler/checker.ts Outdated
@typescript-bot

TypeScript Bot (typescript-bot) commented Feb 18, 2023

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..49627

Metricmain49627DeltaBestWorstp-value
Angular - node (v16.17.1, x64)
Memory used359,093k (± 0.01%)359,112k (± 0.01%)~359,063k359,132kp=0.173 n=6
Parse Time4.16s (± 0.42%)4.15s (± 0.45%)~4.12s4.17sp=0.145 n=6
Bind Time1.24s (± 0.33%)1.24s (± 0.44%)~1.23s1.24sp=0.054 n=6
Check Time9.52s (± 0.44%)9.50s (± 0.37%)~9.46s9.55sp=0.373 n=6
Emit Time8.10s (± 0.68%)8.10s (± 0.73%)~8.01s8.18sp=1.000 n=6
Total Time23.02s (± 0.40%)22.98s (± 0.33%)~22.89s23.08sp=0.575 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used191,731k (± 0.02%)191,770k (± 0.03%)~191,706k191,840kp=0.298 n=6
Parse Time1.81s (± 0.97%)1.81s (± 0.68%)~1.78s1.81sp=1.000 n=6
Bind Time0.84s (± 0.48%)0.84s (± 0.00%)~0.84s0.84sp=0.405 n=6
Check Time10.13s (± 0.70%)10.15s (± 0.47%)~10.06s10.20sp=0.629 n=6
Emit Time3.04s (± 0.70%)3.03s (± 0.65%)~3.01s3.05sp=0.287 n=6
Total Time15.82s (± 0.62%)15.82s (± 0.21%)~15.77s15.86sp=0.520 n=6
Monaco - node (v16.17.1, x64)
Memory used343,537k (± 0.01%)343,559k (± 0.01%)~343,544k343,591kp=0.173 n=6
Parse Time3.10s (± 0.53%)3.14s (± 0.45%)+0.04s (+ 1.40%)3.12s3.16sp=0.006 n=6
Bind Time1.11s (± 0.73%)1.12s (± 0.92%)~1.10s1.13sp=0.546 n=6
Check Time7.79s (± 0.70%)7.78s (± 0.40%)~7.74s7.83sp=0.687 n=6
Emit Time4.51s (± 0.33%)4.52s (± 0.62%)~4.50s4.56sp=0.506 n=6
Total Time16.51s (± 0.37%)16.56s (± 0.22%)~16.51s16.62sp=0.170 n=6
TFS - node (v16.17.1, x64)
Memory used299,646k (± 0.00%)299,649k (± 0.01%)~299,614k299,676kp=0.629 n=6
Parse Time2.45s (± 1.08%)2.45s (± 0.95%)~2.42s2.48sp=1.000 n=6
Bind Time1.26s (± 0.65%)1.25s (± 0.83%)~1.24s1.27sp=0.865 n=6
Check Time7.23s (± 0.43%)7.21s (± 0.36%)~7.18s7.24sp=0.197 n=6
Emit Time4.21s (± 0.47%)4.24s (± 0.80%)~4.19s4.29sp=0.120 n=6
Total Time15.14s (± 0.29%)15.15s (± 0.39%)~15.06s15.23sp=0.873 n=6
material-ui - node (v16.17.1, x64)
Memory used475,972k (± 0.00%)475,989k (± 0.00%)~475,969k476,006kp=0.173 n=6
Parse Time3.68s (± 0.24%)3.68s (± 0.20%)~3.67s3.69sp=0.798 n=6
Bind Time1.02s (± 0.62%)1.02s (± 0.40%)~1.02s1.03sp=0.673 n=6
Check Time18.31s (± 1.24%)18.28s (± 0.99%)~18.16s18.64sp=0.747 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time23.01s (± 0.97%)22.98s (± 0.81%)~22.85s23.35sp=0.936 n=6
xstate - node (v16.17.1, x64)
Memory used546,950k (± 0.02%)546,961k (± 0.03%)~546,697k547,177kp=0.810 n=6
Parse Time4.77s (± 0.54%)4.78s (± 0.45%)~4.75s4.80sp=0.684 n=6
Bind Time1.85s (± 0.22%)1.85s (± 0.44%)~1.84s1.86sp=0.206 n=6
Check Time3.07s (± 0.78%)3.08s (± 0.48%)~3.06s3.10sp=0.560 n=6
Emit Time0.09s (± 4.45%)0.09s (± 4.45%)~0.09s0.10sp=1.000 n=6
Total Time9.78s (± 0.45%)9.80s (± 0.30%)~9.75s9.83sp=0.748 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-135-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
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)
BenchmarkNameIterations
Current496276
Baselinemain6

Developer Information:

Download Benchmark

@jakebailey
Jake Bailey (jakebailey) merged commit 8cc216a into microsoft:mainFeb 18, 2023
@jakebaileyJake Bailey (jakebailey) mentioned this pull request Feb 18, 2023
Closed
@jakebaileyJake Bailey (jakebailey) mentioned this pull request Sep 8, 2023
Closed
@jakebaileyJake Bailey (jakebailey) mentioned this pull request Sep 21, 2023
Merged
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sandersnNathan Shively-Sanders (sandersn)sandersn left review comments
@weswighamWesley Wigham (weswigham)weswigham approved these changes

Assignees

@jakebaileyJake Bailey (jakebailey)

Labels

Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone

Projects

PR Backlog
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

globalThis sometimes being stripped from type emition can leads to errors in emited definition

4 participants

@jakebailey@typescript-bot@sandersn@weswigham