@samitouri / QOS-React / commits / c39da3e4de

[compiler] Off-by-default validation against setState directly in passive effect

Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render. This validation is off by default, i'm putting this up for discussion and to experiment with it internally. ghstack-source-id: 5f385ddab59561ec3939ae5ece265dfee4f2cb56 Pull Request resolved: https://github.com/facebook/react/pull/30685

Joe Savona committed Aug 13, 2024 at 22:38 UTC c39da3e4de0ad0db56ad99119efe3efc6c72abf1
11 files changed +403
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
+5
@@ -104,6 +104,7 @@ import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLoca
104 import {outlineFunctions} from '../Optimization/OutlineFunctions';
105 import {propagatePhiTypes} from '../TypeInference/PropagatePhiTypes';
106 import {lowerContextAccess} from '../Optimization/LowerContextAccess';
107 +import {validateNoSetStateInPassiveEffects} from '../Validation/ValidateNoSetStateInPassiveEffects';
108
109 export type CompilerPipelineValue =
110 | {kind: 'ast'; name: string; value: CodegenFunction}
@@ -244,6 +245,10 @@ function* runWithEnvironment(
245 validateNoSetStateInRender(hir);
246 }
247
248 + if (env.config.validateNoSetStateInPassiveEffects) {
249 + validateNoSetStateInPassiveEffects(hir);
250 + }
251 +
252 inferReactivePlaces(hir);
253 yield log({kind: 'hir', name: 'InferReactivePlaces', value: hir});
254
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
+6
@@ -231,6 +231,12 @@ const EnvironmentConfigSchema = z.object({
231 */
232 validateNoSetStateInRender: z.boolean().default(true),
233
234 + /**
235 + * Validates that setState is not called directly within a passive effect (useEffect).
236 + * Scheduling a setState (with an event listener, subscription, etc) is valid.
237 + */
238 + validateNoSetStateInPassiveEffects: z.boolean().default(false),
239 +
240 /**
241 * Validates that the dependencies of all effect hooks are memoized. This helps ensure
242 * that Forget does not introduce infinite renders caused by a dependency changing,
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts new
+152
@@ -0,0 +1,152 @@
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 + isSetStateType,
13 + isUseEffectHookType,
14 + Place,
15 +} from '../HIR';
16 +import {eachInstructionValueOperand} from '../HIR/visitors';
17 +
18 +/**
19 + * Validates against calling setState in the body of a *passive* effect (useEffect),
20 + * while allowing calling setState in callbacks scheduled by the effect.
21 + *
22 + * Calling setState during execution of a useEffect triggers a re-render, which is
23 + * often bad for performance and frequently has more efficient and straightforward
24 + * alternatives. See https://react.dev/learn/you-might-not-need-an-effect for examples.
25 + */
26 +export function validateNoSetStateInPassiveEffects(fn: HIRFunction): void {
27 + const setStateFunctions: Map<IdentifierId, Place> = new Map();
28 + const errors = new CompilerError();
29 + for (const [, block] of fn.body.blocks) {
30 + for (const instr of block.instructions) {
31 + switch (instr.value.kind) {
32 + case 'LoadLocal': {
33 + if (setStateFunctions.has(instr.value.place.identifier.id)) {
34 + setStateFunctions.set(
35 + instr.lvalue.identifier.id,
36 + instr.value.place,
37 + );
38 + }
39 + break;
40 + }
41 + case 'StoreLocal': {
42 + if (setStateFunctions.has(instr.value.value.identifier.id)) {
43 + setStateFunctions.set(
44 + instr.value.lvalue.place.identifier.id,
45 + instr.value.value,
46 + );
47 + setStateFunctions.set(
48 + instr.lvalue.identifier.id,
49 + instr.value.value,
50 + );
51 + }
52 + break;
53 + }
54 + case 'FunctionExpression': {
55 + if (
56 + // faster-path to check if the function expression references a setState
57 + [...eachInstructionValueOperand(instr.value)].some(
58 + operand =>
59 + isSetStateType(operand.identifier) ||
60 + setStateFunctions.has(operand.identifier.id),
61 + )
62 + ) {
63 + const callee = getSetStateCall(
64 + instr.value.loweredFunc.func,
65 + setStateFunctions,
66 + );
67 + if (callee !== null) {
68 + setStateFunctions.set(instr.lvalue.identifier.id, callee);
69 + }
70 + }
71 + break;
72 + }
73 + case 'MethodCall':
74 + case 'CallExpression': {
75 + const callee =
76 + instr.value.kind === 'MethodCall'
77 + ? instr.value.receiver
78 + : instr.value.callee;
79 + if (isUseEffectHookType(callee.identifier)) {
80 + const arg = instr.value.args[0];
81 + if (arg !== undefined && arg.kind === 'Identifier') {
82 + const setState = setStateFunctions.get(arg.identifier.id);
83 + if (setState !== undefined) {
84 + errors.push({
85 + reason:
86 + '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)',
87 + description: null,
88 + severity: ErrorSeverity.InvalidReact,
89 + loc: setState.loc,
90 + suggestions: null,
91 + });
92 + }
93 + }
94 + }
95 + break;
96 + }
97 + }
98 + }
99 + }
100 +
101 + if (errors.hasErrors()) {
102 + throw errors;
103 + }
104 +}
105 +
106 +function getSetStateCall(
107 + fn: HIRFunction,
108 + setStateFunctions: Map<IdentifierId, Place>,
109 +): Place | null {
110 + for (const [, block] of fn.body.blocks) {
111 + for (const instr of block.instructions) {
112 + switch (instr.value.kind) {
113 + case 'LoadLocal': {
114 + if (setStateFunctions.has(instr.value.place.identifier.id)) {
115 + setStateFunctions.set(
116 + instr.lvalue.identifier.id,
117 + instr.value.place,
118 + );
119 + }
120 + break;
121 + }
122 + case 'StoreLocal': {
123 + if (setStateFunctions.has(instr.value.value.identifier.id)) {
124 + setStateFunctions.set(
125 + instr.value.lvalue.place.identifier.id,
126 + instr.value.value,
127 + );
128 + setStateFunctions.set(
129 + instr.lvalue.identifier.id,
130 + instr.value.value,
131 + );
132 + }
133 + break;
134 + }
135 + case 'CallExpression': {
136 + const callee = instr.value.callee;
137 + if (
138 + isSetStateType(callee.identifier) ||
139 + setStateFunctions.has(callee.identifier.id)
140 + ) {
141 + /*
142 + * TODO: once we support multiple locations per error, we should link to the
143 + * original Place in the case that setStateFunction.has(callee)
144 + */
145 + return callee;
146 + }
147 + }
148 + }
149 + }
150 + }
151 + return null;
152 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.expect.md new
+37
@@ -0,0 +1,37 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateNoSetStateInPassiveEffects
6 +import {useEffect, useState} from 'react';
7 +
8 +function Component() {
9 + const [state, setState] = useState(0);
10 + const f = () => {
11 + setState(s => s + 1);
12 + };
13 + const g = () => {
14 + f();
15 + };
16 + useEffect(() => {
17 + g();
18 + });
19 + return state;
20 +}
21 +
22 +```
23 +
24 +
25 +## Error
26 +
27 +```
28 + 11 | };
29 + 12 | useEffect(() => {
30 +> 13 | g();
31 + | ^ InvalidReact: 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) (13:13)
32 + 14 | });
33 + 15 | return state;
34 + 16 | }
35 +```
36 +
37 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.js new
+16
@@ -0,0 +1,16 @@
1 +// @validateNoSetStateInPassiveEffects
2 +import {useEffect, useState} from 'react';
3 +
4 +function Component() {
5 + const [state, setState] = useState(0);
6 + const f = () => {
7 + setState(s => s + 1);
8 + };
9 + const g = () => {
10 + f();
11 + };
12 + useEffect(() => {
13 + g();
14 + });
15 + return state;
16 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.expect.md new
+31
@@ -0,0 +1,31 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateNoSetStateInPassiveEffects
6 +import {useEffect, useState} from 'react';
7 +
8 +function Component() {
9 + const [state, setState] = useState(0);
10 + useEffect(() => {
11 + setState(s => s + 1);
12 + });
13 + return state;
14 +}
15 +
16 +```
17 +
18 +
19 +## Error
20 +
21 +```
22 + 5 | const [state, setState] = useState(0);
23 + 6 | useEffect(() => {
24 +> 7 | setState(s => s + 1);
25 + | ^^^^^^^^ InvalidReact: 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) (7:7)
26 + 8 | });
27 + 9 | return state;
28 + 10 | }
29 +```
30 +
31 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.js new
+10
@@ -0,0 +1,10 @@
1 +// @validateNoSetStateInPassiveEffects
2 +import {useEffect, useState} from 'react';
3 +
4 +function Component() {
5 + const [state, setState] = useState(0);
6 + useEffect(() => {
7 + setState(s => s + 1);
8 + });
9 + return state;
10 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.expect.md new
+60
@@ -0,0 +1,60 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateNoSetStateInPassiveEffects
6 +import {useEffect, useState} from 'react';
7 +
8 +function Component() {
9 + const [state, setState] = useState(0);
10 + useEffect(() => {
11 + const f = () => {
12 + setState();
13 + };
14 + setTimeout(() => f(), 10);
15 + });
16 + return state;
17 +}
18 +
19 +export const FIXTURE_ENTRYPOINT = {
20 + fn: Component,
21 + params: [{}],
22 +};
23 +
24 +```
25 +
26 +## Code
27 +
28 +```javascript
29 +import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInPassiveEffects
30 +import { useEffect, useState } from "react";
31 +
32 +function Component() {
33 + const $ = _c(1);
34 + const [state, setState] = useState(0);
35 + let t0;
36 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
37 + t0 = () => {
38 + const f = () => {
39 + setState();
40 + };
41 +
42 + setTimeout(() => f(), 10);
43 + };
44 + $[0] = t0;
45 + } else {
46 + t0 = $[0];
47 + }
48 + useEffect(t0);
49 + return state;
50 +}
51 +
52 +export const FIXTURE_ENTRYPOINT = {
53 + fn: Component,
54 + params: [{}],
55 +};
56 +
57 +```
58 +
59 +### Eval output
60 +(kind: ok) 0
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.js new
+18
@@ -0,0 +1,18 @@
1 +// @validateNoSetStateInPassiveEffects
2 +import {useEffect, useState} from 'react';
3 +
4 +function Component() {
5 + const [state, setState] = useState(0);
6 + useEffect(() => {
7 + const f = () => {
8 + setState();
9 + };
10 + setTimeout(() => f(), 10);
11 + });
12 + return state;
13 +}
14 +
15 +export const FIXTURE_ENTRYPOINT = {
16 + fn: Component,
17 + params: [{}],
18 +};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.expect.md new
+53
@@ -0,0 +1,53 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateNoSetStateInPassiveEffects
6 +import {useEffect, useState} from 'react';
7 +
8 +function Component() {
9 + const [state, setState] = useState(0);
10 + useEffect(() => {
11 + setTimeout(setState, 10);
12 + });
13 + return state;
14 +}
15 +
16 +export const FIXTURE_ENTRYPOINT = {
17 + fn: Component,
18 + params: [{}],
19 +};
20 +
21 +```
22 +
23 +## Code
24 +
25 +```javascript
26 +import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInPassiveEffects
27 +import { useEffect, useState } from "react";
28 +
29 +function Component() {
30 + const $ = _c(1);
31 + const [state, setState] = useState(0);
32 + let t0;
33 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
34 + t0 = () => {
35 + setTimeout(setState, 10);
36 + };
37 + $[0] = t0;
38 + } else {
39 + t0 = $[0];
40 + }
41 + useEffect(t0);
42 + return state;
43 +}
44 +
45 +export const FIXTURE_ENTRYPOINT = {
46 + fn: Component,
47 + params: [{}],
48 +};
49 +
50 +```
51 +
52 +### Eval output
53 +(kind: ok) 0
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.js new
+15
@@ -0,0 +1,15 @@
1 +// @validateNoSetStateInPassiveEffects
2 +import {useEffect, useState} from 'react';
3 +
4 +function Component() {
5 + const [state, setState] = useState(0);
6 + useEffect(() => {
7 + setTimeout(setState, 10);
8 + });
9 + return state;
10 +}
11 +
12 +export const FIXTURE_ENTRYPOINT = {
13 + fn: Component,
14 + params: [{}],
15 +};