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

Updates ValidateNoFreezingMutableFunctions to account for indirections, where the frozen function calls another function that actually does the mutation. We just need to propagate on the mutation. Also some fixes for hoisted functions, DeclareContext can actually appear twice, so we need to check that subsequent (re)declarations are treated as a mutation, not a creation.

ghstack-source-id: 8b05494224
Pull Request resolved: https://github.com/facebook/react/pull/33470

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>;
+}