-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Reuse cached resolved signatures early #60208
Reuse cached resolved signatures early #60208
Conversation
BaseClass extends new (...args: any[]) => any, | ||
>(Base: BaseClass): any; | ||
|
||
declare class Item extends ClientDocumentMixin(BaseItem) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
due to the circular nature of this example, the compiler goes into resolveCall
for the same node twice in a nested manner
- the first
getResolvedSignature
ends up callingresolveAnonymousTypeMembers
fortypeof BaseItem
- in the process it preemptively sets empty signatures, members, index infos on that type here
- but then it also computes real construct signatures here
- this in turn leads to calling
getResolvedSignature
again for the same node and all - in that inner call we see no construct signatures on
typeof BaseItem
(those were set to an empty array) so the error is raised becausesignaturesRelatedTo
fails to determine that this source (typeof BaseItem
) has the required target signatures - this inner call errors and exits, caching the
getCandidateForOverloadFailure
's result as thelinks.resolvedSignature
. That signature is(Base: new (...args: any[]) => any): any
- this leads to resolving
(): BaseItem
asgetDefaultConstructSignatures
and that replaces the empty construct signatures preemptively set byresolveAnonymousTypeMembers
- now we climb up the stack, going back to the first/outer
getResolvedSignature
call for this. When callinggetSignatureApplicabilityError
the compiler returns an error because it reuses cachedRelationComparisonResult.Failed
that was cached when relating the argument and parameter types in that inner call - so now the compiler computes
getCandidateForOverloadFailure
again and tries to elaborate this error (again!). - this time it succeeds when relating those 2 types because the construct signatures of
typeof BaseItem
changed "in-between". So the debug assert kicks finally kicks in and the compiler crashes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a ton for this PR and the explanation! It makes the crash make much more sense to me now.
I will say some of these steps still seem to have a bit of concerning logic and seem to help explain something else weird I noted on my issue, namely that there doesn't have to be a "real" loop in the base type for you to get an error like Argument of type 'typeof BaseItem' is not assignable to parameter of type 'new (...args: any[]) => any'.
Playground (make sure to make any edit to overcome the crash and view an error!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh your example is somewhat convoluted to the point that I'm not sure if it should error or not. It's definitely outside of the "regular TS" territory 😅 so all I was after here was to fix the crash. Once this gets fixed you might want to raise a new issue about the circularity issue if you thing it shouldn't be reported
@typescript-bot test it |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a promising idea in the absence of the possibility of fixing the assignability result inconsistency, but I left a question.
Maybe related fix: #49598 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question.
fixes #60202
cc @gabritto