From b836de613d66ff36574af95cb93ad15fd743d1f4 Mon Sep 17 00:00:00 2001
From: Ricky <rickhanlonii@gmail.com>
Date: Mon, 11 Nov 2024 17:25:37 -0500
Subject: [PATCH 1/3] Fix continuation bug (#31434)

## Overview

In `scheduleTaskForRootDuringMicrotask` we clear `root.callbackNode` if
the work loop is [suspended waiting on
data](https://github.com/facebook/react/blob/ac3ca097aeecae8fe3ec7f9b286307a923676518/packages/react-reconciler/src/ReactFiberRootScheduler.js#L338).

But we don't null check `root.callbackNode` before returning a
continuation in `performWorkOnRootViaSchedulerTask` where
`scheduleTaskForRootDuringMicrotask` is synchronously called, causing an
infinite loop when the only thing in the queue is something suspended
waiting on data.

This essentially restores the behavior from here:
https://github.com/facebook/react/pull/26328/files#diff-72ff2175ae3569037f0b16802a41b0cda2b2d66bb97f2bda78ed8445ed487b58L1168

Found by investigating the failures for
https://github.com/facebook/react/pull/31417

## TODO
- add a test

---------

Co-authored-by: Joe Savona <joesavona@fb.com>
---
 .../src/ReactFiberRootScheduler.js            |   2 +-
 .../__tests__/ActivityLegacySuspense-test.js  | 604 ++++++++++++++++++
 .../src/__tests__/ActivitySuspense-test.js    |  94 ++-
 3 files changed, 681 insertions(+), 19 deletions(-)
 create mode 100644 packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js

diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js
index 39f6f466ed..1f529d9e4d 100644
--- a/packages/react-reconciler/src/ReactFiberRootScheduler.js
+++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js
@@ -482,7 +482,7 @@ function performWorkOnRootViaSchedulerTask(
   // only safe to do because we know we're at the end of the browser task.
   // So although it's not an actual microtask, it might as well be.
   scheduleTaskForRootDuringMicrotask(root, now());
-  if (root.callbackNode === originalCallbackNode) {
+  if (root.callbackNode != null && root.callbackNode === originalCallbackNode) {
     // The task node scheduled for this root is the same one that's
     // currently executed. Need to return a continuation.
     return performWorkOnRootViaSchedulerTask.bind(null, root);
diff --git a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js
new file mode 100644
index 0000000000..424594cc35
--- /dev/null
+++ b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js
@@ -0,0 +1,604 @@
+let React;
+let ReactNoop;
+let Scheduler;
+let act;
+let LegacyHidden;
+let Activity;
+let Suspense;
+let useState;
+let useEffect;
+let startTransition;
+let textCache;
+let waitFor;
+let waitForPaint;
+let assertLog;
+
+describe('Activity Suspense', () => {
+  beforeEach(() => {
+    jest.resetModules();
+
+    React = require('react');
+    ReactNoop = require('react-noop-renderer');
+    Scheduler = require('scheduler');
+    act = require('internal-test-utils').act;
+    LegacyHidden = React.unstable_LegacyHidden;
+    Activity = React.unstable_Activity;
+    Suspense = React.Suspense;
+    useState = React.useState;
+    useEffect = React.useEffect;
+    startTransition = React.startTransition;
+
+    const InternalTestUtils = require('internal-test-utils');
+    waitFor = InternalTestUtils.waitFor;
+    waitForPaint = InternalTestUtils.waitForPaint;
+    assertLog = InternalTestUtils.assertLog;
+
+    textCache = new Map();
+  });
+
+  function resolveText(text) {
+    const record = textCache.get(text);
+    if (record === undefined) {
+      const newRecord = {
+        status: 'resolved',
+        value: text,
+      };
+      textCache.set(text, newRecord);
+    } else if (record.status === 'pending') {
+      const thenable = record.value;
+      record.status = 'resolved';
+      record.value = text;
+      thenable.pings.forEach(t => t());
+    }
+  }
+
+  function readText(text) {
+    const record = textCache.get(text);
+    if (record !== undefined) {
+      switch (record.status) {
+        case 'pending':
+          Scheduler.log(`Suspend! [${text}]`);
+          throw record.value;
+        case 'rejected':
+          throw record.value;
+        case 'resolved':
+          return record.value;
+      }
+    } else {
+      Scheduler.log(`Suspend! [${text}]`);
+      const thenable = {
+        pings: [],
+        then(resolve) {
+          if (newRecord.status === 'pending') {
+            thenable.pings.push(resolve);
+          } else {
+            Promise.resolve().then(() => resolve(newRecord.value));
+          }
+        },
+      };
+
+      const newRecord = {
+        status: 'pending',
+        value: thenable,
+      };
+      textCache.set(text, newRecord);
+
+      throw thenable;
+    }
+  }
+
+  function Text({text}) {
+    Scheduler.log(text);
+    return text;
+  }
+
+  function AsyncText({text}) {
+    readText(text);
+    Scheduler.log(text);
+    return text;
+  }
+
+  // @gate enableActivity
+  it('basic example of suspending inside hidden tree', async () => {
+    const root = ReactNoop.createRoot();
+
+    function App() {
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <span>
+            <Text text="Visible" />
+          </span>
+          <Activity mode="hidden">
+            <span>
+              <AsyncText text="Hidden" />
+            </span>
+          </Activity>
+        </Suspense>
+      );
+    }
+
+    // The hidden tree hasn't finished loading, but we should still be able to
+    // show the surrounding contents. The outer Suspense boundary
+    // isn't affected.
+    await act(() => {
+      root.render(<App />);
+    });
+    assertLog(['Visible', 'Suspend! [Hidden]']);
+    expect(root).toMatchRenderedOutput(<span>Visible</span>);
+
+    // When the data resolves, we should be able to finish prerendering
+    // the hidden tree.
+    await act(async () => {
+      await resolveText('Hidden');
+    });
+    assertLog(['Hidden']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span>Visible</span>
+        <span hidden={true}>Hidden</span>
+      </>,
+    );
+  });
+
+  // @gate enableLegacyHidden
+  test('LegacyHidden does not handle suspense', async () => {
+    const root = ReactNoop.createRoot();
+
+    function App() {
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <span>
+            <Text text="Visible" />
+          </span>
+          <LegacyHidden mode="hidden">
+            <span>
+              <AsyncText text="Hidden" />
+            </span>
+          </LegacyHidden>
+        </Suspense>
+      );
+    }
+
+    // Unlike Activity, LegacyHidden never captures if something suspends
+    await act(() => {
+      root.render(<App />);
+    });
+    assertLog(['Visible', 'Suspend! [Hidden]', 'Loading...']);
+    // Nearest Suspense boundary switches to a fallback even though the
+    // suspended content is hidden.
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span hidden={true}>Visible</span>
+        Loading...
+      </>,
+    );
+  });
+
+  // @gate __DEV__ && enableActivity
+  test('Regression: Suspending on hide should not infinite loop.', async () => {
+    // This regression only repros in public act.
+    global.IS_REACT_ACT_ENVIRONMENT = true;
+    const root = ReactNoop.createRoot();
+
+    let setMode;
+    function Container({text}) {
+      const [mode, _setMode] = React.useState('visible');
+      setMode = _setMode;
+      useEffect(() => {
+        return () => {
+          Scheduler.log(`Clear [${text}]`);
+          textCache.delete(text);
+        };
+      });
+      return (
+        //$FlowFixMe
+        <Suspense fallback="Loading">
+          <Activity mode={mode}>
+            <AsyncText text={text} />
+          </Activity>
+        </Suspense>
+      );
+    }
+
+    await React.act(() => {
+      root.render(<Container text="hello" />);
+    });
+    assertLog([
+      'Suspend! [hello]',
+      ...(gate(flags => flags.enableSiblingPrerendering)
+        ? ['Suspend! [hello]']
+        : []),
+    ]);
+    expect(root).toMatchRenderedOutput('Loading');
+
+    await React.act(async () => {
+      await resolveText('hello');
+    });
+    assertLog(['hello']);
+    expect(root).toMatchRenderedOutput('hello');
+
+    await React.act(async () => {
+      setMode('hidden');
+    });
+    assertLog(['Clear [hello]', 'Suspend! [hello]']);
+    expect(root).toMatchRenderedOutput('');
+  });
+
+  // @gate enableActivity
+  test("suspending inside currently hidden tree that's switching to visible", async () => {
+    const root = ReactNoop.createRoot();
+
+    function Details({open, children}) {
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <span>
+            <Text text={open ? 'Open' : 'Closed'} />
+          </span>
+          <Activity mode={open ? 'visible' : 'hidden'}>
+            <span>{children}</span>
+          </Activity>
+        </Suspense>
+      );
+    }
+
+    // The hidden tree hasn't finished loading, but we should still be able to
+    // show the surrounding contents. It doesn't matter that there's no
+    // Suspense boundary because the unfinished content isn't visible.
+    await act(() => {
+      root.render(
+        <Details open={false}>
+          <AsyncText text="Async" />
+        </Details>,
+      );
+    });
+    assertLog(['Closed', 'Suspend! [Async]']);
+    expect(root).toMatchRenderedOutput(<span>Closed</span>);
+
+    // But when we switch the boundary from hidden to visible, it should
+    // now bubble to the nearest Suspense boundary.
+    await act(() => {
+      startTransition(() => {
+        root.render(
+          <Details open={true}>
+            <AsyncText text="Async" />
+          </Details>,
+        );
+      });
+    });
+    assertLog(['Open', 'Suspend! [Async]', 'Loading...']);
+    // It should suspend with delay to prevent the already-visible Suspense
+    // boundary from switching to a fallback
+    expect(root).toMatchRenderedOutput(<span>Closed</span>);
+
+    // Resolve the data and finish rendering
+    await act(async () => {
+      await resolveText('Async');
+    });
+    assertLog(['Open', 'Async']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span>Open</span>
+        <span>Async</span>
+      </>,
+    );
+  });
+
+  // @gate enableActivity
+  test("suspending inside currently visible tree that's switching to hidden", async () => {
+    const root = ReactNoop.createRoot();
+
+    function Details({open, children}) {
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <span>
+            <Text text={open ? 'Open' : 'Closed'} />
+          </span>
+          <Activity mode={open ? 'visible' : 'hidden'}>
+            <span>{children}</span>
+          </Activity>
+        </Suspense>
+      );
+    }
+
+    // Initial mount. Nothing suspends
+    await act(() => {
+      root.render(
+        <Details open={true}>
+          <Text text="(empty)" />
+        </Details>,
+      );
+    });
+    assertLog(['Open', '(empty)']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span>Open</span>
+        <span>(empty)</span>
+      </>,
+    );
+
+    // Update that suspends inside the currently visible tree
+    await act(() => {
+      startTransition(() => {
+        root.render(
+          <Details open={true}>
+            <AsyncText text="Async" />
+          </Details>,
+        );
+      });
+    });
+    assertLog(['Open', 'Suspend! [Async]', 'Loading...']);
+    // It should suspend with delay to prevent the already-visible Suspense
+    // boundary from switching to a fallback
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span>Open</span>
+        <span>(empty)</span>
+      </>,
+    );
+
+    // Update that hides the suspended tree
+    await act(() => {
+      startTransition(() => {
+        root.render(
+          <Details open={false}>
+            <AsyncText text="Async" />
+          </Details>,
+        );
+      });
+    });
+    // Now the visible part of the tree can commit without being blocked
+    // by the suspended content, which is hidden.
+    assertLog(['Closed', 'Suspend! [Async]']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span>Closed</span>
+        <span hidden={true}>(empty)</span>
+      </>,
+    );
+
+    // Resolve the data and finish rendering
+    await act(async () => {
+      await resolveText('Async');
+    });
+    assertLog(['Async']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span>Closed</span>
+        <span hidden={true}>Async</span>
+      </>,
+    );
+  });
+
+  // @gate enableActivity
+  test('update that suspends inside hidden tree', async () => {
+    let setText;
+    function Child() {
+      const [text, _setText] = useState('A');
+      setText = _setText;
+      return <AsyncText text={text} />;
+    }
+
+    function App({show}) {
+      return (
+        <Activity mode={show ? 'visible' : 'hidden'}>
+          <span>
+            <Child />
+          </span>
+        </Activity>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    resolveText('A');
+    await act(() => {
+      root.render(<App show={false} />);
+    });
+    assertLog(['A']);
+
+    await act(() => {
+      startTransition(() => {
+        setText('B');
+      });
+    });
+  });
+
+  // @gate enableActivity
+  test('updates at multiple priorities that suspend inside hidden tree', async () => {
+    let setText;
+    let setStep;
+    function Child() {
+      const [text, _setText] = useState('A');
+      setText = _setText;
+
+      const [step, _setStep] = useState(0);
+      setStep = _setStep;
+
+      return <AsyncText text={text + step} />;
+    }
+
+    function App({show}) {
+      return (
+        <Activity mode={show ? 'visible' : 'hidden'}>
+          <span>
+            <Child />
+          </span>
+        </Activity>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    resolveText('A0');
+    await act(() => {
+      root.render(<App show={false} />);
+    });
+    assertLog(['A0']);
+    expect(root).toMatchRenderedOutput(<span hidden={true}>A0</span>);
+
+    await act(() => {
+      React.startTransition(() => {
+        setStep(1);
+      });
+      ReactNoop.flushSync(() => {
+        setText('B');
+      });
+    });
+    assertLog([
+      // The high priority render suspends again
+      'Suspend! [B0]',
+      // There's still pending work in another lane, so we should attempt
+      // that, too.
+      'Suspend! [B1]',
+    ]);
+    expect(root).toMatchRenderedOutput(<span hidden={true}>A0</span>);
+
+    // Resolve the data and finish rendering
+    await act(() => {
+      resolveText('B1');
+    });
+    assertLog(['B1']);
+    expect(root).toMatchRenderedOutput(<span hidden={true}>B1</span>);
+  });
+
+  // @gate enableActivity
+  test('detect updates to a hidden tree during a concurrent event', async () => {
+    // This is a pretty complex test case. It relates to how we detect if an
+    // update is made to a hidden tree: when scheduling the update, we walk up
+    // the fiber return path to see if any of the parents is a hidden Activity
+    // component. This doesn't work if there's already a render in progress,
+    // because the tree might be about to flip to hidden. To avoid a data race,
+    // queue updates atomically: wait to queue the update until after the
+    // current render has finished.
+
+    let setInner;
+    function Child({outer}) {
+      const [inner, _setInner] = useState(0);
+      setInner = _setInner;
+
+      useEffect(() => {
+        // Inner and outer values are always updated simultaneously, so they
+        // should always be consistent.
+        if (inner !== outer) {
+          Scheduler.log('Tearing! Inner and outer are inconsistent!');
+        } else {
+          Scheduler.log('Inner and outer are consistent');
+        }
+      }, [inner, outer]);
+
+      return <Text text={'Inner: ' + inner} />;
+    }
+
+    let setOuter;
+    function App({show}) {
+      const [outer, _setOuter] = useState(0);
+      setOuter = _setOuter;
+      return (
+        <>
+          <Activity mode={show ? 'visible' : 'hidden'}>
+            <span>
+              <Child outer={outer} />
+            </span>
+          </Activity>
+          <span>
+            <Text text={'Outer: ' + outer} />
+          </span>
+          <Suspense fallback={<Text text="Loading..." />}>
+            <span>
+              <Text text={'Sibling: ' + outer} />
+            </span>
+          </Suspense>
+        </>
+      );
+    }
+
+    // Render a hidden tree
+    const root = ReactNoop.createRoot();
+    resolveText('Async: 0');
+    await act(() => {
+      root.render(<App show={true} />);
+    });
+    assertLog([
+      'Inner: 0',
+      'Outer: 0',
+      'Sibling: 0',
+      'Inner and outer are consistent',
+    ]);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span>Inner: 0</span>
+        <span>Outer: 0</span>
+        <span>Sibling: 0</span>
+      </>,
+    );
+
+    await act(async () => {
+      // Update a value both inside and outside the hidden tree. These values
+      // must always be consistent.
+      startTransition(() => {
+        setOuter(1);
+        setInner(1);
+        // In the same render, also hide the offscreen tree.
+        root.render(<App show={false} />);
+      });
+
+      await waitFor([
+        // The outer update will commit, but the inner update is deferred until
+        // a later render.
+        'Outer: 1',
+      ]);
+
+      // Assert that we haven't committed quite yet
+      expect(root).toMatchRenderedOutput(
+        <>
+          <span>Inner: 0</span>
+          <span>Outer: 0</span>
+          <span>Sibling: 0</span>
+        </>,
+      );
+
+      // Before the tree commits, schedule a concurrent event. The inner update
+      // is to a tree that's just about to be hidden.
+      startTransition(() => {
+        setOuter(2);
+        setInner(2);
+      });
+
+      // Finish rendering and commit the in-progress render.
+      await waitForPaint(['Sibling: 1']);
+      expect(root).toMatchRenderedOutput(
+        <>
+          <span hidden={true}>Inner: 0</span>
+          <span>Outer: 1</span>
+          <span>Sibling: 1</span>
+        </>,
+      );
+
+      // Now reveal the hidden tree at high priority.
+      ReactNoop.flushSync(() => {
+        root.render(<App show={true} />);
+      });
+      assertLog([
+        // There are two pending updates on Inner, but only the first one
+        // is processed, even though they share the same lane. If the second
+        // update were erroneously processed, then Inner would be inconsistent
+        // with Outer.
+        'Inner: 1',
+        'Outer: 1',
+        'Sibling: 1',
+        'Inner and outer are consistent',
+      ]);
+    });
+    assertLog([
+      'Inner: 2',
+      'Outer: 2',
+      'Sibling: 2',
+      'Inner and outer are consistent',
+    ]);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span>Inner: 2</span>
+        <span>Outer: 2</span>
+        <span>Sibling: 2</span>
+      </>,
+    );
+  });
+});
diff --git a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js
index d02ae70e52..2fb35df457 100644
--- a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js
+++ b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js
@@ -12,6 +12,7 @@ let textCache;
 let waitFor;
 let waitForPaint;
 let assertLog;
+let use;
 
 describe('Activity Suspense', () => {
   beforeEach(() => {
@@ -27,6 +28,7 @@ describe('Activity Suspense', () => {
     useState = React.useState;
     useEffect = React.useEffect;
     startTransition = React.startTransition;
+    use = React.use;
 
     const InternalTestUtils = require('internal-test-utils');
     waitFor = InternalTestUtils.waitFor;
@@ -45,10 +47,10 @@ describe('Activity Suspense', () => {
       };
       textCache.set(text, newRecord);
     } else if (record.status === 'pending') {
-      const thenable = record.value;
+      const resolve = record.resolve;
       record.status = 'resolved';
       record.value = text;
-      thenable.pings.forEach(t => t());
+      resolve();
     }
   }
 
@@ -58,7 +60,7 @@ describe('Activity Suspense', () => {
       switch (record.status) {
         case 'pending':
           Scheduler.log(`Suspend! [${text}]`);
-          throw record.value;
+          return use(record.value);
         case 'rejected':
           throw record.value;
         case 'resolved':
@@ -66,24 +68,19 @@ describe('Activity Suspense', () => {
       }
     } else {
       Scheduler.log(`Suspend! [${text}]`);
-      const thenable = {
-        pings: [],
-        then(resolve) {
-          if (newRecord.status === 'pending') {
-            thenable.pings.push(resolve);
-          } else {
-            Promise.resolve().then(() => resolve(newRecord.value));
-          }
-        },
-      };
+      let resolve;
+      const promise = new Promise(_resolve => {
+        resolve = _resolve;
+      });
 
       const newRecord = {
         status: 'pending',
-        value: thenable,
+        value: promise,
+        resolve,
       };
       textCache.set(text, newRecord);
 
-      throw thenable;
+      return use(promise);
     }
   }
 
@@ -174,6 +171,56 @@ describe('Activity Suspense', () => {
     );
   });
 
+  // @gate __DEV__ && enableActivity
+  test('Regression: Suspending on hide should not infinite loop.', async () => {
+    // This regression only repros in public act.
+    global.IS_REACT_ACT_ENVIRONMENT = true;
+    const root = ReactNoop.createRoot();
+
+    let setMode;
+    function Container({text}) {
+      const [mode, _setMode] = React.useState('visible');
+      setMode = _setMode;
+      useEffect(() => {
+        return () => {
+          Scheduler.log(`Clear [${text}]`);
+          textCache.delete(text);
+        };
+      });
+      return (
+        //$FlowFixMe
+        <Suspense fallback="Loading">
+          <Activity mode={mode}>
+            <AsyncText text={text} />
+          </Activity>
+        </Suspense>
+      );
+    }
+
+    await React.act(() => {
+      root.render(<Container text="hello" />);
+    });
+    assertLog([
+      'Suspend! [hello]',
+      ...(gate(flags => flags.enableSiblingPrerendering)
+        ? ['Suspend! [hello]']
+        : []),
+    ]);
+    expect(root).toMatchRenderedOutput('Loading');
+
+    await React.act(async () => {
+      await resolveText('hello');
+    });
+    assertLog(['hello']);
+    expect(root).toMatchRenderedOutput('hello');
+
+    await React.act(() => {
+      setMode('hidden');
+    });
+    assertLog(['Clear [hello]', 'Suspend! [hello]']);
+    expect(root).toMatchRenderedOutput('');
+  });
+
   // @gate enableActivity
   test("suspending inside currently hidden tree that's switching to visible", async () => {
     const root = ReactNoop.createRoot();
@@ -215,7 +262,11 @@ describe('Activity Suspense', () => {
         );
       });
     });
-    assertLog(['Open', 'Suspend! [Async]', 'Loading...']);
+    assertLog([
+      'Open',
+      'Suspend! [Async]',
+      ...(gate(flags => flags.enableSiblingPrerendering) ? ['Loading...'] : []),
+    ]);
     // It should suspend with delay to prevent the already-visible Suspense
     // boundary from switching to a fallback
     expect(root).toMatchRenderedOutput(<span>Closed</span>);
@@ -224,7 +275,10 @@ describe('Activity Suspense', () => {
     await act(async () => {
       await resolveText('Async');
     });
-    assertLog(['Open', 'Async']);
+    assertLog([
+      ...(gate(flags => flags.enableSiblingPrerendering) ? ['Open'] : []),
+      'Async',
+    ]);
     expect(root).toMatchRenderedOutput(
       <>
         <span>Open</span>
@@ -276,7 +330,11 @@ describe('Activity Suspense', () => {
         );
       });
     });
-    assertLog(['Open', 'Suspend! [Async]', 'Loading...']);
+    assertLog([
+      'Open',
+      'Suspend! [Async]',
+      ...(gate(flags => flags.enableSiblingPrerendering) ? ['Loading...'] : []),
+    ]);
     // It should suspend with delay to prevent the already-visible Suspense
     // boundary from switching to a fallback
     expect(root).toMatchRenderedOutput(

From 2ec26bc4323673d1f2035191d3aaf0a18b20d488 Mon Sep 17 00:00:00 2001
From: mofeiZ <34200447+mofeiZ@users.noreply.github.com>
Date: Mon, 11 Nov 2024 18:04:29 -0500
Subject: [PATCH 2/3] [compiler] Repro for mutable range edge case (#31479)

See test fixtures
---
 ...g-aliased-capture-aliased-mutate.expect.md | 107 ++++++++++++++++++
 .../bug-aliased-capture-aliased-mutate.js     |  53 +++++++++
 .../bug-aliased-capture-mutate.expect.md      |  87 ++++++++++++++
 .../compiler/bug-aliased-capture-mutate.js    |  34 ++++++
 .../packages/snap/src/SproutTodoFilter.ts     |   2 +
 5 files changed, 283 insertions(+)
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-aliased-mutate.expect.md
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-aliased-mutate.js
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.expect.md
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.js

diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-aliased-mutate.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-aliased-mutate.expect.md
new file mode 100644
index 0000000000..d0ad9e2f9d
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-aliased-mutate.expect.md
@@ -0,0 +1,107 @@
+
+## Input
+
+```javascript
+// @flow @enableTransitivelyFreezeFunctionExpressions:false
+import {arrayPush, setPropertyByKey, Stringify} from 'shared-runtime';
+
+/**
+ * 1. `InferMutableRanges` derives the mutable range of identifiers and their
+ *     aliases from `LoadLocal`, `PropertyLoad`, etc
+ *   - After this pass, y's mutable range only extends to `arrayPush(x, y)`
+ *   - We avoid assigning mutable ranges to loads after y's mutable range, as
+ *     these are working with an immutable value. As a result, `LoadLocal y` and
+ *     `PropertyLoad y` do not get mutable ranges
+ * 2. `InferReactiveScopeVariables` extends mutable ranges and creates scopes,
+ *    as according to the 'co-mutation' of different values
+ *   - Here, we infer that
+ *     - `arrayPush(y, x)` might alias `x` and `y` to each other
+ *     - `setPropertyKey(x, ...)` may mutate both `x` and `y`
+ *   - This pass correctly extends the mutable range of `y`
+ *   - Since we didn't run `InferMutableRange` logic again, the LoadLocal /
+ *     PropertyLoads still don't have a mutable range
+ *
+ * Note that the this bug is an edge case. Compiler output is only invalid for:
+ *  - function expressions with
+ *    `enableTransitivelyFreezeFunctionExpressions:false`
+ *  - functions that throw and get retried without clearing the memocache
+ *
+ * Found differences in evaluator results
+ * Non-forget (expected):
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":10},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":11},"shouldInvokeFns":true}</div>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":10},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":10},"shouldInvokeFns":true}</div>
+ */
+function useFoo({a, b}: {a: number, b: number}) {
+  const x = [];
+  const y = {value: a};
+
+  arrayPush(x, y); // x and y co-mutate
+  const y_alias = y;
+  const cb = () => y_alias.value;
+  setPropertyByKey(x[0], 'value', b); // might overwrite y.value
+  return <Stringify cb={cb} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [{a: 2, b: 10}],
+  sequentialRenders: [
+    {a: 2, b: 10},
+    {a: 2, b: 11},
+  ],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import { arrayPush, setPropertyByKey, Stringify } from "shared-runtime";
+
+function useFoo(t0) {
+  const $ = _c(5);
+  const { a, b } = t0;
+  let t1;
+  if ($[0] !== a || $[1] !== b) {
+    const x = [];
+    const y = { value: a };
+
+    arrayPush(x, y);
+    const y_alias = y;
+    let t2;
+    if ($[3] !== y_alias.value) {
+      t2 = () => y_alias.value;
+      $[3] = y_alias.value;
+      $[4] = t2;
+    } else {
+      t2 = $[4];
+    }
+    const cb = t2;
+    setPropertyByKey(x[0], "value", b);
+    t1 = <Stringify cb={cb} shouldInvokeFns={true} />;
+    $[0] = a;
+    $[1] = b;
+    $[2] = t1;
+  } else {
+    t1 = $[2];
+  }
+  return t1;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [{ a: 2, b: 10 }],
+  sequentialRenders: [
+    { a: 2, b: 10 },
+    { a: 2, b: 11 },
+  ],
+};
+
+```
+      
\ No newline at end of file
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-aliased-mutate.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-aliased-mutate.js
new file mode 100644
index 0000000000..c46ecd6250
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-aliased-mutate.js
@@ -0,0 +1,53 @@
+// @flow @enableTransitivelyFreezeFunctionExpressions:false
+import {arrayPush, setPropertyByKey, Stringify} from 'shared-runtime';
+
+/**
+ * 1. `InferMutableRanges` derives the mutable range of identifiers and their
+ *     aliases from `LoadLocal`, `PropertyLoad`, etc
+ *   - After this pass, y's mutable range only extends to `arrayPush(x, y)`
+ *   - We avoid assigning mutable ranges to loads after y's mutable range, as
+ *     these are working with an immutable value. As a result, `LoadLocal y` and
+ *     `PropertyLoad y` do not get mutable ranges
+ * 2. `InferReactiveScopeVariables` extends mutable ranges and creates scopes,
+ *    as according to the 'co-mutation' of different values
+ *   - Here, we infer that
+ *     - `arrayPush(y, x)` might alias `x` and `y` to each other
+ *     - `setPropertyKey(x, ...)` may mutate both `x` and `y`
+ *   - This pass correctly extends the mutable range of `y`
+ *   - Since we didn't run `InferMutableRange` logic again, the LoadLocal /
+ *     PropertyLoads still don't have a mutable range
+ *
+ * Note that the this bug is an edge case. Compiler output is only invalid for:
+ *  - function expressions with
+ *    `enableTransitivelyFreezeFunctionExpressions:false`
+ *  - functions that throw and get retried without clearing the memocache
+ *
+ * Found differences in evaluator results
+ * Non-forget (expected):
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":10},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":11},"shouldInvokeFns":true}</div>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":10},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":10},"shouldInvokeFns":true}</div>
+ */
+function useFoo({a, b}: {a: number, b: number}) {
+  const x = [];
+  const y = {value: a};
+
+  arrayPush(x, y); // x and y co-mutate
+  const y_alias = y;
+  const cb = () => y_alias.value;
+  setPropertyByKey(x[0], 'value', b); // might overwrite y.value
+  return <Stringify cb={cb} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [{a: 2, b: 10}],
+  sequentialRenders: [
+    {a: 2, b: 10},
+    {a: 2, b: 11},
+  ],
+};
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.expect.md
new file mode 100644
index 0000000000..c35efe6a16
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.expect.md
@@ -0,0 +1,87 @@
+
+## Input
+
+```javascript
+// @flow @enableTransitivelyFreezeFunctionExpressions:false
+import {setPropertyByKey, Stringify} from 'shared-runtime';
+
+/**
+ * Variation of bug in `bug-aliased-capture-aliased-mutate`
+ * Found differences in evaluator results
+ * Non-forget (expected):
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":3},"shouldInvokeFns":true}</div>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
+ */
+
+function useFoo({a}: {a: number, b: number}) {
+  const arr = [];
+  const obj = {value: a};
+
+  setPropertyByKey(obj, 'arr', arr);
+  const obj_alias = obj;
+  const cb = () => obj_alias.arr.length;
+  for (let i = 0; i < a; i++) {
+    arr.push(i);
+  }
+  return <Stringify cb={cb} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [{a: 2}],
+  sequentialRenders: [{a: 2}, {a: 3}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import { setPropertyByKey, Stringify } from "shared-runtime";
+
+function useFoo(t0) {
+  const $ = _c(4);
+  const { a } = t0;
+  let t1;
+  if ($[0] !== a) {
+    const arr = [];
+    const obj = { value: a };
+
+    setPropertyByKey(obj, "arr", arr);
+    const obj_alias = obj;
+    let t2;
+    if ($[2] !== obj_alias.arr.length) {
+      t2 = () => obj_alias.arr.length;
+      $[2] = obj_alias.arr.length;
+      $[3] = t2;
+    } else {
+      t2 = $[3];
+    }
+    const cb = t2;
+    for (let i = 0; i < a; i++) {
+      arr.push(i);
+    }
+
+    t1 = <Stringify cb={cb} shouldInvokeFns={true} />;
+    $[0] = a;
+    $[1] = t1;
+  } else {
+    t1 = $[1];
+  }
+  return t1;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [{ a: 2 }],
+  sequentialRenders: [{ a: 2 }, { a: 3 }],
+};
+
+```
+      
\ No newline at end of file
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.js
new file mode 100644
index 0000000000..a7e5767266
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.js
@@ -0,0 +1,34 @@
+// @flow @enableTransitivelyFreezeFunctionExpressions:false
+import {setPropertyByKey, Stringify} from 'shared-runtime';
+
+/**
+ * Variation of bug in `bug-aliased-capture-aliased-mutate`
+ * Found differences in evaluator results
+ * Non-forget (expected):
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":3},"shouldInvokeFns":true}</div>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
+ */
+
+function useFoo({a}: {a: number, b: number}) {
+  const arr = [];
+  const obj = {value: a};
+
+  setPropertyByKey(obj, 'arr', arr);
+  const obj_alias = obj;
+  const cb = () => obj_alias.arr.length;
+  for (let i = 0; i < a; i++) {
+    arr.push(i);
+  }
+  return <Stringify cb={cb} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [{a: 2}],
+  sequentialRenders: [{a: 2}, {a: 3}],
+};
diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts
index b868afc52b..1971fe0d33 100644
--- a/compiler/packages/snap/src/SproutTodoFilter.ts
+++ b/compiler/packages/snap/src/SproutTodoFilter.ts
@@ -480,6 +480,8 @@ const skipFilter = new Set([
   'fbt/bug-fbt-plural-multiple-mixed-call-tag',
   'bug-object-expression-computed-key-modified-during-after-construction-hoisted-sequence-expr',
   'bug-invalid-hoisting-functionexpr',
+  'bug-aliased-capture-aliased-mutate',
+  'bug-aliased-capture-mutate',
   'bug-functiondecl-hoisting',
   'bug-try-catch-maybe-null-dependency',
   'reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted',

From 5388985a55eb5ae043d147298b20b0aa1c443b8d Mon Sep 17 00:00:00 2001
From: Mofei Zhang <feifei0@meta.com>
Date: Tue, 12 Nov 2024 12:45:58 -0500
Subject: [PATCH 3/3] [compiler] repro for reactive ref.current accesses

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
---
 .../compiler/bug-nonreactive-ref.expect.md    | 89 +++++++++++++++++++
 .../fixtures/compiler/bug-nonreactive-ref.tsx | 33 +++++++
 .../packages/snap/src/SproutTodoFilter.ts     |  1 +
 3 files changed, 123 insertions(+)
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md
 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx

diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md
new file mode 100644
index 0000000000..f79df15d52
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md
@@ -0,0 +1,89 @@
+
+## Input
+
+```javascript
+import {useRef} from 'react';
+import {Stringify} from 'shared-runtime';
+
+/**
+ * Bug: we're currently filtering out `ref.current` dependencies in
+ * `propagateScopeDependencies:checkValidDependency`. This is incorrect.
+ * Instead, we should always take a dependency on ref values (the outer box) as
+ * they may be reactive. Pruning should be done in
+ * `pruneNonReactiveDependencies`
+ *
+ * Found differences in evaluator results
+ * Non-forget (expected):
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+ */
+function Component({cond}) {
+  const ref1 = useRef(1);
+  const ref2 = useRef(2);
+  const ref = cond ? ref1 : ref2;
+  const cb = () => ref.current;
+  return <Stringify cb={cb} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{cond: true}],
+  sequentialRenders: [{cond: true}, {cond: false}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import { useRef } from "react";
+import { Stringify } from "shared-runtime";
+
+/**
+ * Bug: we're currently filtering out `ref.current` dependencies in
+ * `propagateScopeDependencies:checkValidDependency`. This is incorrect.
+ * Instead, we should always take a dependency on ref values (the outer box) as
+ * they may be reactive. Pruning should be done in
+ * `pruneNonReactiveDependencies`
+ *
+ * Found differences in evaluator results
+ * Non-forget (expected):
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+ */
+function Component(t0) {
+  const $ = _c(1);
+  const { cond } = t0;
+  const ref1 = useRef(1);
+  const ref2 = useRef(2);
+  const ref = cond ? ref1 : ref2;
+  let t1;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    const cb = () => ref.current;
+    t1 = <Stringify cb={cb} shouldInvokeFns={true} />;
+    $[0] = t1;
+  } else {
+    t1 = $[0];
+  }
+  return t1;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ cond: true }],
+  sequentialRenders: [{ cond: true }, { cond: false }],
+};
+
+```
+      
\ No newline at end of file
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx
new file mode 100644
index 0000000000..a0115f2df3
--- /dev/null
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx
@@ -0,0 +1,33 @@
+import {useRef} from 'react';
+import {Stringify} from 'shared-runtime';
+
+/**
+ * Bug: we're currently filtering out `ref.current` dependencies in
+ * `propagateScopeDependencies:checkValidDependency`. This is incorrect.
+ * Instead, we should always take a dependency on ref values (the outer box) as
+ * they may be reactive. Pruning should be done in
+ * `pruneNonReactiveDependencies`
+ *
+ * Found differences in evaluator results
+ * Non-forget (expected):
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+ *   <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+ */
+function Component({cond}) {
+  const ref1 = useRef(1);
+  const ref2 = useRef(2);
+  const ref = cond ? ref1 : ref2;
+  const cb = () => ref.current;
+  return <Stringify cb={cb} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{cond: true}],
+  sequentialRenders: [{cond: true}, {cond: false}],
+};
diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts
index 1971fe0d33..6c136f6310 100644
--- a/compiler/packages/snap/src/SproutTodoFilter.ts
+++ b/compiler/packages/snap/src/SproutTodoFilter.ts
@@ -484,6 +484,7 @@ const skipFilter = new Set([
   'bug-aliased-capture-mutate',
   'bug-functiondecl-hoisting',
   'bug-try-catch-maybe-null-dependency',
+  'bug-nonreactive-ref',
   'reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted',
   'bug-invalid-phi-as-dependency',
   'reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond',