Skip to content

Commit b3f1831

Browse files
committed
[compiler] Prevent innaccurate derivation recording on FunctionExpressions on no-derived-computation-in-effects
Summary: The operands of a function expression are the elements passed as context. This means that it doesn't make sense to record mutations for them. The relevant mutations will happen in the function body, so we need to prevent FunctionExpression type instruction from running the logic for effect mutations. This was also causing some values to depend on themselves in some cases triggering an infinite loop. Also added n invariant to prevent this issue Test Plan: Added fixture test
1 parent 8ac5f4e commit b3f1831

File tree

3 files changed

+168
-0
lines changed

3 files changed

+168
-0
lines changed

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,14 @@ function recordInstructionDerivations(
422422
);
423423
}
424424

425+
if (value.kind === 'FunctionExpression') {
426+
/*
427+
* We don't want to record effect mutations of FunctionExpressions the mutations will happen in the
428+
* function body and we will record them there.
429+
*/
430+
return;
431+
}
432+
425433
for (const operand of eachInstructionOperand(instr)) {
426434
switch (operand.effect) {
427435
case Effect.Capture:
@@ -512,6 +520,19 @@ function buildTreeNode(
512520

513521
const namedSiblings: Set<string> = new Set();
514522
for (const childId of sourceMetadata.sourcesIds) {
523+
CompilerError.invariant(childId !== sourceId, {
524+
reason:
525+
'Unexpected self-reference: a value should not have itself as a source',
526+
description: null,
527+
details: [
528+
{
529+
kind: 'error',
530+
loc: sourceMetadata.place.loc,
531+
message: null,
532+
},
533+
],
534+
});
535+
515536
const childNodes = buildTreeNode(
516537
childId,
517538
context,
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
6+
7+
function Component() {
8+
const [foo, setFoo] = useState({});
9+
const [bar, setBar] = useState(new Set());
10+
11+
/*
12+
* isChanged is considered context of the effect's function expression,
13+
* if we don't bail out of effect mutation derivation tracking, isChanged
14+
* will inherit the sources of the effect's function expression.
15+
*
16+
* This is innacurate and with the multiple passes ends up causing an infinite loop.
17+
*/
18+
useEffect(() => {
19+
let isChanged = false;
20+
21+
const newData = foo.map(val => {
22+
bar.someMethod(val);
23+
isChanged = true;
24+
});
25+
26+
if (isChanged) {
27+
setFoo(newData);
28+
}
29+
}, [foo, bar]);
30+
31+
return (
32+
<div>
33+
{foo}, {bar}
34+
</div>
35+
);
36+
}
37+
38+
```
39+
40+
## Code
41+
42+
```javascript
43+
import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
44+
45+
function Component() {
46+
const $ = _c(9);
47+
let t0;
48+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
49+
t0 = {};
50+
$[0] = t0;
51+
} else {
52+
t0 = $[0];
53+
}
54+
const [foo, setFoo] = useState(t0);
55+
let t1;
56+
if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
57+
t1 = new Set();
58+
$[1] = t1;
59+
} else {
60+
t1 = $[1];
61+
}
62+
const [bar] = useState(t1);
63+
let t2;
64+
let t3;
65+
if ($[2] !== bar || $[3] !== foo) {
66+
t2 = () => {
67+
let isChanged = false;
68+
69+
const newData = foo.map((val) => {
70+
bar.someMethod(val);
71+
isChanged = true;
72+
});
73+
74+
if (isChanged) {
75+
setFoo(newData);
76+
}
77+
};
78+
79+
t3 = [foo, bar];
80+
$[2] = bar;
81+
$[3] = foo;
82+
$[4] = t2;
83+
$[5] = t3;
84+
} else {
85+
t2 = $[4];
86+
t3 = $[5];
87+
}
88+
useEffect(t2, t3);
89+
let t4;
90+
if ($[6] !== bar || $[7] !== foo) {
91+
t4 = (
92+
<div>
93+
{foo}, {bar}
94+
</div>
95+
);
96+
$[6] = bar;
97+
$[7] = foo;
98+
$[8] = t4;
99+
} else {
100+
t4 = $[8];
101+
}
102+
return t4;
103+
}
104+
105+
```
106+
107+
## Logs
108+
109+
```
110+
{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nState: [foo, bar]\n\nData Flow Tree:\n└── newData\n ├── foo (State)\n └── bar (State)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":23,"column":6,"index":663},"end":{"line":23,"column":12,"index":669},"filename":"function-expression-mutation-edge-case.ts","identifierName":"setFoo"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null}
111+
{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":32,"column":1,"index":762},"filename":"function-expression-mutation-edge-case.ts"},"fnName":"Component","memoSlots":9,"memoBlocks":4,"memoValues":5,"prunedMemoBlocks":0,"prunedMemoValues":0}
112+
```
113+
114+
### Eval output
115+
(kind: exception) Fixture not implemented
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
2+
3+
function Component() {
4+
const [foo, setFoo] = useState({});
5+
const [bar, setBar] = useState(new Set());
6+
7+
/*
8+
* isChanged is considered context of the effect's function expression,
9+
* if we don't bail out of effect mutation derivation tracking, isChanged
10+
* will inherit the sources of the effect's function expression.
11+
*
12+
* This is innacurate and with the multiple passes ends up causing an infinite loop.
13+
*/
14+
useEffect(() => {
15+
let isChanged = false;
16+
17+
const newData = foo.map(val => {
18+
bar.someMethod(val);
19+
isChanged = true;
20+
});
21+
22+
if (isChanged) {
23+
setFoo(newData);
24+
}
25+
}, [foo, bar]);
26+
27+
return (
28+
<div>
29+
{foo}, {bar}
30+
</div>
31+
);
32+
}

0 commit comments

Comments
 (0)