[rhir] Represent OptionalMemberExpression as a conditional dependency
--- Every `OptionalMemberExpression` rvalue has the form `<requiredPath>?.<optionalPath>`. ``` // required = [a], optional: [b, c] props.a?.b.c; props.a?.b?.c; ``` When calculating reactive dependencies, recall that it is always correct to add a subpath of a dependency (e.g. we can always take `props.a` instead of `props.a.b` as a dependency). See comments in `DeriveMinimalDependencies` for a longer explanation. There are two ways we can deal with `OptionalMemberExpression`: - We can always truncate a OptionalMemberExpression dependency to its `requiredPath`, taking only the required path as a dependency. - this is the simpler approach, but it potentially loses granularity. e.g. ``` // here, since props.a is already unconditionally accessed, // we can safely add props.a.b as a dependency and preserve both // nullthrows and the correct dependency set. scope @0 { let x = []; x.push(props.a?.b); x.push(props.a.b); } ``` (See added test case `reduce-reactive-cond-memberexpr-join` + its comment block for a more detailed explanation` - (the approach taken by this PR) We can add the `requiredPath` as a potentially unconditional access (dependent on other control flow) and `requiredPath + optionalPath` as a conditional dependency.
Mofei Zhang committed
Mar 27, 2023 at 10:41 UTC
67b2a9f314bae522859cc7e0e67076d09e83524b
7 files changed
+254
-49
compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts
+72
-17
@@ -3,6 +3,25 @@ import { Identifier, ReactiveScopeDependency } from "../HIR";
3
import { printIdentifier } from "../HIR/PrintHIR";
4
import { assertExhaustive } from "../Utils/utils";
5
6
+/**
7
+ * We need to understand optional member expressions only when determining
8
+ * dependencies of a ReactiveScope (i.e. in {@link PropagateScopeDependencies}),
9
+ * hence why this type lives here (not in HIR.ts)
10
+ *
11
+ * {@link ReactiveScopePropertyDependency.optionalPath} is populated only if the Property
12
+ * represents an optional member expression, and it represents the property path
13
+ * loaded conditionally.
14
+ * e.g. the member expr a.b.c?.d.e?.f is represented as
15
+ * {
16
+ * identifier: 'a';
17
+ * path: ['b', 'c'],
18
+ * optionalPath: ['d', 'e', 'f'].
19
+ * }
20
+ */
21
+export type ReactiveScopePropertyDependency = ReactiveScopeDependency & {
22
+ optionalPath: Array<string>;
23
+};
24
+
25
/**
26
* Finalizes a set of ReactiveScopeDependencies to produce a set of minimal unconditional
27
* dependencies, preserving granular accesses when possible.
@@ -42,34 +61,54 @@ export class ReactiveScopeDependencyTree {
61
return rootNode;
62
}
63
45
- add(dep: ReactiveScopeDependency, inConditional: boolean): void {
46
- const { path } = dep;
64
+ add(dep: ReactiveScopePropertyDependency, inConditional: boolean): void {
65
+ const { path, optionalPath } = dep;
66
let currNode = this.#getOrCreateRoot(dep.identifier);
67
68
const accessType = inConditional
69
? PropertyAccessType.ConditionalAccess
70
: PropertyAccessType.UnconditionalAccess;
52
- const depType = inConditional
53
- ? PropertyAccessType.ConditionalDependency
54
- : PropertyAccessType.UnconditionalDependency;
71
72
for (const property of path) {
73
// all properties read 'on the way' to a dependency are marked as 'access'
58
- let currChild = currNode.properties.get(property);
59
- if (currChild == null) {
60
- currChild = {
61
- properties: new Map(),
62
- accessType,
63
- };
64
- currNode.properties.set(property, currChild);
65
- } else {
66
- currChild.accessType = merge(currChild.accessType, accessType);
67
- }
74
+ let currChild = getOrMakeProperty(currNode, property);
75
+ currChild.accessType = merge(currChild.accessType, accessType);
76
currNode = currChild;
77
}
78
71
- // final property read should be marked as `dependency`
72
- currNode.accessType = merge(currNode.accessType, depType);
79
+ if (optionalPath.length === 0) {
80
+ // If this property does not have a conditional path (i.e. a.b.c), the
81
+ // final property node should be marked as an conditional/unconditional
82
+ // `dependency` as based on control flow.
83
+ const depType = inConditional
84
+ ? PropertyAccessType.ConditionalDependency
85
+ : PropertyAccessType.UnconditionalDependency;
86
+
87
+ currNode.accessType = merge(currNode.accessType, depType);
88
+ } else {
89
+ // Technically, we only depend on whether unconditional path `dep.path`
90
+ // is nullish (not its actual value). As long as we preserve the nullthrows
91
+ // behavior of `dep.path`, we can keep it as an access (and not promote
92
+ // to a dependency).
93
+ // See test `reduce-reactive-cond-memberexpr-join` for example.
94
+
95
+ // If this property has an optional path (i.e. a?.b.c), all optional
96
+ // nodes should be marked accordingly.
97
+ for (const property of optionalPath) {
98
+ let currChild = getOrMakeProperty(currNode, property);
99
+ currChild.accessType = merge(
100
+ currChild.accessType,
101
+ PropertyAccessType.ConditionalAccess
102
+ );
103
+ currNode = currChild;
104
+ }
105
+
106
+ // The final node should be marked as a conditional dependency.
107
+ currNode.accessType = merge(
108
+ currNode.accessType,
109
+ PropertyAccessType.ConditionalDependency
110
+ );
111
+ }
112
}
113
114
deriveMinimalDependencies(): Set<ReactiveScopeDependency> {
@@ -176,6 +215,7 @@ enum PropertyAccessType {
215
UnconditionalDependency = "UnconditionalDependency",
216
}
217
218
+const MIN_ACCESS_TYPE = PropertyAccessType.ConditionalAccess;
219
function isUnconditional(access: PropertyAccessType): boolean {
220
return (
221
access === PropertyAccessType.UnconditionalAccess ||
@@ -455,6 +495,21 @@ function printSubtree(
495
return results;
496
}
497
498
+function getOrMakeProperty(
499
+ node: DependencyNode,
500
+ property: string
501
+): DependencyNode {
502
+ let child = node.properties.get(property);
503
+ if (child == null) {
504
+ child = {
505
+ properties: new Map(),
506
+ accessType: MIN_ACCESS_TYPE,
507
+ };
508
+ node.properties.set(property, child);
509
+ }
510
+ return child;
511
+}
512
+
513
function mapNonNull<T extends NonNullable<V>, V, U>(
514
arr: Array<U>,
515
fn: (arg0: U) => T | undefined | null
compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
+58
-30
@@ -24,7 +24,10 @@ import {
24
eachPatternOperand,
25
} from "../HIR/visitors";
26
import { assertExhaustive } from "../Utils/utils";
27
-import { ReactiveScopeDependencyTree } from "./DeriveMinimalDependencies";
27
+import {
28
+ ReactiveScopeDependencyTree,
29
+ ReactiveScopePropertyDependency,
30
+} from "./DeriveMinimalDependencies";
31
32
/**
33
* Infers the dependencies of each scope to include variables whose values
@@ -68,7 +71,7 @@ class Context {
71
// - a ReactiveScope (A) containing a PropertyLoad may differ from the
72
// ReactiveScope (B) that uses the produced temporary.
73
// - codegen will inline these PropertyLoads back into scope (B)
71
- #properties: Map<Identifier, ReactiveScopeDependency> = new Map();
74
+ #properties: Map<Identifier, ReactiveScopePropertyDependency> = new Map();
75
#temporaries: Map<Identifier, Place> = new Map();
76
#inConditionalWithinScope: boolean = false;
77
// Reactive dependencies used unconditionally in the current conditional.
@@ -186,21 +189,53 @@ class Context {
189
this.#temporaries.set(lvalue.identifier, value);
190
}
191
189
- declareProperty(lvalue: Place, object: Place, property: string): void {
192
+ #getProperty(
193
+ object: Place,
194
+ property: string,
195
+ isConditional: boolean
196
+ ): ReactiveScopePropertyDependency {
197
const resolvedObject = this.#temporaries.get(object.identifier) ?? object;
191
- const objectDependency = this.#properties.get(resolvedObject.identifier);
192
- let nextDependency: ReactiveScopeDependency;
193
- if (objectDependency === undefined) {
194
- nextDependency = {
198
+ const resolvedDependency = this.#properties.get(resolvedObject.identifier);
199
+ let objectDependency: ReactiveScopePropertyDependency;
200
+ // (1) Create the base property dependency as either a LoadLocal (from a temporary)
201
+ // or a deep copy of an existing property dependency.
202
+ if (resolvedDependency === undefined) {
203
+ objectDependency = {
204
identifier: resolvedObject.identifier,
196
- path: [property],
205
+ path: [],
206
+ optionalPath: [],
207
};
208
} else {
199
- nextDependency = {
200
- identifier: objectDependency.identifier,
201
- path: [...objectDependency.path, property],
209
+ objectDependency = {
210
+ identifier: resolvedDependency.identifier,
211
+ path: [...resolvedDependency.path],
212
+ optionalPath: [...resolvedDependency.optionalPath],
213
};
214
}
215
+
216
+ // (2) Determine whether property is an optional access
217
+ if (objectDependency.optionalPath.length > 0) {
218
+ // If the base property dependency represents a optional member expression,
219
+ // property is on the optionalPath (regardless of whether this PropertyLoad
220
+ // itself was conditional)
221
+ // e.g. for `a.b?.c.d`, `d` should be added to optionalPath
222
+ objectDependency.optionalPath.push(property);
223
+ } else if (isConditional) {
224
+ objectDependency.optionalPath.push(property);
225
+ } else {
226
+ objectDependency.path.push(property);
227
+ }
228
+
229
+ return objectDependency;
230
+ }
231
+
232
+ declareProperty(
233
+ lvalue: Place,
234
+ object: Place,
235
+ property: string,
236
+ isConditional: boolean
237
+ ): void {
238
+ const nextDependency = this.#getProperty(object, property, isConditional);
239
this.#properties.set(lvalue.identifier, nextDependency);
240
}
241
@@ -234,9 +269,10 @@ class Context {
269
// if this operand is a temporary created for a property load, try to resolve it to
270
// the expanded Place. Fall back to using the operand as-is.
271
237
- let dependency: ReactiveScopeDependency = {
272
+ let dependency: ReactiveScopePropertyDependency = {
273
identifier: resolved.identifier,
274
path: [],
275
+ optionalPath: [],
276
};
277
if (resolved.identifier.name === null) {
278
const propertyDependency = this.#properties.get(resolved.identifier);
@@ -247,25 +283,12 @@ class Context {
283
this.visitDependency(dependency);
284
}
285
250
- visitProperty(object: Place, property: string): void {
251
- const resolvedObject = this.#temporaries.get(object.identifier) ?? object;
252
- const objectDependency = this.#properties.get(resolvedObject.identifier);
253
- let nextDependency: ReactiveScopeDependency;
254
- if (objectDependency === undefined) {
255
- nextDependency = {
256
- identifier: resolvedObject.identifier,
257
- path: [property],
258
- };
259
- } else {
260
- nextDependency = {
261
- identifier: objectDependency.identifier,
262
- path: [...objectDependency.path, property],
263
- };
264
- }
286
+ visitProperty(object: Place, property: string, isConditional: boolean): void {
287
+ const nextDependency = this.#getProperty(object, property, isConditional);
288
this.visitDependency(nextDependency);
289
}
290
268
- visitDependency(maybeDependency: ReactiveScopeDependency): void {
291
+ visitDependency(maybeDependency: ReactiveScopePropertyDependency): void {
292
// Any value used after its originally defining scope has concluded must be added as an
293
// output of its defining scope. Regardless of whether its a const or not,
294
// some later code needs access to the value. If the current
@@ -493,9 +516,14 @@ function visitInstructionValue(
516
}
517
} else if (value.kind === "PropertyLoad") {
518
if (lvalue !== null) {
496
- context.declareProperty(lvalue, value.object, value.property);
519
+ context.declareProperty(
520
+ lvalue,
521
+ value.object,
522
+ value.property,
523
+ value.optional
524
+ );
525
} else {
498
- context.visitProperty(value.object, value.property);
526
+ context.visitProperty(value.object, value.property, value.optional);
527
}
528
} else if (value.kind === "StoreLocal") {
529
context.visitOperand(value.value);
compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.expect.md
new
+40
@@ -0,0 +1,40 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// To preserve the nullthrows behavior and reactive deps of this code,
6
+// Forget needs to add `props.a` as a dependency (since `props.a.b` is
7
+// a conditional dependency, i.e. gated behind control flow)
8
+
9
+function Component(props) {
10
+ let x = [];
11
+ x.push(props.a?.b);
12
+ return x;
13
+}
14
+
15
+```
16
+
17
+## Code
18
+
19
+```javascript
20
+// To preserve the nullthrows behavior and reactive deps of this code,
21
+// Forget needs to add `props.a` as a dependency (since `props.a.b` is
22
+// a conditional dependency, i.e. gated behind control flow)
23
+
24
+function Component(props) {
25
+ const $ = React.unstable_useMemoCache(2);
26
+ const c_0 = $[0] !== props.a;
27
+ let x;
28
+ if (c_0) {
29
+ x = [];
30
+ x.push(props.a?.b);
31
+ $[0] = props.a;
32
+ $[1] = x;
33
+ } else {
34
+ x = $[1];
35
+ }
36
+ return x;
37
+}
38
+
39
+```
40
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.js
new
+9
@@ -0,0 +1,9 @@
1
+// To preserve the nullthrows behavior and reactive deps of this code,
2
+// Forget needs to add `props.a` as a dependency (since `props.a.b` is
3
+// a conditional dependency, i.e. gated behind control flow)
4
+
5
+function Component(props) {
6
+ let x = [];
7
+ x.push(props.a?.b);
8
+ return x;
9
+}
compiler/forget/src/__tests__/fixtures/compiler/nested-optional-member-expr.expect.md
+2
-2
@@ -18,11 +18,11 @@ function Component(props) {
18
// (i.e. placing `?` in the correct PropertyLoad)
19
function Component(props) {
20
const $ = React.unstable_useMemoCache(2);
21
- const c_0 = $[0] !== props.a.b.c.d;
21
+ const c_0 = $[0] !== props.a;
22
let t0;
23
if (c_0) {
24
t0 = foo((props.a?.b).c.d);
25
- $[0] = props.a.b.c.d;
25
+ $[0] = props.a;
26
$[1] = t0;
27
} else {
28
t0 = $[1];
compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.expect.md
new
+56
@@ -0,0 +1,56 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// To preserve the nullthrows behavior and reactive deps of this code,
6
+// Forget needs to add `props.a.b` or a subpath as a dependency.
7
+//
8
+// (1) Since the reactive block producing x unconditionally read props.a.<...>,
9
+// reading `props.a.b` outside of the block would still preserve nullthrows
10
+// semantics of source code
11
+// (2) Technically, props.a, props.a.b, and props.a.b.c are all reactive deps.
12
+// However, `props.a?.b` is only dependent on whether `props.a` is nullish,
13
+// not its actual value. Since we already preserve nullthrows on `props.a`,
14
+// we technically do not need to add `props.a` as a dependency.
15
+
16
+function Component(props) {
17
+ let x = [];
18
+ x.push(props.a?.b);
19
+ x.push(props.a.b.c);
20
+ return x;
21
+}
22
+
23
+```
24
+
25
+## Code
26
+
27
+```javascript
28
+// To preserve the nullthrows behavior and reactive deps of this code,
29
+// Forget needs to add `props.a.b` or a subpath as a dependency.
30
+//
31
+// (1) Since the reactive block producing x unconditionally read props.a.<...>,
32
+// reading `props.a.b` outside of the block would still preserve nullthrows
33
+// semantics of source code
34
+// (2) Technically, props.a, props.a.b, and props.a.b.c are all reactive deps.
35
+// However, `props.a?.b` is only dependent on whether `props.a` is nullish,
36
+// not its actual value. Since we already preserve nullthrows on `props.a`,
37
+// we technically do not need to add `props.a` as a dependency.
38
+
39
+function Component(props) {
40
+ const $ = React.unstable_useMemoCache(2);
41
+ const c_0 = $[0] !== props.a.b;
42
+ let x;
43
+ if (c_0) {
44
+ x = [];
45
+ x.push(props.a?.b);
46
+ x.push(props.a.b.c);
47
+ $[0] = props.a.b;
48
+ $[1] = x;
49
+ } else {
50
+ x = $[1];
51
+ }
52
+ return x;
53
+}
54
+
55
+```
56
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.js
new
+17
@@ -0,0 +1,17 @@
1
+// To preserve the nullthrows behavior and reactive deps of this code,
2
+// Forget needs to add `props.a.b` or a subpath as a dependency.
3
+//
4
+// (1) Since the reactive block producing x unconditionally read props.a.<...>,
5
+// reading `props.a.b` outside of the block would still preserve nullthrows
6
+// semantics of source code
7
+// (2) Technically, props.a, props.a.b, and props.a.b.c are all reactive deps.
8
+// However, `props.a?.b` is only dependent on whether `props.a` is nullish,
9
+// not its actual value. Since we already preserve nullthrows on `props.a`,
10
+// we technically do not need to add `props.a` as a dependency.
11
+
12
+function Component(props) {
13
+ let x = [];
14
+ x.push(props.a?.b);
15
+ x.push(props.a.b.c);
16
+ return x;
17
+}