- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.1k
Fix spurious circularity caused by removeOptionalityFromDeclaredType, cache result #52696
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
Conversation
| @typescript-bot test this | 
| Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at b2d122d. You can monitor the build here. Update: The results are in! | 
| Heya @jakebailey, I've started to run the perf test suite on this PR at b2d122d. You can monitor the build here. Update: The results are in! | 
| Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at b2d122d. You can monitor the build here. Update: The results are in! | 
| Heya @jakebailey, I've started to run the extended test suite on this PR at b2d122d. You can monitor the build here. | 
| Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at b2d122d. You can monitor the build here. | 
| Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at b2d122d. You can monitor the build here. Update: The results are in! | 
| @jakebailey Here are the results of running the user test suite comparing  Everything looks good! | 
| @jakebailey Here are the results of running the user test suite comparing  Something interesting changed - please have a look. Detailsgrunt
 | 
| Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. | 
| @jakebailey Here they are:
 CompilerComparison Report - main..52696
 
System
 
 
Hosts
 
 
Scenarios
 
 
 TSServerComparison Report - main..52696
 
System
 
 
Hosts
 
 
Scenarios
 
 
 StartupComparison Report - main..52696
 
System
 
 
Hosts
 
 
Scenarios
 
 
 Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @jakebailey Here are the results of running the top-repos suite comparing  Something interesting changed - please have a look. Detailsakveo/ngx-admin
 | 
| @jakebailey Here are some more interesting changes from running the top-repos suite Detailsconwnet/github1s
 | 
| @jakebailey Here are some more interesting changes from running the top-repos suite DetailsJedWatson/react-select
 | 
| @jakebailey Here are some more interesting changes from running the top-repos suite Detailsmobxjs/mobx
 | 
| @jakebailey Here are some more interesting changes from running the top-repos suite Detailsreact-bootstrap/react-bootstrap
 | 
| @jakebailey Here are some more interesting changes from running the top-repos suite Detailsstatelyai/xstate
 | 
| @jakebailey Here are some more interesting changes from running the top-repos suite Detailswithfig/autocomplete
 | 
| @jakebailey Here are the results of running the top-repos suite comparing  Everything looks good! | 
| Fixed my oops. I was misled by the variable name used by the old function; it's not related to the annotation entirely anymore. | 
| @typescript-bot test this | 
| Heya @jakebailey, I've started to run the extended test suite on this PR at 503dacc. You can monitor the build here. | 
| Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 503dacc. You can monitor the build here. Update: The results are in! | 
| Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 503dacc. You can monitor the build here. | 
| Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 503dacc. You can monitor the build here. Update: The results are in! | 
| Heya @jakebailey, I've started to run the perf test suite on this PR at 503dacc. You can monitor the build here. Update: The results are in! | 
| Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at 503dacc. You can monitor the build here. Update: The results are in! | 
| @jakebailey Here are the results of running the user test suite comparing  Something interesting changed - please have a look. Detailswebpack
 | 
| @jakebailey Here are the results of running the user test suite comparing  Everything looks good! | 
| Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. | 
| @jakebailey Here they are:
 CompilerComparison Report - main..52696
 
System
 
 
Hosts
 
 
Scenarios
 
 
 TSServerComparison Report - main..52696
 
System
 
 
Hosts
 
 
Scenarios
 
 
 StartupComparison Report - main..52696
 
System
 
 
Hosts
 
 
Scenarios
 
 
 Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @jakebailey Here are the results of running the top-repos suite comparing  Something interesting changed - please have a look. Detailsangular/angular-cli
 | 
| @jakebailey Here are some more interesting changes from running the top-repos suite Detailsionic-team/ionic-framework
 | 
| @jakebailey Here are some more interesting changes from running the top-repos suite DetailsReactiveX/rxjs
 | 
| @jakebailey Here are the results of running the top-repos suite comparing  Everything looks good! | 
| @typescript-bot pack this | 
| Heya @jakebailey, I've started to run the tarball bundle task on this PR at 503dacc. You can monitor the build here. | 
| Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your  and then running  | 
Fixes #50795
The way the
pushTypeResolutionsystem works is that eachTypeSystemPropertyNamemaps to a currently-being-computed property on aNodeLinks,SymbolLinks,Type, etc, with the expectation that the value is set once the resolution stack is popped. If you try to push the sameTypeSystemPropertyNameonto the stack for the same thing, you get a circularity error because the thing isn't ready yet.#37023 added a
pushTypeResolutionto detect a circularity, but then usedTypeSystemPropertyName.DeclaredTypeeven thoughremoveOptionalityFromDeclaredTypenever actually setsdeclaredType. This led to spurious circularity errors depending on the order in which we ask for the types. In this bug's case, if we ask for semantic tokens first, we end up hitting what the old code thought was a cycle but is in fact not.This PR fixes the issue while still emitting the right circularity error added in #37023 by creating a new property on
NodeLinkswhich caches the computation that determines whether or not a parameter's initializer containsundefined. Then, we can add aTypeSystemPropertyNamethat is associated with that value, which then lets us detect cycles specifically in this part of the code, without reusing an unrelatedTypeSystemPropertyName.It also has the benefit of being faster because this computation is now cached rather than being reevaluated each time, and I've also switched it to use
checkDeclarationInitializerwhich is the "correct" way to grab the declaration's initializer type (usescheckExpressionCached, uses JSDoc when needed, and so on).