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 2fccef745ce..f90e18788c3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -91,7 +91,6 @@ import { } from '../Validation'; import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLocalsNotReassignedAfterRender'; import {outlineFunctions} from '../Optimization/OutlineFunctions'; -import {propagatePhiTypes} from '../TypeInference/PropagatePhiTypes'; import {lowerContextAccess} from '../Optimization/LowerContextAccess'; import {validateNoSetStateInPassiveEffects} from '../Validation/ValidateNoSetStateInPassiveEffects'; import {validateNoJSXInTryStatement} from '../Validation/ValidateNoJSXInTryStatement'; @@ -100,6 +99,7 @@ import {outlineJSX} from '../Optimization/OutlineJsx'; import {optimizePropsMethodCalls} from '../Optimization/OptimizePropsMethodCalls'; import {transformFire} from '../Transform'; import {validateNoImpureFunctionsInRender} from '../Validation/ValiateNoImpureFunctionsInRender'; +import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDerivedComputationsInEffects'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -251,6 +251,10 @@ function runWithEnvironment( validateNoSetStateInRender(hir); } + if (env.config.validateNoDerivedComputationsInEffects) { + validateNoDerivedComputationsInEffects(hir); + } + if (env.config.validateNoSetStateInPassiveEffects) { validateNoSetStateInPassiveEffects(hir); } 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 113af2025dd..d17333cc8ae 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -324,6 +324,12 @@ const EnvironmentConfigSchema = z.object({ */ validateNoSetStateInPassiveEffects: z.boolean().default(false), + /** + * Validates that effects are not used to calculate derived data which could instead be computed + * during render. + */ + validateNoDerivedComputationsInEffects: z.boolean().default(false), + /** * Validates against creating JSX within a try block and recommends using an error boundary * instead. diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts new file mode 100644 index 00000000000..d026a94ed4a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -0,0 +1,230 @@ +/** + * 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, SourceLocation} from '..'; +import { + ArrayExpression, + BlockId, + FunctionExpression, + HIRFunction, + IdentifierId, + isSetStateType, + isUseEffectHookType, +} from '../HIR'; +import { + eachInstructionValueOperand, + eachTerminalOperand, +} from '../HIR/visitors'; + +/** + * Validates that useEffect is not used for derived computations which could/should + * be performed in render. + * + * See https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state + * + * Example: + * + * ``` + * // 🔴 Avoid: redundant state and unnecessary Effect + * const [fullName, setFullName] = useState(''); + * useEffect(() => { + * setFullName(firstName + ' ' + lastName); + * }, [firstName, lastName]); + * ``` + * + * Instead use: + * + * ``` + * // ✅ Good: calculated during rendering + * const fullName = firstName + ' ' + lastName; + * ``` + */ +export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { + const candidateDependencies: Map = new Map(); + const functions: Map = new Map(); + const locals: Map = new Map(); + + const errors = new CompilerError(); + + for (const block of fn.body.blocks.values()) { + for (const instr of block.instructions) { + const {lvalue, value} = instr; + if (value.kind === 'LoadLocal') { + locals.set(lvalue.identifier.id, value.place.identifier.id); + } else if (value.kind === 'ArrayExpression') { + candidateDependencies.set(lvalue.identifier.id, value); + } else if (value.kind === 'FunctionExpression') { + functions.set(lvalue.identifier.id, value); + } else if ( + value.kind === 'CallExpression' || + value.kind === 'MethodCall' + ) { + const callee = + value.kind === 'CallExpression' ? value.callee : value.property; + if ( + isUseEffectHookType(callee.identifier) && + value.args.length === 2 && + value.args[0].kind === 'Identifier' && + value.args[1].kind === 'Identifier' + ) { + const effectFunction = functions.get(value.args[0].identifier.id); + const deps = candidateDependencies.get(value.args[1].identifier.id); + if ( + effectFunction != null && + deps != null && + deps.elements.length !== 0 && + deps.elements.every(element => element.kind === 'Identifier') + ) { + const dependencies: Array = deps.elements.map(dep => { + CompilerError.invariant(dep.kind === 'Identifier', { + reason: `Dependency is checked as a place above`, + loc: value.loc, + }); + return locals.get(dep.identifier.id) ?? dep.identifier.id; + }); + validateEffect( + effectFunction.loweredFunc.func, + dependencies, + errors, + ); + } + } + } + } + } + if (errors.hasErrors()) { + throw errors; + } +} + +function validateEffect( + effectFunction: HIRFunction, + effectDeps: Array, + errors: CompilerError, +): void { + for (const operand of effectFunction.context) { + if (isSetStateType(operand.identifier)) { + continue; + } else if (effectDeps.find(dep => dep === operand.identifier.id) != null) { + continue; + } else { + // Captured something other than the effect dep or setState + return; + } + } + for (const dep of effectDeps) { + if ( + effectFunction.context.find(operand => operand.identifier.id === dep) == + null + ) { + // effect dep wasn't actually used in the function + return; + } + } + + const seenBlocks: Set = new Set(); + const values: Map> = new Map(); + for (const dep of effectDeps) { + values.set(dep, [dep]); + } + + const setStateLocations: Array = []; + for (const block of effectFunction.body.blocks.values()) { + for (const pred of block.preds) { + if (!seenBlocks.has(pred)) { + // skip if block has a back edge + return; + } + } + for (const phi of block.phis) { + const aggregateDeps: Set = new Set(); + for (const operand of phi.operands.values()) { + const deps = values.get(operand.identifier.id); + if (deps != null) { + for (const dep of deps) { + aggregateDeps.add(dep); + } + } + } + if (aggregateDeps.size !== 0) { + values.set(phi.place.identifier.id, Array.from(aggregateDeps)); + } + } + for (const instr of block.instructions) { + switch (instr.value.kind) { + case 'Primitive': + case 'JSXText': + case 'LoadGlobal': { + break; + } + case 'LoadLocal': { + const deps = values.get(instr.value.place.identifier.id); + if (deps != null) { + values.set(instr.lvalue.identifier.id, deps); + } + break; + } + case 'ComputedLoad': + case 'PropertyLoad': + case 'BinaryExpression': + case 'TemplateLiteral': + case 'CallExpression': + case 'MethodCall': { + const aggregateDeps: Set = new Set(); + for (const operand of eachInstructionValueOperand(instr.value)) { + const deps = values.get(operand.identifier.id); + if (deps != null) { + for (const dep of deps) { + aggregateDeps.add(dep); + } + } + } + if (aggregateDeps.size !== 0) { + values.set(instr.lvalue.identifier.id, Array.from(aggregateDeps)); + } + + if ( + instr.value.kind === 'CallExpression' && + isSetStateType(instr.value.callee.identifier) && + instr.value.args.length === 1 && + instr.value.args[0].kind === 'Identifier' + ) { + const deps = values.get(instr.value.args[0].identifier.id); + if (deps != null && new Set(deps).size === effectDeps.length) { + setStateLocations.push(instr.value.callee.loc); + } else { + // doesn't depend on any deps + return; + } + } + break; + } + default: { + return; + } + } + } + for (const operand of eachTerminalOperand(block.terminal)) { + if (values.has(operand.identifier.id)) { + // + return; + } + } + seenBlocks.add(block.id); + } + + for (const loc of setStateLocations) { + errors.push({ + reason: + 'Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)', + description: null, + severity: ErrorSeverity.InvalidReact, + loc, + suggestions: null, + }); + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-validate-no-derived-computations-in-effects.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-validate-no-derived-computations-in-effects.expect.md new file mode 100644 index 00000000000..a3065de4eb6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-validate-no-derived-computations-in-effects.expect.md @@ -0,0 +1,34 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +function Form() { + const [firstName, setFirstName] = useState('Taylor'); + const [lastName, setLastName] = useState('Swift'); + + // 🔴 Avoid: redundant state and unnecessary Effect + const [fullName, setFullName] = useState(''); + useEffect(() => { + setFullName(capitalize(firstName + ' ' + lastName)); + }, [firstName, lastName]); + + return
{fullName}
; +} + +``` + + +## Error + +``` + 7 | const [fullName, setFullName] = useState(''); + 8 | useEffect(() => { +> 9 | setFullName(capitalize(firstName + ' ' + lastName)); + | ^^^^^^^^^^^ InvalidReact: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) (9:9) + 10 | }, [firstName, lastName]); + 11 | + 12 | return
{fullName}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-validate-no-derived-computations-in-effects.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-validate-no-derived-computations-in-effects.js new file mode 100644 index 00000000000..f358e75aaa7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-validate-no-derived-computations-in-effects.js @@ -0,0 +1,13 @@ +// @validateNoDerivedComputationsInEffects +function Form() { + const [firstName, setFirstName] = useState('Taylor'); + const [lastName, setLastName] = useState('Swift'); + + // 🔴 Avoid: redundant state and unnecessary Effect + const [fullName, setFullName] = useState(''); + useEffect(() => { + setFullName(capitalize(firstName + ' ' + lastName)); + }, [firstName, lastName]); + + return
{fullName}
; +}