From ff889d9c51ea3e5b1a9ae6c7fc95522ec4e91896 Mon Sep 17 00:00:00 2001
From: Mofei Zhang <feifei0@meta.com>
Date: Thu, 26 Dec 2024 20:03:48 -0500
Subject: [PATCH] [compiler] Fix invalid Array.map type

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
---
 .../src/HIR/ObjectShape.ts                    | 10 ++-
 ...d => mixedreadonly-mutating-map.expect.md} | 75 +++++++++----------
 ...shape.js => mixedreadonly-mutating-map.js} |  3 +
 .../packages/snap/src/SproutTodoFilter.ts     |  1 -
 4 files changed, 46 insertions(+), 43 deletions(-)
 rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{bug-invalid-mixedreadonly-map-shape.expect.md => mixedreadonly-mutating-map.expect.md} (78%)
 rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{bug-invalid-mixedreadonly-map-shape.js => mixedreadonly-mutating-map.js} (92%)

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts
index 4482d17890196..337f0648dc671 100644
--- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts
+++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts
@@ -545,8 +545,16 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [
   [
     'map',
     addFunction(BUILTIN_SHAPES, [], {
+      /**
+       * Note `map`'s arguments are annotated as Effect.ConditionallyMutate as
+       * calling `<array>.map(fn)` might invoke `fn`, which means replaying its
+       * effects.
+       *
+       * (Note that -- Effect.Read / Effect.Capture on a function type means
+       * potential data dependency or aliasing respectively.)
+       */
       positionalParams: [],
-      restParam: Effect.Read,
+      restParam: Effect.ConditionallyMutate,
       returnType: {kind: 'Object', shapeId: BuiltInArrayId},
       calleeEffect: Effect.ConditionallyMutate,
       returnValueKind: ValueKind.Mutable,
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md
similarity index 78%
rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md
rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md
index 1418062c33458..4867388a864d2 100644
--- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md
@@ -36,6 +36,9 @@ import {
 function Component({extraJsx}) {
   const x = makeArray();
   const items = useFragment();
+  // This closure has the following effects that must be replayed:
+  //   - MaybeFreeze / Capture of `items`
+  //   - ConditionalMutate of x
   const jsx = items.a.map((item, i) => {
     arrayPush(x, 2);
     return <Stringify item={item} key={i} />;
@@ -97,60 +100,47 @@ import {
  */
 
 function Component(t0) {
-  const $ = _c(9);
+  const $ = _c(6);
   const { extraJsx } = t0;
-  let t1;
-  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
-    t1 = makeArray();
-    $[0] = t1;
-  } else {
-    t1 = $[0];
-  }
-  const x = t1;
+  const x = makeArray();
   const items = useFragment();
-  let jsx;
-  if ($[1] !== extraJsx || $[2] !== items.a) {
-    jsx = items.a.map((item, i) => {
-      arrayPush(x, 2);
-      return <Stringify item={item} key={i} />;
-    });
-    const offset = jsx.length;
-    for (let i_0 = 0; i_0 < extraJsx; i_0++) {
-      jsx.push(<Stringify item={0} key={i_0 + offset} />);
-    }
-    $[1] = extraJsx;
-    $[2] = items.a;
-    $[3] = jsx;
-  } else {
-    jsx = $[3];
+
+  const jsx = items.a.map((item, i) => {
+    arrayPush(x, 2);
+    return <Stringify item={item} key={i} />;
+  });
+  const offset = jsx.length;
+  for (let i_0 = 0; i_0 < extraJsx; i_0++) {
+    jsx.push(<Stringify item={0} key={i_0 + offset} />);
   }
 
   const count = jsx.length;
   identity(count);
-  let t2;
-  if ($[4] !== count) {
-    t2 = <Stringify x={x} count={count} />;
-    $[4] = count;
-    $[5] = t2;
+  let t1;
+  if ($[0] !== count || $[1] !== x) {
+    t1 = <Stringify x={x} count={count} />;
+    $[0] = count;
+    $[1] = x;
+    $[2] = t1;
   } else {
-    t2 = $[5];
+    t1 = $[2];
   }
-  const t3 = jsx[0];
-  let t4;
-  if ($[6] !== t2 || $[7] !== t3) {
-    t4 = (
+  const t2 = jsx[0];
+  let t3;
+  if ($[3] !== t1 || $[4] !== t2) {
+    t3 = (
       <>
+        {t1}
         {t2}
-        {t3}
       </>
     );
-    $[6] = t2;
-    $[7] = t3;
-    $[8] = t4;
+    $[3] = t1;
+    $[4] = t2;
+    $[5] = t3;
   } else {
-    t4 = $[8];
+    t3 = $[5];
   }
-  return t4;
+  return t3;
 }
 
 export const FIXTURE_ENTRYPOINT = {
@@ -160,4 +150,7 @@ export const FIXTURE_ENTRYPOINT = {
 };
 
 ```
-      
\ No newline at end of file
+      
+### Eval output
+(kind: ok) <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
+<div>{"x":[2,2,2],"count":4}</div><div>{"item":1}</div>
\ No newline at end of file
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js
similarity index 92%
rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js
rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js
index d038cf6cd38a1..858a4ab3dc693 100644
--- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js
+++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js
@@ -32,6 +32,9 @@ import {
 function Component({extraJsx}) {
   const x = makeArray();
   const items = useFragment();
+  // This closure has the following effects that must be replayed:
+  //   - MaybeFreeze / Capture of `items`
+  //   - ConditionalMutate of x
   const jsx = items.a.map((item, i) => {
     arrayPush(x, 2);
     return <Stringify item={item} key={i} />;
diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts
index f363fd922e51a..361070739630c 100644
--- a/compiler/packages/snap/src/SproutTodoFilter.ts
+++ b/compiler/packages/snap/src/SproutTodoFilter.ts
@@ -486,7 +486,6 @@ const skipFilter = new Set([
   'bug-aliased-capture-mutate',
   'bug-functiondecl-hoisting',
   'bug-try-catch-maybe-null-dependency',
-  'bug-invalid-mixedreadonly-map-shape',
   'bug-type-inference-control-flow',
   'reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted',
   'bug-invalid-phi-as-dependency',