Skip to content
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.

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

Increase type instantiation depth limit #45025

Merged
merged 4 commits into from
Aug 17, 2021
Merged

Conversation

ahejlsberg
Copy link
Member

This PR increases the type instantiation depth limit from 50 to 500. Resolution of recursive types (such as conditional types and indexed access types) can cause deeply nested invocations in the type instantiation logic of the compiler, which may ultimately cause stack overflows during compilation. For this reason we limit nested invocations of the instantiateType function, reporting a "Type instantiation is excessively deep and possibly infinite" error when the limit is reached. The current limit of 50 is generally reasonable, but in some scenarios involving tuple types and template literal types it is quickly reached. For example, the type

type GetChars<S> = S extends `${infer Char}${infer Rest}` ? Char | GetChars<Rest> : never;

extracts a union of the single characters in a literal string and reaches the instantiation depth limit after just 24 characters (each level of recursion involves two invocations of instantiateType). Likewise, utility types to manipulate tuple types often suffer from the same restrictions. It is sometimes possible to "batch process" multiple constituents at once, as in

type GetChars<S> =
    S extends `${infer C1}${infer C2}${infer C3}${infer C4}${infer Rest}` ? C1 | C2 | C3 | C4 | GetChars<Rest> :
    S extends `${infer C1}${infer Rest}` ? C1 | GetChars<Rest> : never;

but this is cumbersome and often too complex.

Since the instantiation depth limit is a reasonable approximation of call stack depth in the compiler, if we assume each recursive instantiateType call involves 5 stack frames and each stack frame consumes 100 bytes, then 500 levels of recursion roughly corresponds to 250K bytes, which is 25-50% of the default stack size in Node.js. That seems appropriate.

It's been suggested we make the depth limit configurable through a compiler option. I remain opposed to that as it is ultimately an implementation detail of the compiler that very well could change.

@@ -85,8 +84,6 @@ tests/cases/compiler/recursiveConditionalTypes.ts(116,9): error TS2345: Argument
type TT2 = TupleOf<number, number>;
type TT3 = TupleOf<number, any>;
type TT4 = TupleOf<number, 100>; // Depth error
Copy link
Member

Choose a reason for hiding this comment

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

The comment in tests/cases/compiler/recursiveConditionalTypes.ts will need to be amended.

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this
@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 14, 2021

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 4f79295. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 14, 2021

Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 4f79295. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 14, 2021

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 4f79295. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 14, 2021

Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 4f79295. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 14, 2021

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

@AnyhowStep
Copy link
Contributor

I love you

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 14, 2021

Hey @DanielRosenwasser, 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/106764/artifacts?artifactName=tgz&fileId=5C079C6A6740444AA1A534C55FECB56270903505B9A9A29E546161CE9AEA0C6402&fileName=/typescript-4.4.0-insiders.20210714.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.4.0-pr-45025-7".;

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..45025

Metric main 45025 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 343,509k (± 0.03%) 343,514k (± 0.02%) +5k (+ 0.00%) 343,366k 343,714k
Parse Time 1.79s (± 0.45%) 1.80s (± 0.50%) +0.01s (+ 0.39%) 1.78s 1.82s
Bind Time 0.85s (± 1.11%) 0.84s (± 0.66%) -0.01s (- 0.59%) 0.83s 0.86s
Check Time 5.34s (± 0.49%) 5.33s (± 0.56%) -0.01s (- 0.13%) 5.27s 5.39s
Emit Time 5.85s (± 0.53%) 5.81s (± 0.49%) -0.04s (- 0.70%) 5.74s 5.87s
Total Time 13.83s (± 0.36%) 13.78s (± 0.37%) -0.05s (- 0.34%) 13.64s 13.87s
Compiler-Unions - node (v10.16.3, x64)
Memory used 201,233k (± 0.02%) 201,207k (± 0.04%) -26k (- 0.01%) 201,035k 201,388k
Parse Time 0.78s (± 0.90%) 0.78s (± 0.60%) -0.00s (- 0.13%) 0.77s 0.79s
Bind Time 0.52s (± 1.30%) 0.53s (± 1.14%) +0.00s (+ 0.19%) 0.51s 0.54s
Check Time 7.81s (± 0.61%) 7.81s (± 0.65%) +0.00s (+ 0.05%) 7.70s 7.93s
Emit Time 2.45s (± 0.90%) 2.44s (± 0.56%) -0.01s (- 0.49%) 2.41s 2.47s
Total Time 11.57s (± 0.56%) 11.56s (± 0.49%) -0.01s (- 0.09%) 11.41s 11.68s
Monaco - node (v10.16.3, x64)
Memory used 340,460k (± 0.02%) 340,446k (± 0.02%) -14k (- 0.00%) 340,273k 340,624k
Parse Time 1.45s (± 0.60%) 1.46s (± 0.68%) +0.01s (+ 0.41%) 1.43s 1.48s
Bind Time 0.75s (± 0.91%) 0.75s (± 0.53%) +0.00s (+ 0.67%) 0.74s 0.76s
Check Time 5.38s (± 0.57%) 5.35s (± 0.52%) -0.03s (- 0.52%) 5.28s 5.42s
Emit Time 3.15s (± 1.03%) 3.16s (± 1.34%) +0.01s (+ 0.29%) 3.10s 3.27s
Total Time 10.73s (± 0.49%) 10.72s (± 0.54%) -0.01s (- 0.07%) 10.64s 10.86s
TFS - node (v10.16.3, x64)
Memory used 303,771k (± 0.02%) 303,819k (± 0.02%) +48k (+ 0.02%) 303,671k 303,973k
Parse Time 1.18s (± 0.40%) 1.18s (± 0.47%) +0.00s (+ 0.17%) 1.17s 1.20s
Bind Time 0.71s (± 0.69%) 0.71s (± 0.48%) -0.01s (- 0.98%) 0.70s 0.71s
Check Time 4.89s (± 0.33%) 4.88s (± 0.56%) -0.01s (- 0.10%) 4.82s 4.94s
Emit Time 3.33s (± 1.00%) 3.33s (± 1.22%) +0.00s (+ 0.12%) 3.23s 3.40s
Total Time 10.11s (± 0.41%) 10.10s (± 0.61%) -0.01s (- 0.10%) 9.98s 10.21s
material-ui - node (v10.16.3, x64)
Memory used 469,650k (± 0.01%) 469,619k (± 0.02%) -31k (- 0.01%) 469,498k 469,861k
Parse Time 1.73s (± 0.74%) 1.73s (± 0.54%) 0.00s ( 0.00%) 1.70s 1.74s
Bind Time 0.67s (± 0.89%) 0.67s (± 1.29%) +0.01s (+ 0.90%) 0.66s 0.69s
Check Time 14.17s (± 0.49%) 14.16s (± 0.43%) -0.01s (- 0.04%) 14.05s 14.34s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.57s (± 0.46%) 16.56s (± 0.42%) -0.00s (- 0.02%) 16.43s 16.77s
Angular - node (v12.1.0, x64)
Memory used 321,697k (± 0.03%) 321,646k (± 0.02%) -51k (- 0.02%) 321,489k 321,826k
Parse Time 1.78s (± 0.62%) 1.78s (± 0.60%) +0.00s (+ 0.06%) 1.77s 1.81s
Bind Time 0.83s (± 1.02%) 0.82s (± 0.75%) -0.01s (- 0.97%) 0.81s 0.83s
Check Time 5.18s (± 0.49%) 5.18s (± 0.54%) -0.00s (- 0.10%) 5.12s 5.23s
Emit Time 6.03s (± 0.43%) 6.02s (± 0.67%) -0.01s (- 0.20%) 5.95s 6.12s
Total Time 13.83s (± 0.39%) 13.80s (± 0.44%) -0.02s (- 0.15%) 13.68s 13.98s
Compiler-Unions - node (v12.1.0, x64)
Memory used 188,688k (± 0.02%) 188,676k (± 0.09%) -12k (- 0.01%) 188,057k 188,977k
Parse Time 0.78s (± 0.63%) 0.77s (± 0.68%) -0.01s (- 0.90%) 0.76s 0.78s
Bind Time 0.53s (± 0.92%) 0.54s (± 0.92%) +0.00s (+ 0.37%) 0.53s 0.55s
Check Time 7.34s (± 0.79%) 7.32s (± 0.79%) -0.02s (- 0.22%) 7.22s 7.45s
Emit Time 2.42s (± 0.97%) 2.44s (± 0.76%) +0.02s (+ 0.70%) 2.41s 2.49s
Total Time 11.07s (± 0.66%) 11.07s (± 0.60%) -0.00s (- 0.02%) 10.94s 11.18s
Monaco - node (v12.1.0, x64)
Memory used 323,640k (± 0.01%) 323,611k (± 0.02%) -29k (- 0.01%) 323,494k 323,713k
Parse Time 1.42s (± 0.78%) 1.42s (± 0.48%) -0.01s (- 0.49%) 1.40s 1.43s
Bind Time 0.73s (± 0.65%) 0.72s (± 0.80%) -0.01s (- 0.96%) 0.71s 0.74s
Check Time 5.24s (± 0.62%) 5.21s (± 0.26%) -0.03s (- 0.53%) 5.18s 5.25s
Emit Time 3.16s (± 0.61%) 3.17s (± 0.96%) +0.00s (+ 0.16%) 3.13s 3.27s
Total Time 10.55s (± 0.48%) 10.52s (± 0.29%) -0.03s (- 0.31%) 10.47s 10.63s
TFS - node (v12.1.0, x64)
Memory used 288,573k (± 0.02%) 288,541k (± 0.02%) -32k (- 0.01%) 288,395k 288,687k
Parse Time 1.19s (± 0.80%) 1.19s (± 0.63%) -0.01s (- 0.59%) 1.18s 1.21s
Bind Time 0.69s (± 1.09%) 0.70s (± 0.71%) +0.00s (+ 0.14%) 0.69s 0.71s
Check Time 4.81s (± 0.39%) 4.81s (± 0.41%) -0.00s (- 0.02%) 4.76s 4.86s
Emit Time 3.33s (± 0.55%) 3.36s (± 0.53%) +0.03s (+ 0.87%) 3.32s 3.40s
Total Time 10.03s (± 0.29%) 10.05s (± 0.31%) +0.02s (+ 0.20%) 9.97s 10.11s
material-ui - node (v12.1.0, x64)
Memory used 448,306k (± 0.07%) 448,383k (± 0.01%) +78k (+ 0.02%) 448,249k 448,451k
Parse Time 1.72s (± 0.43%) 1.72s (± 0.59%) -0.00s (- 0.06%) 1.70s 1.74s
Bind Time 0.65s (± 0.69%) 0.65s (± 0.72%) +0.00s (+ 0.31%) 0.64s 0.66s
Check Time 12.77s (± 0.95%) 12.75s (± 0.56%) -0.02s (- 0.15%) 12.59s 12.89s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.13s (± 0.79%) 15.12s (± 0.54%) -0.01s (- 0.09%) 14.94s 15.27s
Angular - node (v14.15.1, x64)
Memory used 320,219k (± 0.01%) 320,222k (± 0.01%) +3k (+ 0.00%) 320,166k 320,256k
Parse Time 1.81s (± 0.99%) 1.80s (± 0.74%) -0.01s (- 0.39%) 1.78s 1.83s
Bind Time 0.88s (± 0.57%) 0.87s (± 0.68%) -0.00s (- 0.11%) 0.86s 0.89s
Check Time 5.20s (± 0.57%) 5.19s (± 0.16%) -0.01s (- 0.12%) 5.18s 5.22s
Emit Time 6.16s (± 0.64%) 6.11s (± 0.60%) -0.05s (- 0.80%) 5.98s 6.16s
Total Time 14.04s (± 0.48%) 13.98s (± 0.36%) -0.07s (- 0.47%) 13.82s 14.09s
Compiler-Unions - node (v14.15.1, x64)
Memory used 189,578k (± 0.58%) 188,420k (± 0.69%) -1,158k (- 0.61%) 186,344k 190,583k
Parse Time 0.80s (± 0.61%) 0.80s (± 1.18%) +0.00s (+ 0.00%) 0.79s 0.84s
Bind Time 0.56s (± 0.65%) 0.56s (± 1.10%) -0.00s (- 0.53%) 0.55s 0.58s
Check Time 7.46s (± 0.57%) 7.53s (± 0.48%) +0.07s (+ 0.88%) 7.45s 7.59s
Emit Time 2.42s (± 0.55%) 2.46s (± 1.29%) +0.04s (+ 1.74%) 2.40s 2.53s
Total Time 11.24s (± 0.47%) 11.35s (± 0.36%) +0.11s (+ 0.94%) 11.26s 11.49s
Monaco - node (v14.15.1, x64)
Memory used 322,367k (± 0.01%) 322,364k (± 0.00%) -3k (- 0.00%) 322,342k 322,383k
Parse Time 1.49s (± 0.89%) 1.48s (± 0.71%) -0.01s (- 0.67%) 1.46s 1.51s
Bind Time 0.75s (± 0.69%) 0.75s (± 1.00%) +0.00s (+ 0.40%) 0.74s 0.77s
Check Time 5.20s (± 0.57%) 5.20s (± 0.50%) -0.01s (- 0.15%) 5.15s 5.26s
Emit Time 3.20s (± 0.59%) 3.21s (± 0.70%) +0.01s (+ 0.31%) 3.16s 3.28s
Total Time 10.64s (± 0.32%) 10.64s (± 0.48%) 0.00s ( 0.00%) 10.53s 10.77s
TFS - node (v14.15.1, x64)
Memory used 287,540k (± 0.01%) 287,547k (± 0.01%) +7k (+ 0.00%) 287,499k 287,605k
Parse Time 1.26s (± 1.56%) 1.25s (± 0.75%) -0.01s (- 0.56%) 1.23s 1.27s
Bind Time 0.72s (± 0.80%) 0.72s (± 0.51%) -0.00s (- 0.56%) 0.71s 0.72s
Check Time 4.83s (± 0.51%) 4.86s (± 0.46%) +0.03s (+ 0.54%) 4.82s 4.92s
Emit Time 3.41s (± 0.76%) 3.46s (± 0.90%) +0.05s (+ 1.41%) 3.40s 3.54s
Total Time 10.22s (± 0.48%) 10.29s (± 0.46%) +0.06s (+ 0.62%) 10.21s 10.43s
material-ui - node (v14.15.1, x64)
Memory used 446,768k (± 0.00%) 446,769k (± 0.01%) +1k (+ 0.00%) 446,665k 446,796k
Parse Time 1.76s (± 0.64%) 1.76s (± 0.56%) +0.00s (+ 0.17%) 1.74s 1.78s
Bind Time 0.69s (± 0.65%) 0.69s (± 0.81%) -0.00s (- 0.00%) 0.68s 0.70s
Check Time 12.95s (± 0.70%) 12.90s (± 0.33%) -0.05s (- 0.42%) 12.80s 13.00s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.40s (± 0.56%) 15.35s (± 0.29%) -0.05s (- 0.32%) 15.25s 15.45s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-206-generic
Architecturex64
Available Memory16 GB
Available Memory1 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)
Benchmark Name Iterations
Current 45025 10
Baseline main 10

Developer Information:

Download Benchmark

@ahejlsberg
Copy link
Member Author

Tests and performance are unaffected, as expected. The only possible causes for concern with this change are (1) stack overflows could occur if each recursive invocation of instantiateType consumes more stack space than we have estimated, and (2) because we permit 10x more recursive invocations, it takes longer to report "Type instantiation is excessively deep..." errors, which may make language services less responsive.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

It's harder to make this number smaller rather than larger, so we really should be sure that we won't stack overflow with 500 in any cases, since adjusting it down if we are is going to be way breakier.

@jjangga0214
Copy link
Contributor

jjangga0214 commented Jul 15, 2021

Thanks for the attention to the issue :)

@weswigham

It's harder to make this number smaller rather than larger,

I believe this is one of the strengths of configurable depth limit/count. Maintainers would be able to modify the default values progressively, in more freedom without too much worry.

@ahejlsberg

It's been suggested we make the depth limit configurable through a compiler option. I remain opposed to that as it is ultimately an implementation detail of the compiler that very well could change.

I understand that change of implementation detail might require breaking change of configuration.
What about taking some time to think and fix the newly exposed configuration?
I believe, with thorough care, it can be stabilized someday.

So I suggest introducing experimental configuration and stabilizing it later.

500 levels of recursion roughly corresponds to 250K bytes, which is 25-50% of the default stack size in Node.js. That seems appropriate.

This is where configurations resolve uncertainty. When people don't use the default stack size(larger/smaller), they would also naturally want to configure tsc's recursive limit, by their own free decision.

@jjangga0214
Copy link
Contributor

jjangga0214 commented Jul 15, 2021

MEMO: relation with #34933 and #44997

This issue(#45025) is related to #34933, but does not resolve it. This is because there can be (theoretically) already published (including famous libraries) codes which even the increased limit(500 depth) cannot cover (e.g. needs 505 depth, 570 depth).
To fully resolve this broken ecosystem, the limitations should be configurable(#44997), I believe.

@sandersn
Copy link
Member

@weswigham @ahejlsberg Now that 4.5 development has started, is this ready to merge, or does the number need more experimentation?

(I'm leaning toward merging given our design meeting discussion.)

@ahejlsberg ahejlsberg merged commit 79474fd into main Aug 17, 2021
@ahejlsberg ahejlsberg deleted the increaseInstantiationDepth branch August 17, 2021 14:00
@colinhacks
Copy link

@AnyhowStep Time to throw the party you've been planning since 2019 😝

@thelinuxlich
Copy link

What happened to this fix?

@Dragon-Hatcher
Copy link

What happened to this fix?

It was reverted in #45711 and was replaced with a tail recursion strategy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants