From 2a18d35301ed21419b02a7dc04210e0a5f1c6530 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 16 Oct 2025 13:08:23 -0700 Subject: [PATCH 1/2] [compiler] Cleanup and enable validateNoVoidUseMemo This is a great validation, so let's enable by default. Changes: * Move the validation logic into ValidateUseMemo alongside the new check that the useMemo result is used * Update the lint description * Make the void memo errors lint-only, they don't require us to skip compilation (as evidenced by the fact that we've had this validation off) --- .../src/CompilerError.ts | 2 +- .../src/HIR/Environment.ts | 2 +- .../src/Inference/DropManualMemoization.ts | 48 -------------- .../src/Validation/ValidateUseMemo.ts | 37 ++++++++++- .../error.invalid-unused-usememo.expect.md | 35 ---------- .../error.useMemo-no-return-value.expect.md | 64 ------------------- .../compiler/invalid-unused-usememo.expect.md | 41 ++++++++++++ ...d-usememo.js => invalid-unused-usememo.js} | 2 +- .../invalid-useMemo-no-return-value.expect.md | 59 +++++++++++++++++ ....js => invalid-useMemo-no-return-value.js} | 2 +- .../invalid-useMemo-return-empty.expect.md | 33 ++++++++++ ...pty.js => invalid-useMemo-return-empty.js} | 1 + .../fixtures/compiler/repro.expect.md | 3 +- .../src/__tests__/fixtures/compiler/repro.js | 1 + .../compiler/useMemo-nested-ifs.expect.md | 11 +++- .../fixtures/compiler/useMemo-nested-ifs.js | 1 + .../compiler/useMemo-return-empty.expect.md | 22 ------- .../__tests__/PluginTest-test.ts | 10 ++- 18 files changed, 195 insertions(+), 179 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-unused-usememo.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.invalid-unused-usememo.js => invalid-unused-usememo.js} (67%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-no-return-value.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.useMemo-no-return-value.js => invalid-useMemo-no-return-value.js} (85%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-return-empty.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{useMemo-return-empty.js => invalid-useMemo-return-empty.js} (82%) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-return-empty.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts index 1c077edd8d8a5..3c131ea653ef3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts @@ -988,7 +988,7 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule { severity: ErrorSeverity.Error, name: 'void-use-memo', description: - 'Validates that useMemos always return a value. See [`useMemo()` docs](https://react.dev/reference/react/useMemo) for more information.', + 'Validates that useMemos always return a value and that the result of the useMemo is used by the component/hook. See [`useMemo()` docs](https://react.dev/reference/react/useMemo) for more information.', preset: LintRulePreset.RecommendedLatest, }; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index a31ef1c336ed6..4ec4a4a795f1b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -659,7 +659,7 @@ export const EnvironmentConfigSchema = z.object({ * Invalid: * useMemo(() => { ... }, [...]); */ - validateNoVoidUseMemo: z.boolean().default(false), + validateNoVoidUseMemo: z.boolean().default(true), /** * Validates that Components/Hooks are always defined at module level. This prevents scope diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts index d31cd1446b99b..0876424e28c14 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts @@ -438,40 +438,6 @@ export function dropManualMemoization( continue; } - /** - * Bailout on void return useMemos. This is an anti-pattern where code might be using - * useMemo like useEffect: running arbirtary side-effects synced to changes in specific - * values. - */ - if ( - func.env.config.validateNoVoidUseMemo && - manualMemo.kind === 'useMemo' - ) { - const funcToCheck = sidemap.functions.get( - fnPlace.identifier.id, - )?.value; - if (funcToCheck !== undefined && funcToCheck.loweredFunc.func) { - if (!hasNonVoidReturn(funcToCheck.loweredFunc.func)) { - errors.pushDiagnostic( - CompilerDiagnostic.create({ - category: ErrorCategory.VoidUseMemo, - reason: 'useMemo() callbacks must return a value', - description: `This ${ - manualMemo.loadInstr.value.kind === 'PropertyLoad' - ? 'React.useMemo()' - : 'useMemo()' - } callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects`, - suggestions: null, - }).withDetails({ - kind: 'error', - loc: instr.value.loc, - message: 'useMemo() callbacks must return a value', - }), - ); - } - } - } - instr.value = getManualMemoizationReplacement( fnPlace, instr.value.loc, @@ -629,17 +595,3 @@ function findOptionalPlaces(fn: HIRFunction): Set { } return optionals; } - -function hasNonVoidReturn(func: HIRFunction): boolean { - for (const [, block] of func.body.blocks) { - if (block.terminal.kind === 'return') { - if ( - block.terminal.returnVariant === 'Explicit' || - block.terminal.returnVariant === 'Implicit' - ) { - return true; - } - } - } - return false; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateUseMemo.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateUseMemo.ts index 0fdfeaab3ea5a..05a4b4b91f557 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateUseMemo.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateUseMemo.ts @@ -24,6 +24,7 @@ import {Result} from '../Utils/Result'; export function validateUseMemo(fn: HIRFunction): Result { const errors = new CompilerError(); + const voidMemoErrors = new CompilerError(); const useMemos = new Set(); const react = new Set(); const functions = new Map(); @@ -125,7 +126,22 @@ export function validateUseMemo(fn: HIRFunction): Result { validateNoContextVariableAssignment(body.loweredFunc.func, errors); if (fn.env.config.validateNoVoidUseMemo) { - unusedUseMemos.set(lvalue.identifier.id, callee.loc); + if (!hasNonVoidReturn(body.loweredFunc.func)) { + voidMemoErrors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.VoidUseMemo, + reason: 'useMemo() callbacks must return a value', + description: `This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects`, + suggestions: null, + }).withDetails({ + kind: 'error', + loc: body.loc, + message: 'useMemo() callbacks must return a value', + }), + ); + } else { + unusedUseMemos.set(lvalue.identifier.id, callee.loc); + } } break; } @@ -146,10 +162,10 @@ export function validateUseMemo(fn: HIRFunction): Result { * Even a DCE-based version could be bypassed with `noop(useMemo(...))`. */ for (const loc of unusedUseMemos.values()) { - errors.pushDiagnostic( + voidMemoErrors.pushDiagnostic( CompilerDiagnostic.create({ category: ErrorCategory.VoidUseMemo, - reason: 'Unused useMemo()', + reason: 'useMemo() result is unused', description: `This useMemo() value is unused. useMemo() is for computing and caching values, not for arbitrary side effects`, suggestions: null, }).withDetails({ @@ -160,6 +176,7 @@ export function validateUseMemo(fn: HIRFunction): Result { ); } } + fn.env.logErrors(voidMemoErrors.asResult()); return errors.asResult(); } @@ -192,3 +209,17 @@ function validateNoContextVariableAssignment( } } } + +function hasNonVoidReturn(func: HIRFunction): boolean { + for (const [, block] of func.body.blocks) { + if (block.terminal.kind === 'return') { + if ( + block.terminal.returnVariant === 'Explicit' || + block.terminal.returnVariant === 'Implicit' + ) { + return true; + } + } + } + return false; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.expect.md deleted file mode 100644 index 3c84a0dda335f..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.expect.md +++ /dev/null @@ -1,35 +0,0 @@ - -## Input - -```javascript -// @validateNoVoidUseMemo -function Component() { - useMemo(() => { - return []; - }, []); - return
; -} - -``` - - -## Error - -``` -Found 1 error: - -Error: Unused useMemo() - -This useMemo() value is unused. useMemo() is for computing and caching values, not for arbitrary side effects. - -error.invalid-unused-usememo.ts:3:2 - 1 | // @validateNoVoidUseMemo - 2 | function Component() { -> 3 | useMemo(() => { - | ^^^^^^^ useMemo() result is unused - 4 | return []; - 5 | }, []); - 6 | return
; -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.expect.md deleted file mode 100644 index 9b62c5364b6fb..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.expect.md +++ /dev/null @@ -1,64 +0,0 @@ - -## Input - -```javascript -// @validateNoVoidUseMemo -function Component() { - const value = useMemo(() => { - console.log('computing'); - }, []); - const value2 = React.useMemo(() => { - console.log('computing'); - }, []); - return ( -
- {value} - {value2} -
- ); -} - -``` - - -## Error - -``` -Found 2 errors: - -Error: useMemo() callbacks must return a value - -This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects. - -error.useMemo-no-return-value.ts:3:16 - 1 | // @validateNoVoidUseMemo - 2 | function Component() { -> 3 | const value = useMemo(() => { - | ^^^^^^^^^^^^^^^ -> 4 | console.log('computing'); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 5 | }, []); - | ^^^^^^^^^ useMemo() callbacks must return a value - 6 | const value2 = React.useMemo(() => { - 7 | console.log('computing'); - 8 | }, []); - -Error: useMemo() callbacks must return a value - -This React.useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects. - -error.useMemo-no-return-value.ts:6:17 - 4 | console.log('computing'); - 5 | }, []); -> 6 | const value2 = React.useMemo(() => { - | ^^^^^^^^^^^^^^^^^^^^^ -> 7 | console.log('computing'); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 8 | }, []); - | ^^^^^^^^^ useMemo() callbacks must return a value - 9 | return ( - 10 |
- 11 | {value} -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-unused-usememo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-unused-usememo.expect.md new file mode 100644 index 0000000000000..a2aba4c7b9160 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-unused-usememo.expect.md @@ -0,0 +1,41 @@ + +## Input + +```javascript +// @validateNoVoidUseMemo @loggerTestOnly +function Component() { + useMemo(() => { + return []; + }, []); + return
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo @loggerTestOnly +function Component() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 =
; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +``` + +## Logs + +``` +{"kind":"CompileError","detail":{"options":{"category":"VoidUseMemo","reason":"useMemo() result is unused","description":"This useMemo() value is unused. useMemo() is for computing and caching values, not for arbitrary side effects","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":3,"column":2,"index":67},"end":{"line":3,"column":9,"index":74},"filename":"invalid-unused-usememo.ts","identifierName":"useMemo"},"message":"useMemo() result is unused"}]}},"fnLoc":null} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":2,"column":0,"index":42},"end":{"line":7,"column":1,"index":127},"filename":"invalid-unused-usememo.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-unused-usememo.js similarity index 67% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-unused-usememo.js index 1983cd061a60e..e254424949bda 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-unused-usememo.js @@ -1,4 +1,4 @@ -// @validateNoVoidUseMemo +// @validateNoVoidUseMemo @loggerTestOnly function Component() { useMemo(() => { return []; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-no-return-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-no-return-value.expect.md new file mode 100644 index 0000000000000..24e62dad2be78 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-no-return-value.expect.md @@ -0,0 +1,59 @@ + +## Input + +```javascript +// @validateNoVoidUseMemo @loggerTestOnly +function Component() { + const value = useMemo(() => { + console.log('computing'); + }, []); + const value2 = React.useMemo(() => { + console.log('computing'); + }, []); + return ( +
+ {value} + {value2} +
+ ); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo @loggerTestOnly +function Component() { + const $ = _c(1); + + console.log("computing"); + + console.log("computing"); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = ( +
+ {undefined} + {undefined} +
+ ); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +``` + +## Logs + +``` +{"kind":"CompileError","detail":{"options":{"category":"VoidUseMemo","reason":"useMemo() callbacks must return a value","description":"This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":3,"column":24,"index":89},"end":{"line":5,"column":3,"index":130},"filename":"invalid-useMemo-no-return-value.ts"},"message":"useMemo() callbacks must return a value"}]}},"fnLoc":null} +{"kind":"CompileError","detail":{"options":{"category":"VoidUseMemo","reason":"useMemo() callbacks must return a value","description":"This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":6,"column":31,"index":168},"end":{"line":8,"column":3,"index":209},"filename":"invalid-useMemo-no-return-value.ts"},"message":"useMemo() callbacks must return a value"}]}},"fnLoc":null} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":2,"column":0,"index":42},"end":{"line":15,"column":1,"index":283},"filename":"invalid-useMemo-no-return-value.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-no-return-value.js similarity index 85% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-no-return-value.js index 0ce35e12f4ff8..781560fefa920 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-no-return-value.js @@ -1,4 +1,4 @@ -// @validateNoVoidUseMemo +// @validateNoVoidUseMemo @loggerTestOnly function Component() { const value = useMemo(() => { console.log('computing'); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-return-empty.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-return-empty.expect.md new file mode 100644 index 0000000000000..c6737d78a1b65 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-return-empty.expect.md @@ -0,0 +1,33 @@ + +## Input + +```javascript +// @loggerTestOnly +function component(a) { + let x = useMemo(() => { + mutate(a); + }, []); + return x; +} + +``` + +## Code + +```javascript +// @loggerTestOnly +function component(a) { + mutate(a); +} + +``` + +## Logs + +``` +{"kind":"CompileError","detail":{"options":{"category":"VoidUseMemo","reason":"useMemo() callbacks must return a value","description":"This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":3,"column":18,"index":61},"end":{"line":5,"column":3,"index":87},"filename":"invalid-useMemo-return-empty.ts"},"message":"useMemo() callbacks must return a value"}]}},"fnLoc":null} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":2,"column":0,"index":19},"end":{"line":7,"column":1,"index":107},"filename":"invalid-useMemo-return-empty.ts"},"fnName":"component","memoSlots":0,"memoBlocks":0,"memoValues":0,"prunedMemoBlocks":1,"prunedMemoValues":0} +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-return-empty.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-return-empty.js similarity index 82% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-return-empty.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-return-empty.js index 0d9b54d989bdd..d70bd138113a8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-return-empty.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-useMemo-return-empty.js @@ -1,3 +1,4 @@ +// @loggerTestOnly function component(a) { let x = useMemo(() => { mutate(a); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro.expect.md index d3fe3045c020c..9ca367e8a8ea0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @validateNoVoidUseMemo:false function Component(props) { const item = props.item; const thumbnails = []; @@ -22,7 +23,7 @@ function Component(props) { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo:false function Component(props) { const $ = _c(6); const item = props.item; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro.js index 9f5d785266874..de3026e4e587b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro.js @@ -1,3 +1,4 @@ +// @validateNoVoidUseMemo:false function Component(props) { const item = props.item; const thumbnails = []; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-nested-ifs.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-nested-ifs.expect.md index f7b3605f4d5a0..e0525518c968b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-nested-ifs.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-nested-ifs.expect.md @@ -6,6 +6,7 @@ function Component(props) { const x = useMemo(() => { if (props.cond) { if (props.cond) { + return props.value; } } }, [props.cond]); @@ -24,10 +25,18 @@ export const FIXTURE_ENTRYPOINT = { ```javascript function Component(props) { - if (props.cond) { + let t0; + bb0: { if (props.cond) { + if (props.cond) { + t0 = props.value; + break bb0; + } } + t0 = undefined; } + const x = t0; + return x; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-nested-ifs.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-nested-ifs.js index f9b329e1290b0..d122dcc7a3e73 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-nested-ifs.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-nested-ifs.js @@ -2,6 +2,7 @@ function Component(props) { const x = useMemo(() => { if (props.cond) { if (props.cond) { + return props.value; } } }, [props.cond]); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-return-empty.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-return-empty.expect.md deleted file mode 100644 index be20ee39bd4ec..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-return-empty.expect.md +++ /dev/null @@ -1,22 +0,0 @@ - -## Input - -```javascript -function component(a) { - let x = useMemo(() => { - mutate(a); - }, []); - return x; -} - -``` - -## Code - -```javascript -function component(a) { - mutate(a); -} - -``` - \ No newline at end of file diff --git a/compiler/packages/eslint-plugin-react-compiler/__tests__/PluginTest-test.ts b/compiler/packages/eslint-plugin-react-compiler/__tests__/PluginTest-test.ts index 89eb2ab22f6b0..e0063bc0a2c91 100644 --- a/compiler/packages/eslint-plugin-react-compiler/__tests__/PluginTest-test.ts +++ b/compiler/packages/eslint-plugin-react-compiler/__tests__/PluginTest-test.ts @@ -120,7 +120,15 @@ testRule('plugin-recommended', TestRecommendedRules, { return ; }`, - errors: [makeTestCaseError('Unused useMemo()')], + errors: [ + makeTestCaseError('useMemo() callbacks must return a value'), + makeTestCaseError( + 'Calling setState from useMemo may trigger an infinite loop', + ), + makeTestCaseError( + 'Calling setState from useMemo may trigger an infinite loop', + ), + ], }, { name: 'Pipeline errors are reported', From 55bf051afd650cffc3601fe0e1ea548665cfa846 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 16 Oct 2025 13:08:23 -0700 Subject: [PATCH 2/2] [compiler][wip] Fix for ValidatePreserveMemo edge case w multiple return in useMemo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Potential fix for #34750. With a useMemo with multiple returns we can sometimes end up thinking the FinishMemoize value wasn't memoized, even though it definitely is. But the same case works if you rewrite to have a let binding, assign to the let instead of returning, and then have a single return with the let — which is the exact shape we convert to when we inline the useMemo! The only difference is that when we inline, we end up with an extra LoadLocal. Adding an extra LoadLocal "fixes" the issue but this is definitely a hack. However, along the way this helped me find that we also have a bug in PruneAlwaysInvalidatingScopes, where we were forgetting to set FinishMemoize instructions to pruned if their declaration always invalidates. Putting up for feedback, not necessarily expecting to land as-is. --- .../src/HIR/HIRBuilder.ts | 2 +- .../src/Inference/DropManualMemoization.ts | 81 ++++++++------- .../PruneAlwaysInvalidatingScopes.ts | 9 ++ ...xpected-consistent-destructuring.expect.md | 2 +- ...ropped-infer-always-invalidating.expect.md | 49 ---------- ...eMemo-dropped-infer-always-invalidating.ts | 21 ---- ...ed-if-it-was-always-invalidating.expect.md | 56 +++++++++++ ...preserved-if-it-was-always-invalidating.ts | 21 ++++ ...ed-useMemo-with-multiple-returns.expect.md | 98 +++++++++++++++++++ ...e-unnamed-useMemo-with-multiple-returns.js | 28 ++++++ 10 files changed, 260 insertions(+), 107 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/repro-useMemo-counts-as-preserved-if-it-was-always-invalidating.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/repro-useMemo-counts-as-preserved-if-it-was-always-invalidating.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-inline-unnamed-useMemo-with-multiple-returns.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-inline-unnamed-useMemo-with-multiple-returns.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts index d3ecb2abdcd7d..1a2a56a7f7982 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts @@ -988,7 +988,7 @@ export function createTemporaryPlace( identifier: makeTemporaryIdentifier(env.nextIdentifierId, loc), reactive: false, effect: Effect.Unknown, - loc: GeneratedSource, + loc, }; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts index 0876424e28c14..a1cdc4e1a35c1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts @@ -11,7 +11,6 @@ import { CallExpression, Effect, Environment, - FinishMemoize, FunctionExpression, HIRFunction, IdentifierId, @@ -25,7 +24,6 @@ import { Place, PropertyLoad, SpreadPattern, - StartMemoize, TInstruction, getHookKindForType, makeInstructionId, @@ -184,36 +182,52 @@ function makeManualMemoizationMarkers( depsList: Array | null, memoDecl: Place, manualMemoId: number, -): [TInstruction, TInstruction] { +): [Array, Array] { + const temp = createTemporaryPlace(env, memoDecl.loc); return [ - { - id: makeInstructionId(0), - lvalue: createTemporaryPlace(env, fnExpr.loc), - value: { - kind: 'StartMemoize', - manualMemoId, - /* - * Use deps list from source instead of inferred deps - * as dependencies - */ - deps: depsList, + [ + { + id: makeInstructionId(0), + lvalue: createTemporaryPlace(env, fnExpr.loc), + value: { + kind: 'StartMemoize', + manualMemoId, + /* + * Use deps list from source instead of inferred deps + * as dependencies + */ + deps: depsList, + loc: fnExpr.loc, + }, + effects: null, loc: fnExpr.loc, }, - effects: null, - loc: fnExpr.loc, - }, - { - id: makeInstructionId(0), - lvalue: createTemporaryPlace(env, fnExpr.loc), - value: { - kind: 'FinishMemoize', - manualMemoId, - decl: {...memoDecl}, - loc: fnExpr.loc, + ], + [ + { + id: makeInstructionId(0), + lvalue: {...temp}, + value: { + kind: 'LoadLocal', + place: {...memoDecl}, + loc: memoDecl.loc, + }, + effects: null, + loc: memoDecl.loc, + }, + { + id: makeInstructionId(0), + lvalue: createTemporaryPlace(env, memoDecl.loc), + value: { + kind: 'FinishMemoize', + manualMemoId, + decl: {...temp}, + loc: memoDecl.loc, + }, + effects: null, + loc: memoDecl.loc, }, - effects: null, - loc: fnExpr.loc, - }, + ], ]; } @@ -409,10 +423,7 @@ export function dropManualMemoization( * LoadLocal fnArg * - (if validation is enabled) collect manual memoization markers */ - const queuedInserts: Map< - InstructionId, - TInstruction | TInstruction - > = new Map(); + const queuedInserts: Map> = new Map(); for (const [_, block] of func.body.blocks) { for (let i = 0; i < block.instructions.length; i++) { const instr = block.instructions[i]!; @@ -523,11 +534,11 @@ export function dropManualMemoization( let nextInstructions: Array | null = null; for (let i = 0; i < block.instructions.length; i++) { const instr = block.instructions[i]; - const insertInstr = queuedInserts.get(instr.id); - if (insertInstr != null) { + const insertInstructions = queuedInserts.get(instr.id); + if (insertInstructions != null) { nextInstructions = nextInstructions ?? block.instructions.slice(0, i); nextInstructions.push(instr); - nextInstructions.push(insertInstr); + nextInstructions.push(...insertInstructions); } else if (nextInstructions != null) { nextInstructions.push(instr); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts index b470271cf00b9..66b2bdcb56e98 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts @@ -77,6 +77,15 @@ class Transform extends ReactiveFunctionTransform { } break; } + case 'FinishMemoize': { + if ( + !withinScope && + this.alwaysInvalidatingValues.has(value.decl.identifier) + ) { + value.pruned = true; + } + break; + } } return {kind: 'keep'}; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-invariant-expected-consistent-destructuring.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-invariant-expected-consistent-destructuring.expect.md index a30ccffcd7bd0..bd787b2f0cabd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-invariant-expected-consistent-destructuring.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.bug-invariant-expected-consistent-destructuring.expect.md @@ -29,7 +29,7 @@ Found 1 error: Invariant: Expected consistent kind for destructuring -Other places were `Reassign` but 'mutate? #t8$46[7:9]{reactive}' is const. +Other places were `Reassign` but 'mutate? #t8$47[7:9]{reactive}' is const. error.bug-invariant-expected-consistent-destructuring.ts:9:9 7 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md deleted file mode 100644 index aead04aa95354..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md +++ /dev/null @@ -1,49 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees - -import {useMemo} from 'react'; -import {useHook} from 'shared-runtime'; - -// useMemo values may not be memoized in Forget output if we -// infer that their deps always invalidate. -// This is technically a false positive as the useMemo in source -// was effectively a no-op -function useFoo(props) { - const x = []; - useHook(); - x.push(props); - - return useMemo(() => [x], [x]); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [{}], -}; - -``` - - -## Error - -``` -Found 1 error: - -Compilation Skipped: Existing memoization could not be preserved - -React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. - -error.false-positive-useMemo-dropped-infer-always-invalidating.ts:15:9 - 13 | x.push(props); - 14 | -> 15 | return useMemo(() => [x], [x]); - | ^^^^^^^^^^^^^^^^^^^^^^^ Could not preserve existing memoization - 16 | } - 17 | - 18 | export const FIXTURE_ENTRYPOINT = { -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.ts deleted file mode 100644 index 3265182c51dfd..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.ts +++ /dev/null @@ -1,21 +0,0 @@ -// @validatePreserveExistingMemoizationGuarantees - -import {useMemo} from 'react'; -import {useHook} from 'shared-runtime'; - -// useMemo values may not be memoized in Forget output if we -// infer that their deps always invalidate. -// This is technically a false positive as the useMemo in source -// was effectively a no-op -function useFoo(props) { - const x = []; - useHook(); - x.push(props); - - return useMemo(() => [x], [x]); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [{}], -}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/repro-useMemo-counts-as-preserved-if-it-was-always-invalidating.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/repro-useMemo-counts-as-preserved-if-it-was-always-invalidating.expect.md new file mode 100644 index 0000000000000..83c93faf895ee --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/repro-useMemo-counts-as-preserved-if-it-was-always-invalidating.expect.md @@ -0,0 +1,56 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {useHook} from 'shared-runtime'; + +// If we can prove that a useMemo was ineffective because it would always invalidate, +// then we shouldn't throw a "couldn't preserve existing memoization" error +// TODO: consider reporting a separate error to the user for this case, if you're going +// to memoize manually, then you probably want to know that it's a no-op +function useFoo(props) { + const x = []; + useHook(); + x.push(props); + + return useMemo(() => [x], [x]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{}], +}; + +``` + +## Code + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import { useMemo } from "react"; +import { useHook } from "shared-runtime"; + +// If we can prove that a useMemo was ineffective because it would always invalidate, +// then we shouldn't throw a "couldn't preserve existing memoization" error +// TODO: consider reporting a separate error to the user for this case, if you're going +// to memoize manually, then you probably want to know that it's a no-op +function useFoo(props) { + const x = []; + useHook(); + x.push(props); + return [x]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) [[{}]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/repro-useMemo-counts-as-preserved-if-it-was-always-invalidating.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/repro-useMemo-counts-as-preserved-if-it-was-always-invalidating.ts new file mode 100644 index 0000000000000..28689b141f8e4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/repro-useMemo-counts-as-preserved-if-it-was-always-invalidating.ts @@ -0,0 +1,21 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {useHook} from 'shared-runtime'; + +// If we can prove that a useMemo was ineffective because it would always invalidate, +// then we shouldn't throw a "couldn't preserve existing memoization" error +// TODO: consider reporting a separate error to the user for this case, if you're going +// to memoize manually, then you probably want to know that it's a no-op +function useFoo(props) { + const x = []; + useHook(); + x.push(props); + + return useMemo(() => [x], [x]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-inline-unnamed-useMemo-with-multiple-returns.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-inline-unnamed-useMemo-with-multiple-returns.expect.md new file mode 100644 index 0000000000000..bdec6f48d4a7e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-inline-unnamed-useMemo-with-multiple-returns.expect.md @@ -0,0 +1,98 @@ + +## Input + +```javascript +import {useMemo} from 'react'; +import {identity, useIdentity} from 'shared-runtime'; + +// Adapted from https://github.com/facebook/react/issues/34750 +function useLocalCampaignBySlug(slug: string) { + const campaigns = useIdentity({a: {slug: 'a', name: 'campaign'}}); + // The useMemo result is never assigned to a local so we did not previously ensure + // that there was a variable declaration for it when promoting the result temporary + return useMemo(() => { + for (const id of Object.keys(campaigns)) { + const campaign = campaigns[id]; + if (campaign.slug === slug) { + return identity(campaign); + } + } + return null; + }, [campaigns, slug]); +} + +function Component() { + const campaign = useLocalCampaignBySlug('a'); + return
{campaign.name}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useMemo } from "react"; +import { identity, useIdentity } from "shared-runtime"; + +// Adapted from https://github.com/facebook/react/issues/34750 +function useLocalCampaignBySlug(slug) { + const $ = _c(4); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = { a: { slug: "a", name: "campaign" } }; + $[0] = t0; + } else { + t0 = $[0]; + } + const campaigns = useIdentity(t0); + let t1; + if ($[1] !== campaigns || $[2] !== slug) { + bb0: { + for (const id of Object.keys(campaigns)) { + const campaign = campaigns[id]; + if (campaign.slug === slug) { + t1 = identity(campaign); + break bb0; + } + } + + t1 = null; + } + $[1] = campaigns; + $[2] = slug; + $[3] = t1; + } else { + t1 = $[3]; + } + return t1; +} + +function Component() { + const $ = _c(2); + const campaign = useLocalCampaignBySlug("a"); + let t0; + if ($[0] !== campaign.name) { + t0 =
{campaign.name}
; + $[0] = campaign.name; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok)
campaign
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-inline-unnamed-useMemo-with-multiple-returns.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-inline-unnamed-useMemo-with-multiple-returns.js new file mode 100644 index 0000000000000..2c939ccce2574 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-inline-unnamed-useMemo-with-multiple-returns.js @@ -0,0 +1,28 @@ +import {useMemo} from 'react'; +import {identity, useIdentity} from 'shared-runtime'; + +// Adapted from https://github.com/facebook/react/issues/34750 +function useLocalCampaignBySlug(slug: string) { + const campaigns = useIdentity({a: {slug: 'a', name: 'campaign'}}); + // The useMemo result is never assigned to a local so we did not previously ensure + // that there was a variable declaration for it when promoting the result temporary + return useMemo(() => { + for (const id of Object.keys(campaigns)) { + const campaign = campaigns[id]; + if (campaign.slug === slug) { + return identity(campaign); + } + } + return null; + }, [campaigns, slug]); +} + +function Component() { + const campaign = useLocalCampaignBySlug('a'); + return
{campaign.name}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +};