Skip to content

Commit c60eebf

Browse files
potetopoteto
authored
[compiler] Add new ValidateNoVoidUseMemo pass (facebook#33990)
Adds a new validation pass to validate against `useMemo`s that don't return anything. This usually indicates some kind of "useEffect"-like code that has side effects that need to be memoized to prevent overfiring, and is an anti-pattern. A follow up validation could also look at the return value of `useMemo`s to see if they are being used. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33990). * facebook#34022 * facebook#34002 * facebook#34001 * __->__ facebook#33990 * facebook#33989
1 parent 5dd622e commit c60eebf

14 files changed

+416
-0
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ import {
8282
import {inferTypes} from '../TypeInference';
8383
import {
8484
validateContextVariableLValues,
85+
validateNoVoidUseMemo,
8586
validateHooksUsage,
8687
validateMemoizedEffectDependencies,
8788
validateNoCapitalizedCalls,
@@ -167,6 +168,9 @@ function runWithEnvironment(
167168

168169
validateContextVariableLValues(hir);
169170
validateUseMemo(hir).unwrap();
171+
if (env.config.validateNoVoidUseMemo) {
172+
validateNoVoidUseMemo(hir).unwrap();
173+
}
170174

171175
if (
172176
env.isInferredMemoEnabled &&

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -631,6 +631,17 @@ export const EnvironmentConfigSchema = z.object({
631631
* ```
632632
*/
633633
lowerContextAccess: ExternalFunctionSchema.nullable().default(null),
634+
635+
/**
636+
* If enabled, will validate useMemos that don't return any values:
637+
*
638+
* Valid:
639+
* useMemo(() => foo, [foo]);
640+
* useMemo(() => { return foo }, [foo]);
641+
* Invalid:
642+
* useMemo(() => { ... }, [...]);
643+
*/
644+
validateNoVoidUseMemo: z.boolean().default(false),
634645
});
635646

636647
export type EnvironmentConfig = z.infer<typeof EnvironmentConfigSchema>;

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

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
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} from '../CompilerError';
9+
import {
10+
HIRFunction,
11+
IdentifierId,
12+
FunctionExpression,
13+
SourceLocation,
14+
Environment,
15+
Instruction,
16+
getHookKindForType,
17+
} from '../HIR';
18+
import {Result} from '../Utils/Result';
19+
20+
type TemporariesSidemap = {
21+
useMemoHooks: Map<IdentifierId, {name: string; loc: SourceLocation}>;
22+
funcExprs: Map<IdentifierId, FunctionExpression>;
23+
react: Set<IdentifierId>;
24+
};
25+
26+
/**
27+
* Validates that useMemo has at least one explicit return statement.
28+
*
29+
* Valid cases:
30+
* - useMemo(() => value) // implicit arrow function return
31+
* - useMemo(() => { return value; }) // explicit return
32+
* - useMemo(() => { return; }) // explicit undefined
33+
* - useMemo(() => { if (cond) return val; }) // at least one return
34+
*
35+
* Invalid cases:
36+
* - useMemo(() => { console.log(); }) // no return statement at all
37+
*/
38+
export function validateNoVoidUseMemo(
39+
fn: HIRFunction,
40+
): Result<void, CompilerError> {
41+
const errors = new CompilerError();
42+
const sidemap: TemporariesSidemap = {
43+
useMemoHooks: new Map(),
44+
funcExprs: new Map(),
45+
react: new Set(),
46+
};
47+
48+
for (const [, block] of fn.body.blocks) {
49+
for (const instr of block.instructions) {
50+
collectTemporaries(instr, fn.env, sidemap);
51+
}
52+
}
53+
54+
for (const [, block] of fn.body.blocks) {
55+
for (const instr of block.instructions) {
56+
if (instr.value.kind === 'CallExpression') {
57+
const callee = instr.value.callee.identifier;
58+
const useMemoHook = sidemap.useMemoHooks.get(callee.id);
59+
60+
if (useMemoHook !== undefined && instr.value.args.length > 0) {
61+
const firstArg = instr.value.args[0];
62+
if (firstArg.kind !== 'Identifier') {
63+
continue;
64+
}
65+
66+
let funcToCheck = sidemap.funcExprs.get(firstArg.identifier.id);
67+
68+
if (!funcToCheck) {
69+
for (const [, searchBlock] of fn.body.blocks) {
70+
for (const searchInstr of searchBlock.instructions) {
71+
if (
72+
searchInstr.lvalue &&
73+
searchInstr.lvalue.identifier.id === firstArg.identifier.id &&
74+
searchInstr.value.kind === 'FunctionExpression'
75+
) {
76+
funcToCheck = searchInstr.value;
77+
break;
78+
}
79+
}
80+
if (funcToCheck) break;
81+
}
82+
}
83+
84+
if (funcToCheck) {
85+
const hasReturn = checkFunctionHasNonVoidReturn(
86+
funcToCheck.loweredFunc.func,
87+
);
88+
89+
if (!hasReturn) {
90+
errors.push({
91+
severity: ErrorSeverity.InvalidReact,
92+
reason: `React Compiler has skipped optimizing this component because ${useMemoHook.name} doesn't return a value. ${useMemoHook.name} should only be used for memoizing values, not running arbitrary side effects.`,
93+
loc: useMemoHook.loc,
94+
suggestions: null,
95+
description: null,
96+
});
97+
}
98+
}
99+
}
100+
}
101+
}
102+
}
103+
return errors.asResult();
104+
}
105+
106+
function checkFunctionHasNonVoidReturn(func: HIRFunction): boolean {
107+
for (const [, block] of func.body.blocks) {
108+
if (block.terminal.kind === 'return') {
109+
if (
110+
block.terminal.returnVariant === 'Explicit' ||
111+
block.terminal.returnVariant === 'Implicit'
112+
) {
113+
return true;
114+
}
115+
}
116+
}
117+
return false;
118+
}
119+
120+
function collectTemporaries(
121+
instr: Instruction,
122+
env: Environment,
123+
sidemap: TemporariesSidemap,
124+
): void {
125+
const {value, lvalue} = instr;
126+
switch (value.kind) {
127+
case 'FunctionExpression': {
128+
sidemap.funcExprs.set(lvalue.identifier.id, value);
129+
break;
130+
}
131+
case 'LoadGlobal': {
132+
const global = env.getGlobalDeclaration(value.binding, value.loc);
133+
const hookKind = global !== null ? getHookKindForType(env, global) : null;
134+
if (hookKind === 'useMemo') {
135+
sidemap.useMemoHooks.set(lvalue.identifier.id, {
136+
name: value.binding.name,
137+
loc: instr.loc,
138+
});
139+
} else if (value.binding.name === 'React') {
140+
sidemap.react.add(lvalue.identifier.id);
141+
}
142+
break;
143+
}
144+
case 'PropertyLoad': {
145+
if (sidemap.react.has(value.object.identifier.id)) {
146+
if (value.property === 'useMemo') {
147+
sidemap.useMemoHooks.set(lvalue.identifier.id, {
148+
name: value.property,
149+
loc: instr.loc,
150+
});
151+
}
152+
}
153+
break;
154+
}
155+
}
156+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
*/
77

88
export {validateContextVariableLValues} from './ValidateContextVariableLValues';
9+
export {validateNoVoidUseMemo} from './ValidateNoVoidUseMemo';
910
export {validateHooksUsage} from './ValidateHooksUsage';
1011
export {validateMemoizedEffectDependencies} from './ValidateMemoizedEffectDependencies';
1112
export {validateNoCapitalizedCalls} from './ValidateNoCapitalizedCalls';

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.expect.md‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validateNoVoidUseMemo
6+
function Component() {
7+
const value = useMemo(() => {
8+
console.log('computing');
9+
}, []);
10+
const value2 = React.useMemo(() => {
11+
console.log('computing');
12+
}, []);
13+
return (
14+
<div>
15+
{value}
16+
{value2}
17+
</div>
18+
);
19+
}
20+
21+
```
22+
23+
24+
## Error
25+
26+
```
27+
Found 1 error:
28+
29+
Error: React Compiler has skipped optimizing this component because useMemo doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects.
30+
31+
error.useMemo-no-return-value.ts:3:16
32+
1 | // @validateNoVoidUseMemo
33+
2 | function Component() {
34+
> 3 | const value = useMemo(() => {
35+
| ^^^^^^^ React Compiler has skipped optimizing this component because useMemo doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects.
36+
4 | console.log('computing');
37+
5 | }, []);
38+
6 | const value2 = React.useMemo(() => {
39+
```
40+
41+

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// @validateNoVoidUseMemo
2+
function Component() {
3+
const value = useMemo(() => {
4+
console.log('computing');
5+
}, []);
6+
const value2 = React.useMemo(() => {
7+
console.log('computing');
8+
}, []);
9+
return (
10+
<div>
11+
{value}
12+
{value2}
13+
</div>
14+
);
15+
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-arrow-implicit-return.expect.md‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validateNoVoidUseMemo
6+
function Component() {
7+
const value = useMemo(() => computeValue(), []);
8+
return <div>{value}</div>;
9+
}
10+
11+
```
12+
13+
## Code
14+
15+
```javascript
16+
import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo
17+
function Component() {
18+
const $ = _c(2);
19+
let t0;
20+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
21+
t0 = computeValue();
22+
$[0] = t0;
23+
} else {
24+
t0 = $[0];
25+
}
26+
const value = t0;
27+
let t1;
28+
if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
29+
t1 = <div>{value}</div>;
30+
$[1] = t1;
31+
} else {
32+
t1 = $[1];
33+
}
34+
return t1;
35+
}
36+
37+
```
38+
39+
### Eval output
40+
(kind: exception) Fixture not implemented

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-arrow-implicit-return.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
// @validateNoVoidUseMemo
2+
function Component() {
3+
const value = useMemo(() => computeValue(), []);
4+
return <div>{value}</div>;
5+
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-empty-return.expect.md‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validateNoVoidUseMemo
6+
function Component() {
7+
const value = useMemo(() => {
8+
return;
9+
}, []);
10+
return <div>{value}</div>;
11+
}
12+
13+
```
14+
15+
## Code
16+
17+
```javascript
18+
import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo
19+
function Component() {
20+
const $ = _c(1);
21+
let t0;
22+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
23+
t0 = <div>{undefined}</div>;
24+
$[0] = t0;
25+
} else {
26+
t0 = $[0];
27+
}
28+
return t0;
29+
}
30+
31+
```
32+
33+
### Eval output
34+
(kind: exception) Fixture not implemented

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-empty-return.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// @validateNoVoidUseMemo
2+
function Component() {
3+
const value = useMemo(() => {
4+
return;
5+
}, []);
6+
return <div>{value}</div>;
7+
}

0 commit comments

Comments
 (0)