From bcc3fd8b05acc6cb4947b15938dc55b4b72fe31f Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Thu, 20 Nov 2025 19:26:26 -0800 Subject: [PATCH 1/3] [compiler] Implement exhaustive dependency checking for manual memoization (#34394) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compiler currently drops manual memoization and rewrites it using its own inference. If the existing manual memo dependencies has missing or extra dependencies, compilation can change behavior by running the computation more often (if deps were missing) or less often (if there were extra deps). We currently address this by relying on the developer to use the ESLint plugin and have `eslint-disable-next-line react-hooks/exhaustive-deps` suppressions in their code. If a suppression exists, we skip compilation. But not everyone is using the linter! Relying on the linter is also imprecise since it forces us to bail out on exhaustive-deps checks that only effect (ahem) effects — and while it isn't good to have incorrect deps on effects, it isn't a problem for compilation. So this PR is a rough sketch of validating manual memoization dependencies in the compiler. Long-term we could use this to also check effect deps and replace the ExhaustiveDeps lint rule, but for now I'm focused specifically on manual memoization use-cases. If this works, we can stop bailing out on ESLint suppressions, since the compiler will implement all the appropriate checks (we already check rules of hooks). --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34394). * #34472 * #34471 * __->__ #34394 --- .../src/CompilerError.ts | 24 + .../src/Entrypoint/Pipeline.ts | 6 + .../src/HIR/Environment.ts | 5 + .../src/HIR/HIR.ts | 22 + .../ValidateExhaustiveDependencies.ts | 749 ++++++++++++++++++ .../error.invalid-exhaustive-deps.expect.md | 98 +++ .../compiler/error.invalid-exhaustive-deps.js | 27 + .../compiler/exhaustive-deps.expect.md | 159 ++++ .../fixtures/compiler/exhaustive-deps.js | 54 ++ 9 files changed, 1144 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts index 18bcd7791d0..ed64936610f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts @@ -304,6 +304,30 @@ export class CompilerError extends Error { disabledDetails: Array = []; printedMessage: string | null = null; + static simpleInvariant( + condition: unknown, + options: { + reason: CompilerDiagnosticOptions['reason']; + description?: CompilerDiagnosticOptions['description']; + loc: SourceLocation; + }, + ): asserts condition { + if (!condition) { + const errors = new CompilerError(); + errors.pushDiagnostic( + CompilerDiagnostic.create({ + reason: options.reason, + description: options.description ?? null, + category: ErrorCategory.Invariant, + }).withDetails({ + kind: 'error', + loc: options.loc, + message: options.reason, + }), + ); + throw errors; + } + } static invariant( condition: unknown, options: Omit, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 68b609e7f8e..3d97d4ddf04 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -105,6 +105,7 @@ import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDe import {validateNoDerivedComputationsInEffects_exp} from '../Validation/ValidateNoDerivedComputationsInEffects_exp'; import {nameAnonymousFunctions} from '../Transform/NameAnonymousFunctions'; import {optimizeForSSR} from '../Optimization/OptimizeForSSR'; +import {validateExhaustiveDependencies} from '../Validation/ValidateExhaustiveDependencies'; import {validateSourceLocations} from '../Validation/ValidateSourceLocations'; export type CompilerPipelineValue = @@ -302,6 +303,11 @@ function runWithEnvironment( inferReactivePlaces(hir); log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); + if (env.config.validateExhaustiveMemoizationDependencies) { + // NOTE: this relies on reactivity inference running first + validateExhaustiveDependencies(hir).unwrap(); + } + rewriteInstructionKindsBasedOnReassignment(hir); log({ kind: 'hir', diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 2a0266881b0..92bfe913d7e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -218,6 +218,11 @@ export const EnvironmentConfigSchema = z.object({ */ validatePreserveExistingMemoizationGuarantees: z.boolean().default(true), + /** + * Validate that dependencies supplied to manual memoization calls are exhaustive. + */ + validateExhaustiveMemoizationDependencies: z.boolean().default(false), + /** * When this is true, rather than pruning existing manual memoization but ensuring or validating * that the memoized values remain memoized, the compiler will simply not prune existing calls to 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 87ca692a95f..5440035183d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -1680,6 +1680,28 @@ export function areEqualPaths(a: DependencyPath, b: DependencyPath): boolean { ) ); } +export function isSubPath( + subpath: DependencyPath, + path: DependencyPath, +): boolean { + return ( + subpath.length <= path.length && + subpath.every( + (item, ix) => + item.property === path[ix].property && + item.optional === path[ix].optional, + ) + ); +} +export function isSubPathIgnoringOptionals( + subpath: DependencyPath, + path: DependencyPath, +): boolean { + return ( + subpath.length <= path.length && + subpath.every((item, ix) => item.property === path[ix].property) + ); +} export function getPlaceScope( id: InstructionId, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts new file mode 100644 index 00000000000..ad9b83e3927 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts @@ -0,0 +1,749 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import prettyFormat from 'pretty-format'; +import {CompilerDiagnostic, CompilerError, SourceLocation} from '..'; +import {ErrorCategory} from '../CompilerError'; +import { + areEqualPaths, + BlockId, + DependencyPath, + FinishMemoize, + HIRFunction, + Identifier, + IdentifierId, + InstructionKind, + isSubPath, + LoadGlobal, + ManualMemoDependency, + Place, + StartMemoize, +} from '../HIR'; +import { + eachInstructionLValue, + eachInstructionValueLValue, + eachInstructionValueOperand, + eachTerminalOperand, +} from '../HIR/visitors'; +import {Result} from '../Utils/Result'; +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. + * + * 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 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 = + * ...non-DCE'd code for manual deps... + * FinishMemoize decl=t0 + * + * When we go to compute the dependencies, we then think that the user's manual dep + * logic is part of what the memo computation logic. + */ +export function validateExhaustiveDependencies( + fn: HIRFunction, +): Result { + const reactive = collectReactiveIdentifiersHIR(fn); + + const temporaries: Map = new Map(); + for (const param of fn.params) { + const place = param.kind === 'Identifier' ? param : param.place; + temporaries.set(place.identifier.id, { + kind: 'Local', + identifier: place.identifier, + path: [], + context: false, + loc: place.loc, + }); + } + const error = new CompilerError(); + let startMemo: StartMemoize | null = null; + + function onStartMemoize( + value: StartMemoize, + dependencies: Set, + locals: Set, + ): void { + CompilerError.simpleInvariant(startMemo == null, { + reason: 'Unexpected nested memo calls', + loc: value.loc, + }); + startMemo = value; + dependencies.clear(); + locals.clear(); + } + function onFinishMemoize( + value: FinishMemoize, + dependencies: Set, + locals: Set, + ): void { + CompilerError.simpleInvariant( + startMemo != null && startMemo.manualMemoId === value.manualMemoId, + { + reason: 'Found FinishMemoize without corresponding StartMemoize', + loc: value.loc, + }, + ); + visitCandidateDependency(value.decl, temporaries, dependencies, locals); + const inferred: Array = Array.from(dependencies); + // 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); + } else if (a.kind == 'Local' && b.kind == 'Local') { + CompilerError.simpleInvariant( + a.identifier.name != null && + a.identifier.name.kind === 'named' && + b.identifier.name != null && + b.identifier.name.kind === 'named', + { + reason: 'Expected dependencies to be named variables', + loc: a.loc, + }, + ); + if (a.identifier.id !== b.identifier.id) { + return a.identifier.name.value.localeCompare(b.identifier.name.value); + } + if (a.path.length !== b.path.length) { + // if a's path is shorter this returns a negative, sorting a first + return a.path.length - b.path.length; + } + for (let i = 0; i < a.path.length; i++) { + const aProperty = a.path[i]; + const bProperty = b.path[i]; + const aOptional = aProperty.optional ? 0 : 1; + const bOptional = bProperty.optional ? 0 : 1; + if (aOptional !== bOptional) { + // sort non-optionals first + return aOptional - bOptional; + } else if (aProperty.property !== bProperty.property) { + return String(aProperty.property).localeCompare( + String(bProperty.property), + ); + } + } + return 0; + } else { + const aName = + a.kind === 'Global' ? a.binding.name : a.identifier.name?.value; + const bName = + b.kind === 'Global' ? b.binding.name : b.identifier.name?.value; + if (aName != null && bName != null) { + return aName.localeCompare(bName); + } + return 0; + } + }); + // remove redundant inferred dependencies + retainWhere(inferred, (dep, ix) => { + const match = inferred.findIndex(prevDep => { + return ( + isEqualTemporary(prevDep, dep) || + (prevDep.kind === 'Local' && + dep.kind === 'Local' && + prevDep.identifier.id === dep.identifier.id && + isSubPath(prevDep.path, dep.path)) + ); + }); + // only retain entries that don't have a prior match + return match === -1 || match >= ix; + }); + // Validate that all manual dependencies belong there + if (DEBUG) { + console.log('manual'); + console.log( + (startMemo.deps ?? []) + .map(x => ' ' + printManualMemoDependency(x)) + .join('\n'), + ); + console.log('inferred'); + console.log( + inferred.map(x => ' ' + printInferredDependency(x)).join('\n'), + ); + } + const manualDependencies = startMemo.deps ?? []; + const matched: Set = new Set(); + const missing: Array> = []; + const extra: Array = []; + for (const inferredDependency of inferred) { + if (inferredDependency.kind === 'Global') { + for (const manualDependency of manualDependencies) { + if ( + manualDependency.root.kind === 'Global' && + manualDependency.root.identifierName === + inferredDependency.binding.name + ) { + matched.add(manualDependency); + extra.push(manualDependency); + } + } + continue; + } + CompilerError.simpleInvariant(inferredDependency.kind === 'Local', { + reason: 'Unexpected function dependency', + loc: value.loc, + }); + let hasMatchingManualDependency = false; + for (const manualDependency of manualDependencies) { + if ( + manualDependency.root.kind === 'NamedLocal' && + manualDependency.root.value.identifier.id === + inferredDependency.identifier.id && + (areEqualPaths(manualDependency.path, inferredDependency.path) || + isSubPath(manualDependency.path, inferredDependency.path)) + ) { + hasMatchingManualDependency = true; + matched.add(manualDependency); + } + } + if (!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)) + ) { + continue; + } + extra.push(dep); + } + + if (missing.length !== 0) { + // Error + const diagnostic = CompilerDiagnostic.create({ + category: ErrorCategory.PreserveManualMemo, + 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.', + }); + for (const dep of missing) { + diagnostic.withDetails({ + kind: 'error', + message: `Missing dependency \`${printInferredDependency(dep)}\``, + loc: dep.loc, + }); + } + error.pushDiagnostic(diagnostic); + } else if (extra.length !== 0) { + const diagnostic = CompilerDiagnostic.create({ + category: ErrorCategory.PreserveManualMemo, + 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', + }); + diagnostic.withDetails({ + kind: 'error', + message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`, + loc: value.loc, + }); + error.pushDiagnostic(diagnostic); + } + + dependencies.clear(); + locals.clear(); + startMemo = null; + } + + collectDependencies(fn, temporaries, { + onStartMemoize, + onFinishMemoize, + }); + return error.asResult(); +} + +function addDependency( + dep: Temporary, + dependencies: Set, + locals: Set, +): void { + if (dep.kind === 'Function') { + for (const x of dep.dependencies) { + addDependency(x, dependencies, locals); + } + } else if (dep.kind === 'Global') { + dependencies.add(dep); + } else if (!locals.has(dep.identifier.id)) { + dependencies.add(dep); + } +} + +function visitCandidateDependency( + place: Place, + temporaries: Map, + dependencies: Set, + locals: Set, +): void { + const dep = temporaries.get(place.identifier.id); + if (dep != null) { + addDependency(dep, dependencies, locals); + } +} + +/** + * This function determines the dependencies of the given function relative to + * its external context. Dependencies are collected eagerly, the first time an + * external variable is referenced, as opposed to trying to delay or aggregate + * calculation of dependencies until they are later "used". + * + * For example, in + * + * ``` + * function f() { + * let x = y; // we record a dependency on `y` here + * ... + * use(x); // as opposed to trying to delay that dependency until here + * } + * ``` + * + * That said, LoadLocal/LoadContext does not immediately take a dependency, + * we store the dependency in a temporary and set it as used when that temporary + * is referenced as an operand. + * + * As we proceed through the function we track local variables that it creates + * and don't consider later references to these variables as dependencies. + * + * For function expressions we first collect the function's dependencies by + * calling this function recursively, _without_ taking into account whether + * the "external" variables it accesses are actually external or just locals + * in the parent. We then prune any locals and immediately consider any + * remaining externals that it accesses as a dependency: + * + * ``` + * function Component() { + * const local = ...; + * const f = () => { return [external, local] }; + * } + * ``` + * + * Here we calculate `f` as having dependencies `external, `local` and save + * this into `temporaries`. We then also immediately take these as dependencies + * at the Component scope, at which point we filter out `local` as a local variable, + * leaving just a dependency on `external`. + * + * When calling this function on a top-level component or hook, the collected dependencies + * will only contain the globals that it accesses which isn't useful. Instead, passing + * onStartMemoize/onFinishMemoize callbacks allows looking at the dependencies within + * blocks of manual memoization. + */ +function collectDependencies( + fn: HIRFunction, + temporaries: Map, + callbacks: { + onStartMemoize: ( + startMemo: StartMemoize, + dependencies: Set, + locals: Set, + ) => void; + onFinishMemoize: ( + finishMemo: FinishMemoize, + dependencies: Set, + locals: Set, + ) => void; + } | null, +): Extract { + const optionals = findOptionalPlaces(fn); + if (DEBUG) { + console.log(prettyFormat(optionals)); + } + const locals: Set = new Set(); + const dependencies: Set = new Set(); + function visit(place: Place): void { + visitCandidateDependency(place, temporaries, dependencies, locals); + } + for (const block of fn.body.blocks.values()) { + for (const phi of block.phis) { + let deps: Array | null = null; + for (const operand of phi.operands.values()) { + const dep = temporaries.get(operand.identifier.id); + if (dep == null) { + continue; + } + if (deps == null) { + deps = [dep]; + } else { + deps.push(dep); + } + } + if (deps == null) { + continue; + } else if (deps.length === 1) { + temporaries.set(phi.place.identifier.id, deps[0]!); + } else { + temporaries.set(phi.place.identifier.id, { + kind: 'Function', + dependencies: new Set(deps), + }); + } + } + + for (const instr of block.instructions) { + const {lvalue, value} = instr; + switch (value.kind) { + case 'LoadGlobal': { + temporaries.set(lvalue.identifier.id, { + kind: 'Global', + binding: value.binding, + }); + break; + } + case 'LoadContext': + case 'LoadLocal': { + if (locals.has(value.place.identifier.id)) { + break; + } + const temp = temporaries.get(value.place.identifier.id); + if (temp != null) { + if (temp.kind === 'Local') { + const local: Temporary = {...temp, loc: value.place.loc}; + temporaries.set(lvalue.identifier.id, local); + } else { + temporaries.set(lvalue.identifier.id, temp); + } + } + break; + } + case 'DeclareLocal': { + const local: Temporary = { + kind: 'Local', + identifier: value.lvalue.place.identifier, + path: [], + context: false, + loc: value.lvalue.place.loc, + }; + temporaries.set(value.lvalue.place.identifier.id, local); + locals.add(value.lvalue.place.identifier.id); + break; + } + case 'StoreLocal': { + if (value.lvalue.place.identifier.name == null) { + const temp = temporaries.get(value.value.identifier.id); + if (temp != null) { + temporaries.set(value.lvalue.place.identifier.id, temp); + } + break; + } + visit(value.value); + if (value.lvalue.kind !== InstructionKind.Reassign) { + const local: Temporary = { + kind: 'Local', + identifier: value.lvalue.place.identifier, + path: [], + context: false, + loc: value.lvalue.place.loc, + }; + temporaries.set(value.lvalue.place.identifier.id, local); + locals.add(value.lvalue.place.identifier.id); + } + break; + } + case 'DeclareContext': { + const local: Temporary = { + kind: 'Local', + identifier: value.lvalue.place.identifier, + path: [], + context: true, + loc: value.lvalue.place.loc, + }; + temporaries.set(value.lvalue.place.identifier.id, local); + break; + } + case 'StoreContext': { + visit(value.value); + if (value.lvalue.kind !== InstructionKind.Reassign) { + const local: Temporary = { + kind: 'Local', + identifier: value.lvalue.place.identifier, + path: [], + context: true, + loc: value.lvalue.place.loc, + }; + temporaries.set(value.lvalue.place.identifier.id, local); + locals.add(value.lvalue.place.identifier.id); + } + break; + } + case 'Destructure': { + visit(value.value); + if (value.lvalue.kind !== InstructionKind.Reassign) { + for (const lvalue of eachInstructionValueLValue(value)) { + const local: Temporary = { + kind: 'Local', + identifier: lvalue.identifier, + path: [], + context: false, + loc: lvalue.loc, + }; + temporaries.set(lvalue.identifier.id, local); + locals.add(lvalue.identifier.id); + } + } + break; + } + case 'PropertyLoad': { + if (typeof value.property === 'number') { + visit(value.object); + break; + } + const object = temporaries.get(value.object.identifier.id); + if (object != null && object.kind === 'Local') { + const optional = optionals.get(value.object.identifier.id) ?? false; + const local: Temporary = { + kind: 'Local', + identifier: object.identifier, + context: object.context, + path: [ + ...object.path, + { + optional, + property: value.property, + }, + ], + loc: value.loc, + }; + temporaries.set(lvalue.identifier.id, local); + } + break; + } + case 'FunctionExpression': + case 'ObjectMethod': { + const functionDeps = collectDependencies( + value.loweredFunc.func, + temporaries, + null, + ); + temporaries.set(lvalue.identifier.id, functionDeps); + addDependency(functionDeps, dependencies, locals); + break; + } + case 'StartMemoize': { + const onStartMemoize = callbacks?.onStartMemoize; + if (onStartMemoize != null) { + onStartMemoize(value, dependencies, locals); + } + break; + } + case 'FinishMemoize': { + const onFinishMemoize = callbacks?.onFinishMemoize; + if (onFinishMemoize != null) { + onFinishMemoize(value, dependencies, locals); + } + break; + } + case 'MethodCall': { + // Ignore the method itself + for (const operand of eachInstructionValueOperand(value)) { + if (operand.identifier.id === value.property.identifier.id) { + continue; + } + visit(operand); + } + break; + } + default: { + for (const operand of eachInstructionValueOperand(value)) { + visit(operand); + } + for (const lvalue of eachInstructionLValue(instr)) { + locals.add(lvalue.identifier.id); + } + } + } + } + for (const operand of eachTerminalOperand(block.terminal)) { + if (optionals.has(operand.identifier.id)) { + continue; + } + visit(operand); + } + } + return {kind: 'Function', dependencies}; +} + +function printInferredDependency(dep: InferredDependency): string { + switch (dep.kind) { + case 'Global': { + return dep.binding.name; + } + case 'Local': { + CompilerError.simpleInvariant( + dep.identifier.name != null && dep.identifier.name.kind === 'named', + { + reason: 'Expected dependencies to be named variables', + loc: dep.loc, + }, + ); + return `${dep.identifier.name.value}${dep.path.map(p => (p.optional ? '?' : '') + '.' + p.property).join('')}`; + } + } +} + +function printManualMemoDependency(dep: ManualMemoDependency): string { + let identifierName: string; + if (dep.root.kind === 'Global') { + identifierName = dep.root.identifierName; + } else { + const name = dep.root.value.identifier.name; + CompilerError.simpleInvariant(name != null && name.kind === 'named', { + reason: 'Expected manual dependencies to be named variables', + loc: dep.root.value.loc, + }); + identifierName = name.value; + } + return `${identifierName}${dep.path.map(p => (p.optional ? '?' : '') + '.' + p.property).join('')}`; +} + +function isEqualTemporary(a: Temporary, b: Temporary): boolean { + switch (a.kind) { + case 'Function': { + return false; + } + case 'Global': { + return b.kind === 'Global' && a.binding.name === b.binding.name; + } + case 'Local': { + return ( + b.kind === 'Local' && + a.identifier.id === b.identifier.id && + areEqualPaths(a.path, b.path) + ); + } + } +} + +type Temporary = + | {kind: 'Global'; binding: LoadGlobal['binding']} + | { + kind: 'Local'; + identifier: Identifier; + path: DependencyPath; + context: boolean; + loc: SourceLocation; + } + | {kind: 'Function'; dependencies: Set}; +type InferredDependency = Extract; + +function collectReactiveIdentifiersHIR(fn: HIRFunction): Set { + const reactive = new Set(); + for (const block of fn.body.blocks.values()) { + for (const instr of block.instructions) { + for (const lvalue of eachInstructionLValue(instr)) { + if (lvalue.reactive) { + reactive.add(lvalue.identifier.id); + } + } + for (const operand of eachInstructionValueOperand(instr.value)) { + if (operand.reactive) { + reactive.add(operand.identifier.id); + } + } + } + for (const operand of eachTerminalOperand(block.terminal)) { + if (operand.reactive) { + reactive.add(operand.identifier.id); + } + } + } + return reactive; +} + +export function findOptionalPlaces( + fn: HIRFunction, +): Map { + const optionals = new Map(); + const visited: Set = new Set(); + for (const [, block] of fn.body.blocks) { + if (visited.has(block.id)) { + continue; + } + if (block.terminal.kind === 'optional') { + visited.add(block.id); + const optionalTerminal = block.terminal; + let testBlock = fn.body.blocks.get(block.terminal.test)!; + const queue: Array = [block.terminal.optional]; + loop: while (true) { + visited.add(testBlock.id); + const terminal = testBlock.terminal; + switch (terminal.kind) { + case 'branch': { + const isOptional = queue.pop(); + CompilerError.simpleInvariant(isOptional !== undefined, { + reason: + 'Expected an optional value for each optional test condition', + loc: terminal.test.loc, + }); + if (isOptional != null) { + optionals.set(terminal.test.identifier.id, isOptional); + } + if (terminal.fallthrough === optionalTerminal.fallthrough) { + // found it + const consequent = fn.body.blocks.get(terminal.consequent)!; + const last = consequent.instructions.at(-1); + if (last !== undefined && last.value.kind === 'StoreLocal') { + if (isOptional != null) { + optionals.set(last.value.value.identifier.id, isOptional); + } + } + break loop; + } else { + testBlock = fn.body.blocks.get(terminal.fallthrough)!; + } + break; + } + case 'optional': { + queue.push(terminal.optional); + testBlock = fn.body.blocks.get(terminal.test)!; + break; + } + case 'logical': + case 'ternary': { + queue.push(null); + testBlock = fn.body.blocks.get(terminal.test)!; + break; + } + + case 'sequence': { + // Do we need sequence?? In any case, don't push to queue bc there is no corresponding branch terminal + testBlock = fn.body.blocks.get(terminal.block)!; + break; + } + default: { + CompilerError.simpleInvariant(false, { + reason: `Unexpected terminal in optional`, + loc: terminal.loc, + }); + } + } + } + CompilerError.simpleInvariant(queue.length === 0, { + reason: + 'Expected a matching number of conditional blocks and branch points', + loc: block.terminal.loc, + }); + } + } + return optionals; +} 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 new file mode 100644 index 00000000000..3af5cc2d6b4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md @@ -0,0 +1,98 @@ + +## Input + +```javascript +// @validateExhaustiveMemoizationDependencies +import {useMemo} from 'react'; +import {Stringify} from 'shared-runtime'; + +function Component({x, y, z}) { + const a = useMemo(() => { + return x?.y.z?.a; + }, [x?.y.z?.a.b]); + const b = useMemo(() => { + return x.y.z?.a; + }, [x.y.z.a]); + const c = useMemo(() => { + return x?.y.z.a?.b; + }, [x?.y.z.a?.b.z]); + const d = useMemo(() => { + return x?.y?.[(console.log(y), z?.b)]; + }, [x?.y, y, z?.b]); + const e = useMemo(() => { + const e = []; + e.push(x); + return e; + }, [x]); + const f = useMemo(() => { + return []; + }, [x, y.z, z?.y?.a]); + return ; +} + +``` + + +## Error + +``` +Found 4 errors: + +Compilation Skipped: 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.. + +error.invalid-exhaustive-deps.ts:7:11 + 5 | function Component({x, y, z}) { + 6 | const a = useMemo(() => { +> 7 | return x?.y.z?.a; + | ^^^^^^^^^ Missing dependency `x?.y.z?.a` + 8 | }, [x?.y.z?.a.b]); + 9 | const b = useMemo(() => { + 10 | return x.y.z?.a; + +Compilation Skipped: 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.. + +error.invalid-exhaustive-deps.ts:10:11 + 8 | }, [x?.y.z?.a.b]); + 9 | const b = useMemo(() => { +> 10 | return x.y.z?.a; + | ^^^^^^^^ Missing dependency `x.y.z?.a` + 11 | }, [x.y.z.a]); + 12 | const c = useMemo(() => { + 13 | return x?.y.z.a?.b; + +Compilation Skipped: 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.. + +error.invalid-exhaustive-deps.ts:13:11 + 11 | }, [x.y.z.a]); + 12 | const c = useMemo(() => { +> 13 | return x?.y.z.a?.b; + | ^^^^^^^^^^^ Missing dependency `x?.y.z.a?.b` + 14 | }, [x?.y.z.a?.b.z]); + 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 | +``` + + \ 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 new file mode 100644 index 00000000000..7a16a210a43 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js @@ -0,0 +1,27 @@ +// @validateExhaustiveMemoizationDependencies +import {useMemo} from 'react'; +import {Stringify} from 'shared-runtime'; + +function Component({x, y, z}) { + const a = useMemo(() => { + return x?.y.z?.a; + }, [x?.y.z?.a.b]); + const b = useMemo(() => { + return x.y.z?.a; + }, [x.y.z.a]); + const c = useMemo(() => { + return x?.y.z.a?.b; + }, [x?.y.z.a?.b.z]); + const d = useMemo(() => { + return x?.y?.[(console.log(y), z?.b)]; + }, [x?.y, y, z?.b]); + const e = useMemo(() => { + const e = []; + e.push(x); + return e; + }, [x]); + const f = useMemo(() => { + return []; + }, [x, y.z, z?.y?.a]); + return ; +} 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 new file mode 100644 index 00000000000..655d6bff415 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md @@ -0,0 +1,159 @@ + +## Input + +```javascript +// @validateExhaustiveMemoizationDependencies +import {useMemo} from 'react'; +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function useHook1(x) { + return useMemo(() => { + return x?.y.z?.a; + }, [x?.y.z?.a]); +} +function useHook2(x) { + useMemo(() => { + return x.y.z?.a; + }, [x.y.z?.a]); +} +function useHook3(x) { + return useMemo(() => { + return x?.y.z.a?.b; + }, [x?.y.z.a?.b]); +} +function useHook4(x, y, z) { + return useMemo(() => { + return x?.y?.[(console.log(y), z?.b)]; + }, [x?.y, y, z?.b]); +} +function useHook5(x) { + return useMemo(() => { + const e = []; + const local = makeObject_Primitives(x); + const fn = () => { + e.push(local); + }; + fn(); + return e; + }, [x]); +} +function useHook6(x) { + return useMemo(() => { + const f = []; + f.push(x.y.z); + f.push(x.y); + f.push(x); + return f; + }, [x]); +} + +function Component({x, y, z}) { + const a = useHook1(x); + const b = useHook2(x); + const c = useHook3(x); + const d = useHook4(x, y, z); + const e = useHook5(x); + const f = useHook6(x); + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies +import { useMemo } from "react"; +import { makeObject_Primitives, Stringify } from "shared-runtime"; + +function useHook1(x) { + x?.y.z?.a; + return x?.y.z?.a; +} + +function useHook2(x) { + x.y.z?.a; +} + +function useHook3(x) { + x?.y.z.a?.b; + return x?.y.z.a?.b; +} + +function useHook4(x, y, z) { + x?.y; + z?.b; + return x?.y?.[(console.log(y), z?.b)]; +} + +function useHook5(x) { + const $ = _c(2); + let e; + if ($[0] !== x) { + e = []; + const local = makeObject_Primitives(x); + const fn = () => { + e.push(local); + }; + + fn(); + $[0] = x; + $[1] = e; + } else { + e = $[1]; + } + return e; +} + +function useHook6(x) { + const $ = _c(2); + let f; + if ($[0] !== x) { + f = []; + f.push(x.y.z); + f.push(x.y); + f.push(x); + $[0] = x; + $[1] = f; + } else { + f = $[1]; + } + return f; +} + +function Component(t0) { + const $ = _c(7); + const { x, y, z } = t0; + const a = useHook1(x); + const b = useHook2(x); + const c = useHook3(x); + const d = useHook4(x, y, z); + const e = useHook5(x); + const f = useHook6(x); + let t1; + if ( + $[0] !== a || + $[1] !== b || + $[2] !== c || + $[3] !== d || + $[4] !== e || + $[5] !== f + ) { + t1 = ; + $[0] = a; + $[1] = b; + $[2] = c; + $[3] = d; + $[4] = e; + $[5] = f; + $[6] = t1; + } else { + t1 = $[6]; + } + return t1; +} + +``` + +### 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.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js new file mode 100644 index 00000000000..d4c833c7d29 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js @@ -0,0 +1,54 @@ +// @validateExhaustiveMemoizationDependencies +import {useMemo} from 'react'; +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function useHook1(x) { + return useMemo(() => { + return x?.y.z?.a; + }, [x?.y.z?.a]); +} +function useHook2(x) { + useMemo(() => { + return x.y.z?.a; + }, [x.y.z?.a]); +} +function useHook3(x) { + return useMemo(() => { + return x?.y.z.a?.b; + }, [x?.y.z.a?.b]); +} +function useHook4(x, y, z) { + return useMemo(() => { + return x?.y?.[(console.log(y), z?.b)]; + }, [x?.y, y, z?.b]); +} +function useHook5(x) { + return useMemo(() => { + const e = []; + const local = makeObject_Primitives(x); + const fn = () => { + e.push(local); + }; + fn(); + return e; + }, [x]); +} +function useHook6(x) { + return useMemo(() => { + const f = []; + f.push(x.y.z); + f.push(x.y); + f.push(x); + return f; + }, [x]); +} + +function Component({x, y, z}) { + const a = useHook1(x); + const b = useHook2(x); + const c = useHook3(x); + const d = useHook4(x, y, z); + const e = useHook5(x); + const f = useHook6(x); + return ; +} From df75af4edca7f316e6bfcfcde67197e8a57d1101 Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Thu, 20 Nov 2025 19:28:08 -0800 Subject: [PATCH 2/3] [compiler] Auto-fix for non-exhaustive deps (#34471) Records more information in DropManualMemoization so that we know the full span of the manual dependencies array (if present). This allows ValidateExhaustiveDeps to include a suggestion with the correct deps. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34471). * #34472 * __->__ #34471 --- .../src/HIR/HIR.ts | 5 + .../src/Inference/DropManualMemoization.ts | 103 +++++++++--------- .../ValidateExhaustiveDependencies.ts | 79 +++++++++----- 3 files changed, 107 insertions(+), 80 deletions(-) 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 5440035183d..55bcd0bc5ce 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -817,6 +817,11 @@ export type StartMemoize = { * (e.g. useMemo without a second arg) */ deps: Array | null; + /** + * The source location of the dependencies argument. Used for + * emitting diagnostics with a suggested replacement + */ + depsLoc: SourceLocation | null; loc: SourceLocation; }; export type FinishMemoize = { 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 0876424e28c..bcfc53413ab 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts @@ -42,7 +42,7 @@ type IdentifierSidemap = { functions: Map>; manualMemos: Map; react: Set; - maybeDepsLists: Map>; + maybeDepsLists: Map}>; maybeDeps: Map; optionals: Set; }; @@ -159,10 +159,10 @@ function collectTemporaries( } case 'ArrayExpression': { if (value.elements.every(e => e.kind === 'Identifier')) { - sidemap.maybeDepsLists.set( - instr.lvalue.identifier.id, - value.elements as Array, - ); + sidemap.maybeDepsLists.set(instr.lvalue.identifier.id, { + loc: value.loc, + deps: value.elements as Array, + }); } break; } @@ -182,6 +182,7 @@ function makeManualMemoizationMarkers( fnExpr: Place, env: Environment, depsList: Array | null, + depsLoc: SourceLocation | null, memoDecl: Place, manualMemoId: number, ): [TInstruction, TInstruction] { @@ -197,6 +198,7 @@ function makeManualMemoizationMarkers( * as dependencies */ deps: depsList, + depsLoc, loc: fnExpr.loc, }, effects: null, @@ -287,86 +289,85 @@ function extractManualMemoizationArgs( sidemap: IdentifierSidemap, errors: CompilerError, ): { - fnPlace: Place | null; + fnPlace: Place; depsList: Array | null; -} { + depsLoc: SourceLocation | null; +} | null { const [fnPlace, depsListPlace] = instr.value.args as Array< Place | SpreadPattern | undefined >; - if (fnPlace == null) { + if (fnPlace == null || fnPlace.kind !== 'Identifier') { errors.pushDiagnostic( CompilerDiagnostic.create({ category: ErrorCategory.UseMemo, reason: `Expected a callback function to be passed to ${kind}`, - description: `Expected a callback function to be passed to ${kind}`, + description: + kind === 'useCallback' + ? 'The first argument to useCallback() must be a function to cache' + : 'The first argument to useMemo() must be a function that calculates a result to cache', suggestions: null, }).withDetails({ kind: 'error', loc: instr.value.loc, - message: `Expected a callback function to be passed to ${kind}`, + message: + kind === 'useCallback' + ? `Expected a callback function` + : `Expected a memoization function`, }), ); - return {fnPlace: null, depsList: null}; + return null; } - if (fnPlace.kind === 'Spread' || depsListPlace?.kind === 'Spread') { + if (depsListPlace == null) { + return { + fnPlace, + depsList: null, + depsLoc: null, + }; + } + const maybeDepsList = + depsListPlace.kind === 'Identifier' + ? sidemap.maybeDepsLists.get(depsListPlace.identifier.id) + : null; + if (maybeDepsList == null) { errors.pushDiagnostic( CompilerDiagnostic.create({ category: ErrorCategory.UseMemo, - reason: `Unexpected spread argument to ${kind}`, - description: `Unexpected spread argument to ${kind}`, + reason: `Expected the dependency list for ${kind} to be an array literal`, + description: `Expected the dependency list for ${kind} to be an array literal`, suggestions: null, }).withDetails({ kind: 'error', - loc: instr.value.loc, - message: `Unexpected spread argument to ${kind}`, + loc: + depsListPlace?.kind === 'Identifier' ? depsListPlace.loc : instr.loc, + message: `Expected the dependency list for ${kind} to be an array literal`, }), ); - return {fnPlace: null, depsList: null}; + return null; } - let depsList: Array | null = null; - if (depsListPlace != null) { - const maybeDepsList = sidemap.maybeDepsLists.get( - depsListPlace.identifier.id, - ); - if (maybeDepsList == null) { + const depsList: Array = []; + for (const dep of maybeDepsList.deps) { + const maybeDep = sidemap.maybeDeps.get(dep.identifier.id); + if (maybeDep == null) { errors.pushDiagnostic( CompilerDiagnostic.create({ category: ErrorCategory.UseMemo, - reason: `Expected the dependency list for ${kind} to be an array literal`, - description: `Expected the dependency list for ${kind} to be an array literal`, + reason: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, + description: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, suggestions: null, }).withDetails({ kind: 'error', - loc: depsListPlace.loc, - message: `Expected the dependency list for ${kind} to be an array literal`, + loc: dep.loc, + message: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, }), ); - return {fnPlace, depsList: null}; - } - depsList = []; - for (const dep of maybeDepsList) { - const maybeDep = sidemap.maybeDeps.get(dep.identifier.id); - if (maybeDep == null) { - errors.pushDiagnostic( - CompilerDiagnostic.create({ - category: ErrorCategory.UseMemo, - reason: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, - description: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, - suggestions: null, - }).withDetails({ - kind: 'error', - loc: dep.loc, - message: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, - }), - ); - } else { - depsList.push(maybeDep); - } + } else { + depsList.push(maybeDep); } } return { fnPlace, depsList, + depsLoc: maybeDepsList.loc, }; } @@ -427,16 +428,17 @@ export function dropManualMemoization( const manualMemo = sidemap.manualMemos.get(id); if (manualMemo != null) { - const {fnPlace, depsList} = extractManualMemoizationArgs( + const memoDetails = extractManualMemoizationArgs( instr as TInstruction | TInstruction, manualMemo.kind, sidemap, errors, ); - if (fnPlace == null) { + if (memoDetails == null) { continue; } + const {fnPlace, depsList, depsLoc} = memoDetails; instr.value = getManualMemoizationReplacement( fnPlace, @@ -487,6 +489,7 @@ export function dropManualMemoization( fnPlace, func.env, depsList, + depsLoc, memoDecl, nextManualMemoId++, ); 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 ad9b83e3927..fb1f9445aa0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts @@ -6,8 +6,13 @@ */ import prettyFormat from 'pretty-format'; -import {CompilerDiagnostic, CompilerError, SourceLocation} from '..'; -import {ErrorCategory} from '../CompilerError'; +import { + CompilerDiagnostic, + CompilerError, + CompilerSuggestionOperation, + SourceLocation, +} from '..'; +import {CompilerSuggestion, ErrorCategory} from '../CompilerError'; import { areEqualPaths, BlockId, @@ -229,38 +234,52 @@ export function validateExhaustiveDependencies( extra.push(dep); } - if (missing.length !== 0) { - // Error - const diagnostic = CompilerDiagnostic.create({ - category: ErrorCategory.PreserveManualMemo, - 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.', - }); - for (const dep of missing) { + if (missing.length !== 0 || extra.length !== 0) { + let suggestions: Array | null = null; + if (startMemo.depsLoc != null && typeof startMemo.depsLoc !== 'symbol') { + suggestions = [ + { + description: 'Update dependencies', + range: [startMemo.depsLoc.start.index, startMemo.depsLoc.end.index], + op: CompilerSuggestionOperation.Replace, + text: `[${inferred.map(printInferredDependency).join(', ')}]`, + }, + ]; + } + if (missing.length !== 0) { + // Error + const diagnostic = CompilerDiagnostic.create({ + category: ErrorCategory.PreserveManualMemo, + 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.', + suggestions, + }); + for (const dep of missing) { + diagnostic.withDetails({ + kind: 'error', + message: `Missing dependency \`${printInferredDependency(dep)}\``, + loc: dep.loc, + }); + } + error.pushDiagnostic(diagnostic); + } else if (extra.length !== 0) { + const diagnostic = CompilerDiagnostic.create({ + category: ErrorCategory.PreserveManualMemo, + 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', + }); diagnostic.withDetails({ kind: 'error', - message: `Missing dependency \`${printInferredDependency(dep)}\``, - loc: dep.loc, + message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`, + loc: value.loc, }); + error.pushDiagnostic(diagnostic); } - error.pushDiagnostic(diagnostic); - } else if (extra.length !== 0) { - const diagnostic = CompilerDiagnostic.create({ - category: ErrorCategory.PreserveManualMemo, - 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', - }); - diagnostic.withDetails({ - kind: 'error', - message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`, - loc: value.loc, - }); - error.pushDiagnostic(diagnostic); } dependencies.clear(); From 40b4a5bf71ba7864556a5589b270b237f453c032 Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Thu, 20 Nov 2025 19:30:35 -0800 Subject: [PATCH 3/3] [compiler] ValidateExhaustiveDeps disallows unnecessary non-reactive deps (#34472) Just to be consistent, we disallow unnecessary deps even if they're known to be non-reactive. --- .../src/CompilerError.ts | 17 +++- .../ValidateExhaustiveDependencies.ts | 94 ++++++++++++++----- .../error.invalid-exhaustive-deps.expect.md | 67 ++++++++----- .../compiler/error.invalid-exhaustive-deps.js | 12 ++- 4 files changed, 139 insertions(+), 51 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts index ed64936610f..352d31b2f89 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 fb1f9445aa0..313f773e294 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 3af5cc2d6b4..5152f4121a2 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 7a16a210a43..53c189cc428 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 ; }