Skip to content

Commit 2a9615b

Browse files
jorge-cabjorge-cab
committed
[compiler] Fix false negatives and add data flow tree to compiler error for no-deriving-state-in-effects
Summary: Revamped the derivationCache graph. This fixes a bunch of bugs where sometimes we fail to track from which props/state we derived values from. Also, it is more intuitive and allows us to easily implement a Data Flow Tree. We can print this tree which gives insight on how the data is derived and should facilitate error resolution in complicated components Test Plan: Added a test case where we were failing to track derivations. Also updated the test cases with the new error containing the data flow tree
1 parent 663ddab commit 2a9615b

12 files changed

+339
-64
lines changed

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

Lines changed: 187 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ type DerivationMetadata = {
3333
typeOfValue: TypeOfValue;
3434
place: Place;
3535
sourcesIds: Set<IdentifierId>;
36+
isStateSource: boolean;
3637
};
3738

3839
type ValidationContext = {
@@ -56,6 +57,7 @@ class DerivationCache {
5657
place: value.place,
5758
sourcesIds: new Set(value.sourcesIds),
5859
typeOfValue: value.typeOfValue,
60+
isStateSource: value.isStateSource,
5961
});
6062
}
6163
}
@@ -95,41 +97,28 @@ class DerivationCache {
9597
derivedVar: Place,
9698
sourcesIds: Set<IdentifierId>,
9799
typeOfValue: TypeOfValue,
100+
isStateSource: boolean,
98101
): void {
99-
let newValue: DerivationMetadata = {
100-
place: derivedVar,
101-
sourcesIds: new Set(),
102-
typeOfValue: typeOfValue ?? 'ignored',
103-
};
104-
105-
if (sourcesIds !== undefined) {
106-
for (const id of sourcesIds) {
107-
const sourcePlace = this.cache.get(id)?.place;
108-
109-
if (sourcePlace === undefined) {
110-
continue;
111-
}
112-
113-
/*
114-
* If the identifier of the source is a promoted identifier, then
115-
* we should set the target as the source.
116-
*/
102+
let finalIsSource = isStateSource;
103+
if (!finalIsSource) {
104+
for (const sourceId of sourcesIds) {
105+
const sourceMetadata = this.cache.get(sourceId);
117106
if (
118-
sourcePlace.identifier.name === null ||
119-
sourcePlace.identifier.name?.kind === 'promoted'
107+
sourceMetadata?.isStateSource &&
108+
sourceMetadata.place.identifier.name?.kind !== 'named'
120109
) {
121-
newValue.sourcesIds.add(derivedVar.identifier.id);
122-
} else {
123-
newValue.sourcesIds.add(sourcePlace.identifier.id);
110+
finalIsSource = true;
111+
break;
124112
}
125113
}
126114
}
127115

128-
if (newValue.sourcesIds.size === 0) {
129-
newValue.sourcesIds.add(derivedVar.identifier.id);
130-
}
131-
132-
this.cache.set(derivedVar.identifier.id, newValue);
116+
this.cache.set(derivedVar.identifier.id, {
117+
place: derivedVar,
118+
sourcesIds: sourcesIds,
119+
typeOfValue: typeOfValue ?? 'ignored',
120+
isStateSource: finalIsSource,
121+
});
133122
}
134123

135124
private isDerivationEqual(
@@ -151,6 +140,14 @@ class DerivationCache {
151140
}
152141
}
153142

143+
function isNamedIdentifier(place: Place): place is Place & {
144+
identifier: {name: NonNullable<Place['identifier']['name']>};
145+
} {
146+
return (
147+
place.identifier.name !== null && place.identifier.name.kind === 'named'
148+
);
149+
}
150+
154151
/**
155152
* Validates that useEffect is not used for derived computations which could/should
156153
* be performed in render.
@@ -202,8 +199,9 @@ export function validateNoDerivedComputationsInEffects_exp(
202199
if (param.kind === 'Identifier') {
203200
context.derivationCache.cache.set(param.identifier.id, {
204201
place: param,
205-
sourcesIds: new Set([param.identifier.id]),
202+
sourcesIds: new Set(),
206203
typeOfValue: 'fromProps',
204+
isStateSource: true,
207205
});
208206
}
209207
}
@@ -212,8 +210,9 @@ export function validateNoDerivedComputationsInEffects_exp(
212210
if (props != null && props.kind === 'Identifier') {
213211
context.derivationCache.cache.set(props.identifier.id, {
214212
place: props,
215-
sourcesIds: new Set([props.identifier.id]),
213+
sourcesIds: new Set(),
216214
typeOfValue: 'fromProps',
215+
isStateSource: true,
217216
});
218217
}
219218
}
@@ -267,6 +266,7 @@ function recordPhiDerivations(
267266
phi.place,
268267
sourcesIds,
269268
typeOfValue,
269+
false,
270270
);
271271
}
272272
}
@@ -288,11 +288,13 @@ function recordInstructionDerivations(
288288
isFirstPass: boolean,
289289
): void {
290290
let typeOfValue: TypeOfValue = 'ignored';
291+
let isSource: boolean = false;
291292
const sources: Set<IdentifierId> = new Set();
292293
const {lvalue, value} = instr;
293294
if (value.kind === 'FunctionExpression') {
294295
context.functions.set(lvalue.identifier.id, value);
295296
for (const [, block] of value.loweredFunc.func.body.blocks) {
297+
recordPhiDerivations(block, context);
296298
for (const instr of block.instructions) {
297299
recordInstructionDerivations(instr, context, isFirstPass);
298300
}
@@ -311,10 +313,7 @@ function recordInstructionDerivations(
311313
context.effects.add(effectFunction.loweredFunc.func);
312314
}
313315
} else if (isUseStateType(lvalue.identifier) && value.args.length > 0) {
314-
const stateValueSource = value.args[0];
315-
if (stateValueSource.kind === 'Identifier') {
316-
sources.add(stateValueSource.identifier.id);
317-
}
316+
isSource = true;
318317
typeOfValue = joinValue(typeOfValue, 'fromState');
319318
}
320319
}
@@ -341,17 +340,20 @@ function recordInstructionDerivations(
341340
}
342341

343342
typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue);
344-
for (const id of operandMetadata.sourcesIds) {
345-
sources.add(id);
346-
}
343+
sources.add(operand.identifier.id);
347344
}
348345

349346
if (typeOfValue === 'ignored') {
350347
return;
351348
}
352349

353350
for (const lvalue of eachInstructionLValue(instr)) {
354-
context.derivationCache.addDerivationEntry(lvalue, sources, typeOfValue);
351+
context.derivationCache.addDerivationEntry(
352+
lvalue,
353+
sources,
354+
typeOfValue,
355+
isSource,
356+
);
355357
}
356358

357359
for (const operand of eachInstructionOperand(instr)) {
@@ -378,6 +380,7 @@ function recordInstructionDerivations(
378380
operand,
379381
sources,
380382
typeOfValue,
383+
false,
381384
);
382385
}
383386
}
@@ -411,6 +414,107 @@ function recordInstructionDerivations(
411414
}
412415
}
413416

417+
type TreeNode = {
418+
name: string;
419+
typeOfValue: TypeOfValue;
420+
isSource: boolean;
421+
children: Array<TreeNode>;
422+
};
423+
424+
function buildTreeNode(
425+
sourceId: IdentifierId,
426+
context: ValidationContext,
427+
): TreeNode | null {
428+
const sourceMetadata = context.derivationCache.cache.get(sourceId);
429+
if (!sourceMetadata) {
430+
return null;
431+
}
432+
433+
const childSourceIds = Array.from(sourceMetadata.sourcesIds).filter(
434+
id => id !== sourceId,
435+
);
436+
437+
if (!isNamedIdentifier(sourceMetadata.place)) {
438+
const childrenMap = new Map<string, TreeNode>();
439+
for (const childId of childSourceIds) {
440+
const childNode = buildTreeNode(childId, context);
441+
if (childNode) {
442+
if (!childrenMap.has(childNode.name)) {
443+
childrenMap.set(childNode.name, childNode);
444+
}
445+
}
446+
}
447+
const children = Array.from(childrenMap.values());
448+
449+
if (children.length === 1) {
450+
return children[0];
451+
} else if (children.length > 1) {
452+
return null;
453+
}
454+
return null;
455+
}
456+
457+
const childrenMap = new Map<string, TreeNode>();
458+
for (const childId of childSourceIds) {
459+
const childNode = buildTreeNode(childId, context);
460+
if (childNode) {
461+
if (!childrenMap.has(childNode.name)) {
462+
childrenMap.set(childNode.name, childNode);
463+
}
464+
}
465+
}
466+
const children = Array.from(childrenMap.values());
467+
468+
return {
469+
name: sourceMetadata.place.identifier.name.value,
470+
typeOfValue: sourceMetadata.typeOfValue,
471+
isSource: sourceMetadata.isStateSource,
472+
children,
473+
};
474+
}
475+
476+
function renderTree(
477+
node: TreeNode,
478+
indent: string = '',
479+
isLast: boolean = true,
480+
propsSet: Set<string>,
481+
stateSet: Set<string>,
482+
): string {
483+
const prefix = indent + (isLast ? '└── ' : '├── ');
484+
const childIndent = indent + (isLast ? ' ' : '│ ');
485+
486+
let result = `${prefix}${node.name}`;
487+
488+
if (node.isSource) {
489+
let typeLabel: string;
490+
if (node.typeOfValue === 'fromProps') {
491+
propsSet.add(node.name);
492+
typeLabel = 'Prop';
493+
} else if (node.typeOfValue === 'fromState') {
494+
stateSet.add(node.name);
495+
typeLabel = 'State';
496+
} else {
497+
propsSet.add(node.name);
498+
stateSet.add(node.name);
499+
typeLabel = 'Prop and State';
500+
}
501+
result += ` (${typeLabel})`;
502+
}
503+
504+
if (node.children.length > 0) {
505+
result += '\n';
506+
node.children.forEach((child, index) => {
507+
const isLastChild = index === node.children.length - 1;
508+
result += renderTree(child, childIndent, isLastChild, propsSet, stateSet);
509+
if (index < node.children.length - 1) {
510+
result += '\n';
511+
}
512+
});
513+
}
514+
515+
return result;
516+
}
517+
414518
function validateEffect(
415519
effectFunction: HIRFunction,
416520
context: ValidationContext,
@@ -513,27 +617,55 @@ function validateEffect(
513617
.length -
514618
1
515619
) {
516-
const derivedDepsStr = Array.from(derivedSetStateCall.sourceIds)
517-
.map(sourceId => {
518-
const sourceMetadata = context.derivationCache.cache.get(sourceId);
519-
return sourceMetadata?.place.identifier.name?.value;
520-
})
521-
.filter(Boolean)
522-
.join(', ');
523-
524-
let description;
525-
526-
if (derivedSetStateCall.typeOfValue === 'fromProps') {
527-
description = `From props: [${derivedDepsStr}]`;
528-
} else if (derivedSetStateCall.typeOfValue === 'fromState') {
529-
description = `From local state: [${derivedDepsStr}]`;
530-
} else {
531-
description = `From props and local state: [${derivedDepsStr}]`;
620+
const allSourceIds = Array.from(derivedSetStateCall.sourceIds);
621+
const propsSet = new Set<string>();
622+
const stateSet = new Set<string>();
623+
624+
const treeNodesMap = new Map<string, TreeNode>();
625+
for (const id of allSourceIds) {
626+
const node = buildTreeNode(id, context);
627+
if (node && !treeNodesMap.has(node.name)) {
628+
treeNodesMap.set(node.name, node);
629+
}
630+
}
631+
const treeNodes = Array.from(treeNodesMap.values());
632+
633+
const trees = treeNodes.map((node, index) =>
634+
renderTree(
635+
node,
636+
'',
637+
index === treeNodes.length - 1,
638+
propsSet,
639+
stateSet,
640+
),
641+
);
642+
643+
const propsArr = Array.from(propsSet);
644+
const stateArr = Array.from(stateSet);
645+
646+
let rootSources = '';
647+
if (propsArr.length > 0) {
648+
rootSources += `Props: [${propsArr.join(', ')}]`;
649+
}
650+
if (stateArr.length > 0) {
651+
if (rootSources) rootSources += '\n';
652+
rootSources += `State: [${stateArr.join(', ')}]`;
532653
}
533654

655+
const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
656+
657+
This setState call is setting a derived value that depends on the following reactive sources:
658+
659+
${rootSources}
660+
661+
Data Flow Tree:
662+
${trees.join('\n')}
663+
664+
See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state`;
665+
534666
context.errors.pushDiagnostic(
535667
CompilerDiagnostic.create({
536-
description: `Derived values (${description}) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user`,
668+
description: description,
537669
category: ErrorCategory.EffectDerivationsOfState,
538670
reason:
539671
'You might not need an effect. Derive values in render, not effects.',

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,16 @@ Found 1 error:
3434
3535
Error: You might not need an effect. Derive values in render, not effects.
3636
37-
Derived values (From props: [value]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
37+
Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
38+
39+
This setState call is setting a derived value that depends on the following reactive sources:
40+
41+
Props: [value]
42+
43+
Data Flow Tree:
44+
└── value (Prop)
45+
46+
See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
3847
3948
error.derived-state-conditionally-in-effect.ts:9:6
4049
7 | useEffect(() => {

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,16 @@ Found 1 error:
3131
3232
Error: You might not need an effect. Derive values in render, not effects.
3333
34-
Derived values (From props: [input]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
34+
Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
35+
36+
This setState call is setting a derived value that depends on the following reactive sources:
37+
38+
Props: [input]
39+
40+
Data Flow Tree:
41+
└── input (Prop)
42+
43+
See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
3544
3645
error.derived-state-from-default-props.ts:9:4
3746
7 |

0 commit comments

Comments
 (0)