Skip to content

Commit 2b390b0

Browse files
committed
[compiler] Prevent overriding a derivationEntry on effect mutation and instead update typeOfValue and fix infinite loops
Summary: With this we are now comparing a snapshot of the derivationCache with the new changes every time we are done recording the derivations happening in the HIR. We have to do this after recording everything since we still do some mutations on the cache when recording mutations. Test Plan: Test the following in playground: ``` // @validateNoDerivedComputationsInEffects_exp function Component({ value }) { const [checked, setChecked] = useState(''); useEffect(() => { setChecked(value === '' ? [] : value.split(',')); }, [value]); return ( <div>{checked}</div> ) } ``` This no longer causes an infinite loop. Added a test case in the next PR in the stack
1 parent 408b38e commit 2b390b0

File tree

1 file changed

+55
-16
lines changed

1 file changed

+55
-16
lines changed

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts

Lines changed: 55 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,43 @@ type ValidationContext = {
4747
class DerivationCache {
4848
hasChanges: boolean = false;
4949
cache: Map<IdentifierId, DerivationMetadata> = new Map();
50+
private previousCache: Map<IdentifierId, DerivationMetadata> | null = null;
51+
52+
takeSnapshot(): void {
53+
this.previousCache = new Map();
54+
for (const [key, value] of this.cache.entries()) {
55+
this.previousCache.set(key, {
56+
place: value.place,
57+
sourcesIds: new Set(value.sourcesIds),
58+
typeOfValue: value.typeOfValue,
59+
});
60+
}
61+
}
62+
63+
checkForChanges(): void {
64+
if (this.previousCache === null) {
65+
this.hasChanges = true;
66+
return;
67+
}
68+
69+
for (const [key, value] of this.cache.entries()) {
70+
const previousValue = this.previousCache.get(key);
71+
if (
72+
previousValue === undefined ||
73+
!this.isDerivationEqual(previousValue, value)
74+
) {
75+
this.hasChanges = true;
76+
return;
77+
}
78+
}
79+
80+
if (this.cache.size !== this.previousCache.size) {
81+
this.hasChanges = true;
82+
return;
83+
}
84+
85+
this.hasChanges = false;
86+
}
5087

5188
snapshot(): boolean {
5289
const hasChanges = this.hasChanges;
@@ -92,14 +129,7 @@ class DerivationCache {
92129
newValue.sourcesIds.add(derivedVar.identifier.id);
93130
}
94131

95-
const existingValue = this.cache.get(derivedVar.identifier.id);
96-
if (
97-
existingValue === undefined ||
98-
!this.isDerivationEqual(existingValue, newValue)
99-
) {
100-
this.cache.set(derivedVar.identifier.id, newValue);
101-
this.hasChanges = true;
102-
}
132+
this.cache.set(derivedVar.identifier.id, newValue);
103133
}
104134

105135
private isDerivationEqual(
@@ -175,7 +205,6 @@ export function validateNoDerivedComputationsInEffects_exp(
175205
sourcesIds: new Set([param.identifier.id]),
176206
typeOfValue: 'fromProps',
177207
});
178-
context.derivationCache.hasChanges = true;
179208
}
180209
}
181210
} else if (fn.fnType === 'Component') {
@@ -186,19 +215,21 @@ export function validateNoDerivedComputationsInEffects_exp(
186215
sourcesIds: new Set([props.identifier.id]),
187216
typeOfValue: 'fromProps',
188217
});
189-
context.derivationCache.hasChanges = true;
190218
}
191219
}
192220

193221
let isFirstPass = true;
194222
do {
223+
context.derivationCache.takeSnapshot();
224+
195225
for (const block of fn.body.blocks.values()) {
196-
recordPhiDerivations(block, context);
197226
for (const instr of block.instructions) {
198227
recordInstructionDerivations(instr, context, isFirstPass);
199228
}
229+
recordPhiDerivations(block, context);
200230
}
201231

232+
context.derivationCache.checkForChanges();
202233
isFirstPass = false;
203234
} while (context.derivationCache.snapshot());
204235

@@ -330,12 +361,20 @@ function recordInstructionDerivations(
330361
case Effect.ConditionallyMutate:
331362
case Effect.ConditionallyMutateIterator:
332363
case Effect.Mutate: {
333-
if (isMutable(instr, operand)) {
334-
context.derivationCache.addDerivationEntry(
335-
operand,
336-
sources,
337-
typeOfValue,
364+
if (
365+
isMutable(instr, operand) &&
366+
context.derivationCache.cache.has(operand.identifier.id)
367+
) {
368+
const operandMetadata = context.derivationCache.cache.get(
369+
operand.identifier.id,
338370
);
371+
372+
if (operandMetadata !== undefined) {
373+
operandMetadata.typeOfValue = joinValue(
374+
typeOfValue,
375+
operandMetadata.typeOfValue,
376+
);
377+
}
339378
}
340379
break;
341380
}

0 commit comments

Comments
 (0)