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; }