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 f86747618dd59..d31cd1446b99b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts @@ -458,9 +458,9 @@ export function dropManualMemoization( 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`, + ? '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', 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 de74af93b6d82..0fdfeaab3ea5a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateUseMemo.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateUseMemo.ts @@ -10,7 +10,16 @@ import { CompilerError, ErrorCategory, } from '../CompilerError'; -import {FunctionExpression, HIRFunction, IdentifierId} from '../HIR'; +import { + FunctionExpression, + HIRFunction, + IdentifierId, + SourceLocation, +} from '../HIR'; +import { + eachInstructionValueOperand, + eachTerminalOperand, +} from '../HIR/visitors'; import {Result} from '../Utils/Result'; export function validateUseMemo(fn: HIRFunction): Result { @@ -18,8 +27,19 @@ export function validateUseMemo(fn: HIRFunction): Result { const useMemos = new Set(); const react = new Set(); const functions = new Map(); + const unusedUseMemos = new Map(); for (const [, block] of fn.body.blocks) { for (const {lvalue, value} of block.instructions) { + if (unusedUseMemos.size !== 0) { + /** + * Most of the time useMemo results are referenced immediately. Don't bother + * scanning instruction operands for useMemos unless there is an as-yet-unused + * useMemo. + */ + for (const operand of eachInstructionValueOperand(value)) { + unusedUseMemos.delete(operand.identifier.id); + } + } switch (value.kind) { case 'LoadGlobal': { if (value.binding.name === 'useMemo') { @@ -45,10 +65,8 @@ export function validateUseMemo(fn: HIRFunction): Result { case 'CallExpression': { // Is the function being called useMemo, with at least 1 argument? const callee = - value.kind === 'CallExpression' - ? value.callee.identifier.id - : value.property.identifier.id; - const isUseMemo = useMemos.has(callee); + value.kind === 'CallExpression' ? value.callee : value.property; + const isUseMemo = useMemos.has(callee.identifier.id); if (!isUseMemo || value.args.length === 0) { continue; } @@ -104,10 +122,73 @@ export function validateUseMemo(fn: HIRFunction): Result { ); } + validateNoContextVariableAssignment(body.loweredFunc.func, errors); + + if (fn.env.config.validateNoVoidUseMemo) { + unusedUseMemos.set(lvalue.identifier.id, callee.loc); + } break; } } } + if (unusedUseMemos.size !== 0) { + for (const operand of eachTerminalOperand(block.terminal)) { + unusedUseMemos.delete(operand.identifier.id); + } + } + } + if (unusedUseMemos.size !== 0) { + /** + * Basic check for unused memos, where the result of the call is never referenced. This runs + * before DCE so it's more of an AST-level check that something, _anything_, cares about the value. + * + * This is easy to defeat with e.g. `const _ = useMemo(...)` but it at least gives us something to teach. + * Even a DCE-based version could be bypassed with `noop(useMemo(...))`. + */ + for (const loc of unusedUseMemos.values()) { + errors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.VoidUseMemo, + reason: 'Unused useMemo()', + description: `This useMemo() value is unused. useMemo() is for computing and caching values, not for arbitrary side effects`, + suggestions: null, + }).withDetails({ + kind: 'error', + loc, + message: 'useMemo() result is unused', + }), + ); + } } return errors.asResult(); } + +function validateNoContextVariableAssignment( + fn: HIRFunction, + errors: CompilerError, +): void { + for (const block of fn.body.blocks.values()) { + for (const instr of block.instructions) { + const value = instr.value; + switch (value.kind) { + case 'StoreContext': { + errors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.UseMemo, + reason: + 'useMemo() callbacks may not reassign variables declared outside of the callback', + description: + 'useMemo() callbacks must be pure functions and cannot reassign variables defined outside of the callback function', + suggestions: null, + }).withDetails({ + kind: 'error', + loc: value.lvalue.place.loc, + message: 'Cannot reassign variable', + }), + ); + break; + } + } + } + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-variable-in-usememo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-variable-in-usememo.expect.md new file mode 100644 index 0000000000000..ba35168bcfedb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-variable-in-usememo.expect.md @@ -0,0 +1,38 @@ + +## Input + +```javascript +function Component() { + let x; + const y = useMemo(() => { + let z; + x = []; + z = true; + return z; + }, []); + return [x, y]; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: useMemo() callbacks may not reassign variables declared outside of the callback + +useMemo() callbacks must be pure functions and cannot reassign variables defined outside of the callback function. + +error.invalid-reassign-variable-in-usememo.ts:5:4 + 3 | const y = useMemo(() => { + 4 | let z; +> 5 | x = []; + | ^ Cannot reassign variable + 6 | z = true; + 7 | return z; + 8 | }, []); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-variable-in-usememo.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-variable-in-usememo.js new file mode 100644 index 0000000000000..885ba4b52d5a0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-variable-in-usememo.js @@ -0,0 +1,10 @@ +function Component() { + let x; + const y = useMemo(() => { + let z; + x = []; + z = true; + return z; + }, []); + return [x, y]; +} 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 new file mode 100644 index 0000000000000..3c84a0dda335f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.expect.md @@ -0,0 +1,35 @@ + +## 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.invalid-unused-usememo.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.js new file mode 100644 index 0000000000000..1983cd061a60e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.js @@ -0,0 +1,7 @@ +// @validateNoVoidUseMemo +function Component() { + useMemo(() => { + return []; + }, []); + return
; +} 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 index fb5959640662c..9b62c5364b6fb 100644 --- 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 @@ -28,7 +28,7 @@ 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. +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 @@ -45,7 +45,7 @@ error.useMemo-no-return-value.ts:3:16 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. +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'); 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 6efd069aafa07..89eb2ab22f6b0 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,7 @@ testRule('plugin-recommended', TestRecommendedRules, { return ; }`, - errors: [makeTestCaseError('useMemo() callbacks must return a value')], + errors: [makeTestCaseError('Unused useMemo()')], }, { name: 'Pipeline errors are reported',