Attempt to reduce types which are about to produce a complexity error #53739
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
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 2d21364. You can monitor the build here. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 2d21364. You can monitor the build here. |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 2d21364. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 2d21364. You can monitor the build here. |
|
Modified the implementation slightly to reduce each element being considered a bit more aggressively and then the test case from #53234. |
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 757c56a. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 757c56a. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 757c56a. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 757c56a. You can monitor the build here. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 757c56a. You can monitor the build here. Update: The results are in! |
|
Hey @weswigham, 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 |
|
@weswigham Here they are:
CompilerComparison Report - main..53739
System
Hosts
Scenarios
TSServerComparison Report - main..53739
System
Hosts
Scenarios
StartupComparison Report - main..53739
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster |
|
Heya @gabritto, I've started to run the abridged perf test suite on this PR at 757c56a. You can monitor the build here. Update: The results are in! |
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
gabritto
reviewed
Apr 11, 2023
| if (i === typeSet.length - 1 || isTypeAny(runningResult) || runningResult.flags & TypeFlags.Never) { | ||
| return runningResult; | ||
| } | ||
| if (!(runningResult.flags & TypeFlags.Union) || (runningResult as UnionType).types.length > typeSet.length) { |
There was a problem hiding this comment.
Why do we bail if the second condition here is true ((runningResult as UnionType).types.length > typeSet.length)? Is it because then the heuristic is not being helpful in reducing the number of types to intersect?
There was a problem hiding this comment.
Yeah. If going element-by-element is growing the type, this probably isn't helping, so we give up on it.
|
@gabritto Here they are:Comparison Report - main..53739
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
gabritto
approved these changes
Apr 13, 2023
|
@weswigham Is this ready to go? I don't know whether you want to double check xstate results first or not. |
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.
Basically #42772 but synced with
main, but also improves how often we defer union/intersection member types; namely with this we now always defer construction of a union or intersection's symbol's type if one of its' constituent symbols is itself deferred. I had to add this in to the original change, as in the intervening time between the original PR and now, we got noticeably worse at deferring union members, and actually started manufacturing members likerefeven if they weren't explicitly referenced (in this case, to check if the member can be used for contextual type discimination, which jsx now tries).The original PR description (which is also still true) for reference:
Fixes #53234