[compiler][newinference] Improve hoisted functions, validation of mutate-after-render

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts

@@ -247,6 +247,7 @@ function runWithEnvironment(
   }
 
   if (!env.config.enableNewMutationAliasingModel) {
+    // NOTE: in the new model this is part of validateNoFreezingKnownMutableFunctions
     validateLocalsNotReassignedAfterRender(hir);
   }
 

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

@@ -527,7 +527,11 @@ function applyEffect(
     }
     case 'CreateFunction': {
       effects.push(effect);
-      const isMutable = effect.captures.some(capture => {
+      /**
+       * We consider the function mutable if it has any mutable context variables or
+       * any side-effects that need to be tracked if the function is called.
+       */
+      const hasCaptures = effect.captures.some(capture => {
         switch (state.kind(capture).kind) {
           case ValueKind.Context:
           case ValueKind.Mutable: {
@@ -538,6 +542,12 @@ function applyEffect(
           }
         }
       });
+      const hasTrackedSideEffects =
+        effect.function.loweredFunc.func.aliasingEffects?.some(
+          effect =>
+            effect.kind === 'MutateFrozen' || effect.kind === 'MutateGlobal',
+        );
+      const isMutable = hasCaptures || hasTrackedSideEffects;
       for (const operand of effect.function.loweredFunc.func.context) {
         if (operand.effect !== Effect.Capture) {
           continue;
@@ -1593,23 +1603,6 @@ function computeSignatureForInstruction(
       }
       break;
     }
-    case 'DeclareContext': {
-      // Context variables are conceptually like mutable boxes
-      effects.push({
-        kind: 'Create',
-        into: value.lvalue.place,
-        value: ValueKind.Mutable,
-        reason: ValueReason.Other,
-      });
-      effects.push({
-        kind: 'Create',
-        into: lvalue,
-        // The result can't be referenced so this value doesn't matter
-        value: ValueKind.Primitive,
-        reason: ValueReason.Other,
-      });
-      break;
-    }
     case 'DeclareLocal': {
       // TODO check this
       effects.push({
@@ -1648,6 +1641,43 @@ function computeSignatureForInstruction(
       effects.push({kind: 'CreateFrom', from: value.place, into: lvalue});
       break;
     }
+    case 'DeclareContext': {
+      // Context variables are conceptually like mutable boxes
+      const kind = value.lvalue.kind;
+      if (
+        !context.hoistedContextDeclarations.has(
+          value.lvalue.place.identifier.declarationId,
+        ) ||
+        kind === InstructionKind.HoistedConst ||
+        kind === InstructionKind.HoistedFunction ||
+        kind === InstructionKind.HoistedLet
+      ) {
+        /**
+         * If this context variable is not hoisted, or this is the declaration doing the hoisting,
+         * then we create the box.
+         */
+        effects.push({
+          kind: 'Create',
+          into: value.lvalue.place,
+          value: ValueKind.Mutable,
+          reason: ValueReason.Other,
+        });
+      } else {
+        /**
+         * Otherwise this may be a "declare", but there was a previous DeclareContext that
+         * hoisted this variable, and we're mutating it here.
+         */
+        effects.push({kind: 'Mutate', value: value.lvalue.place});
+      }
+      effects.push({
+        kind: 'Create',
+        into: lvalue,
+        // The result can't be referenced so this value doesn't matter
+        value: ValueKind.Primitive,
+        reason: ValueReason.Other,
+      });
+      break;
+    }
     case 'StoreContext': {
       /*
        * Context variables are like mutable boxes, so semantically
@@ -1921,6 +1951,14 @@ function computeEffectsForLegacySignature(
       arg.kind === 'Identifier' && i < signature.positionalParams.length
         ? signature.positionalParams[i]!
         : (signature.restParam ?? Effect.ConditionallyMutate);
+
+    if (arg.kind === 'Spread' && effect === Effect.Freeze) {
+      CompilerError.throwTodo({
+        reason: 'Support spread syntax for hook arguments',
+        loc: arg.place.loc,
+      });
+    }
+
     visit(place, effect);
   }
   if (captures.length !== 0) {

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

@@ -321,10 +321,6 @@ export function inferMutationAliasingRanges(
             }
             break;
           }
-          case 'ImmutableCapture': {
-            operandEffects.set(effect.from.identifier.id, Effect.Read);
-            break;
-          }
           case 'CreateFunction':
           case 'Create': {
             break;
@@ -352,6 +348,10 @@ export function inferMutationAliasingRanges(
             operandEffects.set(effect.value.identifier.id, Effect.Freeze);
             break;
           }
+          case 'ImmutableCapture': {
+            // no-op, Read is the default
+            break;
+          }
           case 'MutateFrozen':
           case 'MutateGlobal': {
             // no-op

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts

@@ -58,8 +58,7 @@ export function validateNoFreezingKnownMutableFunctions(
       const effect = contextMutationEffects.get(operand.identifier.id);
       if (effect != null) {
         errors.push({
-          reason: `This argument is a function which modifies local variables when called, which can bypass memoization and cause the UI not to update`,
-          description: `Functions that are returned from hooks, passed as arguments to hooks, or passed as props to components may not mutate local variables`,
+          reason: `This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead`,
           loc: operand.loc,
           severity: ErrorSeverity.InvalidReact,
         });
@@ -124,7 +123,15 @@ export function validateNoFreezingKnownMutableFunctions(
               switch (effect.kind) {
                 case 'Mutate':
                 case 'MutateTransitive': {
-                  if (context.has(effect.value.identifier.id)) {
+                  const knownMutation = contextMutationEffects.get(
+                    effect.value.identifier.id,
+                  );
+                  if (knownMutation != null) {
+                    contextMutationEffects.set(
+                      lvalue.identifier.id,
+                      knownMutation,
+                    );
+                  } else if (context.has(effect.value.identifier.id)) {
                     contextMutationEffects.set(lvalue.identifier.id, {
                       kind: 'ContextMutation',
                       effect: Effect.Mutate,
@@ -135,6 +142,19 @@ export function validateNoFreezingKnownMutableFunctions(
                   }
                   break;
                 }
+                case 'MutateConditionally':
+                case 'MutateTransitiveConditionally': {
+                  const knownMutation = contextMutationEffects.get(
+                    effect.value.identifier.id,
+                  );
+                  if (knownMutation != null) {
+                    contextMutationEffects.set(
+                      lvalue.identifier.id,
+                      knownMutation,
+                    );
+                  }
+                  break;
+                }
               }
             }
           }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.js

@@ -14,7 +14,7 @@ function Component(props) {
     a.property = true;
     b.push(false);
   };
-  return <div onClick={f()} />;
+  return <div onClick={f} />;
 }
 
 export const FIXTURE_ENTRYPOINT = {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.expect.md

@@ -0,0 +1,108 @@
+
+## Input
+
+```javascript
+function Component() {
+  let local;
+
+  const reassignLocal = newValue => {
+    local = newValue;
+  };
+
+  const onClick = newValue => {
+    reassignLocal('hello');
+
+    if (local === newValue) {
+      // Without React Compiler, `reassignLocal` is freshly created
+      // on each render, capturing a binding to the latest `local`,
+      // such that invoking reassignLocal will reassign the same
+      // binding that we are observing in the if condition, and
+      // we reach this branch
+      console.log('`local` was updated!');
+    } else {
+      // With React Compiler enabled, `reassignLocal` is only created
+      // once, capturing a binding to `local` in that render pass.
+      // Therefore, calling `reassignLocal` will reassign the wrong
+      // version of `local`, and not update the binding we are checking
+      // in the if condition.
+      //
+      // To protect against this, we disallow reassigning locals from
+      // functions that escape
+      throw new Error('`local` not updated!');
+    }
+  };
+
+  return <button onClick={onClick}>Submit</button>;
+}
+
+```
+
+
+## Error
+
+```
+  29 |   };
+  30 |
+> 31 |   return <button onClick={onClick}>Submit</button>;
+     |                           ^^^^^^^ InvalidReact: This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead (31:31)
+
+InvalidReact: The function modifies a local variable here (5:5)
+  32 | }
+  33 |
+
+ReactCompilerError: InvalidReact: This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead (31:31)
+
+InvalidReact: The function modifies a local variable here (5:5)
+    at validateNoFreezingKnownMutableFunctions (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:113829:18)
+    at runWithEnvironment (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114076:7)
+    at run (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:113959:10)
+    at compileFn (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114301:10)
+    at tryCompileFunction (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114793:19)
+    at processFn (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114730:25)
+    at compileProgram (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114638:22)
+    at PluginPass.enter (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:115773:26)
+    at newFn (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/visitors.js:172:14)
+    at NodePath._call (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/path/context.js:49:20)
+    at NodePath.call (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/path/context.js:39:18)
+    at NodePath.visit (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/path/context.js:88:31)
+    at TraversalContext.visitQueue (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/context.js:90:16)
+    at TraversalContext.visitSingle (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/context.js:66:19)
+    at TraversalContext.visit (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/context.js:113:19)
+    at traverseNode (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/traverse-node.js:22:17)
+    at traverse (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/index.js:53:34)
+    at transformFile (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transformation/index.js:80:31)
+    at transformFile.next (<anonymous>)
+    at run (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transformation/index.js:25:12)
+    at run.next (<anonymous>)
+    at /Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transform-ast.js:23:33
+    at Generator.next (<anonymous>)
+    at evaluateSync (/Users/joesavona/github/react/compiler/node_modules/gensync/index.js:251:28)
+    at sync (/Users/joesavona/github/react/compiler/node_modules/gensync/index.js:89:14)
+    at stopHiding - secret - don't use this - v1 (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/errors/rewrite-stack-trace.js:47:12)
+    at transformFromAstSync (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transform-ast.js:43:83)
+    at /Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:204:62
+    at Generator.next (<anonymous>)
+    at /Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:37:71
+    at new Promise (<anonymous>)
+    at __awaiter (/Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:33:12)
+    at transformFixtureInput (/Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:186:12)
+    at /Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:97:71
+    at Generator.next (<anonymous>)
+    at /Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:14:71
+    at new Promise (<anonymous>)
+    at __awaiter (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:10:12)
+    at compile (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:44:12)
+    at Object.<anonymous> (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:163:48)
+    at Generator.next (<anonymous>)
+    at /Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:14:71
+    at new Promise (<anonymous>)
+    at __awaiter (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:10:12)
+    at Object.transformFixture (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:148:12)
+    at execFunction (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:148:17)
+    at execHelper (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:127:5)
+    at execMethod (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:131:5)
+    at MessagePort.messageListener (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:49:7)
+    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:827:20)
+```
+
+

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.js

@@ -0,0 +1,32 @@
+function Component() {
+  let local;
+
+  const reassignLocal = newValue => {
+    local = newValue;
+  };
+
+  const onClick = newValue => {
+    reassignLocal('hello');
+
+    if (local === newValue) {
+      // Without React Compiler, `reassignLocal` is freshly created
+      // on each render, capturing a binding to the latest `local`,
+      // such that invoking reassignLocal will reassign the same
+      // binding that we are observing in the if condition, and
+      // we reach this branch
+      console.log('`local` was updated!');
+    } else {
+      // With React Compiler enabled, `reassignLocal` is only created
+      // once, capturing a binding to `local` in that render pass.
+      // Therefore, calling `reassignLocal` will reassign the wrong
+      // version of `local`, and not update the binding we are checking
+      // in the if condition.
+      //
+      // To protect against this, we disallow reassigning locals from
+      // functions that escape
+      throw new Error('`local` not updated!');
+    }
+  };
+
+  return <button onClick={onClick}>Submit</button>;
+}