-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Defer checking of class static blocks #59607
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
base: main
Are you sure you want to change the base?
Conversation
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.
this one was already fixed by #53549 , this is just an extra test case
@typescript-bot test it |
Hey @RyanCavanaugh, the results of running the DT tests are ready. Everything looks the same! |
@RyanCavanaugh Here are the results of running the user tests with tsc comparing Everything looks good! |
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@RyanCavanaugh Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
src/compiler/checker.ts
Outdated
@@ -47937,7 +47937,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
case SyntaxKind.MethodSignature: | |||
return checkMethodDeclaration(node as MethodDeclaration | MethodSignature); | |||
case SyntaxKind.ClassStaticBlockDeclaration: | |||
return checkClassStaticBlockDeclaration(node as ClassStaticBlockDeclaration); | |||
return checkNodeDeferred(node); |
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.
Maybe a stupid question, but why is it that this matters here, yet MethodSignatures are not deferred?
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.
As in, can't we write that same local type declaration in a method and it breaks? Or is that fine because the body gets deferred?
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.
My understanding is that:
- The same local type declaration in a method shouldn't suppress the error either.
- Object literal methods are deferred but class methods are not. If I try to defer class methods... a ton of tests break. So there has to be a reason behind why they are not deferred. Perhaps it's important for the variance worker?
- Class static blocks conceptually execute after the class declaration and they shouldn't affect the variance measurement and shouldn't trigger circularities in a way class methods/properties can. So it should be OK to defer them. They should behave in an almost same way as statements executed after the class. They only get a special treatment when it comes to class type parameters and
this
. But it feels like it shouldn't break anything if those get simply resolved in a deferred manner - after the class declaration gets processed. - Right now it looks like this PR doesn't do anything because a bug has been introduced in the meantime: Missing constraint error in instantiation expression for a nested class #61982
closes #52892
This only helps with a specific situation when a type alias defined in a class static block triggered the first variance check in those circular situations. Then all properties referencing
A
again were reenteringgetVariancesWorker
and thus it was assumed that they were OK. My comment at the bottom of this post explains more about this kind of situation: #59606 (comment)In other words, this only mitigates the problem in this specific situation but it's still possible to run into problems in similar circular situations.