-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Conversation
@@ -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 |
There was a problem hiding this comment.
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.
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 4f79295. You can monitor the build here. |
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! |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 4f79295. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 4f79295. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 4f79295. You can monitor the build here. |
I love you |
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
@DanielRosenwasser Here they are:Comparison Report - main..45025
System
Hosts
Scenarios
Developer Information: |
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 |
There was a problem hiding this 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.
Thanks for the attention to the issue :)
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.
I understand that change of implementation detail might require breaking change of configuration. So I suggest introducing experimental configuration and stabilizing it later.
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. |
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). |
@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.) |
@AnyhowStep Time to throw the party you've been planning since 2019 😝 |
* Bump instantiation depth limit to 500 * Accept new baselines * Update tests * Accept new baselines
What happened to this fix? |
It was reverted in #45711 and was replaced with a tail recursion strategy. |
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 typeextracts 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 inbut 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.