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 01c82a375c9ec..d5f9ab2932359 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts @@ -43,20 +43,37 @@ import {retainWhere} from '../Utils/utils'; const DEBUG = false; /** - * Validates that existing manual memoization had exhaustive dependencies. - * Memoization with missing or extra reactive dependencies is invalid React - * and compilation can change behavior, causing a value to be computed more - * or less times. + * Validates that existing manual memoization is exhaustive and does not + * have extraneous dependencies. The goal of the validation is to ensure + * that auto-memoization will not substantially change the behavior of + * the program: + * - If the manual dependencies were non-exhaustive (missing important deps) + * then auto-memoization will include those dependencies, and cause the + * value to update *more* frequently. + * - If the manual dependencies had extraneous deps, then auto memoization + * will remove them and cause the value to update *less* frequently. * - * TODOs: - * - Handle cases of mixed optional and non-optional versions of the same path, - * eg referecing both x.y.z and x.y?.z in the same memo block. we should collapse - * this into a single canonical dep that we look for in the manual deps. see the - * existing exhaustive deps rule for implementation. - * - Handle cases where the user deps were not simple identifiers + property chains. - * We try to detect this in ValidateUseMemo but we miss some cases. The problem - * is that invalid forms can be value blocks or function calls that don't get - * removed by DCE, leaving a structure like: + * 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. + * + * ## TODO: Invalid, Complex Deps + * + * Handle cases where the user deps were not simple identifiers + property chains. + * We try to detect this in ValidateUseMemo but we miss some cases. The problem + * is that invalid forms can be value blocks or function calls that don't get + * removed by DCE, leaving a structure like: * * StartMemoize * t0 = @@ -209,31 +226,9 @@ export function validateExhaustiveDependencies( reason: 'Unexpected function dependency', loc: value.loc, }); - /** - * Dependencies technically only need to include reactive values. However, - * reactivity inference for general values is subtle since it involves all - * of our complex control and data flow analysis. To keep results more - * stable and predictable to developers, we intentionally stay closer to - * the rules of the classic exhaustive-deps rule. Values should be included - * as dependencies if either of the following is true: - * - They're reactive - * - They're non-reactive and not a known-stable value type. - * - * Thus `const ref: Ref = cond ? ref1 : ref2` has to be a dependency - * (assuming `cond` is reactive) since it's reactive despite being a ref. - * - * Similarly, `const x = [1,2,3]` has to be a dependency since even - * though it's non reactive, it's not a known stable type. - * - * TODO: consider reimplementing a simpler form of reactivity inference. - * Ideally we'd consider `const ref: Ref = cond ? ref1 : ref2` as a required - * dependency even if our data/control flow tells us that `cond` is non-reactive. - * It's simpler for developers to reason about based on a more structural/AST - * driven approach. - */ - const isRequiredDependency = - reactive.has(inferredDependency.identifier.id) || - !isStableType(inferredDependency.identifier); + const isRequiredDependency = reactive.has( + inferredDependency.identifier.id, + ); let hasMatchingManualDependency = false; for (const manualDependency of manualDependencies) { if ( @@ -265,18 +260,13 @@ export function validateExhaustiveDependencies( extra.push(dep); } - /* - * For compatiblity with the existing exhaustive-deps rule, we allow - * known-stable values as dependencies even if the value is not reactive. - * This allows code that takes a dep on a non-reactive setState function - * to pass, for example. + /** + * 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 => { - const isNonReactiveStableValue = - dep.root.kind === 'NamedLocal' && - !dep.root.value.reactive && - isStableType(dep.root.value.identifier); - return !isNonReactiveStableValue; + return dep.root.kind === 'Global' || dep.root.value.reactive; }); if (missing.length !== 0 || extra.length !== 0) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md index 655d6bff4153b..70d8a3abbf66e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md @@ -3,7 +3,7 @@ ```javascript // @validateExhaustiveMemoizationDependencies -import {useMemo} from 'react'; +import {useCallback, useMemo} from 'react'; import {makeObject_Primitives, Stringify} from 'shared-runtime'; function useHook1(x) { @@ -47,6 +47,16 @@ function useHook6(x) { }, [x]); } +function useHook7(x) { + const [state, setState] = useState(true); + const f = () => { + setState(x => !x); + }; + return useCallback(() => { + f(); + }, [f]); +} + function Component({x, y, z}) { const a = useHook1(x); const b = useHook2(x); @@ -54,7 +64,8 @@ function Component({x, y, z}) { const d = useHook4(x, y, z); const e = useHook5(x); const f = useHook6(x); - return ; + const g = useHook7(x); + return ; } ``` @@ -63,7 +74,7 @@ function Component({x, y, z}) { ```javascript import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies -import { useMemo } from "react"; +import { useCallback, useMemo } from "react"; import { makeObject_Primitives, Stringify } from "shared-runtime"; function useHook1(x) { @@ -121,8 +132,36 @@ function useHook6(x) { return f; } +function useHook7(x) { + const $ = _c(2); + const [, setState] = useState(true); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = () => { + setState(_temp); + }; + $[0] = t0; + } else { + t0 = $[0]; + } + const f = t0; + let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = () => { + f(); + }; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} +function _temp(x_0) { + return !x_0; +} + function Component(t0) { - const $ = _c(7); + const $ = _c(8); const { x, y, z } = t0; const a = useHook1(x); const b = useHook2(x); @@ -130,6 +169,7 @@ function Component(t0) { const d = useHook4(x, y, z); const e = useHook5(x); const f = useHook6(x); + const g = useHook7(x); let t1; if ( $[0] !== a || @@ -137,18 +177,20 @@ function Component(t0) { $[2] !== c || $[3] !== d || $[4] !== e || - $[5] !== f + $[5] !== f || + $[6] !== g ) { - t1 = ; + t1 = ; $[0] = a; $[1] = b; $[2] = c; $[3] = d; $[4] = e; $[5] = f; - $[6] = t1; + $[6] = g; + $[7] = t1; } else { - t1 = $[6]; + t1 = $[7]; } return t1; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js index d4c833c7d29ba..38e730b0d21ae 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js @@ -1,5 +1,5 @@ // @validateExhaustiveMemoizationDependencies -import {useMemo} from 'react'; +import {useCallback, useMemo} from 'react'; import {makeObject_Primitives, Stringify} from 'shared-runtime'; function useHook1(x) { @@ -43,6 +43,16 @@ function useHook6(x) { }, [x]); } +function useHook7(x) { + const [state, setState] = useState(true); + const f = () => { + setState(x => !x); + }; + return useCallback(() => { + f(); + }, [f]); +} + function Component({x, y, z}) { const a = useHook1(x); const b = useHook2(x); @@ -50,5 +60,6 @@ function Component({x, y, z}) { const d = useHook4(x, y, z); const e = useHook5(x); const f = useHook6(x); - return ; + const g = useHook7(x); + return ; }