[donotcommit] Alternative fix for false positive memo validation

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

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<VisitorState> {
         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;

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 <Stringify value={useInputValue({value: 42}).value} />;
+}
+
+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 <Stringify value={useInputValue({ value: 42 }).value} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+      
+### Eval output
+(kind: ok) <div>{"value":42}</div>

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 <Stringify value={useInputValue({value: 42}).value} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};