Fix scope merging in MergeOverlappingReactiveScopes
When `MergeOverlappingReactiveScopes` identified scopes to be merged, it was currently (oops, my bad) updating the _existing_ scope's id and range. Later, PropagateScopeDependencies marks outputs of a scope by updating that identifier's scope instance — so if that scope instance isn't shared, then the output is lost. This PR fixes MergeOverlappingReactiveScopes to correctly update all operands for a scope to have the same scope instance.
Joe Savona committed
Jan 19, 2023 at 10:10 UTC
d295bb45167f265f99d7481ce0f3c84484677db8
6 files changed
+116
-86
compiler/forget/src/ReactiveScopes/MergeOverlappingReactiveScopes.ts
+21
-13
@@ -106,6 +106,7 @@ export function mergeOverlappingReactiveScopes(fn: ReactiveFunction): void {
106
context.enter(() => {
107
visitBlock(context, fn.body);
108
});
109
+ context.complete();
110
}
111
112
function visitBlock(context: Context, block: ReactiveBlock): void {
@@ -205,6 +206,7 @@ class Context {
206
scopes: Array<BlockScope> = [];
207
seenScopes: Set<ScopeId> = new Set();
208
joinedScopes: DisjointSet<ReactiveScope> = new DisjointSet();
209
+ operandScopes: Map<Place, ReactiveScope> = new Map();
210
211
visitId(id: InstructionId): void {
212
const currentBlock = this.scopes[this.scopes.length - 1]!;
@@ -223,6 +225,7 @@ class Context {
225
if (scope === null) {
226
return;
227
}
228
+ this.operandScopes.set(place, scope);
229
const currentBlock = this.scopes[this.scopes.length - 1]!;
230
// Fast-path for the first time we see a new scope
231
if (!this.seenScopes.has(scope.id)) {
@@ -290,19 +293,24 @@ class Context {
293
this.scopes.push(new BlockScope());
294
fn();
295
this.scopes.pop();
293
- if (this.scopes.length === 0) {
294
- this.joinedScopes.forEach((scope, groupScope) => {
295
- if (scope !== groupScope) {
296
- groupScope.range.start = makeInstructionId(
297
- Math.min(groupScope.range.start, scope.range.start)
298
- );
299
- groupScope.range.end = makeInstructionId(
300
- Math.max(groupScope.range.end, scope.range.end)
301
- );
302
- scope.range = groupScope.range;
303
- scope.id = groupScope.id;
304
- }
305
- });
296
+ }
297
+
298
+ complete(): void {
299
+ this.joinedScopes.forEach((scope, groupScope) => {
300
+ if (scope !== groupScope) {
301
+ groupScope.range.start = makeInstructionId(
302
+ Math.min(groupScope.range.start, scope.range.start)
303
+ );
304
+ groupScope.range.end = makeInstructionId(
305
+ Math.max(groupScope.range.end, scope.range.end)
306
+ );
307
+ }
308
+ });
309
+ for (const [operand, originalScope] of this.operandScopes) {
310
+ const mergedScope = this.joinedScopes.find(originalScope);
311
+ if (mergedScope !== null) {
312
+ operand.identifier.scope = mergedScope;
313
+ }
314
}
315
}
316
}
compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.expect.md
+12
-5
@@ -17,11 +17,18 @@ function foo() {
17
18
```javascript
19
function foo() {
20
- const x = { x: 0 };
21
- const y = { z: 0 };
22
- const z = { z: 0 };
23
- x.x = x.x + (y.y = y.y * 1);
24
- z.z = z.z + (y.y = y.y * (x.x = x.x & 3));
20
+ const $ = React.useMemoCache();
21
+ let z;
22
+ if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
23
+ const x = { x: 0 };
24
+ const y = { z: 0 };
25
+ z = { z: 0 };
26
+ x.x = x.x + (y.y = y.y * 1);
27
+ z.z = z.z + (y.y = y.y * (x.x = x.x & 3));
28
+ $[0] = z;
29
+ } else {
30
+ z = $[0];
31
+ }
32
return z;
33
}
34
compiler/forget/src/__tests__/fixtures/hir/conditional-on-mutable.expect.md
+28
-22
@@ -40,9 +40,10 @@ function Component(props) {
40
const c_1 = $[1] !== props.p1;
41
const c_2 = $[2] !== props.p2;
42
let a;
43
+ let b;
44
if (c_0 || c_1 || c_2) {
45
a = [];
45
- const b = [];
46
+ b = [];
47
if (b) {
48
a.push(props.p0);
49
}
@@ -53,21 +54,23 @@ function Component(props) {
54
$[1] = props.p1;
55
$[2] = props.p2;
56
$[3] = a;
57
+ $[4] = b;
58
} else {
59
a = $[3];
60
+ b = $[4];
61
}
59
- const c_4 = $[4] !== a;
60
- const c_5 = $[5] !== b;
61
- let t6;
62
- if (c_4 || c_5) {
63
- t6 = <Foo a={a} b={b}></Foo>;
64
- $[4] = a;
65
- $[5] = b;
66
- $[6] = t6;
62
+ const c_5 = $[5] !== a;
63
+ const c_6 = $[6] !== b;
64
+ let t7;
65
+ if (c_5 || c_6) {
66
+ t7 = <Foo a={a} b={b}></Foo>;
67
+ $[5] = a;
68
+ $[6] = b;
69
+ $[7] = t7;
70
} else {
68
- t6 = $[6];
71
+ t7 = $[7];
72
}
70
- return t6;
73
+ return t7;
74
}
75
76
function Component(props) {
@@ -76,9 +79,10 @@ function Component(props) {
79
const c_1 = $[1] !== props.p1;
80
const c_2 = $[2] !== props.p2;
81
let a;
82
+ let b;
83
if (c_0 || c_1 || c_2) {
84
a = [];
81
- const b = [];
85
+ b = [];
86
if (mayMutate(b)) {
87
a.push(props.p0);
88
}
@@ -89,21 +93,23 @@ function Component(props) {
93
$[1] = props.p1;
94
$[2] = props.p2;
95
$[3] = a;
96
+ $[4] = b;
97
} else {
98
a = $[3];
99
+ b = $[4];
100
}
95
- const c_4 = $[4] !== a;
96
- const c_5 = $[5] !== b;
97
- let t6;
98
- if (c_4 || c_5) {
99
- t6 = <Foo a={a} b={b}></Foo>;
100
- $[4] = a;
101
- $[5] = b;
102
- $[6] = t6;
101
+ const c_5 = $[5] !== a;
102
+ const c_6 = $[6] !== b;
103
+ let t7;
104
+ if (c_5 || c_6) {
105
+ t7 = <Foo a={a} b={b}></Foo>;
106
+ $[5] = a;
107
+ $[6] = b;
108
+ $[7] = t7;
109
} else {
104
- t6 = $[6];
110
+ t7 = $[7];
111
}
106
- return t6;
112
+ return t7;
113
}
114
115
function Foo() {}
compiler/forget/src/__tests__/fixtures/hir/independent-across-if.expect.md
+14
-11
@@ -61,9 +61,10 @@ function Component(props) {
61
const c_1 = $[1] !== props.b;
62
const c_2 = $[2] !== props.c;
63
let a;
64
+ let b;
65
if (c_0 || c_1 || c_2) {
66
a = compute(props.a);
66
- const b = compute(props.b);
67
+ b = compute(props.b);
68
if (props.c) {
69
mutate(a);
70
mutate(b);
@@ -72,21 +73,23 @@ function Component(props) {
73
$[1] = props.b;
74
$[2] = props.c;
75
$[3] = a;
76
+ $[4] = b;
77
} else {
78
a = $[3];
79
+ b = $[4];
80
}
78
- const c_4 = $[4] !== a;
79
- const c_5 = $[5] !== b;
80
- let t6;
81
- if (c_4 || c_5) {
82
- t6 = <Foo a={a} b={b}></Foo>;
83
- $[4] = a;
84
- $[5] = b;
85
- $[6] = t6;
81
+ const c_5 = $[5] !== a;
82
+ const c_6 = $[6] !== b;
83
+ let t7;
84
+ if (c_5 || c_6) {
85
+ t7 = <Foo a={a} b={b}></Foo>;
86
+ $[5] = a;
87
+ $[6] = b;
88
+ $[7] = t7;
89
} else {
87
- t6 = $[6];
90
+ t7 = $[7];
91
}
89
- return t6;
92
+ return t7;
93
}
94
95
```
compiler/forget/src/__tests__/fixtures/hir/switch-non-final-default.expect.md
+22
-19
@@ -36,10 +36,11 @@ function Component(props) {
36
const c_0 = $[0] !== props.p0;
37
const c_1 = $[1] !== props.p2;
38
let x;
39
+ let y$0;
40
if (c_0 || c_1) {
41
x = [];
42
const y = undefined;
42
- let y$0 = y;
43
+ y$0 = y;
44
bb1: switch (props.p0) {
45
case 1: {
46
break bb1;
@@ -47,11 +48,11 @@ function Component(props) {
48
case true: {
49
x.push(props.p2);
50
let y$1;
50
- if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
51
+ if ($[4] === Symbol.for("react.memo_cache_sentinel")) {
52
y$1 = [];
52
- $[3] = y$1;
53
+ $[4] = y$1;
54
} else {
54
- y$1 = $[3];
55
+ y$1 = $[4];
56
}
57
y$0 = y$1;
58
break bb1;
@@ -67,31 +68,33 @@ function Component(props) {
68
$[0] = props.p0;
69
$[1] = props.p2;
70
$[2] = x;
71
+ $[3] = y$0;
72
} else {
73
x = $[2];
74
+ y$0 = $[3];
75
}
73
- const c_4 = $[4] !== x;
76
+ const c_5 = $[5] !== x;
77
let child;
75
- if (c_4) {
78
+ if (c_5) {
79
child = <Component data={x}></Component>;
77
- $[4] = x;
78
- $[5] = child;
80
+ $[5] = x;
81
+ $[6] = child;
82
} else {
80
- child = $[5];
83
+ child = $[6];
84
}
85
y$0.push(props.p4);
83
- const c_6 = $[6] !== y$0;
84
- const c_7 = $[7] !== child;
85
- let t8;
86
- if (c_6 || c_7) {
87
- t8 = <Component data={y$0}>{child}</Component>;
88
- $[6] = y$0;
89
- $[7] = child;
90
- $[8] = t8;
86
+ const c_7 = $[7] !== y$0;
87
+ const c_8 = $[8] !== child;
88
+ let t9;
89
+ if (c_7 || c_8) {
90
+ t9 = <Component data={y$0}>{child}</Component>;
91
+ $[7] = y$0;
92
+ $[8] = child;
93
+ $[9] = t9;
94
} else {
92
- t8 = $[8];
95
+ t9 = $[9];
96
}
94
- return t8;
97
+ return t9;
98
}
99
100
```
compiler/forget/src/__tests__/fixtures/hir/switch.expect.md
+19
-16
@@ -32,10 +32,11 @@ function Component(props) {
32
const c_1 = $[1] !== props.p2;
33
const c_2 = $[2] !== props.p3;
34
let x;
35
+ let y$0;
36
if (c_0 || c_1 || c_2) {
37
x = [];
38
const y = undefined;
38
- let y$0 = y;
39
+ y$0 = y;
40
switch (props.p0) {
41
case true: {
42
x.push(props.p2);
@@ -51,31 +52,33 @@ function Component(props) {
52
$[1] = props.p2;
53
$[2] = props.p3;
54
$[3] = x;
55
+ $[4] = y$0;
56
} else {
57
x = $[3];
58
+ y$0 = $[4];
59
}
57
- const c_4 = $[4] !== x;
60
+ const c_5 = $[5] !== x;
61
let child;
59
- if (c_4) {
62
+ if (c_5) {
63
child = <Component data={x}></Component>;
61
- $[4] = x;
62
- $[5] = child;
64
+ $[5] = x;
65
+ $[6] = child;
66
} else {
64
- child = $[5];
67
+ child = $[6];
68
}
69
y$0.push(props.p4);
67
- const c_6 = $[6] !== y$0;
68
- const c_7 = $[7] !== child;
69
- let t8;
70
- if (c_6 || c_7) {
71
- t8 = <Component data={y$0}>{child}</Component>;
72
- $[6] = y$0;
73
- $[7] = child;
74
- $[8] = t8;
70
+ const c_7 = $[7] !== y$0;
71
+ const c_8 = $[8] !== child;
72
+ let t9;
73
+ if (c_7 || c_8) {
74
+ t9 = <Component data={y$0}>{child}</Component>;
75
+ $[7] = y$0;
76
+ $[8] = child;
77
+ $[9] = t9;
78
} else {
76
- t8 = $[8];
79
+ t9 = $[9];
80
}
78
- return t8;
81
+ return t9;
82
}
83
84
```