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 a32823cb5f8..b29d65e6aa5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -672,9 +672,14 @@ export const EnvironmentConfigSchema = z.object({ validateNoDynamicallyCreatedComponentsOrHooks: z.boolean().default(false), /** - * When enabled, allows setState calls in effects when the value being set is - * derived from a ref. This is useful for patterns where initial layout measurements - * from refs need to be stored in state during mount. + * When enabled, allows setState calls in effects based on valid patterns involving refs: + * - Allow setState where the value being set is derived from a ref. This is useful where + * state needs to take into account layer information, and a layout effect reads layout + * data from a ref and sets state. + * - Allow conditionally calling setState after manually comparing previous/new values + * for changes via a ref. Relying on effect deps is insufficient for non-primitive values, + * so a ref is generally required to manually track previous values and compare prev/next + * for meaningful changes before setting state. */ enableAllowSetStateFromRefsInEffects: z.boolean().default(true), diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts index 656c9a67148..795969b3f3b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts @@ -21,13 +21,17 @@ import { isUseRefType, isRefValueType, Place, + Effect, + BlockId, } from '../HIR'; import { eachInstructionLValue, eachInstructionValueOperand, } from '../HIR/visitors'; +import {createControlDominators} from '../Inference/ControlDominators'; +import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; import {Result} from '../Utils/Result'; -import {Iterable_some} from '../Utils/utils'; +import {assertExhaustive, Iterable_some} from '../Utils/utils'; /** * Validates against calling setState in the body of an effect (useEffect and friends), @@ -140,6 +144,8 @@ function getSetStateCall( setStateFunctions: Map, env: Environment, ): Place | null { + const enableAllowSetStateFromRefsInEffects = + env.config.enableAllowSetStateFromRefsInEffects; const refDerivedValues: Set = new Set(); const isDerivedFromRef = (place: Place): boolean => { @@ -150,9 +156,38 @@ function getSetStateCall( ); }; + const isRefControlledBlock: (id: BlockId) => boolean = + enableAllowSetStateFromRefsInEffects + ? createControlDominators(fn, place => isDerivedFromRef(place)) + : (): boolean => false; + for (const [, block] of fn.body.blocks) { + if (enableAllowSetStateFromRefsInEffects) { + for (const phi of block.phis) { + if (isDerivedFromRef(phi.place)) { + continue; + } + let isPhiDerivedFromRef = false; + for (const [, operand] of phi.operands) { + if (isDerivedFromRef(operand)) { + isPhiDerivedFromRef = true; + break; + } + } + if (isPhiDerivedFromRef) { + refDerivedValues.add(phi.place.identifier.id); + } else { + for (const [pred] of phi.operands) { + if (isRefControlledBlock(pred)) { + refDerivedValues.add(phi.place.identifier.id); + break; + } + } + } + } + } for (const instr of block.instructions) { - if (env.config.enableAllowSetStateFromRefsInEffects) { + if (enableAllowSetStateFromRefsInEffects) { const hasRefOperand = Iterable_some( eachInstructionValueOperand(instr.value), isDerivedFromRef, @@ -162,6 +197,46 @@ function getSetStateCall( for (const lvalue of eachInstructionLValue(instr)) { refDerivedValues.add(lvalue.identifier.id); } + // Ref-derived values can also propagate through mutation + for (const operand of eachInstructionValueOperand(instr.value)) { + switch (operand.effect) { + case Effect.Capture: + case Effect.Store: + case Effect.ConditionallyMutate: + case Effect.ConditionallyMutateIterator: + case Effect.Mutate: { + if (isMutable(instr, operand)) { + refDerivedValues.add(operand.identifier.id); + } + 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: null, + }, + ], + suggestions: null, + }); + } + default: { + assertExhaustive( + operand.effect, + `Unexpected effect kind \`${operand.effect}\``, + ); + } + } + } } if ( @@ -203,7 +278,7 @@ function getSetStateCall( isSetStateType(callee.identifier) || setStateFunctions.has(callee.identifier.id) ) { - if (env.config.enableAllowSetStateFromRefsInEffects) { + if (enableAllowSetStateFromRefsInEffects) { const arg = instr.value.args.at(0); if ( arg !== undefined && @@ -216,6 +291,8 @@ function getSetStateCall( * be needed when initial layout measurements from refs need to be stored in state. */ return null; + } else if (isRefControlledBlock(block.id)) { + continue; } } /* diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.expect.md new file mode 100644 index 00000000000..43a84784ea0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.expect.md @@ -0,0 +1,117 @@ + +## Input + +```javascript +// @validateNoSetStateInEffects @enableAllowSetStateFromRefsInEffects @loggerTestOnly @compilationMode:"infer" +import {useState, useRef, useEffect} from 'react'; + +function Component({x, y}) { + const previousXRef = useRef(null); + const previousYRef = useRef(null); + + const [data, setData] = useState(null); + + useEffect(() => { + const previousX = previousXRef.current; + previousXRef.current = x; + const previousY = previousYRef.current; + previousYRef.current = y; + if (!areEqual(x, previousX) || !areEqual(y, previousY)) { + const data = load({x, y}); + setData(data); + } + }, [x, y]); + + return data; +} + +function areEqual(a, b) { + return a === b; +} + +function load({x, y}) { + return x * y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 0, y: 0}], + sequentialRenders: [ + {x: 0, y: 0}, + {x: 1, y: 0}, + {x: 1, y: 1}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInEffects @enableAllowSetStateFromRefsInEffects @loggerTestOnly @compilationMode:"infer" +import { useState, useRef, useEffect } from "react"; + +function Component(t0) { + const $ = _c(4); + const { x, y } = t0; + const previousXRef = useRef(null); + const previousYRef = useRef(null); + + const [data, setData] = useState(null); + let t1; + let t2; + if ($[0] !== x || $[1] !== y) { + t1 = () => { + const previousX = previousXRef.current; + previousXRef.current = x; + const previousY = previousYRef.current; + previousYRef.current = y; + if (!areEqual(x, previousX) || !areEqual(y, previousY)) { + const data_0 = load({ x, y }); + setData(data_0); + } + }; + + t2 = [x, y]; + $[0] = x; + $[1] = y; + $[2] = t1; + $[3] = t2; + } else { + t1 = $[2]; + t2 = $[3]; + } + useEffect(t1, t2); + return data; +} + +function areEqual(a, b) { + return a === b; +} + +function load({ x, y }) { + return x * y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ x: 0, y: 0 }], + sequentialRenders: [ + { x: 0, y: 0 }, + { x: 1, y: 0 }, + { x: 1, y: 1 }, + ], +}; + +``` + +## Logs + +``` +{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":163},"end":{"line":22,"column":1,"index":631},"filename":"valid-setState-in-useEffect-controlled-by-ref-value.ts"},"fnName":"Component","memoSlots":4,"memoBlocks":1,"memoValues":2,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### Eval output +(kind: ok) 0 +0 +1 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.js new file mode 100644 index 00000000000..46f11057d11 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.js @@ -0,0 +1,40 @@ +// @validateNoSetStateInEffects @enableAllowSetStateFromRefsInEffects @loggerTestOnly @compilationMode:"infer" +import {useState, useRef, useEffect} from 'react'; + +function Component({x, y}) { + const previousXRef = useRef(null); + const previousYRef = useRef(null); + + const [data, setData] = useState(null); + + useEffect(() => { + const previousX = previousXRef.current; + previousXRef.current = x; + const previousY = previousYRef.current; + previousYRef.current = y; + if (!areEqual(x, previousX) || !areEqual(y, previousY)) { + const data = load({x, y}); + setData(data); + } + }, [x, y]); + + return data; +} + +function areEqual(a, b) { + return a === b; +} + +function load({x, y}) { + return x * y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 0, y: 0}], + sequentialRenders: [ + {x: 0, y: 0}, + {x: 1, y: 0}, + {x: 1, y: 1}, + ], +};