[compiler] Fix false positive memo validation (alternative)

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<VisitorState> {
    * @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<VisitorState> {
       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: [],
+      });
     }
   }
 

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 | }
+```
+
+

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;
+}

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 <Stringify value={useInputValue({value: 42}).value} />;
+}
+
+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 |
+```
+
+

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