From 87af9cdb63d9e71d95c5a2f6a686e4e0c8e6a173 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 24 Nov 2025 16:34:08 -0800 Subject: [PATCH] [compiler] Distingush optional/extraneous deps In ValidateExhaustiveDependencies, I previously changed to allow extraneous dependencies as long as they were non-reactive. Here we make that more precise, and distinguish between values that are definitely referenced in the memo function but optional as dependencies vs values that are not even referenced in the memo function. The latter now error as extraneous even if they're non-reactive. This also turned up a case where constant-folded primitives could show up as false positives of the latter category, so now we track manual deps which quality for constant folding and don't error on them. --- .../src/HIR/HIR.ts | 1 + .../src/Inference/DropManualMemoization.ts | 1 + .../src/Optimization/ConstantPropagation.ts | 13 ++++ .../ValidateExhaustiveDependencies.ts | 66 ++++++++++--------- .../ValidatePreservedManualMemoization.ts | 3 + ...text-variable-as-jsx-element-tag.expect.md | 3 +- .../context-variable-as-jsx-element-tag.js | 2 +- ...eps-disallow-unused-stable-types.expect.md | 42 ++++++++++++ ...stive-deps-disallow-unused-stable-types.js | 14 ++++ ...eps-allow-constant-folded-values.expect.md | 41 ++++++++++++ ...stive-deps-allow-constant-folded-values.js | 11 ++++ ...-constant-prop-decls-get-removed.expect.md | 4 +- ...-ensure-constant-prop-decls-get-removed.ts | 2 +- ...e-preserve-memoization-guarantee.expect.md | 2 +- ...variable-preserve-memoization-guarantee.js | 2 +- 15 files changed, 169 insertions(+), 38 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps-disallow-unused-stable-types.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps-disallow-unused-stable-types.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-constant-folded-values.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-constant-folded-values.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 55bcd0bc5ce89..9fb360cf8a5ad 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -803,6 +803,7 @@ export type ManualMemoDependency = { | { kind: 'NamedLocal'; value: Place; + constant: boolean; } | {kind: 'Global'; identifierName: string}; path: DependencyPath; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts index bcfc53413ab41..0138e52ef60e4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts @@ -92,6 +92,7 @@ export function collectMaybeMemoDependencies( root: { kind: 'NamedLocal', value: {...value.place}, + constant: false, }, path: [], }; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts index ca2f6e00a5d0b..7330b63ddce42 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts @@ -609,6 +609,19 @@ function evaluateInstruction( constantPropagationImpl(value.loweredFunc.func, constants); return null; } + case 'StartMemoize': { + if (value.deps != null) { + for (const dep of value.deps) { + if (dep.root.kind === 'NamedLocal') { + const placeValue = read(constants, dep.root.value); + if (placeValue != null && placeValue.kind === 'Primitive') { + dep.root.constant = true; + } + } + } + } + return null; + } default: { // TODO: handle more cases return null; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts index b3a68fc0134ba..5035fa9260f02 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts @@ -22,6 +22,7 @@ import { Identifier, IdentifierId, InstructionKind, + isPrimitiveType, isStableType, isSubPath, isSubPathIgnoringOptionals, @@ -53,20 +54,18 @@ const DEBUG = false; * - If the manual dependencies had extraneous deps, then auto memoization * will remove them and cause the value to update *less* frequently. * - * We consider a value V as missing if ALL of the following conditions are met: - * - V is reactive - * - There is no manual dependency path P such that whenever V would change, - * P would also change. If V is `x.y.z`, this means there must be some - * path P that is either `x.y.z`, `x.y`, or `x`. Note that we assume no - * interior mutability, such that a shorter path "covers" changes to longer - * more precise paths. - * - * We consider a value V extraneous if either of the folowing are true: - * - V is a reactive local that is unreferenced - * - V is a global that is unreferenced - * - * In other words, we allow extraneous non-reactive values since we know they cannot - * impact how often the memoization would run. + * The implementation compares the manual dependencies against the values + * actually used within the memoization function + * - For each value V referenced in the memo function, either: + * - If the value is non-reactive *and* a known stable type, then the + * value may optionally be specified as an exact dependency. + * - Otherwise, report an error unless there is a manual dependency that will + * invalidate whenever V invalidates. If `x.y.z` is referenced, there must + * be a manual dependency for `x.y.z`, `x.y`, or `x`. Note that we assume + * no interior mutability, ie we assume that any changes to inner paths must + * always cause the other path to change as well. + * - Any dependencies that do not correspond to a value referenced in the memo + * function are considered extraneous and throw an error * * ## TODO: Invalid, Complex Deps * @@ -226,9 +225,6 @@ export function validateExhaustiveDependencies( reason: 'Unexpected function dependency', loc: value.loc, }); - const isRequiredDependency = reactive.has( - inferredDependency.identifier.id, - ); let hasMatchingManualDependency = false; for (const manualDependency of manualDependencies) { if ( @@ -243,32 +239,40 @@ export function validateExhaustiveDependencies( ) { hasMatchingManualDependency = true; matched.add(manualDependency); - if (!isRequiredDependency) { - extra.push(manualDependency); - } } } - if (isRequiredDependency && !hasMatchingManualDependency) { - missing.push(inferredDependency); + const isOptionalDependency = + !reactive.has(inferredDependency.identifier.id) && + (isStableType(inferredDependency.identifier) || + isPrimitiveType(inferredDependency.identifier)); + if (hasMatchingManualDependency || isOptionalDependency) { + continue; } + missing.push(inferredDependency); } for (const dep of startMemo.deps ?? []) { if (matched.has(dep)) { continue; } + if (dep.root.kind === 'NamedLocal' && dep.root.constant) { + CompilerError.simpleInvariant( + !dep.root.value.reactive && + isPrimitiveType(dep.root.value.identifier), + { + reason: 'Expected constant-folded dependency to be non-reactive', + loc: dep.root.value.loc, + }, + ); + /* + * Constant primitives can get constant-folded, which means we won't + * see a LoadLocal for the value within the memo function. + */ + continue; + } extra.push(dep); } - /** - * Per docblock, we only consider dependencies as extraneous if - * they are unused globals or reactive locals. Notably, this allows - * non-reactive locals. - */ - retainWhere(extra, dep => { - return dep.root.kind === 'Global' || dep.root.value.reactive; - }); - if (missing.length !== 0 || extra.length !== 0) { let suggestions: Array | null = null; if (startMemo.depsLoc != null && typeof startMemo.depsLoc !== 'symbol') { 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 19af0ed080142..48cec3cb12202 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -267,6 +267,7 @@ function validateInferredDep( effect: Effect.Read, reactive: false, }, + constant: false, }, path: [...dep.path], }; @@ -379,6 +380,7 @@ class Visitor extends ReactiveFunctionVisitor { root: { kind: 'NamedLocal', value: storeTarget, + constant: false, }, path: [], }); @@ -408,6 +410,7 @@ class Visitor extends ReactiveFunctionVisitor { root: { kind: 'NamedLocal', value: {...lvalue}, + constant: false, }, path: [], }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.expect.md index 407fdcb0488f2..636bc53a172e5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.expect.md @@ -11,7 +11,7 @@ function Component(props) { Component = useMemo(() => { return Component; - }); + }, [Component]); return ; } @@ -36,6 +36,7 @@ function Component(props) { if ($[0] === Symbol.for("react.memo_cache_sentinel")) { Component = Stringify; + Component; Component = Component; $[0] = Component; } else { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.js index 5ed1a9157bdfd..49cf3364b1c4a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.js @@ -7,7 +7,7 @@ function Component(props) { Component = useMemo(() => { return Component; - }); + }, [Component]); return ; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps-disallow-unused-stable-types.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps-disallow-unused-stable-types.expect.md new file mode 100644 index 0000000000000..b6d8a46742673 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps-disallow-unused-stable-types.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +// @validateExhaustiveMemoizationDependencies + +import {useState} from 'react'; +import {Stringify} from 'shared-runtime'; + +function Component() { + const [state, setState] = useState(0); + const x = useMemo(() => { + return [state]; + // error: `setState` is a stable type, but not actually referenced + }, [state, setState]); + + return 'oops'; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: Found unnecessary memoization dependencies + +Unnecessary dependencies can cause a value to update more often than necessary, causing performance regressions and effects to fire more often than expected. + +error.invalid-exhaustive-deps-disallow-unused-stable-types.ts:11:5 + 9 | return [state]; + 10 | // error: `setState` is a stable type, but not actually referenced +> 11 | }, [state, setState]); + | ^^^^^^^^^^^^^^^^^ Unnecessary dependencies `setState` + 12 | + 13 | return 'oops'; + 14 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps-disallow-unused-stable-types.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps-disallow-unused-stable-types.js new file mode 100644 index 0000000000000..2bb03c366113f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps-disallow-unused-stable-types.js @@ -0,0 +1,14 @@ +// @validateExhaustiveMemoizationDependencies + +import {useState} from 'react'; +import {Stringify} from 'shared-runtime'; + +function Component() { + const [state, setState] = useState(0); + const x = useMemo(() => { + return [state]; + // error: `setState` is a stable type, but not actually referenced + }, [state, setState]); + + return 'oops'; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-constant-folded-values.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-constant-folded-values.expect.md new file mode 100644 index 0000000000000..bda2b03562507 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-constant-folded-values.expect.md @@ -0,0 +1,41 @@ + +## Input + +```javascript +// @validateExhaustiveMemoizationDependencies + +function Component() { + const x = 0; + const y = useMemo(() => { + return [x]; + // x gets constant-folded but shouldn't count as extraneous, + // it was referenced in the memo block + }, [x]); + return y; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies + +function Component() { + const $ = _c(1); + const x = 0; + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = [0]; + $[0] = t0; + } else { + t0 = $[0]; + } + const y = t0; + return y; +} + +``` + +### 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/exhaustive-deps-allow-constant-folded-values.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-constant-folded-values.js new file mode 100644 index 0000000000000..6ee141cb3ae04 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-constant-folded-values.js @@ -0,0 +1,11 @@ +// @validateExhaustiveMemoizationDependencies + +function Component() { + const x = 0; + const y = useMemo(() => { + return [x]; + // x gets constant-folded but shouldn't count as extraneous, + // it was referenced in the memo block + }, [x]); + return y; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.expect.md index cf36d6ed67308..f23cf84e504fd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.expect.md @@ -2,7 +2,7 @@ ## Input ```javascript -// @validatePreserveExistingMemoizationGuarantees +// @validatePreserveExistingMemoizationGuarantees @validateExhaustiveMemoizationDependencies:false import {useMemo} from 'react'; @@ -27,7 +27,7 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @validateExhaustiveMemoizationDependencies:false import { useMemo } from "react"; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.ts index b0c0139526922..bfbc0ab5056a4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/todo-ensure-constant-prop-decls-get-removed.ts @@ -1,4 +1,4 @@ -// @validatePreserveExistingMemoizationGuarantees +// @validatePreserveExistingMemoizationGuarantees @validateExhaustiveMemoizationDependencies:false import {useMemo} from 'react'; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-maybe-modify-free-variable-preserve-memoization-guarantee.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-maybe-modify-free-variable-preserve-memoization-guarantee.expect.md index 43b3b599e198b..16169a74d716b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-maybe-modify-free-variable-preserve-memoization-guarantee.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-maybe-modify-free-variable-preserve-memoization-guarantee.expect.md @@ -15,7 +15,7 @@ function Component(props) { const x = makeObject_Primitives(); x.value = props.value; mutate(x, free, part); - }, [props.value]); + }, [props.value, free, part]); mutate(free, part); return callback; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-maybe-modify-free-variable-preserve-memoization-guarantee.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-maybe-modify-free-variable-preserve-memoization-guarantee.js index 662162ed64407..e9d11525e2c21 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-maybe-modify-free-variable-preserve-memoization-guarantee.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-maybe-modify-free-variable-preserve-memoization-guarantee.js @@ -11,7 +11,7 @@ function Component(props) { const x = makeObject_Primitives(); x.value = props.value; mutate(x, free, part); - }, [props.value]); + }, [props.value, free, part]); mutate(free, part); return callback; }