[compiler] Fix AnalyzeFunctions to fully reset context identifiers

AnalyzeFunctions had logic to reset the mutable ranges of context variables after visiting inner function expressions. However, there was a bug in that logic: InferReactiveScopeVariables makes all the identifiers in a scope point to the same mutable range instance. That meant that it was possible for a later function expression to indirectly cause an earlier function expressions' context variables to get a non-zero mutable range.

The fix is to not just reset start/end of context var ranges, but assign a new range instance. Thanks for the help on debugging, @mofeiz!

compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts

@@ -42,8 +42,16 @@ export default function analyseFunctions(func: HIRFunction): void {
            * Reset mutable range for outer inferReferenceEffects
            */
           for (const operand of instr.value.loweredFunc.func.context) {
-            operand.identifier.mutableRange.start = makeInstructionId(0);
-            operand.identifier.mutableRange.end = makeInstructionId(0);
+            /**
+             * NOTE: inferReactiveScopeVariables makes identifiers in the scope
+             * point to the *same* mutableRange instance. Resetting start/end
+             * here is insufficient, because a later mutation of the range
+             * for any one identifier could affect the range for other identifiers.
+             */
+            operand.identifier.mutableRange = {
+              start: makeInstructionId(0),
+              end: makeInstructionId(0),
+            };
             operand.identifier.scope = null;
           }
           break;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inner-function/nullable-objects/array-map-named-callback-cross-context.expect.md

@@ -57,67 +57,62 @@ import { Stringify } from "shared-runtime";
  * - cb1 is not assumed to be called since it's only used as a call operand
  */
 function useFoo(t0) {
-  const $ = _c(14);
-  let arr1;
-  let arr2;
+  const $ = _c(13);
+  const { arr1, arr2 } = t0;
   let t1;
-  if ($[0] !== t0) {
-    ({ arr1, arr2 } = t0);
-    let t2;
-    if ($[4] !== arr1[0]) {
-      t2 = (e) => arr1[0].value + e.value;
-      $[4] = arr1[0];
-      $[5] = t2;
-    } else {
-      t2 = $[5];
-    }
-    const cb1 = t2;
-    t1 = () => arr1.map(cb1);
-    $[0] = t0;
-    $[1] = arr1;
-    $[2] = arr2;
-    $[3] = t1;
+  if ($[0] !== arr1[0]) {
+    t1 = (e) => arr1[0].value + e.value;
+    $[0] = arr1[0];
+    $[1] = t1;
   } else {
-    arr1 = $[1];
-    arr2 = $[2];
-    t1 = $[3];
+    t1 = $[1];
   }
-  const getArrMap1 = t1;
+  const cb1 = t1;
   let t2;
-  if ($[6] !== arr2) {
-    t2 = (e_0) => arr2[0].value + e_0.value;
-    $[6] = arr2;
-    $[7] = t2;
+  if ($[2] !== arr1 || $[3] !== cb1) {
+    t2 = () => arr1.map(cb1);
+    $[2] = arr1;
+    $[3] = cb1;
+    $[4] = t2;
   } else {
-    t2 = $[7];
+    t2 = $[4];
   }
-  const cb2 = t2;
+  const getArrMap1 = t2;
   let t3;
-  if ($[8] !== arr1 || $[9] !== cb2) {
-    t3 = () => arr1.map(cb2);
-    $[8] = arr1;
-    $[9] = cb2;
-    $[10] = t3;
+  if ($[5] !== arr2) {
+    t3 = (e_0) => arr2[0].value + e_0.value;
+    $[5] = arr2;
+    $[6] = t3;
   } else {
-    t3 = $[10];
+    t3 = $[6];
   }
-  const getArrMap2 = t3;
+  const cb2 = t3;
   let t4;
-  if ($[11] !== getArrMap1 || $[12] !== getArrMap2) {
-    t4 = (
+  if ($[7] !== arr1 || $[8] !== cb2) {
+    t4 = () => arr1.map(cb2);
+    $[7] = arr1;
+    $[8] = cb2;
+    $[9] = t4;
+  } else {
+    t4 = $[9];
+  }
+  const getArrMap2 = t4;
+  let t5;
+  if ($[10] !== getArrMap1 || $[11] !== getArrMap2) {
+    t5 = (
       <Stringify
         getArrMap1={getArrMap1}
         getArrMap2={getArrMap2}
         shouldInvokeFns={true}
       />
     );
-    $[11] = getArrMap1;
-    $[12] = getArrMap2;
-    $[13] = t4;
+    $[10] = getArrMap1;
+    $[11] = getArrMap2;
+    $[12] = t5;
   } else {
-    t4 = $[13];
+    t5 = $[12];
   }
-  return t4;
+  return t5;
 }
 
 export const FIXTURE_ENTRYPOINT = {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/array-map-named-callback-cross-context.expect.md

@@ -58,67 +58,62 @@ import { Stringify } from "shared-runtime";
  * - cb1 is not assumed to be called since it's only used as a call operand
  */
 function useFoo(t0) {
-  const $ = _c(14);
-  let arr1;
-  let arr2;
+  const $ = _c(13);
+  const { arr1, arr2 } = t0;
   let t1;
-  if ($[0] !== t0) {
-    ({ arr1, arr2 } = t0);
-    let t2;
-    if ($[4] !== arr1[0]) {
-      t2 = (e) => arr1[0].value + e.value;
-      $[4] = arr1[0];
-      $[5] = t2;
-    } else {
-      t2 = $[5];
-    }
-    const cb1 = t2;
-    t1 = () => arr1.map(cb1);
-    $[0] = t0;
-    $[1] = arr1;
-    $[2] = arr2;
-    $[3] = t1;
+  if ($[0] !== arr1[0]) {
+    t1 = (e) => arr1[0].value + e.value;
+    $[0] = arr1[0];
+    $[1] = t1;
   } else {
-    arr1 = $[1];
-    arr2 = $[2];
-    t1 = $[3];
+    t1 = $[1];
   }
-  const getArrMap1 = t1;
+  const cb1 = t1;
   let t2;
-  if ($[6] !== arr2) {
-    t2 = (e_0) => arr2[0].value + e_0.value;
-    $[6] = arr2;
-    $[7] = t2;
+  if ($[2] !== arr1 || $[3] !== cb1) {
+    t2 = () => arr1.map(cb1);
+    $[2] = arr1;
+    $[3] = cb1;
+    $[4] = t2;
   } else {
-    t2 = $[7];
+    t2 = $[4];
   }
-  const cb2 = t2;
+  const getArrMap1 = t2;
   let t3;
-  if ($[8] !== arr1 || $[9] !== cb2) {
-    t3 = () => arr1.map(cb2);
-    $[8] = arr1;
-    $[9] = cb2;
-    $[10] = t3;
+  if ($[5] !== arr2) {
+    t3 = (e_0) => arr2[0].value + e_0.value;
+    $[5] = arr2;
+    $[6] = t3;
   } else {
-    t3 = $[10];
+    t3 = $[6];
   }
-  const getArrMap2 = t3;
+  const cb2 = t3;
   let t4;
-  if ($[11] !== getArrMap1 || $[12] !== getArrMap2) {
-    t4 = (
+  if ($[7] !== arr1 || $[8] !== cb2) {
+    t4 = () => arr1.map(cb2);
+    $[7] = arr1;
+    $[8] = cb2;
+    $[9] = t4;
+  } else {
+    t4 = $[9];
+  }
+  const getArrMap2 = t4;
+  let t5;
+  if ($[10] !== getArrMap1 || $[11] !== getArrMap2) {
+    t5 = (
       <Stringify
         getArrMap1={getArrMap1}
         getArrMap2={getArrMap2}
         shouldInvokeFns={true}
       />
     );
-    $[11] = getArrMap1;
-    $[12] = getArrMap2;
-    $[13] = t4;
+    $[10] = getArrMap1;
+    $[11] = getArrMap2;
+    $[12] = t5;
   } else {
-    t4 = $[13];
+    t5 = $[12];
   }
-  return t4;
+  return t5;
 }
 
 export const FIXTURE_ENTRYPOINT = {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-internal-compiler-shared-mutablerange-bug.expect.md

@@ -0,0 +1,80 @@
+
+## Input
+
+```javascript
+//@flow @validatePreserveExistingMemoizationGuarantees @enableNewMutationAliasingModel
+component Component(
+  onAsyncSubmit?: (() => void) => void,
+  onClose: (isConfirmed: boolean) => void
+) {
+  // When running inferReactiveScopeVariables,
+  // onAsyncSubmit and onClose update to share
+  // a mutableRange instance.
+  const onSubmit = useCallback(() => {
+    if (onAsyncSubmit) {
+      onAsyncSubmit(() => {
+        onClose(true);
+      });
+      return;
+    }
+  }, [onAsyncSubmit, onClose]);
+  // When running inferReactiveScopeVariables here,
+  // first the existing range gets updated (affecting
+  // onAsyncSubmit) and then onClose gets assigned a
+  // different mutable range instance, which is the
+  // one reset after AnalyzeFunctions.
+  // The fix is to fully reset mutable ranges *instances*
+  // after AnalyzeFunctions visit a function expression
+  return <Dialog onSubmit={onSubmit} onClose={() => onClose(false)} />;
+}
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+function Component(t0) {
+  const $ = _c(8);
+  const { onAsyncSubmit, onClose } = t0;
+  let t1;
+  if ($[0] !== onAsyncSubmit || $[1] !== onClose) {
+    t1 = () => {
+      if (onAsyncSubmit) {
+        onAsyncSubmit(() => {
+          onClose(true);
+        });
+        return;
+      }
+    };
+    $[0] = onAsyncSubmit;
+    $[1] = onClose;
+    $[2] = t1;
+  } else {
+    t1 = $[2];
+  }
+  const onSubmit = t1;
+  let t2;
+  if ($[3] !== onClose) {
+    t2 = () => onClose(false);
+    $[3] = onClose;
+    $[4] = t2;
+  } else {
+    t2 = $[4];
+  }
+  let t3;
+  if ($[5] !== onSubmit || $[6] !== t2) {
+    t3 = <Dialog onSubmit={onSubmit} onClose={t2} />;
+    $[5] = onSubmit;
+    $[6] = t2;
+    $[7] = t3;
+  } else {
+    t3 = $[7];
+  }
+  return t3;
+}
+
+```
+      
+### Eval output
+(kind: exception) Fixture not implemented

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-internal-compiler-shared-mutablerange-bug.js

@@ -0,0 +1,25 @@
+//@flow @validatePreserveExistingMemoizationGuarantees @enableNewMutationAliasingModel
+component Component(
+  onAsyncSubmit?: (() => void) => void,
+  onClose: (isConfirmed: boolean) => void
+) {
+  // When running inferReactiveScopeVariables,
+  // onAsyncSubmit and onClose update to share
+  // a mutableRange instance.
+  const onSubmit = useCallback(() => {
+    if (onAsyncSubmit) {
+      onAsyncSubmit(() => {
+        onClose(true);
+      });
+      return;
+    }
+  }, [onAsyncSubmit, onClose]);
+  // When running inferReactiveScopeVariables here,
+  // first the existing range gets updated (affecting
+  // onAsyncSubmit) and then onClose gets assigned a
+  // different mutable range instance, which is the
+  // one reset after AnalyzeFunctions.
+  // The fix is to fully reset mutable ranges *instances*
+  // after AnalyzeFunctions visit a function expression
+  return <Dialog onSubmit={onSubmit} onClose={() => onClose(false)} />;
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/shared-hook-calls.expect.md

@@ -30,60 +30,47 @@ import { c as _c, useFire } from "react/compiler-runtime"; // @enableFire @enabl
 import { fire } from "react";
 
 function Component(t0) {
-  const $ = _c(13);
-  let bar;
-  let baz;
-  let foo;
-  if ($[0] !== t0) {
-    ({ bar, baz } = t0);
-    let t1;
-    if ($[4] !== bar) {
-      t1 = () => {
-        console.log(bar);
-      };
-      $[4] = bar;
-      $[5] = t1;
-    } else {
-      t1 = $[5];
-    }
-    foo = t1;
-    $[0] = t0;
-    $[1] = bar;
-    $[2] = baz;
-    $[3] = foo;
+  const $ = _c(9);
+  const { bar, baz } = t0;
+  let t1;
+  if ($[0] !== bar) {
+    t1 = () => {
+      console.log(bar);
+    };
+    $[0] = bar;
+    $[1] = t1;
   } else {
-    bar = $[1];
-    baz = $[2];
-    foo = $[3];
+    t1 = $[1];
   }
-  const t1 = useFire(foo);
-  const t2 = useFire(baz);
-  let t3;
-  if ($[6] !== bar || $[7] !== t1 || $[8] !== t2) {
-    t3 = () => {
-      t1(bar);
+  const foo = t1;
+  const t2 = useFire(foo);
+  const t3 = useFire(baz);
+  let t4;
+  if ($[2] !== bar || $[3] !== t2 || $[4] !== t3) {
+    t4 = () => {
+      t2(bar);
+      t3(bar);
+    };
+    $[2] = bar;
+    $[3] = t2;
+    $[4] = t3;
+    $[5] = t4;
+  } else {
+    t4 = $[5];
+  }
+  useEffect(t4);
+  let t5;
+  if ($[6] !== bar || $[7] !== t2) {
+    t5 = () => {
       t2(bar);
     };
     $[6] = bar;
-    $[7] = t1;
-    $[8] = t2;
-    $[9] = t3;
+    $[7] = t2;
+    $[8] = t5;
   } else {
-    t3 = $[9];
+    t5 = $[8];
   }
-  useEffect(t3);
-  let t4;
-  if ($[10] !== bar || $[11] !== t1) {
-    t4 = () => {
-      t1(bar);
-    };
-    $[10] = bar;
-    $[11] = t1;
-    $[12] = t4;
-  } else {
-    t4 = $[12];
-  }
-  useEffect(t4);
+  useEffect(t5);
   return null;
 }
 

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/shared-hook-calls.expect.md

@@ -30,60 +30,47 @@ import { c as _c, useFire } from "react/compiler-runtime"; // @enableFire
 import { fire } from "react";
 
 function Component(t0) {
-  const $ = _c(13);
-  let bar;
-  let baz;
-  let foo;
-  if ($[0] !== t0) {
-    ({ bar, baz } = t0);
-    let t1;
-    if ($[4] !== bar) {
-      t1 = () => {
-        console.log(bar);
-      };
-      $[4] = bar;
-      $[5] = t1;
-    } else {
-      t1 = $[5];
-    }
-    foo = t1;
-    $[0] = t0;
-    $[1] = bar;
-    $[2] = baz;
-    $[3] = foo;
+  const $ = _c(9);
+  const { bar, baz } = t0;
+  let t1;
+  if ($[0] !== bar) {
+    t1 = () => {
+      console.log(bar);
+    };
+    $[0] = bar;
+    $[1] = t1;
   } else {
-    bar = $[1];
-    baz = $[2];
-    foo = $[3];
+    t1 = $[1];
   }
-  const t1 = useFire(foo);
-  const t2 = useFire(baz);
-  let t3;
-  if ($[6] !== bar || $[7] !== t1 || $[8] !== t2) {
-    t3 = () => {
-      t1(bar);
+  const foo = t1;
+  const t2 = useFire(foo);
+  const t3 = useFire(baz);
+  let t4;
+  if ($[2] !== bar || $[3] !== t2 || $[4] !== t3) {
+    t4 = () => {
+      t2(bar);
+      t3(bar);
+    };
+    $[2] = bar;
+    $[3] = t2;
+    $[4] = t3;
+    $[5] = t4;
+  } else {
+    t4 = $[5];
+  }
+  useEffect(t4);
+  let t5;
+  if ($[6] !== bar || $[7] !== t2) {
+    t5 = () => {
       t2(bar);
     };
     $[6] = bar;
-    $[7] = t1;
-    $[8] = t2;
-    $[9] = t3;
+    $[7] = t2;
+    $[8] = t5;
   } else {
-    t3 = $[9];
+    t5 = $[8];
   }
-  useEffect(t3);
-  let t4;
-  if ($[10] !== bar || $[11] !== t1) {
-    t4 = () => {
-      t1(bar);
-    };
-    $[10] = bar;
-    $[11] = t1;
-    $[12] = t4;
-  } else {
-    t4 = $[12];
-  }
-  useEffect(t4);
+  useEffect(t5);
   return null;
 }