diff --git a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts index ed64936610f73..352d31b2f8972 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts @@ -600,7 +600,8 @@ function printErrorSummary(category: ErrorCategory, message: string): string { case ErrorCategory.Suppression: case ErrorCategory.Syntax: case ErrorCategory.UseMemo: - case ErrorCategory.VoidUseMemo: { + case ErrorCategory.VoidUseMemo: + case ErrorCategory.MemoDependencies: { heading = 'Error'; break; } @@ -658,6 +659,10 @@ export enum ErrorCategory { * Checks that manual memoization is preserved */ PreserveManualMemo = 'PreserveManualMemo', + /** + * Checks for exhaustive useMemo/useCallback dependencies without extraneous values + */ + MemoDependencies = 'MemoDependencies', /** * Checks for known incompatible libraries */ @@ -1055,6 +1060,16 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule { preset: LintRulePreset.RecommendedLatest, }; } + case ErrorCategory.MemoDependencies: { + return { + category, + severity: ErrorSeverity.Error, + name: 'memo-dependencies', + description: + 'Validates that useMemo() and useCallback() specify comprehensive dependencies without extraneous values. See [`useMemo()` docs](https://react.dev/reference/react/useMemo) for more information.', + preset: LintRulePreset.RecommendedLatest, + }; + } case ErrorCategory.IncompatibleLibrary: { return { category, 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 fb1f9445aa0d3..313f773e2945b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts @@ -22,7 +22,9 @@ import { Identifier, IdentifierId, InstructionKind, + isStableType, isSubPath, + isUseRefType, LoadGlobal, ManualMemoDependency, Place, @@ -46,9 +48,10 @@ const DEBUG = false; * or less times. * * TODOs: - * - Better handling of cases where we infer multiple dependencies related to a single - * variable. Eg if the user has dep `x` and we inferred `x.y, x.z`, the user's dep - * is sufficient. + * - 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 @@ -108,7 +111,7 @@ export function validateExhaustiveDependencies( ); visitCandidateDependency(value.decl, temporaries, dependencies, locals); const inferred: Array = Array.from(dependencies); - // Sort dependencies by name, and path, with shorter/non-optional paths first + // Sort dependencies by name and path, with shorter/non-optional paths first inferred.sort((a, b) => { if (a.kind === 'Global' && b.kind == 'Global') { return a.binding.name.localeCompare(b.binding.name); @@ -205,6 +208,31 @@ 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); let hasMatchingManualDependency = false; for (const manualDependency of manualDependencies) { if ( @@ -216,19 +244,18 @@ export function validateExhaustiveDependencies( ) { hasMatchingManualDependency = true; matched.add(manualDependency); + if (!isRequiredDependency) { + extra.push(manualDependency); + } } } - if (!hasMatchingManualDependency) { + if (isRequiredDependency && !hasMatchingManualDependency) { missing.push(inferredDependency); } } for (const dep of startMemo.deps ?? []) { - if ( - matched.has(dep) || - (dep.root.kind === 'NamedLocal' && - !reactive.has(dep.root.value.identifier.id)) - ) { + if (matched.has(dep)) { continue; } extra.push(dep); @@ -247,36 +274,39 @@ export function validateExhaustiveDependencies( ]; } if (missing.length !== 0) { - // Error const diagnostic = CompilerDiagnostic.create({ - category: ErrorCategory.PreserveManualMemo, + category: ErrorCategory.MemoDependencies, reason: 'Found non-exhaustive dependencies', description: 'Missing dependencies can cause a value not to update when those inputs change, ' + - 'resulting in stale UI. This memoization cannot be safely rewritten by the compiler.', + 'resulting in stale UI', suggestions, }); for (const dep of missing) { + let reactiveStableValueHint = ''; + if (isStableType(dep.identifier)) { + reactiveStableValueHint = + '. Refs, setState functions, and other "stable" values generally do not need to be added as dependencies, but this variable may change over time to point to different values'; + } diagnostic.withDetails({ kind: 'error', - message: `Missing dependency \`${printInferredDependency(dep)}\``, + message: `Missing dependency \`${printInferredDependency(dep)}\`${reactiveStableValueHint}`, loc: dep.loc, }); } error.pushDiagnostic(diagnostic); } else if (extra.length !== 0) { const diagnostic = CompilerDiagnostic.create({ - category: ErrorCategory.PreserveManualMemo, + category: ErrorCategory.MemoDependencies, reason: 'Found unnecessary memoization dependencies', description: 'Unnecessary dependencies can cause a value to update more often than necessary, ' + - 'which can cause effects to run more than expected. This memoization cannot be safely ' + - 'rewritten by the compiler', + 'which can cause effects to run more than expected', }); diagnostic.withDetails({ kind: 'error', message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`, - loc: value.loc, + loc: startMemo.depsLoc ?? value.loc, }); error.pushDiagnostic(diagnostic); } @@ -287,10 +317,15 @@ export function validateExhaustiveDependencies( startMemo = null; } - collectDependencies(fn, temporaries, { - onStartMemoize, - onFinishMemoize, - }); + collectDependencies( + fn, + temporaries, + { + onStartMemoize, + onFinishMemoize, + }, + false, // isFunctionExpression + ); return error.asResult(); } @@ -383,12 +418,20 @@ function collectDependencies( locals: Set, ) => void; } | null, + isFunctionExpression: boolean, ): Extract { const optionals = findOptionalPlaces(fn); if (DEBUG) { console.log(prettyFormat(optionals)); } const locals: Set = new Set(); + if (isFunctionExpression) { + for (const param of fn.params) { + const place = param.kind === 'Identifier' ? param : param.place; + locals.add(place.identifier.id); + } + } + const dependencies: Set = new Set(); function visit(place: Place): void { visitCandidateDependency(place, temporaries, dependencies, locals); @@ -523,7 +566,11 @@ function collectDependencies( break; } case 'PropertyLoad': { - if (typeof value.property === 'number') { + if ( + typeof value.property === 'number' || + (isUseRefType(value.object.identifier) && + value.property === 'current') + ) { visit(value.object); break; } @@ -553,6 +600,7 @@ function collectDependencies( value.loweredFunc.func, temporaries, null, + true, // isFunctionExpression ); temporaries.set(lvalue.identifier.id, functionDeps); addDependency(functionDeps, dependencies, locals); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md index 3af5cc2d6b4bc..5152f4121a2e0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md @@ -26,8 +26,16 @@ function Component({x, y, z}) { }, [x]); const f = useMemo(() => { return []; - }, [x, y.z, z?.y?.a]); - return ; + }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); + const ref1 = useRef(null); + const ref2 = useRef(null); + const ref = z ? ref1 : ref2; + const cb = useMemo(() => { + return () => { + return ref.current; + }; + }, []); + return ; } ``` @@ -36,11 +44,11 @@ function Component({x, y, z}) { ## Error ``` -Found 4 errors: +Found 5 errors: -Compilation Skipped: Found non-exhaustive dependencies +Error: Found non-exhaustive dependencies -Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. This memoization cannot be safely rewritten by the compiler.. +Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. error.invalid-exhaustive-deps.ts:7:11 5 | function Component({x, y, z}) { @@ -51,9 +59,9 @@ error.invalid-exhaustive-deps.ts:7:11 9 | const b = useMemo(() => { 10 | return x.y.z?.a; -Compilation Skipped: Found non-exhaustive dependencies +Error: Found non-exhaustive dependencies -Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. This memoization cannot be safely rewritten by the compiler.. +Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. error.invalid-exhaustive-deps.ts:10:11 8 | }, [x?.y.z?.a.b]); @@ -64,9 +72,9 @@ error.invalid-exhaustive-deps.ts:10:11 12 | const c = useMemo(() => { 13 | return x?.y.z.a?.b; -Compilation Skipped: Found non-exhaustive dependencies +Error: Found non-exhaustive dependencies -Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. This memoization cannot be safely rewritten by the compiler.. +Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. error.invalid-exhaustive-deps.ts:13:11 11 | }, [x.y.z.a]); @@ -77,22 +85,31 @@ error.invalid-exhaustive-deps.ts:13:11 15 | const d = useMemo(() => { 16 | return x?.y?.[(console.log(y), z?.b)]; -Compilation Skipped: Found unnecessary memoization dependencies - -Unnecessary dependencies can cause a value to update more often than necessary, which can cause effects to run more than expected. This memoization cannot be safely rewritten by the compiler. - -error.invalid-exhaustive-deps.ts:23:20 - 21 | return e; - 22 | }, [x]); -> 23 | const f = useMemo(() => { - | ^^^^^^^ -> 24 | return []; - | ^^^^^^^^^^^^^^ -> 25 | }, [x, y.z, z?.y?.a]); - | ^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a` - 26 | return ; - 27 | } - 28 | +Error: Found unnecessary memoization dependencies + +Unnecessary dependencies can cause a value to update more often than necessary, which can cause effects to run more than expected. + +error.invalid-exhaustive-deps.ts:25:5 + 23 | const f = useMemo(() => { + 24 | return []; +> 25 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a`, `UNUSED_GLOBAL` + 26 | const ref1 = useRef(null); + 27 | const ref2 = useRef(null); + 28 | const ref = z ? ref1 : ref2; + +Error: Found non-exhaustive dependencies + +Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. + +error.invalid-exhaustive-deps.ts:31:13 + 29 | const cb = useMemo(() => { + 30 | return () => { +> 31 | return ref.current; + | ^^^ Missing dependency `ref`. Refs, setState functions, and other "stable" values generally do not need to be added as dependencies, but this variable may change over time to point to different values + 32 | }; + 33 | }, []); + 34 | return ; ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js index 7a16a210a43cd..53c189cc4287c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js @@ -22,6 +22,14 @@ function Component({x, y, z}) { }, [x]); const f = useMemo(() => { return []; - }, [x, y.z, z?.y?.a]); - return ; + }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); + const ref1 = useRef(null); + const ref2 = useRef(null); + const ref = z ? ref1 : ref2; + const cb = useMemo(() => { + return () => { + return ref.current; + }; + }, []); + return ; }