Enforce a size limit in getSpreadType #40755
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a union is spread into a union, the sizes are multiplied, potentially resulting in an enormous union (especially if there are repeated spreads). This check detects cases that used to run out of memory. Fixes microsoft#40754
typescript-bot
added
the
For Uncommitted Bug
|
@typescript-bot user test this |
weswigham
reviewed
Sep 24, 2020
src/compiler/checker.ts
Outdated
| const resultSize = (left as UnionType).types.length * (right as UnionType).types.length; | ||
| if (resultSize > 100000) { | ||
| tracing.instant(tracing.Phase.Check, "getSpreadType_DepthLimit", { leftId: left.id, rightId: right.id }); | ||
| error(currentNode, Diagnostics.Spread_expression_produces_a_union_type_that_is_too_complex_to_represent); |
There was a problem hiding this comment.
I think this error is going to be frustrating to encounter with it also providing a suggestion for a workaround. What is the general-form workaround? Casting something somewhere to any? Should we add a related span/quickfix for that?
There was a problem hiding this comment.
I don't think there's a fix I'm sufficiently confident in to make it a quick fix. I gave this a new error number so that it would be easier to document/google.
There was a problem hiding this comment.
I did wonder about looking inside the union to figure out whether there's a smaller span we could squiggle.
There was a problem hiding this comment.
For the real code where this arose, the mitigation I suggested was adding a cast to each spread with a pre-existing type that listed all possible properties (optionally). I imagine any would also work, possible at the expense of completions and such.
|
If we broaden #34853, I'd probably drop the limit substantially. I based it on what would still compile quickly in my toy example. |
|
@typescript-bot test this |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
Since there are no occurrences of my new error code in the user test baseline diff, I'm assuming I can ignore the failure. |
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 6650496. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 6650496. You can monitor the build here. Update: The results are in! |
|
@typescript-bot test this |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 6650496. You can monitor the build here. |
amcasey
commented
Sep 25, 2020
src/compiler/checker.ts
Outdated
| if (resultSize > 100000) { | ||
| tracing.instant(tracing.Phase.Check, "getSpreadType_DepthLimit", { leftId: left.id, rightId: right.id }); | ||
| error(currentNode, Diagnostics.Spread_expression_produces_a_union_type_that_is_too_complex_to_represent); | ||
| return errorType; |
There was a problem hiding this comment.
@minestarks raised an interesting point: should this be anyType so that users have the option of ts-ignoring this error?
|
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. |
|
@DanielRosenwasser Here they are:Comparison Report - master..40755
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
This should probably use |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a union is spread into a union, the sizes are multiplied,
potentially resulting in an enormous union (especially if there are
repeated spreads). This check detects cases that used to run out of
memory.
Fixes #40754