Skip to content

Commit 1d9ccd1

Browse files
committed
[Compiler] ValidateNoDerivedComputationsInEffects test cases
Summary: This creates the test cases we expect this first iteration of calculate in render to catch The goal is to have tests that will be in a good state once we have the first iteration of the calculate in render validation working, which should be pretty limited in what its capturing. Test Plan: Test cases
1 parent 720bb13 commit 1d9ccd1

File tree

29 files changed

+1338
-9
lines changed

29 files changed

+1338
-9
lines changed

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoF
103103
import {inferMutationAliasingEffects} from '../Inference/InferMutationAliasingEffects';
104104
import {inferMutationAliasingRanges} from '../Inference/InferMutationAliasingRanges';
105105
import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDerivedComputationsInEffects';
106+
import {validateNoDerivedComputationsInEffects_exp} from '../Validation/ValidateNoDerivedComputationsInEffects_exp';
106107
import {nameAnonymousFunctions} from '../Transform/NameAnonymousFunctions';
107108

108109
export type CompilerPipelineValue =
@@ -275,6 +276,10 @@ function runWithEnvironment(
275276
validateNoDerivedComputationsInEffects(hir);
276277
}
277278

279+
if (env.config.validateNoDerivedComputationsInEffects_exp) {
280+
validateNoDerivedComputationsInEffects_exp(hir);
281+
}
282+
278283
if (env.config.validateNoSetStateInEffects) {
279284
env.logErrors(validateNoSetStateInEffects(hir, env));
280285
}

compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,12 @@ export const EnvironmentConfigSchema = z.object({
334334
*/
335335
validateNoDerivedComputationsInEffects: z.boolean().default(false),
336336

337+
/**
338+
* Experimental: Validates that effects are not used to calculate derived data which could instead be computed
339+
* during render. Generates a custom error message for each type of violation.
340+
*/
341+
validateNoDerivedComputationsInEffects_exp: z.boolean().default(false),
342+
337343
/**
338344
* Validates against creating JSX within a try block and recommends using an error boundary
339345
* instead.
Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,240 @@
1+
/**
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
import {CompilerError, SourceLocation} from '..';
9+
import {ErrorCategory} from '../CompilerError';
10+
import {
11+
ArrayExpression,
12+
BlockId,
13+
FunctionExpression,
14+
HIRFunction,
15+
IdentifierId,
16+
isSetStateType,
17+
isUseEffectHookType,
18+
} from '../HIR';
19+
import {
20+
eachInstructionValueOperand,
21+
eachTerminalOperand,
22+
} from '../HIR/visitors';
23+
24+
/**
25+
* Validates that useEffect is not used for derived computations which could/should
26+
* be performed in render.
27+
*
28+
* See https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state
29+
*
30+
* Example:
31+
*
32+
* ```
33+
* // 🔴 Avoid: redundant state and unnecessary Effect
34+
* const [fullName, setFullName] = useState('');
35+
* useEffect(() => {
36+
* setFullName(firstName + ' ' + lastName);
37+
* }, [firstName, lastName]);
38+
* ```
39+
*
40+
* Instead use:
41+
*
42+
* ```
43+
* // ✅ Good: calculated during rendering
44+
* const fullName = firstName + ' ' + lastName;
45+
* ```
46+
*/
47+
export function validateNoDerivedComputationsInEffects_exp(
48+
fn: HIRFunction,
49+
): void {
50+
const candidateDependencies: Map<IdentifierId, ArrayExpression> = new Map();
51+
const functions: Map<IdentifierId, FunctionExpression> = new Map();
52+
const locals: Map<IdentifierId, IdentifierId> = new Map();
53+
54+
const errors = new CompilerError();
55+
56+
for (const block of fn.body.blocks.values()) {
57+
for (const instr of block.instructions) {
58+
const {lvalue, value} = instr;
59+
if (value.kind === 'LoadLocal') {
60+
locals.set(lvalue.identifier.id, value.place.identifier.id);
61+
} else if (value.kind === 'ArrayExpression') {
62+
candidateDependencies.set(lvalue.identifier.id, value);
63+
} else if (value.kind === 'FunctionExpression') {
64+
functions.set(lvalue.identifier.id, value);
65+
} else if (
66+
value.kind === 'CallExpression' ||
67+
value.kind === 'MethodCall'
68+
) {
69+
const callee =
70+
value.kind === 'CallExpression' ? value.callee : value.property;
71+
if (
72+
isUseEffectHookType(callee.identifier) &&
73+
value.args.length === 2 &&
74+
value.args[0].kind === 'Identifier' &&
75+
value.args[1].kind === 'Identifier'
76+
) {
77+
const effectFunction = functions.get(value.args[0].identifier.id);
78+
const deps = candidateDependencies.get(value.args[1].identifier.id);
79+
if (
80+
effectFunction != null &&
81+
deps != null &&
82+
deps.elements.length !== 0 &&
83+
deps.elements.every(element => element.kind === 'Identifier')
84+
) {
85+
const dependencies: Array<IdentifierId> = deps.elements.map(dep => {
86+
CompilerError.invariant(dep.kind === 'Identifier', {
87+
reason: `Dependency is checked as a place above`,
88+
description: null,
89+
details: [
90+
{
91+
kind: 'error',
92+
loc: value.loc,
93+
message: 'this is checked as a place above',
94+
},
95+
],
96+
});
97+
return locals.get(dep.identifier.id) ?? dep.identifier.id;
98+
});
99+
validateEffect(
100+
effectFunction.loweredFunc.func,
101+
dependencies,
102+
errors,
103+
);
104+
}
105+
}
106+
}
107+
}
108+
}
109+
if (errors.hasAnyErrors()) {
110+
throw errors;
111+
}
112+
}
113+
114+
function validateEffect(
115+
effectFunction: HIRFunction,
116+
effectDeps: Array<IdentifierId>,
117+
errors: CompilerError,
118+
): void {
119+
for (const operand of effectFunction.context) {
120+
if (isSetStateType(operand.identifier)) {
121+
continue;
122+
} else if (effectDeps.find(dep => dep === operand.identifier.id) != null) {
123+
continue;
124+
} else {
125+
// Captured something other than the effect dep or setState
126+
return;
127+
}
128+
}
129+
for (const dep of effectDeps) {
130+
if (
131+
effectFunction.context.find(operand => operand.identifier.id === dep) ==
132+
null
133+
) {
134+
// effect dep wasn't actually used in the function
135+
return;
136+
}
137+
}
138+
139+
const seenBlocks: Set<BlockId> = new Set();
140+
const values: Map<IdentifierId, Array<IdentifierId>> = new Map();
141+
for (const dep of effectDeps) {
142+
values.set(dep, [dep]);
143+
}
144+
145+
const setStateLocations: Array<SourceLocation> = [];
146+
for (const block of effectFunction.body.blocks.values()) {
147+
for (const pred of block.preds) {
148+
if (!seenBlocks.has(pred)) {
149+
// skip if block has a back edge
150+
return;
151+
}
152+
}
153+
for (const phi of block.phis) {
154+
const aggregateDeps: Set<IdentifierId> = new Set();
155+
for (const operand of phi.operands.values()) {
156+
const deps = values.get(operand.identifier.id);
157+
if (deps != null) {
158+
for (const dep of deps) {
159+
aggregateDeps.add(dep);
160+
}
161+
}
162+
}
163+
if (aggregateDeps.size !== 0) {
164+
values.set(phi.place.identifier.id, Array.from(aggregateDeps));
165+
}
166+
}
167+
for (const instr of block.instructions) {
168+
switch (instr.value.kind) {
169+
case 'Primitive':
170+
case 'JSXText':
171+
case 'LoadGlobal': {
172+
break;
173+
}
174+
case 'LoadLocal': {
175+
const deps = values.get(instr.value.place.identifier.id);
176+
if (deps != null) {
177+
values.set(instr.lvalue.identifier.id, deps);
178+
}
179+
break;
180+
}
181+
case 'ComputedLoad':
182+
case 'PropertyLoad':
183+
case 'BinaryExpression':
184+
case 'TemplateLiteral':
185+
case 'CallExpression':
186+
case 'MethodCall': {
187+
const aggregateDeps: Set<IdentifierId> = new Set();
188+
for (const operand of eachInstructionValueOperand(instr.value)) {
189+
const deps = values.get(operand.identifier.id);
190+
if (deps != null) {
191+
for (const dep of deps) {
192+
aggregateDeps.add(dep);
193+
}
194+
}
195+
}
196+
if (aggregateDeps.size !== 0) {
197+
values.set(instr.lvalue.identifier.id, Array.from(aggregateDeps));
198+
}
199+
200+
if (
201+
instr.value.kind === 'CallExpression' &&
202+
isSetStateType(instr.value.callee.identifier) &&
203+
instr.value.args.length === 1 &&
204+
instr.value.args[0].kind === 'Identifier'
205+
) {
206+
const deps = values.get(instr.value.args[0].identifier.id);
207+
if (deps != null && new Set(deps).size === effectDeps.length) {
208+
setStateLocations.push(instr.value.callee.loc);
209+
} else {
210+
// doesn't depend on any deps
211+
return;
212+
}
213+
}
214+
break;
215+
}
216+
default: {
217+
return;
218+
}
219+
}
220+
}
221+
for (const operand of eachTerminalOperand(block.terminal)) {
222+
if (values.has(operand.identifier.id)) {
223+
//
224+
return;
225+
}
226+
}
227+
seenBlocks.add(block.id);
228+
}
229+
230+
for (const loc of setStateLocations) {
231+
errors.push({
232+
category: ErrorCategory.EffectDerivationsOfState,
233+
reason:
234+
'Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)',
235+
description: null,
236+
loc,
237+
suggestions: null,
238+
});
239+
}
240+
}
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validateNoDerivedComputationsInEffects_exp
6+
import {useEffect, useState} from 'react';
7+
8+
function Component({value, enabled}) {
9+
const [localValue, setLocalValue] = useState('');
10+
11+
useEffect(() => {
12+
if (enabled) {
13+
setLocalValue(value);
14+
} else {
15+
setLocalValue('disabled');
16+
}
17+
}, [value, enabled]);
18+
19+
return <div>{localValue}</div>;
20+
}
21+
22+
export const FIXTURE_ENTRYPOINT = {
23+
fn: Component,
24+
params: [{value: 'test', enabled: true}],
25+
};
26+
27+
```
28+
29+
## Code
30+
31+
```javascript
32+
import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp
33+
import { useEffect, useState } from "react";
34+
35+
function Component(t0) {
36+
const $ = _c(6);
37+
const { value, enabled } = t0;
38+
const [localValue, setLocalValue] = useState("");
39+
let t1;
40+
let t2;
41+
if ($[0] !== enabled || $[1] !== value) {
42+
t1 = () => {
43+
if (enabled) {
44+
setLocalValue(value);
45+
} else {
46+
setLocalValue("disabled");
47+
}
48+
};
49+
50+
t2 = [value, enabled];
51+
$[0] = enabled;
52+
$[1] = value;
53+
$[2] = t1;
54+
$[3] = t2;
55+
} else {
56+
t1 = $[2];
57+
t2 = $[3];
58+
}
59+
useEffect(t1, t2);
60+
let t3;
61+
if ($[4] !== localValue) {
62+
t3 = <div>{localValue}</div>;
63+
$[4] = localValue;
64+
$[5] = t3;
65+
} else {
66+
t3 = $[5];
67+
}
68+
return t3;
69+
}
70+
71+
export const FIXTURE_ENTRYPOINT = {
72+
fn: Component,
73+
params: [{ value: "test", enabled: true }],
74+
};
75+
76+
```
77+
78+
### Eval output
79+
(kind: ok) <div>test</div>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// @validateNoDerivedComputationsInEffects_exp
2+
import {useEffect, useState} from 'react';
3+
4+
function Component({value, enabled}) {
5+
const [localValue, setLocalValue] = useState('');
6+
7+
useEffect(() => {
8+
if (enabled) {
9+
setLocalValue(value);
10+
} else {
11+
setLocalValue('disabled');
12+
}
13+
}, [value, enabled]);
14+
15+
return <div>{localValue}</div>;
16+
}
17+
18+
export const FIXTURE_ENTRYPOINT = {
19+
fn: Component,
20+
params: [{value: 'test', enabled: true}],
21+
};

0 commit comments

Comments
 (0)