Skip to content

Commit 5d64f74

Browse files
authored
[compiler] Fix for scopes with unreachable fallthroughs (facebook#34335)
Fixes facebook#34108. If a scope ends with with a conditional where some/all branches exit via labeled break, we currently compile in a way that works but bypasses memoization. We end up with a shape like ```js let t0; label: { if (changed) { ... if (cond) { t0 = ...; break label; } // we don't save the output if the break happens! t0 = ...; $[0] = t0; } else { t0 = $[0]; } ``` The fix here is to update AlignReactiveScopesToBlockScopes to take account of breaks that don't go to the natural fallthrough. In this case, we take any active scopes and extend them to start at least as early as the label, and extend at least to the label fallthrough. Thus we produce the correct: ```js let t0; if (changed) { label: { ... if (cond) { t0 = ...; break label; } t0 = ...; } // now the break jumps here, and we cache the value $[0] = t0; } else { t0 = $[0]; } ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34335). * facebook#34347 * facebook#34346 * facebook#34343 * __->__ facebook#34335
1 parent 3302d1f commit 5d64f74

File tree

7 files changed

+241
-56
lines changed

7 files changed

+241
-56
lines changed

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,41 @@ export function alignReactiveScopesToBlockScopesHIR(fn: HIRFunction): void {
175175
if (node != null) {
176176
valueBlockNodes.set(fallthrough, node);
177177
}
178+
} else if (terminal.kind === 'goto') {
179+
/**
180+
* If we encounter a goto that is not to the natural fallthrough of the current
181+
* block (not the topmost fallthrough on the stack), then this is a goto to a
182+
* label. Any scopes that extend beyond the goto must be extended to include
183+
* the labeled range, so that the break statement doesn't accidentally jump
184+
* out of the scope. We do this by extending the start and end of the scope's
185+
* range to the label and its fallthrough respectively.
186+
*/
187+
const start = activeBlockFallthroughRanges.find(
188+
range => range.fallthrough === terminal.block,
189+
);
190+
if (start != null && start !== activeBlockFallthroughRanges.at(-1)) {
191+
const fallthroughBlock = fn.body.blocks.get(start.fallthrough)!;
192+
const firstId =
193+
fallthroughBlock.instructions[0]?.id ?? fallthroughBlock.terminal.id;
194+
for (const scope of activeScopes) {
195+
/**
196+
* activeScopes is only filtered at block start points, so some of the
197+
* scopes may not actually be active anymore, ie we've past their end
198+
* instruction. Only extend ranges for scopes that are actually active.
199+
*
200+
* TODO: consider pruning activeScopes per instruction
201+
*/
202+
if (scope.range.end <= terminal.id) {
203+
continue;
204+
}
205+
scope.range.start = makeInstructionId(
206+
Math.min(start.range.start, scope.range.start),
207+
);
208+
scope.range.end = makeInstructionId(
209+
Math.max(firstId, scope.range.end),
210+
);
211+
}
212+
}
178213
}
179214

180215
/*

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-reactive-scope-overlaps-if.expect.md

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,14 +46,16 @@ function useFoo(t0) {
4646
t1 = $[0];
4747
}
4848
let items = t1;
49-
bb0: if ($[1] !== cond) {
50-
if (cond) {
51-
items = [];
52-
} else {
53-
break bb0;
54-
}
49+
if ($[1] !== cond) {
50+
bb0: {
51+
if (cond) {
52+
items = [];
53+
} else {
54+
break bb0;
55+
}
5556

56-
items.push(2);
57+
items.push(2);
58+
}
5759
$[1] = cond;
5860
$[2] = items;
5961
} else {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutation-within-jsx-and-break.expect.md

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -49,26 +49,27 @@ import {
4949
} from "shared-runtime";
5050

5151
function useFoo(t0) {
52-
const $ = _c(3);
52+
const $ = _c(4);
5353
const { data } = t0;
5454
let obj;
5555
let myDiv = null;
56-
bb0: if (data.cond) {
57-
if ($[0] !== data.cond1) {
56+
if ($[0] !== data.cond || $[1] !== data.cond1) {
57+
bb0: if (data.cond) {
5858
obj = makeObject_Primitives();
5959
if (data.cond1) {
6060
myDiv = <Stringify value={mutateAndReturn(obj)} />;
6161
break bb0;
6262
}
6363

6464
mutate(obj);
65-
$[0] = data.cond1;
66-
$[1] = obj;
67-
$[2] = myDiv;
68-
} else {
69-
obj = $[1];
70-
myDiv = $[2];
7165
}
66+
$[0] = data.cond;
67+
$[1] = data.cond1;
68+
$[2] = obj;
69+
$[3] = myDiv;
70+
} else {
71+
obj = $[2];
72+
myDiv = $[3];
7273
}
7374
return myDiv;
7475
}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/useMemo-multiple-if-else.expect.md

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -34,17 +34,16 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR
3434
import { useMemo } from "react";
3535

3636
function Component(props) {
37-
const $ = _c(6);
37+
const $ = _c(5);
3838
let t0;
39-
bb0: {
40-
let y;
41-
if (
42-
$[0] !== props.a ||
43-
$[1] !== props.b ||
44-
$[2] !== props.cond ||
45-
$[3] !== props.cond2
46-
) {
47-
y = [];
39+
if (
40+
$[0] !== props.a ||
41+
$[1] !== props.b ||
42+
$[2] !== props.cond ||
43+
$[3] !== props.cond2
44+
) {
45+
bb0: {
46+
const y = [];
4847
if (props.cond) {
4948
y.push(props.a);
5049
}
@@ -54,17 +53,15 @@ function Component(props) {
5453
}
5554

5655
y.push(props.b);
57-
$[0] = props.a;
58-
$[1] = props.b;
59-
$[2] = props.cond;
60-
$[3] = props.cond2;
61-
$[4] = y;
62-
$[5] = t0;
63-
} else {
64-
y = $[4];
65-
t0 = $[5];
56+
t0 = y;
6657
}
67-
t0 = y;
58+
$[0] = props.a;
59+
$[1] = props.b;
60+
$[2] = props.cond;
61+
$[3] = props.cond2;
62+
$[4] = t0;
63+
} else {
64+
t0 = $[4];
6865
}
6966
const x = t0;
7067
return x;
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {useMemo} from 'react';
6+
import {
7+
makeObject_Primitives,
8+
mutate,
9+
Stringify,
10+
ValidateMemoization,
11+
} from 'shared-runtime';
12+
13+
function Component({cond}) {
14+
const memoized = useMemo(() => {
15+
const value = makeObject_Primitives();
16+
if (cond) {
17+
return value;
18+
} else {
19+
mutate(value);
20+
return value;
21+
}
22+
}, [cond]);
23+
return <ValidateMemoization inputs={[cond]} output={memoized} />;
24+
}
25+
26+
export const FIXTURE_ENTRYPOINT = {
27+
fn: Component,
28+
params: [{cond: false}],
29+
sequentialRenders: [
30+
{cond: false},
31+
{cond: false},
32+
{cond: true},
33+
{cond: true},
34+
{cond: false},
35+
{cond: true},
36+
{cond: false},
37+
{cond: true},
38+
],
39+
};
40+
41+
```
42+
43+
## Code
44+
45+
```javascript
46+
import { c as _c } from "react/compiler-runtime";
47+
import { useMemo } from "react";
48+
import {
49+
makeObject_Primitives,
50+
mutate,
51+
Stringify,
52+
ValidateMemoization,
53+
} from "shared-runtime";
54+
55+
function Component(t0) {
56+
const $ = _c(7);
57+
const { cond } = t0;
58+
let t1;
59+
if ($[0] !== cond) {
60+
const value = makeObject_Primitives();
61+
if (cond) {
62+
t1 = value;
63+
} else {
64+
mutate(value);
65+
t1 = value;
66+
}
67+
$[0] = cond;
68+
$[1] = t1;
69+
} else {
70+
t1 = $[1];
71+
}
72+
const memoized = t1;
73+
let t2;
74+
if ($[2] !== cond) {
75+
t2 = [cond];
76+
$[2] = cond;
77+
$[3] = t2;
78+
} else {
79+
t2 = $[3];
80+
}
81+
let t3;
82+
if ($[4] !== memoized || $[5] !== t2) {
83+
t3 = <ValidateMemoization inputs={t2} output={memoized} />;
84+
$[4] = memoized;
85+
$[5] = t2;
86+
$[6] = t3;
87+
} else {
88+
t3 = $[6];
89+
}
90+
return t3;
91+
}
92+
93+
export const FIXTURE_ENTRYPOINT = {
94+
fn: Component,
95+
params: [{ cond: false }],
96+
sequentialRenders: [
97+
{ cond: false },
98+
{ cond: false },
99+
{ cond: true },
100+
{ cond: true },
101+
{ cond: false },
102+
{ cond: true },
103+
{ cond: false },
104+
{ cond: true },
105+
],
106+
};
107+
108+
```
109+
110+
### Eval output
111+
(kind: ok) <div>{"inputs":[false],"output":{"a":0,"b":"value1","c":true,"wat0":"joe"}}</div>
112+
<div>{"inputs":[false],"output":{"a":0,"b":"value1","c":true,"wat0":"joe"}}</div>
113+
<div>{"inputs":[true],"output":{"a":0,"b":"value1","c":true}}</div>
114+
<div>{"inputs":[true],"output":{"a":0,"b":"value1","c":true}}</div>
115+
<div>{"inputs":[false],"output":{"a":0,"b":"value1","c":true,"wat0":"joe"}}</div>
116+
<div>{"inputs":[true],"output":{"a":0,"b":"value1","c":true}}</div>
117+
<div>{"inputs":[false],"output":{"a":0,"b":"value1","c":true,"wat0":"joe"}}</div>
118+
<div>{"inputs":[true],"output":{"a":0,"b":"value1","c":true}}</div>
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import {useMemo} from 'react';
2+
import {
3+
makeObject_Primitives,
4+
mutate,
5+
Stringify,
6+
ValidateMemoization,
7+
} from 'shared-runtime';
8+
9+
function Component({cond}) {
10+
const memoized = useMemo(() => {
11+
const value = makeObject_Primitives();
12+
if (cond) {
13+
return value;
14+
} else {
15+
mutate(value);
16+
return value;
17+
}
18+
}, [cond]);
19+
return <ValidateMemoization inputs={[cond]} output={memoized} />;
20+
}
21+
22+
export const FIXTURE_ENTRYPOINT = {
23+
fn: Component,
24+
params: [{cond: false}],
25+
sequentialRenders: [
26+
{cond: false},
27+
{cond: false},
28+
{cond: true},
29+
{cond: true},
30+
{cond: false},
31+
{cond: true},
32+
{cond: false},
33+
{cond: true},
34+
],
35+
};

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-multiple-if-else.expect.md

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,16 @@ import { c as _c } from "react/compiler-runtime";
3333
import { useMemo } from "react";
3434

3535
function Component(props) {
36-
const $ = _c(6);
36+
const $ = _c(5);
3737
let t0;
38-
bb0: {
39-
let y;
40-
if (
41-
$[0] !== props.a ||
42-
$[1] !== props.b ||
43-
$[2] !== props.cond ||
44-
$[3] !== props.cond2
45-
) {
46-
y = [];
38+
if (
39+
$[0] !== props.a ||
40+
$[1] !== props.b ||
41+
$[2] !== props.cond ||
42+
$[3] !== props.cond2
43+
) {
44+
bb0: {
45+
const y = [];
4746
if (props.cond) {
4847
y.push(props.a);
4948
}
@@ -53,17 +52,15 @@ function Component(props) {
5352
}
5453

5554
y.push(props.b);
56-
$[0] = props.a;
57-
$[1] = props.b;
58-
$[2] = props.cond;
59-
$[3] = props.cond2;
60-
$[4] = y;
61-
$[5] = t0;
62-
} else {
63-
y = $[4];
64-
t0 = $[5];
55+
t0 = y;
6556
}
66-
t0 = y;
57+
$[0] = props.a;
58+
$[1] = props.b;
59+
$[2] = props.cond;
60+
$[3] = props.cond2;
61+
$[4] = t0;
62+
} else {
63+
t0 = $[4];
6764
}
6865
const x = t0;
6966
return x;

0 commit comments

Comments
 (0)