-
Couldn't load subscription status.
- Fork 49.7k
[Compiler] Fix incorrect closure hoisting causing runtime errors (#34901 #34909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Compiler] Fix incorrect closure hoisting causing runtime errors (#34901 #34909
Conversation
…ebook#34901) The React Compiler incorrectly outlines closure-scoped functions to the top level when it detects they have no context variables from the immediate component scope. This breaks lexical scoping for nested components that capture variables from parent scopes. The issue occurs when: 1. A component contains a nested function component 2. That nested component has an arrow function 3. The arrow function captures a variable from the parent component The `outlineFunctions` optimization only checked `context.length === 0`, which verifies variables from the immediate component scope, but doesn't account for variables from outer parent scopes accessed via LoadContext instructions. Fix: Added `hasUnaccountedOuterScopeReferences()` helper that checks for LoadContext instructions in the function's HIR. If any exist, the function cannot be safely outlined as it depends on runtime context. Test: Added closure-hoisting-bug.js fixture that reproduces the issue and verifies the fix.
|
Hi @kuomars110! 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 cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts
Outdated
Show resolved
Hide resolved
compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! See comments, but overall this looks good
compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts
Outdated
Show resolved
Hide resolved
compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts
Outdated
Show resolved
Hide resolved
…tion naming [Version] Update React version to 19.3.0
|
Thanks for the review! I went through everything you mentioned please take another look when you can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked this locally and this is the wrong fix. We already check for context variables — and in general we try to add outlined functions as siblings of the original component in order to have access to the same variables. This just isn't working for factory functions where the inner component is defined with an arrow function.
In your example, change Component to a regular function expression (not an arrow function) and it will already work.
|
Thank you for clarifying! You're absolutely right. I misunderstood the root cause. Looking at the original issue #34901 again, the problem is specifically with arrow functions inside factory functions like: `export function createSomething() { const Cmp = () => { // Arrow function component {observedStore.test} ;}; return Cmp; The issue is that: Cmp (the arrow function) has [context.length === 0] Could you point me in the right direction for the correct approach? |
|
There are a few different directions to go, and the implications are subtle enough that I think this is something that we'll have to look into and fix ourselves. I appreciate the interest! |
Summary
This PR fixes a critical bug in the React Compiler where functions with closure-scoped variables were incorrectly being outlined (hoisted) to top-level, breaking lexical scope and causing runtime errors.
Problem: The compiler's outlineFunctions optimization was only checking if context.length === 0 before outlining a function. However, this check only validates the immediate component scope, not parent scopes. When a nested component creates a closure over its parent component's variables (e.g., () => store), those variables aren't tracked in the nested component's [context] array, but the function was still being incorrectly outlined.
Root Cause: In the reported issue #34901, the arrow function () => store captures [store] from the parent scope. When the compiler outlined this to a top-level _temp() function, [store] became undefined, causing a runtime error.
Solution: Added a [hasUnaccountedOuterScopeReferences()] helper function that recursively checks for LoadContext HIR instructions. These instructions indicate that a function is accessing variables from outer scopes that aren't tracked in the [context] array. Functions with such references are now prevented from being outlined.
Reference: Fixes #34901
How did you test this change?
Added a regression test case ([closure-hoisting-bug.js] that reproduces the exact scenario from issue #34901:
A parent function creates a local variable [store]
A nested component uses that variable in a closure (() => store)
Verified the compiled output keeps the arrow function inline instead of incorrectly outlining it
Ran the compiler build and tests:
cd compiler yarn build yarn test --testPathPattern="fixtures" --testNamePattern="closure-hoisting-bug"Verified the fix:
The test case now compiles correctly with () => store staying inline within the component
The compiled code properly accesses [store] from the closure
Eval output shows the expected result:
Code formatting and linting:
npx prettier --write OutlineFunctions.ts closure-hoisting-bug.jsAll files pass lint checks with no errors.
The fix ensures that the compiler's optimization respects lexical scope and doesn't break closure semantics, preventing the undefined variable runtime errors reported in #34901.