[compiler][bugfix] Avoid inserting duplicate context variable declarations #32745

mofeiZ · 2025-03-25T18:16:42Z

(note: also see alternative implementation in #32747)

PruneHoistedContexts currently strips hoisted declarations and rewrites the first StoreContext reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic DeclareContext let is inserted before instruction 1.

// source
const cb = () => x; // reference that causes x to be hoisted

let x = 4;
x = 5;

// React Compiler IR
[0] DeclareContext HoistedLet 'x'
...
[1] StoreContext reassign 'x' = 4
[2] StoreContext reassign 'x' = 5

Currently, we don't account for DeclareContext let. As a result, we're rewriting to insert duplicate declarations.
For the below example, we should only remove instruction 0 (no need to insert a DeclareContext let since one is already present).

// source
const cb = () => x; // reference that causes x to be hoisted

let x;
x = 5;

// React Compiler IR
[0] DeclareContext HoistedLet 'x'
...
[1] DeclareContext Let 'x'
[2] StoreContext reassign 'x' = 5

…ants This is an alternative to pruning + rewriting context variables (#32745)

…ants This is an alternative to pruning + rewriting context variables (#32745). Instead of always lowering context variables to a DeclareContext followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations

…tions (note: also see alternative implementation in #32747) `PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1. ```js // source const cb = () => x; // reference that causes x to be hoisted let x = 4; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] StoreContext reassign 'x' = 4 [2] StoreContext reassign 'x' = 5 ``` Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations. For the below example, we should only remove instruction 0 (no need to insert a DeclareContext `let` since one is already present). ```js // source const cb = () => x; // reference that causes x to be hoisted let x; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] DeclareContext Let 'x' [2] StoreContext reassign 'x' = 5 ```

…ants This is an alternative to pruning + rewriting context variables (#32745). Instead of always lowering context variables to a DeclareContext followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations

mofeiZ · 2025-03-27T01:24:43Z

abandoning in favor of #32747, which (1) has more explicit HIR representation and (2) lets us handle hoisted functions explicitly

…ants (TODO: handle edge cases with some context variables no longer having mutable ranges ```js function Component() { useEffect(() => { let hasCleanedUp = false; document.addEventListener(..., () => hasCleanedUp ? foo() : bar()); // effect return values shouldn't be typed as frozen return () => { hasCleanedUp = true; } }; } ``` ### Problem `PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1. ```js // source const cb = () => x; // reference that causes x to be hoisted let x = 4; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] StoreContext reassign 'x' = 4 [2] StoreContext reassign 'x' = 5 ``` Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations. ```js // source const cb = () => x; // reference that causes x to be hoisted let x; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] DeclareContext Let 'x' [2] StoreContext reassign 'x' = 5 ``` ### Solution Instead of always lowering context variables to a DeclareContext followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations (note: also see alternative implementation in #32745)

…ants, after fixes, tmp ```js function Component() { useEffect(() => { let hasCleanedUp = false; document.addEventListener(..., () => hasCleanedUp ? foo() : bar()); // effect return values shouldn't be typed as frozen return () => { hasCleanedUp = true; } }; } ``` ### Problem `PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1. ```js // source const cb = () => x; // reference that causes x to be hoisted let x = 4; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] StoreContext reassign 'x' = 4 [2] StoreContext reassign 'x' = 5 ``` Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations. ```js // source const cb = () => x; // reference that causes x to be hoisted let x; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] DeclareContext Let 'x' [2] StoreContext reassign 'x' = 5 ``` ### Solution Instead of always lowering context variables to a DeclareContext followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations ~(note: also see alternative implementation in #32745

…ants, after fixes, tmp ```js function Component() { useEffect(() => { let hasCleanedUp = false; document.addEventListener(..., () => hasCleanedUp ? foo() : bar()); // effect return values shouldn't be typed as frozen return () => { hasCleanedUp = true; } }; } ``` ### Problem `PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1. ```js // source const cb = () => x; // reference that causes x to be hoisted let x = 4; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] StoreContext reassign 'x' = 4 [2] StoreContext reassign 'x' = 5 ``` Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations. ```js // source const cb = () => x; // reference that causes x to be hoisted let x; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] DeclareContext Let 'x' [2] StoreContext reassign 'x' = 5 ``` ### Solution Instead of always lowering context variables to a DeclareContext followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations ~(note: also see alternative implementation in #32745 #### Testing Context variables are tricky. I synced and diffed changes in a large meta codebase and feel pretty confident about landing this. About 0.01% of compiled files changed. Among these changes, ~25% were [direct bugfixes](https://www.internalfb.com/phabricator/paste/view/P1800029094). The [other changes](https://www.internalfb.com/phabricator/paste/view/P1800028575) were primarily due to changed (corrected) mutable ranges from #33047. I tried to represent most interesting changes in new test fixtures

…ants ```js function Component() { useEffect(() => { let hasCleanedUp = false; document.addEventListener(..., () => hasCleanedUp ? foo() : bar()); // effect return values shouldn't be typed as frozen return () => { hasCleanedUp = true; } }; } ``` ### Problem `PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1. ```js // source const cb = () => x; // reference that causes x to be hoisted let x = 4; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] StoreContext reassign 'x' = 4 [2] StoreContext reassign 'x' = 5 ``` Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations. ```js // source const cb = () => x; // reference that causes x to be hoisted let x; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] DeclareContext Let 'x' [2] StoreContext reassign 'x' = 5 ``` ### Solution Instead of always lowering context variables to a DeclareContext follo 8000 wed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations ~(note: also see alternative implementation in #32745 ### Testing Context variables are tricky. I synced and diffed changes in a large meta codebase and feel pretty confident about landing this. About 0.01% of compiled files changed. Among these changes, ~25% were [direct bugfixes](https://www.internalfb.com/phabricator/paste/view/P1800029094). The [other changes](https://www.internalfb.com/phabricator/paste/view/P1800028575) were primarily due to changed (corrected) mutable ranges from #33047. I tried to represent most interesting changes in new test fixtures `

…ants ```js function Component() { useEffect(() => { let hasCleanedUp = false; document.addEventListener(..., () => hasCleanedUp ? foo() : bar()); // effect return values shouldn't be typed as frozen return () => { hasCleanedUp = true; } }; } ``` ### Problem `PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1. ```js // source const cb = () => x; // reference that causes x to be hoisted let x = 4; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] StoreContext reassign 'x' = 4 [2] StoreContext reassign 'x' = 5 ``` Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations. ```js // source const cb = () => x; // reference that causes x to be hoisted let x; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] DeclareContext Let 'x' [2] StoreContext reassign 'x' = 5 ``` ### Solution Instead of always lowering context variables to a DeclareContext followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations ~(note: also see alternative implementation in #32745 ### Testing Context variables are tricky. I synced and diffed changes in a large meta codebase and feel pretty confident about landing this. About 0.01% of compiled files changed. Among these changes, ~25% were [direct bugfixes](https://www.internalfb.com/phabricator/paste/view/P1800029094). The [other changes](https://www.internalfb.com/phabricator/paste/view/P1800028575) were primarily due to changed (corrected) mutable ranges from #33047. I tried to represent most interesting changes in new test fixtures `

…ants (#32747) ```js function Component() { useEffect(() => { let hasCleanedUp = false; document.addEventListener(..., () => hasCleanedUp ? foo() : bar()); // effect return values shouldn't be typed as frozen return () => { hasCleanedUp = true; } }; } ``` ### Problem `PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1. ```js // source const cb = () => x; // reference that causes x to be hoisted let x = 4; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] StoreContext reassign 'x' = 4 [2] StoreContext reassign 'x' = 5 ``` Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations. ```js // source const cb = () => x; // reference that causes x to be hoisted let x; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] DeclareContext Let 'x' [2] StoreContext reassign 'x' = 5 ``` ### Solution Instead of always lowering context variables to a DeclareContext followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations ~(note: also see alternative implementation in #32745 ### Testing Context variables are tricky. I synced and diffed changes in a large meta codebase and feel pretty confident about landing this. About 0.01% of compiled files changed. Among these changes, ~25% were [direct bugfixes](https://www.internalfb.com/phabricator/paste/view/P1800029094). The [other changes](https://www.internalfb.com/phabricator/paste/view/P1800028575) were primarily due to changed (corrected) mutable ranges from #33047. I tried to represent most interesting changes in new test fixtures `

…ants (#32747) ```js function Component() { useEffect(() => { let hasCleanedUp = false; document.addEventListener(..., () => hasCleanedUp ? foo() : bar()); // effect return values shouldn't be typed as frozen return () => { hasCleanedUp = true; } }; } ``` ### Problem `PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1. ```js // source const cb = () => x; // reference that causes x to be hoisted let x = 4; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] StoreContext reassign 'x' = 4 [2] StoreContext reassign 'x' = 5 ``` Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations. ```js // source const cb = () => x; // reference that causes x to be hoisted let x; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] DeclareContext Let 'x' [2] StoreContext reassign 'x' = 5 ``` ### Solution Instead of always lowering context variables to a DeclareContext followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations ~(note: also see alternative implementation in #32745 ### Testing Context variables are tricky. I synced and diffed changes in a large meta codebase and feel pretty confident about landing this. About 0.01% of compiled files changed. Among these changes, ~25% were [direct bugfixes](https://www.internalfb.com/phabricator/paste/view/P1800029094). The [other changes](https://www.internalfb.com/phabricator/paste/view/P1800028575) were 68AF primarily due to changed (corrected) mutable ranges from #33047. I tried to represent most interesting changes in new test fixtures ` DiffTrain build for [9d795d3](9d795d3)

facebook-github-bot added the CLA Signed label Mar 25, 2025

github-actions bot added the React Core Team Opened by a member of the React Core Team label Mar 25, 2025

vercel bot deployed to Preview March 25, 2025 18:18 View deployment

mofeiZ added a commit that referenced this pull request Mar 25, 2025

[compiler][bugfix] expand StoreContext to const / let / function vari…

6d5ddb3

…ants This is an alternative to pruning + rewriting context variables (#32745)

mofeiZ mentioned this pull request Mar 25, 2025

[compiler][bugfix] expand StoreContext to const / let / function variants #32747

Merged

mofeiZ marked this pull request as ready for review March 25, 2025 18:32

mofeiZ force-pushed the pr32745 branch from d794f2b to 78856ef Compare March 25, 2025 18:34

vercel bot deployed to Preview March 25, 2025 18:35 View deployment

mofeiZ closed this Mar 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[compiler][bugfix] Avoid inserting duplicate context variable declarations #32745

[compiler][bugfix] Avoid inserting duplicate context variable declarations #32745

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[compiler][bugfix] Avoid inserting duplicate context variable declarations #32745

[compiler][bugfix] Avoid inserting duplicate context variable declarations #32745

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!