facebook
Fix function hoisting behaviour (non-strict mode)
Summary
Addresses #1455
Hermes currently has divergent function hoisting behavior compared to QuickJS, V8, and JSC in non-strict mode. The following code prints FAIL instead of SUCCESS
{
function promotedFunction(endRecursion) {
if(endRecursion) {
promotedFunction(true);
return;
}
print ('SUCCESS');
}
callback = promotedFunction;
}
{
function promotedFunction(endRecursion) {
if(endRecursion) {
promotedFunction(true);
return;
}
print ('FAIL');
}
}
callback(false);
This happens as scoped function promoted to global scope are treated as global functions, like for var.
To fix this behaviour:
- Two declarations are created for a promoted identifier instead of one: a global and a scoped declaration, stored in
sideIdentifierDeclarationDecl_andpromotedFunctionDecls_ - If the identifier is promoted, we call
emitStoreon both pointers associated to the declarations
Test Plan
- Added a unit test with the failing code
Hi @trossimel-sc!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
@trossimel-sc The update-lit build target will update the tests that are autogenerated, then check-hermes to run them. LIT_FILTER will let you narrow down what exactly gets updated or run as well. Thanks for submitting this fix!
The CLA is signed and we can proceed with the review process 😄 .
We can start with this PR and for await of PR.
I also noticed the build is failing on static_h. Is this happening to me only?
I also noticed the build is failing on static_h.
That should be fixed with ae186905e9b2d6c30e7015d3b418fba7aea51d11
@avp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@avp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@trossimel-sc The internal failure is due to a bad interaction with lazy compilation. Test case:
(function() {
{ function* x() {} }
})();
(function() {
function* y() { yield 1; }
})();
Run that with
bin/hermes ~/test.js --lazy
and there'll be an assertion failure:
Assertion failed: (llvh::isa<CreateScopeInst>(RSI) && "variable must be initialized in its own function"), function emitStore, file ESTreeIRGen.cpp, line 1208.
fish: Job 1, 'bin/hermes ~/test.js --lazy' terminated by signal SIGABRT (Abort)
Some documentation about how lazy compilation works: https://github.com/facebook/hermes/blob/static_h/doc/LazyEvalCompilation.md
I haven't gone through and debugged but there's potentially some extra work that has to be done in the lazy compilation path due to the new information you've added to SemContext.
Seems like the address of identifier x and y are the same at some point, so the code is trying to promote y when it shouldn't, causing the problem to arise.
From my understanding, this probably happens in lazy mode because memory is being re-used during execution for AST nodes.
Edit: addressed by clearing all promoted decls inside runLazy.
@avp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@trossimel-sc We have one more issue with lazy compilation.
Test case:
function foo() {
var result;
{
let x = 0;
result = ret;
function ret() {
return x;
}
}
return result;
}
print(typeof foo());
This should print function but it prints undefined in lazy mode.
@avp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.