From 47db947a261f1853d2aa138b46a90cb4acee123d Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 24 Mar 2025 13:53:35 -0400 Subject: [PATCH] [compiler][bugfix] Fix hoisting of let declarations (Found when compiling Meta React code) Let variable declarations and reassignments are currently rewritten to `StoreLocal ` instructions, which each translates to a new `const varName` declaration in codegen. ```js // Example input function useHook() { const getX = () => x; let x = CONSTANT1; if (cond) { x += CONSTANT2; } return } // Compiled output, prior to this PR import { c as _c } from "react/compiler-runtime"; function useHook() { const $ = _c(1); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { const getX = () => x; let x = CONSTANT1; if (cond) { let x = x + CONSTANT2; x; } t0 = ; $[0] = t0; } else { t0 = $[0]; } return t0; } ``` This also manifests as a babel internal error when replacing the original function declaration with the compiler output. The below compilation output fails with `Duplicate declaration "x" (This is an error on an internal node. Probably an internal error.)`. ```js // example input let x = CONSTANT1; if (cond) { x += CONSTANT2; x = CONSTANT3; } // current output let x = CONSTANT1; if (playheadDragState) { let x = x + CONSTANT2 x; let x = CONSTANT3; } ``` --- .../ReactiveScopes/PruneHoistedContexts.ts | 101 ++++++++++++++---- .../hoisting-invalid-tdz-let.expect.md | 59 ++++++++++ .../compiler/hoisting-invalid-tdz-let.js | 14 +++ ...oisting-nested-let-declaration-2.expect.md | 3 +- .../hoisting-nested-let-declaration.expect.md | 6 +- ...sting-reassigned-let-declaration.expect.md | 67 ++++++++++++ .../hoisting-reassigned-let-declaration.js | 18 ++++ ...reassigned-twice-let-declaration.expect.md | 69 ++++++++++++ ...isting-reassigned-twice-let-declaration.js | 19 ++++ .../hoisting-simple-let-declaration.expect.md | 6 +- .../mutate-captured-arg-separately.expect.md | 3 +- .../recursive-function-expression.expect.md | 29 +++++ .../compiler/recursive-function-expression.js | 11 ++ 13 files changed, 378 insertions(+), 27 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-invalid-tdz-let.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-invalid-tdz-let.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts index 07b099c2ea5fe..b3754721cab37 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +import {CompilerError} from '..'; import { DeclarationId, InstructionKind, @@ -27,7 +28,17 @@ export function pruneHoistedContexts(fn: ReactiveFunction): void { visitReactiveFunction(fn, new Visitor(), hoistedIdentifiers); } -type HoistedIdentifiers = Map; +const REWRITTEN_HOISTED_CONST: unique symbol = Symbol( + 'REWRITTEN_HOISTED_CONST', +); +const REWRITTEN_HOISTED_LET: unique symbol = Symbol('REWRITTEN_HOISTED_LET'); + +type HoistedIdentifiers = Map< + DeclarationId, + | InstructionKind + | typeof REWRITTEN_HOISTED_CONST + | typeof REWRITTEN_HOISTED_LET +>; class Visitor extends ReactiveFunctionTransform { override transformInstruction( @@ -35,6 +46,10 @@ class Visitor extends ReactiveFunctionTransform { state: HoistedIdentifiers, ): Transformed { this.visitInstruction(instruction, state); + + /** + * Remove hoisted declarations to preserve TDZ + */ if ( instruction.value.kind === 'DeclareContext' && instruction.value.lvalue.kind === 'HoistedConst' @@ -68,31 +83,75 @@ class Visitor extends ReactiveFunctionTransform { return {kind: 'remove'}; } - if ( - instruction.value.kind === 'StoreContext' && - state.has(instruction.value.lvalue.place.identifier.declarationId) - ) { + if (instruction.value.kind === 'StoreContext') { const kind = state.get( instruction.value.lvalue.place.identifier.declarationId, - )!; - return { - kind: 'replace', - value: { - kind: 'instruction', - instruction: { - ...instruction, + ); + if (kind != null) { + CompilerError.invariant(kind !== REWRITTEN_HOISTED_CONST, { + reason: 'Expected exactly one store to a hoisted const variable', + loc: instruction.loc, + }); + if ( + kind === InstructionKind.Const || + kind === InstructionKind.Function + ) { + state.set( + instruction.value.lvalue.place.identifier.declarationId, + REWRITTEN_HOISTED_CONST, + ); + return { + kind: 'replace', value: { - ...instruction.value, - lvalue: { - ...instruction.value.lvalue, - kind, + kind: 'instruction', + instruction: { + ...instruction, + value: { + ...instruction.value, + lvalue: { + ...instruction.value.lvalue, + kind, + }, + type: null, + kind: 'StoreLocal', + }, }, - type: null, - kind: 'StoreLocal', }, - }, - }, - }; + }; + } else if (kind !== REWRITTEN_HOISTED_LET) { + /** + * Context variables declared with let may have reassignments. Only + * insert a `DeclareContext` for the first encountered `StoreContext` + * instruction. + */ + state.set( + instruction.value.lvalue.place.identifier.declarationId, + REWRITTEN_HOISTED_LET, + ); + return { + kind: 'replace-many', + value: [ + { + kind: 'instruction', + instruction: { + id: instruction.id, + lvalue: null, + value: { + kind: 'DeclareContext', + lvalue: { + kind: InstructionKind.Let, + place: {...instruction.value.lvalue.place}, + }, + loc: instruction.value.loc, + }, + loc: instruction.loc, + }, + }, + {kind: 'instruction', instruction}, + ], + }; + } + } } return {kind: 'keep'}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-invalid-tdz-let.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-invalid-tdz-let.expect.md new file mode 100644 index 0000000000000..2c2b559f02622 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-invalid-tdz-let.expect.md @@ -0,0 +1,59 @@ + +## Input + +```javascript +function Foo() { + const getX = () => x; + console.log(getX()); + + let x = 4; + x += 5; + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Foo() { + const $ = _c(2); + let getX; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + getX = () => x; + console.log(getX()); + + let x; + x = 4; + x = x + 5; + $[0] = getX; + } else { + getX = $[0]; + } + x; + let t0; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t0 = ; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; + +``` + +### Eval output +(kind: exception) Cannot access 'x' before initialization \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-invalid-tdz-let.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-invalid-tdz-let.js new file mode 100644 index 0000000000000..4097ee37970cc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-invalid-tdz-let.js @@ -0,0 +1,14 @@ +function Foo() { + const getX = () => x; + console.log(getX()); + + let x = 4; + x += 5; + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md index fe0c6618f51ad..aa6ee80d69bf1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md @@ -36,7 +36,8 @@ function hoisting(cond) { items.push(bar()); }; - let bar = _temp; + let bar; + bar = _temp; foo(); } $[0] = cond; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md index faf7a064d748f..25205ca02372c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md @@ -41,9 +41,11 @@ function hoisting() { return result; }; - let foo = () => bar + baz; + let foo; + foo = () => bar + baz; - let bar = 3; + let bar; + bar = 3; const baz = 2; t0 = qux(); $[0] = t0; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.expect.md new file mode 100644 index 0000000000000..3f7b16cf23697 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.expect.md @@ -0,0 +1,67 @@ + +## Input + +```javascript +import {CONST_NUMBER0, CONST_NUMBER1, Stringify} from 'shared-runtime'; + +function useHook({cond}) { + 'use memo'; + const getX = () => x; + + let x = CONST_NUMBER0; + if (cond) { + x += CONST_NUMBER1; + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useHook, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: true}, {cond: false}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { CONST_NUMBER0, CONST_NUMBER1, Stringify } from "shared-runtime"; + +function useHook(t0) { + "use memo"; + const $ = _c(2); + const { cond } = t0; + let t1; + if ($[0] !== cond) { + const getX = () => x; + + let x; + x = CONST_NUMBER0; + if (cond) { + x = x + CONST_NUMBER1; + x; + } + + t1 = ; + $[0] = cond; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useHook, + params: [{ cond: true }], + sequentialRenders: [{ cond: true }, { cond: true }, { cond: false }], +}; + +``` + +### Eval output +(kind: ok)
{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}
+
{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}
+
{"getX":{"kind":"Function","result":0},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.js new file mode 100644 index 0000000000000..e8d55039befbd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.js @@ -0,0 +1,18 @@ +import {CONST_NUMBER0, CONST_NUMBER1, Stringify} from 'shared-runtime'; + +function useHook({cond}) { + 'use memo'; + const getX = () => x; + + let x = CONST_NUMBER0; + if (cond) { + x += CONST_NUMBER1; + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useHook, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: true}, {cond: false}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.expect.md new file mode 100644 index 0000000000000..54d49b9282cdf --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.expect.md @@ -0,0 +1,69 @@ + +## Input + +```javascript +import {CONST_NUMBER0, CONST_NUMBER1, Stringify} from 'shared-runtime'; + +function useHook({cond}) { + 'use memo'; + const getX = () => x; + + let x = CONST_NUMBER0; + if (cond) { + x += CONST_NUMBER1; + x = Math.min(x, 100); + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useHook, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: true}, {cond: false}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { CONST_NUMBER0, CONST_NUMBER1, Stringify } from "shared-runtime"; + +function useHook(t0) { + "use memo"; + const $ = _c(2); + const { cond } = t0; + let t1; + if ($[0] !== cond) { + const getX = () => x; + + let x; + x = CONST_NUMBER0; + if (cond) { + x = x + CONST_NUMBER1; + x; + x = Math.min(x, 100); + } + + t1 = ; + $[0] = cond; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useHook, + params: [{ cond: true }], + sequentialRenders: [{ cond: true }, { cond: true }, { cond: false }], +}; + +``` + +### Eval output +(kind: ok)
{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}
+
{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}
+
{"getX":{"kind":"Function","result":0},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.js new file mode 100644 index 0000000000000..2c2b8187effaa --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.js @@ -0,0 +1,19 @@ +import {CONST_NUMBER0, CONST_NUMBER1, Stringify} from 'shared-runtime'; + +function useHook({cond}) { + 'use memo'; + const getX = () => x; + + let x = CONST_NUMBER0; + if (cond) { + x += CONST_NUMBER1; + x = Math.min(x, 100); + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useHook, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: true}, {cond: false}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.expect.md index 8d694a984aed5..958f2c7afdf8f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.expect.md @@ -29,8 +29,10 @@ function hoisting() { if ($[0] === Symbol.for("react.memo_cache_sentinel")) { foo = () => bar + baz; - let bar = 3; - let baz = 2; + let bar; + bar = 3; + let baz; + baz = 2; $[0] = foo; } else { foo = $[0]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.expect.md index f3f21f8f6222b..990b1bf14713b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.expect.md @@ -33,7 +33,8 @@ function component(a) { m(x); }; - let x = { a }; + let x; + x = { a }; m(x); $[0] = a; $[1] = y; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.expect.md index bc46c1b85578b..1e7295da57f45 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.expect.md @@ -2,6 +2,17 @@ ## Input ```javascript +function Component1() { + const x = callback(10); + function callback(x) { + if (x == 0) { + return null; + } + return callback(x - 1); + } + return x; +} + function Component() { function callback(x) { if (x == 0) { @@ -23,6 +34,24 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { c as _c } from "react/compiler-runtime"; +function Component1() { + const $ = _c(1); + let x; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + x = callback(10); + function callback(x_0) { + if (x_0 == 0) { + return null; + } + return callback(x_0 - 1); + } + $[0] = x; + } else { + x = $[0]; + } + return x; +} + function Component() { const $ = _c(1); let t0; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.js index 5d50de75bae50..b09769da2f22b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.js @@ -1,3 +1,14 @@ +function Component1() { + const x = callback(10); + function callback(x) { + if (x == 0) { + return null; + } + return callback(x - 1); + } + return x; +} + function Component() { function callback(x) { if (x == 0) {