[compiler] Fix false positive memo validation (alternative) (#34319)

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.

Co-authored-by: Joe Savona <[email protected]>

Author: mofeiZ

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