[compiler] More complete validation against locals being reassigned after render
Summary: This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns. This causes a number of new bailouts in test cases, all of which appear to me to be legit. ghstack-source-id: 770bf02d079ea2480be243a49caa6f69573d8092 Pull Request resolved: https://github.com/facebook/react/pull/30540
Mike Vitousek committed
Jul 31, 2024 at 11:11 UTC
56dbd58feb1a75f075c67067ce0d20d7cee7f482
13 files changed
+157
-238
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts
+34
-6
@@ -8,9 +8,11 @@
8
import {CompilerError, Effect} from '..';
9
import {HIRFunction, IdentifierId, Place} from '../HIR';
10
import {
11
+ eachInstructionLValue,
12
eachInstructionValueOperand,
13
eachTerminalOperand,
14
} from '../HIR/visitors';
15
+import {getFunctionCallSignature} from '../Inference/InferReferenceEffects';
16
17
/**
18
* Validates that local variables cannot be reassigned after render.
@@ -131,7 +133,26 @@ function getContextReassignment(
133
break;
134
}
135
default: {
134
- for (const operand of eachInstructionValueOperand(value)) {
136
+ let operands = eachInstructionValueOperand(value);
137
+ // If we're calling a function that doesn't let its arguments escape, only test the callee
138
+ if (value.kind === 'CallExpression') {
139
+ const signature = getFunctionCallSignature(
140
+ fn.env,
141
+ value.callee.identifier.type,
142
+ );
143
+ if (signature?.noAlias) {
144
+ operands = [value.callee];
145
+ }
146
+ } else if (value.kind === 'MethodCall') {
147
+ const signature = getFunctionCallSignature(
148
+ fn.env,
149
+ value.property.identifier.type,
150
+ );
151
+ if (signature?.noAlias) {
152
+ operands = [value.receiver, value.property];
153
+ }
154
+ }
155
+ for (const operand of operands) {
156
CompilerError.invariant(operand.effect !== Effect.Unknown, {
157
reason: `Expected effects to be inferred prior to ValidateLocalsNotReassignedAfterRender`,
158
loc: operand.loc,
@@ -139,15 +160,22 @@ function getContextReassignment(
160
const reassignment = reassigningFunctions.get(
161
operand.identifier.id,
162
);
142
- if (
143
- reassignment !== undefined &&
144
- operand.effect === Effect.Freeze
145
- ) {
163
+ if (reassignment !== undefined) {
164
/*
165
* Functions that reassign local variables are inherently mutable and are unsafe to pass
166
* to a place that expects a frozen value. Propagate the reassignment upward.
167
*/
150
- return reassignment;
168
+ if (operand.effect === Effect.Freeze) {
169
+ return reassignment;
170
+ } else {
171
+ /*
172
+ * If the operand is not frozen but it does reassign, then the lvalues
173
+ * of the instruction could also be reassigning
174
+ */
175
+ for (const lval of eachInstructionLValue(instr)) {
176
+ reassigningFunctions.set(lval.identifier.id, reassignment);
177
+ }
178
+ }
179
}
180
}
181
break;
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-only-chained-assign.expect.md
deleted
-65
@@ -1,65 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-import {identity, invoke} from 'shared-runtime';
6
-
7
-function foo() {
8
- let x = 2;
9
- const fn1 = () => {
10
- const copy1 = (x = 3);
11
- return identity(copy1);
12
- };
13
- const fn2 = () => {
14
- const copy2 = (x = 4);
15
- return [invoke(fn1), copy2, identity(copy2)];
16
- };
17
- return invoke(fn2);
18
-}
19
-
20
-export const FIXTURE_ENTRYPOINT = {
21
- fn: foo,
22
- params: [],
23
-};
24
-
25
-```
26
-
27
-## Code
28
-
29
-```javascript
30
-import { c as _c } from "react/compiler-runtime";
31
-import { identity, invoke } from "shared-runtime";
32
-
33
-function foo() {
34
- const $ = _c(1);
35
- let t0;
36
- if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
37
- let x;
38
- x = 2;
39
- const fn1 = () => {
40
- const copy1 = (x = 3);
41
- return identity(copy1);
42
- };
43
-
44
- const fn2 = () => {
45
- const copy2 = (x = 4);
46
- return [invoke(fn1), copy2, identity(copy2)];
47
- };
48
-
49
- t0 = invoke(fn2);
50
- $[0] = t0;
51
- } else {
52
- t0 = $[0];
53
- }
54
- return t0;
55
-}
56
-
57
-export const FIXTURE_ENTRYPOINT = {
58
- fn: foo,
59
- params: [],
60
-};
61
-
62
-```
63
-
64
-### Eval output
65
-(kind: ok) [3,4,4]
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/declare-reassign-variable-in-function-declaration.expect.md
deleted
-40
@@ -1,40 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-function Component() {
6
- let x = null;
7
- function foo() {
8
- x = 9;
9
- }
10
- const y = bar(foo);
11
- return <Child y={y} />;
12
-}
13
-
14
-```
15
-
16
-## Code
17
-
18
-```javascript
19
-import { c as _c } from "react/compiler-runtime";
20
-function Component() {
21
- const $ = _c(1);
22
- let t0;
23
- if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
24
- let x;
25
- x = null;
26
- const foo = function foo() {
27
- x = 9;
28
- };
29
-
30
- const y = bar(foo);
31
- t0 = <Child y={y} />;
32
- $[0] = t0;
33
- } else {
34
- t0 = $[0];
35
- }
36
- return t0;
37
-}
38
-
39
-```
40
-
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.expect.md
new
+40
@@ -0,0 +1,40 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import {identity, invoke} from 'shared-runtime';
6
+
7
+function foo() {
8
+ let x = 2;
9
+ const fn1 = () => {
10
+ const copy1 = (x = 3);
11
+ return identity(copy1);
12
+ };
13
+ const fn2 = () => {
14
+ const copy2 = (x = 4);
15
+ return [invoke(fn1), copy2, identity(copy2)];
16
+ };
17
+ return invoke(fn2);
18
+}
19
+
20
+export const FIXTURE_ENTRYPOINT = {
21
+ fn: foo,
22
+ params: [],
23
+};
24
+
25
+```
26
+
27
+
28
+## Error
29
+
30
+```
31
+ 8 | };
32
+ 9 | const fn2 = () => {
33
+> 10 | const copy2 = (x = 4);
34
+ | ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `x` cannot be reassigned after render (10:10)
35
+ 11 | return [invoke(fn1), copy2, identity(copy2)];
36
+ 12 | };
37
+ 13 | return invoke(fn2);
38
+```
39
+
40
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.js
renamed
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.declare-reassign-variable-in-function-declaration.expect.md
new
+29
@@ -0,0 +1,29 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component() {
6
+ let x = null;
7
+ function foo() {
8
+ x = 9;
9
+ }
10
+ const y = bar(foo);
11
+ return <Child y={y} />;
12
+}
13
+
14
+```
15
+
16
+
17
+## Error
18
+
19
+```
20
+ 2 | let x = null;
21
+ 3 | function foo() {
22
+> 4 | x = 9;
23
+ | ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `x` cannot be reassigned after render (4:4)
24
+ 5 | }
25
+ 6 | const y = bar(foo);
26
+ 7 | return <Child y={y} />;
27
+```
28
+
29
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.declare-reassign-variable-in-function-declaration.js
renamed
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-nested-function-reassign-local-variable-in-effect.expect.md
renamed
+10
-53
@@ -42,60 +42,17 @@ function Component() {
42
43
```
44
45
-## Code
45
47
-```javascript
48
-import { c as _c } from "react/compiler-runtime";
49
-import { useEffect } from "react";
50
-function Component() {
51
- const $ = _c(4);
52
- let local;
53
- let t0;
54
- if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
55
- const mk_reassignlocal = () => {
56
- const reassignLocal = (newValue) => {
57
- local = newValue;
58
- };
59
- return reassignLocal;
60
- };
61
-
62
- t0 = mk_reassignlocal();
63
- $[0] = t0;
64
- } else {
65
- t0 = $[0];
66
- }
67
- const reassignLocal_0 = t0;
68
- let t1;
69
- if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
70
- t1 = (newValue_0) => {
71
- reassignLocal_0("hello");
72
- if (local === newValue_0) {
73
- console.log("`local` was updated!");
74
- } else {
75
- throw new Error("`local` not updated!");
76
- }
77
- };
78
- $[1] = t1;
79
- } else {
80
- t1 = $[1];
81
- }
82
- const onMount = t1;
83
- let t2;
84
- let t3;
85
- if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
86
- t2 = () => {
87
- onMount();
88
- };
89
- t3 = [onMount];
90
- $[2] = t2;
91
- $[3] = t3;
92
- } else {
93
- t2 = $[2];
94
- t3 = $[3];
95
- }
96
- useEffect(t2, t3);
97
- return "ok";
98
-}
46
+## Error
47
48
```
49
+ 5 | // Create the reassignment function inside another function, then return it
50
+ 6 | const reassignLocal = newValue => {
51
+> 7 | local = newValue;
52
+ | ^^^^^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `local` cannot be reassigned after render (7:7)
53
+ 8 | };
54
+ 9 | return reassignLocal;
55
+ 10 | };
56
+```
57
+
58
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-nested-function-reassign-local-variable-in-effect.js
renamed
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.expect.md
new
+44
@@ -0,0 +1,44 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
6
+let cond = true;
7
+function Component(props) {
8
+ let a;
9
+ let b;
10
+ const f = () => {
11
+ if (cond) {
12
+ a = {};
13
+ b = [];
14
+ } else {
15
+ a = {};
16
+ b = [];
17
+ }
18
+ a.property = true;
19
+ b.push(false);
20
+ };
21
+ return <div onClick={f()} />;
22
+}
23
+
24
+export const FIXTURE_ENTRYPOINT = {
25
+ fn: Component,
26
+ params: [{}],
27
+};
28
+
29
+```
30
+
31
+
32
+## Error
33
+
34
+```
35
+ 6 | const f = () => {
36
+ 7 | if (cond) {
37
+> 8 | a = {};
38
+ | ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `a` cannot be reassigned after render (8:8)
39
+ 9 | b = [];
40
+ 10 | } else {
41
+ 11 | a = {};
42
+```
43
+
44
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.js
renamed
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutable-range-shared-inner-outer-function.expect.md
deleted
-71
@@ -1,71 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-// @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
6
-let cond = true;
7
-function Component(props) {
8
- let a;
9
- let b;
10
- const f = () => {
11
- if (cond) {
12
- a = {};
13
- b = [];
14
- } else {
15
- a = {};
16
- b = [];
17
- }
18
- a.property = true;
19
- b.push(false);
20
- };
21
- return <div onClick={f()} />;
22
-}
23
-
24
-export const FIXTURE_ENTRYPOINT = {
25
- fn: Component,
26
- params: [{}],
27
-};
28
-
29
-```
30
-
31
-## Code
32
-
33
-```javascript
34
-import { c as _c } from "react/compiler-runtime"; // @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
35
-let cond = true;
36
-function Component(props) {
37
- const $ = _c(1);
38
- let t0;
39
- if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
40
- let a;
41
- let b;
42
- const f = () => {
43
- if (cond) {
44
- a = {};
45
- b = [];
46
- } else {
47
- a = {};
48
- b = [];
49
- }
50
-
51
- a.property = true;
52
- b.push(false);
53
- };
54
-
55
- t0 = <div onClick={f()} />;
56
- $[0] = t0;
57
- } else {
58
- t0 = $[0];
59
- }
60
- return t0;
61
-}
62
-
63
-export const FIXTURE_ENTRYPOINT = {
64
- fn: Component,
65
- params: [{}],
66
-};
67
-
68
-```
69
-
70
-### Eval output
71
-(kind: ok) <div></div>
\ No newline at end of file
compiler/packages/snap/src/SproutTodoFilter.ts
-3
@@ -187,7 +187,6 @@ const skipFilter = new Set([
187
'alias-nested-member-path-mutate',
188
'concise-arrow-expr',
189
'const-propagation-into-function-expression-global',
190
- 'declare-reassign-variable-in-function-declaration',
190
'lambda-mutate-shadowed-object',
191
'fbt/lambda-with-fbt',
192
'recursive-function-expr',
@@ -503,8 +502,6 @@ const skipFilter = new Set([
502
503
// needs to be executed as a module
504
'meta-property',
506
-
507
- 'todo.invalid-nested-function-reassign-local-variable-in-effect',
505
]);
506
507
export default skipFilter;