From d3b26b2953c7e780abaa49f422c53fd4cda08e47 Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Mon, 4 Aug 2025 12:04:44 -0700 Subject: [PATCH 1/2] [compiler] rebase #32285 (#34102) Redo of #32285 which was created with ghstack and is tedious to rebase with sapling. --- .../src/Entrypoint/Pipeline.ts | 5 + .../src/HIR/Environment.ts | 6 + .../ValidateNoDerivedComputationsInEffects.ts | 230 ++++++++++++++++++ ...id-derived-computation-in-effect.expect.md | 39 +++ ...r.invalid-derived-computation-in-effect.js | 13 + 5 files changed, 293 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.js 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 b7a679dd6241e..dbf974d818cf8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -106,6 +106,7 @@ import {validateStaticComponents} from '../Validation/ValidateStaticComponents'; import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoFreezingKnownMutableFunctions'; import {inferMutationAliasingEffects} from '../Inference/InferMutationAliasingEffects'; import {inferMutationAliasingRanges} from '../Inference/InferMutationAliasingRanges'; +import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDerivedComputationsInEffects'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -292,6 +293,10 @@ function runWithEnvironment( validateNoSetStateInRender(hir).unwrap(); } + if (env.config.validateNoDerivedComputationsInEffects) { + validateNoDerivedComputationsInEffects(hir); + } + if (env.config.validateNoSetStateInEffects) { env.logErrors(validateNoSetStateInEffects(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 f94870fc03032..0ac59a8982e2f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -323,6 +323,12 @@ export const EnvironmentConfigSchema = z.object({ */ validateNoSetStateInEffects: 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 0000000000000..d026a94ed4a29 --- /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-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md new file mode 100644 index 0000000000000..d97a665ae698c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md @@ -0,0 +1,39 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +function BadExample() { + 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 + +``` +Found 1 error: + +Error: 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) + +error.invalid-derived-computation-in-effect.ts:9:4 + 7 | const [fullName, setFullName] = useState(''); + 8 | useEffect(() => { +> 9 | setFullName(capitalize(firstName + ' ' + lastName)); + | ^^^^^^^^^^^ 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) + 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-derived-computation-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.js new file mode 100644 index 0000000000000..d803d3c4a3a1f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.js @@ -0,0 +1,13 @@ +// @validateNoDerivedComputationsInEffects +function BadExample() { + 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}
; +} From 7deda941f7f77e82de0311fc3e0cf94d8a863069 Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Mon, 4 Aug 2025 15:15:51 -0700 Subject: [PATCH 2/2] [compiler] Delete PropagatePhiTypes (#34107) We moved this logic into InferTypes a long time ago and the PRs to clean it up keep getting lost in the shuffle. --- .../src/Entrypoint/Pipeline.ts | 8 -- .../src/TypeInference/PropagatePhiTypes.ts | 110 ------------------ 2 files changed, 118 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/TypeInference/PropagatePhiTypes.ts 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 dbf974d818cf8..a4f984f1958d1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -92,7 +92,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 {validateNoSetStateInEffects} from '../Validation/ValidateNoSetStateInEffects'; import {validateNoJSXInTryStatement} from '../Validation/ValidateNoJSXInTryStatement'; @@ -327,13 +326,6 @@ function runWithEnvironment( value: hir, }); - propagatePhiTypes(hir); - log({ - kind: 'hir', - name: 'PropagatePhiTypes', - value: hir, - }); - if (env.isInferredMemoEnabled) { if (env.config.validateStaticComponents) { env.logErrors(validateStaticComponents(hir)); diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/PropagatePhiTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/PropagatePhiTypes.ts deleted file mode 100644 index 0369945525bcf..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/PropagatePhiTypes.ts +++ /dev/null @@ -1,110 +0,0 @@ -/** - * 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 {HIRFunction, IdentifierId, Type, typeEquals} from '../HIR'; - -/** - * Temporary workaround for InferTypes not propagating the types of phis. - * Previously, LeaveSSA would replace all the identifiers for each phi (operands and - * the phi itself) with a single "canonical" identifier, generally chosen as the first - * operand to flow into the phi. In case of a phi whose operand was a phi, this could - * sometimes be an operand from the earlier phi. - * - * As a result, even though InferTypes did not propagate types for phis, LeaveSSA - * could end up replacing the phi Identifier with another identifer from an operand, - * which _did_ have a type inferred. - * - * This didn't affect the initial construction of mutable ranges because InferMutableRanges - * runs before LeaveSSA - thus, the types propagated by LeaveSSA only affected later optimizations, - * notably MergeScopesThatInvalidateTogether which uses type to determine if a scope's output - * will always invalidate with its input. - * - * The long-term correct approach is to update InferTypes to infer the types of phis, - * but this is complicated because InferMutableRanges inadvertently depends on phis - * never having a known type, such that a Store effect cannot occur on a phi value. - * Once we fix InferTypes to infer phi types, then we'll also have to update InferMutableRanges - * to handle this case. - * - * As a temporary workaround, this pass propagates the type of phis and can be called - * safely *after* InferMutableRanges. Unlike LeaveSSA, this pass only propagates the - * type if all operands have the same type, it's its more correct. - */ -export function propagatePhiTypes(fn: HIRFunction): void { - /** - * We track which SSA ids have had their types propagated to handle nested ternaries, - * see the StoreLocal handling below - */ - const propagated = new Set(); - for (const [, block] of fn.body.blocks) { - for (const phi of block.phis) { - /* - * We replicate the previous LeaveSSA behavior and only propagate types for - * unnamed variables. LeaveSSA would have chosen one of the operands as the - * canonical id and taken its type as the type of all identifiers. We're - * more conservative and only propagate if the types are the same and the - * phi didn't have a type inferred. - * - * Note that this can change output slightly in cases such as - * `cond ?
: null`. - * - * Previously the first operand's type (BuiltInJsx) would have been propagated, - * and this expression may have been merged with subsequent reactive scopes - * since it appears (based on that type) to always invalidate. - * - * But the correct type is `BuiltInJsx | null`, which we can't express and - * so leave as a generic `Type`, which does not always invalidate and therefore - * does not merge with subsequent scopes. - * - * We also don't propagate scopes for named variables, to preserve compatibility - * with previous LeaveSSA behavior. - */ - if ( - phi.place.identifier.type.kind !== 'Type' || - phi.place.identifier.name !== null - ) { - continue; - } - let type: Type | null = null; - for (const [, operand] of phi.operands) { - if (type === null) { - type = operand.identifier.type; - } else if (!typeEquals(type, operand.identifier.type)) { - type = null; - break; - } - } - if (type !== null) { - phi.place.identifier.type = type; - propagated.add(phi.place.identifier.id); - } - } - for (const instr of block.instructions) { - const {value} = instr; - switch (value.kind) { - case 'StoreLocal': { - /** - * Nested ternaries can lower to a form with an intermediate StoreLocal where - * the value.lvalue is the temporary of the outer ternary, and the value.value - * is the result of the inner ternary. - * - * This is a common pattern in practice and easy enough to support. Again, the - * long-term approach is to update InferTypes and InferMutableRanges. - */ - const lvalue = value.lvalue.place; - if ( - propagated.has(value.value.identifier.id) && - lvalue.identifier.type.kind === 'Type' && - lvalue.identifier.name === null - ) { - lvalue.identifier.type = value.value.identifier.type; - propagated.add(lvalue.identifier.id); - } - } - } - } - } -}