From 629feee55a2efd240450d7dcb79014256716c7c2 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 17 Nov 2025 11:15:31 -0800 Subject: [PATCH] [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. --- .../src/HIR/visitors.ts | 27 +++ .../ReactiveScopes/CodegenReactiveFunction.ts | 22 --- ...tractScopeDeclarationsFromDestructuring.ts | 31 +++- ...-reassignment-undefined-variable.expect.md | 162 ++++++++++++++++++ ...cturing-reassignment-undefined-variable.js | 53 ++++++ 5 files changed, 268 insertions(+), 27 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts index af1cffe85fd3..5735358cecf2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts +++ b/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 { diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index c497253a22fb..f81c962edf86 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -1359,8 +1359,6 @@ 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 && @@ -1368,26 +1366,6 @@ function codegenInstructionNullable( ) { 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); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts index 642b89747e6e..f24861152aa0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts +++ b/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,6 +117,9 @@ class Visitor extends ReactiveFunctionTransform { ): Transformed { this.visitInstruction(instruction, state); + let instructionsToProcess: Array = [instruction]; + let result: Transformed = {kind: 'keep'}; + if (instruction.value.kind === 'Destructure') { const transformed = transformDestructuring( state, @@ -120,7 +127,8 @@ class Visitor extends ReactiveFunctionTransform { 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 { }; } } - 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; } /* diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.expect.md new file mode 100644 index 000000000000..544366b1de6e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.expect.md @@ -0,0 +1,162 @@ + +## Input + +```javascript +// @flow @compilationMode:"infer" +'use strict'; + +function getWeekendDays(user) { + return [0, 6]; +} + +function getConfig(weekendDays) { + return [1, 5]; +} + +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]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Calendar, + params: [ + { + user: {}, + defaultFirstDay: 1, + currentDate: {getDayOfWeek: () => 3}, + view: 'week', + }, + ], + sequentialRenders: [ + { + user: {}, + defaultFirstDay: 1, + currentDate: {getDayOfWeek: () => 3}, + view: 'week', + }, + { + user: {}, + defaultFirstDay: 1, + currentDate: {getDayOfWeek: () => 3}, + view: 'day', + }, + ], +}; + +``` + +## Code + +```javascript +"use strict"; +import { c as _c } from "react/compiler-runtime"; + +function getWeekendDays(user) { + return [0, 6]; +} + +function getConfig(weekendDays) { + return [1, 5]; +} + +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; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Calendar, + params: [ + { + user: {}, + defaultFirstDay: 1, + currentDate: { getDayOfWeek: () => 3 }, + view: "week", + }, + ], + + sequentialRenders: [ + { + user: {}, + defaultFirstDay: 1, + currentDate: { getDayOfWeek: () => 3 }, + view: "week", + }, + { + user: {}, + defaultFirstDay: 1, + currentDate: { getDayOfWeek: () => 3 }, + view: "day", + }, + ], +}; + +``` + +### Eval output +(kind: ok) [{"getDayOfWeek":"[[ function params=0 ]]"},1,5] +[{"getDayOfWeek":"[[ function params=0 ]]"},3,1] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.js new file mode 100644 index 000000000000..461d6bf16dfb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-invalid-destructuring-reassignment-undefined-variable.js @@ -0,0 +1,53 @@ +// @flow @compilationMode:"infer" +'use strict'; + +function getWeekendDays(user) { + return [0, 6]; +} + +function getConfig(weekendDays) { + return [1, 5]; +} + +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]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Calendar, + params: [ + { + user: {}, + defaultFirstDay: 1, + currentDate: {getDayOfWeek: () => 3}, + view: 'week', + }, + ], + sequentialRenders: [ + { + user: {}, + defaultFirstDay: 1, + currentDate: {getDayOfWeek: () => 3}, + view: 'week', + }, + { + user: {}, + defaultFirstDay: 1, + currentDate: {getDayOfWeek: () => 3}, + view: 'day', + }, + ], +};