From 5aa10ca5ca8d00d6e3cfe7e1d2a520948a650188 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 8 Sep 2025 15:36:41 -0400 Subject: [PATCH] [compiler] Fix false positive memo validation (alternative) Alternative to #34276 --- (Summary taken from @josephsavona 's #34276) 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 | 85 +++++++------- ...ken-as-dependency-later-mutation.expect.md | 59 ++++++++++ ...e-mistaken-as-dependency-later-mutation.js | 25 ++++ ...staken-as-dependency-mutated-dep.expect.md | 64 ++++++++++ ...alue-mistaken-as-dependency-mutated-dep.js | 36 ++++++ ...red-value-mistaken-as-dependency.expect.md | 109 ++++++++++++++++++ ...structured-value-mistaken-as-dependency.js | 31 +++++ 7 files changed, 364 insertions(+), 45 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js 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 49d8a85a96b3c..19af0ed080142 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -18,7 +18,6 @@ import { IdentifierId, InstructionValue, ManualMemoDependency, - Place, PrunedReactiveScopeBlock, ReactiveFunction, ReactiveInstruction, @@ -29,7 +28,10 @@ import { SourceLocation, } from '../HIR'; import {printIdentifier, printManualMemoDependency} from '../HIR/PrintHIR'; -import {eachInstructionValueOperand} from '../HIR/visitors'; +import { + eachInstructionValueLValue, + eachInstructionValueOperand, +} from '../HIR/visitors'; import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization'; import { ReactiveFunctionVisitor, @@ -337,56 +339,53 @@ class Visitor extends ReactiveFunctionVisitor { * @returns a @{ManualMemoDependency} representing the variable + * property reads represented by @value */ - recordDepsInValue( - value: ReactiveValue, - state: VisitorState, - ): ManualMemoDependency | null { + recordDepsInValue(value: ReactiveValue, state: VisitorState): void { switch (value.kind) { case 'SequenceExpression': { for (const instr of value.instructions) { this.visitInstruction(instr, state); } - const result = this.recordDepsInValue(value.value, state); - return result; + this.recordDepsInValue(value.value, state); + break; } case 'OptionalExpression': { - return this.recordDepsInValue(value.value, state); + this.recordDepsInValue(value.value, state); + break; } case 'ConditionalExpression': { this.recordDepsInValue(value.test, state); this.recordDepsInValue(value.consequent, state); this.recordDepsInValue(value.alternate, state); - return null; + break; } case 'LogicalExpression': { this.recordDepsInValue(value.left, state); this.recordDepsInValue(value.right, state); - return null; + break; } default: { - const dep = collectMaybeMemoDependencies( - value, - this.temporaries, - false, - ); - if (value.kind === 'StoreLocal' || value.kind === 'StoreContext') { - const storeTarget = value.lvalue.place; - state.manualMemoState?.decls.add( - storeTarget.identifier.declarationId, - ); - if (storeTarget.identifier.name?.kind === 'named' && dep == null) { - const dep: ManualMemoDependency = { - root: { - kind: 'NamedLocal', - value: storeTarget, - }, - path: [], - }; - this.temporaries.set(storeTarget.identifier.id, dep); - return dep; + collectMaybeMemoDependencies(value, this.temporaries, false); + if ( + value.kind === 'StoreLocal' || + value.kind === 'StoreContext' || + value.kind === 'Destructure' + ) { + for (const storeTarget of eachInstructionValueLValue(value)) { + state.manualMemoState?.decls.add( + storeTarget.identifier.declarationId, + ); + if (storeTarget.identifier.name?.kind === 'named') { + this.temporaries.set(storeTarget.identifier.id, { + root: { + kind: 'NamedLocal', + value: storeTarget, + }, + path: [], + }); + } } } - return dep; + break; } } } @@ -403,19 +402,15 @@ class Visitor extends ReactiveFunctionVisitor { state.manualMemoState.decls.add(lvalue.identifier.declarationId); } - const maybeDep = this.recordDepsInValue(value, state); - if (lvalId != null) { - if (maybeDep != null) { - temporaries.set(lvalId, maybeDep); - } else if (isNamedLocal) { - temporaries.set(lvalId, { - root: { - kind: 'NamedLocal', - value: {...(instr.lvalue as Place)}, - }, - path: [], - }); - } + this.recordDepsInValue(value, state); + if (lvalue != null) { + temporaries.set(lvalue.identifier.id, { + root: { + kind: 'NamedLocal', + value: {...lvalue}, + }, + path: [], + }); } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md new file mode 100644 index 0000000000000..d8223e2949837 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md @@ -0,0 +1,59 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + * + * This fixture adds a later potential mutation, which extends the scope and should + * fail validation. This confirms that even though we allow the dependency to diverge, + * we still check that the output value is memoized. + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = transform(input); + return {value}; + }, [input]); + mutate(object); + return object; +} + +``` + + +## Error + +``` +Found 1 error: + +Compilation Skipped: Existing memoization could not be preserved + +React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. + +error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.ts:19:17 + 17 | */ + 18 | function useInputValue(input) { +> 19 | const object = React.useMemo(() => { + | ^^^^^^^^^^^^^^^^^^^^^ +> 20 | const {value} = transform(input); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 21 | return {value}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 22 | }, [input]); + | ^^^^^^^^^^^^^^ Could not preserve existing memoization + 23 | mutate(object); + 24 | return object; + 25 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js new file mode 100644 index 0000000000000..f07d00854cd85 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js @@ -0,0 +1,25 @@ +// @validatePreserveExistingMemoizationGuarantees + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + * + * This fixture adds a later potential mutation, which extends the scope and should + * fail validation. This confirms that even though we allow the dependency to diverge, + * we still check that the output value is memoized. + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = transform(input); + return {value}; + }, [input]); + mutate(object); + return object; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md new file mode 100644 index 0000000000000..371348a560643 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md @@ -0,0 +1,64 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify, useHook} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + */ +function useInputValue(input) { + // Conflate the `identity(input, x)` call with something outside the useMemo, + // to try and break memoization of `value`. This gets correctly flagged since + // the dependency is being mutated + let x = {}; + useHook(); + const object = React.useMemo(() => { + const {value} = identity(input, x); + return {value}; + }, [input, x]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Compilation Skipped: Existing memoization could not be preserved + +React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This dependency may be mutated later, which could cause the value to change unexpectedly. + +error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.ts:25:13 + 23 | const {value} = identity(input, x); + 24 | return {value}; +> 25 | }, [input, x]); + | ^ This dependency may be modified later + 26 | return object; + 27 | } + 28 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js new file mode 100644 index 0000000000000..06d2945868dd1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js @@ -0,0 +1,36 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify, useHook} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + */ +function useInputValue(input) { + // Conflate the `identity(input, x)` call with something outside the useMemo, + // to try and break memoization of `value`. This gets correctly flagged since + // the dependency is being mutated + let x = {}; + useHook(); + const object = React.useMemo(() => { + const {value} = identity(input, x); + return {value}; + }, [input, x]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; 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 0000000000000..eca36e2a7919d --- /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,109 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * Previously ValidatePreservedManualMemoization rejected this input, because + * the original memoization had `object` depending on `input` but we split the scope per above, + * and the scope for the FinishMemoize instruction is the second scope which depends on `value` + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = identity(input); + return {value}; + }, [input]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees + +import { identity, Stringify } from "shared-runtime"; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * Previously ValidatePreservedManualMemoization rejected this input, because + * the original memoization had `object` depending on `input` but we split the scope per above, + * and the scope for the FinishMemoize instruction is the second scope which depends on `value` + */ +function useInputValue(input) { + const $ = _c(4); + let t0; + if ($[0] !== input) { + t0 = identity(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; +} + +function Component() { + const $ = _c(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = { value: 42 }; + $[0] = t0; + } else { + t0 = $[0]; + } + const t1 = useInputValue(t0); + let t2; + if ($[1] !== t1.value) { + t2 = ; + $[1] = t1.value; + $[2] = t2; + } else { + t2 = $[2]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok)
{"value":42}
\ 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 0000000000000..a9533ce44bbde --- /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,31 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * Previously ValidatePreservedManualMemoization rejected this input, because + * the original memoization had `object` depending on `input` but we split the scope per above, + * and the scope for the FinishMemoize instruction is the second scope which depends on `value` + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = identity(input); + return {value}; + }, [input]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +};