[compiler] Fix false positive memo validation

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts

@@ -135,7 +135,12 @@ export type PluginOptions = {
    */
   eslintSuppressionRules: Array<string> | null | undefined;
 
+  /**
+   * Whether to report "suppression" errors for Flow suppressions. If false, suppression errors
+   * are only emitted for ESLint suppressions
+   */
   flowSuppressions: boolean;
+
   /*
    * Ignore 'use no forget' annotations. Helpful during testing but should not be used in production.
    */

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Suppression.ts

@@ -87,12 +87,18 @@ export function findProgramSuppressions(
   let enableComment: t.Comment | null = null;
   let source: SuppressionSource | null = null;
 
-  const rulePattern = `(${ruleNames.join('|')})`;
-  const disableNextLinePattern = new RegExp(
-    `eslint-disable-next-line ${rulePattern}`,
-  );
-  const disablePattern = new RegExp(`eslint-disable ${rulePattern}`);
-  const enablePattern = new RegExp(`eslint-enable ${rulePattern}`);
+  let disableNextLinePattern: RegExp | null = null;
+  let disablePattern: RegExp | null = null;
+  let enablePattern: RegExp | null = null;
+  if (ruleNames.length !== 0) {
+    const rulePattern = `(${ruleNames.join('|')})`;
+    disableNextLinePattern = new RegExp(
+      `eslint-disable-next-line ${rulePattern}`,
+    );
+    disablePattern = new RegExp(`eslint-disable ${rulePattern}`);
+    enablePattern = new RegExp(`eslint-enable ${rulePattern}`);
+  }
+
   const flowSuppressionPattern = new RegExp(
     '\\$(FlowFixMe\\w*|FlowExpectedError|FlowIssue)\\[react\\-rule',
   );
@@ -108,6 +114,7 @@ export function findProgramSuppressions(
        * CommentLine within the block.
        */
       disableComment == null &&
+      disableNextLinePattern != null &&
       disableNextLinePattern.test(comment.value)
     ) {
       disableComment = comment;
@@ -125,12 +132,16 @@ export function findProgramSuppressions(
       source = 'Flow';
     }
 
-    if (disablePattern.test(comment.value)) {
+    if (disablePattern != null && disablePattern.test(comment.value)) {
       disableComment = comment;
       source = 'Eslint';
     }
 
-    if (enablePattern.test(comment.value) && source === 'Eslint') {
+    if (
+      enablePattern != null &&
+      enablePattern.test(comment.value) &&
+      source === 'Eslint'
+    ) {
       enableComment = comment;
     }
 

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts

@@ -17,6 +17,7 @@ import {
   GeneratedSource,
   Identifier,
   IdentifierId,
+  InstructionId,
   InstructionValue,
   ManualMemoDependency,
   Place,
@@ -109,6 +110,7 @@ type ManualMemoBlockState = {
    */
   depsFromSource: Array<ManualMemoDependency> | null;
   manualMemoId: number;
+  start: InstructionId;
 };
 
 type VisitorState = {
@@ -234,6 +236,8 @@ function validateInferredDep(
   validDepsInMemoBlock: Array<ManualMemoDependency>,
   errorState: CompilerError,
   memoLocation: SourceLocation,
+  memoStartInstruction: InstructionId,
+  scopes: Set<ScopeId>,
 ): void {
   let normalizedDep: ManualMemoDependency;
   const maybeNormalizedRoot = temporaries.get(dep.identifier.id);
@@ -271,6 +275,13 @@ function validateInferredDep(
       return;
     }
   }
+  if (
+    dep.identifier.mutableRange.start > memoStartInstruction &&
+    !isUnmemoized(dep.identifier, scopes)
+  ) {
+    return;
+  }
+
   let errorDiagnostic: CompareDependencyResult | null = null;
   for (const originalDep of validDepsInMemoBlock) {
     const compareResult = compareDeps(normalizedDep, originalDep);
@@ -433,6 +444,8 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
           state.manualMemoState.depsFromSource,
           state.errors,
           state.manualMemoState.loc,
+          state.manualMemoState.start,
+          this.scopes,
         );
       }
     }
@@ -508,6 +521,7 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
         depsFromSource,
         manualMemoId: value.manualMemoId,
         reassignments: new Map(),
+        start: instruction.id,
       };
 
       /**

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.expect.md

@@ -0,0 +1,53 @@
+
+## Input
+
+```javascript
+// @eslintSuppressionRules:[]
+
+// The suppression here shouldn't cause compilation to get skipped
+// Previously we had a bug where an empty list of suppressions would
+// create a regexp that matched any suppression
+function Component(props) {
+  'use forget';
+  // eslint-disable-next-line foo/not-react-related
+  return <div>{props.text}</div>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{text: 'Hello'}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @eslintSuppressionRules:[]
+
+// The suppression here shouldn't cause compilation to get skipped
+// Previously we had a bug where an empty list of suppressions would
+// create a regexp that matched any suppression
+function Component(props) {
+  "use forget";
+  const $ = _c(2);
+  let t0;
+  if ($[0] !== props.text) {
+    t0 = <div>{props.text}</div>;
+    $[0] = props.text;
+    $[1] = t0;
+  } else {
+    t0 = $[1];
+  }
+  return t0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ text: "Hello" }],
+};
+
+```
+      
+### Eval output
+(kind: ok) <div>Hello</div>

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.js

@@ -0,0 +1,15 @@
+// @eslintSuppressionRules:[]
+
+// The suppression here shouldn't cause compilation to get skipped
+// Previously we had a bug where an empty list of suppressions would
+// create a regexp that matched any suppression
+function Component(props) {
+  'use forget';
+  // eslint-disable-next-line foo/not-react-related
+  return <div>{props.text}</div>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{text: 'Hello'}],
+};

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