Skip to content

Commit 98308ae

Browse files
josephsavonajosephsavona
committed
[compiler] Fix AnalyzeFunctions to fully reset context identifiers
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!
1 parent 7603b48 commit 98308ae

File tree

7 files changed

+255
-178
lines changed

7 files changed

+255
-178
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;

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inner-function/nullable-objects/array-map-named-callback-cross-context.expect.md‎

Lines changed: 38 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -57,67 +57,62 @@ import { Stringify } from "shared-runtime";
5757
* - cb1 is not assumed to be called since it's only used as a call operand
5858
*/
5959
function useFoo(t0) {
60-
const $ = _c(14);
61-
let arr1;
62-
let arr2;
60+
const $ = _c(13);
61+
const { arr1, arr2 } = t0;
6362
let t1;
64-
if ($[0] !== t0) {
65-
({ arr1, arr2 } = t0);
66-
let t2;
67-
if ($[4] !== arr1[0]) {
68-
t2 = (e) => arr1[0].value + e.value;
69-
$[4] = arr1[0];
70-
$[5] = t2;
71-
} else {
72-
t2 = $[5];
73-
}
74-
const cb1 = t2;
75-
t1 = () => arr1.map(cb1);
76-
$[0] = t0;
77-
$[1] = arr1;
78-
$[2] = arr2;
79-
$[3] = t1;
63+
if ($[0] !== arr1[0]) {
64+
t1 = (e) => arr1[0].value + e.value;
65+
$[0] = arr1[0];
66+
$[1] = t1;
8067
} else {
81-
arr1 = $[1];
82-
arr2 = $[2];
83-
t1 = $[3];
68+
t1 = $[1];
8469
}
85-
const getArrMap1 = t1;
70+
const cb1 = t1;
8671
let t2;
87-
if ($[6] !== arr2) {
88-
t2 = (e_0) => arr2[0].value + e_0.value;
89-
$[6] = arr2;
90-
$[7] = t2;
72+
if ($[2] !== arr1 || $[3] !== cb1) {
73+
t2 = () => arr1.map(cb1);
74+
$[2] = arr1;
75+
$[3] = cb1;
76+
$[4] = t2;
9177
} else {
92-
t2 = $[7];
78+
t2 = $[4];
9379
}
94-
const cb2 = t2;
80+
const getArrMap1 = t2;
9581
let t3;
96-
if ($[8] !== arr1 || $[9] !== cb2) {
97-
t3 = () => arr1.map(cb2);
98-
$[8] = arr1;
99-
$[9] = cb2;
100-
$[10] = t3;
82+
if ($[5] !== arr2) {
83+
t3 = (e_0) => arr2[0].value + e_0.value;
84+
$[5] = arr2;
85+
$[6] = t3;
10186
} else {
102-
t3 = $[10];
87+
t3 = $[6];
10388
}
104-
const getArrMap2 = t3;
89+
const cb2 = t3;
10590
let t4;
106-
if ($[11] !== getArrMap1 || $[12] !== getArrMap2) {
107-
t4 = (
91+
if ($[7] !== arr1 || $[8] !== cb2) {
92+
t4 = () => arr1.map(cb2);
93+
$[7] = arr1;
94+
$[8] = cb2;
95+
$[9] = t4;
96+
} else {
97+
t4 = $[9];
98+
}
99+
const getArrMap2 = t4;
100+
let t5;
101+
if ($[10] !== getArrMap1 || $[11] !== getArrMap2) {
102+
t5 = (
108103
<Stringify
109104
getArrMap1={getArrMap1}
110105
getArrMap2={getArrMap2}
111106
shouldInvokeFns={true}
112107
/>
113108
);
114-
$[11] = getArrMap1;
115-
$[12] = getArrMap2;
116-
$[13] = t4;
109+
$[10] = getArrMap1;
110+
$[11] = getArrMap2;
111+
$[12] = t5;
117112
} else {
118-
t4 = $[13];
113+
t5 = $[12];
119114
}
120-
return t4;
115+
return t5;
121116
}
122117

123118
export const FIXTURE_ENTRYPOINT = {

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/array-map-named-callback-cross-context.expect.md‎

Lines changed: 38 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -58,67 +58,62 @@ import { Stringify } from "shared-runtime";
5858
* - cb1 is not assumed to be called since it's only used as a call operand
5959
*/
6060
function useFoo(t0) {
61-
const $ = _c(14);
62-
let arr1;
63-
let arr2;
61+
const $ = _c(13);
62+
const { arr1, arr2 } = t0;
6463
let t1;
65-
if ($[0] !== t0) {
66-
({ arr1, arr2 } = t0);
67-
let t2;
68-
if ($[4] !== arr1[0]) {
69-
t2 = (e) => arr1[0].value + e.value;
70-
$[4] = arr1[0];
71-
$[5] = t2;
72-
} else {
73-
t2 = $[5];
74-
}
75-
const cb1 = t2;
76-
t1 = () => arr1.map(cb1);
77-
$[0] = t0;
78-
$[1] = arr1;
79-
$[2] = arr2;
80-
$[3] = t1;
64+
if ($[0] !== arr1[0]) {
65+
t1 = (e) => arr1[0].value + e.value;
66+
$[0] = arr1[0];
67+
$[1] = t1;
8168
} else {
82-
arr1 = $[1];
83-
arr2 = $[2];
84-
t1 = $[3];
69+
t1 = $[1];
8570
}
86-
const getArrMap1 = t1;
71+
const cb1 = t1;
8772
let t2;
88-
if ($[6] !== arr2) {
89-
t2 = (e_0) => arr2[0].value + e_0.value;
90-
$[6] = arr2;
91-
$[7] = t2;
73+
if ($[2] !== arr1 || $[3] !== cb1) {
74+
t2 = () => arr1.map(cb1);
75+
$[2] = arr1;
76+
$[3] = cb1;
77+
$[4] = t2;
9278
} else {
93-
t2 = $[7];
79+
t2 = $[4];
9480
}
95-
const cb2 = t2;
81+
const getArrMap1 = t2;
9682
let t3;
97-
if ($[8] !== arr1 || $[9] !== cb2) {
98-
t3 = () => arr1.map(cb2);
99-
$[8] = arr1;
100-
$[9] = cb2;
101-
$[10] = t3;
83+
if ($[5] !== arr2) {
84+
t3 = (e_0) => arr2[0].value + e_0.value;
85+
$[5] = arr2;
86+
$[6] = t3;
10287
} else {
103-
t3 = $[10];
88+
t3 = $[6];
10489
}
105-
const getArrMap2 = t3;
90+
const cb2 = t3;
10691
let t4;
107-
if ($[11] !== getArrMap1 || $[12] !== getArrMap2) {
108-
t4 = (
92+
if ($[7] !== arr1 || $[8] !== cb2) {
93+
t4 = () => arr1.map(cb2);
94+
$[7] = arr1;
95+
$[8] = cb2;
96+
$[9] = t4;
97+
} else {
98+
t4 = $[9];
99+
}
100+
const getArrMap2 = t4;
101+
let t5;
102+
if ($[10] !== getArrMap1 || $[11] !== getArrMap2) {
103+
t5 = (
109104
<Stringify
110105
getArrMap1={getArrMap1}
111106
getArrMap2={getArrMap2}
112107
shouldInvokeFns={true}
113108
/>
114109
);
115-
$[11] = getArrMap1;
116-
$[12] = getArrMap2;
117-
$[13] = t4;
110+
$[10] = getArrMap1;
111+
$[11] = getArrMap2;
112+
$[12] = t5;
118113
} else {
119-
t4 = $[13];
114+
t5 = $[12];
120115
}
121-
return t4;
116+
return t5;
122117
}
123118

124119
export const FIXTURE_ENTRYPOINT = {

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-internal-compiler-shared-mutablerange-bug.expect.md‎

Lines changed: 80 additions & 0 deletions
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

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-internal-compiler-shared-mutablerange-bug.js‎

Lines changed: 25 additions & 0 deletions
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)