From d1f01a789b0ad15809df94df6d08e60fb17d8d77 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Thu, 30 Oct 2025 15:07:46 -0700 Subject: [PATCH] [compiler] Don't validate when effect cleanup function depends on effect localized setState state derived values --- ...idateNoDerivedComputationsInEffects_exp.ts | 45 +++++++++++ ...ing-on-derived-computation-value.expect.md | 76 +++++++++++++++++++ ...-depending-on-derived-computation-value.js | 21 +++++ 3 files changed, 142 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-cleanup-function-depending-on-derived-computation-value.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-cleanup-function-depending-on-derived-computation-value.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts index 7eb820d87d..a2066c7c3b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts @@ -462,6 +462,7 @@ function buildDataFlowTree( context: ValidationContext, propsSet: Set, stateSet: Set, + allSetStateDeps: Set, ): string { const sourceMetadata = context.derivationCache.cache.get(sourceId); if (!sourceMetadata || !sourceMetadata.place.identifier.name?.value) { @@ -480,6 +481,8 @@ function buildDataFlowTree( let result = `${prefix}${sourceName}`; + allSetStateDeps.add(sourceMetadata.place.identifier.id); + if (isOriginal) { let typeLabel: string; if (sourceMetadata.typeOfValue === 'fromProps') { @@ -506,6 +509,7 @@ function buildDataFlowTree( context, propsSet, stateSet, + allSetStateDeps, ); if (childTree) { result += childTree + '\n'; @@ -517,6 +521,23 @@ function buildDataFlowTree( return result; } +function getFnGlobalDeps( + fn: FunctionExpression | undefined, + deps: Set, +) { + if (!fn) { + return; + } + + for (const [, block] of fn.loweredFunc.func.body.blocks) { + for (const instr of block.instructions) { + if (instr.value.kind === 'LoadLocal') { + deps.add(instr.value.place.identifier.id); + } + } + } +} + function validateEffect( effectFunction: HIRFunction, context: ValidationContext, @@ -535,8 +556,24 @@ function validateEffect( Set > = new Map(); + const cleanUpFunctionDeps: Set = new Set(); + const globals: Set = new Set(); for (const block of effectFunction.body.blocks.values()) { + /* + * if the block is in an effect and is of type return then its an effect's cleanup function + * if the cleanup function depends on a value from which effect-set state is derived then + * we can't validate + */ + if ( + block.terminal.kind === 'return' && + block.terminal.returnVariant === 'Explicit' + ) { + getFnGlobalDeps( + context.functions.get(block.terminal.value.identifier.id), + cleanUpFunctionDeps, + ); + } for (const pred of block.preds) { if (!seenBlocks.has(pred)) { // skip if block has a back edge @@ -626,6 +663,7 @@ function validateEffect( const allSourceIds = Array.from(derivedSetStateCall.sourceIds); const propsSet = new Set(); const stateSet = new Set(); + const allSetStateDeps = new Set(); const trees = allSourceIds .map((id, index) => @@ -636,10 +674,17 @@ function validateEffect( context, propsSet, stateSet, + allSetStateDeps, ), ) .filter(Boolean); + for (const dep of allSetStateDeps) { + if (cleanUpFunctionDeps.has(dep)) { + return; + } + } + const propsArr = Array.from(propsSet); const stateArr = Array.from(stateSet); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-cleanup-function-depending-on-derived-computation-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-cleanup-function-depending-on-derived-computation-value.expect.md new file mode 100644 index 0000000000..e84031591e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-cleanup-function-depending-on-derived-computation-value.expect.md @@ -0,0 +1,76 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +import {useEffect, useState} from 'react'; + +function Component(file: File) { + const [imageUrl, setImageUrl] = useState(null); + + /* + * Cleaning up the variable or a source of the variable used to setState + * inside the effect communicates that we always need to clean up something + * which is a valid use case for useEffect. In which case we want to + * avoid an throwing + */ + useEffect(() => { + const imageUrlPrepared = URL.createObjectURL(file); + setImageUrl(imageUrlPrepared); + return () => URL.revokeObjectURL(imageUrlPrepared); + }, [file]); + + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +import { useEffect, useState } from "react"; + +function Component(file) { + const $ = _c(5); + const [imageUrl, setImageUrl] = useState(null); + let t0; + let t1; + if ($[0] !== file) { + t0 = () => { + const imageUrlPrepared = URL.createObjectURL(file); + setImageUrl(imageUrlPrepared); + return () => URL.revokeObjectURL(imageUrlPrepared); + }; + t1 = [file]; + $[0] = file; + $[1] = t0; + $[2] = t1; + } else { + t0 = $[1]; + t1 = $[2]; + } + useEffect(t0, t1); + let t2; + if ($[3] !== imageUrl) { + t2 = ; + $[3] = imageUrl; + $[4] = t2; + } else { + t2 = $[4]; + } + return t2; +} + +``` + +## Logs + +``` +{"kind":"CompileSuccess","fnLoc":{"start":{"line":5,"column":0,"index":108},"end":{"line":21,"column":1,"index":700},"filename":"effect-with-cleanup-function-depending-on-derived-computation-value.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-cleanup-function-depending-on-derived-computation-value.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-cleanup-function-depending-on-derived-computation-value.js new file mode 100644 index 0000000000..e419583cc6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-cleanup-function-depending-on-derived-computation-value.js @@ -0,0 +1,21 @@ +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +import {useEffect, useState} from 'react'; + +function Component(file: File) { + const [imageUrl, setImageUrl] = useState(null); + + /* + * Cleaning up the variable or a source of the variable used to setState + * inside the effect communicates that we always need to clean up something + * which is a valid use case for useEffect. In which case we want to + * avoid an throwing + */ + useEffect(() => { + const imageUrlPrepared = URL.createObjectURL(file); + setImageUrl(imageUrlPrepared); + return () => URL.revokeObjectURL(imageUrlPrepared); + }, [file]); + + return ; +}