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 5c9462fd55106..af5927548af52 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 @@ -22,6 +22,7 @@ import { BasicBlock, isUseRefType, SourceLocation, + ArrayExpression, } from '../HIR'; import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors'; import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; @@ -36,11 +37,17 @@ type DerivationMetadata = { isStateSource: boolean; }; +type EffectMetadata = { + effect: HIRFunction; + dependencies: ArrayExpression; +}; + type ValidationContext = { readonly functions: Map; + readonly candidateDependencies: Map; readonly errors: CompilerError; readonly derivationCache: DerivationCache; - readonly effects: Set; + readonly effectsCache: Map; readonly setStateLoads: Map; readonly setStateUsages: Map>; }; @@ -175,18 +182,20 @@ export function validateNoDerivedComputationsInEffects_exp( fn: HIRFunction, ): Result { const functions: Map = new Map(); + const candidateDependencies: Map = new Map(); const derivationCache = new DerivationCache(); const errors = new CompilerError(); - const effects: Set = new Set(); + const effectsCache: Map = new Map(); const setStateLoads: Map = new Map(); const setStateUsages: Map> = new Map(); const context: ValidationContext = { functions, + candidateDependencies, errors, derivationCache, - effects, + effectsCache, setStateLoads, setStateUsages, }; @@ -229,8 +238,8 @@ export function validateNoDerivedComputationsInEffects_exp( isFirstPass = false; } while (context.derivationCache.snapshot()); - for (const effect of effects) { - validateEffect(effect, context); + for (const [, effect] of effectsCache) { + validateEffect(effect.effect, effect.dependencies, context); } return errors.asResult(); @@ -354,8 +363,14 @@ function recordInstructionDerivations( value.args[1].kind === 'Identifier' ) { const effectFunction = context.functions.get(value.args[0].identifier.id); - if (effectFunction != null) { - context.effects.add(effectFunction.loweredFunc.func); + const deps = context.candidateDependencies.get( + value.args[1].identifier.id, + ); + if (effectFunction != null && deps != null) { + context.effectsCache.set(value.args[0].identifier.id, { + effect: effectFunction.loweredFunc.func, + dependencies: deps, + }); } } else if (isUseStateType(lvalue.identifier) && value.args.length > 0) { typeOfValue = 'fromState'; @@ -367,6 +382,8 @@ function recordInstructionDerivations( ); return; } + } else if (value.kind === 'ArrayExpression') { + context.candidateDependencies.set(lvalue.identifier.id, value); } for (const operand of eachInstructionOperand(instr)) { @@ -596,6 +613,7 @@ function getFnLocalDeps( function validateEffect( effectFunction: HIRFunction, + dependencies: ArrayExpression, context: ValidationContext, ): void { const seenBlocks: Set = new Set(); @@ -612,6 +630,16 @@ function validateEffect( Set > = new Map(); + // Consider setStates in the effect's dependency array as being part of effectSetStateUsages + for (const dep of dependencies.elements) { + if (dep.kind === 'Identifier') { + const root = getRootSetState(dep.identifier.id, context.setStateLoads); + if (root !== null) { + effectSetStateUsages.set(root, new Set([dep.loc])); + } + } + } + let cleanUpFunctionDeps: Set | undefined; const globals: Set = new Set(); @@ -662,6 +690,18 @@ function validateEffect( instr.value.args.length === 1 && instr.value.args[0].kind === 'Identifier' ) { + const calleeMetadata = context.derivationCache.cache.get( + instr.value.callee.identifier.id, + ); + + /* + * If the setState comes from a source other than local state skip + * since the fix is not to calculate in render + */ + if (calleeMetadata?.typeOfValue != 'fromState') { + continue; + } + const argMetadata = context.derivationCache.cache.get( instr.value.args[0].identifier.id, ); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.expect.md new file mode 100644 index 0000000000000..6a3593a3b27c3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.expect.md @@ -0,0 +1,63 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +function Component({prop}) { + const [s, setS] = useState(0); + useEffect(() => { + setS(prop); + }, [prop, setS]); + + return
{prop}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +function Component(t0) { + const $ = _c(5); + const { prop } = t0; + const [, setS] = useState(0); + let t1; + let t2; + if ($[0] !== prop) { + t1 = () => { + setS(prop); + }; + t2 = [prop, setS]; + $[0] = prop; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== prop) { + t3 =
{prop}
; + $[3] = prop; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} + +``` + +## Logs + +``` +{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nProps: [prop]\n\nData Flow Tree:\n└── prop (Prop)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":6,"column":4,"index":150},"end":{"line":6,"column":8,"index":154},"filename":"effect-used-in-dep-array-still-errors.ts","identifierName":"setS"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":10,"column":1,"index":212},"filename":"effect-used-in-dep-array-still-errors.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### 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/effect-used-in-dep-array-still-errors.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.js new file mode 100644 index 0000000000000..1df99a191dcfe --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.js @@ -0,0 +1,10 @@ +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +function Component({prop}) { + const [s, setS] = useState(0); + useEffect(() => { + setS(prop); + }, [prop, setS]); + + return
{prop}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.expect.md new file mode 100644 index 0000000000000..f23f51d6cb8bb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly + +function Component({setParentState, prop}) { + useEffect(() => { + setParentState(prop); + }, [prop]); + + return
{prop}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly + +function Component(t0) { + const $ = _c(7); + const { setParentState, prop } = t0; + let t1; + if ($[0] !== prop || $[1] !== setParentState) { + t1 = () => { + setParentState(prop); + }; + $[0] = prop; + $[1] = setParentState; + $[2] = t1; + } else { + t1 = $[2]; + } + let t2; + if ($[3] !== prop) { + t2 = [prop]; + $[3] = prop; + $[4] = t2; + } else { + t2 = $[4]; + } + useEffect(t1, t2); + let t3; + if ($[5] !== prop) { + t3 =
{prop}
; + $[5] = prop; + $[6] = t3; + } else { + t3 = $[6]; + } + return t3; +} + +``` + +## Logs + +``` +{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":105},"end":{"line":9,"column":1,"index":240},"filename":"from-props-setstate-in-effect-no-error.ts"},"fnName":"Component","memoSlots":7,"memoBlocks":3,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### 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/from-props-setstate-in-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.js new file mode 100644 index 0000000000000..4075975b32549 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.js @@ -0,0 +1,9 @@ +// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly + +function Component({setParentState, prop}) { + useEffect(() => { + setParentState(prop); + }, [prop]); + + return
{prop}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md index 87cf7722da353..602fb9fff3a6c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md @@ -64,7 +64,6 @@ function Component(t0) { ## Logs ``` -{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nState: [second]\n\nData Flow Tree:\n└── second (State)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":14,"column":4,"index":443},"end":{"line":14,"column":8,"index":447},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts","identifierName":"setS"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null} {"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":18,"column":1,"index":500},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0} ```