Skip to content

Commit d3b26b2

Browse files
josephsavonajosephsavona
authored
[compiler] rebase facebook#32285 (facebook#34102)
Redo of facebook#32285 which was created with ghstack and is tedious to rebase with sapling.
1 parent b211d70 commit d3b26b2

File tree

5 files changed

+293
-0
lines changed

5 files changed

+293
-0
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
@@ -106,6 +106,7 @@ import {validateStaticComponents} from '../Validation/ValidateStaticComponents';
106106
import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoFreezingKnownMutableFunctions';
107107
import {inferMutationAliasingEffects} from '../Inference/InferMutationAliasingEffects';
108108
import {inferMutationAliasingRanges} from '../Inference/InferMutationAliasingRanges';
109+
import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDerivedComputationsInEffects';
109110

110111
export type CompilerPipelineValue =
111112
| {kind: 'ast'; name: string; value: CodegenFunction}
@@ -292,6 +293,10 @@ function runWithEnvironment(
292293
validateNoSetStateInRender(hir).unwrap();
293294
}
294295

296+
if (env.config.validateNoDerivedComputationsInEffects) {
297+
validateNoDerivedComputationsInEffects(hir);
298+
}
299+
295300
if (env.config.validateNoSetStateInEffects) {
296301
env.logErrors(validateNoSetStateInEffects(hir));
297302
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,12 @@ export const EnvironmentConfigSchema = z.object({
323323
*/
324324
validateNoSetStateInEffects: z.boolean().default(false),
325325

326+
/**
327+
* Validates that effects are not used to calculate derived data which could instead be computed
328+
* during render.
329+
*/
330+
validateNoDerivedComputationsInEffects: z.boolean().default(false),
331+
326332
/**
327333
* Validates against creating JSX within a try block and recommends using an error boundary
328334
* instead.

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

Lines changed: 230 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,230 @@
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, ErrorSeverity, SourceLocation} from '..';
9+
import {
10+
ArrayExpression,
11+
BlockId,
12+
FunctionExpression,
13+
HIRFunction,
14+
IdentifierId,
15+
isSetStateType,
16+
isUseEffectHookType,
17+
} from '../HIR';
18+
import {
19+
eachInstructionValueOperand,
20+
eachTerminalOperand,
21+
} from '../HIR/visitors';
22+
23+
/**
24+
* Validates that useEffect is not used for derived computations which could/should
25+
* be performed in render.
26+
*
27+
* See https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state
28+
*
29+
* Example:
30+
*
31+
* ```
32+
* // 🔴 Avoid: redundant state and unnecessary Effect
33+
* const [fullName, setFullName] = useState('');
34+
* useEffect(() => {
35+
* setFullName(firstName + ' ' + lastName);
36+
* }, [firstName, lastName]);
37+
* ```
38+
*
39+
* Instead use:
40+
*
41+
* ```
42+
* // ✅ Good: calculated during rendering
43+
* const fullName = firstName + ' ' + lastName;
44+
* ```
45+
*/
46+
export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
47+
const candidateDependencies: Map<IdentifierId, ArrayExpression> = new Map();
48+
const functions: Map<IdentifierId, FunctionExpression> = new Map();
49+
const locals: Map<IdentifierId, IdentifierId> = new Map();
50+
51+
const errors = new CompilerError();
52+
53+
for (const block of fn.body.blocks.values()) {
54+
for (const instr of block.instructions) {
55+
const {lvalue, value} = instr;
56+
if (value.kind === 'LoadLocal') {
57+
locals.set(lvalue.identifier.id, value.place.identifier.id);
58+
} else if (value.kind === 'ArrayExpression') {
59+
candidateDependencies.set(lvalue.identifier.id, value);
60+
} else if (value.kind === 'FunctionExpression') {
61+
functions.set(lvalue.identifier.id, value);
62+
} else if (
63+
value.kind === 'CallExpression' ||
64+
value.kind === 'MethodCall'
65+
) {
66+
const callee =
67+
value.kind === 'CallExpression' ? value.callee : value.property;
68+
if (
69+
isUseEffectHookType(callee.identifier) &&
70+
value.args.length === 2 &&
71+
value.args[0].kind === 'Identifier' &&
72+
value.args[1].kind === 'Identifier'
73+
) {
74+
const effectFunction = functions.get(value.args[0].identifier.id);
75+
const deps = candidateDependencies.get(value.args[1].identifier.id);
76+
if (
77+
effectFunction != null &&
78+
deps != null &&
79+
deps.elements.length !== 0 &&
80+
deps.elements.every(element => element.kind === 'Identifier')
81+
) {
82+
const dependencies: Array<IdentifierId> = deps.elements.map(dep => {
83+
CompilerError.invariant(dep.kind === 'Identifier', {
84+
reason: `Dependency is checked as a place above`,
85+
loc: value.loc,
86+
});
87+
return locals.get(dep.identifier.id) ?? dep.identifier.id;
88+
});
89+
validateEffect(
90+
effectFunction.loweredFunc.func,
91+
dependencies,
92+
errors,
93+
);
94+
}
95+
}
96+
}
97+
}
98+
}
99+
if (errors.hasErrors()) {
100+
throw errors;
101+
}
102+
}
103+
104+
function validateEffect(
105+
effectFunction: HIRFunction,
106+
effectDeps: Array<IdentifierId>,
107+
errors: CompilerError,
108+
): void {
109+
for (const operand of effectFunction.context) {
110+
if (isSetStateType(operand.identifier)) {
111+
continue;
112+
} else if (effectDeps.find(dep => dep === operand.identifier.id) != null) {
113+
continue;
114+
} else {
115+
// Captured something other than the effect dep or setState
116+
return;
117+
}
118+
}
119+
for (const dep of effectDeps) {
120+
if (
121+
effectFunction.context.find(operand => operand.identifier.id === dep) ==
122+
null
123+
) {
124+
// effect dep wasn't actually used in the function
125+
return;
126+
}
127+
}
128+
129+
const seenBlocks: Set<BlockId> = new Set();
130+
const values: Map<IdentifierId, Array<IdentifierId>> = new Map();
131+
for (const dep of effectDeps) {
132+
values.set(dep, [dep]);
133+
}
134+
135+
const setStateLocations: Array<SourceLocation> = [];
136+
for (const block of effectFunction.body.blocks.values()) {
137+
for (const pred of block.preds) {
138+
if (!seenBlocks.has(pred)) {
139+
// skip if block has a back edge
140+
return;
141+
}
142+
}
143+
for (const phi of block.phis) {
144+
const aggregateDeps: Set<IdentifierId> = new Set();
145+
for (const operand of phi.operands.values()) {
146+
const deps = values.get(operand.identifier.id);
147+
if (deps != null) {
148+
for (const dep of deps) {
149+
aggregateDeps.add(dep);
150+
}
151+
}
152+
}
153+
if (aggregateDeps.size !== 0) {
154+
values.set(phi.place.identifier.id, Array.from(aggregateDeps));
155+
}
156+
}
157+
for (const instr of block.instructions) {
158+
switch (instr.value.kind) {
159+
case 'Primitive':
160+
case 'JSXText':
161+
case 'LoadGlobal': {
162+
break;
163+
}
164+
case 'LoadLocal': {
165+
const deps = values.get(instr.value.place.identifier.id);
166+
if (deps != null) {
167+
values.set(instr.lvalue.identifier.id, deps);
168+
}
169+
break;
170+
}
171+
case 'ComputedLoad':
172+
case 'PropertyLoad':
173+
case 'BinaryExpression':
174+
case 'TemplateLiteral':
175+
case 'CallExpression':
176+
case 'MethodCall': {
177+
const aggregateDeps: Set<IdentifierId> = new Set();
178+
for (const operand of eachInstructionValueOperand(instr.value)) {
179+
const deps = values.get(operand.identifier.id);
180+
if (deps != null) {
181+
for (const dep of deps) {
182+
aggregateDeps.add(dep);
183+
}
184+
}
185+
}
186+
if (aggregateDeps.size !== 0) {
187+
values.set(instr.lvalue.identifier.id, Array.from(aggregateDeps));
188+
}
189+
190+
if (
191+
instr.value.kind === 'CallExpression' &&
192+
isSetStateType(instr.value.callee.identifier) &&
193+
instr.value.args.length === 1 &&
194+
instr.value.args[0].kind === 'Identifier'
195+
) {
196+
const deps = values.get(instr.value.args[0].identifier.id);
197+
if (deps != null && new Set(deps).size === effectDeps.length) {
198+
setStateLocations.push(instr.value.callee.loc);
199+
} else {
200+
// doesn't depend on any deps
201+
return;
202+
}
203+
}
204+
break;
205+
}
206+
default: {
207+
return;
208+
}
209+
}
210+
}
211+
for (const operand of eachTerminalOperand(block.terminal)) {
212+
if (values.has(operand.identifier.id)) {
213+
//
214+
return;
215+
}
216+
}
217+
seenBlocks.add(block.id);
218+
}
219+
220+
for (const loc of setStateLocations) {
221+
errors.push({
222+
reason:
223+
'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)',
224+
description: null,
225+
severity: ErrorSeverity.InvalidReact,
226+
loc,
227+
suggestions: null,
228+
});
229+
}
230+
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validateNoDerivedComputationsInEffects
6+
function BadExample() {
7+
const [firstName, setFirstName] = useState('Taylor');
8+
const [lastName, setLastName] = useState('Swift');
9+
10+
// 🔴 Avoid: redundant state and unnecessary Effect
11+
const [fullName, setFullName] = useState('');
12+
useEffect(() => {
13+
setFullName(capitalize(firstName + ' ' + lastName));
14+
}, [firstName, lastName]);
15+
16+
return <div>{fullName}</div>;
17+
}
18+
19+
```
20+
21+
22+
## Error
23+
24+
```
25+
Found 1 error:
26+
27+
Error: 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)
28+
29+
error.invalid-derived-computation-in-effect.ts:9:4
30+
7 | const [fullName, setFullName] = useState('');
31+
8 | useEffect(() => {
32+
> 9 | setFullName(capitalize(firstName + ' ' + lastName));
33+
| ^^^^^^^^^^^ 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)
34+
10 | }, [firstName, lastName]);
35+
11 |
36+
12 | return <div>{fullName}</div>;
37+
```
38+
39+

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.js

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// @validateNoDerivedComputationsInEffects
2+
function BadExample() {
3+
const [firstName, setFirstName] = useState('Taylor');
4+
const [lastName, setLastName] = useState('Swift');
5+
6+
// 🔴 Avoid: redundant state and unnecessary Effect
7+
const [fullName, setFullName] = useState('');
8+
useEffect(() => {
9+
setFullName(capitalize(firstName + ' ' + lastName));
10+
}, [firstName, lastName]);
11+
12+
return <div>{fullName}</div>;
13+
}

0 commit comments

Comments
 (0)