Skip to content

Commit d4e81f0

Browse files
committed
[compiler] Don't validate when effect cleanup function depends on effect localized setState state derived values (#35020)
Summary: If we are using a clean up function in an effect and that clean up function depends on a value that is used to set the state we are validating for we shouldn't throw an error since it is a valid use case for an effect. Test Plan: added test --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35020). * #35044 * __->__ #35020 DiffTrain build for [92ac4e8](92ac4e8)
1 parent 8d2f7ad commit d4e81f0

35 files changed

+167
-122
lines changed

compiled/eslint-plugin-react-hooks/index.js

Lines changed: 81 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -52310,15 +52310,15 @@ function validateNoDerivedComputationsInEffects_exp(fn) {
5231052310
const derivationCache = new DerivationCache();
5231152311
const errors = new CompilerError();
5231252312
const effects = new Set();
52313-
const setStateCache = new Map();
52314-
const effectSetStateCache = new Map();
52313+
const setStateLoads = new Map();
52314+
const setStateUsages = new Map();
5231552315
const context = {
5231652316
functions,
5231752317
errors,
5231852318
derivationCache,
5231952319
effects,
52320-
setStateCache,
52321-
effectSetStateCache,
52320+
setStateLoads,
52321+
setStateUsages,
5232252322
};
5232352323
if (fn.fnType === 'Hook') {
5232452324
for (const param of fn.params) {
@@ -52343,17 +52343,15 @@ function validateNoDerivedComputationsInEffects_exp(fn) {
5234352343
});
5234452344
}
5234552345
}
52346-
let isFirstPass = true;
5234752346
do {
5234852347
context.derivationCache.takeSnapshot();
5234952348
for (const block of fn.body.blocks.values()) {
5235052349
recordPhiDerivations(block, context);
5235152350
for (const instr of block.instructions) {
52352-
recordInstructionDerivations(instr, context, isFirstPass);
52351+
recordInstructionDerivations(instr, context);
5235352352
}
5235452353
}
5235552354
context.derivationCache.checkForChanges();
52356-
isFirstPass = false;
5235752355
} while (context.derivationCache.snapshot());
5235852356
for (const effect of effects) {
5235952357
validateEffect(effect, context);
@@ -52386,7 +52384,40 @@ function joinValue(lvalueType, valueType) {
5238652384
return lvalueType;
5238752385
return 'fromPropsAndState';
5238852386
}
52387+
function getRootSetState(key, loads, visited = new Set()) {
52388+
if (visited.has(key)) {
52389+
return null;
52390+
}
52391+
visited.add(key);
52392+
const parentId = loads.get(key);
52393+
if (parentId === undefined) {
52394+
return null;
52395+
}
52396+
if (parentId === null) {
52397+
return key;
52398+
}
52399+
return getRootSetState(parentId, loads, visited);
52400+
}
52401+
function maybeRecordSetState(instr, loads, usages) {
52402+
for (const operand of eachInstructionLValue(instr)) {
52403+
if (instr.value.kind === 'LoadLocal' &&
52404+
loads.has(instr.value.place.identifier.id)) {
52405+
loads.set(operand.identifier.id, instr.value.place.identifier.id);
52406+
}
52407+
else {
52408+
if (isSetStateType(operand.identifier)) {
52409+
loads.set(operand.identifier.id, null);
52410+
}
52411+
}
52412+
const rootSetState = getRootSetState(operand.identifier.id, loads);
52413+
if (rootSetState !== null && usages.get(rootSetState) === undefined) {
52414+
usages.set(rootSetState, new Set([operand.loc]));
52415+
}
52416+
}
52417+
}
5238952418
function recordInstructionDerivations(instr, context, isFirstPass) {
52419+
var _a;
52420+
maybeRecordSetState(instr, context.setStateLoads, context.setStateUsages);
5239052421
let typeOfValue = 'ignored';
5239152422
let isSource = false;
5239252423
const sources = new Set();
@@ -52396,7 +52427,7 @@ function recordInstructionDerivations(instr, context, isFirstPass) {
5239652427
for (const [, block] of value.loweredFunc.func.body.blocks) {
5239752428
recordPhiDerivations(block, context);
5239852429
for (const instr of block.instructions) {
52399-
recordInstructionDerivations(instr, context, isFirstPass);
52430+
recordInstructionDerivations(instr, context);
5240052431
}
5240152432
}
5240252433
}
@@ -52417,14 +52448,10 @@ function recordInstructionDerivations(instr, context, isFirstPass) {
5241752448
}
5241852449
}
5241952450
for (const operand of eachInstructionOperand(instr)) {
52420-
if (isSetStateType(operand.identifier) &&
52421-
operand.loc !== GeneratedSource &&
52422-
isFirstPass) {
52423-
if (context.setStateCache.has(operand.loc.identifierName)) {
52424-
context.setStateCache.get(operand.loc.identifierName).push(operand);
52425-
}
52426-
else {
52427-
context.setStateCache.set(operand.loc.identifierName, [operand]);
52451+
if (context.setStateLoads.has(operand.identifier.id)) {
52452+
const rootSetStateId = getRootSetState(operand.identifier.id, context.setStateLoads);
52453+
if (rootSetStateId !== null) {
52454+
(_a = context.setStateUsages.get(rootSetStateId)) === null || _a === void 0 ? void 0 : _a.add(operand.loc);
5242852455
}
5242952456
}
5243052457
const operandMetadata = context.derivationCache.cache.get(operand.identifier.id);
@@ -52562,11 +52589,32 @@ function renderTree(node, indent = '', isLast = true, propsSet, stateSet) {
5256252589
}
5256352590
return result;
5256452591
}
52592+
function getFnLocalDeps(fn) {
52593+
if (!fn) {
52594+
return undefined;
52595+
}
52596+
const deps = new Set();
52597+
for (const [, block] of fn.loweredFunc.func.body.blocks) {
52598+
for (const instr of block.instructions) {
52599+
if (instr.value.kind === 'LoadLocal') {
52600+
deps.add(instr.value.place.identifier.id);
52601+
}
52602+
}
52603+
}
52604+
return deps;
52605+
}
5256552606
function validateEffect(effectFunction, context) {
52607+
var _a;
5256652608
const seenBlocks = new Set();
5256752609
const effectDerivedSetStateCalls = [];
52610+
const effectSetStateUsages = new Map();
52611+
let cleanUpFunctionDeps;
5256852612
const globals = new Set();
5256952613
for (const block of effectFunction.body.blocks.values()) {
52614+
if (block.terminal.kind === 'return' &&
52615+
block.terminal.returnVariant === 'Explicit') {
52616+
cleanUpFunctionDeps = getFnLocalDeps(context.functions.get(block.terminal.value.identifier.id));
52617+
}
5257052618
for (const pred of block.preds) {
5257152619
if (!seenBlocks.has(pred)) {
5257252620
return;
@@ -52576,18 +52624,12 @@ function validateEffect(effectFunction, context) {
5257652624
if (isUseRefType(instr.lvalue.identifier)) {
5257752625
return;
5257852626
}
52627+
maybeRecordSetState(instr, context.setStateLoads, effectSetStateUsages);
5257952628
for (const operand of eachInstructionOperand(instr)) {
52580-
if (isSetStateType(operand.identifier) &&
52581-
operand.loc !== GeneratedSource) {
52582-
if (context.effectSetStateCache.has(operand.loc.identifierName)) {
52583-
context.effectSetStateCache
52584-
.get(operand.loc.identifierName)
52585-
.push(operand);
52586-
}
52587-
else {
52588-
context.effectSetStateCache.set(operand.loc.identifierName, [
52589-
operand,
52590-
]);
52629+
if (context.setStateLoads.has(operand.identifier.id)) {
52630+
const rootSetStateId = getRootSetState(operand.identifier.id, context.setStateLoads);
52631+
if (rootSetStateId !== null) {
52632+
(_a = effectSetStateUsages.get(rootSetStateId)) === null || _a === void 0 ? void 0 : _a.add(operand.loc);
5259152633
}
5259252634
}
5259352635
}
@@ -52599,7 +52641,7 @@ function validateEffect(effectFunction, context) {
5259952641
if (argMetadata !== undefined) {
5260052642
effectDerivedSetStateCalls.push({
5260152643
value: instr.value,
52602-
loc: instr.value.callee.loc,
52644+
id: instr.value.callee.identifier.id,
5260352645
sourceIds: argMetadata.sourcesIds,
5260452646
typeOfValue: argMetadata.typeOfValue,
5260552647
});
@@ -52626,14 +52668,12 @@ function validateEffect(effectFunction, context) {
5262652668
seenBlocks.add(block.id);
5262752669
}
5262852670
for (const derivedSetStateCall of effectDerivedSetStateCalls) {
52629-
if (derivedSetStateCall.loc !== GeneratedSource &&
52630-
context.effectSetStateCache.has(derivedSetStateCall.loc.identifierName) &&
52631-
context.setStateCache.has(derivedSetStateCall.loc.identifierName) &&
52632-
context.effectSetStateCache.get(derivedSetStateCall.loc.identifierName)
52633-
.length ===
52634-
context.setStateCache.get(derivedSetStateCall.loc.identifierName)
52635-
.length -
52636-
1) {
52671+
const rootSetStateCall = getRootSetState(derivedSetStateCall.id, context.setStateLoads);
52672+
if (rootSetStateCall !== null &&
52673+
effectSetStateUsages.has(rootSetStateCall) &&
52674+
context.setStateUsages.has(rootSetStateCall) &&
52675+
effectSetStateUsages.get(rootSetStateCall).size ===
52676+
context.setStateUsages.get(rootSetStateCall).size - 1) {
5263752677
const propsSet = new Set();
5263852678
const stateSet = new Set();
5263952679
const rootNodesMap = new Map();
@@ -52647,6 +52687,11 @@ function validateEffect(effectFunction, context) {
5264752687
}
5264852688
const rootNodes = Array.from(rootNodesMap.values());
5264952689
const trees = rootNodes.map((node, index) => renderTree(node, '', index === rootNodes.length - 1, propsSet, stateSet));
52690+
for (const dep of derivedSetStateCall.sourceIds) {
52691+
if (cleanUpFunctionDeps !== undefined && cleanUpFunctionDeps.has(dep)) {
52692+
return;
52693+
}
52694+
}
5265052695
const propsArr = Array.from(propsSet);
5265152696
const stateArr = Array.from(stateSet);
5265252697
let rootSources = '';

compiled/facebook-www/REVISION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
72961203966a2f1d34dfca089e0a94a94ead7658
1+
92ac4e8b80cb51a1be7071e8338176680ce8f619
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
72961203966a2f1d34dfca089e0a94a94ead7658
1+
92ac4e8b80cb51a1be7071e8338176680ce8f619

compiled/facebook-www/React-dev.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1499,7 +1499,7 @@ __DEV__ &&
14991499
exports.useTransition = function () {
15001500
return resolveDispatcher().useTransition();
15011501
};
1502-
exports.version = "19.3.0-www-classic-72961203-20251110";
1502+
exports.version = "19.3.0-www-classic-92ac4e8b-20251110";
15031503
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
15041504
"function" ===
15051505
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-dev.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1499,7 +1499,7 @@ __DEV__ &&
14991499
exports.useTransition = function () {
15001500
return resolveDispatcher().useTransition();
15011501
};
1502-
exports.version = "19.3.0-www-modern-72961203-20251110";
1502+
exports.version = "19.3.0-www-modern-92ac4e8b-20251110";
15031503
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
15041504
"function" ===
15051505
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-prod.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -606,4 +606,4 @@ exports.useSyncExternalStore = function (
606606
exports.useTransition = function () {
607607
return ReactSharedInternals.H.useTransition();
608608
};
609-
exports.version = "19.3.0-www-classic-72961203-20251110";
609+
exports.version = "19.3.0-www-classic-92ac4e8b-20251110";

compiled/facebook-www/React-prod.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -606,4 +606,4 @@ exports.useSyncExternalStore = function (
606606
exports.useTransition = function () {
607607
return ReactSharedInternals.H.useTransition();
608608
};
609-
exports.version = "19.3.0-www-modern-72961203-20251110";
609+
exports.version = "19.3.0-www-modern-92ac4e8b-20251110";

compiled/facebook-www/React-profiling.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ exports.useSyncExternalStore = function (
610610
exports.useTransition = function () {
611611
return ReactSharedInternals.H.useTransition();
612612
};
613-
exports.version = "19.3.0-www-classic-72961203-20251110";
613+
exports.version = "19.3.0-www-classic-92ac4e8b-20251110";
614614
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
615615
"function" ===
616616
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-profiling.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ exports.useSyncExternalStore = function (
610610
exports.useTransition = function () {
611611
return ReactSharedInternals.H.useTransition();
612612
};
613-
exports.version = "19.3.0-www-modern-72961203-20251110";
613+
exports.version = "19.3.0-www-modern-92ac4e8b-20251110";
614614
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
615615
"function" ===
616616
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/ReactART-dev.classic.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20466,10 +20466,10 @@ __DEV__ &&
2046620466
(function () {
2046720467
var internals = {
2046820468
bundleType: 1,
20469-
version: "19.3.0-www-classic-72961203-20251110",
20469+
version: "19.3.0-www-classic-92ac4e8b-20251110",
2047020470
rendererPackageName: "react-art",
2047120471
currentDispatcherRef: ReactSharedInternals,
20472-
reconcilerVersion: "19.3.0-www-classic-72961203-20251110"
20472+
reconcilerVersion: "19.3.0-www-classic-92ac4e8b-20251110"
2047320473
};
2047420474
internals.overrideHookState = overrideHookState;
2047520475
internals.overrideHookStateDeletePath = overrideHookStateDeletePath;
@@ -20504,7 +20504,7 @@ __DEV__ &&
2050420504
exports.Shape = Shape;
2050520505
exports.Surface = Surface;
2050620506
exports.Text = Text;
20507-
exports.version = "19.3.0-www-classic-72961203-20251110";
20507+
exports.version = "19.3.0-www-classic-92ac4e8b-20251110";
2050820508
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
2050920509
"function" ===
2051020510
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

0 commit comments

Comments
 (0)