Skip to content

Consistently return errorType when detecting circularities - #56429

Closed
Mateusz Burzyński (Andarist) wants to merge 1 commit into
microsoft:mainfrom
Andarist:consistent-error-type-on-circularities
Closed

Consistently return errorType when detecting circularities#56429
Mateusz Burzyński (Andarist) wants to merge 1 commit into
microsoft:mainfrom
Andarist:consistent-error-type-on-circularities

Conversation

@Andarist

Copy link
Copy Markdown
Contributor

a quick experiment based on discussion/observations we had here: https://github.com/microsoft/TypeScript/pull/56258/files#r1393385776

@typescript-botTypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 16, 2023
@typescript-bot

Copy link
Copy Markdown
Contributor

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@Andarist
Mateusz Burzyński (Andarist) marked this pull request as draft November 16, 2023 08:35
witness.ts(8,14): error TS2729: Property 'x' is used before its initialization.
witness.ts(20,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'a' must be of type 'any', but here has type 'never'.
witness.ts(28,12): error TS2695: Left side of comma operator is unused and has no side effects.
witness.ts(29,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'co1' must be of type 'any', but here has type 'number'.

@AndaristMateusz Burzyński (Andarist)Nov 16, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

it seems that this is even a small improvement since the intention of the caller was not to raise those errors when the type is the errorType, see: https://github.dev/microsoft/TypeScript/blob/32b618c2d8f42948e7d6fa2e9b264079e30b5349/src/compiler/checker.ts#L42290-L42296

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.

Swinging back here, it seems sorta odd just from the PoV that a self referencing thing will just not have any errors. It seems like this test is mainly to "witness" the type of these expressions in errors, but maybe it's fine. Odd that the .types file does not show a change?

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.

Will have to do some forensics but I'm not sure why we even want to ignore these anyway...

@Andarist
Mateusz Burzyński (Andarist) marked this pull request as ready for review November 16, 2023 08:43
@jakebailey

Copy link
Copy Markdown
Member

@typescript-bot

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

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 df7435d. You can monitor the build here.

Update: The results are in!

@typescript-bot

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

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

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

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

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

Copy link
Copy Markdown
Contributor

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

@typescript-bot

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

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

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

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/158643/artifacts?artifactName=tgz&fileId=1E5FC2A3954F892F1155CE85799029CEE36C40AFE8FB71B39396BAA5D40F93DE02&fileName=/typescript-5.4.0-insiders.20231116.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.4.0-pr-56429-7".;

@Andarist

Copy link
Copy Markdown
ContributorAuthor

Honestly I like this better

Than the targeted if in #56258 ? Or in general? ;p

@typescript-bot

Copy link
Copy Markdown
Contributor

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

There were infrastructure failures potentially unrelated to your change:

  • 2 instances of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

@typescript-bot

Copy link
Copy Markdown
Contributor

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

Here they are:

Compiler

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Angular - node (v18.15.0, x64)
Memory used295,199k (± 0.01%)295,193k (± 0.01%)~295,167k295,220kp=0.810 n=6
Parse Time2.64s (± 0.31%)2.64s (± 0.44%)~2.63s2.66sp=0.738 n=6
Bind Time0.83s (± 0.91%)0.83s (± 0.66%)~0.82s0.83sp=0.476 n=6
Check Time8.03s (± 0.15%)8.04s (± 0.32%)~8.01s8.07sp=0.935 n=6
Emit Time7.08s (± 0.39%)7.07s (± 0.43%)~7.03s7.11sp=0.629 n=6
Total Time18.59s (± 0.16%)18.58s (± 0.21%)~18.52s18.63sp=0.687 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used191,662k (± 1.24%)191,669k (± 1.23%)~190,672k196,474kp=0.575 n=6
Parse Time1.36s (± 0.89%)1.36s (± 0.98%)~1.35s1.38sp=0.865 n=6
Bind Time0.72s (± 0.00%)0.72s (± 0.00%)~0.72s0.72sp=1.000 n=6
Check Time9.17s (± 0.25%)9.18s (± 0.24%)~9.15s9.21sp=0.744 n=6
Emit Time2.64s (± 0.56%)2.65s (± 0.73%)~2.62s2.67sp=0.372 n=6
Total Time13.89s (± 0.21%)13.91s (± 0.15%)~13.88s13.94sp=0.295 n=6
Monaco - node (v18.15.0, x64)
Memory used347,352k (± 0.00%)347,335k (± 0.01%)~347,302k347,381kp=0.128 n=6
Parse Time2.45s (± 0.31%)2.45s (± 0.68%)~2.42s2.47sp=1.000 n=6
Bind Time0.92s (± 0.56%)0.92s (± 0.56%)~0.92s0.93sp=1.000 n=6
Check Time6.89s (± 0.28%)6.91s (± 0.50%)~6.87s6.96sp=0.418 n=6
Emit Time4.05s (± 0.22%)4.05s (± 0.20%)~4.04s4.06sp=0.550 n=6
Total Time14.32s (± 0.14%)14.33s (± 0.36%)~14.28s14.41sp=0.744 n=6
TFS - node (v18.15.0, x64)
Memory used302,630k (± 0.01%)302,648k (± 0.01%)~302,610k302,705kp=0.520 n=6
Parse Time1.98s (± 1.06%)2.00s (± 0.86%)~1.98s2.02sp=0.118 n=6
Bind Time1.00s (± 1.03%)1.00s (± 1.36%)~0.99s1.02sp=0.933 n=6
Check Time6.28s (± 0.24%)6.28s (± 0.27%)~6.25s6.30sp=0.681 n=6
Emit Time3.58s (± 0.71%)3.57s (± 0.33%)~3.56s3.59sp=0.569 n=6
Total Time12.85s (± 0.22%)12.85s (± 0.21%)~12.82s12.89sp=0.936 n=6
material-ui - node (v18.15.0, x64)
Memory used470,557k (± 0.01%)470,581k (± 0.01%)~470,543k470,632kp=0.230 n=6
Parse Time2.57s (± 0.64%)2.58s (± 0.29%)~2.57s2.59sp=0.672 n=6
Bind Time0.99s (± 1.49%)1.00s (± 0.82%)~0.99s1.01sp=0.284 n=6
Check Time16.61s (± 0.42%)16.66s (± 0.38%)~16.54s16.71sp=0.261 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time20.18s (± 0.38%)20.24s (± 0.32%)~20.11s20.29sp=0.199 n=6
xstate - node (v18.15.0, x64)
Memory used512,818k (± 0.01%)512,815k (± 0.01%)~512,766k512,902kp=1.000 n=6
Parse Time3.27s (± 0.19%)3.28s (± 0.17%)~3.27s3.28sp=0.201 n=6
Bind Time1.54s (± 0.53%)1.54s (± 0.49%)~1.53s1.55sp=0.306 n=6
Check Time2.86s (± 0.65%)2.87s (± 0.42%)~2.86s2.89sp=0.416 n=6
Emit Time0.09s (± 6.44%)0.08s (± 0.00%)~0.08s0.08sp=0.071 n=6
Total Time7.76s (± 0.25%)7.76s (± 0.16%)~7.75s7.78sp=0.366 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

tsserver

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen2,360ms (± 0.51%)2,377ms (± 0.59%)~2,363ms2,399msp=0.078 n=6
Req 2 - geterr5,393ms (± 1.73%)5,361ms (± 1.72%)~5,307ms5,547msp=0.936 n=6
Req 3 - references325ms (± 0.42%)327ms (± 0.79%)~323ms331msp=0.358 n=6
Req 4 - navto277ms (± 1.20%)279ms (± 0.27%)~278ms280msp=0.406 n=6
Req 5 - completionInfo count1,356 (± 0.00%)1,356 (± 0.00%)~1,3561,356p=1.000 n=6
Req 5 - completionInfo84ms (± 7.21%)80ms (± 7.15%)~75ms91msp=0.295 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen2,494ms (± 0.77%)2,493ms (± 0.62%)~2,475ms2,517msp=1.000 n=6
Req 2 - geterr4,085ms (± 1.86%)4,080ms (± 1.87%)~4,012ms4,179msp=0.748 n=6
Req 3 - references341ms (± 1.69%)340ms (± 1.76%)~332ms345msp=0.627 n=6
Req 4 - navto283ms (± 0.43%)283ms (± 0.69%)~281ms286msp=1.000 n=6
Req 5 - completionInfo count1,518 (± 0.00%)1,518 (± 0.00%)~1,5181,518p=1.000 n=6
Req 5 - completionInfo86ms (± 6.98%)85ms (± 7.21%)~77ms90msp=1.000 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen2,595ms (± 0.68%)2,604ms (± 0.36%)~2,595ms2,619msp=0.419 n=6
Req 2 - geterr1,729ms (± 1.83%)1,723ms (± 2.36%)~1,677ms1,772msp=0.936 n=6
Req 3 - references119ms (± 7.10%)105ms (± 5.81%)🟩-14ms (-12.04%)101ms117msp=0.029 n=6
Req 4 - navto367ms (± 0.58%)367ms (± 0.57%)~364ms369msp=0.743 n=6
Req 5 - completionInfo count2,073 (± 0.00%)2,073 (± 0.00%)~2,0732,073p=1.000 n=6
Req 5 - completionInfo311ms (± 1.97%)305ms (± 1.99%)~300ms316msp=0.077 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Startup

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
tsc-startup - node (v18.15.0, x64)
Execution time152.79ms (± 0.18%)152.75ms (± 0.17%)~151.57ms156.08msp=0.364 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time228.18ms (± 0.17%)227.86ms (± 0.14%)-0.32ms (- 0.14%)226.69ms230.44msp=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time229.06ms (± 0.17%)228.86ms (± 0.17%)-0.21ms (- 0.09%)227.21ms234.76msp=0.000 n=600
typescript-startup - node (v18.15.0, x64)
Execution time229.44ms (± 0.16%)229.30ms (± 0.17%)-0.14ms (- 0.06%)227.41ms232.45msp=0.000 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@jakebailey

Copy link
Copy Markdown
Member

Honestly I like this better

Than the targeted if in #56258 ? Or in general? ;p

Both!

@typescript-bot

Copy link
Copy Markdown
Contributor

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

Everything looks good!

@github-project-automationgithub-project-automationBot moved this from Waiting on author to Done in PR BacklogDec 8, 2025
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Jun 8, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Uncommitted BugPR for untriaged, rejected, closed or missing bug

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@Andarist@typescript-bot@jakebailey@sandersn