From b315a0f7133a3251f72970d56c2ad454bdd47003 Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Mon, 17 Nov 2025 11:34:49 -0800 Subject: [PATCH 1/4] [compiler] Fix for destructuring with mixed declaration/reassignment (#35144) 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. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35144). * #35148 * #35147 * #35146 * __->__ #35144 --- .../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 af1cffe85fd..5735358cecf 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 c497253a22f..f81c962edf8 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 642b89747e6..f24861152aa 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 00000000000..544366b1de6 --- /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 00000000000..461d6bf16df --- /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', + }, + ], +}; From d6b1a0573b4c43e5222aee1de5b11a8e4b575c8e Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Mon, 17 Nov 2025 12:05:52 -0800 Subject: [PATCH 2/4] [compiler] Extract reusable logic for control dominators (#35146) The next PR needs to check if a block is controlled by a value derived from a ref. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35146). * #35148 * #35147 * __->__ #35146 --- .../src/Inference/ControlDominators.ts | 114 ++++++++++++++++++ .../src/Inference/InferReactivePlaces.ts | 101 +--------------- 2 files changed, 118 insertions(+), 97 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Inference/ControlDominators.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/ControlDominators.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/ControlDominators.ts new file mode 100644 index 00000000000..1fab651947a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/ControlDominators.ts @@ -0,0 +1,114 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {BlockId, computePostDominatorTree, HIRFunction, Place} from '../HIR'; +import {PostDominator} from '../HIR/Dominator'; + +export type ControlDominators = (id: BlockId) => boolean; + +/** + * Returns an object that lazily calculates whether particular blocks are controlled + * by values of interest. Which values matter are up to the caller. + */ +export function createControlDominators( + fn: HIRFunction, + isControlVariable: (place: Place) => boolean, +): ControlDominators { + const postDominators = computePostDominatorTree(fn, { + includeThrowsAsExitNode: false, + }); + const postDominatorFrontierCache = new Map>(); + + function isControlledBlock(id: BlockId): boolean { + let controlBlocks = postDominatorFrontierCache.get(id); + if (controlBlocks === undefined) { + controlBlocks = postDominatorFrontier(fn, postDominators, id); + postDominatorFrontierCache.set(id, controlBlocks); + } + for (const blockId of controlBlocks) { + const controlBlock = fn.body.blocks.get(blockId)!; + switch (controlBlock.terminal.kind) { + case 'if': + case 'branch': { + if (isControlVariable(controlBlock.terminal.test)) { + return true; + } + break; + } + case 'switch': { + if (isControlVariable(controlBlock.terminal.test)) { + return true; + } + for (const case_ of controlBlock.terminal.cases) { + if (case_.test !== null && isControlVariable(case_.test)) { + return true; + } + } + break; + } + } + } + return false; + } + + return isControlledBlock; +} + +/* + * Computes the post-dominator frontier of @param block. These are immediate successors of nodes that + * post-dominate @param targetId and from which execution may not reach @param block. Intuitively, these + * are the earliest blocks from which execution branches such that it may or may not reach the target block. + */ +function postDominatorFrontier( + fn: HIRFunction, + postDominators: PostDominator, + targetId: BlockId, +): Set { + const visited = new Set(); + const frontier = new Set(); + const targetPostDominators = postDominatorsOf(fn, postDominators, targetId); + for (const blockId of [...targetPostDominators, targetId]) { + if (visited.has(blockId)) { + continue; + } + visited.add(blockId); + const block = fn.body.blocks.get(blockId)!; + for (const pred of block.preds) { + if (!targetPostDominators.has(pred)) { + // The predecessor does not always reach this block, we found an item on the frontier! + frontier.add(pred); + } + } + } + return frontier; +} + +function postDominatorsOf( + fn: HIRFunction, + postDominators: PostDominator, + targetId: BlockId, +): Set { + const result = new Set(); + const visited = new Set(); + const queue = [targetId]; + while (queue.length) { + const currentId = queue.shift()!; + if (visited.has(currentId)) { + continue; + } + visited.add(currentId); + const current = fn.body.blocks.get(currentId)!; + for (const pred of current.preds) { + const predPostDominator = postDominators.get(pred) ?? pred; + if (predPostDominator === targetId || result.has(predPostDominator)) { + result.add(pred); + } + queue.push(pred); + } + } + return result; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts index 271a76e92c1..d7ebcfe2fbc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts @@ -7,7 +7,6 @@ import {CompilerError} from '..'; import { - BlockId, Effect, Environment, HIRFunction, @@ -15,14 +14,12 @@ import { IdentifierId, Instruction, Place, - computePostDominatorTree, evaluatesToStableTypeOrContainer, getHookKind, isStableType, isStableTypeContainer, isUseOperator, } from '../HIR'; -import {PostDominator} from '../HIR/Dominator'; import { eachInstructionLValue, eachInstructionOperand, @@ -35,6 +32,7 @@ import { } from '../ReactiveScopes/InferReactiveScopeVariables'; import DisjointSet from '../Utils/DisjointSet'; import {assertExhaustive} from '../Utils/utils'; +import {createControlDominators} from './ControlDominators'; /** * Side map to track and propagate sources of stability (i.e. hook calls such as @@ -212,45 +210,9 @@ export function inferReactivePlaces(fn: HIRFunction): void { reactiveIdentifiers.markReactive(place); } - const postDominators = computePostDominatorTree(fn, { - includeThrowsAsExitNode: false, - }); - const postDominatorFrontierCache = new Map>(); - - function isReactiveControlledBlock(id: BlockId): boolean { - let controlBlocks = postDominatorFrontierCache.get(id); - if (controlBlocks === undefined) { - controlBlocks = postDominatorFrontier(fn, postDominators, id); - postDominatorFrontierCache.set(id, controlBlocks); - } - for (const blockId of controlBlocks) { - const controlBlock = fn.body.blocks.get(blockId)!; - switch (controlBlock.terminal.kind) { - case 'if': - case 'branch': { - if (reactiveIdentifiers.isReactive(controlBlock.terminal.test)) { - return true; - } - break; - } - case 'switch': { - if (reactiveIdentifiers.isReactive(controlBlock.terminal.test)) { - return true; - } - for (const case_ of controlBlock.terminal.cases) { - if ( - case_.test !== null && - reactiveIdentifiers.isReactive(case_.test) - ) { - return true; - } - } - break; - } - } - } - return false; - } + const isReactiveControlledBlock = createControlDominators(fn, place => + reactiveIdentifiers.isReactive(place), + ); do { for (const [, block] of fn.body.blocks) { @@ -411,61 +373,6 @@ export function inferReactivePlaces(fn: HIRFunction): void { propagateReactivityToInnerFunctions(fn, true); } -/* - * Computes the post-dominator frontier of @param block. These are immediate successors of nodes that - * post-dominate @param targetId and from which execution may not reach @param block. Intuitively, these - * are the earliest blocks from which execution branches such that it may or may not reach the target block. - */ -function postDominatorFrontier( - fn: HIRFunction, - postDominators: PostDominator, - targetId: BlockId, -): Set { - const visited = new Set(); - const frontier = new Set(); - const targetPostDominators = postDominatorsOf(fn, postDominators, targetId); - for (const blockId of [...targetPostDominators, targetId]) { - if (visited.has(blockId)) { - continue; - } - visited.add(blockId); - const block = fn.body.blocks.get(blockId)!; - for (const pred of block.preds) { - if (!targetPostDominators.has(pred)) { - // The predecessor does not always reach this block, we found an item on the frontier! - frontier.add(pred); - } - } - } - return frontier; -} - -function postDominatorsOf( - fn: HIRFunction, - postDominators: PostDominator, - targetId: BlockId, -): Set { - const result = new Set(); - const visited = new Set(); - const queue = [targetId]; - while (queue.length) { - const currentId = queue.shift()!; - if (visited.has(currentId)) { - continue; - } - visited.add(currentId); - const current = fn.body.blocks.get(currentId)!; - for (const pred of current.preds) { - const predPostDominator = postDominators.get(pred) ?? pred; - if (predPostDominator === targetId || result.has(predPostDominator)) { - result.add(pred); - } - queue.push(pred); - } - } - return result; -} - class ReactivityMap { hasChanges: boolean = false; reactive: Set = new Set(); From b946a249b560a2d3afe1a1c8553d5491b1767cb3 Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Mon, 17 Nov 2025 12:07:43 -0800 Subject: [PATCH 3/4] [compiler] Improve setState-in-effects rule to account for ref-gated conditionals (#35147) Conditionally calling setState in an effect is sometimes necessary, but should generally follow the pattern of using a "previous vaue" ref to manually compare and ensure that the setState is idempotent. See fixture for an example. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35147). * #35148 * __->__ #35147 --- .../src/HIR/Environment.ts | 11 +- .../Validation/ValidateNoSetStateInEffects.ts | 83 ++++++++++++- ...seEffect-controlled-by-ref-value.expect.md | 117 ++++++++++++++++++ ...te-in-useEffect-controlled-by-ref-value.js | 40 ++++++ 4 files changed, 245 insertions(+), 6 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index a32823cb5f8..b29d65e6aa5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -672,9 +672,14 @@ export const EnvironmentConfigSchema = z.object({ validateNoDynamicallyCreatedComponentsOrHooks: z.boolean().default(false), /** - * When enabled, allows setState calls in effects when the value being set is - * derived from a ref. This is useful for patterns where initial layout measurements - * from refs need to be stored in state during mount. + * When enabled, allows setState calls in effects based on valid patterns involving refs: + * - Allow setState where the value being set is derived from a ref. This is useful where + * state needs to take into account layer information, and a layout effect reads layout + * data from a ref and sets state. + * - Allow conditionally calling setState after manually comparing previous/new values + * for changes via a ref. Relying on effect deps is insufficient for non-primitive values, + * so a ref is generally required to manually track previous values and compare prev/next + * for meaningful changes before setting state. */ enableAllowSetStateFromRefsInEffects: z.boolean().default(true), diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts index 656c9a67148..795969b3f3b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts @@ -21,13 +21,17 @@ import { isUseRefType, isRefValueType, Place, + Effect, + BlockId, } from '../HIR'; import { eachInstructionLValue, eachInstructionValueOperand, } from '../HIR/visitors'; +import {createControlDominators} from '../Inference/ControlDominators'; +import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; import {Result} from '../Utils/Result'; -import {Iterable_some} from '../Utils/utils'; +import {assertExhaustive, Iterable_some} from '../Utils/utils'; /** * Validates against calling setState in the body of an effect (useEffect and friends), @@ -140,6 +144,8 @@ function getSetStateCall( setStateFunctions: Map, env: Environment, ): Place | null { + const enableAllowSetStateFromRefsInEffects = + env.config.enableAllowSetStateFromRefsInEffects; const refDerivedValues: Set = new Set(); const isDerivedFromRef = (place: Place): boolean => { @@ -150,9 +156,38 @@ function getSetStateCall( ); }; + const isRefControlledBlock: (id: BlockId) => boolean = + enableAllowSetStateFromRefsInEffects + ? createControlDominators(fn, place => isDerivedFromRef(place)) + : (): boolean => false; + for (const [, block] of fn.body.blocks) { + if (enableAllowSetStateFromRefsInEffects) { + for (const phi of block.phis) { + if (isDerivedFromRef(phi.place)) { + continue; + } + let isPhiDerivedFromRef = false; + for (const [, operand] of phi.operands) { + if (isDerivedFromRef(operand)) { + isPhiDerivedFromRef = true; + break; + } + } + if (isPhiDerivedFromRef) { + refDerivedValues.add(phi.place.identifier.id); + } else { + for (const [pred] of phi.operands) { + if (isRefControlledBlock(pred)) { + refDerivedValues.add(phi.place.identifier.id); + break; + } + } + } + } + } for (const instr of block.instructions) { - if (env.config.enableAllowSetStateFromRefsInEffects) { + if (enableAllowSetStateFromRefsInEffects) { const hasRefOperand = Iterable_some( eachInstructionValueOperand(instr.value), isDerivedFromRef, @@ -162,6 +197,46 @@ function getSetStateCall( for (const lvalue of eachInstructionLValue(instr)) { refDerivedValues.add(lvalue.identifier.id); } + // Ref-derived values can also propagate through mutation + for (const operand of eachInstructionValueOperand(instr.value)) { + switch (operand.effect) { + case Effect.Capture: + case Effect.Store: + case Effect.ConditionallyMutate: + case Effect.ConditionallyMutateIterator: + case Effect.Mutate: { + if (isMutable(instr, operand)) { + refDerivedValues.add(operand.identifier.id); + } + break; + } + case Effect.Freeze: + case Effect.Read: { + // no-op + break; + } + case Effect.Unknown: { + CompilerError.invariant(false, { + reason: 'Unexpected unknown effect', + description: null, + details: [ + { + kind: 'error', + loc: operand.loc, + message: null, + }, + ], + suggestions: null, + }); + } + default: { + assertExhaustive( + operand.effect, + `Unexpected effect kind \`${operand.effect}\``, + ); + } + } + } } if ( @@ -203,7 +278,7 @@ function getSetStateCall( isSetStateType(callee.identifier) || setStateFunctions.has(callee.identifier.id) ) { - if (env.config.enableAllowSetStateFromRefsInEffects) { + if (enableAllowSetStateFromRefsInEffects) { const arg = instr.value.args.at(0); if ( arg !== undefined && @@ -216,6 +291,8 @@ function getSetStateCall( * be needed when initial layout measurements from refs need to be stored in state. */ return null; + } else if (isRefControlledBlock(block.id)) { + continue; } } /* diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.expect.md new file mode 100644 index 00000000000..43a84784ea0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.expect.md @@ -0,0 +1,117 @@ + +## Input + +```javascript +// @validateNoSetStateInEffects @enableAllowSetStateFromRefsInEffects @loggerTestOnly @compilationMode:"infer" +import {useState, useRef, useEffect} from 'react'; + +function Component({x, y}) { + const previousXRef = useRef(null); + const previousYRef = useRef(null); + + const [data, setData] = useState(null); + + useEffect(() => { + const previousX = previousXRef.current; + previousXRef.current = x; + const previousY = previousYRef.current; + previousYRef.current = y; + if (!areEqual(x, previousX) || !areEqual(y, previousY)) { + const data = load({x, y}); + setData(data); + } + }, [x, y]); + + return data; +} + +function areEqual(a, b) { + return a === b; +} + +function load({x, y}) { + return x * y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 0, y: 0}], + sequentialRenders: [ + {x: 0, y: 0}, + {x: 1, y: 0}, + {x: 1, y: 1}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInEffects @enableAllowSetStateFromRefsInEffects @loggerTestOnly @compilationMode:"infer" +import { useState, useRef, useEffect } from "react"; + +function Component(t0) { + const $ = _c(4); + const { x, y } = t0; + const previousXRef = useRef(null); + const previousYRef = useRef(null); + + const [data, setData] = useState(null); + let t1; + let t2; + if ($[0] !== x || $[1] !== y) { + t1 = () => { + const previousX = previousXRef.current; + previousXRef.current = x; + const previousY = previousYRef.current; + previousYRef.current = y; + if (!areEqual(x, previousX) || !areEqual(y, previousY)) { + const data_0 = load({ x, y }); + setData(data_0); + } + }; + + t2 = [x, y]; + $[0] = x; + $[1] = y; + $[2] = t1; + $[3] = t2; + } else { + t1 = $[2]; + t2 = $[3]; + } + useEffect(t1, t2); + return data; +} + +function areEqual(a, b) { + return a === b; +} + +function load({ x, y }) { + return x * y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ x: 0, y: 0 }], + sequentialRenders: [ + { x: 0, y: 0 }, + { x: 1, y: 0 }, + { x: 1, y: 1 }, + ], +}; + +``` + +## Logs + +``` +{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":163},"end":{"line":22,"column":1,"index":631},"filename":"valid-setState-in-useEffect-controlled-by-ref-value.ts"},"fnName":"Component","memoSlots":4,"memoBlocks":1,"memoValues":2,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### Eval output +(kind: ok) 0 +0 +1 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.js new file mode 100644 index 00000000000..46f11057d11 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-controlled-by-ref-value.js @@ -0,0 +1,40 @@ +// @validateNoSetStateInEffects @enableAllowSetStateFromRefsInEffects @loggerTestOnly @compilationMode:"infer" +import {useState, useRef, useEffect} from 'react'; + +function Component({x, y}) { + const previousXRef = useRef(null); + const previousYRef = useRef(null); + + const [data, setData] = useState(null); + + useEffect(() => { + const previousX = previousXRef.current; + previousXRef.current = x; + const previousY = previousYRef.current; + previousYRef.current = y; + if (!areEqual(x, previousX) || !areEqual(y, previousY)) { + const data = load({x, y}); + setData(data); + } + }, [x, y]); + + return data; +} + +function areEqual(a, b) { + return a === b; +} + +function load({x, y}) { + return x * y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 0, y: 0}], + sequentialRenders: [ + {x: 0, y: 0}, + {x: 1, y: 0}, + {x: 1, y: 1}, + ], +}; From ea4899e13f9e29815321e3cac70fa08bb8ed790a Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Mon, 17 Nov 2025 12:09:09 -0800 Subject: [PATCH 4/4] [compiler][snap] Support pattern of files to test as CLI argument (#35148) I've been trying out LLM agents for compiler development, and one thing i found is that the agent naturally wants to run `yarn snap ` to test a specific fixture, and I want to be able to tell it (directly or in rules/skills) to do this in order to get the debug output from all the compiler passes. Agents can figure out our current testfilter.txt file system but that's just tedious. So here we add support for `yarn snap -p `. If you pass in a pattern with an extension, we target that extension specifically. If you pass in a .expect.md file, we look at that specific fixture. And if the pattern doesn't have extensions, we search for `{.js,.jsx,.ts,.tsx}`. When patterns are enabled we automatically log as in debug mode (if there is a single match), and disable watch mode. Open to feedback! --- compiler/packages/snap/src/fixture-utils.ts | 47 +++++++++++++++++---- compiler/packages/snap/src/runner.ts | 30 +++++++++++-- 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/compiler/packages/snap/src/fixture-utils.ts b/compiler/packages/snap/src/fixture-utils.ts index 931b4f29352..ebf6a34e1db 100644 --- a/compiler/packages/snap/src/fixture-utils.ts +++ b/compiler/packages/snap/src/fixture-utils.ts @@ -44,6 +44,21 @@ function stripExtension(filename: string, extensions: Array): string { return filename; } +/** + * Strip all extensions from a filename + * e.g., "foo.expect.md" -> "foo" + */ +function stripAllExtensions(filename: string): string { + let result = filename; + while (true) { + const extension = path.extname(result); + if (extension === '') { + return result; + } + result = path.basename(result, extension); + } +} + export async function readTestFilter(): Promise { if (!(await exists(FILTER_PATH))) { throw new Error(`testfilter file not found at \`${FILTER_PATH}\``); @@ -111,11 +126,25 @@ async function readInputFixtures( } else { inputFiles = ( await Promise.all( - filter.paths.map(pattern => - glob.glob(`${pattern}{${INPUT_EXTENSIONS.join(',')}}`, { + filter.paths.map(pattern => { + // If the pattern already has an extension other than .expect.md, + // search for the pattern directly. Otherwise, search for the + // pattern with the expected input extensions added. + // Eg + // `alias-while` => search for `alias-while{.js,.jsx,.ts,.tsx}` + // `alias-while.js` => search as-is + // `alias-while.expect.md` => search for `alias-while{.js,.jsx,.ts,.tsx}` + const basename = path.basename(pattern); + const basenameWithoutExt = stripAllExtensions(basename); + const hasExtension = basename !== basenameWithoutExt; + const globPattern = + hasExtension && !pattern.endsWith(SNAPSHOT_EXTENSION) + ? pattern + : `${basenameWithoutExt}{${INPUT_EXTENSIONS.join(',')}}`; + return glob.glob(globPattern, { cwd: rootDir, - }), - ), + }); + }), ) ).flat(); } @@ -150,11 +179,13 @@ async function readOutputFixtures( } else { outputFiles = ( await Promise.all( - filter.paths.map(pattern => - glob.glob(`${pattern}${SNAPSHOT_EXTENSION}`, { + filter.paths.map(pattern => { + // Strip all extensions and find matching .expect.md files + const basenameWithoutExt = stripAllExtensions(pattern); + return glob.glob(`${basenameWithoutExt}${SNAPSHOT_EXTENSION}`, { cwd: rootDir, - }), - ), + }); + }), ) ).flat(); } diff --git a/compiler/packages/snap/src/runner.ts b/compiler/packages/snap/src/runner.ts index d46a18712e8..92a0a0f82ee 100644 --- a/compiler/packages/snap/src/runner.ts +++ b/compiler/packages/snap/src/runner.ts @@ -35,6 +35,7 @@ type RunnerOptions = { watch: boolean; filter: boolean; update: boolean; + pattern?: string; }; const opts: RunnerOptions = yargs @@ -62,9 +63,15 @@ const opts: RunnerOptions = yargs 'Only run fixtures which match the contents of testfilter.txt', ) .default('filter', false) + .string('pattern') + .alias('p', 'pattern') + .describe( + 'pattern', + 'Optional glob pattern to filter fixtures (e.g., "error.*", "use-memo")', + ) .help('help') .strict() - .parseSync(hideBin(process.argv)); + .parseSync(hideBin(process.argv)) as RunnerOptions; /** * Do a test run and return the test results @@ -171,7 +178,13 @@ export async function main(opts: RunnerOptions): Promise { worker.getStderr().pipe(process.stderr); worker.getStdout().pipe(process.stdout); - if (opts.watch) { + // If pattern is provided, force watch mode off and use pattern filter + const shouldWatch = opts.watch && opts.pattern == null; + if (opts.watch && opts.pattern != null) { + console.warn('NOTE: --watch is ignored when a --pattern is supplied'); + } + + if (shouldWatch) { makeWatchRunner(state => onChange(worker, state), opts.filter); if (opts.filter) { /** @@ -216,7 +229,18 @@ export async function main(opts: RunnerOptions): Promise { try { execSync('yarn build', {cwd: PROJECT_ROOT}); console.log('Built compiler successfully with tsup'); - const testFilter = opts.filter ? await readTestFilter() : null; + + // Determine which filter to use + let testFilter: TestFilter | null = null; + if (opts.pattern) { + testFilter = { + debug: true, + paths: [opts.pattern], + }; + } else if (opts.filter) { + testFilter = await readTestFilter(); + } + const results = await runFixtures(worker, testFilter, 0); if (opts.update) { update(results);