diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonReactiveDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonReactiveDependencies.ts index 9bdf15aeb534..5c2fb1b19d95 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonReactiveDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonReactiveDependencies.ts @@ -97,7 +97,23 @@ class Visitor extends ReactiveFunctionVisitor { this.traverseScope(scopeBlock, state); for (const dep of scopeBlock.scope.dependencies) { const isReactive = state.has(dep.identifier.id); - if (!isReactive) { + /* + * Stable values (state setters, dispatch functions, etc.) are guaranteed + * to have a stable identity across renders and should never be scope + * dependencies. Even if they appear reactive due to how they're captured + * in closures, including them as deps produces unnecessary cache checks. + */ + /* + * Stable values (state setters, dispatch functions, etc.) are guaranteed + * to have a stable identity across renders and should never be scope + * dependencies. Even if they appear reactive due to how they're captured + * in closures, including them as deps produces unnecessary cache checks. + * + * Note: we guard on !dep.reactive to preserve reactive identifiers that + * happen to have a stable type (e.g. `const ref = cond ? ref1 : ref2`), + * which DO change identity based on reactive inputs. + */ + if (!isReactive || (!dep.reactive && isStableType(dep.identifier))) { scopeBlock.scope.dependencies.delete(dep); } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index d39aa307dfb3..662ebb56f8e9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -26,6 +26,7 @@ import { ReactiveValue, ScopeId, SourceLocation, + isStableType, } from '../HIR'; import {Environment} from '../HIR/Environment'; import {printIdentifier, printManualMemoDependency} from '../HIR/PrintHIR'; @@ -233,6 +234,22 @@ function validateInferredDep( env: Environment, memoLocation: SourceLocation, ): void { + /* + * Stable values (state setters, dispatch functions, refs, etc.) that are + * not reactive are guaranteed to never change identity across renders. The + * Rules of React and exhaustive-deps rules do not require them in + * dependency arrays, so the preserve-manual-memoization validation should + * not require them either. Requiring stable, non-reactive values causes + * false-positive skips when user code correctly omits them from a manual + * `useCallback`/`useMemo` deps list. + * + * Note: we only skip non-reactive stable values. Reactive identifiers that + * happen to have a stable type (e.g. `const ref = cond ? ref1 : ref2`) + * still need to appear in the deps array because their identity changes. + */ + if (!dep.reactive && isStableType(dep.identifier)) { + return; + } let normalizedDep: ManualMemoDependency; const maybeNormalizedRoot = temporaries.get(dep.identifier.id); if (maybeNormalizedRoot != null) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.expect.md index 9397518d2d11..f9833e98b3f1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.expect.md @@ -27,7 +27,7 @@ function useFoo() { ```javascript import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees function useFoo() { - const $ = _c(7); + const $ = _c(4); const onClick = (response) => { setState(DISABLED_FORM); }; @@ -35,34 +35,31 @@ function useFoo() { const [, t0] = useState(); const setState = t0; let t1; - if ($[0] !== setState) { + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t1 = () => { setState(DISABLED_FORM); }; - $[0] = setState; - $[1] = t1; + $[0] = t1; } else { - t1 = $[1]; + t1 = $[0]; } setState; const handleLogout = t1; let t2; - if ($[2] !== handleLogout) { + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { const getComponent = () => handleLogout()} />; t2 = getComponent(); - $[2] = handleLogout; - $[3] = t2; + $[1] = t2; } else { - t2 = $[3]; + t2 = $[1]; } let t3; - if ($[4] !== onClick || $[5] !== t2) { + if ($[2] !== onClick) { t3 = [t2, onClick]; - $[4] = onClick; - $[5] = t2; - $[6] = t3; + $[2] = onClick; + $[3] = t3; } else { - t3 = $[6]; + t3 = $[3]; } return t3; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-inferred-dep-issue-36384.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-inferred-dep-issue-36384.expect.md new file mode 100644 index 000000000000..62eb618b0290 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-inferred-dep-issue-36384.expect.md @@ -0,0 +1,118 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +import {useCallback, useEffect, useState} from 'react'; + +/** + * Regression test for: https://github.com/facebook/react/issues/36384 + * + * The compiler was incorrectly inferring `setImages` (a state setter) as a + * required dependency of the useCallback scope in this specific pattern, + * where JSON.parse of an external value is used as initial state. + * + * Without the fix, the compiler would produce: + * "Compilation Skipped: Existing memoization could not be preserved. + * The inferred dependency was `setImages`, but the source dependencies were []." + * + * This test should compile successfully — state setters are stable values + * and must not be required in manual dependency arrays. + */ + +function getValue() { + return 'true'; +} + +function ImageLibraryPicker() { + const boolString = getValue(); + const [images, setImages] = useState(''); + const [booleanState, setBooleanState] = useState( + JSON.parse(boolString), + ); + + const search = useCallback(() => { + setImages(''); + }, []); + + useEffect(() => { + search(); + }, [search]); + + return null; +} + +export const FIXTURE_ENTRYPOINT = { + fn: ImageLibraryPicker, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees +import { useCallback, useEffect, useState } from "react"; + +/** + * Regression test for: https://github.com/facebook/react/issues/36384 + * + * The compiler was incorrectly inferring `setImages` (a state setter) as a + * required dependency of the useCallback scope in this specific pattern, + * where JSON.parse of an external value is used as initial state. + * + * Without the fix, the compiler would produce: + * "Compilation Skipped: Existing memoization could not be preserved. + * The inferred dependency was `setImages`, but the source dependencies were []." + * + * This test should compile successfully — state setters are stable values + * and must not be required in manual dependency arrays. + */ + +function getValue() { + return "true"; +} + +function ImageLibraryPicker() { + const $ = _c(3); + const boolString = getValue(); + const [, setImages] = useState(""); + useState(JSON.parse(boolString)); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = () => { + setImages(""); + }; + $[0] = t0; + } else { + t0 = $[0]; + } + const search = t0; + let t1; + let t2; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = () => { + search(); + }; + t2 = [search]; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + + return null; +} + +export const FIXTURE_ENTRYPOINT = { + fn: ImageLibraryPicker, + params: [], +}; + +``` + +### Eval output +(kind: ok) null \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-inferred-dep-issue-36384.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-inferred-dep-issue-36384.ts new file mode 100644 index 000000000000..7253d0b47bf2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-inferred-dep-issue-36384.ts @@ -0,0 +1,44 @@ +// @validatePreserveExistingMemoizationGuarantees +import {useCallback, useEffect, useState} from 'react'; + +/** + * Regression test for: https://github.com/facebook/react/issues/36384 + * + * The compiler was incorrectly inferring `setImages` (a state setter) as a + * required dependency of the useCallback scope in this specific pattern, + * where JSON.parse of an external value is used as initial state. + * + * Without the fix, the compiler would produce: + * "Compilation Skipped: Existing memoization could not be preserved. + * The inferred dependency was `setImages`, but the source dependencies were []." + * + * This test should compile successfully — state setters are stable values + * and must not be required in manual dependency arrays. + */ + +function getValue() { + return 'true'; +} + +function ImageLibraryPicker() { + const boolString = getValue(); + const [images, setImages] = useState(''); + const [booleanState, setBooleanState] = useState( + JSON.parse(boolString), + ); + + const search = useCallback(() => { + setImages(''); + }, []); + + useEffect(() => { + search(); + }, [search]); + + return null; +} + +export const FIXTURE_ENTRYPOINT = { + fn: ImageLibraryPicker, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-stable-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-stable-dep.expect.md new file mode 100644 index 000000000000..c4b3f57c6e86 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-stable-dep.expect.md @@ -0,0 +1,145 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +import {useCallback, useState} from 'react'; + +/** + * State setters (and other stable values like dispatch) must not be required + * in manual dependency arrays. The compiler should not reject a component + * because its useCallback omits a state setter from its deps list, since + * state setters are guaranteed stable across renders. + * + * Regression test for: Compiler requires state setter to be added to + * dependency array (github.com/facebook/react/issues/36384) + */ +function Component({data}: {data: Array}) { + const [processed, setProcessed] = useState>([]); + const [count, setCount] = useState(0); + + const handleProcess = useCallback(async () => { + const result = data.map(d => d.trim()); + setProcessed(prev => [...prev, ...result]); + setCount(c => c + 1); + }, [data]); + + return ( +
+ + {count} + {processed.map((p, i) => ( +
{p}
+ ))} +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{data: ['hello ', ' world']}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees +import { useCallback, useState } from "react"; + +/** + * State setters (and other stable values like dispatch) must not be required + * in manual dependency arrays. The compiler should not reject a component + * because its useCallback omits a state setter from its deps list, since + * state setters are guaranteed stable across renders. + * + * Regression test for: Compiler requires state setter to be added to + * dependency array (github.com/facebook/react/issues/36384) + */ +function Component(t0) { + const $ = _c(13); + const { data } = t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t1 = []; + $[0] = t1; + } else { + t1 = $[0]; + } + const [processed, setProcessed] = useState(t1); + const [count, setCount] = useState(0); + let t2; + if ($[1] !== data) { + t2 = async () => { + const result = data.map(_temp); + setProcessed((prev) => [...prev, ...result]); + setCount(_temp2); + }; + $[1] = data; + $[2] = t2; + } else { + t2 = $[2]; + } + const handleProcess = t2; + let t3; + if ($[3] !== handleProcess) { + t3 = ; + $[3] = handleProcess; + $[4] = t3; + } else { + t3 = $[4]; + } + let t4; + if ($[5] !== count) { + t4 = {count}; + $[5] = count; + $[6] = t4; + } else { + t4 = $[6]; + } + let t5; + if ($[7] !== processed) { + t5 = processed.map(_temp3); + $[7] = processed; + $[8] = t5; + } else { + t5 = $[8]; + } + let t6; + if ($[9] !== t3 || $[10] !== t4 || $[11] !== t5) { + t6 = ( +
+ {t3} + {t4} + {t5} +
+ ); + $[9] = t3; + $[10] = t4; + $[11] = t5; + $[12] = t6; + } else { + t6 = $[12]; + } + return t6; +} +function _temp3(p, i) { + return
{p}
; +} +function _temp2(c) { + return c + 1; +} +function _temp(d) { + return d.trim(); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ data: ["hello ", " world"] }], +}; + +``` + +### Eval output +(kind: ok)
0
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-stable-dep.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-stable-dep.ts new file mode 100644 index 000000000000..20b963ff5f19 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-state-setter-stable-dep.ts @@ -0,0 +1,37 @@ +// @validatePreserveExistingMemoizationGuarantees +import {useCallback, useState} from 'react'; + +/** + * State setters (and other stable values like dispatch) must not be required + * in manual dependency arrays. The compiler should not reject a component + * because its useCallback omits a state setter from its deps list, since + * state setters are guaranteed stable across renders. + * + * Regression test for: Compiler requires state setter to be added to + * dependency array (github.com/facebook/react/issues/36384) + */ +function Component({data}: {data: Array}) { + const [processed, setProcessed] = useState>([]); + const [count, setCount] = useState(0); + + const handleProcess = useCallback(async () => { + const result = data.map(d => d.trim()); + setProcessed(prev => [...prev, ...result]); + setCount(c => c + 1); + }, [data]); + + return ( +
+ + {count} + {processed.map((p, i) => ( +
{p}
+ ))} +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{data: ['hello ', ' world']}], +};