Fix additional function mutable binding function scope #1291
Conversation
jeffreytan81
assigned simonhj, cblappert and hermanventer and unassigned simonhj, cblappert and hermanventer
|
@trueadm, only react tests failed for this. Is there any wiki talking about how to best debug single react test failure? I can try investigate the react test runner, but it would be time saving if there is a documentation for it. |
|
@yinghuitan Might be worth pining @cblappert as I helped him yesterday set up a debug environment for the React test cases. What I do, is setup a local file and run it via the CLI with the Node debugger enabled. You'll need to enable |
| @@ -729,11 +729,12 @@ export class ResidualHeapVisitor { | |||
| instance: funcInstance, | |||
| }); | |||
| this.visitGenerator(generator); | |||
| this._withScope(generator, visitPropertiesAndBindings); | |||
| // All modified properties and bindings should be accessible | |||
There was a problem hiding this comment.
The comment sounds reasonable. Should the visitGenerator call one line earlier also get the same treatment? If not, why not?
There was a problem hiding this comment.
Maybe not, "generator" above represents any time sensitive generator entries captured in the additional function so it should be visited from that generator(or its sub-generators).
While for modified properties and bindings that are not captured in generator, they should be in additional function's scope(the change here).
jeffreytan81
force-pushed
the
FixAdditionalFunctionScopeBug
branch
from
09ca2a7
to
9991988
Compare
There was a problem hiding this comment.
@yinghuitan is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Release Note: none
Fix #1238
The behavior of the bug is that, get_scope_binding() helper function is generated in wrong scope: global scope instead of additional function scope.
It happens because there are two scopes referencing it, additional function itself and its parent generator scope. The second one is problematic because "obj" is not referenced in parent generator scope. We should visitPropertiesAndBindings only from additional function scope itself instead of parent generator scope.
[The changes in react compiler]: with above change, all the hoistable react elements will be in additional function's parent generator scope so align the change in the react/hoisting.js file.