facebook
Add with statement support
Summary
This PR introduces support for the with statement via IR
- Identifiers within a with statement are transformed into a series of conditional chain. Each condition checks the presence of a key in the object properties, and accesses it if present. This is done through
createConditionalChainImpl - We need to keep track of each with statement depth. Hermes uses a task queue to lazily compile functions, and for this reason, we also need to copy the with depths when adding the declaration to the queue.
The changes are designed to minimize disruption. The logic only activates when a with statement is present, and the bulk of the implementation is located in a separate file, ESTreeIRGen-with.
Test Plan
- Added a unit test covering most common scenarios
- Test262 result on with statements tests: 92.98%.
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!
While the output is correct, I've noticed that when running in debug mode, it occasionally fails with the following message:
Assertion failed: (I->hasOutput() && "Instruction has no output"), function encodeValue, file ISel.cpp, line 264.
And in this case the IR output seems to be incorrectly generated. This issue arises specifically when a store operation is present within the with statement. Here's an example:
let e = 20;
with (obj) {
e = 30;
}
Is there a straightforward way to handle this case? I'm not very familiar with Hermes IR, so I'm unsure why this failure occurs, while the output is still correct. Thank you for your help!
Please feel free to respond once my CLA is signed, if needed. —I hope this process won't take much longer. 🫤
@trossimel-sc I haven't reviewed the PR, but I compiled it and looked at the IR output of your example:
%10 = CallInst (:any) %8: any, empty: any, false: boolean, empty: any, undefined: undefined, undefined: undefined, %9: any, "e": string
%11 = CondBranchInst %10: any, %BB2, %BB1
%BB1:
%12 = StoreFrameInst %0: environment, 30: number, [%VS0.e]: any
%13 = BranchInst %BB3
%BB2:
%14 = LoadFrameInst (:any) %0: environment, [%VS0.with1]: any
%15 = StorePropertyLooseInst 30: number, %14: any, "e": string
%16 = BranchInst %BB3
%BB3:
%17 = PhiInst (:notype) %15: notype, %BB2, %12: notype, %BB1
You are calling a helper method to check if e exists in the object and branching to either a property store %15 or a variable store %12. The problem is that you are treating these stores as if they have a result and in instruction %17 you are reading this result. Stores have no result, they just have side effects.
AFAICT, instruction %17 is incorrect and more importantly, unnecessary. You don't need to combine the stores with Phi, they have already done their side effect.
BTW, I think that we probably want a new bytecode instruction for this instead of calling a helper, but we can do that incrementally. Also, the helper should just be a "builtin", which is safer and easier to call. But these are details that we can address when we start the review.
(We don't want to start reviewing before we know that we can accept the PR, I hope you understand)
Thanks for the response and the assistance @tmikov Just wanted to update, I'm still waiting for my request to be approved by the company. Unfortunately, it's taking longer than expected, and there's not much I can do except wait for now 🫤
great work 🎉 adding Fixes: https://github.com/facebook/hermes/issues/1056 to description should link the issue to this PR (auto-close on merge)
