Skip to content

Commit 2aa5f9d

Browse files
authored
[compiler] fix false positive "mutate frozen" validation with refs (facebook#33993)
The test case here previously reported a "Cannot modify local variables after render completes" error (from ValidateNoFreezingKnownMutableFunctions). This happens because one of the functions passed to a hook clearly mutates a ref — except that we try to ignore mutations of refs! The problem in this case is that the `const ref = ...` was getting converted to a context variable since the ref is accessed in a function before its declaration. We don't infer types for context variables at all, and our ref handling is based on types, so we failed to ignore this ref mutation. The fix is to recognize that `StoreLocal const ...` is a special case: the variable may be referenced in code before the declaration, but at runtime it's either a TDZ error or the variable will have the type from the declaration. So we can safely infer a type. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33993). * __->__ facebook#33993 * facebook#33991 * facebook#33984
1 parent 8c587a2 commit 2aa5f9d

File tree

4 files changed

+171
-2
lines changed

4 files changed

+171
-2
lines changed

compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ export function inferMutationAliasingRanges(
249249
}
250250
for (const param of [...fn.context, ...fn.params]) {
251251
const place = param.kind === 'Identifier' ? param : param.place;
252+
252253
const node = state.nodes.get(place.identifier);
253254
if (node == null) {
254255
continue;

compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
Identifier,
1515
IdentifierId,
1616
Instruction,
17+
InstructionKind,
1718
makePropertyLiteral,
1819
makeType,
1920
PropType,
@@ -194,12 +195,29 @@ function* generateInstructionTypes(
194195
break;
195196
}
196197

197-
// We intentionally do not infer types for context variables
198+
// We intentionally do not infer types for most context variables
198199
case 'DeclareContext':
199-
case 'StoreContext':
200200
case 'LoadContext': {
201201
break;
202202
}
203+
case 'StoreContext': {
204+
/**
205+
* The caveat is StoreContext const, where we know the value is
206+
* assigned once such that everywhere the value is accessed, it
207+
* must have the same type from the rvalue.
208+
*
209+
* A concrete example where this is useful is `const ref = useRef()`
210+
* where the ref is referenced before its declaration in a function
211+
* expression, causing it to be converted to a const context variable.
212+
*/
213+
if (value.lvalue.kind === InstructionKind.Const) {
214+
yield equation(
215+
value.lvalue.place.identifier.type,
216+
value.value.identifier.type,
217+
);
218+
}
219+
break;
220+
}
203221

204222
case 'StoreLocal': {
205223
if (env.config.enableUseTypeAnnotations) {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @flow
6+
component Example() {
7+
const fooRef = useRef();
8+
9+
function updateStyles() {
10+
const foo = fooRef.current;
11+
// The access of `barRef` here before its declaration causes it be hoisted...
12+
if (barRef.current == null || foo == null) {
13+
return;
14+
}
15+
foo.style.height = '100px';
16+
}
17+
18+
// ...which previously meant that we didn't infer a type...
19+
const barRef = useRef(null);
20+
21+
const resizeRef = useResizeObserver(
22+
rect => {
23+
const {width} = rect;
24+
// ...which meant that we failed to ignore the mutation here...
25+
barRef.current = width;
26+
} // ...which caused this to fail with "can't freeze a mutable function"
27+
);
28+
29+
useLayoutEffect(() => {
30+
const observer = new ResizeObserver(_ => {
31+
updateStyles();
32+
});
33+
34+
return () => {
35+
observer.disconnect();
36+
};
37+
}, []);
38+
39+
return <div ref={resizeRef} />;
40+
}
41+
42+
```
43+
44+
## Code
45+
46+
```javascript
47+
import { c as _c } from "react/compiler-runtime";
48+
function Example() {
49+
const $ = _c(6);
50+
const fooRef = useRef();
51+
let t0;
52+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
53+
t0 = function updateStyles() {
54+
const foo = fooRef.current;
55+
if (barRef.current == null || foo == null) {
56+
return;
57+
}
58+
59+
foo.style.height = "100px";
60+
};
61+
$[0] = t0;
62+
} else {
63+
t0 = $[0];
64+
}
65+
const updateStyles = t0;
66+
67+
const barRef = useRef(null);
68+
let t1;
69+
if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
70+
t1 = (rect) => {
71+
const { width } = rect;
72+
73+
barRef.current = width;
74+
};
75+
$[1] = t1;
76+
} else {
77+
t1 = $[1];
78+
}
79+
const resizeRef = useResizeObserver(t1);
80+
let t2;
81+
let t3;
82+
if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
83+
t2 = () => {
84+
const observer = new ResizeObserver((_) => {
85+
updateStyles();
86+
});
87+
return () => {
88+
observer.disconnect();
89+
};
90+
};
91+
92+
t3 = [];
93+
$[2] = t2;
94+
$[3] = t3;
95+
} else {
96+
t2 = $[2];
97+
t3 = $[3];
98+
}
99+
useLayoutEffect(t2, t3);
100+
let t4;
101+
if ($[4] !== resizeRef) {
102+
t4 = <div ref={resizeRef} />;
103+
$[4] = resizeRef;
104+
$[5] = t4;
105+
} else {
106+
t4 = $[5];
107+
}
108+
return t4;
109+
}
110+
111+
```
112+
113+
### Eval output
114+
(kind: exception) Fixture not implemented
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// @flow
2+
component Example() {
3+
const fooRef = useRef();
4+
5+
function updateStyles() {
6+
const foo = fooRef.current;
7+
// The access of `barRef` here before its declaration causes it be hoisted...
8+
if (barRef.current == null || foo == null) {
9+
return;
10+
}
11+
foo.style.height = '100px';
12+
}
13+
14+
// ...which previously meant that we didn't infer a type...
15+
const barRef = useRef(null);
16+
17+
const resizeRef = useResizeObserver(
18+
rect => {
19+
const {width} = rect;
20+
// ...which meant that we failed to ignore the mutation here...
21+
barRef.current = width;
22+
} // ...which caused this to fail with "can't freeze a mutable function"
23+
);
24+
25+
useLayoutEffect(() => {
26+
const observer = new ResizeObserver(_ => {
27+
updateStyles();
28+
});
29+
30+
return () => {
31+
observer.disconnect();
32+
};
33+
}, []);
34+
35+
return <div ref={resizeRef} />;
36+
}

0 commit comments

Comments
 (0)