-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Fix spurious circularity caused by removeOptionalityFromDeclaredType, cache result #52696
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
pushTypeResolution
system works is that eachTypeSystemPropertyName
maps 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 sameTypeSystemPropertyName
onto the stack for the same thing, you get a circularity error because the thing isn't ready yet.#37023 added a
pushTypeResolution
to detect a circularity, but then usedTypeSystemPropertyName.DeclaredType
even thoughremoveOptionalityFromDeclaredType
never 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
NodeLinks
which caches the computation that determines whether or not a parameter's initializer containsundefined
. Then, we can add aTypeSystemPropertyName
that 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
checkDeclarationInitializer
which is the "correct" way to grab the declaration's initializer type (usescheckExpressionCached
, uses JSDoc when needed, and so on).