[compiler] In change detection, assign reactive scopes to values that may have changed between renders

compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts

@@ -6,7 +6,7 @@ import {
   makeInstructionId,
 } from '.';
 import {getPlaceScope} from '../ReactiveScopes/BuildReactiveBlocks';
-import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
+import {isMutableAtInstruction} from '../ReactiveScopes/InferReactiveScopeVariables';
 import DisjointSet from '../Utils/DisjointSet';
 import {getOrInsertDefault} from '../Utils/utils';
 import {
@@ -254,7 +254,7 @@ function visitPlace(
    * of the stack to the mutated outer scope.
    */
   const placeScope = getPlaceScope(id, place);
-  if (placeScope != null && isMutable({id} as any, place)) {
+  if (placeScope != null && isMutableAtInstruction({id} as any, place)) {
     const placeScopeIdx = activeScopes.indexOf(placeScope);
     if (placeScopeIdx !== -1 && placeScopeIdx !== activeScopes.length - 1) {
       joined.union([placeScope, ...activeScopes.slice(placeScopeIdx + 1)]);

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

@@ -26,7 +26,7 @@ import {
 } from '../HIR/visitors';
 import {
   findDisjointMutableValues,
-  isMutable,
+  isMutableAtInstruction,
 } from '../ReactiveScopes/InferReactiveScopeVariables';
 import DisjointSet from '../Utils/DisjointSet';
 import {assertExhaustive} from '../Utils/utils';
@@ -232,7 +232,7 @@ export function inferReactivePlaces(fn: HIRFunction): void {
               case Effect.Store:
               case Effect.ConditionallyMutate:
               case Effect.Mutate: {
-                if (isMutable(instruction, operand)) {
+                if (isMutableAtInstruction(instruction, operand)) {
                   const resolvedId = identifierMapping.get(operand.identifier);
                   if (resolvedId !== undefined) {
                     reactiveIdentifiers.markReactiveIdentifier(resolvedId);

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts

@@ -687,10 +687,7 @@ function codegenReactiveScope(
   let computationBlock = codegenBlock(cx, block);
 
   let memoStatement;
-  if (
-    cx.env.config.enableChangeDetectionForDebugging != null &&
-    changeExpressions.length > 0
-  ) {
+  if (cx.env.config.enableChangeDetectionForDebugging != null) {
     const loc =
       typeof scope.loc === 'symbol'
         ? 'unknown location'

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts

@@ -185,7 +185,10 @@ function mergeLocation(l: SourceLocation, r: SourceLocation): SourceLocation {
 }
 
 // Is the operand mutable at this given instruction
-export function isMutable({id}: Instruction, place: Place): boolean {
+export function isMutableAtInstruction(
+  {id}: Instruction,
+  place: Place
+): boolean {
   const range = place.identifier.mutableRange;
   return id >= range.start && id < range.end;
 }
@@ -253,6 +256,89 @@ function mayAllocate(env: Environment, instruction: Instruction): boolean {
   }
 }
 
+/*
+ * These instructions may pick up external changes due to rules of react violations.
+ * Instructions should be included here if they may change without their inputs changing.
+ * For example, PostfixUpdate is not included because it only has a changed lval if
+ * it has a changed argument, but LoadProperty is included because the argument can be
+ * mutated elsewhere.
+ */
+function mayHaveChanged(env: Environment, instruction: Instruction): boolean {
+  if (env.config.enableChangeDetectionForDebugging == null) {
+    return false;
+  }
+  switch (instruction.value.kind) {
+    case "Await":
+    case "ComputedLoad":
+    case "Destructure":
+    case "GetIterator":
+    case "IteratorNext":
+    case "NextPropertyOf":
+    case "CallExpression":
+    case "MethodCall":
+    case "NewExpression": {
+      return true;
+    }
+    case "PropertyLoad": {
+      return instruction.value.property !== "current"
+    }
+    case "LoadGlobal": {
+      return (
+        instruction.value.binding.kind === "ModuleLocal" &&
+        !instruction.value.binding.immutable
+      );
+    }
+    case "PostfixUpdate":
+    case "PrefixUpdate":
+    case "DeclareLocal":
+    case "DeclareContext":
+    case "StoreLocal":
+    case "MetaProperty":
+    case "TypeCastExpression":
+    case "LoadLocal":
+    case "LoadContext":
+    case "StoreContext":
+    case "PropertyDelete":
+    case "ComputedDelete":
+    case "JSXText":
+    case "TemplateLiteral":
+    case "Primitive":
+    case "Debugger":
+    case "StartMemoize":
+    case "FinishMemoize":
+    case "UnaryExpression":
+    case "BinaryExpression":
+    case "StoreGlobal":
+    case "RegExpLiteral":
+    case "PropertyStore":
+    case "ComputedStore":
+    case "ArrayExpression":
+    case "JsxExpression":
+    case "JsxFragment":
+    case "ObjectExpression":
+    case "UnsupportedNode":
+    case "ObjectMethod":
+    case "FunctionExpression":
+    case "TaggedTemplateExpression": {
+      return false;
+    }
+    default: {
+      assertExhaustive(
+        instruction.value,
+        `Unexpected value kind \`${(instruction.value as any).kind}\``
+      );
+    }
+  }
+}
+
+function isIdentifierMutable(id: Identifier): boolean {
+  return id.mutableRange.end > id.mutableRange.start + 1;
+}
+
+function identifierHasMutableRange(id: Identifier): boolean {
+  return id.mutableRange.start > 0;
+}
+
 export function findDisjointMutableValues(
   fn: HIRFunction,
 ): DisjointSet<Identifier> {
@@ -265,7 +351,7 @@ export function findDisjointMutableValues(
     for (const phi of block.phis) {
       if (
         // The phi was reset because it was not mutated after creation
-        phi.id.mutableRange.start + 1 !== phi.id.mutableRange.end &&
+        isIdentifierMutable(phi.id) &&
         phi.id.mutableRange.end >
           (block.instructions.at(0)?.id ?? block.terminal.id)
       ) {
@@ -281,50 +367,52 @@ export function findDisjointMutableValues(
 
     for (const instr of block.instructions) {
       const operands: Array<Identifier> = [];
-      const range = instr.lvalue.identifier.mutableRange;
-      if (range.end > range.start + 1 || mayAllocate(fn.env, instr)) {
+      if (
+        isIdentifierMutable(instr.lvalue.identifier) ||
+        mayAllocate(fn.env, instr) ||
+        mayHaveChanged(fn.env, instr)
+      ) {
         operands.push(instr.lvalue!.identifier);
       }
       if (
         instr.value.kind === 'StoreLocal' ||
         instr.value.kind === 'StoreContext'
       ) {
-        if (
-          instr.value.lvalue.place.identifier.mutableRange.end >
-          instr.value.lvalue.place.identifier.mutableRange.start + 1
-        ) {
+        if (isIdentifierMutable(instr.value.lvalue.place.identifier)) {
           operands.push(instr.value.lvalue.place.identifier);
         }
         if (
-          isMutable(instr, instr.value.value) &&
-          instr.value.value.identifier.mutableRange.start > 0
+          isMutableAtInstruction(instr, instr.value.value) &&
+          identifierHasMutableRange(instr.value.value.identifier)
         ) {
           operands.push(instr.value.value.identifier);
         }
       } else if (instr.value.kind === 'Destructure') {
         for (const place of eachPatternOperand(instr.value.lvalue.pattern)) {
           if (
-            place.identifier.mutableRange.end >
-            place.identifier.mutableRange.start + 1
+            isIdentifierMutable(place.identifier) ||
+            mayHaveChanged(fn.env, instr)
           ) {
             operands.push(place.identifier);
           }
         }
         if (
-          isMutable(instr, instr.value.value) &&
-          instr.value.value.identifier.mutableRange.start > 0
+          (isMutableAtInstruction(instr, instr.value.value) &&
+            identifierHasMutableRange(instr.value.value.identifier)) ||
+          mayHaveChanged(fn.env, instr)
         ) {
           operands.push(instr.value.value.identifier);
         }
       } else if (instr.value.kind === 'MethodCall') {
         for (const operand of eachInstructionOperand(instr)) {
           if (
-            isMutable(instr, operand) &&
-            /*
-             * exclude global variables from being added to scopes, we can't recreate them!
-             * TODO: improve handling of module-scoped variables and globals
-             */
-            operand.identifier.mutableRange.start > 0
+            (isMutableAtInstruction(instr, operand) &&
+              /*
+               * exclude global variables from being added to scopes, we can't recreate them!
+               * TODO: improve handling of module-scoped variables and globals
+               */
+              identifierHasMutableRange(operand.identifier)) ||
+            mayHaveChanged(fn.env, instr)
           ) {
             operands.push(operand.identifier);
           }
@@ -337,12 +425,13 @@ export function findDisjointMutableValues(
       } else {
         for (const operand of eachInstructionOperand(instr)) {
           if (
-            isMutable(instr, operand) &&
-            /*
-             * exclude global variables from being added to scopes, we can't recreate them!
-             * TODO: improve handling of module-scoped variables and globals
-             */
-            operand.identifier.mutableRange.start > 0
+            (isMutableAtInstruction(instr, operand) &&
+              /*
+               * exclude global variables from being added to scopes, we can't recreate them!
+               * TODO: improve handling of module-scoped variables and globals
+               */
+              identifierHasMutableRange(operand.identifier)) ||
+            mayHaveChanged(fn.env, instr)
           ) {
             operands.push(operand.identifier);
           }

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts

@@ -811,7 +811,9 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor<State> {
     this.env = env;
     this.options = {
       memoizeJsxElements: !this.env.config.enableForest,
-      forceMemoizePrimitives: this.env.config.enableForest,
+      forceMemoizePrimitives:
+        this.env.config.enableForest ||
+        this.env.config.enableChangeDetectionForDebugging != null,
     };
   }
 

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

@@ -17,7 +17,7 @@ import {
   isUseInsertionEffectHookType,
   isUseLayoutEffectHookType,
 } from '../HIR';
-import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
+import {isMutableAtInstruction} from '../ReactiveScopes/InferReactiveScopeVariables';
 import {
   ReactiveFunctionVisitor,
   visitReactiveFunction,
@@ -103,7 +103,7 @@ class Visitor extends ReactiveFunctionVisitor<CompilerError> {
          * TODO: isMutable is not safe to call here as it relies on identifier mutableRange which is no longer valid at this point
          * in the pipeline
          */
-        (isMutable(instruction as Instruction, deps) ||
+        (isMutableAtInstruction(instruction as Instruction, deps) ||
           isUnmemoized(deps.identifier, this.scopes))
       ) {
         state.push({