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 6bb59247da..1e8b66b715 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -29,7 +29,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, @@ -358,26 +361,26 @@ class Visitor extends ReactiveFunctionVisitor { return null; } 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; + let 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') { + dep = { + root: { + kind: 'NamedLocal', + value: storeTarget, + }, + path: [], + }; + this.temporaries.set(storeTarget.identifier.id, dep); + } } } return dep; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/false-positive-repro.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/false-positive-repro.expect.md new file mode 100644 index 0000000000..8026ba3a5a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/false-positive-repro.expect.md @@ -0,0 +1,94 @@ + +## 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() { + 'use no memo'; + 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() { + "use no memo"; + return ; +} + +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/false-positive-repro.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/false-positive-repro.js new file mode 100644 index 0000000000..5955e67305 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/false-positive-repro.js @@ -0,0 +1,32 @@ +// @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() { + 'use no memo'; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +};