Skip to content

Commit 7c28c15

Browse files
authored
[compiler] Fix AnalyzeFunctions to fully reset context identifiers (#33500)
AnalyzeFunctions had logic to reset the mutable ranges of context variables after visiting inner function expressions. However, there was a bug in that logic: InferReactiveScopeVariables makes all the identifiers in a scope point to the same mutable range instance. That meant that it was possible for a later function expression to indirectly cause an earlier function expressions' context variables to get a non-zero mutable range. The fix is to not just reset start/end of context var ranges, but assign a new range instance. Thanks for the help on debugging, @mofeiZ! --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33500). * #33571 * #33558 * #33547 * #33543 * #33533 * #33532 * #33530 * #33526 * #33522 * #33518 * #33514 * #33513 * #33512 * #33504 * __->__ #33500 * #33497 * #33496
1 parent 90ccbd7 commit 7c28c15

File tree

3 files changed

+115
-2
lines changed

3 files changed

+115
-2
lines changed

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,16 @@ export default function analyseFunctions(func: HIRFunction): void {
4242
* Reset mutable range for outer inferReferenceEffects
4343
*/
4444
for (const operand of instr.value.loweredFunc.func.context) {
45-
operand.identifier.mutableRange.start = makeInstructionId(0);
46-
operand.identifier.mutableRange.end = makeInstructionId(0);
45+
/**
46+
* NOTE: inferReactiveScopeVariables makes identifiers in the scope
47+
* point to the *same* mutableRange instance. Resetting start/end
48+
* here is insufficient, because a later mutation of the range
49+
* for any one identifier could affect the range for other identifiers.
50+
*/
51+
operand.identifier.mutableRange = {
52+
start: makeInstructionId(0),
53+
end: makeInstructionId(0),
54+
};
4755
operand.identifier.scope = null;
4856
}
4957
break;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
2+
## Input
3+
4+
```javascript
5+
//@flow @validatePreserveExistingMemoizationGuarantees @enableNewMutationAliasingModel
6+
component Component(
7+
onAsyncSubmit?: (() => void) => void,
8+
onClose: (isConfirmed: boolean) => void
9+
) {
10+
// When running inferReactiveScopeVariables,
11+
// onAsyncSubmit and onClose update to share
12+
// a mutableRange instance.
13+
const onSubmit = useCallback(() => {
14+
if (onAsyncSubmit) {
15+
onAsyncSubmit(() => {
16+
onClose(true);
17+
});
18+
return;
19+
}
20+
}, [onAsyncSubmit, onClose]);
21+
// When running inferReactiveScopeVariables here,
22+
// first the existing range gets updated (affecting
23+
// onAsyncSubmit) and then onClose gets assigned a
24+
// different mutable range instance, which is the
25+
// one reset after AnalyzeFunctions.
26+
// The fix is to fully reset mutable ranges *instances*
27+
// after AnalyzeFunctions visit a function expression
28+
return <Dialog onSubmit={onSubmit} onClose={() => onClose(false)} />;
29+
}
30+
31+
```
32+
33+
## Code
34+
35+
```javascript
36+
import { c as _c } from "react/compiler-runtime";
37+
function Component(t0) {
38+
const $ = _c(8);
39+
const { onAsyncSubmit, onClose } = t0;
40+
let t1;
41+
if ($[0] !== onAsyncSubmit || $[1] !== onClose) {
42+
t1 = () => {
43+
if (onAsyncSubmit) {
44+
onAsyncSubmit(() => {
45+
onClose(true);
46+
});
47+
return;
48+
}
49+
};
50+
$[0] = onAsyncSubmit;
51+
$[1] = onClose;
52+
$[2] = t1;
53+
} else {
54+
t1 = $[2];
55+
}
56+
const onSubmit = t1;
57+
let t2;
58+
if ($[3] !== onClose) {
59+
t2 = () => onClose(false);
60+
$[3] = onClose;
61+
$[4] = t2;
62+
} else {
63+
t2 = $[4];
64+
}
65+
let t3;
66+
if ($[5] !== onSubmit || $[6] !== t2) {
67+
t3 = <Dialog onSubmit={onSubmit} onClose={t2} />;
68+
$[5] = onSubmit;
69+
$[6] = t2;
70+
$[7] = t3;
71+
} else {
72+
t3 = $[7];
73+
}
74+
return t3;
75+
}
76+
77+
```
78+
79+
### Eval output
80+
(kind: exception) Fixture not implemented
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
//@flow @validatePreserveExistingMemoizationGuarantees @enableNewMutationAliasingModel
2+
component Component(
3+
onAsyncSubmit?: (() => void) => void,
4+
onClose: (isConfirmed: boolean) => void
5+
) {
6+
// When running inferReactiveScopeVariables,
7+
// onAsyncSubmit and onClose update to share
8+
// a mutableRange instance.
9+
const onSubmit = useCallback(() => {
10+
if (onAsyncSubmit) {
11+
onAsyncSubmit(() => {
12+
onClose(true);
13+
});
14+
return;
15+
}
16+
}, [onAsyncSubmit, onClose]);
17+
// When running inferReactiveScopeVariables here,
18+
// first the existing range gets updated (affecting
19+
// onAsyncSubmit) and then onClose gets assigned a
20+
// different mutable range instance, which is the
21+
// one reset after AnalyzeFunctions.
22+
// The fix is to fully reset mutable ranges *instances*
23+
// after AnalyzeFunctions visit a function expression
24+
return <Dialog onSubmit={onSubmit} onClose={() => onClose(false)} />;
25+
}

0 commit comments

Comments
 (0)