diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 648ff01ba7..8acd868dde 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -82,6 +82,7 @@ import { import {inferTypes} from '../TypeInference'; import { validateContextVariableLValues, + validateNoVoidUseMemo, validateHooksUsage, validateMemoizedEffectDependencies, validateNoCapitalizedCalls, @@ -167,6 +168,9 @@ function runWithEnvironment( validateContextVariableLValues(hir); validateUseMemo(hir).unwrap(); + if (env.config.enableValidateNoVoidUseMemo) { + validateNoVoidUseMemo(hir).unwrap(); + } if ( env.isInferredMemoEnabled && 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 5b5b78fc52..63cf84f331 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -631,6 +631,17 @@ export const EnvironmentConfigSchema = z.object({ * ``` */ lowerContextAccess: ExternalFunctionSchema.nullable().default(null), + + /** + * If enabled, will validate useMemos that don't return any values: + * + * Valid: + * useMemo(() => foo, [foo]); + * useMemo(() => { return foo }, [foo]); + * Invalid: + * useMemo(() => { ... }, [...]); + */ + enableValidateNoVoidUseMemo: z.boolean().default(false), }); export type EnvironmentConfig = z.infer; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoVoidUseMemo.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoVoidUseMemo.ts new file mode 100644 index 0000000000..c60ea6269d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoVoidUseMemo.ts @@ -0,0 +1,156 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {CompilerError, ErrorSeverity} from '../CompilerError'; +import { + HIRFunction, + IdentifierId, + FunctionExpression, + SourceLocation, + Environment, + Instruction, + getHookKindForType, +} from '../HIR'; +import {Result} from '../Utils/Result'; + +type TemporariesSidemap = { + useMemoHooks: Map; + funcExprs: Map; + react: Set; +}; + +/** + * Validates that useMemo has at least one explicit return statement. + * + * Valid cases: + * - useMemo(() => value) // implicit arrow function return + * - useMemo(() => { return value; }) // explicit return + * - useMemo(() => { return; }) // explicit undefined + * - useMemo(() => { if (cond) return val; }) // at least one return + * + * Invalid cases: + * - useMemo(() => { console.log(); }) // no return statement at all + */ +export function validateNoVoidUseMemo( + fn: HIRFunction, +): Result { + const errors = new CompilerError(); + const sidemap: TemporariesSidemap = { + useMemoHooks: new Map(), + funcExprs: new Map(), + react: new Set(), + }; + + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + collectTemporaries(instr, fn.env, sidemap); + } + } + + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + if (instr.value.kind === 'CallExpression') { + const callee = instr.value.callee.identifier; + const useMemoHook = sidemap.useMemoHooks.get(callee.id); + + if (useMemoHook !== undefined && instr.value.args.length > 0) { + const firstArg = instr.value.args[0]; + if (firstArg.kind !== 'Identifier') { + continue; + } + + let funcToCheck = sidemap.funcExprs.get(firstArg.identifier.id); + + if (!funcToCheck) { + for (const [, searchBlock] of fn.body.blocks) { + for (const searchInstr of searchBlock.instructions) { + if ( + searchInstr.lvalue && + searchInstr.lvalue.identifier.id === firstArg.identifier.id && + searchInstr.value.kind === 'FunctionExpression' + ) { + funcToCheck = searchInstr.value; + break; + } + } + if (funcToCheck) break; + } + } + + if (funcToCheck) { + const hasReturn = checkFunctionHasNonVoidReturn( + funcToCheck.loweredFunc.func, + ); + + if (!hasReturn) { + errors.push({ + severity: ErrorSeverity.InvalidReact, + reason: `React Compiler has skipped optimizing this component because ${useMemoHook.name} doesn't return a value. ${useMemoHook.name} should only be used for memoizing values, not running arbitrary side effects.`, + loc: useMemoHook.loc, + suggestions: null, + description: null, + }); + } + } + } + } + } + } + return errors.asResult(); +} + +function checkFunctionHasNonVoidReturn(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; +} + +function collectTemporaries( + instr: Instruction, + env: Environment, + sidemap: TemporariesSidemap, +): void { + const {value, lvalue} = instr; + switch (value.kind) { + case 'FunctionExpression': { + sidemap.funcExprs.set(lvalue.identifier.id, value); + break; + } + case 'LoadGlobal': { + const global = env.getGlobalDeclaration(value.binding, value.loc); + const hookKind = global !== null ? getHookKindForType(env, global) : null; + if (hookKind === 'useMemo') { + sidemap.useMemoHooks.set(lvalue.identifier.id, { + name: value.binding.name, + loc: instr.loc, + }); + } else if (value.binding.name === 'React') { + sidemap.react.add(lvalue.identifier.id); + } + break; + } + case 'PropertyLoad': { + if (sidemap.react.has(value.object.identifier.id)) { + if (value.property === 'useMemo') { + sidemap.useMemoHooks.set(lvalue.identifier.id, { + name: value.property, + loc: instr.loc, + }); + } + } + break; + } + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/index.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/index.ts index 3bf03f362f..2e7676dcd9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/index.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/index.ts @@ -6,6 +6,7 @@ */ export {validateContextVariableLValues} from './ValidateContextVariableLValues'; +export {validateNoVoidUseMemo} from './ValidateNoVoidUseMemo'; export {validateHooksUsage} from './ValidateHooksUsage'; export {validateMemoizedEffectDependencies} from './ValidateMemoizedEffectDependencies'; export {validateNoCapitalizedCalls} from './ValidateNoCapitalizedCalls'; 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 new file mode 100644 index 0000000000..79fa6c86da --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.expect.md @@ -0,0 +1,33 @@ + +## Input + +```javascript +// @enableValidateNoVoidUseMemo +function Component() { + const value = useMemo(() => { + console.log('computing'); + }, []); + return
{value}
; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: React Compiler has skipped optimizing this component because useMemo doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects. + +error.useMemo-no-return-value.ts:3:16 + 1 | // @enableValidateNoVoidUseMemo + 2 | function Component() { +> 3 | const value = useMemo(() => { + | ^^^^^^^ React Compiler has skipped optimizing this component because useMemo doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects. + 4 | console.log('computing'); + 5 | }, []); + 6 | return
{value}
; +``` + + \ 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/error.useMemo-no-return-value.js new file mode 100644 index 0000000000..be29a988ec --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.js @@ -0,0 +1,7 @@ +// @enableValidateNoVoidUseMemo +function Component() { + const value = useMemo(() => { + console.log('computing'); + }, []); + return
{value}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-arrow-implicit-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-arrow-implicit-return.expect.md new file mode 100644 index 0000000000..467de1ee6a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-arrow-implicit-return.expect.md @@ -0,0 +1,39 @@ + +## Input + +```javascript +function Component() { + const value = useMemo(() => computeValue(), []); + return
{value}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component() { + const $ = _c(2); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = computeValue(); + $[0] = t0; + } else { + t0 = $[0]; + } + const value = t0; + let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 =
{value}
; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +``` + +### 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-arrow-implicit-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-arrow-implicit-return.js new file mode 100644 index 0000000000..af8bf5aa03 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-arrow-implicit-return.js @@ -0,0 +1,4 @@ +function Component() { + const value = useMemo(() => computeValue(), []); + return
{value}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-empty-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-empty-return.expect.md new file mode 100644 index 0000000000..0deffbf815 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-empty-return.expect.md @@ -0,0 +1,33 @@ + +## Input + +```javascript +function Component() { + const value = useMemo(() => { + return; + }, []); + return
{value}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 =
{undefined}
; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +``` + +### 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-empty-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-empty-return.js new file mode 100644 index 0000000000..737c6695af --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-empty-return.js @@ -0,0 +1,6 @@ +function Component() { + const value = useMemo(() => { + return; + }, []); + return
{value}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-explicit-null-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-explicit-null-return.expect.md new file mode 100644 index 0000000000..cf1517deaa --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-explicit-null-return.expect.md @@ -0,0 +1,33 @@ + +## Input + +```javascript +function Component() { + const value = useMemo(() => { + return null; + }, []); + return
{value}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 =
{null}
; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +``` + +### 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-explicit-null-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-explicit-null-return.js new file mode 100644 index 0000000000..7664830b2b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-explicit-null-return.js @@ -0,0 +1,6 @@ +function Component() { + const value = useMemo(() => { + return null; + }, []); + return
{value}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-returns.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-returns.expect.md new file mode 100644 index 0000000000..3a1cd2bc18 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-returns.expect.md @@ -0,0 +1,50 @@ + +## Input + +```javascript +function Component({items}) { + const value = useMemo(() => { + for (let item of items) { + if (item.match) return item; + } + return null; + }, [items]); + return
{value}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component(t0) { + const $ = _c(2); + const { items } = t0; + let t1; + bb0: { + for (const item of items) { + if (item.match) { + t1 = item; + break bb0; + } + } + + t1 = null; + } + const value = t1; + let t2; + if ($[0] !== value) { + t2 =
{value}
; + $[0] = value; + $[1] = t2; + } else { + t2 = $[1]; + } + return t2; +} + +``` + +### 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-multiple-returns.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-returns.js new file mode 100644 index 0000000000..624c78e4d1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-returns.js @@ -0,0 +1,9 @@ +function Component({items}) { + const value = useMemo(() => { + for (let item of items) { + if (item.match) return item; + } + return null; + }, [items]); + return
{value}
; +}