Repository navigation
Fix for Incorrect class 'this' type used during type checking of static methods - #2249
Open
of-moons-birds-and-monsters wants to merge 1 commit into
Conversation
…type was being passed during checking of properties and methods, leading to compiler errors when attempting to access static members from static methods via 'this', and not erroring when accessing instance members via 'this' in static context. The issue was caused by FunctionContext constructor receiving the wrong 'this' type when FlowChecker was visiting MethodDefintionNode and specializing generic methods with parsed types. The two methods this was happening in were FlowChecker::visit(ESTree::MethodDefinition \*node) and FlowChecker::specializeGenericMethodWithParsedTypes To fix this issue checks were added to see if we are in a static method, if we are then the constructor type is used as the 'this' type instead of the instance type. In visit(ESTree::MethodDefinition \*node) this means grabbing the constructor type from 'curclassContext\_->newTargetType' and in specializeGenericMethodWithParsedTypes the existing 'classType' was used since that is the constructor type. In both locations defensive future checks were added, that also see if the method is typed, and if it is then grab the 'this' from the method and pass that to the constructor instead. This was done in case future versions of shermes allow you to specify a different 'this' type for static methods, or allow for a different 'this' type in some other way. Tests were added to verify that the compiler will error when attempting to access instance members from a static method via 'this', and tests were added to verify that the compiler will now access the correct static member via 'this' in a static method, and that the compiler will produce an error when attempting to access an instance member via 'this ' in a static method. This does not correct the issue of 'this' being the class the static method was declared instead of the call receiver. So if a base class static method uses 'this' but you call it with the derived class as the receiver then the base class will be the 'this' instead of the expected derived class. I believe would be a much larger fix and would require more significant changes making fall outside this PR.
This was referenced Oct 16, 2026
of-moons-birds-and-monsters
deployed
to
npm-publish
October 17, 2026 00:14 — with
GitHub Actions
Active
This branch was successfully deployed
This file contains hidden or 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
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.
During the flow checking of static class members an incorrect 'this' type was being passed during checking of properties and methods, leading to compiler errors when attempting
to access static members from static methods via 'this', and not erroring when accessing instance members via 'this' in static context.
The compiler would successfully compile instance member access from static methods but would error when attempt to access static members from static methods when using
'this.memberName' to access them.
The issue was caused by FunctionContext constructor receiving the wrong 'this' type when FlowChecker was visiting MethodDefintionNode and specializing generic methods with parsed types.
The two methods this was happening in were FlowChecker::visit(ESTree::MethodDefinition *node) and FlowChecker::specializeGenericMethodWithParsedTypes
To fix this issue checks were added to see if we are in a static method, if we are then the constructor type is used as the 'this' type instead of the instance type.
In visit(ESTree::MethodDefinition *node) I checked 'node->_static' to see if it was a static method, I created a new 'Type *thisParamType' variable and it holds
'curClassContext_->classType' initially, which is the class instance type, and if this is a static method then I assign 'curClassContext_->newTargetType', which is the
static class type, to the 'thisParamType' variable.
Type *thisParamType = curClassContext_->classType; // if the method is static the 'this' type needs to be set to the // constructor type if (node->_static) { thisParamType = curClassContext_->newTargetType; // uses the function signature's 'this' type when present if (auto *typedFuncType = llvh::dyn_cast<TypedFunctionType>(funcType->info)) { if (auto *thisParam = typedFuncType->getThisParam()) { thisParamType = thisParam; } } }In specializeGenericMethodWithParsedTypes the existing 'classType' variable was used since
that is the constructor type. A check was added to see if the method is static and if it is then assign the 'classType' to a new 'Type *thisParamType' variable.
Type *thisParamType = classTypeForContext; // if the method is static we need 'this' type to be the constructor type // that contains static members if (method->_static) { thisParamType = classType; // uses the function signatures 'this' type when present if (auto *typedFuncType = llvh::dyn_cast<TypedFunctionType>(ftype->info)) { if (auto *thisParam = typedFuncType->getThisParam()) { thisParamType = thisParam; } } }I default to assigning the 'classTypeForContext' to it initially because that should be the instance class type.
In both locations defensive checks were added. These checks see if the method is typed, and if it is then grab the 'this' from the method and pass that to the constructor instead.
This was done in case future versions of shermes allow you to specify a different 'this' type for static methods, or allow for a different 'this' type in some other way.
Currently shermes does not allow you specify a different 'this' type for methods.
Strictly speaking neither of those extra checks are needed and the code would work just fine without them.
This PR does not touch the separate issue of 'this' only being resolved to the class where the method is defined for static non-generic methods.
This means calling a static base method from a derived class will still incorrectly use the 'this' of the base class , sine the method was defined there.
This PR also does not touch the separate issue of 'this' param not being factored into generic specialization type calculations correctly, leaving
you unable to call a base method from a derived class then call that same base method from the base class.
Tests were added to verify that the compiler will error when attempting to access instance members from a static method via 'this', and tests were added to verify that the
compiler will now access the correct static member via 'this' in a static method, and that the compiler will produce an error when attempting to access an instance member via 'this '
in a static method.
This PR does not touch the separate issue of 'this' not being resolved correctly for inherited static methods.
So if you have a setup like
It would incorrectly print "Moth Man" instead of "Creature from the Black Lagoon" when run, and this PR does not attempt to fix that issue.
Test Results
Results of "check-hermes" before changes
Testing Time: 82.03s
Failing Tests (2):
Hermes :: hermes/function-tostring-builtins-microtask.js
Hermes :: hermes/function-tostring-builtins.js
Expected Passes : 4158
Expected Failures : 6
Unsupported Tests : 196
Unexpected Failures: 2
Results of "check-hermes" after changes
Testing Time: 87.10s
Failing Tests (2):
Hermes :: hermes/function-tostring-builtins-microtask.js
Hermes :: hermes/function-tostring-builtins.js
Expected Passes : 4160
Expected Failures : 6
Unsupported Tests : 196
Unexpected Failures: 2