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 648ff01ba713c..1f9b3d3f8498b 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.validateNoVoidUseMemo) { + validateNoVoidUseMemo(hir).unwrap(); + } if ( env.isInferredMemoEnabled && diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts index aa940c99e6128..3b11670146b60 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -189,6 +189,7 @@ export function lower( const fallthrough = builder.reserve('block'); const terminal: ReturnTerminal = { kind: 'return', + returnVariant: 'Implicit', loc: GeneratedSource, value: lowerExpressionToTemporary(builder, body), id: makeInstructionId(0), @@ -219,6 +220,7 @@ export function lower( builder.terminate( { kind: 'return', + returnVariant: 'Void', loc: GeneratedSource, value: lowerValueToTemporary(builder, { kind: 'Primitive', @@ -302,6 +304,7 @@ function lowerStatement( } const terminal: ReturnTerminal = { kind: 'return', + returnVariant: 'Explicit', loc: stmt.node.loc ?? GeneratedSource, value, id: makeInstructionId(0), 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 5b5b78fc5202c..ba7396e0d7d13 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(() => { ... }, [...]); + */ + validateNoVoidUseMemo: z.boolean().default(false), }); export type EnvironmentConfig = z.infer; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index deb725a0482e1..f9caa844f3c52 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -446,8 +446,20 @@ export type ThrowTerminal = { }; export type Case = {test: Place | null; block: BlockId}; +export type ReturnVariant = 'Void' | 'Implicit' | 'Explicit'; export type ReturnTerminal = { kind: 'return'; + /** + * Void: + * () => { ... } + * function() { ... } + * Implicit (ArrowFunctionExpression only): + * () => foo + * Explicit: + * () => { return ... } + * function () { return ... } + */ + returnVariant: ReturnVariant; loc: SourceLocation; value: Place; id: InstructionId; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index 869799073e49c..aa6a7b0c65cea 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -215,7 +215,7 @@ export function printTerminal(terminal: Terminal): Array | string { break; } case 'return': { - value = `[${terminal.id}] Return${ + value = `[${terminal.id}] Return ${terminal.returnVariant}${ terminal.value != null ? ' ' + printPlace(terminal.value) : '' }`; if (terminal.effects != null) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts index 88786bc5dd2bb..64702c8abcdb6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts @@ -777,6 +777,7 @@ export function mapTerminalSuccessors( case 'return': { return { kind: 'return', + returnVariant: terminal.returnVariant, loc: terminal.loc, value: terminal.value, id: makeInstructionId(0), diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/LowerContextAccess.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/LowerContextAccess.ts index 921ec59ecd2a1..2d0b073a04596 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Optimization/LowerContextAccess.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Optimization/LowerContextAccess.ts @@ -237,6 +237,7 @@ function emitSelectorFn(env: Environment, keys: Array): Instruction { terminal: { id: makeInstructionId(0), kind: 'return', + returnVariant: 'Explicit', loc: GeneratedSource, value: arrayInstr.lvalue, effects: null, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineJsx.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineJsx.ts index b7590a570197a..e59d60271a0d4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineJsx.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineJsx.ts @@ -352,6 +352,7 @@ function emitOutlinedFn( terminal: { id: makeInstructionId(0), kind: 'return', + returnVariant: 'Explicit', loc: GeneratedSource, value: instructions.at(-1)!.lvalue, effects: null, 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 0000000000000..c60ea6269d991 --- /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 3bf03f362fa19..2e7676dcd9788 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 0000000000000..e87c222e6ee39 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.expect.md @@ -0,0 +1,41 @@ + +## Input + +```javascript +// @validateNoVoidUseMemo +function Component() { + const value = useMemo(() => { + console.log('computing'); + }, []); + const value2 = React.useMemo(() => { + console.log('computing'); + }, []); + return ( +
+ {value} + {value2} +
+ ); +} + +``` + + +## 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 | // @validateNoVoidUseMemo + 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 | const value2 = React.useMemo(() => { +``` + + \ 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 0000000000000..0ce35e12f4ff8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.js @@ -0,0 +1,15 @@ +// @validateNoVoidUseMemo +function Component() { + const value = useMemo(() => { + console.log('computing'); + }, []); + const value2 = React.useMemo(() => { + console.log('computing'); + }, []); + return ( +
+ {value} + {value2} +
+ ); +} 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 0000000000000..df3dae1d83a0c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-arrow-implicit-return.expect.md @@ -0,0 +1,40 @@ + +## Input + +```javascript +// @validateNoVoidUseMemo +function Component() { + const value = useMemo(() => computeValue(), []); + return
{value}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo +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 0000000000000..0ea121430d1da --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-arrow-implicit-return.js @@ -0,0 +1,5 @@ +// @validateNoVoidUseMemo +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 0000000000000..7be708ef5017c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-empty-return.expect.md @@ -0,0 +1,34 @@ + +## Input + +```javascript +// @validateNoVoidUseMemo +function Component() { + const value = useMemo(() => { + return; + }, []); + return
{value}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo +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 0000000000000..7985884d5657c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-empty-return.js @@ -0,0 +1,7 @@ +// @validateNoVoidUseMemo +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 0000000000000..d35213b00800a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-explicit-null-return.expect.md @@ -0,0 +1,34 @@ + +## Input + +```javascript +// @validateNoVoidUseMemo +function Component() { + const value = useMemo(() => { + return null; + }, []); + return
{value}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo +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 0000000000000..9b0a1a8253dcb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-explicit-null-return.js @@ -0,0 +1,7 @@ +// @validateNoVoidUseMemo +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 0000000000000..0fa2700721b4c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-returns.expect.md @@ -0,0 +1,51 @@ + +## Input + +```javascript +// @validateNoVoidUseMemo +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"; // @validateNoVoidUseMemo +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 0000000000000..dce32663f1cb4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-returns.js @@ -0,0 +1,10 @@ +// @validateNoVoidUseMemo +function Component({items}) { + const value = useMemo(() => { + for (let item of items) { + if (item.match) return item; + } + return null; + }, [items]); + return
{value}
; +}