Skip to content

Commit 9b14f8f

Browse files
committed
[compiler] Validate against locals being reassigned after render
Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we: 1. Build up a mapping of context variables in the outer component/hook 2. Find ObjectMethod/FunctionExpressions which may reassign those context variables 3. Propagate aliases of those functions via StoreLocal/LoadLocal 4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc. Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX. ghstack-source-id: a1afaeea248564d85048a6d2e46c5feea63df9f7 Pull Request resolved: #30107
1 parent 85a6fbf commit 9b14f8f

12 files changed

+217
-134
lines changed

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

+3
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ import {
9696
validatePreservedManualMemoization,
9797
validateUseMemo,
9898
} from "../Validation";
99+
import { validateLocalsNotReassignedAfterRender } from "../Validation/ValidateLocalsNotReassignedAfterRender";
99100

100101
export type CompilerPipelineValue =
101102
| { kind: "ast"; name: string; value: CodegenFunction }
@@ -202,6 +203,8 @@ function* runWithEnvironment(
202203
inferReferenceEffects(hir);
203204
yield log({ kind: "hir", name: "InferReferenceEffects", value: hir });
204205

206+
validateLocalsNotReassignedAfterRender(hir);
207+
205208
// Note: Has to come after infer reference effects because "dead" code may still affect inference
206209
deadCodeElimination(hir);
207210
yield log({ kind: "hir", name: "DeadCodeElimination", value: hir });
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
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, Effect } from "..";
9+
import { HIRFunction, IdentifierId, Place } from "../HIR";
10+
import {
11+
eachInstructionValueOperand,
12+
eachTerminalOperand,
13+
} from "../HIR/visitors";
14+
15+
/**
16+
* Validates that local variables cannot be reassigned after render.
17+
* This prevents a category of bugs in which a closure captures a
18+
* binding from one render but does not update
19+
*/
20+
export function validateLocalsNotReassignedAfterRender(fn: HIRFunction): void {
21+
const contextVariables = new Set<IdentifierId>();
22+
const reassignment = getContextReassignment(fn, contextVariables, false);
23+
if (reassignment !== null) {
24+
CompilerError.throwInvalidJS({
25+
reason:
26+
"Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead",
27+
description:
28+
reassignment.identifier.name !== null &&
29+
reassignment.identifier.name.kind === "named"
30+
? `Variable \`${reassignment.identifier.name.value}\` cannot be reassigned after render`
31+
: "",
32+
loc: reassignment.loc,
33+
});
34+
}
35+
}
36+
37+
function getContextReassignment(
38+
fn: HIRFunction,
39+
contextVariables: Set<IdentifierId>,
40+
isFunctionExpression: boolean
41+
): Place | null {
42+
const reassigningFunctions = new Map<IdentifierId, Place>();
43+
for (const [, block] of fn.body.blocks) {
44+
for (const instr of block.instructions) {
45+
const { lvalue, value } = instr;
46+
switch (value.kind) {
47+
case "FunctionExpression":
48+
case "ObjectMethod": {
49+
let reassignment = getContextReassignment(
50+
value.loweredFunc.func,
51+
contextVariables,
52+
true
53+
);
54+
if (reassignment === null) {
55+
// If the function itself doesn't reassign, does one of its dependencies?
56+
for (const operand of eachInstructionValueOperand(value)) {
57+
const reassignmentFromOperand = reassigningFunctions.get(
58+
operand.identifier.id
59+
);
60+
if (reassignmentFromOperand !== undefined) {
61+
reassignment = reassignmentFromOperand;
62+
break;
63+
}
64+
}
65+
}
66+
// if the function or its depends reassign, propagate that fact on the lvalue
67+
if (reassignment !== null) {
68+
reassigningFunctions.set(lvalue.identifier.id, reassignment);
69+
}
70+
break;
71+
}
72+
case "StoreLocal": {
73+
const reassignment = reassigningFunctions.get(
74+
value.value.identifier.id
75+
);
76+
if (reassignment !== undefined) {
77+
reassigningFunctions.set(
78+
value.lvalue.place.identifier.id,
79+
reassignment
80+
);
81+
reassigningFunctions.set(lvalue.identifier.id, reassignment);
82+
}
83+
break;
84+
}
85+
case "LoadLocal": {
86+
const reassignment = reassigningFunctions.get(
87+
value.place.identifier.id
88+
);
89+
if (reassignment !== undefined) {
90+
reassigningFunctions.set(lvalue.identifier.id, reassignment);
91+
}
92+
break;
93+
}
94+
case "DeclareContext": {
95+
if (!isFunctionExpression) {
96+
contextVariables.add(value.lvalue.place.identifier.id);
97+
}
98+
break;
99+
}
100+
case "StoreContext": {
101+
if (isFunctionExpression) {
102+
if (contextVariables.has(value.lvalue.place.identifier.id)) {
103+
return value.lvalue.place;
104+
}
105+
} else {
106+
/*
107+
* We only track reassignments of variables defined in the outer
108+
* component or hook.
109+
*/
110+
contextVariables.add(value.lvalue.place.identifier.id);
111+
}
112+
break;
113+
}
114+
default: {
115+
for (const operand of eachInstructionValueOperand(value)) {
116+
CompilerError.invariant(operand.effect !== Effect.Unknown, {
117+
reason: `Expected effects to be inferred prior to ValidateLocalsNotReassignedAfterRender`,
118+
loc: operand.loc,
119+
});
120+
const reassignment = reassigningFunctions.get(
121+
operand.identifier.id
122+
);
123+
if (
124+
reassignment !== undefined &&
125+
operand.effect === Effect.Freeze
126+
) {
127+
/*
128+
* Functions that reassign local variables are inherently mutable and are unsafe to pass
129+
* to a place that expects a frozen value. Propagate the reassignment upward.
130+
*/
131+
return reassignment;
132+
}
133+
}
134+
break;
135+
}
136+
}
137+
}
138+
for (const operand of eachTerminalOperand(block.terminal)) {
139+
const reassignment = reassigningFunctions.get(operand.identifier.id);
140+
if (reassignment !== undefined) {
141+
return reassignment;
142+
}
143+
}
144+
}
145+
return null;
146+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
2+
## Input
3+
4+
```javascript
5+
function useFoo() {
6+
let x = 0;
7+
return (value) => {
8+
x = value;
9+
};
10+
}
11+
12+
```
13+
14+
15+
## Error
16+
17+
```
18+
2 | let x = 0;
19+
3 | return (value) => {
20+
> 4 | x = value;
21+
| ^ InvalidJS: 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)
22+
5 | };
23+
6 | }
24+
7 |
25+
```
26+
27+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
function useFoo() {
2+
let x = 0;
3+
return (value) => {
4+
x = value;
5+
};
6+
}
+10-49
Original file line numberDiff line numberDiff line change
@@ -43,56 +43,17 @@ function Component() {
4343

4444
```
4545

46-
## Code
4746

48-
```javascript
49-
import { c as _c } from "react/compiler-runtime";
50-
import { useEffect } from "react";
51-
52-
function Component() {
53-
const $ = _c(4);
54-
let local;
55-
let t0;
56-
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
57-
t0 = (newValue) => {
58-
local = newValue;
59-
};
60-
$[0] = t0;
61-
} else {
62-
t0 = $[0];
63-
}
64-
const reassignLocal = t0;
65-
let t1;
66-
if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
67-
t1 = (newValue_0) => {
68-
reassignLocal("hello");
69-
if (local === newValue_0) {
70-
console.log("`local` was updated!");
71-
} else {
72-
throw new Error("`local` not updated!");
73-
}
74-
};
75-
$[1] = t1;
76-
} else {
77-
t1 = $[1];
78-
}
79-
const onMount = t1;
80-
let t2;
81-
let t3;
82-
if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
83-
t2 = () => {
84-
onMount();
85-
};
86-
t3 = [onMount];
87-
$[2] = t2;
88-
$[3] = t3;
89-
} else {
90-
t2 = $[2];
91-
t3 = $[3];
92-
}
93-
useEffect(t2, t3);
94-
return "ok";
95-
}
47+
## Error
9648

9749
```
50+
5 |
51+
6 | const reassignLocal = (newValue) => {
52+
> 7 | local = newValue;
53+
| ^^^^^ InvalidJS: 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)
54+
8 | };
55+
9 |
56+
10 | const onMount = (newValue) => {
57+
```
58+
9859
Original file line numberDiff line numberDiff line change
@@ -44,53 +44,17 @@ function Component() {
4444

4545
```
4646

47-
## Code
4847

49-
```javascript
50-
import { c as _c } from "react/compiler-runtime";
51-
import { useEffect } from "react";
52-
import { useIdentity } from "shared-runtime";
53-
54-
function Component() {
55-
const $ = _c(3);
56-
let local;
57-
let t0;
58-
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
59-
t0 = (newValue) => {
60-
local = newValue;
61-
};
62-
$[0] = t0;
63-
} else {
64-
t0 = $[0];
65-
}
66-
const reassignLocal = t0;
67-
let t1;
68-
if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
69-
t1 = (newValue_0) => {
70-
reassignLocal("hello");
71-
if (local === newValue_0) {
72-
console.log("`local` was updated!");
73-
} else {
74-
throw new Error("`local` not updated!");
75-
}
76-
};
77-
$[1] = t1;
78-
} else {
79-
t1 = $[1];
80-
}
81-
const callback = t1;
82-
let t2;
83-
if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
84-
t2 = () => {
85-
callback();
86-
};
87-
$[2] = t2;
88-
} else {
89-
t2 = $[2];
90-
}
91-
useIdentity(t2);
92-
return "ok";
93-
}
48+
## Error
9449

9550
```
51+
6 |
52+
7 | const reassignLocal = (newValue) => {
53+
> 8 | local = newValue;
54+
| ^^^^^ InvalidJS: 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 (8:8)
55+
9 | };
56+
10 |
57+
11 | const callback = (newValue) => {
58+
```
59+
9660
+10-34
Original file line numberDiff line numberDiff line change
@@ -37,41 +37,17 @@ function Component() {
3737

3838
```
3939

40-
## Code
4140

42-
```javascript
43-
import { c as _c } from "react/compiler-runtime";
44-
function Component() {
45-
const $ = _c(2);
46-
let local;
47-
let t0;
48-
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
49-
t0 = (newValue) => {
50-
local = newValue;
51-
};
52-
$[0] = t0;
53-
} else {
54-
t0 = $[0];
55-
}
56-
const reassignLocal = t0;
57-
let t1;
58-
if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
59-
const onClick = (newValue_0) => {
60-
reassignLocal("hello");
61-
if (local === newValue_0) {
62-
console.log("`local` was updated!");
63-
} else {
64-
throw new Error("`local` not updated!");
65-
}
66-
};
67-
68-
t1 = <button onClick={onClick}>Submit</button>;
69-
$[1] = t1;
70-
} else {
71-
t1 = $[1];
72-
}
73-
return t1;
74-
}
41+
## Error
7542

7643
```
44+
3 |
45+
4 | const reassignLocal = (newValue) => {
46+
> 5 | local = newValue;
47+
| ^^^^^ InvalidJS: 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 (5:5)
48+
6 | };
49+
7 |
50+
8 | const onClick = (newValue) => {
51+
```
52+
7753

0 commit comments

Comments
 (0)