From 2f2d9b73f8e41d68deb2e2ccdb4e842bff6f0d92 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 4 Sep 2025 12:08:59 -0700 Subject: [PATCH] [compiler] Implement exhaustive dependency checking for manual memoization 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). --- .../src/Entrypoint/Pipeline.ts | 3 + .../ValidateExhaustiveDependencies.ts | 362 ++++++++++++++++++ .../fixtures/compiler/exhaustive-deps.js | 12 + 3 files changed, 377 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/exhaustive-deps.js 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 746da01b33..1472050c83 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -103,6 +103,7 @@ import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoF import {inferMutationAliasingEffects} from '../Inference/InferMutationAliasingEffects'; import {inferMutationAliasingRanges} from '../Inference/InferMutationAliasingRanges'; import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDerivedComputationsInEffects'; +import {validateExhaustiveDependencies} from '../Validation/ValidateExhaustiveDependencies'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -294,6 +295,8 @@ function runWithEnvironment( inferReactivePlaces(hir); log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); + validateExhaustiveDependencies(hir).unwrap(); + rewriteInstructionKindsBasedOnReassignment(hir); log({ kind: 'hir', 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 0000000000..8f0ffc1c35 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts @@ -0,0 +1,362 @@ +/** + * 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, ErrorSeverity} from '..'; +import {ErrorCategory} from '../CompilerError'; +import { + areEqualPaths, + DependencyPath, + HIRFunction, + Identifier, + IdentifierId, + InstructionKind, + LoadGlobal, + makePropertyLiteral, + Place, + StartMemoize, +} from '../HIR'; +import { + printIdentifier, + printManualMemoDependency, + printPlace, +} from '../HIR/PrintHIR'; +import { + eachInstructionLValue, + eachInstructionValueLValue, + eachInstructionValueOperand, + eachTerminalOperand, +} from '../HIR/visitors'; +import {Result} from '../Utils/Result'; + +/** + * 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. + */ +export function validateExhaustiveDependencies( + fn: HIRFunction, +): Result { + 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: [], + reactive: place.reactive, + }); + } + collectTemporaries(fn, temporaries); + + const dependencies = new Set(); + const locals = new Set(); + function visit(place: Place): void { + const dep = temporaries.get(place.identifier.id); + if (dep != null && !locals.has(place.identifier.id)) { + if (dep.kind === 'Function') { + dep.dependencies.forEach(x => dependencies.add(x)); + } else { + dependencies.add(dep); + } + } + } + + const error = new CompilerError(); + let startMemo: StartMemoize | null = null; + for (const block of fn.body.blocks.values()) { + for (const instr of block.instructions) { + const {lvalue, value} = instr; + switch (value.kind) { + case 'StartMemoize': { + CompilerError.invariant(startMemo == null, { + reason: 'Unexpected nested memo calls', + loc: value.loc, + }); + startMemo = value; + dependencies.clear(); + locals.clear(); + break; + } + case 'FinishMemoize': { + CompilerError.invariant( + startMemo != null && startMemo.manualMemoId === value.manualMemoId, + { + reason: 'Found FinishMemoize without corresponding StartMemoize', + loc: value.loc, + }, + ); + visit(value.decl); + const inferred = [...dependencies]; + // Validate that all manual dependencies belong there + for (const dep of startMemo.deps ?? []) { + if (dep.root.kind === 'Global') { + const root = dep.root.identifierName; + const match = inferred.find( + d => d.kind === 'Global' && d.binding.name === root, + ); + if (match == null) { + error.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.PreserveManualMemo, + severity: ErrorSeverity.InvalidReact, + reason: 'Found unnecessary memoization dependency', + description: + 'Adding unnecessary memoization dependencies can cause a value to recompute ' + + 'more often than necessary and change behavior. This memoization cannot be safely rewritten by the compiler.', + }).withDetail({ + kind: 'error', + message: 'Unnecessary dependency', + loc: startMemo.loc, + }), + ); + } else { + console.log(printManualMemoDependency(dep, false)); + console.log(printTemporary(match)); + } + } else { + if (!dep.root.value.reactive) { + continue; + } + const root = dep.root.value; + const match = inferred.find( + d => + d.kind === 'Local' && + d.identifier.id === root.identifier.id && + areEqualPaths(d.path, dep.path), + ); + if (match == null) { + console.log(printManualMemoDependency(dep, false)); + console.log(inferred.map(printTemporary).join('\n')); + console.log( + prettyFormat( + new Map( + [...temporaries].map(([k, v]) => [k, printTemporary(v)]), + ), + ), + ); + error.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.PreserveManualMemo, + severity: ErrorSeverity.InvalidReact, + reason: 'Found unnecessary memoization dependency', + description: + 'Adding unnecessary memoization dependencies can cause a value to recompute ' + + 'more often than necessary and change behavior. This memoization cannot be safely rewritten by the compiler.', + }).withDetail({ + kind: 'error', + message: 'Unnecessary dependency', + loc: dep.root.value.loc, + }), + ); + } else { + console.log(printManualMemoDependency(dep, false)); + console.log(printTemporary(match)); + } + } + } + // TODO: validate that all inferred dependencies were in manual deps list too + dependencies.clear(); + locals.clear(); + startMemo = null; + break; + } + default: { + if (startMemo == null) { + continue; + } + if (temporaries.has(lvalue.identifier.id)) { + // This instruction produces a temporary, delay recording until the temporary is consumed + } else { + 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)) { + visit(operand); + } + } + return error.asResult(); +} + +function collectTemporaries( + fn: HIRFunction, + temporaries: Map, +): Extract { + const locals: Set = new Set(); + const dependencies: Set = new Set(); + function visit(operand: Place): void { + const temp = temporaries.get(operand.identifier.id); + if (temp != null && !locals.has(operand.identifier.id)) { + dependencies.add(temp); + } + } + for (const block of fn.body.blocks.values()) { + 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 'LoadLocal': { + const temp = temporaries.get(value.place.identifier.id); + if (temp != null) { + temporaries.set(lvalue.identifier.id, temp); + } + break; + } + case 'DeclareLocal': { + const local: Temporary = { + kind: 'Local', + identifier: value.lvalue.place.identifier, + path: [], + reactive: false, // TODO: need to know if the value is ultimately reactive (?) + }; + temporaries.set(value.lvalue.place.identifier.id, local); + break; + } + case 'StoreLocal': { + visit(value.value); + if (value.lvalue.kind !== InstructionKind.Reassign) { + const local: Temporary = { + kind: 'Local', + identifier: value.lvalue.place.identifier, + path: [], + reactive: value.value.reactive, + }; + temporaries.set(value.lvalue.place.identifier.id, local); + } + break; + } + case 'LoadContext': { + const temp = temporaries.get(value.place.identifier.id); + if (temp != null) { + temporaries.set(lvalue.identifier.id, temp); + } + break; + } + case 'DeclareContext': { + const local: Temporary = { + kind: 'Context', + identifier: value.lvalue.place.identifier, + }; + temporaries.set(value.lvalue.place.identifier.id, local); + break; + } + case 'StoreContext': { + visit(value.value); + if (value.lvalue.kind !== InstructionKind.Reassign) { + const local: Temporary = { + kind: 'Context', + identifier: value.lvalue.place.identifier, + }; + temporaries.set(value.lvalue.place.identifier.id, local); + } + 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: [], + reactive: value.value.reactive, + }; + temporaries.set(lvalue.identifier.id, local); + } + } + break; + } + case 'PropertyLoad': { + const object = temporaries.get(value.object.identifier.id); + if (object != null && object.kind === 'Local') { + const local: Temporary = { + kind: 'Local', + identifier: object.identifier, + path: [ + ...object.path, + {optional: false, property: value.property}, + ], + reactive: lvalue.reactive, + }; + temporaries.set(lvalue.identifier.id, local); + } + break; + } + case 'FunctionExpression': + case 'ObjectMethod': { + const functionDeps = collectTemporaries( + value.loweredFunc.func, + temporaries, + ); + temporaries.set(lvalue.identifier.id, functionDeps); + for (const dep of functionDeps.dependencies) { + dependencies.add(dep); + } + break; + } + case 'StartMemoize': + case 'FinishMemoize': { + 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)) { + visit(operand); + } + } + return {kind: 'Function', dependencies}; +} + +function printTemporary(temporary: Temporary): string { + switch (temporary.kind) { + case 'Context': { + return `Context ${printIdentifier(temporary.identifier)}`; + } + case 'Global': { + return `Global ${temporary.binding.name} [${temporary.binding.kind}]`; + } + case 'Local': { + return `Local ${printIdentifier(temporary.identifier)}${temporary.path.map(p => '.' + p.property + (p.optional ? '?' : '')).join('')} ${temporary.reactive ? '{reactive}' : ''}`; + } + case 'Function': { + return `Function dependencies=[${Array.from(temporary.dependencies).map(printTemporary).join(', ')}]`; + } + } +} + +type Temporary = + | {kind: 'Global'; binding: LoadGlobal['binding']} + | { + kind: 'Local'; + identifier: Identifier; + path: DependencyPath; + reactive: boolean; + } + | {kind: 'Context'; identifier: Identifier} + | {kind: 'Function'; dependencies: Set}; 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 0000000000..3ef81b9c52 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js @@ -0,0 +1,12 @@ +// + +import {useMemo} from 'react'; +import {Stringify} from 'shared-runtime'; + +function Component({x}) { + const DEBUG = false; + const y = useMemo(() => { + return () => x.y; + }, [x.y]); + return ; +}