Fix function hoisting behaviour (non-strict mode) #1547
Conversation
Hermes currently has divergent function hoisting behavior compared to QuickJS, V8, and JSC in non-strict mode. The following code prints Error1; Error1 instead of Error; Error1
```
{
function test1(a) {
if(a == "a") {
test1("b");
return;
}
print ('Error');
}
callback1 = test1;
}
{
function test1(a) {
if(a == "a") {
test1("b");
return;
}
print ('Error1');
}
callback2 = test1;
}
callback1("a");
callback2("a");
```
This happens as scoped function promoted to global scope are treated as global function, like for `var`.
New approach:
- Creating two declaration associated with each promoted function identifier and storing the function code into two pointers, one global and one scoped. Each declaration has one pointer associated.
- Each identifier which is not a declaration is resolved into either the global or scoped declaration of the promoted function
Test 262 results:
- 100% on test/statements, 98.53% overall. **Nothing changed in the results** before and after the change.
---
|
Hi @trossimel-sc! Thank you for your pull request and welcome to our community. Action RequiredIn 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. ProcessIn 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 If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Seems like the failing errors are due to auto generated checks |
|
@trossimel-sc The |
|
Hi, we really need the CLA before we can review it internally. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
The CLA is signed and we can proceed with the review process 😄 . |
| if (semCtx_.getDeclarationDecl(ident) && | ||
| functionContext()->promotedFuncDecls.count(ident->_name)) { | ||
| decl = semCtx_.newDeclInScope(ident->_name, kind, curScope_); | ||
| bindingTable_.put(ident->_name, Binding{decl, ident}); |
There was a problem hiding this comment.
This needs a comment explaining what the special case is doing.
e.g. Why is it using put to override instead of try_emplace as below?
| /// Resolve the identifier node associated with the promoted function | ||
| /// to the corresponding global variable in expression context. | ||
| Value *resolveIdentifierPromoted(ESTree::IdentifierNode *id) { | ||
| auto declPromoted = getIDDeclPromoted(id); |
There was a problem hiding this comment.
| auto declPromoted = getIDDeclPromoted(id); | |
| auto *declPromoted = getIDDeclPromoted(id); |
That should be fixed with ae18690 |
Summary
Addresses #1455
Hermes currently has divergent function hoisting behavior compared to QuickJS, V8, and JSC in non-strict mode. The following code prints
FAILinstead ofSUCCESSThis happens as scoped function promoted to global scope are treated as global functions, like for
var.To fix this behaviour:
sideIdentifierDeclarationDecl_andpromotedFunctionDecls_emitStoreon both pointers associated to the declarationsTest Plan