mirror of
https://github.com/zebrajr/react.git
synced 2025-12-06 00:20:04 +01:00
[compiler] Cleanup and enable validateNoVoidUseMemo (#34882)
This is a great validation, so let's enable by default. Changes: * Move the validation logic into ValidateUseMemo alongside the new check that the useMemo result is used * Update the lint description * Make the void memo errors lint-only, they don't require us to skip compilation (as evidenced by the fact that we've had this validation off) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34882). * #34855 * __->__ #34882
This commit is contained in:
Joseph Savona
GitHub
parent
7f5ea1bf67
commit
1324e1bb1f
|
|
@ -988,7 +988,7 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule {
|
|||
severity: ErrorSeverity.Error,
|
||||
name: 'void-use-memo',
|
||||
description:
|
||||
'Validates that useMemos always return a value. See [`useMemo()` docs](https://react.dev/reference/react/useMemo) for more information.',
|
||||
'Validates that useMemos always return a value and that the result of the useMemo is used by the component/hook. See [`useMemo()` docs](https://react.dev/reference/react/useMemo) for more information.',
|
||||
preset: LintRulePreset.RecommendedLatest,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -659,7 +659,7 @@ export const EnvironmentConfigSchema = z.object({
|
|||
* Invalid:
|
||||
* useMemo(() => { ... }, [...]);
|
||||
*/
|
||||
validateNoVoidUseMemo: z.boolean().default(false),
|
||||
validateNoVoidUseMemo: z.boolean().default(true),
|
||||
|
||||
/**
|
||||
* Validates that Components/Hooks are always defined at module level. This prevents scope
|
||||
|
|
|
|||
|
|
@ -438,40 +438,6 @@ export function dropManualMemoization(
|
|||
continue;
|
||||
}
|
||||
|
||||
/**
|
||||
* Bailout on void return useMemos. This is an anti-pattern where code might be using
|
||||
* useMemo like useEffect: running arbirtary side-effects synced to changes in specific
|
||||
* values.
|
||||
*/
|
||||
if (
|
||||
func.env.config.validateNoVoidUseMemo &&
|
||||
manualMemo.kind === 'useMemo'
|
||||
) {
|
||||
const funcToCheck = sidemap.functions.get(
|
||||
fnPlace.identifier.id,
|
||||
)?.value;
|
||||
if (funcToCheck !== undefined && funcToCheck.loweredFunc.func) {
|
||||
if (!hasNonVoidReturn(funcToCheck.loweredFunc.func)) {
|
||||
errors.pushDiagnostic(
|
||||
CompilerDiagnostic.create({
|
||||
category: ErrorCategory.VoidUseMemo,
|
||||
reason: 'useMemo() callbacks must return a value',
|
||||
description: `This ${
|
||||
manualMemo.loadInstr.value.kind === 'PropertyLoad'
|
||||
? 'React.useMemo()'
|
||||
: 'useMemo()'
|
||||
} callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects`,
|
||||
suggestions: null,
|
||||
}).withDetails({
|
||||
kind: 'error',
|
||||
loc: instr.value.loc,
|
||||
message: 'useMemo() callbacks must return a value',
|
||||
}),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
instr.value = getManualMemoizationReplacement(
|
||||
fnPlace,
|
||||
instr.value.loc,
|
||||
|
|
@ -629,17 +595,3 @@ function findOptionalPlaces(fn: HIRFunction): Set<IdentifierId> {
|
|||
}
|
||||
return optionals;
|
||||
}
|
||||
|
||||
function hasNonVoidReturn(func: HIRFunction): boolean {
|
||||
for (const [, block] of func.body.blocks) {
|
||||
if (block.terminal.kind === 'return') {
|
||||
if (
|
||||
block.terminal.returnVariant === 'Explicit' ||
|
||||
block.terminal.returnVariant === 'Implicit'
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -24,6 +24,7 @@ import {Result} from '../Utils/Result';
|
|||
|
||||
export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
|
||||
const errors = new CompilerError();
|
||||
const voidMemoErrors = new CompilerError();
|
||||
const useMemos = new Set<IdentifierId>();
|
||||
const react = new Set<IdentifierId>();
|
||||
const functions = new Map<IdentifierId, FunctionExpression>();
|
||||
|
|
@ -125,7 +126,22 @@ export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
|
|||
validateNoContextVariableAssignment(body.loweredFunc.func, errors);
|
||||
|
||||
if (fn.env.config.validateNoVoidUseMemo) {
|
||||
unusedUseMemos.set(lvalue.identifier.id, callee.loc);
|
||||
if (!hasNonVoidReturn(body.loweredFunc.func)) {
|
||||
voidMemoErrors.pushDiagnostic(
|
||||
CompilerDiagnostic.create({
|
||||
category: ErrorCategory.VoidUseMemo,
|
||||
reason: 'useMemo() callbacks must return a value',
|
||||
description: `This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects`,
|
||||
suggestions: null,
|
||||
}).withDetails({
|
||||
kind: 'error',
|
||||
loc: body.loc,
|
||||
message: 'useMemo() callbacks must return a value',
|
||||
}),
|
||||
);
|
||||
} else {
|
||||
unusedUseMemos.set(lvalue.identifier.id, callee.loc);
|
||||
}
|
||||
}
|
||||
break;
|
||||
}
|
||||
|
|
@ -146,10 +162,10 @@ export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
|
|||
* Even a DCE-based version could be bypassed with `noop(useMemo(...))`.
|
||||
*/
|
||||
for (const loc of unusedUseMemos.values()) {
|
||||
errors.pushDiagnostic(
|
||||
voidMemoErrors.pushDiagnostic(
|
||||
CompilerDiagnostic.create({
|
||||
category: ErrorCategory.VoidUseMemo,
|
||||
reason: 'Unused useMemo()',
|
||||
reason: 'useMemo() result is unused',
|
||||
description: `This useMemo() value is unused. useMemo() is for computing and caching values, not for arbitrary side effects`,
|
||||
suggestions: null,
|
||||
}).withDetails({
|
||||
|
|
@ -160,6 +176,7 @@ export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
|
|||
);
|
||||
}
|
||||
}
|
||||
fn.env.logErrors(voidMemoErrors.asResult());
|
||||
return errors.asResult();
|
||||
}
|
||||
|
||||
|
|
@ -192,3 +209,17 @@ function validateNoContextVariableAssignment(
|
|||
}
|
||||
}
|
||||
}
|
||||
|
||||
function hasNonVoidReturn(func: HIRFunction): boolean {
|
||||
for (const [, block] of func.body.blocks) {
|
||||
if (block.terminal.kind === 'return') {
|
||||
if (
|
||||
block.terminal.returnVariant === 'Explicit' ||
|
||||
block.terminal.returnVariant === 'Implicit'
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,35 +0,0 @@
|
|||
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
// @validateNoVoidUseMemo
|
||||
function Component() {
|
||||
useMemo(() => {
|
||||
return [];
|
||||
}, []);
|
||||
return <div />;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
|
||||
## Error
|
||||
|
||||
```
|
||||
Found 1 error:
|
||||
|
||||
Error: Unused useMemo()
|
||||
|
||||
This useMemo() value is unused. useMemo() is for computing and caching values, not for arbitrary side effects.
|
||||
|
||||
error.invalid-unused-usememo.ts:3:2
|
||||
1 | // @validateNoVoidUseMemo
|
||||
2 | function Component() {
|
||||
> 3 | useMemo(() => {
|
||||
| ^^^^^^^ useMemo() result is unused
|
||||
4 | return [];
|
||||
5 | }, []);
|
||||
6 | return <div />;
|
||||
```
|
||||
|
||||
|
||||
|
|
@ -1,64 +0,0 @@
|
|||
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
// @validateNoVoidUseMemo
|
||||
function Component() {
|
||||
const value = useMemo(() => {
|
||||
console.log('computing');
|
||||
}, []);
|
||||
const value2 = React.useMemo(() => {
|
||||
console.log('computing');
|
||||
}, []);
|
||||
return (
|
||||
<div>
|
||||
{value}
|
||||
{value2}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
|
||||
## Error
|
||||
|
||||
```
|
||||
Found 2 errors:
|
||||
|
||||
Error: useMemo() callbacks must return a value
|
||||
|
||||
This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects.
|
||||
|
||||
error.useMemo-no-return-value.ts:3:16
|
||||
1 | // @validateNoVoidUseMemo
|
||||
2 | function Component() {
|
||||
> 3 | const value = useMemo(() => {
|
||||
| ^^^^^^^^^^^^^^^
|
||||
> 4 | console.log('computing');
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
> 5 | }, []);
|
||||
| ^^^^^^^^^ useMemo() callbacks must return a value
|
||||
6 | const value2 = React.useMemo(() => {
|
||||
7 | console.log('computing');
|
||||
8 | }, []);
|
||||
|
||||
Error: useMemo() callbacks must return a value
|
||||
|
||||
This React.useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects.
|
||||
|
||||
error.useMemo-no-return-value.ts:6:17
|
||||
4 | console.log('computing');
|
||||
5 | }, []);
|
||||
> 6 | const value2 = React.useMemo(() => {
|
||||
| ^^^^^^^^^^^^^^^^^^^^^
|
||||
> 7 | console.log('computing');
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
> 8 | }, []);
|
||||
| ^^^^^^^^^ useMemo() callbacks must return a value
|
||||
9 | return (
|
||||
10 | <div>
|
||||
11 | {value}
|
||||
```
|
||||
|
||||
|
||||
|
|
@ -0,0 +1,41 @@
|
|||
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
// @validateNoVoidUseMemo @loggerTestOnly
|
||||
function Component() {
|
||||
useMemo(() => {
|
||||
return [];
|
||||
}, []);
|
||||
return <div />;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Code
|
||||
|
||||
```javascript
|
||||
import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo @loggerTestOnly
|
||||
function Component() {
|
||||
const $ = _c(1);
|
||||
let t0;
|
||||
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
|
||||
t0 = <div />;
|
||||
$[0] = t0;
|
||||
} else {
|
||||
t0 = $[0];
|
||||
}
|
||||
return t0;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Logs
|
||||
|
||||
```
|
||||
{"kind":"CompileError","detail":{"options":{"category":"VoidUseMemo","reason":"useMemo() result is unused","description":"This useMemo() value is unused. useMemo() is for computing and caching values, not for arbitrary side effects","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":3,"column":2,"index":67},"end":{"line":3,"column":9,"index":74},"filename":"invalid-unused-usememo.ts","identifierName":"useMemo"},"message":"useMemo() result is unused"}]}},"fnLoc":null}
|
||||
{"kind":"CompileSuccess","fnLoc":{"start":{"line":2,"column":0,"index":42},"end":{"line":7,"column":1,"index":127},"filename":"invalid-unused-usememo.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0}
|
||||
```
|
||||
|
||||
### Eval output
|
||||
(kind: exception) Fixture not implemented
|
||||
|
|
@ -1,4 +1,4 @@
|
|||
// @validateNoVoidUseMemo
|
||||
// @validateNoVoidUseMemo @loggerTestOnly
|
||||
function Component() {
|
||||
useMemo(() => {
|
||||
return [];
|
||||
|
|
@ -0,0 +1,59 @@
|
|||
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
// @validateNoVoidUseMemo @loggerTestOnly
|
||||
function Component() {
|
||||
const value = useMemo(() => {
|
||||
console.log('computing');
|
||||
}, []);
|
||||
const value2 = React.useMemo(() => {
|
||||
console.log('computing');
|
||||
}, []);
|
||||
return (
|
||||
<div>
|
||||
{value}
|
||||
{value2}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Code
|
||||
|
||||
```javascript
|
||||
import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo @loggerTestOnly
|
||||
function Component() {
|
||||
const $ = _c(1);
|
||||
|
||||
console.log("computing");
|
||||
|
||||
console.log("computing");
|
||||
let t0;
|
||||
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
|
||||
t0 = (
|
||||
<div>
|
||||
{undefined}
|
||||
{undefined}
|
||||
</div>
|
||||
);
|
||||
$[0] = t0;
|
||||
} else {
|
||||
t0 = $[0];
|
||||
}
|
||||
return t0;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Logs
|
||||
|
||||
```
|
||||
{"kind":"CompileError","detail":{"options":{"category":"VoidUseMemo","reason":"useMemo() callbacks must return a value","description":"This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":3,"column":24,"index":89},"end":{"line":5,"column":3,"index":130},"filename":"invalid-useMemo-no-return-value.ts"},"message":"useMemo() callbacks must return a value"}]}},"fnLoc":null}
|
||||
{"kind":"CompileError","detail":{"options":{"category":"VoidUseMemo","reason":"useMemo() callbacks must return a value","description":"This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":6,"column":31,"index":168},"end":{"line":8,"column":3,"index":209},"filename":"invalid-useMemo-no-return-value.ts"},"message":"useMemo() callbacks must return a value"}]}},"fnLoc":null}
|
||||
{"kind":"CompileSuccess","fnLoc":{"start":{"line":2,"column":0,"index":42},"end":{"line":15,"column":1,"index":283},"filename":"invalid-useMemo-no-return-value.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0}
|
||||
```
|
||||
|
||||
### Eval output
|
||||
(kind: exception) Fixture not implemented
|
||||
|
|
@ -1,4 +1,4 @@
|
|||
// @validateNoVoidUseMemo
|
||||
// @validateNoVoidUseMemo @loggerTestOnly
|
||||
function Component() {
|
||||
const value = useMemo(() => {
|
||||
console.log('computing');
|
||||
|
|
@ -0,0 +1,33 @@
|
|||
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
// @loggerTestOnly
|
||||
function component(a) {
|
||||
let x = useMemo(() => {
|
||||
mutate(a);
|
||||
}, []);
|
||||
return x;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Code
|
||||
|
||||
```javascript
|
||||
// @loggerTestOnly
|
||||
function component(a) {
|
||||
mutate(a);
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Logs
|
||||
|
||||
```
|
||||
{"kind":"CompileError","detail":{"options":{"category":"VoidUseMemo","reason":"useMemo() callbacks must return a value","description":"This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":3,"column":18,"index":61},"end":{"line":5,"column":3,"index":87},"filename":"invalid-useMemo-return-empty.ts"},"message":"useMemo() callbacks must return a value"}]}},"fnLoc":null}
|
||||
{"kind":"CompileSuccess","fnLoc":{"start":{"line":2,"column":0,"index":19},"end":{"line":7,"column":1,"index":107},"filename":"invalid-useMemo-return-empty.ts"},"fnName":"component","memoSlots":0,"memoBlocks":0,"memoValues":0,"prunedMemoBlocks":1,"prunedMemoValues":0}
|
||||
```
|
||||
|
||||
### Eval output
|
||||
(kind: exception) Fixture not implemented
|
||||
|
|
@ -1,3 +1,4 @@
|
|||
// @loggerTestOnly
|
||||
function component(a) {
|
||||
let x = useMemo(() => {
|
||||
mutate(a);
|
||||
|
|
@ -2,6 +2,7 @@
|
|||
## Input
|
||||
|
||||
```javascript
|
||||
// @validateNoVoidUseMemo:false
|
||||
function Component(props) {
|
||||
const item = props.item;
|
||||
const thumbnails = [];
|
||||
|
|
@ -22,7 +23,7 @@ function Component(props) {
|
|||
## Code
|
||||
|
||||
```javascript
|
||||
import { c as _c } from "react/compiler-runtime";
|
||||
import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo:false
|
||||
function Component(props) {
|
||||
const $ = _c(6);
|
||||
const item = props.item;
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
// @validateNoVoidUseMemo:false
|
||||
function Component(props) {
|
||||
const item = props.item;
|
||||
const thumbnails = [];
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ function Component(props) {
|
|||
const x = useMemo(() => {
|
||||
if (props.cond) {
|
||||
if (props.cond) {
|
||||
return props.value;
|
||||
}
|
||||
}
|
||||
}, [props.cond]);
|
||||
|
|
@ -24,10 +25,18 @@ export const FIXTURE_ENTRYPOINT = {
|
|||
|
||||
```javascript
|
||||
function Component(props) {
|
||||
if (props.cond) {
|
||||
let t0;
|
||||
bb0: {
|
||||
if (props.cond) {
|
||||
if (props.cond) {
|
||||
t0 = props.value;
|
||||
break bb0;
|
||||
}
|
||||
}
|
||||
t0 = undefined;
|
||||
}
|
||||
const x = t0;
|
||||
return x;
|
||||
}
|
||||
|
||||
export const FIXTURE_ENTRYPOINT = {
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ function Component(props) {
|
|||
const x = useMemo(() => {
|
||||
if (props.cond) {
|
||||
if (props.cond) {
|
||||
return props.value;
|
||||
}
|
||||
}
|
||||
}, [props.cond]);
|
||||
|
|
|
|||
|
|
@ -1,22 +0,0 @@
|
|||
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
function component(a) {
|
||||
let x = useMemo(() => {
|
||||
mutate(a);
|
||||
}, []);
|
||||
return x;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Code
|
||||
|
||||
```javascript
|
||||
function component(a) {
|
||||
mutate(a);
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
|
|
@ -120,7 +120,15 @@ testRule('plugin-recommended', TestRecommendedRules, {
|
|||
|
||||
return <Child x={state} />;
|
||||
}`,
|
||||
errors: [makeTestCaseError('Unused useMemo()')],
|
||||
errors: [
|
||||
makeTestCaseError('useMemo() callbacks must return a value'),
|
||||
makeTestCaseError(
|
||||
'Calling setState from useMemo may trigger an infinite loop',
|
||||
),
|
||||
makeTestCaseError(
|
||||
'Calling setState from useMemo may trigger an infinite loop',
|
||||
),
|
||||
],
|
||||
},
|
||||
{
|
||||
name: 'Pipeline errors are reported',
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user