Fix leaked alias symbol independent type params in printback #42211
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
…ce finding doesnt blow up in some scenarios
…ards and introducing better deep nesting heuristic for unions and intersections
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot perf test this - since this affects caches, this should cause some change to perf, but I can't predict if it'll be good or bad in real world scenarios; the caching is noncritial to fixing this, but making the extra caching conditional would complicate this change a bit, so I'd like to avoid it if possible. Also: And @typescript-bot pack this for good measure. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 3fe794d. You can monitor the build here. |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 3fe794d. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 3fe794d. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 3fe794d. You can monitor the build here. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 3fe794d. You can monitor the build here. |
|
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 |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
The user baselines all look great excepting a few new excessive stack warnings when calculating variance for some types in |
|
@weswigham Here they are:Comparison Report - master..42211
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
With #37910 merged, this should no longer have an outsized performance benefit (since it did the same thing with respect to expanding caching). I'll ask @typescript-bot perf test this again so we can see if removing the size limit really does make perf worse. If it does, I'll need to rejigger it to still do recursive checks but skip the cache for small unions; but I'd like to avoid that complexity if at all possible. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 7879343. You can monitor the build here. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..42211
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster |
|
Heya @weswigham, I've started to run the abridged perf test suite on this PR at 7879343. You can monitor the build here. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..42211
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Yeah, those perf numbers look fine. |
|
Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs. |
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 we measure the variance of an alias, an
Independenttype parameter is an unused type parameter - for whatever reason, it has no effect on the comparability of the resulting type. What this often means is that that type parameter also does not play into the construction of the the target type at all. That, in turn, means that due to interning, you can have multiple seemingly unique references to a type, which all point at a single underlying type - usually the one first manufactured within the type alias declaration itself. That underlying type, in turn, has an alias from when it was first created, and that alias's type arguments may contain type parameters only available within the scope of the original alias declaration. Normally, this would disqualify use of the alias as a reference external to that declaration, but we failed to detect such a case as our assumption is that if we are doing printback using a type parameter, that it will be made available in scope (otherwise, how did we get it?). Since this type parameter comes from far afield, we can't safely do printback using it, however, in this fix, rather than issuing an error in such a circumstance (which would be unfortunate and hard to fix for users), I detect when this situation arises by checking the variance and visibility of the alias symbol type arguments, and substitute an unreachableIndependenttype argument withunknown, as it will not affect the resulting type's shape, thus creating a functioning declaration file.This approach required checking the variance of some type aliases where we previously had not bothered (usually simply because the test in question had not been written to require it), and in turn exposed a crash-y problem we had calculating variance while handling recursive references to unions and intersections within type aliases (and recursive normalization thereof - see the types in
recursivelyExpandingUnionNoStackoverflowas an example of a type which we crashed when fetching the variance on it). To fix that, #37910 (which just got merged intomaster) mostly handles it, however the size limit it originally imposed has to be removed, as we need the deeply nested type guarding part ofrecursiveTypeRelatedToeven for the smallest unions, as recursive type references can create situations where these can infinitely expand upon normalization. In addition, I've added an identity for unions and intersections with aliases intogetRecursionIdentityto improve how quickly we recognize these structures as deeply nested (the time it took to calculate variances inrecursiveTypeReferences2proved that needed).TL;DR: We now print
rather than the erroneous
where
PublicWrap's first type parameter isIndependent.Fixes #42079
Fixes #44727