[compiler] Fix invalid Array.map type

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,

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

@@ -41,11 +41,16 @@ function inferOperandEffect(state: State, place: Place): null | FunctionEffect {
       if (isRefOrRefValue(place.identifier)) {
         break;
       } else if (value.kind === ValueKind.Context) {
+        CompilerError.invariant(value.context.size > 0, {
+          reason:
+            "[InferFunctionEffects] Expected Context-kind value's capture list to be non-empty.",
+          loc: place.loc,
+        });
         return {
           kind: 'ContextMutation',
           loc: place.loc,
           effect: place.effect,
-          places: value.context.size === 0 ? new Set([place]) : value.context,
+          places: value.context,
         };
       } else if (
         value.kind !== ValueKind.Mutable &&

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

@@ -857,17 +857,19 @@ function inferBlock(
         break;
       }
       case 'ArrayExpression': {
-        const valueKind: AbstractValue = hasContextRefOperand(state, instrValue)
-          ? {
-              kind: ValueKind.Context,
-              reason: new Set([ValueReason.Other]),
-              context: new Set(),
-            }
-          : {
-              kind: ValueKind.Mutable,
-              reason: new Set([ValueReason.Other]),
-              context: new Set(),
-            };
+        const contextRefOperands = getContextRefOperand(state, instrValue);
+        const valueKind: AbstractValue =
+          contextRefOperands.length > 0
+            ? {
+                kind: ValueKind.Context,
+                reason: new Set([ValueReason.Other]),
+                context: new Set(contextRefOperands),
+              }
+            : {
+                kind: ValueKind.Mutable,
+                reason: new Set([ValueReason.Other]),
+                context: new Set(),
+              };
         continuation = {
           kind: 'initialize',
           valueKind,
@@ -918,17 +920,19 @@ function inferBlock(
         break;
       }
       case 'ObjectExpression': {
-        const valueKind: AbstractValue = hasContextRefOperand(state, instrValue)
-          ? {
-              kind: ValueKind.Context,
-              reason: new Set([ValueReason.Other]),
-              context: new Set(),
-            }
-          : {
-              kind: ValueKind.Mutable,
-              reason: new Set([ValueReason.Other]),
-              context: new Set(),
-            };
+        const contextRefOperands = getContextRefOperand(state, instrValue);
+        const valueKind: AbstractValue =
+          contextRefOperands.length > 0
+            ? {
+                kind: ValueKind.Context,
+                reason: new Set([ValueReason.Other]),
+                context: new Set(contextRefOperands),
+              }
+            : {
+                kind: ValueKind.Mutable,
+                reason: new Set([ValueReason.Other]),
+                context: new Set(),
+              };
 
         for (const property of instrValue.properties) {
           switch (property.kind) {
@@ -1593,15 +1597,21 @@ function inferBlock(
       }
       case 'LoadLocal': {
         const lvalue = instr.lvalue;
-        const effect =
-          state.isDefined(lvalue) &&
-          state.kind(lvalue).kind === ValueKind.Context
-            ? Effect.ConditionallyMutate
-            : Effect.Capture;
+        CompilerError.invariant(
+          !(
+            state.isDefined(lvalue) &&
+            state.kind(lvalue).kind === ValueKind.Context
+          ),
+          {
+            reason:
+              '[InferReferenceEffects] Unexpected LoadLocal with context kind',
+            loc: lvalue.loc,
+          },
+        );
         state.referenceAndRecordEffects(
           freezeActions,
           instrValue.place,
-          effect,
+          Effect.Capture,
           ValueReason.Other,
         );
         lvalue.effect = Effect.ConditionallyMutate;
@@ -1932,19 +1942,20 @@ function inferBlock(
   );
 }
 
-function hasContextRefOperand(
+function getContextRefOperand(
   state: InferenceState,
   instrValue: InstructionValue,
-): boolean {
+): Array<Place> {
+  const result = [];
   for (const place of eachInstructionValueOperand(instrValue)) {
     if (
       state.isDefined(place) &&
       state.kind(place).kind === ValueKind.Context
     ) {
-      return true;
+      result.push(place);
     }
   }
-  return false;
+  return result;
 }
 
 export function getFunctionCallSignature(

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md

@@ -0,0 +1,156 @@
+
+## Input
+
+```javascript
+import {
+  arrayPush,
+  identity,
+  makeArray,
+  Stringify,
+  useFragment,
+} from 'shared-runtime';
+
+/**
+ * Bug repro showing why it's invalid for function references to be annotated
+ * with a `Read` effect when that reference might lead to the function being
+ * invoked.
+ *
+ * Note that currently, `Array.map` is annotated to have `Read` effects on its
+ * operands. This is incorrect as function effects must be replayed when `map`
+ * is called
+ * - Read:                non-aliasing data dependency
+ * - Capture:             maybe-aliasing data dependency
+ * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke
+ *     but only if the value is mutable
+ *
+ * Invalid evaluator result: Found differences in evaluator results Non-forget
+ * (expected): (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>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
+ *   <div>{"x":[2,2,2,2,2,2],"count":4}</div><div>{"item":1}</div>
+ */
+
+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} />;
+  });
+  const offset = jsx.length;
+  for (let i = 0; i < extraJsx; i++) {
+    jsx.push(<Stringify item={0} key={i + offset} />);
+  }
+  const count = jsx.length;
+  identity(count);
+  return (
+    <>
+      <Stringify x={x} count={count} />
+      {jsx[0]}
+    </>
+  );
+}
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{extraJsx: 0}],
+  sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import {
+  arrayPush,
+  identity,
+  makeArray,
+  Stringify,
+  useFragment,
+} from "shared-runtime";
+
+/**
+ * Bug repro showing why it's invalid for function references to be annotated
+ * with a `Read` effect when that reference might lead to the function being
+ * invoked.
+ *
+ * Note that currently, `Array.map` is annotated to have `Read` effects on its
+ * operands. This is incorrect as function effects must be replayed when `map`
+ * is called
+ * - Read:                non-aliasing data dependency
+ * - Capture:             maybe-aliasing data dependency
+ * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke
+ *     but only if the value is mutable
+ *
+ * Invalid evaluator result: Found differences in evaluator results Non-forget
+ * (expected): (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>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
+ *   <div>{"x":[2,2,2,2,2,2],"count":4}</div><div>{"item":1}</div>
+ */
+
+function Component(t0) {
+  const $ = _c(6);
+  const { extraJsx } = t0;
+  const x = makeArray();
+  const items = useFragment();
+
+  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 t1;
+  if ($[0] !== count || $[1] !== x) {
+    t1 = <Stringify x={x} count={count} />;
+    $[0] = count;
+    $[1] = x;
+    $[2] = t1;
+  } else {
+    t1 = $[2];
+  }
+  const t2 = jsx[0];
+  let t3;
+  if ($[3] !== t1 || $[4] !== t2) {
+    t3 = (
+      <>
+        {t1}
+        {t2}
+      </>
+    );
+    $[3] = t1;
+    $[4] = t2;
+    $[5] = t3;
+  } else {
+    t3 = $[5];
+  }
+  return t3;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ extraJsx: 0 }],
+  sequentialRenders: [{ extraJsx: 0 }, { extraJsx: 1 }],
+};
+
+```
+
+### 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>

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js

@@ -0,0 +1,59 @@
+import {
+  arrayPush,
+  identity,
+  makeArray,
+  Stringify,
+  useFragment,
+} from 'shared-runtime';
+
+/**
+ * Bug repro showing why it's invalid for function references to be annotated
+ * with a `Read` effect when that reference might lead to the function being
+ * invoked.
+ *
+ * Note that currently, `Array.map` is annotated to have `Read` effects on its
+ * operands. This is incorrect as function effects must be replayed when `map`
+ * is called
+ * - Read:                non-aliasing data dependency
+ * - Capture:             maybe-aliasing data dependency
+ * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke
+ *     but only if the value is mutable
+ *
+ * Invalid evaluator result: Found differences in evaluator results Non-forget
+ * (expected): (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>
+ * Forget:
+ *   (kind: ok)
+ *   <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
+ *   <div>{"x":[2,2,2,2,2,2],"count":4}</div><div>{"item":1}</div>
+ */
+
+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} />;
+  });
+  const offset = jsx.length;
+  for (let i = 0; i < extraJsx; i++) {
+    jsx.push(<Stringify item={0} key={i + offset} />);
+  }
+  const count = jsx.length;
+  identity(count);
+  return (
+    <>
+      <Stringify x={x} count={count} />
+      {jsx[0]}
+    </>
+  );
+}
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{extraJsx: 0}],
+  sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}],
+};