Support OptionalCallExpression as LHS of LogicalExpression
A common idiom is to map over some possibly-missing list of items from a data payload and fall back to an empty array: ```javascript const renderedItems = data?.items?.map(renderItem) ?? []; ``` The way we were lowering OptionalCallExpression meant that in this case, we'd end up with an OptionalCallTerminal as the terminal of the logical expression's test block, which violates our internal invariant. Logical test blocks must end in a Branch! This PR fixes the immediate issue, which is that the callee - in this case `data?.items?.map` — was being lowered prior to the OptionalCallTerminal instead of inside its test block. Changing that fixes the shape of the IR and makes this example work. As part of investigating this I realized that the way I originally handled lowering of optional call isn't quite right. The difference isn't observable unless we did more sophisticated DCE but we don't correctly model the fact that if `data.items` is null that the `map()` call won't occur. That is technically fine bc we do model the fact that the `map()` call is conditional, and notably its arguments are only conditional dependencies. So it's good enough. But in a follow-up I'll change to model the fact that `data.items` is null, that the map call isn't reachable at all.
Joe Savona committed
May 2, 2023 at 15:00 UTC
9a12e8ba58a14288359036b130c1e86ae579da84
11 files changed
+195
-63
compiler/forget/src/HIR/BuildHIR.ts
+55
-52
@@ -1093,6 +1093,29 @@ function lowerExpression(
1093
const loc = expr.node.loc ?? GeneratedSource;
1094
const place = buildTemporaryPlace(builder, loc);
1095
const continuationBlock = builder.reserve(builder.currentBlockKind());
1096
+ const consequent = builder.reserve("value");
1097
+
1098
+ // block to evaluate if the callee is null/undefined, this sets the result of the call to undefined.
1099
+ const alternate = builder.enter("value", () => {
1100
+ const temp = lowerValueToTemporary(builder, {
1101
+ kind: "Primitive",
1102
+ value: undefined,
1103
+ loc,
1104
+ });
1105
+ lowerValueToTemporary(builder, {
1106
+ kind: "StoreLocal",
1107
+ lvalue: { kind: InstructionKind.Const, place: { ...place } },
1108
+ value: { ...temp },
1109
+ loc,
1110
+ });
1111
+ return {
1112
+ kind: "goto",
1113
+ variant: GotoVariant.Break,
1114
+ block: continuationBlock.id,
1115
+ id: makeInstructionId(0),
1116
+ loc,
1117
+ };
1118
+ });
1119
1120
// Lower the callee in the current block: the callee is always unconditionally evaluated
1121
// The test block's branch will test on this value to determine whether to evaluate the call (consequent)
@@ -1100,27 +1123,42 @@ function lowerExpression(
1123
let callee:
1124
| { kind: "CallExpression"; callee: Place }
1125
| { kind: "MethodCall"; receiver: Place; property: Place };
1103
- if (
1104
- calleePath.isMemberExpression() ||
1105
- calleePath.isOptionalMemberExpression()
1106
- ) {
1107
- const memberExpr = lowerMemberExpression(builder, calleePath);
1108
- const propertyPlace = lowerValueToTemporary(builder, memberExpr.value);
1109
- callee = {
1110
- kind: "MethodCall",
1111
- receiver: memberExpr.object,
1112
- property: propertyPlace,
1113
- };
1114
- } else {
1115
- callee = {
1116
- kind: "CallExpression",
1117
- callee: lowerExpressionToTemporary(builder, calleePath),
1126
+ const testBlock = builder.enter("value", () => {
1127
+ if (
1128
+ calleePath.isMemberExpression() ||
1129
+ calleePath.isOptionalMemberExpression()
1130
+ ) {
1131
+ const memberExpr = lowerMemberExpression(builder, calleePath);
1132
+ const propertyPlace = lowerValueToTemporary(
1133
+ builder,
1134
+ memberExpr.value
1135
+ );
1136
+ callee = {
1137
+ kind: "MethodCall",
1138
+ receiver: memberExpr.object,
1139
+ property: propertyPlace,
1140
+ };
1141
+ } else {
1142
+ callee = {
1143
+ kind: "CallExpression",
1144
+ callee: lowerExpressionToTemporary(builder, calleePath),
1145
+ };
1146
+ }
1147
+ const testPlace =
1148
+ callee.kind === "CallExpression" ? callee.callee : callee.property;
1149
+ return {
1150
+ kind: "branch",
1151
+ test: { ...testPlace },
1152
+ consequent: consequent.id,
1153
+ alternate,
1154
+ id: makeInstructionId(0),
1155
+ loc,
1156
};
1119
- }
1157
+ });
1158
1159
// block to evaluate if the callee is non-null/undefined. arguments are lowered in this block to preserve
1160
// the semantic of conditional evaluation depending on the callee
1123
- const consequent = builder.enter("value", () => {
1161
+ builder.enterReserved(consequent, () => {
1162
const args = lowerArguments(builder, expr.get("arguments"));
1163
const temp = buildTemporaryPlace(builder, loc);
1164
if (callee.kind === "CallExpression") {
@@ -1164,41 +1202,6 @@ function lowerExpression(
1202
};
1203
});
1204
1167
- // block to evaluate if the callee is null/undefined, this sets the result of the call to undefined.
1168
- const alternate = builder.enter("value", () => {
1169
- const temp = lowerValueToTemporary(builder, {
1170
- kind: "Primitive",
1171
- value: undefined,
1172
- loc,
1173
- });
1174
- lowerValueToTemporary(builder, {
1175
- kind: "StoreLocal",
1176
- lvalue: { kind: InstructionKind.Const, place: { ...place } },
1177
- value: { ...temp },
1178
- loc,
1179
- });
1180
- return {
1181
- kind: "goto",
1182
- variant: GotoVariant.Break,
1183
- block: continuationBlock.id,
1184
- id: makeInstructionId(0),
1185
- loc,
1186
- };
1187
- });
1188
-
1189
- const testBlock = builder.enter("value", () => {
1190
- const testPlace =
1191
- callee.kind === "CallExpression" ? callee.callee : callee.property;
1192
- return {
1193
- kind: "branch",
1194
- test: { ...testPlace },
1195
- consequent,
1196
- alternate,
1197
- id: makeInstructionId(0),
1198
- loc,
1199
- };
1200
- });
1201
-
1205
builder.terminateWithContinuation(
1206
{
1207
kind: "optional-call",
compiler/forget/src/HIR/HIRBuilder.ts
+19
-9
@@ -340,16 +340,13 @@ export default class HIRBuilder {
340
}
341
342
/**
343
- * Create a new block and execute the provided callback with the new block
344
- * set as the current, resetting to the previously active block upon exit.
345
- * The lambda must return a terminal node, which is used to terminate the
346
- * newly constructed block.
343
+ * Sets the given wip block as the current block, executes the provided callback to populate the block
344
+ * up to its terminal, and then resets the previous actively block.
345
*/
348
- enter(nextBlockKind: BlockKind, fn: (blockId: BlockId) => Terminal): BlockId {
346
+ enterReserved(wip: WipBlock, fn: () => Terminal): void {
347
const current = this.#current;
350
- const nextId = this.#env.nextBlockId;
351
- this.#current = newBlock(nextId, nextBlockKind);
352
- const terminal = fn(nextId);
348
+ this.#current = wip;
349
+ const terminal = fn();
350
const { id: blockId, kind, instructions } = this.#current;
351
this.#completed.set(blockId, {
352
kind,
@@ -360,7 +357,20 @@ export default class HIRBuilder {
357
phis: new Set(),
358
});
359
this.#current = current;
363
- return nextId;
360
+ }
361
+
362
+ /**
363
+ * Create a new block and execute the provided callback with the new block
364
+ * set as the current, resetting to the previously active block upon exit.
365
+ * The lambda must return a terminal node, which is used to terminate the
366
+ * newly constructed block.
367
+ */
368
+ enter(nextBlockKind: BlockKind, fn: (blockId: BlockId) => Terminal): BlockId {
369
+ const wip = this.reserve(nextBlockKind);
370
+ this.enterReserved(wip, () => {
371
+ return fn(wip.id);
372
+ });
373
+ return wip.id;
374
}
375
376
label<T>(label: string, breakBlock: BlockId, fn: () => T): T {
compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts
+15
-1
@@ -768,12 +768,26 @@ class Driver {
768
testBlock.terminal.consequent,
769
terminal.loc
770
);
771
+ const call: ReactiveSequenceValue = {
772
+ kind: "SequenceExpression",
773
+ instructions: [
774
+ {
775
+ id: test.id,
776
+ loc: testBlock.terminal.loc,
777
+ lvalue: test.place,
778
+ value: test.value,
779
+ },
780
+ ],
781
+ id: consequent.id,
782
+ value: consequent.value,
783
+ loc: terminal.loc,
784
+ };
785
return {
786
place: { ...consequent.place },
787
value: {
788
kind: "OptionalCall",
789
optional: terminal.optional,
776
- call: consequent.value,
790
+ call: call,
791
id: terminal.id,
792
loc: terminal.loc,
793
},
compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts
+8
@@ -151,6 +151,14 @@ function printReactiveValue(writer: Writer, value: ReactiveValue): void {
151
});
152
break;
153
}
154
+ case "OptionalCall": {
155
+ writer.append(`OptionalCall optional=${value.optional}`);
156
+ writer.newline();
157
+ writer.indented(() => {
158
+ printReactiveValue(writer, value.call);
159
+ });
160
+ break;
161
+ }
162
default: {
163
writer.append(printInstructionValue(value));
164
}
compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts
+4
-1
@@ -262,7 +262,10 @@ function computeMemoizedIdentifiers(state: State): Set<IdentifierId> {
262
// Visit an identifier, optionally forcing it to be memoized
263
function visit(id: IdentifierId, forceMemoize: boolean = false): boolean {
264
const node = state.identifiers.get(id);
265
- invariant(node !== undefined, "Expected a node for all identifiers");
265
+ invariant(
266
+ node !== undefined,
267
+ `Expected a node for all identifiers, none found for '${id}'`
268
+ );
269
if (node.seen) {
270
return node.memoized;
271
}
compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.expect.md
new
+32
@@ -0,0 +1,32 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const object = makeObject();
7
+ return object.a?.b?.c(props);
8
+}
9
+
10
+```
11
+
12
+## Code
13
+
14
+```javascript
15
+import { unstable_useMemoCache as useMemoCache } from "react";
16
+function Component(props) {
17
+ const $ = useMemoCache(2);
18
+ const c_0 = $[0] !== props;
19
+ let t0;
20
+ if (c_0) {
21
+ const object = makeObject();
22
+ t0 = object.a?.b?.c(props);
23
+ $[0] = props;
24
+ $[1] = t0;
25
+ } else {
26
+ t0 = $[1];
27
+ }
28
+ return t0;
29
+}
30
+
31
+```
32
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.js
new
+4
@@ -0,0 +1,4 @@
1
+function Component(props) {
2
+ const object = makeObject();
3
+ return object.a?.b?.c(props);
4
+}
compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.expect.md
new
+21
@@ -0,0 +1,21 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const item = useFragment(graphql`...`, props.item);
7
+ return item.items?.map((item) => renderItem(item)) ?? [];
8
+}
9
+
10
+```
11
+
12
+## Code
13
+
14
+```javascript
15
+function Component(props) {
16
+ const item = useFragment(graphql`...`, props.item);
17
+ return item.items?.map((item_0) => renderItem(item_0)) ?? [];
18
+}
19
+
20
+```
21
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.js
new
+4
@@ -0,0 +1,4 @@
1
+function Component(props) {
2
+ const item = useFragment(graphql`...`, props.item);
3
+ return item.items?.map((item) => renderItem(item)) ?? [];
4
+}
compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.expect.md
new
+30
@@ -0,0 +1,30 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ return foo?.(props);
7
+}
8
+
9
+```
10
+
11
+## Code
12
+
13
+```javascript
14
+import { unstable_useMemoCache as useMemoCache } from "react";
15
+function Component(props) {
16
+ const $ = useMemoCache(2);
17
+ const c_0 = $[0] !== props;
18
+ let t0;
19
+ if (c_0) {
20
+ t0 = foo?.(props);
21
+ $[0] = props;
22
+ $[1] = t0;
23
+ } else {
24
+ t0 = $[1];
25
+ }
26
+ return t0;
27
+}
28
+
29
+```
30
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.js
new
+3
@@ -0,0 +1,3 @@
1
+function Component(props) {
2
+ return foo?.(props);
3
+}