Skip to content

Commit 663ddab

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 663ddab

File tree

1 file changed

+59
-15
lines changed

1 file changed

+59
-15
lines changed

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

Lines changed: 59 additions & 15 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()) {
196226
recordPhiDerivations(block, context);
197227
for (const instr of block.instructions) {
198228
recordInstructionDerivations(instr, context, isFirstPass);
199229
}
200230
}
201231

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

@@ -331,11 +362,24 @@ function recordInstructionDerivations(
331362
case Effect.ConditionallyMutateIterator:
332363
case Effect.Mutate: {
333364
if (isMutable(instr, operand)) {
334-
context.derivationCache.addDerivationEntry(
335-
operand,
336-
sources,
337-
typeOfValue,
338-
);
365+
if (context.derivationCache.cache.has(operand.identifier.id)) {
366+
const operandMetadata = context.derivationCache.cache.get(
367+
operand.identifier.id,
368+
);
369+
370+
if (operandMetadata !== undefined) {
371+
operandMetadata.typeOfValue = joinValue(
372+
typeOfValue,
373+
operandMetadata.typeOfValue,
374+
);
375+
}
376+
} else {
377+
context.derivationCache.addDerivationEntry(
378+
operand,
379+
sources,
380+
typeOfValue,
381+
);
382+
}
339383
}
340384
break;
341385
}

0 commit comments

Comments
 (0)