[compiler] Fix for destructuring with mixed declaration/reassignment

Destructing statements that start off as declarations can end up becoming reassignments if the variable is a scope declaration, so we have existing logic to handle cases where some parts of a destructure need to be converted into new locals, with a reassignment to the hoisted scope variable afterwards. However, there is an edge case where all of the values are reassigned, in which case we don't need to rewrite and can just set the instruction kind to reassign.

Author: josephsavona

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

@@ -11,6 +11,7 @@ import {
   BasicBlock,
   BlockId,
   Instruction,
+  InstructionKind,
   InstructionValue,
   makeInstructionId,
   Pattern,
@@ -32,6 +33,32 @@ export function* eachInstructionLValue(
   yield* eachInstructionValueLValue(instr.value);
 }
 
+export function* eachInstructionLValueWithKind(
+  instr: ReactiveInstruction,
+): Iterable<[Place, InstructionKind]> {
+  switch (instr.value.kind) {
+    case 'DeclareContext':
+    case 'StoreContext':
+    case 'DeclareLocal':
+    case 'StoreLocal': {
+      yield [instr.value.lvalue.place, instr.value.lvalue.kind];
+      break;
+    }
+    case 'Destructure': {
+      const kind = instr.value.lvalue.kind;
+      for (const place of eachPatternOperand(instr.value.lvalue.pattern)) {
+        yield [place, kind];
+      }
+      break;
+    }
+    case 'PostfixUpdate':
+    case 'PrefixUpdate': {
+      yield [instr.value.lvalue, InstructionKind.Reassign];
+      break;
+    }
+  }
+}
+
 export function* eachInstructionValueLValue(
   value: ReactiveValue,
 ): Iterable<Place> {

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

@@ -1359,35 +1359,13 @@ function codegenInstructionNullable(
       value = null;
     } else {
       lvalue = instr.value.lvalue.pattern;
-      let hasReassign = false;
-      let hasDeclaration = false;
       for (const place of eachPatternOperand(lvalue)) {
         if (
           kind !== InstructionKind.Reassign &&
           place.identifier.name === null
         ) {
           cx.temp.set(place.identifier.declarationId, null);
         }
-        const isDeclared = cx.hasDeclared(place.identifier);
-        hasReassign ||= isDeclared;
-        hasDeclaration ||= !isDeclared;
-      }
-      if (hasReassign && hasDeclaration) {
-        CompilerError.invariant(false, {
-          reason:
-            'Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations)',
-          description: null,
-          details: [
-            {
-              kind: 'error',
-              loc: instr.loc,
-              message: null,
-            },
-          ],
-          suggestions: null,
-        });
-      } else if (hasReassign) {
-        kind = InstructionKind.Reassign;
       }
       value = codegenPlaceToExpression(cx, instr.value.value);
     }

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

@@ -19,7 +19,11 @@ import {
   promoteTemporary,
 } from '../HIR';
 import {clonePlaceToTemporary} from '../HIR/HIRBuilder';
-import {eachPatternOperand, mapPatternOperands} from '../HIR/visitors';
+import {
+  eachInstructionLValueWithKind,
+  eachPatternOperand,
+  mapPatternOperands,
+} from '../HIR/visitors';
 import {
   ReactiveFunctionTransform,
   Transformed,
@@ -113,14 +117,18 @@ class Visitor extends ReactiveFunctionTransform<State> {
   ): Transformed<ReactiveStatement> {
     this.visitInstruction(instruction, state);
 
+    let instructionsToProcess: Array<ReactiveInstruction> = [instruction];
+    let result: Transformed<ReactiveStatement> = {kind: 'keep'};
+
     if (instruction.value.kind === 'Destructure') {
       const transformed = transformDestructuring(
         state,
         instruction,
         instruction.value,
       );
       if (transformed) {
-        return {
+        instructionsToProcess = transformed;
+        result = {
           kind: 'replace-many',
           value: transformed.map(instruction => ({
             kind: 'instruction',
@@ -129,7 +137,17 @@ class Visitor extends ReactiveFunctionTransform<State> {
         };
       }
     }
-    return {kind: 'keep'};
+
+    // Update state.declared with declarations from the instruction(s)
+    for (const instr of instructionsToProcess) {
+      for (const [place, kind] of eachInstructionLValueWithKind(instr)) {
+        if (kind !== InstructionKind.Reassign) {
+          state.declared.add(place.identifier.declarationId);
+        }
+      }
+    }
+
+    return result;
   }
 }
 
@@ -144,10 +162,13 @@ function transformDestructuring(
     const isDeclared = state.declared.has(place.identifier.declarationId);
     if (isDeclared) {
       reassigned.add(place.identifier.id);
+    } else {
+      hasDeclaration = true;
     }
-    hasDeclaration ||= !isDeclared;
   }
-  if (reassigned.size === 0 || !hasDeclaration) {
+  if (!hasDeclaration) {
+    // all reassignments
+    destructure.lvalue.kind = InstructionKind.Reassign;
     return null;
   }
   /*

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.expect.md

@@ -0,0 +1,92 @@
+
+## Input
+
+```javascript
+// @flow
+'use strict';
+
+component Calendar(user, defaultFirstDay, currentDate, view) {
+  const weekendDays = getWeekendDays(user);
+  let firstDay = defaultFirstDay;
+  let daysToDisplay = 7;
+  if (view === 'week') {
+    let lastDay;
+    // this assignment produces invalid code
+    [firstDay, lastDay] = getConfig(weekendDays);
+    daysToDisplay = ((7 + lastDay - firstDay) % 7) + 1;
+  } else if (view === 'day') {
+    firstDay = currentDate.getDayOfWeek();
+    daysToDisplay = 1;
+  }
+
+  return [currentDate, firstDay, daysToDisplay];
+}
+
+```
+
+## Code
+
+```javascript
+"use strict";
+import { c as _c } from "react/compiler-runtime";
+
+function Calendar(t0) {
+  const $ = _c(12);
+  const { user, defaultFirstDay, currentDate, view } = t0;
+  let daysToDisplay;
+  let firstDay;
+  if (
+    $[0] !== currentDate ||
+    $[1] !== defaultFirstDay ||
+    $[2] !== user ||
+    $[3] !== view
+  ) {
+    const weekendDays = getWeekendDays(user);
+    firstDay = defaultFirstDay;
+    daysToDisplay = 7;
+    if (view === "week") {
+      let lastDay;
+
+      [firstDay, lastDay] = getConfig(weekendDays);
+      daysToDisplay = ((7 + lastDay - firstDay) % 7) + 1;
+    } else {
+      if (view === "day") {
+        let t1;
+        if ($[6] !== currentDate) {
+          t1 = currentDate.getDayOfWeek();
+          $[6] = currentDate;
+          $[7] = t1;
+        } else {
+          t1 = $[7];
+        }
+        firstDay = t1;
+        daysToDisplay = 1;
+      }
+    }
+    $[0] = currentDate;
+    $[1] = defaultFirstDay;
+    $[2] = user;
+    $[3] = view;
+    $[4] = daysToDisplay;
+    $[5] = firstDay;
+  } else {
+    daysToDisplay = $[4];
+    firstDay = $[5];
+  }
+  let t1;
+  if ($[8] !== currentDate || $[9] !== daysToDisplay || $[10] !== firstDay) {
+    t1 = [currentDate, firstDay, daysToDisplay];
+    $[8] = currentDate;
+    $[9] = daysToDisplay;
+    $[10] = firstDay;
+    $[11] = t1;
+  } else {
+    t1 = $[11];
+  }
+  return t1;
+}
+
+```
+      
+### Eval output
+(kind: exception) Fixture not implemented

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.js

@@ -0,0 +1,19 @@
+// @flow
+'use strict';
+
+component Calendar(user, defaultFirstDay, currentDate, view) {
+  const weekendDays = getWeekendDays(user);
+  let firstDay = defaultFirstDay;
+  let daysToDisplay = 7;
+  if (view === 'week') {
+    let lastDay;
+    // this assignment produces invalid code
+    [firstDay, lastDay] = getConfig(weekendDays);
+    daysToDisplay = ((7 + lastDay - firstDay) % 7) + 1;
+  } else if (view === 'day') {
+    firstDay = currentDate.getDayOfWeek();
+    daysToDisplay = 1;
+  }
+
+  return [currentDate, firstDay, daysToDisplay];
+}