From 1d9ccd1d1b17f2aa028fe2d7de92535f86134e81 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Wed, 24 Sep 2025 13:13:00 -0700 Subject: [PATCH 1/3] [Compiler] ValidateNoDerivedComputationsInEffects test cases Summary: This creates the test cases we expect this first iteration of calculate in render to catch The goal is to have tests that will be in a good state once we have the first iteration of the calculate in render validation working, which should be pretty limited in what its capturing. Test Plan: Test cases --- .../src/Entrypoint/Pipeline.ts | 5 + .../src/HIR/Environment.ts | 6 + ...idateNoDerivedComputationsInEffects_exp.ts | 240 ++++++++++++++++++ ...ed-state-conditionally-in-effect.expect.md | 79 ++++++ .../derived-state-conditionally-in-effect.js | 21 ++ ...derived-state-from-default-props.expect.md | 71 ++++++ .../derived-state-from-default-props.js | 18 ++ ...state-from-local-state-in-effect.expect.md | 70 +++++ ...erived-state-from-local-state-in-effect.js | 15 ++ ...-local-state-and-component-scope.expect.md | 108 ++++++++ ...om-prop-local-state-and-component-scope.js | 25 ++ ...state-from-prop-with-side-effect.expect.md | 71 ++++++ ...erived-state-from-prop-with-side-effect.js | 18 ++ ...ect-contains-local-function-call.expect.md | 86 +++++++ .../effect-contains-local-function-call.js | 22 ++ ...ter-call-outside-effect-no-error.expect.md | 47 ++++ ...rop-setter-call-outside-effect-no-error.js | 21 ++ ...ter-used-outside-effect-no-error.expect.md | 46 ++++ ...rop-setter-used-outside-effect-no-error.js | 20 ++ ...th-global-function-call-no-error.expect.md | 43 ++++ ...fect-with-global-function-call-no-error.js | 17 ++ ...id-derived-computation-in-effect.expect.md | 73 ++++++ .../invalid-derived-computation-in-effect.js | 20 ++ ...erived-state-from-computed-props.expect.md | 72 ++++++ ...valid-derived-state-from-computed-props.js | 18 ++ ...ed-state-from-destructured-props.expect.md | 74 ++++++ ...d-derived-state-from-destructured-props.js | 19 ++ ...id-derived-computation-in-effect.expect.md | 18 +- ...r.invalid-derived-computation-in-effect.js | 4 +- 29 files changed, 1338 insertions(+), 9 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-with-global-function-call-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-with-global-function-call-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.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 1085d4c69e061..a83b22651e741 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -103,6 +103,7 @@ import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoF import {inferMutationAliasingEffects} from '../Inference/InferMutationAliasingEffects'; import {inferMutationAliasingRanges} from '../Inference/InferMutationAliasingRanges'; import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDerivedComputationsInEffects'; +import {validateNoDerivedComputationsInEffects_exp} from '../Validation/ValidateNoDerivedComputationsInEffects_exp'; import {nameAnonymousFunctions} from '../Transform/NameAnonymousFunctions'; export type CompilerPipelineValue = @@ -275,6 +276,10 @@ function runWithEnvironment( validateNoDerivedComputationsInEffects(hir); } + if (env.config.validateNoDerivedComputationsInEffects_exp) { + validateNoDerivedComputationsInEffects_exp(hir); + } + if (env.config.validateNoSetStateInEffects) { env.logErrors(validateNoSetStateInEffects(hir, env)); } 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 57567f325fd9e..5712526a34e87 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -334,6 +334,12 @@ export const EnvironmentConfigSchema = z.object({ */ validateNoDerivedComputationsInEffects: z.boolean().default(false), + /** + * Experimental: Validates that effects are not used to calculate derived data which could instead be computed + * during render. Generates a custom error message for each type of violation. + */ + validateNoDerivedComputationsInEffects_exp: 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_exp.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts new file mode 100644 index 0000000000000..9f914b1a626e9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts @@ -0,0 +1,240 @@ +/** + * 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, SourceLocation} from '..'; +import {ErrorCategory} from '../CompilerError'; +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_exp( + 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`, + description: null, + details: [ + { + kind: 'error', + loc: value.loc, + message: 'this is checked as a place above', + }, + ], + }); + return locals.get(dep.identifier.id) ?? dep.identifier.id; + }); + validateEffect( + effectFunction.loweredFunc.func, + dependencies, + errors, + ); + } + } + } + } + } + if (errors.hasAnyErrors()) { + 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({ + category: ErrorCategory.EffectDerivationsOfState, + 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, + loc, + suggestions: null, + }); + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.expect.md new file mode 100644 index 0000000000000..26e7f1066dd13 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.expect.md @@ -0,0 +1,79 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value, enabled}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + if (enabled) { + setLocalValue(value); + } else { + setLocalValue('disabled'); + } + }, [value, enabled]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test', enabled: true}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(6); + const { value, enabled } = t0; + const [localValue, setLocalValue] = useState(""); + let t1; + let t2; + if ($[0] !== enabled || $[1] !== value) { + t1 = () => { + if (enabled) { + setLocalValue(value); + } else { + setLocalValue("disabled"); + } + }; + + t2 = [value, enabled]; + $[0] = enabled; + $[1] = value; + $[2] = t1; + $[3] = t2; + } else { + t1 = $[2]; + t2 = $[3]; + } + useEffect(t1, t2); + let t3; + if ($[4] !== localValue) { + t3 =
{localValue}
; + $[4] = localValue; + $[5] = t3; + } else { + t3 = $[5]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: "test", enabled: true }], +}; + +``` + +### Eval output +(kind: ok)
test
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.js new file mode 100644 index 0000000000000..2ccd52500c773 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.js @@ -0,0 +1,21 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value, enabled}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + if (enabled) { + setLocalValue(value); + } else { + setLocalValue('disabled'); + } + }, [value, enabled]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test', enabled: true}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.expect.md new file mode 100644 index 0000000000000..542be3d242b7f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.expect.md @@ -0,0 +1,71 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({input = 'empty'}) { + const [currInput, setCurrInput] = useState(input); + const localConst = 'local const'; + + useEffect(() => { + setCurrInput(input + localConst); + }, [input, localConst]); + + return
{currInput}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{input: 'test'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +export default function Component(t0) { + const $ = _c(5); + const { input: t1 } = t0; + const input = t1 === undefined ? "empty" : t1; + const [currInput, setCurrInput] = useState(input); + let t2; + let t3; + if ($[0] !== input) { + t2 = () => { + setCurrInput(input + "local const"); + }; + t3 = [input, "local const"]; + $[0] = input; + $[1] = t2; + $[2] = t3; + } else { + t2 = $[1]; + t3 = $[2]; + } + useEffect(t2, t3); + let t4; + if ($[3] !== currInput) { + t4 =
{currInput}
; + $[3] = currInput; + $[4] = t4; + } else { + t4 = $[4]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ input: "test" }], +}; + +``` + +### Eval output +(kind: ok)
testlocal const
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.js new file mode 100644 index 0000000000000..1a0f5126e7a0b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.js @@ -0,0 +1,18 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({input = 'empty'}) { + const [currInput, setCurrInput] = useState(input); + const localConst = 'local const'; + + useEffect(() => { + setCurrInput(input + localConst); + }, [input, localConst]); + + return
{currInput}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{input: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.expect.md new file mode 100644 index 0000000000000..f62a6c0b710cc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.expect.md @@ -0,0 +1,70 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp + +import {useEffect, useState} from 'react'; + +function Component({shouldChange}) { + const [count, setCount] = useState(0); + + useEffect(() => { + if (shouldChange) { + setCount(count + 1); + } + }, [count]); + + return
{count}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp + +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(7); + const { shouldChange } = t0; + const [count, setCount] = useState(0); + let t1; + if ($[0] !== count || $[1] !== shouldChange) { + t1 = () => { + if (shouldChange) { + setCount(count + 1); + } + }; + $[0] = count; + $[1] = shouldChange; + $[2] = t1; + } else { + t1 = $[2]; + } + let t2; + if ($[3] !== count) { + t2 = [count]; + $[3] = count; + $[4] = t2; + } else { + t2 = $[4]; + } + useEffect(t1, t2); + let t3; + if ($[5] !== count) { + t3 =
{count}
; + $[5] = count; + $[6] = t3; + } else { + t3 = $[6]; + } + return t3; +} + +``` + +### 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/effect-derived-computations/derived-state-from-local-state-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.js new file mode 100644 index 0000000000000..9568e4900296e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.js @@ -0,0 +1,15 @@ +// @validateNoDerivedComputationsInEffects_exp + +import {useEffect, useState} from 'react'; + +function Component({shouldChange}) { + const [count, setCount] = useState(0); + + useEffect(() => { + if (shouldChange) { + setCount(count + 1); + } + }, [count]); + + return
{count}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.expect.md new file mode 100644 index 0000000000000..0dc43f0fba6c1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.expect.md @@ -0,0 +1,108 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({firstName}) { + const [lastName, setLastName] = useState('Doe'); + const [fullName, setFullName] = useState('John'); + + const middleName = 'D.'; + + useEffect(() => { + setFullName(firstName + ' ' + middleName + ' ' + lastName); + }, [firstName, middleName, lastName]); + + return ( +
+ setLastName(e.target.value)} /> +
{fullName}
+
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{firstName: 'John'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(12); + const { firstName } = t0; + const [lastName, setLastName] = useState("Doe"); + const [fullName, setFullName] = useState("John"); + let t1; + let t2; + if ($[0] !== firstName || $[1] !== lastName) { + t1 = () => { + setFullName(firstName + " " + "D." + " " + lastName); + }; + t2 = [firstName, "D.", lastName]; + $[0] = firstName; + $[1] = lastName; + $[2] = t1; + $[3] = t2; + } else { + t1 = $[2]; + t2 = $[3]; + } + useEffect(t1, t2); + let t3; + if ($[4] === Symbol.for("react.memo_cache_sentinel")) { + t3 = (e) => setLastName(e.target.value); + $[4] = t3; + } else { + t3 = $[4]; + } + let t4; + if ($[5] !== lastName) { + t4 = ; + $[5] = lastName; + $[6] = t4; + } else { + t4 = $[6]; + } + let t5; + if ($[7] !== fullName) { + t5 =
{fullName}
; + $[7] = fullName; + $[8] = t5; + } else { + t5 = $[8]; + } + let t6; + if ($[9] !== t4 || $[10] !== t5) { + t6 = ( +
+ {t4} + {t5} +
+ ); + $[9] = t4; + $[10] = t5; + $[11] = t6; + } else { + t6 = $[11]; + } + return t6; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ firstName: "John" }], +}; + +``` + +### Eval output +(kind: ok)
John D. Doe
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.js new file mode 100644 index 0000000000000..3090ef00412d8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.js @@ -0,0 +1,25 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({firstName}) { + const [lastName, setLastName] = useState('Doe'); + const [fullName, setFullName] = useState('John'); + + const middleName = 'D.'; + + useEffect(() => { + setFullName(firstName + ' ' + middleName + ' ' + lastName); + }, [firstName, middleName, lastName]); + + return ( +
+ setLastName(e.target.value)} /> +
{fullName}
+
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{firstName: 'John'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.expect.md new file mode 100644 index 0000000000000..4cc097bcfdaf7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.expect.md @@ -0,0 +1,71 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + setLocalValue(value); + document.title = `Value: ${value}`; + }, [value]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(5); + const { value } = t0; + const [localValue, setLocalValue] = useState(""); + let t1; + let t2; + if ($[0] !== value) { + t1 = () => { + setLocalValue(value); + document.title = `Value: ${value}`; + }; + t2 = [value]; + $[0] = value; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== localValue) { + t3 =
{localValue}
; + $[3] = localValue; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: "test" }], +}; + +``` + +### Eval output +(kind: ok)
test
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.js new file mode 100644 index 0000000000000..88c66ce1ef650 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.js @@ -0,0 +1,18 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + setLocalValue(value); + document.title = `Value: ${value}`; + }, [value]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.expect.md new file mode 100644 index 0000000000000..f267b50bd128a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.expect.md @@ -0,0 +1,86 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue}) { + const [value, setValue] = useState(null); + + function localFunction() { + console.log('local function'); + } + + useEffect(() => { + setValue(propValue); + localFunction(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(6); + const { propValue } = t0; + const [value, setValue] = useState(null); + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t1 = function localFunction() { + console.log("local function"); + }; + $[0] = t1; + } else { + t1 = $[0]; + } + const localFunction = t1; + let t2; + let t3; + if ($[1] !== propValue) { + t2 = () => { + setValue(propValue); + localFunction(); + }; + t3 = [propValue]; + $[1] = propValue; + $[2] = t2; + $[3] = t3; + } else { + t2 = $[2]; + t3 = $[3]; + } + useEffect(t2, t3); + let t4; + if ($[4] !== value) { + t4 =
{value}
; + $[4] = value; + $[5] = t4; + } else { + t4 = $[5]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ propValue: "test" }], +}; + +``` + +### Eval output +(kind: ok)
test
+logs: ['local function'] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.js new file mode 100644 index 0000000000000..1efb3177e51e5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.js @@ -0,0 +1,22 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue}) { + const [value, setValue] = useState(null); + + function localFunction() { + console.log('local function'); + } + + useEffect(() => { + setValue(propValue); + localFunction(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.expect.md new file mode 100644 index 0000000000000..1b204093b03a2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.expect.md @@ -0,0 +1,47 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({initialName}) { + const [name, setName] = useState(''); + + useEffect(() => { + setName(initialName); + }, [initialName]); + + return ( +
+ setName(e.target.value)} /> +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{initialName: 'John'}], +}; + +``` + + +## 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.derived-state-from-prop-setter-call-outside-effect-no-error.ts:8:4 + 6 | + 7 | useEffect(() => { +> 8 | setName(initialName); + | ^^^^^^^ 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 | }, [initialName]); + 10 | + 11 | return ( +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.js new file mode 100644 index 0000000000000..502402be51a3f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.js @@ -0,0 +1,21 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({initialName}) { + const [name, setName] = useState(''); + + useEffect(() => { + setName(initialName); + }, [initialName]); + + return ( +
+ setName(e.target.value)} /> +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{initialName: 'John'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.expect.md new file mode 100644 index 0000000000000..f3f45a24efea1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.expect.md @@ -0,0 +1,46 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function MockComponent({onSet}) { + return
onSet('clicked')}>Mock Component
; +} + +function Component({propValue}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + }, [propValue]); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; + +``` + + +## 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.derived-state-from-prop-setter-used-outside-effect-no-error.ts:11:4 + 9 | const [value, setValue] = useState(null); + 10 | useEffect(() => { +> 11 | setValue(propValue); + | ^^^^^^^^ 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) + 12 | }, [propValue]); + 13 | + 14 | return ; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.js new file mode 100644 index 0000000000000..d33af16ec5901 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.js @@ -0,0 +1,20 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function MockComponent({onSet}) { + return
onSet('clicked')}>Mock Component
; +} + +function Component({propValue}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + }, [propValue]); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-with-global-function-call-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-with-global-function-call-no-error.expect.md new file mode 100644 index 0000000000000..9c9b6dc368f26 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-with-global-function-call-no-error.expect.md @@ -0,0 +1,43 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + globalCall(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; + +``` + + +## 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.effect-with-global-function-call-no-error.ts:7:4 + 5 | const [value, setValue] = useState(null); + 6 | useEffect(() => { +> 7 | setValue(propValue); + | ^^^^^^^^ 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) + 8 | globalCall(); + 9 | }, [propValue]); + 10 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-with-global-function-call-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-with-global-function-call-no-error.js new file mode 100644 index 0000000000000..4cded6dcc8ef3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-with-global-function-call-no-error.js @@ -0,0 +1,17 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + globalCall(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.expect.md new file mode 100644 index 0000000000000..5622f5daa6dbd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.expect.md @@ -0,0 +1,73 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component() { + const [firstName, setFirstName] = useState('Taylor'); + const lastName = 'Swift'; + + // 🔴 Avoid: redundant state and unnecessary Effect + const [fullName, setFullName] = useState(''); + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function Component() { + const $ = _c(5); + const [firstName] = useState("Taylor"); + + const [fullName, setFullName] = useState(""); + let t0; + let t1; + if ($[0] !== firstName) { + t0 = () => { + setFullName(firstName + " " + "Swift"); + }; + t1 = [firstName, "Swift"]; + $[0] = firstName; + $[1] = t0; + $[2] = t1; + } else { + t0 = $[1]; + t1 = $[2]; + } + useEffect(t0, t1); + let t2; + if ($[3] !== fullName) { + t2 =
{fullName}
; + $[3] = fullName; + $[4] = t2; + } else { + t2 = $[4]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; + +``` + +### Eval output +(kind: ok)
Taylor Swift
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.js new file mode 100644 index 0000000000000..17779a5b4c576 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.js @@ -0,0 +1,20 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component() { + const [firstName, setFirstName] = useState('Taylor'); + const lastName = 'Swift'; + + // 🔴 Avoid: redundant state and unnecessary Effect + const [fullName, setFullName] = useState(''); + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.expect.md new file mode 100644 index 0000000000000..7c16e9ad06abb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.expect.md @@ -0,0 +1,72 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component(props) { + const [displayValue, setDisplayValue] = useState(''); + + useEffect(() => { + const computed = props.prefix + props.value + props.suffix; + setDisplayValue(computed); + }, [props.prefix, props.value, props.suffix]); + + return
{displayValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prefix: '[', value: 'test', suffix: ']'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +export default function Component(props) { + const $ = _c(7); + const [displayValue, setDisplayValue] = useState(""); + let t0; + let t1; + if ($[0] !== props.prefix || $[1] !== props.suffix || $[2] !== props.value) { + t0 = () => { + const computed = props.prefix + props.value + props.suffix; + setDisplayValue(computed); + }; + t1 = [props.prefix, props.value, props.suffix]; + $[0] = props.prefix; + $[1] = props.suffix; + $[2] = props.value; + $[3] = t0; + $[4] = t1; + } else { + t0 = $[3]; + t1 = $[4]; + } + useEffect(t0, t1); + let t2; + if ($[5] !== displayValue) { + t2 =
{displayValue}
; + $[5] = displayValue; + $[6] = t2; + } else { + t2 = $[6]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ prefix: "[", value: "test", suffix: "]" }], +}; + +``` + +### Eval output +(kind: ok)
[test]
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.js new file mode 100644 index 0000000000000..24afa944fc566 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.js @@ -0,0 +1,18 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component(props) { + const [displayValue, setDisplayValue] = useState(''); + + useEffect(() => { + const computed = props.prefix + props.value + props.suffix; + setDisplayValue(computed); + }, [props.prefix, props.value, props.suffix]); + + return
{displayValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prefix: '[', value: 'test', suffix: ']'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.expect.md new file mode 100644 index 0000000000000..869328a6e6c49 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.expect.md @@ -0,0 +1,74 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({props}) { + const [fullName, setFullName] = useState( + props.firstName + ' ' + props.lastName + ); + + useEffect(() => { + setFullName(props.firstName + ' ' + props.lastName); + }, [props.firstName, props.lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{props: {firstName: 'John', lastName: 'Doe'}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +export default function Component(t0) { + const $ = _c(6); + const { props } = t0; + const [fullName, setFullName] = useState( + props.firstName + " " + props.lastName, + ); + let t1; + let t2; + if ($[0] !== props.firstName || $[1] !== props.lastName) { + t1 = () => { + setFullName(props.firstName + " " + props.lastName); + }; + t2 = [props.firstName, props.lastName]; + $[0] = props.firstName; + $[1] = props.lastName; + $[2] = t1; + $[3] = t2; + } else { + t1 = $[2]; + t2 = $[3]; + } + useEffect(t1, t2); + let t3; + if ($[4] !== fullName) { + t3 =
{fullName}
; + $[4] = fullName; + $[5] = t3; + } else { + t3 = $[5]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ props: { firstName: "John", lastName: "Doe" } }], +}; + +``` + +### Eval output +(kind: ok)
John Doe
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.js new file mode 100644 index 0000000000000..bdfb47a2c6aad --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.js @@ -0,0 +1,19 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({props}) { + const [fullName, setFullName] = useState( + props.firstName + ' ' + props.lastName + ); + + useEffect(() => { + setFullName(props.firstName + ' ' + props.lastName); + }, [props.firstName, props.lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{props: {firstName: 'John', lastName: 'Doe'}}], +}; 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 index d97a665ae698c..ccbcf68a576b3 100644 --- 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 @@ -3,6 +3,8 @@ ```javascript // @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + function BadExample() { const [firstName, setFirstName] = useState('Taylor'); const [lastName, setLastName] = useState('Swift'); @@ -10,7 +12,7 @@ function BadExample() { // 🔴 Avoid: redundant state and unnecessary Effect const [fullName, setFullName] = useState(''); useEffect(() => { - setFullName(capitalize(firstName + ' ' + lastName)); + setFullName(firstName + ' ' + lastName); }, [firstName, lastName]); return
{fullName}
; @@ -26,14 +28,14 @@ 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)); +error.invalid-derived-computation-in-effect.ts:11:4 + 9 | const [fullName, setFullName] = useState(''); + 10 | useEffect(() => { +> 11 | setFullName(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}
; + 12 | }, [firstName, lastName]); + 13 | + 14 | 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 index d803d3c4a3a1f..0209b47ce39bb 100644 --- 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 @@ -1,4 +1,6 @@ // @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + function BadExample() { const [firstName, setFirstName] = useState('Taylor'); const [lastName, setLastName] = useState('Swift'); @@ -6,7 +8,7 @@ function BadExample() { // 🔴 Avoid: redundant state and unnecessary Effect const [fullName, setFullName] = useState(''); useEffect(() => { - setFullName(capitalize(firstName + ' ' + lastName)); + setFullName(firstName + ' ' + lastName); }, [firstName, lastName]); return
{fullName}
; From 04927c2ee799ec1015e4aefb30b4f22886396cf1 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Mon, 20 Oct 2025 17:04:25 -0700 Subject: [PATCH 2/3] [Compiler] Change ValidateNoDerivedComputationsInEffect logic to track prop and local state derived values variables and add extra tests Summary: Biggest change of the stack, we track how values prop and local state values are derived throughout the entire component. We are iterating over instructions instead of effects since some mutations can not be caught otherwise. For every derivation we track the type of value its coming from (props or local state) and also the top most relevant sources (These would be the ones that are actually named instead of promoted like t0) We propagate these relevant sources to each derivation. This allows us to catch more complex useEffects though right now we are overcapturing some more complex cases which will be refined further up the stack. This PR also adds a couple tests we will work towards fixing Test Plan: Added: ref-conditional-in-effect-no-error effect-contains-prop-function-call-no-error derived-state-from-ref-and-state-no-error --- ...idateNoDerivedComputationsInEffects_exp.ts | 460 ++++++++++++------ ...ed-state-conditionally-in-effect.expect.md | 79 --- ...derived-state-from-default-props.expect.md | 71 --- ...state-from-local-state-in-effect.expect.md | 70 --- ...-local-state-and-component-scope.expect.md | 108 ---- ...state-from-prop-with-side-effect.expect.md | 71 --- ...ect-contains-local-function-call.expect.md | 86 ---- ...ed-state-conditionally-in-effect.expect.md | 47 ++ ....derived-state-conditionally-in-effect.js} | 0 ...derived-state-from-default-props.expect.md | 44 ++ ...error.derived-state-from-default-props.js} | 0 ...state-from-local-state-in-effect.expect.md | 41 ++ ...rived-state-from-local-state-in-effect.js} | 0 ...-local-state-and-component-scope.expect.md | 51 ++ ...m-prop-local-state-and-component-scope.js} | 0 ...state-from-prop-with-side-effect.expect.md | 44 ++ ...rived-state-from-prop-with-side-effect.js} | 0 ...ect-contains-local-function-call.expect.md | 48 ++ ...or.effect-contains-local-function-call.js} | 0 ...id-derived-computation-in-effect.expect.md | 46 ++ ....invalid-derived-computation-in-effect.js} | 0 ...erived-state-from-computed-props.expect.md | 44 ++ ...alid-derived-state-from-computed-props.js} | 0 ...ed-state-from-destructured-props.expect.md | 45 ++ ...-derived-state-from-destructured-props.js} | 0 ...id-derived-computation-in-effect.expect.md | 73 --- ...erived-state-from-computed-props.expect.md | 72 --- ...ed-state-from-destructured-props.expect.md | 74 --- 28 files changed, 716 insertions(+), 858 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{derived-state-conditionally-in-effect.js => error.derived-state-conditionally-in-effect.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{derived-state-from-default-props.js => error.derived-state-from-default-props.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{derived-state-from-local-state-in-effect.js => error.derived-state-from-local-state-in-effect.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{derived-state-from-prop-local-state-and-component-scope.js => error.derived-state-from-prop-local-state-and-component-scope.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{derived-state-from-prop-with-side-effect.js => error.derived-state-from-prop-with-side-effect.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{effect-contains-local-function-call.js => error.effect-contains-local-function-call.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{invalid-derived-computation-in-effect.js => error.invalid-derived-computation-in-effect.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{invalid-derived-state-from-computed-props.js => error.invalid-derived-state-from-computed-props.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{invalid-derived-state-from-destructured-props.js => error.invalid-derived-state-from-destructured-props.js} (100%) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts index 9f914b1a626e9..f3a7892f24b98 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts @@ -5,21 +5,116 @@ * LICENSE file in the root directory of this source tree. */ -import {CompilerError, SourceLocation} from '..'; +import {CompilerError, Effect} from '..'; import {ErrorCategory} from '../CompilerError'; import { - ArrayExpression, BlockId, FunctionExpression, HIRFunction, IdentifierId, isSetStateType, isUseEffectHookType, + Place, + CallExpression, + Instruction, + isUseStateType, + BasicBlock, } from '../HIR'; -import { - eachInstructionValueOperand, - eachTerminalOperand, -} from '../HIR/visitors'; +import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors'; +import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; +import {assertExhaustive} from '../Utils/utils'; + +type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsAndState'; + +type DerivationMetadata = { + typeOfValue: TypeOfValue; + place: Place; + sourcesIds: Set; +}; + +type ValidationContext = { + readonly functions: Map; + readonly errors: CompilerError; + readonly derivationCache: DerivationCache; + readonly effects: Set; +}; + +class DerivationCache { + hasChanges: boolean = false; + cache: Map = new Map(); + + snapshot(): boolean { + const hasChanges = this.hasChanges; + this.hasChanges = false; + return hasChanges; + } + + addDerivationEntry( + derivedVar: Place, + sourcesIds: Set, + typeOfValue: TypeOfValue, + ): void { + let newValue: DerivationMetadata = { + place: derivedVar, + sourcesIds: new Set(), + typeOfValue: typeOfValue ?? 'ignored', + }; + + if (sourcesIds !== undefined) { + for (const id of sourcesIds) { + const sourcePlace = this.cache.get(id)?.place; + + if (sourcePlace === undefined) { + continue; + } + + /* + * If the identifier of the source is a promoted identifier, then + * we should set the target as the source. + */ + if ( + sourcePlace.identifier.name === null || + sourcePlace.identifier.name?.kind === 'promoted' + ) { + newValue.sourcesIds.add(derivedVar.identifier.id); + } else { + newValue.sourcesIds.add(sourcePlace.identifier.id); + } + } + } + + if (newValue.sourcesIds.size === 0) { + newValue.sourcesIds.add(derivedVar.identifier.id); + } + + const existingValue = this.cache.get(derivedVar.identifier.id); + if ( + existingValue === undefined || + !this.isDerivationEqual(existingValue, newValue) + ) { + this.cache.set(derivedVar.identifier.id, newValue); + this.hasChanges = true; + } + } + + private isDerivationEqual( + a: DerivationMetadata, + b: DerivationMetadata, + ): boolean { + if (a.typeOfValue !== b.typeOfValue) { + return false; + } + if (a.sourcesIds.size !== b.sourcesIds.size) { + return false; + } + for (const id of a.sourcesIds) { + if (!b.sourcesIds.has(id)) { + return false; + } + } + return true; + } +} /** * Validates that useEffect is not used for derived computations which could/should @@ -47,102 +142,213 @@ import { export function validateNoDerivedComputationsInEffects_exp( fn: HIRFunction, ): void { - const candidateDependencies: Map = new Map(); const functions: Map = new Map(); - const locals: Map = new Map(); - + const derivationCache = new DerivationCache(); const errors = new CompilerError(); + const effects: Set = new Set(); - 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`, - description: null, - details: [ - { - kind: 'error', - loc: value.loc, - message: 'this is checked as a place above', - }, - ], - }); - return locals.get(dep.identifier.id) ?? dep.identifier.id; - }); - validateEffect( - effectFunction.loweredFunc.func, - dependencies, - errors, - ); - } - } + const context: ValidationContext = { + functions, + errors, + derivationCache, + effects, + }; + + if (fn.fnType === 'Hook') { + for (const param of fn.params) { + if (param.kind === 'Identifier') { + context.derivationCache.cache.set(param.identifier.id, { + place: param, + sourcesIds: new Set([param.identifier.id]), + typeOfValue: 'fromProps', + }); + context.derivationCache.hasChanges = true; } } + } else if (fn.fnType === 'Component') { + const props = fn.params[0]; + if (props != null && props.kind === 'Identifier') { + context.derivationCache.cache.set(props.identifier.id, { + place: props, + sourcesIds: new Set([props.identifier.id]), + typeOfValue: 'fromProps', + }); + context.derivationCache.hasChanges = true; + } } + + do { + for (const block of fn.body.blocks.values()) { + recordPhiDerivations(block, context); + for (const instr of block.instructions) { + recordInstructionDerivations(instr, context); + } + } + } while (context.derivationCache.snapshot()); + + for (const effect of effects) { + validateEffect(effect, context); + } + if (errors.hasAnyErrors()) { throw errors; } } -function validateEffect( - effectFunction: HIRFunction, - effectDeps: Array, - errors: CompilerError, +function recordPhiDerivations( + block: BasicBlock, + context: ValidationContext, ): 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 phi of block.phis) { + let typeOfValue: TypeOfValue = 'ignored'; + let sourcesIds: Set = new Set(); + for (const operand of phi.operands.values()) { + const operandMetadata = context.derivationCache.cache.get( + operand.identifier.id, + ); + + if (operandMetadata === undefined) { + continue; + } + + typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue); + sourcesIds.add(operand.identifier.id); + } + + if (typeOfValue !== 'ignored') { + context.derivationCache.addDerivationEntry( + phi.place, + sourcesIds, + typeOfValue, + ); } } - for (const dep of effectDeps) { +} + +function joinValue( + lvalueType: TypeOfValue, + valueType: TypeOfValue, +): TypeOfValue { + if (lvalueType === 'ignored') return valueType; + if (valueType === 'ignored') return lvalueType; + if (lvalueType === valueType) return lvalueType; + return 'fromPropsAndState'; +} + +function recordInstructionDerivations( + instr: Instruction, + context: ValidationContext, +): void { + let typeOfValue: TypeOfValue = 'ignored'; + const sources: Set = new Set(); + const {lvalue, value} = instr; + if (value.kind === 'FunctionExpression') { + context.functions.set(lvalue.identifier.id, value); + for (const [, block] of value.loweredFunc.func.body.blocks) { + for (const instr of block.instructions) { + recordInstructionDerivations(instr, context); + } + } + } else if (value.kind === 'CallExpression' || value.kind === 'MethodCall') { + const callee = + value.kind === 'CallExpression' ? value.callee : value.property; if ( - effectFunction.context.find(operand => operand.identifier.id === dep) == - null + isUseEffectHookType(callee.identifier) && + value.args.length === 2 && + value.args[0].kind === 'Identifier' && + value.args[1].kind === 'Identifier' ) { - // effect dep wasn't actually used in the function - return; + const effectFunction = context.functions.get(value.args[0].identifier.id); + if (effectFunction != null) { + context.effects.add(effectFunction.loweredFunc.func); + } + } else if (isUseStateType(lvalue.identifier) && value.args.length > 0) { + const stateValueSource = value.args[0]; + if (stateValueSource.kind === 'Identifier') { + sources.add(stateValueSource.identifier.id); + } + typeOfValue = joinValue(typeOfValue, 'fromState'); } } - const seenBlocks: Set = new Set(); - const values: Map> = new Map(); - for (const dep of effectDeps) { - values.set(dep, [dep]); + for (const operand of eachInstructionOperand(instr)) { + const operandMetadata = context.derivationCache.cache.get( + operand.identifier.id, + ); + + if (operandMetadata === undefined) { + continue; + } + + typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue); + for (const id of operandMetadata.sourcesIds) { + sources.add(id); + } + } + + if (typeOfValue === 'ignored') { + return; + } + + for (const lvalue of eachInstructionLValue(instr)) { + context.derivationCache.addDerivationEntry(lvalue, sources, typeOfValue); + } + + for (const operand of eachInstructionOperand(instr)) { + switch (operand.effect) { + case Effect.Capture: + case Effect.Store: + case Effect.ConditionallyMutate: + case Effect.ConditionallyMutateIterator: + case Effect.Mutate: { + if (isMutable(instr, operand)) { + context.derivationCache.addDerivationEntry( + operand, + sources, + typeOfValue, + ); + } + break; + } + case Effect.Freeze: + case Effect.Read: { + // no-op + break; + } + case Effect.Unknown: { + CompilerError.invariant(false, { + reason: 'Unexpected unknown effect', + description: null, + details: [ + { + kind: 'error', + loc: operand.loc, + message: 'Unexpected unknown effect', + }, + ], + }); + } + default: { + assertExhaustive( + operand.effect, + `Unexpected effect kind \`${operand.effect}\``, + ); + } + } } +} + +function validateEffect( + effectFunction: HIRFunction, + context: ValidationContext, +): void { + const seenBlocks: Set = new Set(); + + const effectDerivedSetStateCalls: Array<{ + value: CallExpression; + sourceIds: Set; + }> = []; - const setStateLocations: Array = []; for (const block of effectFunction.body.blocks.values()) { for (const pred of block.preds) { if (!seenBlocks.has(pred)) { @@ -150,90 +356,36 @@ function validateEffect( 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; + if ( + instr.value.kind === 'CallExpression' && + isSetStateType(instr.value.callee.identifier) && + instr.value.args.length === 1 && + instr.value.args[0].kind === 'Identifier' + ) { + const argMetadata = context.derivationCache.cache.get( + instr.value.args[0].identifier.id, + ); + + if (argMetadata !== undefined) { + effectDerivedSetStateCalls.push({ + value: instr.value, + sourceIds: argMetadata.sourcesIds, + }); } } } - 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({ + for (const derivedSetStateCall of effectDerivedSetStateCalls) { + context.errors.push({ category: ErrorCategory.EffectDerivationsOfState, 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, - loc, + loc: derivedSetStateCall.value.callee.loc, suggestions: null, }); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.expect.md deleted file mode 100644 index 26e7f1066dd13..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.expect.md +++ /dev/null @@ -1,79 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp -import {useEffect, useState} from 'react'; - -function Component({value, enabled}) { - const [localValue, setLocalValue] = useState(''); - - useEffect(() => { - if (enabled) { - setLocalValue(value); - } else { - setLocalValue('disabled'); - } - }, [value, enabled]); - - return
{localValue}
; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{value: 'test', enabled: true}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp -import { useEffect, useState } from "react"; - -function Component(t0) { - const $ = _c(6); - const { value, enabled } = t0; - const [localValue, setLocalValue] = useState(""); - let t1; - let t2; - if ($[0] !== enabled || $[1] !== value) { - t1 = () => { - if (enabled) { - setLocalValue(value); - } else { - setLocalValue("disabled"); - } - }; - - t2 = [value, enabled]; - $[0] = enabled; - $[1] = value; - $[2] = t1; - $[3] = t2; - } else { - t1 = $[2]; - t2 = $[3]; - } - useEffect(t1, t2); - let t3; - if ($[4] !== localValue) { - t3 =
{localValue}
; - $[4] = localValue; - $[5] = t3; - } else { - t3 = $[5]; - } - return t3; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ value: "test", enabled: true }], -}; - -``` - -### Eval output -(kind: ok)
test
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.expect.md deleted file mode 100644 index 542be3d242b7f..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.expect.md +++ /dev/null @@ -1,71 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp -import {useEffect, useState} from 'react'; - -export default function Component({input = 'empty'}) { - const [currInput, setCurrInput] = useState(input); - const localConst = 'local const'; - - useEffect(() => { - setCurrInput(input + localConst); - }, [input, localConst]); - - return
{currInput}
; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{input: 'test'}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp -import { useEffect, useState } from "react"; - -export default function Component(t0) { - const $ = _c(5); - const { input: t1 } = t0; - const input = t1 === undefined ? "empty" : t1; - const [currInput, setCurrInput] = useState(input); - let t2; - let t3; - if ($[0] !== input) { - t2 = () => { - setCurrInput(input + "local const"); - }; - t3 = [input, "local const"]; - $[0] = input; - $[1] = t2; - $[2] = t3; - } else { - t2 = $[1]; - t3 = $[2]; - } - useEffect(t2, t3); - let t4; - if ($[3] !== currInput) { - t4 =
{currInput}
; - $[3] = currInput; - $[4] = t4; - } else { - t4 = $[4]; - } - return t4; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ input: "test" }], -}; - -``` - -### Eval output -(kind: ok)
testlocal const
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.expect.md deleted file mode 100644 index f62a6c0b710cc..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.expect.md +++ /dev/null @@ -1,70 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp - -import {useEffect, useState} from 'react'; - -function Component({shouldChange}) { - const [count, setCount] = useState(0); - - useEffect(() => { - if (shouldChange) { - setCount(count + 1); - } - }, [count]); - - return
{count}
; -} - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp - -import { useEffect, useState } from "react"; - -function Component(t0) { - const $ = _c(7); - const { shouldChange } = t0; - const [count, setCount] = useState(0); - let t1; - if ($[0] !== count || $[1] !== shouldChange) { - t1 = () => { - if (shouldChange) { - setCount(count + 1); - } - }; - $[0] = count; - $[1] = shouldChange; - $[2] = t1; - } else { - t1 = $[2]; - } - let t2; - if ($[3] !== count) { - t2 = [count]; - $[3] = count; - $[4] = t2; - } else { - t2 = $[4]; - } - useEffect(t1, t2); - let t3; - if ($[5] !== count) { - t3 =
{count}
; - $[5] = count; - $[6] = t3; - } else { - t3 = $[6]; - } - return t3; -} - -``` - -### 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/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.expect.md deleted file mode 100644 index 0dc43f0fba6c1..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.expect.md +++ /dev/null @@ -1,108 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp -import {useEffect, useState} from 'react'; - -function Component({firstName}) { - const [lastName, setLastName] = useState('Doe'); - const [fullName, setFullName] = useState('John'); - - const middleName = 'D.'; - - useEffect(() => { - setFullName(firstName + ' ' + middleName + ' ' + lastName); - }, [firstName, middleName, lastName]); - - return ( -
- setLastName(e.target.value)} /> -
{fullName}
-
- ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{firstName: 'John'}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp -import { useEffect, useState } from "react"; - -function Component(t0) { - const $ = _c(12); - const { firstName } = t0; - const [lastName, setLastName] = useState("Doe"); - const [fullName, setFullName] = useState("John"); - let t1; - let t2; - if ($[0] !== firstName || $[1] !== lastName) { - t1 = () => { - setFullName(firstName + " " + "D." + " " + lastName); - }; - t2 = [firstName, "D.", lastName]; - $[0] = firstName; - $[1] = lastName; - $[2] = t1; - $[3] = t2; - } else { - t1 = $[2]; - t2 = $[3]; - } - useEffect(t1, t2); - let t3; - if ($[4] === Symbol.for("react.memo_cache_sentinel")) { - t3 = (e) => setLastName(e.target.value); - $[4] = t3; - } else { - t3 = $[4]; - } - let t4; - if ($[5] !== lastName) { - t4 = ; - $[5] = lastName; - $[6] = t4; - } else { - t4 = $[6]; - } - let t5; - if ($[7] !== fullName) { - t5 =
{fullName}
; - $[7] = fullName; - $[8] = t5; - } else { - t5 = $[8]; - } - let t6; - if ($[9] !== t4 || $[10] !== t5) { - t6 = ( -
- {t4} - {t5} -
- ); - $[9] = t4; - $[10] = t5; - $[11] = t6; - } else { - t6 = $[11]; - } - return t6; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ firstName: "John" }], -}; - -``` - -### Eval output -(kind: ok)
John D. Doe
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.expect.md deleted file mode 100644 index 4cc097bcfdaf7..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.expect.md +++ /dev/null @@ -1,71 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp -import {useEffect, useState} from 'react'; - -function Component({value}) { - const [localValue, setLocalValue] = useState(''); - - useEffect(() => { - setLocalValue(value); - document.title = `Value: ${value}`; - }, [value]); - - return
{localValue}
; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{value: 'test'}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp -import { useEffect, useState } from "react"; - -function Component(t0) { - const $ = _c(5); - const { value } = t0; - const [localValue, setLocalValue] = useState(""); - let t1; - let t2; - if ($[0] !== value) { - t1 = () => { - setLocalValue(value); - document.title = `Value: ${value}`; - }; - t2 = [value]; - $[0] = value; - $[1] = t1; - $[2] = t2; - } else { - t1 = $[1]; - t2 = $[2]; - } - useEffect(t1, t2); - let t3; - if ($[3] !== localValue) { - t3 =
{localValue}
; - $[3] = localValue; - $[4] = t3; - } else { - t3 = $[4]; - } - return t3; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ value: "test" }], -}; - -``` - -### Eval output -(kind: ok)
test
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.expect.md deleted file mode 100644 index f267b50bd128a..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.expect.md +++ /dev/null @@ -1,86 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp -import {useEffect, useState} from 'react'; - -function Component({propValue}) { - const [value, setValue] = useState(null); - - function localFunction() { - console.log('local function'); - } - - useEffect(() => { - setValue(propValue); - localFunction(); - }, [propValue]); - - return
{value}
; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{propValue: 'test'}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp -import { useEffect, useState } from "react"; - -function Component(t0) { - const $ = _c(6); - const { propValue } = t0; - const [value, setValue] = useState(null); - let t1; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t1 = function localFunction() { - console.log("local function"); - }; - $[0] = t1; - } else { - t1 = $[0]; - } - const localFunction = t1; - let t2; - let t3; - if ($[1] !== propValue) { - t2 = () => { - setValue(propValue); - localFunction(); - }; - t3 = [propValue]; - $[1] = propValue; - $[2] = t2; - $[3] = t3; - } else { - t2 = $[2]; - t3 = $[3]; - } - useEffect(t2, t3); - let t4; - if ($[4] !== value) { - t4 =
{value}
; - $[4] = value; - $[5] = t4; - } else { - t4 = $[5]; - } - return t4; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ propValue: "test" }], -}; - -``` - -### Eval output -(kind: ok)
test
-logs: ['local function'] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md new file mode 100644 index 0000000000000..8d378e4a398a8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md @@ -0,0 +1,47 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value, enabled}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + if (enabled) { + setLocalValue(value); + } else { + setLocalValue('disabled'); + } + }, [value, enabled]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test', enabled: true}], +}; + +``` + + +## 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.derived-state-conditionally-in-effect.ts:9:6 + 7 | useEffect(() => { + 8 | if (enabled) { +> 9 | setLocalValue(value); + | ^^^^^^^^^^^^^ 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 | } else { + 11 | setLocalValue('disabled'); + 12 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-conditionally-in-effect.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md new file mode 100644 index 0000000000000..8e1ec8ae5febd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md @@ -0,0 +1,44 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({input = 'empty'}) { + const [currInput, setCurrInput] = useState(input); + const localConst = 'local const'; + + useEffect(() => { + setCurrInput(input + localConst); + }, [input, localConst]); + + return
{currInput}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{input: 'test'}], +}; + +``` + + +## 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.derived-state-from-default-props.ts:9:4 + 7 | + 8 | useEffect(() => { +> 9 | setCurrInput(input + localConst); + | ^^^^^^^^^^^^ 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 | }, [input, localConst]); + 11 | + 12 | return
{currInput}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-default-props.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.expect.md new file mode 100644 index 0000000000000..2723d7a79992d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.expect.md @@ -0,0 +1,41 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp + +import {useEffect, useState} from 'react'; + +function Component({shouldChange}) { + const [count, setCount] = useState(0); + + useEffect(() => { + if (shouldChange) { + setCount(count + 1); + } + }, [count]); + + return
{count}
; +} + +``` + + +## 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.derived-state-from-local-state-in-effect.ts:10:6 + 8 | useEffect(() => { + 9 | if (shouldChange) { +> 10 | setCount(count + 1); + | ^^^^^^^^ 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) + 11 | } + 12 | }, [count]); + 13 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-local-state-in-effect.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.expect.md new file mode 100644 index 0000000000000..48c173028ecf0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.expect.md @@ -0,0 +1,51 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({firstName}) { + const [lastName, setLastName] = useState('Doe'); + const [fullName, setFullName] = useState('John'); + + const middleName = 'D.'; + + useEffect(() => { + setFullName(firstName + ' ' + middleName + ' ' + lastName); + }, [firstName, middleName, lastName]); + + return ( +
+ setLastName(e.target.value)} /> +
{fullName}
+
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{firstName: 'John'}], +}; + +``` + + +## 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.derived-state-from-prop-local-state-and-component-scope.ts:11:4 + 9 | + 10 | useEffect(() => { +> 11 | setFullName(firstName + ' ' + middleName + ' ' + 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) + 12 | }, [firstName, middleName, lastName]); + 13 | + 14 | return ( +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-local-state-and-component-scope.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md new file mode 100644 index 0000000000000..2577db54bef3b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md @@ -0,0 +1,44 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + setLocalValue(value); + document.title = `Value: ${value}`; + }, [value]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test'}], +}; + +``` + + +## 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.derived-state-from-prop-with-side-effect.ts:8:4 + 6 | + 7 | useEffect(() => { +> 8 | setLocalValue(value); + | ^^^^^^^^^^^^^ 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 | document.title = `Value: ${value}`; + 10 | }, [value]); + 11 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-with-side-effect.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md new file mode 100644 index 0000000000000..d389851d7faea --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md @@ -0,0 +1,48 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue}) { + const [value, setValue] = useState(null); + + function localFunction() { + console.log('local function'); + } + + useEffect(() => { + setValue(propValue); + localFunction(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; + +``` + + +## 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.effect-contains-local-function-call.ts:12:4 + 10 | + 11 | useEffect(() => { +> 12 | setValue(propValue); + | ^^^^^^^^ 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) + 13 | localFunction(); + 14 | }, [propValue]); + 15 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-local-function-call.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md new file mode 100644 index 0000000000000..f06f72692765f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md @@ -0,0 +1,46 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component() { + const [firstName, setFirstName] = useState('Taylor'); + const lastName = 'Swift'; + + // 🔴 Avoid: redundant state and unnecessary Effect + const [fullName, setFullName] = useState(''); + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; + +``` + + +## 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:11:4 + 9 | const [fullName, setFullName] = useState(''); + 10 | useEffect(() => { +> 11 | setFullName(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) + 12 | }, [firstName, lastName]); + 13 | + 14 | return
{fullName}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md new file mode 100644 index 0000000000000..4f0469b7912aa --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md @@ -0,0 +1,44 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component(props) { + const [displayValue, setDisplayValue] = useState(''); + + useEffect(() => { + const computed = props.prefix + props.value + props.suffix; + setDisplayValue(computed); + }, [props.prefix, props.value, props.suffix]); + + return
{displayValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prefix: '[', value: 'test', suffix: ']'}], +}; + +``` + + +## 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-state-from-computed-props.ts:9:4 + 7 | useEffect(() => { + 8 | const computed = props.prefix + props.value + props.suffix; +> 9 | setDisplayValue(computed); + | ^^^^^^^^^^^^^^^ 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 | }, [props.prefix, props.value, props.suffix]); + 11 | + 12 | return
{displayValue}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md new file mode 100644 index 0000000000000..309e9f73fa23f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md @@ -0,0 +1,45 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({props}) { + const [fullName, setFullName] = useState( + props.firstName + ' ' + props.lastName + ); + + useEffect(() => { + setFullName(props.firstName + ' ' + props.lastName); + }, [props.firstName, props.lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{props: {firstName: 'John', lastName: 'Doe'}}], +}; + +``` + + +## 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-state-from-destructured-props.ts:10:4 + 8 | + 9 | useEffect(() => { +> 10 | setFullName(props.firstName + ' ' + props.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) + 11 | }, [props.firstName, props.lastName]); + 12 | + 13 | return
{fullName}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.expect.md deleted file mode 100644 index 5622f5daa6dbd..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-computation-in-effect.expect.md +++ /dev/null @@ -1,73 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp -import {useEffect, useState} from 'react'; - -function Component() { - const [firstName, setFirstName] = useState('Taylor'); - const lastName = 'Swift'; - - // 🔴 Avoid: redundant state and unnecessary Effect - const [fullName, setFullName] = useState(''); - useEffect(() => { - setFullName(firstName + ' ' + lastName); - }, [firstName, lastName]); - - return
{fullName}
; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp -import { useEffect, useState } from "react"; - -function Component() { - const $ = _c(5); - const [firstName] = useState("Taylor"); - - const [fullName, setFullName] = useState(""); - let t0; - let t1; - if ($[0] !== firstName) { - t0 = () => { - setFullName(firstName + " " + "Swift"); - }; - t1 = [firstName, "Swift"]; - $[0] = firstName; - $[1] = t0; - $[2] = t1; - } else { - t0 = $[1]; - t1 = $[2]; - } - useEffect(t0, t1); - let t2; - if ($[3] !== fullName) { - t2 =
{fullName}
; - $[3] = fullName; - $[4] = t2; - } else { - t2 = $[4]; - } - return t2; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [], -}; - -``` - -### Eval output -(kind: ok)
Taylor Swift
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.expect.md deleted file mode 100644 index 7c16e9ad06abb..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-computed-props.expect.md +++ /dev/null @@ -1,72 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp -import {useEffect, useState} from 'react'; - -export default function Component(props) { - const [displayValue, setDisplayValue] = useState(''); - - useEffect(() => { - const computed = props.prefix + props.value + props.suffix; - setDisplayValue(computed); - }, [props.prefix, props.value, props.suffix]); - - return
{displayValue}
; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{prefix: '[', value: 'test', suffix: ']'}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp -import { useEffect, useState } from "react"; - -export default function Component(props) { - const $ = _c(7); - const [displayValue, setDisplayValue] = useState(""); - let t0; - let t1; - if ($[0] !== props.prefix || $[1] !== props.suffix || $[2] !== props.value) { - t0 = () => { - const computed = props.prefix + props.value + props.suffix; - setDisplayValue(computed); - }; - t1 = [props.prefix, props.value, props.suffix]; - $[0] = props.prefix; - $[1] = props.suffix; - $[2] = props.value; - $[3] = t0; - $[4] = t1; - } else { - t0 = $[3]; - t1 = $[4]; - } - useEffect(t0, t1); - let t2; - if ($[5] !== displayValue) { - t2 =
{displayValue}
; - $[5] = displayValue; - $[6] = t2; - } else { - t2 = $[6]; - } - return t2; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ prefix: "[", value: "test", suffix: "]" }], -}; - -``` - -### Eval output -(kind: ok)
[test]
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.expect.md deleted file mode 100644 index 869328a6e6c49..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/invalid-derived-state-from-destructured-props.expect.md +++ /dev/null @@ -1,74 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp -import {useEffect, useState} from 'react'; - -export default function Component({props}) { - const [fullName, setFullName] = useState( - props.firstName + ' ' + props.lastName - ); - - useEffect(() => { - setFullName(props.firstName + ' ' + props.lastName); - }, [props.firstName, props.lastName]); - - return
{fullName}
; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{props: {firstName: 'John', lastName: 'Doe'}}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp -import { useEffect, useState } from "react"; - -export default function Component(t0) { - const $ = _c(6); - const { props } = t0; - const [fullName, setFullName] = useState( - props.firstName + " " + props.lastName, - ); - let t1; - let t2; - if ($[0] !== props.firstName || $[1] !== props.lastName) { - t1 = () => { - setFullName(props.firstName + " " + props.lastName); - }; - t2 = [props.firstName, props.lastName]; - $[0] = props.firstName; - $[1] = props.lastName; - $[2] = t1; - $[3] = t2; - } else { - t1 = $[2]; - t2 = $[3]; - } - useEffect(t1, t2); - let t3; - if ($[4] !== fullName) { - t3 =
{fullName}
; - $[4] = fullName; - $[5] = t3; - } else { - t3 = $[5]; - } - return t3; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ props: { firstName: "John", lastName: "Doe" } }], -}; - -``` - -### Eval output -(kind: ok)
John Doe
\ No newline at end of file From 648c5045c4ff0c747d2d7314a532fd8547c3c9a7 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Tue, 21 Oct 2025 14:42:53 -0700 Subject: [PATCH 3/3] [Compiler] Don't throw calculate in render when there is a ref in the effect Summary: Using refs in an effect signify we are synchronizing with external state so to avoid overcapturing we just bail when we encounter one --- ...idateNoDerivedComputationsInEffects_exp.ts | 19 +++++ ...tate-from-ref-and-state-no-error.expect.md | 73 +++++++++++++++++ ...rived-state-from-ref-and-state-no-error.js | 19 +++++ ...ains-prop-function-call-no-error.expect.md | 75 +++++++++++++++++ ...ct-contains-prop-function-call-no-error.js | 17 ++++ ...f-conditional-in-effect-no-error.expect.md | 82 +++++++++++++++++++ .../ref-conditional-in-effect-no-error.js | 23 ++++++ 7 files changed, 308 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts index f3a7892f24b98..30aed7632ef30 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts @@ -19,6 +19,7 @@ import { Instruction, isUseStateType, BasicBlock, + isUseRefType, } from '../HIR'; import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors'; import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; @@ -358,6 +359,11 @@ function validateEffect( } for (const instr of block.instructions) { + // Early return if any instruction is deriving a value from a ref + if (isUseRefType(instr.lvalue.identifier)) { + return; + } + if ( instr.value.kind === 'CallExpression' && isSetStateType(instr.value.callee.identifier) && @@ -374,6 +380,19 @@ function validateEffect( sourceIds: argMetadata.sourcesIds, }); } + } else if (instr.value.kind === 'CallExpression') { + const calleeMetadata = context.derivationCache.cache.get( + instr.value.callee.identifier.id, + ); + + if ( + calleeMetadata !== undefined && + (calleeMetadata.typeOfValue === 'fromProps' || + calleeMetadata.typeOfValue === 'fromPropsAndState') + ) { + // If the callee is a prop we can't confidently say that it should be derived in render + return; + } } } seenBlocks.add(block.id); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.expect.md new file mode 100644 index 0000000000000..4d0b6663e325a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.expect.md @@ -0,0 +1,73 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState, useRef} from 'react'; + +export default function Component({test}) { + const [local, setLocal] = useState(''); + + const myRef = useRef(null); + + useEffect(() => { + setLocal(myRef.current + test); + }, [test]); + + return <>{local}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{test: 'testString'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState, useRef } from "react"; + +export default function Component(t0) { + const $ = _c(5); + const { test } = t0; + const [local, setLocal] = useState(""); + + const myRef = useRef(null); + let t1; + let t2; + if ($[0] !== test) { + t1 = () => { + setLocal(myRef.current + test); + }; + t2 = [test]; + $[0] = test; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== local) { + t3 = <>{local}; + $[3] = local; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ test: "testString" }], +}; + +``` + +### Eval output +(kind: ok) nulltestString \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.js new file mode 100644 index 0000000000000..6b24f73ac7400 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.js @@ -0,0 +1,19 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState, useRef} from 'react'; + +export default function Component({test}) { + const [local, setLocal] = useState(''); + + const myRef = useRef(null); + + useEffect(() => { + setLocal(myRef.current + test); + }, [test]); + + return <>{local}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{test: 'testString'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.expect.md new file mode 100644 index 0000000000000..c83ea552a6a78 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.expect.md @@ -0,0 +1,75 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue, onChange}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + onChange(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test', onChange: () => {}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(7); + const { propValue, onChange } = t0; + const [value, setValue] = useState(null); + let t1; + if ($[0] !== onChange || $[1] !== propValue) { + t1 = () => { + setValue(propValue); + onChange(); + }; + $[0] = onChange; + $[1] = propValue; + $[2] = t1; + } else { + t1 = $[2]; + } + let t2; + if ($[3] !== propValue) { + t2 = [propValue]; + $[3] = propValue; + $[4] = t2; + } else { + t2 = $[4]; + } + useEffect(t1, t2); + let t3; + if ($[5] !== value) { + t3 =
{value}
; + $[5] = value; + $[6] = t3; + } else { + t3 = $[6]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ propValue: "test", onChange: () => {} }], +}; + +``` + +### Eval output +(kind: ok)
test
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.js new file mode 100644 index 0000000000000..512df7cb36ebd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.js @@ -0,0 +1,17 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue, onChange}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + onChange(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test', onChange: () => {}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.expect.md new file mode 100644 index 0000000000000..365ee1fef4548 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.expect.md @@ -0,0 +1,82 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState, useRef} from 'react'; + +export default function Component({test}) { + const [local, setLocal] = useState(0); + + const myRef = useRef(null); + + useEffect(() => { + if (myRef.current) { + setLocal(test); + } else { + setLocal(test + test); + } + }, [test]); + + return <>{local}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{test: 4}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState, useRef } from "react"; + +export default function Component(t0) { + const $ = _c(5); + const { test } = t0; + const [local, setLocal] = useState(0); + + const myRef = useRef(null); + let t1; + let t2; + if ($[0] !== test) { + t1 = () => { + if (myRef.current) { + setLocal(test); + } else { + setLocal(test + test); + } + }; + + t2 = [test]; + $[0] = test; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== local) { + t3 = <>{local}; + $[3] = local; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ test: 4 }], +}; + +``` + +### Eval output +(kind: ok) 8 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.js new file mode 100644 index 0000000000000..ee59ccb78f037 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.js @@ -0,0 +1,23 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState, useRef} from 'react'; + +export default function Component({test}) { + const [local, setLocal] = useState(0); + + const myRef = useRef(null); + + useEffect(() => { + if (myRef.current) { + setLocal(test); + } else { + setLocal(test + test); + } + }, [test]); + + return <>{local}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{test: 4}], +};