Skip to content

Commit 6f4294a

Browse files
authored
[compiler] Validate against setState in all effect types (#33753)
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33753). * #33981 * #33777 * #33767 * #33765 * #33760 * #33759 * #33758 * #33751 * #33752 * __->__ #33753
1 parent 448f781 commit 6f4294a

12 files changed

+33
-27
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLoca
9494
import {outlineFunctions} from '../Optimization/OutlineFunctions';
9595
import {propagatePhiTypes} from '../TypeInference/PropagatePhiTypes';
9696
import {lowerContextAccess} from '../Optimization/LowerContextAccess';
97-
import {validateNoSetStateInPassiveEffects} from '../Validation/ValidateNoSetStateInPassiveEffects';
97+
import {validateNoSetStateInEffects} from '../Validation/ValidateNoSetStateInEffects';
9898
import {validateNoJSXInTryStatement} from '../Validation/ValidateNoJSXInTryStatement';
9999
import {propagateScopeDependenciesHIR} from '../HIR/PropagateScopeDependenciesHIR';
100100
import {outlineJSX} from '../Optimization/OutlineJsx';
@@ -292,8 +292,8 @@ function runWithEnvironment(
292292
validateNoSetStateInRender(hir).unwrap();
293293
}
294294

295-
if (env.config.validateNoSetStateInPassiveEffects) {
296-
env.logErrors(validateNoSetStateInPassiveEffects(hir));
295+
if (env.config.validateNoSetStateInEffects) {
296+
env.logErrors(validateNoSetStateInEffects(hir));
297297
}
298298

299299
if (env.config.validateNoJSXInTryStatements) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -318,10 +318,10 @@ export const EnvironmentConfigSchema = z.object({
318318
validateNoSetStateInRender: z.boolean().default(true),
319319

320320
/**
321-
* Validates that setState is not called directly within a passive effect (useEffect).
321+
* Validates that setState is not called synchronously within an effect (useEffect and friends).
322322
* Scheduling a setState (with an event listener, subscription, etc) is valid.
323323
*/
324-
validateNoSetStateInPassiveEffects: z.boolean().default(false),
324+
validateNoSetStateInEffects: z.boolean().default(false),
325325

326326
/**
327327
* Validates against creating JSX within a try block and recommends using an error boundary
Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,20 +11,22 @@ import {
1111
IdentifierId,
1212
isSetStateType,
1313
isUseEffectHookType,
14+
isUseInsertionEffectHookType,
15+
isUseLayoutEffectHookType,
1416
Place,
1517
} from '../HIR';
1618
import {eachInstructionValueOperand} from '../HIR/visitors';
1719
import {Result} from '../Utils/Result';
1820

1921
/**
20-
* Validates against calling setState in the body of a *passive* effect (useEffect),
22+
* Validates against calling setState in the body of an effect (useEffect and friends),
2123
* while allowing calling setState in callbacks scheduled by the effect.
2224
*
2325
* Calling setState during execution of a useEffect triggers a re-render, which is
2426
* often bad for performance and frequently has more efficient and straightforward
2527
* alternatives. See https://react.dev/learn/you-might-not-need-an-effect for examples.
2628
*/
27-
export function validateNoSetStateInPassiveEffects(
29+
export function validateNoSetStateInEffects(
2830
fn: HIRFunction,
2931
): Result<void, CompilerError> {
3032
const setStateFunctions: Map<IdentifierId, Place> = new Map();
@@ -79,7 +81,11 @@ export function validateNoSetStateInPassiveEffects(
7981
instr.value.kind === 'MethodCall'
8082
? instr.value.receiver
8183
: instr.value.callee;
82-
if (isUseEffectHookType(callee.identifier)) {
84+
if (
85+
isUseEffectHookType(callee.identifier) ||
86+
isUseLayoutEffectHookType(callee.identifier) ||
87+
isUseInsertionEffectHookType(callee.identifier)
88+
) {
8389
const arg = instr.value.args[0];
8490
if (arg !== undefined && arg.kind === 'Identifier') {
8591
const setState = setStateFunctions.get(arg.identifier.id);

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
## Input
33

44
```javascript
5-
// @loggerTestOnly @validateNoSetStateInPassiveEffects
5+
// @loggerTestOnly @validateNoSetStateInEffects
66
import {useEffect, useState} from 'react';
77

88
function Component() {
@@ -24,7 +24,7 @@ function Component() {
2424
## Code
2525

2626
```javascript
27-
import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateNoSetStateInPassiveEffects
27+
import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateNoSetStateInEffects
2828
import { useEffect, useState } from "react";
2929

3030
function Component() {
@@ -65,8 +65,8 @@ function _temp(s) {
6565
## Logs
6666

6767
```
68-
{"kind":"CompileError","detail":{"options":{"reason":"Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","description":null,"severity":"InvalidReact","suggestions":null,"loc":{"start":{"line":13,"column":4,"index":272},"end":{"line":13,"column":5,"index":273},"filename":"invalid-setState-in-useEffect-transitive.ts","identifierName":"g"}}},"fnLoc":null}
69-
{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":99},"end":{"line":16,"column":1,"index":300},"filename":"invalid-setState-in-useEffect-transitive.ts"},"fnName":"Component","memoSlots":2,"memoBlocks":2,"memoValues":2,"prunedMemoBlocks":0,"prunedMemoValues":0}
68+
{"kind":"CompileError","detail":{"options":{"reason":"Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","description":null,"severity":"InvalidReact","suggestions":null,"loc":{"start":{"line":13,"column":4,"index":265},"end":{"line":13,"column":5,"index":266},"filename":"invalid-setState-in-useEffect-transitive.ts","identifierName":"g"}}},"fnLoc":null}
69+
{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":92},"end":{"line":16,"column":1,"index":293},"filename":"invalid-setState-in-useEffect-transitive.ts"},"fnName":"Component","memoSlots":2,"memoBlocks":2,"memoValues":2,"prunedMemoBlocks":0,"prunedMemoValues":0}
7070
```
7171
7272
### Eval output

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// @loggerTestOnly @validateNoSetStateInPassiveEffects
1+
// @loggerTestOnly @validateNoSetStateInEffects
22
import {useEffect, useState} from 'react';
33

44
function Component() {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
## Input
33

44
```javascript
5-
// @loggerTestOnly @validateNoSetStateInPassiveEffects
5+
// @loggerTestOnly @validateNoSetStateInEffects
66
import {useEffect, useState} from 'react';
77

88
function Component() {
@@ -18,7 +18,7 @@ function Component() {
1818
## Code
1919

2020
```javascript
21-
import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateNoSetStateInPassiveEffects
21+
import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateNoSetStateInEffects
2222
import { useEffect, useState } from "react";
2323

2424
function Component() {
@@ -45,8 +45,8 @@ function _temp(s) {
4545
## Logs
4646

4747
```
48-
{"kind":"CompileError","detail":{"options":{"reason":"Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","description":null,"severity":"InvalidReact","suggestions":null,"loc":{"start":{"line":7,"column":4,"index":187},"end":{"line":7,"column":12,"index":195},"filename":"invalid-setState-in-useEffect.ts","identifierName":"setState"}}},"fnLoc":null}
49-
{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":99},"end":{"line":10,"column":1,"index":232},"filename":"invalid-setState-in-useEffect.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0}
48+
{"kind":"CompileError","detail":{"options":{"reason":"Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","description":null,"severity":"InvalidReact","suggestions":null,"loc":{"start":{"line":7,"column":4,"index":180},"end":{"line":7,"column":12,"index":188},"filename":"invalid-setState-in-useEffect.ts","identifierName":"setState"}}},"fnLoc":null}
49+
{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":92},"end":{"line":10,"column":1,"index":225},"filename":"invalid-setState-in-useEffect.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0}
5050
```
5151
5252
### Eval output

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// @loggerTestOnly @validateNoSetStateInPassiveEffects
1+
// @loggerTestOnly @validateNoSetStateInEffects
22
import {useEffect, useState} from 'react';
33

44
function Component() {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.expect.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
## Input
33

44
```javascript
5-
// @validateNoSetStateInPassiveEffects
5+
// @validateNoSetStateInEffects
66
import {useEffect, useState} from 'react';
77

88
function Component() {
@@ -26,7 +26,7 @@ export const FIXTURE_ENTRYPOINT = {
2626
## Code
2727

2828
```javascript
29-
import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInPassiveEffects
29+
import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInEffects
3030
import { useEffect, useState } from "react";
3131

3232
function Component() {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// @validateNoSetStateInPassiveEffects
1+
// @validateNoSetStateInEffects
22
import {useEffect, useState} from 'react';
33

44
function Component() {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.expect.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
## Input
33

44
```javascript
5-
// @validateNoSetStateInPassiveEffects
5+
// @validateNoSetStateInEffects
66
import {useEffect, useState} from 'react';
77

88
function Component() {
@@ -23,7 +23,7 @@ export const FIXTURE_ENTRYPOINT = {
2323
## Code
2424

2525
```javascript
26-
import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInPassiveEffects
26+
import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInEffects
2727
import { useEffect, useState } from "react";
2828

2929
function Component() {

0 commit comments

Comments
 (0)