@samitouri / QOS-React-1 / commits / e6c5c9a005

Improve NoSetStateInRender function expression check

Updates the approach used in ValidateNoSetStateInRender to detect function expressions called during render. We now do the following: * Track function expression which are known to unconditionally call setState themselves- if these functions get called, that’s equivalent to calling setState. We call the validation recursively to compute this. * Track LoadLocal/StoreLocal indirections for such function expressions. * Check CallExpressions where the callee is either a known SetState (via type info) _or_ (new) where the callee is in the set of known-to-setState function expressions. The Set is shared throughout the analysis, so we can even find multiple levels of indirection (see new test case).

Joe Savona committed Nov 13, 2023 at 11:28 UTC e6c5c9a0053a466b3049e3bd4513b92b5c18e1c5
13 files changed +347 -78
compiler/packages/babel-plugin-react-forget/src/Utils/logger.ts
+1 -1
@@ -22,7 +22,7 @@ export function toggleLogging(enabled: boolean): void {
22
23 export function logDebug(step: string, value: string): void {
24 if (ENABLED) {
25 - process.stdout.write(`${chalk.gray(step)}:\n${value}\n\n`);
25 + process.stdout.write(`${chalk.green(step)}:\n${value}\n\n`);
26 }
27 }
28
compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoSetStateInRender.ts
+89 -15
@@ -9,6 +9,7 @@ import { CompilerError, ErrorSeverity } from "../CompilerError";
9 import {
10 BlockId,
11 HIRFunction,
12 + IdentifierId,
13 Place,
14 computePostDominatorTree,
15 isSetStateType,
@@ -18,8 +19,44 @@ import { eachInstructionValueOperand } from "../HIR/visitors";
19 import { findBlocksWithBackEdges } from "../Optimization/DeadCodeElimination";
20 import { Err, Ok, Result } from "../Utils/Result";
21
22 +/**
23 + * Validates that the given function does not have an infinite update loop
24 + * caused by unconditionally calling setState during render. This validation
25 + * is conservative and cannot catch all cases of unconditional setState in
26 + * render, but avoids false positives. Examples of cases that are caught:
27 + *
28 + * ```javascript
29 + * // Direct call of setState:
30 + * const [state, setState] = useState(false);
31 + * setState(true);
32 + *
33 + * // Indirect via a function:
34 + * const [state, setState] = useState(false);
35 + * const setTrue = () => setState(true);
36 + * setTrue();
37 + * ```
38 + *
39 + * However, storing setState inside another value and accessing it is not yet
40 + * validated:
41 + *
42 + * ```
43 + * // false negative, not detected but will cause an infinite render loop
44 + * const [state, setState] = useState(false);
45 + * const x = [setState];
46 + * const y = x.pop();
47 + * y();
48 + * ```
49 + */
50 export function validateNoSetStateInRender(
51 fn: HIRFunction
52 +): Result<PostDominator<BlockId>, CompilerError> {
53 + const unconditionalSetStateFunctions: Set<IdentifierId> = new Set();
54 + return validateNoSetStateInRenderImpl(fn, unconditionalSetStateFunctions);
55 +}
56 +
57 +function validateNoSetStateInRenderImpl(
58 + fn: HIRFunction,
59 + unconditionalSetStateFunctions: Set<IdentifierId>
60 ): Result<PostDominator<BlockId>, CompilerError> {
61 // Construct the set of blocks that is always reachable from the entry block.
62 const unconditionalBlocks = new Set<BlockId>();
@@ -43,27 +80,57 @@ export function validateNoSetStateInRender(
80 if (unconditionalBlocks.has(block.id)) {
81 for (const instr of block.instructions) {
82 switch (instr.value.kind) {
83 + case "LoadLocal": {
84 + if (
85 + unconditionalSetStateFunctions.has(
86 + instr.value.place.identifier.id
87 + )
88 + ) {
89 + unconditionalSetStateFunctions.add(instr.lvalue.identifier.id);
90 + }
91 + break;
92 + }
93 + case "StoreLocal": {
94 + if (
95 + unconditionalSetStateFunctions.has(
96 + instr.value.value.identifier.id
97 + )
98 + ) {
99 + unconditionalSetStateFunctions.add(
100 + instr.value.lvalue.place.identifier.id
101 + );
102 + unconditionalSetStateFunctions.add(instr.lvalue.identifier.id);
103 + }
104 + break;
105 + }
106 case "ObjectMethod":
107 case "FunctionExpression": {
108 if (fn.env.config.validateNoSetStateInRenderFunctionExpressions) {
49 - /*
50 - * TODO: setState's return value is considered Frozen, so the lambda's mutable range
51 - * does not get extended even if the lambda is called in render. The below only catches
52 - * setStates where the lambda has another instruction that extends its mutable range
53 - */
54 - const mutableRange = instr.lvalue.identifier.mutableRange;
55 - if (mutableRange.end > mutableRange.start + 1) {
56 - for (const operand of eachInstructionValueOperand(
57 - instr.value
58 - )) {
59 - validateNonSetState(errors, operand);
60 - }
109 + if (
110 + // faster-path to check if the function expression references a setState
111 + [...eachInstructionValueOperand(instr.value)].some(
112 + (operand) =>
113 + isSetStateType(operand.identifier) ||
114 + unconditionalSetStateFunctions.has(operand.identifier.id)
115 + ) &&
116 + // if yes, does it unconditonally call it?
117 + validateNoSetStateInRenderImpl(
118 + instr.value.loweredFunc.func,
119 + unconditionalSetStateFunctions
120 + ).isErr()
121 + ) {
122 + // This function expression unconditionally calls a setState
123 + unconditionalSetStateFunctions.add(instr.lvalue.identifier.id);
124 }
125 }
126 break;
127 }
128 case "CallExpression": {
66 - validateNonSetState(errors, instr.value.callee);
129 + validateNonSetState(
130 + errors,
131 + unconditionalSetStateFunctions,
132 + instr.value.callee
133 + );
134 break;
135 }
136 }
@@ -78,8 +145,15 @@ export function validateNoSetStateInRender(
145 }
146 }
147
81 -function validateNonSetState(errors: CompilerError, operand: Place): void {
82 - if (isSetStateType(operand.identifier)) {
148 +function validateNonSetState(
149 + errors: CompilerError,
150 + unconditionalSetStateFunctions: Set<IdentifierId>,
151 + operand: Place
152 +): void {
153 + if (
154 + isSetStateType(operand.identifier) ||
155 + unconditionalSetStateFunctions.has(operand.identifier.id)
156 + ) {
157 errors.push({
158 reason:
159 "This is an unconditional set state during render, which will trigger an infinite loop. (https://react.dev/reference/react/useState)",
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-validate-no-set-state-not-all-mutable-range-extensions-are-bad.expect.md deleted
-40
@@ -1,40 +0,0 @@
1 -
2 -## Input
3 -
4 -```javascript
5 -// @validateNoSetStateInRenderFunctionExpressions
6 -function Component(props) {
7 - const logEvent = useLogging(props.appId);
8 - const [currentStep, setCurrentStep] = useState(0);
9 -
10 - const onSubmit = (errorEvent) => {
11 - // 2. onSubmit inherits the mutable range of logEvent
12 - logEvent(errorEvent);
13 - // 3. this call then triggers the ValidateNoSetStateInRender check incorrectly, even though
14 - // onSubmit is not called during render (although it _could_ be, if OtherComponent does so.
15 - // but we can't tell without x-file analysis)
16 - setCurrentStep(1);
17 - };
18 -
19 - switch (currentStep) {
20 - case 0:
21 - return <OtherComponent data={{ foo: "bar" }} />;
22 - case 1:
23 - return <OtherComponent data={{ foo: "joe" }} onSubmit={onSubmit} />;
24 - default:
25 - // 1. logEvent's mutable range is extended to this instruction
26 - logEvent("Invalid step");
27 - return <OtherComponent data={null} />;
28 - }
29 -}
30 -
31 -```
32 -
33 -
34 -## Error
35 -
36 -```
37 -[ReactForget] InvalidReact: This is an unconditional set state during render, which will trigger an infinite loop. (https://react.dev/reference/react/useState) (12:12)
38 -```
39 -
40 -
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-unconditional-set-state-lambda.js deleted
-13
@@ -1,13 +0,0 @@
1 -// @validateNoSetStateInRenderFunctionExpressions
2 -function Component(props) {
3 - let y = 0;
4 - const [x, setX] = useState(0);
5 -
6 - const foo = () => {
7 - setX(1);
8 - y = 1; // TODO: force foo's mutable range to extend, but ideally we can just remove this line
9 - };
10 - foo();
11 -
12 - return [x, y];
13 -}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.unconditional-set-state-lambda.expect.md renamed
+3 -5
@@ -2,18 +2,16 @@
2 ## Input
3
4 ```javascript
5 -// @validateNoSetStateInRenderFunctionExpressions
5 +// @validateNoSetStateInRender @validateNoSetStateInRenderFunctionExpressions
6 function Component(props) {
7 - let y = 0;
7 const [x, setX] = useState(0);
8
9 const foo = () => {
10 setX(1);
12 - y = 1; // TODO: force foo's mutable range to extend, but ideally we can just remove this line
11 };
12 foo();
13
16 - return [x, y];
14 + return [x];
15 }
16
17 ```
@@ -22,7 +20,7 @@ function Component(props) {
20 ## Error
21
22 ```
25 -[ReactForget] InvalidReact: This is an unconditional set state during render, which will trigger an infinite loop. (https://react.dev/reference/react/useState) (7:7)
23 +[ReactForget] InvalidReact: This is an unconditional set state during render, which will trigger an infinite loop. (https://react.dev/reference/react/useState) (8:8)
24 ```
25
26
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.unconditional-set-state-lambda.js new
+11
@@ -0,0 +1,11 @@
1 +// @validateNoSetStateInRender @validateNoSetStateInRenderFunctionExpressions
2 +function Component(props) {
3 + const [x, setX] = useState(0);
4 +
5 + const foo = () => {
6 + setX(1);
7 + };
8 + foo();
9 +
10 + return [x];
11 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.unconditional-set-state-nested-function-expressions.expect.md new
+34
@@ -0,0 +1,34 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateNoSetStateInRender @validateNoSetStateInRenderFunctionExpressions
6 +function Component(props) {
7 + const [x, setX] = useState(0);
8 +
9 + const foo = () => {
10 + setX(1);
11 + };
12 +
13 + const bar = () => {
14 + foo();
15 + };
16 +
17 + const baz = () => {
18 + bar();
19 + };
20 + baz();
21 +
22 + return [x];
23 +}
24 +
25 +```
26 +
27 +
28 +## Error
29 +
30 +```
31 +[ReactForget] InvalidReact: This is an unconditional set state during render, which will trigger an infinite loop. (https://react.dev/reference/react/useState) (16:16)
32 +```
33 +
34 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.unconditional-set-state-nested-function-expressions.js new
+19
@@ -0,0 +1,19 @@
1 +// @validateNoSetStateInRender @validateNoSetStateInRenderFunctionExpressions
2 +function Component(props) {
3 + const [x, setX] = useState(0);
4 +
5 + const foo = () => {
6 + setX(1);
7 + };
8 +
9 + const bar = () => {
10 + foo();
11 + };
12 +
13 + const baz = () => {
14 + bar();
15 + };
16 + baz();
17 +
18 + return [x];
19 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/validate-no-set-state-in-render-uncalled-function-with-mutable-range-is-valid.expect.md new
+83
@@ -0,0 +1,83 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateNoSetStateInRenderFunctionExpressions
6 +function Component(props) {
7 + const logEvent = useLogging(props.appId);
8 + const [currentStep, setCurrentStep] = useState(0);
9 +
10 + const onSubmit = (errorEvent) => {
11 + // 2. onSubmit inherits the mutable range of logEvent
12 + logEvent(errorEvent);
13 + // 3. this call then triggers the ValidateNoSetStateInRender check incorrectly, even though
14 + // onSubmit is not called during render (although it _could_ be, if OtherComponent does so.
15 + // but we can't tell without x-file analysis)
16 + setCurrentStep(1);
17 + };
18 +
19 + switch (currentStep) {
20 + case 0:
21 + return <OtherComponent data={{ foo: "bar" }} />;
22 + case 1:
23 + return <OtherComponent data={{ foo: "joe" }} onSubmit={onSubmit} />;
24 + default:
25 + // 1. logEvent's mutable range is extended to this instruction
26 + logEvent("Invalid step");
27 + return <OtherComponent data={null} />;
28 + }
29 +}
30 +
31 +```
32 +
33 +## Code
34 +
35 +```javascript
36 +import { unstable_useMemoCache as useMemoCache } from "react"; // @validateNoSetStateInRenderFunctionExpressions
37 +function Component(props) {
38 + const $ = useMemoCache(3);
39 + const logEvent = useLogging(props.appId);
40 + const [currentStep, setCurrentStep] = useState(0);
41 +
42 + const onSubmit = (errorEvent) => {
43 + logEvent(errorEvent);
44 +
45 + setCurrentStep(1);
46 + };
47 + switch (currentStep) {
48 + case 0: {
49 + let t0;
50 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
51 + t0 = <OtherComponent data={{ foo: "bar" }} />;
52 + $[0] = t0;
53 + } else {
54 + t0 = $[0];
55 + }
56 + return t0;
57 + }
58 + case 1: {
59 + let t1;
60 + if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
61 + t1 = { foo: "joe" };
62 + $[1] = t1;
63 + } else {
64 + t1 = $[1];
65 + }
66 + return <OtherComponent data={t1} onSubmit={onSubmit} />;
67 + }
68 + default: {
69 + logEvent("Invalid step");
70 + let t2;
71 + if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
72 + t2 = <OtherComponent data={null} />;
73 + $[2] = t2;
74 + } else {
75 + t2 = $[2];
76 + }
77 + return t2;
78 + }
79 + }
80 +}
81 +
82 +```
83 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/validate-no-set-state-in-render-uncalled-function-with-mutable-range-is-valid.js renamed
+4 -4
@@ -3,12 +3,12 @@ function Component(props) {
3 const logEvent = useLogging(props.appId);
4 const [currentStep, setCurrentStep] = useState(0);
5
6 + // onSubmit gets the same mutable range as `logEvent`, since that is called
7 + // later. however, our validation uses direct aliasing to track function
8 + // expressions which are invoked, and understands that this function isn't
9 + // called during render:
10 const onSubmit = (errorEvent) => {
7 - // 2. onSubmit inherits the mutable range of logEvent
11 logEvent(errorEvent);
9 - // 3. this call then triggers the ValidateNoSetStateInRender check incorrectly, even though
10 - // onSubmit is not called during render (although it _could_ be, if OtherComponent does so.
11 - // but we can't tell without x-file analysis)
12 setCurrentStep(1);
13 };
14
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/validate-no-set-state-in-render-unconditional-lambda-which-conditionally-sets-state-ok.expect.md new
+75
@@ -0,0 +1,75 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateNoSetStateInRender @validateNoSetStateInRenderFunctionExpressions
6 +function Component(props) {
7 + const [x, setX] = useState(0);
8 +
9 + const foo = () => {
10 + setX(1);
11 + };
12 +
13 + const bar = () => {
14 + if (props.cond) {
15 + // This call is now conditional, so this should pass validation
16 + foo();
17 + }
18 + };
19 +
20 + const baz = () => {
21 + bar();
22 + };
23 + baz();
24 +
25 + return [x];
26 +}
27 +
28 +export const FIXTURE_ENTRYPOINT = {
29 + fn: Component,
30 + params: [{ cond: false }],
31 +};
32 +
33 +```
34 +
35 +## Code
36 +
37 +```javascript
38 +import { unstable_useMemoCache as useMemoCache } from "react"; // @validateNoSetStateInRender @validateNoSetStateInRenderFunctionExpressions
39 +function Component(props) {
40 + const $ = useMemoCache(2);
41 + const [x, setX] = useState(0);
42 +
43 + const foo = () => {
44 + setX(1);
45 + };
46 +
47 + const bar = () => {
48 + if (props.cond) {
49 + foo();
50 + }
51 + };
52 +
53 + const baz = () => {
54 + bar();
55 + };
56 +
57 + baz();
58 + let t0;
59 + if ($[0] !== x) {
60 + t0 = [x];
61 + $[0] = x;
62 + $[1] = t0;
63 + } else {
64 + t0 = $[1];
65 + }
66 + return t0;
67 +}
68 +
69 +export const FIXTURE_ENTRYPOINT = {
70 + fn: Component,
71 + params: [{ cond: false }],
72 +};
73 +
74 +```
75 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/validate-no-set-state-in-render-unconditional-lambda-which-conditionally-sets-state-ok.js new
+27
@@ -0,0 +1,27 @@
1 +// @validateNoSetStateInRender @validateNoSetStateInRenderFunctionExpressions
2 +function Component(props) {
3 + const [x, setX] = useState(0);
4 +
5 + const foo = () => {
6 + setX(1);
7 + };
8 +
9 + const bar = () => {
10 + if (props.cond) {
11 + // This call is now conditional, so this should pass validation
12 + foo();
13 + }
14 + };
15 +
16 + const baz = () => {
17 + bar();
18 + };
19 + baz();
20 +
21 + return [x];
22 +}
23 +
24 +export const FIXTURE_ENTRYPOINT = {
25 + fn: Component,
26 + params: [{ cond: false }],
27 +};
compiler/packages/sprout/src/SproutTodoFilter.ts
+1
@@ -181,6 +181,7 @@ const skipFilter = new Set([
181 "while-conditional-continue",
182 "while-logical",
183 "while-property",
184 + "validate-no-set-state-in-render-uncalled-function-with-mutable-range-is-valid",
185 // Category B with multiple entrypoints,
186 "conditional-break",
187