@samitouri / QOS-React / commits / 73a203c199

More accurately model nested OptionalCallExpression

More accurately models nested OptionalCallExpression. Consider: ```javascript a?.(b)?.(c) ``` Our previous representation modeled it such that we treated the second function call as if it would be called regardless of whether `a` existed or not. We knew that the second call was conditional, so our test output was correct, but the control-flow graph didn't faithfully model the semantics. That bothered me. The new representation correctly models the control flow, and the fact that if `a` is null/undefined execution immediately aborts (not reaching the second call at all, nor the evaluation of its args), and evaluates the whole outer OptionalCallExpression to `undefined`. Note that nested optional member expressions still have the previous model — that's next to address.

Joe Savona committed May 2, 2023 at 15:00 UTC 73a203c1996071c448b00dbf7e6f7a15296550a9
5 files changed +181 -133
compiler/forget/src/HIR/BuildHIR.ts
+145 -127
@@ -1088,133 +1088,7 @@ function lowerExpression(
1088 }
1089 case "OptionalCallExpression": {
1090 const expr = exprPath as NodePath<t.OptionalCallExpression>;
1091 - const optional = expr.node.optional;
1092 - const calleePath = expr.get("callee");
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)
1122 - // or evaluate to undefined (alternate)
1123 - let callee:
1124 - | { kind: "CallExpression"; callee: Place }
1125 - | { kind: "MethodCall"; receiver: Place; property: Place };
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 - };
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
1161 - builder.enterReserved(consequent, () => {
1162 - const args = lowerArguments(builder, expr.get("arguments"));
1163 - const temp = buildTemporaryPlace(builder, loc);
1164 - if (callee.kind === "CallExpression") {
1165 - builder.push({
1166 - id: makeInstructionId(0),
1167 - lvalue: { ...temp },
1168 - value: {
1169 - kind: "CallExpression",
1170 - callee: { ...callee.callee },
1171 - args,
1172 - loc,
1173 - },
1174 - loc,
1175 - });
1176 - } else {
1177 - builder.push({
1178 - id: makeInstructionId(0),
1179 - lvalue: { ...temp },
1180 - value: {
1181 - kind: "MethodCall",
1182 - receiver: { ...callee.receiver },
1183 - property: { ...callee.property },
1184 - args,
1185 - loc: exprLoc,
1186 - },
1187 - loc,
1188 - });
1189 - }
1190 - lowerValueToTemporary(builder, {
1191 - kind: "StoreLocal",
1192 - lvalue: { kind: InstructionKind.Const, place: { ...place } },
1193 - value: { ...temp },
1194 - loc,
1195 - });
1196 - return {
1197 - kind: "goto",
1198 - variant: GotoVariant.Break,
1199 - block: continuationBlock.id,
1200 - id: makeInstructionId(0),
1201 - loc,
1202 - };
1203 - });
1204 -
1205 - builder.terminateWithContinuation(
1206 - {
1207 - kind: "optional-call",
1208 - optional,
1209 - test: testBlock,
1210 - fallthrough: continuationBlock.id,
1211 - id: makeInstructionId(0),
1212 - loc,
1213 - },
1214 - continuationBlock
1215 - );
1216 -
1217 - return { kind: "LoadLocal", place, loc: place.loc };
1091 + return lowerOptionalCallExpression(builder, expr, null);
1092 }
1093 case "CallExpression": {
1094 const expr = exprPath as NodePath<t.CallExpression>;
@@ -1854,6 +1728,150 @@ function lowerExpression(
1728 }
1729 }
1730
1731 +function lowerOptionalCallExpression(
1732 + builder: HIRBuilder,
1733 + exprPath: NodePath<t.OptionalCallExpression>,
1734 + parentAlternate: BlockId | null
1735 +): InstructionValue {
1736 + const expr = exprPath as NodePath<t.OptionalCallExpression>;
1737 + const optional = expr.node.optional;
1738 + const calleePath = expr.get("callee");
1739 + const loc = expr.node.loc ?? GeneratedSource;
1740 + const place = buildTemporaryPlace(builder, loc);
1741 + const continuationBlock = builder.reserve(builder.currentBlockKind());
1742 + const consequent = builder.reserve("value");
1743 +
1744 + // block to evaluate if the callee is null/undefined, this sets the result of the call to undefined.
1745 + // note that we only create an alternate when first entering an optional subtree of the ast: if this
1746 + // is a child of an optional node, we use the alterate created by the parent.
1747 + const alternate =
1748 + parentAlternate !== null
1749 + ? parentAlternate
1750 + : builder.enter("value", () => {
1751 + const temp = lowerValueToTemporary(builder, {
1752 + kind: "Primitive",
1753 + value: undefined,
1754 + loc,
1755 + });
1756 + lowerValueToTemporary(builder, {
1757 + kind: "StoreLocal",
1758 + lvalue: { kind: InstructionKind.Const, place: { ...place } },
1759 + value: { ...temp },
1760 + loc,
1761 + });
1762 + return {
1763 + kind: "goto",
1764 + variant: GotoVariant.Break,
1765 + block: continuationBlock.id,
1766 + id: makeInstructionId(0),
1767 + loc,
1768 + };
1769 + });
1770 +
1771 + // Lower the callee within the test block to represent the fact that the code for the callee is
1772 + // scoped within the optional
1773 + let callee:
1774 + | { kind: "CallExpression"; callee: Place }
1775 + | { kind: "MethodCall"; receiver: Place; property: Place };
1776 + const testBlock = builder.enter("value", () => {
1777 + if (calleePath.isOptionalCallExpression()) {
1778 + // Recursively call lowerOptionalCallExpression to thread down the alternate block
1779 + const value = lowerOptionalCallExpression(builder, calleePath, alternate);
1780 + const valuePlace = lowerValueToTemporary(builder, value);
1781 + callee = {
1782 + kind: "CallExpression",
1783 + callee: valuePlace,
1784 + };
1785 + } else if (
1786 + calleePath.isMemberExpression() ||
1787 + calleePath.isOptionalMemberExpression()
1788 + ) {
1789 + const memberExpr = lowerMemberExpression(builder, calleePath);
1790 + const propertyPlace = lowerValueToTemporary(builder, memberExpr.value);
1791 + callee = {
1792 + kind: "MethodCall",
1793 + receiver: memberExpr.object,
1794 + property: propertyPlace,
1795 + };
1796 + } else {
1797 + callee = {
1798 + kind: "CallExpression",
1799 + callee: lowerExpressionToTemporary(builder, calleePath),
1800 + };
1801 + }
1802 + const testPlace =
1803 + callee.kind === "CallExpression" ? callee.callee : callee.property;
1804 + return {
1805 + kind: "branch",
1806 + test: { ...testPlace },
1807 + consequent: consequent.id,
1808 + alternate,
1809 + id: makeInstructionId(0),
1810 + loc,
1811 + };
1812 + });
1813 +
1814 + // block to evaluate if the callee is non-null/undefined. arguments are lowered in this block to preserve
1815 + // the semantic of conditional evaluation depending on the callee
1816 + builder.enterReserved(consequent, () => {
1817 + const args = lowerArguments(builder, expr.get("arguments"));
1818 + const temp = buildTemporaryPlace(builder, loc);
1819 + if (callee.kind === "CallExpression") {
1820 + builder.push({
1821 + id: makeInstructionId(0),
1822 + lvalue: { ...temp },
1823 + value: {
1824 + kind: "CallExpression",
1825 + callee: { ...callee.callee },
1826 + args,
1827 + loc,
1828 + },
1829 + loc,
1830 + });
1831 + } else {
1832 + builder.push({
1833 + id: makeInstructionId(0),
1834 + lvalue: { ...temp },
1835 + value: {
1836 + kind: "MethodCall",
1837 + receiver: { ...callee.receiver },
1838 + property: { ...callee.property },
1839 + args,
1840 + loc,
1841 + },
1842 + loc,
1843 + });
1844 + }
1845 + lowerValueToTemporary(builder, {
1846 + kind: "StoreLocal",
1847 + lvalue: { kind: InstructionKind.Const, place: { ...place } },
1848 + value: { ...temp },
1849 + loc,
1850 + });
1851 + return {
1852 + kind: "goto",
1853 + variant: GotoVariant.Break,
1854 + block: continuationBlock.id,
1855 + id: makeInstructionId(0),
1856 + loc,
1857 + };
1858 + });
1859 +
1860 + builder.terminateWithContinuation(
1861 + {
1862 + kind: "optional-call",
1863 + optional,
1864 + test: testBlock,
1865 + fallthrough: continuationBlock.id,
1866 + id: makeInstructionId(0),
1867 + loc,
1868 + },
1869 + continuationBlock
1870 + );
1871 +
1872 + return { kind: "LoadLocal", place, loc: place.loc };
1873 +}
1874 +
1875 /**
1876 * There are a few places where we do not preserve original evaluation ordering and/or control flow, such as
1877 * switch case test values and default values in destructuring (assignment patterns). In these cases we allow
compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.expect.md
+2 -4
@@ -3,8 +3,7 @@
3
4 ```javascript
5 function Component(props) {
6 - const object = makeObject();
7 - return object.a?.b?.c(props);
6 + return call?.(props.a)?.(props.b)?.(props.c);
7 }
8
9 ```
@@ -18,8 +17,7 @@ function Component(props) {
17 const c_0 = $[0] !== props;
18 let t0;
19 if (c_0) {
21 - const object = makeObject();
22 - t0 = object.a?.b?.c(props);
20 + t0 = call?.(props.a)?.(props.b)?.(props.c);
21 $[0] = props;
22 $[1] = t0;
23 } else {
compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.js
+1 -2
@@ -1,4 +1,3 @@
1 function Component(props) {
2 - const object = makeObject();
3 - return object.a?.b?.c(props);
2 + return call?.(props.a)?.(props.b)?.(props.c);
3 }
compiler/forget/src/__tests__/fixtures/compiler/optional-call-with-optional-property-load.expect.md new
+30
@@ -0,0 +1,30 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + return props?.items?.map?.(render)?.filter(Boolean) ?? [];
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 = props?.items?.map?.(render)?.filter(Boolean) ?? [];
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-with-optional-property-load.js new
+3
@@ -0,0 +1,3 @@
1 +function Component(props) {
2 + return props?.items?.map?.(render)?.filter(Boolean) ?? [];
3 +}