Fix temporaries accessed outside of their defining scope
Fixed a bug identified in repro cases earlier in the stack. The case is where some later value is composed of several values, say A and B, where A is an identifier that is reassigned within B. Also, the mutable range of B surrounds the evaluation of A. In this case, the reference to A gets lowered to a temporary (say a t0 = LoadLocal A), and that temporary is created within the reactive scope for B. PropagateScopeDependencies bypasses LoadLocal indirections, and considers the reference to the temporary (t0) as if it was a reference to the identifier (A). That breaks the whole reason we lower Identifiers to temporaries - to preserve evaluation order. This PR fixes the bug by promoting temporaries to names values if they are referenced outside their defining scope. So, the reference to t0 stays a reference to t0, which correctly preserves the value of A at the right point in time. This is all much easier to see in the new test case.
Joe Savona committed
Apr 20, 2023 at 18:52 UTC
f4896b45b2ff4c7f10bf710c9c9d2b87d6352e0d
6 files changed
+161
-33
compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
+20
-5
@@ -71,7 +71,8 @@ class Context {
71
// ReactiveScope (B) that uses the produced temporary.
72
// - codegen will inline these PropertyLoads back into scope (B)
73
#properties: Map<Identifier, ReactiveScopePropertyDependency> = new Map();
74
- #temporaries: Map<Identifier, Place> = new Map();
74
+ #temporaries: Map<Identifier, { place: Place; scope: ReactiveScope | null }> =
75
+ new Map();
76
#inConditionalWithinScope: boolean = false;
77
// Reactive dependencies used unconditionally in the current conditional.
78
// Composed of dependencies:
@@ -184,8 +185,22 @@ class Context {
185
this.#reassignments.set(identifier, decl);
186
}
187
187
- declareTemporary(lvalue: Place, value: Place): void {
188
- this.#temporaries.set(lvalue.identifier, value);
188
+ declareTemporary(lvalue: Place, place: Place): void {
189
+ this.#temporaries.set(lvalue.identifier, {
190
+ place,
191
+ scope: this.currentScope.value,
192
+ });
193
+ }
194
+
195
+ resolveTemporary(place: Place): Place {
196
+ const temporary = this.#temporaries.get(place.identifier);
197
+ if (
198
+ temporary !== undefined &&
199
+ (temporary.scope === null || this.#isScopeActive(temporary.scope))
200
+ ) {
201
+ return temporary.place;
202
+ }
203
+ return place;
204
}
205
206
#getProperty(
@@ -193,7 +208,7 @@ class Context {
208
property: string,
209
isConditional: boolean
210
): ReactiveScopePropertyDependency {
196
- const resolvedObject = this.#temporaries.get(object.identifier) ?? object;
211
+ const resolvedObject = this.resolveTemporary(object);
212
const resolvedDependency = this.#properties.get(resolvedObject.identifier);
213
let objectDependency: ReactiveScopePropertyDependency;
214
// (1) Create the base property dependency as either a LoadLocal (from a temporary)
@@ -267,7 +282,7 @@ class Context {
282
}
283
284
visitOperand(place: Place): void {
270
- const resolved = this.#temporaries.get(place.identifier) ?? place;
285
+ const resolved = this.resolveTemporary(place);
286
// if this operand is a temporary created for a property load, try to resolve it to
287
// the expanded Place. Fall back to using the operand as-is.
288
compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.expect.md
deleted
-28
@@ -1,28 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-function Component(props) {
6
- const maybeMutable = new MaybeMutable();
7
- let Tag = props.component;
8
- // NOTE: the order of evaluation in the lowering is incorrect:
9
- // the jsx element's tag observes `Tag` after reassignment, but should observe
10
- // it before the reassignment.
11
- return (
12
- <Tag>
13
- {((Tag = props.alternateComponent), maybeMutate(maybeMutable))}
14
- <Tag />
15
- </Tag>
16
- );
17
-}
18
-
19
-```
20
-
21
-
22
-## Error
23
-
24
-```
25
-[ReactForget] Invariant: [Codegen] No value found for temporary. Value for 'read $33' was not set in the codegen context (8:8)
26
-```
27
-
28
-
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.expect.md
new
+87
@@ -0,0 +1,87 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const maybeMutable = new MaybeMutable();
7
+ let Tag = props.component;
8
+ // NOTE: the order of evaluation in the lowering is incorrect:
9
+ // the jsx element's tag observes `Tag` after reassignment, but should observe
10
+ // it before the reassignment.
11
+ return (
12
+ <Tag>
13
+ {((Tag = props.alternateComponent), maybeMutate(maybeMutable))}
14
+ <Tag />
15
+ </Tag>
16
+ );
17
+}
18
+
19
+```
20
+
21
+## Code
22
+
23
+```javascript
24
+import * as React from "react";
25
+function Component(props) {
26
+ const $ = React.unstable_useMemoCache(13);
27
+ const c_0 = $[0] !== props.component;
28
+ const c_1 = $[1] !== props.alternateComponent;
29
+ let Tag;
30
+ let t0;
31
+ let t1;
32
+ let t2;
33
+ if (c_0 || c_1) {
34
+ const maybeMutable = new MaybeMutable();
35
+ Tag = props.component;
36
+
37
+ t0 = Tag;
38
+ t1 = "\n ";
39
+ Tag = props.alternateComponent;
40
+ t2 = maybeMutate(maybeMutable);
41
+ $[0] = props.component;
42
+ $[1] = props.alternateComponent;
43
+ $[2] = Tag;
44
+ $[3] = t0;
45
+ $[4] = t1;
46
+ $[5] = t2;
47
+ } else {
48
+ Tag = $[2];
49
+ t0 = $[3];
50
+ t1 = $[4];
51
+ t2 = $[5];
52
+ }
53
+ const c_6 = $[6] !== Tag;
54
+ let t3;
55
+ if (c_6) {
56
+ t3 = <Tag />;
57
+ $[6] = Tag;
58
+ $[7] = t3;
59
+ } else {
60
+ t3 = $[7];
61
+ }
62
+ const c_8 = $[8] !== t0;
63
+ const c_9 = $[9] !== t1;
64
+ const c_10 = $[10] !== t2;
65
+ const c_11 = $[11] !== t3;
66
+ let t4;
67
+ if (c_8 || c_9 || c_10 || c_11) {
68
+ t4 = (
69
+ <t0>
70
+ {t1}
71
+ {t2}
72
+ {t3}
73
+ </t0>
74
+ );
75
+ $[8] = t0;
76
+ $[9] = t1;
77
+ $[10] = t2;
78
+ $[11] = t3;
79
+ $[12] = t4;
80
+ } else {
81
+ t4 = $[12];
82
+ }
83
+ return t4;
84
+}
85
+
86
+```
87
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.js
renamed
compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.expect.md
new
+49
@@ -0,0 +1,49 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const maybeMutable = new MaybeMutable();
7
+ let x = props.value;
8
+ return [x, maybeMutate(maybeMutable)];
9
+}
10
+
11
+```
12
+
13
+## Code
14
+
15
+```javascript
16
+import * as React from "react";
17
+function Component(props) {
18
+ const $ = React.unstable_useMemoCache(6);
19
+ const c_0 = $[0] !== props.value;
20
+ let t0;
21
+ let t1;
22
+ if (c_0) {
23
+ const maybeMutable = new MaybeMutable();
24
+ const x = props.value;
25
+ t0 = x;
26
+ t1 = maybeMutate(maybeMutable);
27
+ $[0] = props.value;
28
+ $[1] = t0;
29
+ $[2] = t1;
30
+ } else {
31
+ t0 = $[1];
32
+ t1 = $[2];
33
+ }
34
+ const c_3 = $[3] !== t0;
35
+ const c_4 = $[4] !== t1;
36
+ let t2;
37
+ if (c_3 || c_4) {
38
+ t2 = [t0, t1];
39
+ $[3] = t0;
40
+ $[4] = t1;
41
+ $[5] = t2;
42
+ } else {
43
+ t2 = $[5];
44
+ }
45
+ return t2;
46
+}
47
+
48
+```
49
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.js
new
+5
@@ -0,0 +1,5 @@
1
+function Component(props) {
2
+ const maybeMutable = new MaybeMutable();
3
+ let x = props.value;
4
+ return [x, maybeMutate(maybeMutable)];
5
+}