From 052f2c9802c5ef2d32eed57839997409baf26c7f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 22 Aug 2025 14:09:50 -0700 Subject: [PATCH] [compiler] Fix false positive memo validation Partial fix for #34262. Consider this example: ```js function useInputValue(input) { const object = React.useMemo(() => { const {value} = transform(input); return {value}; }, [input]); return object; } ``` React Compiler breaks this code into two reactive scopes: * One for `transform(input)` * One for `{value}` When we run ValidatePreserveExistingMemo, we see that the scope for `{value}` has the dependency `value`, whereas the original memoization had the dependency `input`, and throw an error that the dependencies didn't match. In other words, we're flagging the fact that memoized _better than the user_ as a problem. The more complete solution would be to validate that there is a subgraph of reactive scopes with a single input and output node, where the input node has the same dependencies as the original useMemo, and the output has the same outputs. That is true in this case, with the subgraph being the two consecutive scopes mentioned above. But that's complicated. As a shortcut, this PR checks for any dependencies that are defined after the start of the original useMemo. If we find one, we know that it's a case where we were able to memoize more precisely than the original, and we don't report an error on the dependency. We still check that the original _output_ value is able to be memoized, though. So if the scope of `object` were extended, eg with a call to `mutate(object)`, then we'd still correctly report an error that we couldn't preserve memoization. --- .../ValidatePreservedManualMemoization.ts | 9 +++ ...red-value-mistaken-as-dependency.expect.md | 58 +++++++++++++++++++ ...structured-value-mistaken-as-dependency.js | 14 +++++ 3 files changed, 81 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 516ca232e9..7fb52cf3eb 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -17,6 +17,7 @@ import { GeneratedSource, Identifier, IdentifierId, + InstructionId, InstructionValue, ManualMemoDependency, Place, @@ -109,6 +110,7 @@ type ManualMemoBlockState = { */ depsFromSource: Array | null; manualMemoId: number; + start: InstructionId; }; type VisitorState = { @@ -234,6 +236,7 @@ function validateInferredDep( validDepsInMemoBlock: Array, errorState: CompilerError, memoLocation: SourceLocation, + memoStartInstruction: InstructionId, ): void { let normalizedDep: ManualMemoDependency; const maybeNormalizedRoot = temporaries.get(dep.identifier.id); @@ -271,6 +274,10 @@ function validateInferredDep( return; } } + if (dep.identifier.mutableRange.start > memoStartInstruction) { + return; + } + let errorDiagnostic: CompareDependencyResult | null = null; for (const originalDep of validDepsInMemoBlock) { const compareResult = compareDeps(normalizedDep, originalDep); @@ -433,6 +440,7 @@ class Visitor extends ReactiveFunctionVisitor { state.manualMemoState.depsFromSource, state.errors, state.manualMemoState.loc, + state.manualMemoState.start, ); } } @@ -508,6 +516,7 @@ class Visitor extends ReactiveFunctionVisitor { depsFromSource, manualMemoId: value.manualMemoId, reassignments: new Map(), + start: instruction.id, }; /** diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md new file mode 100644 index 0000000000..480c7288b9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md @@ -0,0 +1,58 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * We incorrectly infer `value` as the dependency, but that is a local value within the useMemo. + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = transform(input); + return {value}; + }, [input]); + return object; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * We incorrectly infer `value` as the dependency, but that is a local value within the useMemo. + */ +function useInputValue(input) { + const $ = _c(4); + let t0; + if ($[0] !== input) { + t0 = transform(input); + $[0] = input; + $[1] = t0; + } else { + t0 = $[1]; + } + const { value } = t0; + let t1; + if ($[2] !== value) { + t1 = { value }; + $[2] = value; + $[3] = t1; + } else { + t1 = $[3]; + } + const object = t1; + return object; +} + +``` + +### 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/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js new file mode 100644 index 0000000000..b67a850cd5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js @@ -0,0 +1,14 @@ +// @validatePreserveExistingMemoizationGuarantees + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * We incorrectly infer `value` as the dependency, but that is a local value within the useMemo. + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = transform(input); + return {value}; + }, [input]); + return object; +}