[compiler] Bailout on mutations of frozen/global values

ghstack-source-id: ea86f072a1
Pull Request resolved: https://github.com/facebook/react/pull/33377

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

@@ -342,7 +342,7 @@ function isEffectSafeOutsideRender(effect: FunctionEffect): boolean {
   return effect.kind === 'GlobalMutation';
 }
 
-function getWriteErrorReason(abstractValue: AbstractValue): string {
+export function getWriteErrorReason(abstractValue: AbstractValue): string {
   if (abstractValue.reason.has(ValueReason.Global)) {
     return 'Writing to a variable defined outside a component or hook is not allowed. Consider using an effect';
   } else if (abstractValue.reason.has(ValueReason.JsxCaptured)) {

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

@@ -46,6 +46,7 @@ import {
   printSourceLocation,
 } from '../HIR/PrintHIR';
 import {FunctionSignature} from '../HIR/ObjectShape';
+import {getWriteErrorReason} from './InferFunctionEffects';
 
 export function inferMutationAliasingEffects(
   fn: HIRFunction,
@@ -435,11 +436,11 @@ function applySignature(
       case 'Apply': {
         const values = state.values(effect.function.place);
         if (values.length !== 1 || values[0].kind !== 'FunctionExpression') {
-          const didMutate = state.mutate(
+          const mutationKind = state.mutate(
             'MutateTransitiveConditionally',
             effect.function.place,
           );
-          if (didMutate) {
+          if (mutationKind === 'mutate') {
             effects.push({
               kind: 'MutateTransitiveConditionally',
               value: effect.function.place,
@@ -457,9 +458,29 @@ function applySignature(
       case 'MutateConditionally':
       case 'MutateTransitive':
       case 'MutateTransitiveConditionally': {
-        const didMutate = state.mutate(effect.kind, effect.value);
-        if (didMutate) {
+        const mutationKind = state.mutate(effect.kind, effect.value);
+        if (mutationKind === 'mutate') {
           effects.push(effect);
+        } else if (
+          mutationKind !== 'none' &&
+          (effect.kind === 'Mutate' || effect.kind === 'MutateTransitive')
+        ) {
+          const value = state.kind(effect.value);
+          const reason = getWriteErrorReason({
+            kind: value.kind,
+            reason: value.reason,
+            context: new Set(),
+          });
+          CompilerError.throwInvalidReact({
+            reason,
+            description:
+              effect.value.identifier.name !== null &&
+              effect.value.identifier.name.kind === 'named'
+                ? `Found mutation of \`${effect.value.identifier.name}\``
+                : null,
+            loc: effect.value.loc,
+            suggestions: null,
+          });
         }
         break;
       }
@@ -663,7 +684,7 @@ class InferenceState {
       | 'MutateTransitive'
       | 'MutateTransitiveConditionally',
     place: Place,
-  ): boolean {
+  ): 'none' | 'mutate' | 'mutate-frozen' | 'mutate-global' {
     // TODO: consider handling of function expressions by looking at their effects
     const kind = this.kind(place).kind;
     switch (variant) {
@@ -672,10 +693,10 @@ class InferenceState {
         switch (kind) {
           case ValueKind.Mutable:
           case ValueKind.Context: {
-            return true;
+            return 'mutate';
           }
           default: {
-            return false;
+            return 'none';
           }
         }
       }
@@ -684,15 +705,23 @@ class InferenceState {
         switch (kind) {
           case ValueKind.Mutable:
           case ValueKind.Context: {
-            return true;
+            return 'mutate';
           }
           case ValueKind.Primitive: {
             // technically an error, but it's not React specific
-            return false;
+            return 'none';
+          }
+          case ValueKind.Frozen: {
+            return 'mutate-frozen';
+          }
+          case ValueKind.Global: {
+            return 'mutate-global';
+          }
+          case ValueKind.MaybeFrozen: {
+            return 'none';
           }
           default: {
-            // TODO this is an error!
-            return false;
+            assertExhaustive(kind, `Unexpected kind ${kind}`);
           }
         }
       }
@@ -1082,10 +1111,23 @@ function computeSignatureForInstruction(
     }
     case 'ObjectMethod':
     case 'FunctionExpression': {
+      /*
+       * We consider the function frozen if it has no potential mutations of its context
+       * variables.
+       * TODO: this may lose some precision relative to InferReferenceEffects, where we
+       * decide the valuekind of the function based on the actual state of the context
+       * vars. Maybe a `CreateFunction captures=[...] into=place` effect or similar to
+       * express the logic
+       */
+      const kind = value.loweredFunc.func.context.some(
+        operand => operand.effect === Effect.Capture,
+      )
+        ? ValueKind.Mutable
+        : ValueKind.Frozen;
       effects.push({
         kind: 'Create',
         into: lvalue,
-        value: ValueKind.Mutable,
+        value: kind,
       });
       /**
        * We've already analyzed the function expression in AnalyzeFunctions. There, we assign
@@ -1372,11 +1414,16 @@ function computeEffectsForLegacySignature(
         break;
       }
       case Effect.ConditionallyMutateIterator: {
-        // We don't currently use this effect in any signatures
-        CompilerError.throwTodo({
-          reason: `Unexpected ${effect} effect in a legacy function signature`,
-          loc: place.loc,
+        effects.push({
+          kind: 'Capture',
+          from: place,
+          into: lvalue,
         });
+        effects.push({
+          kind: 'MutateTransitiveConditionally',
+          value: place,
+        });
+        break;
       }
       case Effect.Freeze: {
         effects.push({

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.mutate-frozen-value.expect.md

@@ -0,0 +1,28 @@
+
+## Input
+
+```javascript
+// @enableNewMutationAliasingModel
+function Component({a, b}) {
+  const x = {a};
+  useFreeze(x);
+  x.y = true;
+  return <div>error</div>;
+}
+
+```
+
+
+## Error
+
+```
+  3 |   const x = {a};
+  4 |   useFreeze(x);
+> 5 |   x.y = true;
+    |   ^ InvalidReact: This mutates a variable that React considers immutable (5:5)
+  6 |   return <div>error</div>;
+  7 | }
+  8 |
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.mutate-frozen-value.js

@@ -0,0 +1,7 @@
+// @enableNewMutationAliasingModel
+function Component({a, b}) {
+  const x = {a};
+  useFreeze(x);
+  x.y = true;
+  return <div>error</div>;
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/nullable-objects-assume-invoked-direct-call.expect.md

@@ -0,0 +1,75 @@
+
+## Input
+
+```javascript
+// @enableNewMutationAliasingModel
+import {useState} from 'react';
+import {useIdentity} from 'shared-runtime';
+
+function useMakeCallback({obj}: {obj: {value: number}}) {
+  const [state, setState] = useState(0);
+  const cb = () => {
+    if (obj.value !== state) setState(obj.value);
+  };
+  useIdentity();
+  cb();
+  return [cb];
+}
+export const FIXTURE_ENTRYPOINT = {
+  fn: useMakeCallback,
+  params: [{obj: {value: 1}}],
+  sequentialRenders: [{obj: {value: 1}}, {obj: {value: 2}}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @enableNewMutationAliasingModel
+import { useState } from "react";
+import { useIdentity } from "shared-runtime";
+
+function useMakeCallback(t0) {
+  const $ = _c(5);
+  const { obj } = t0;
+  const [state, setState] = useState(0);
+  let t1;
+  if ($[0] !== obj.value || $[1] !== state) {
+    t1 = () => {
+      if (obj.value !== state) {
+        setState(obj.value);
+      }
+    };
+    $[0] = obj.value;
+    $[1] = state;
+    $[2] = t1;
+  } else {
+    t1 = $[2];
+  }
+  const cb = t1;
+
+  useIdentity();
+  cb();
+  let t2;
+  if ($[3] !== cb) {
+    t2 = [cb];
+    $[3] = cb;
+    $[4] = t2;
+  } else {
+    t2 = $[4];
+  }
+  return t2;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useMakeCallback,
+  params: [{ obj: { value: 1 } }],
+  sequentialRenders: [{ obj: { value: 1 } }, { obj: { value: 2 } }],
+};
+
+```
+      
+### Eval output
+(kind: ok) ["[[ function params=0 ]]"]
+["[[ function params=0 ]]"]

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/nullable-objects-assume-invoked-direct-call.js

@@ -0,0 +1,18 @@
+// @enableNewMutationAliasingModel
+import {useState} from 'react';
+import {useIdentity} from 'shared-runtime';
+
+function useMakeCallback({obj}: {obj: {value: number}}) {
+  const [state, setState] = useState(0);
+  const cb = () => {
+    if (obj.value !== state) setState(obj.value);
+  };
+  useIdentity();
+  cb();
+  return [cb];
+}
+export const FIXTURE_ENTRYPOINT = {
+  fn: useMakeCallback,
+  params: [{obj: {value: 1}}],
+  sequentialRenders: [{obj: {value: 1}}, {obj: {value: 2}}],
+};