Generalize helper for reorderable expressions
Adds a new helper method that we can use when processing expressions whose evaluation ordering may not be preserved. This was previously the case only for switch test case values, but we can use this for AssignmentPattern (destructuring default values) as well.
Joe Savona committed
Mar 27, 2023 at 10:34 UTC
6d434cc777112e261159c39dc2a2dae5710b5e58
6 files changed
+95
-55
compiler/forget/src/HIR/BuildHIR.ts
+86
-46
@@ -514,51 +514,7 @@ function lowerStatement(
514
});
515
let test: Place | null = null;
516
if (testExpr.node != null) {
517
- switch (testExpr.node.type) {
518
- case "Identifier":
519
- case "StringLiteral":
520
- case "NumericLiteral":
521
- case "NullLiteral":
522
- case "BooleanLiteral":
523
- case "BigIntLiteral": {
524
- // ok
525
- break;
526
- }
527
- case "MemberExpression": {
528
- // A common pattern is switch statements where the case test values are properties of a global,
529
- // eg `case ProductOptions.Option: { ... }`
530
- // We therefore allow expressions where the innermost object is a global identifier, and reject
531
- // all other member expressions (for now).
532
- const test = testExpr as NodePath<t.MemberExpression>;
533
- let innerObject: NodePath<t.Expression> = test;
534
- while (innerObject.isMemberExpression()) {
535
- innerObject = innerObject.get("object");
536
- }
537
- if (
538
- innerObject.isIdentifier() &&
539
- builder.resolveIdentifier(innerObject) === null // null means global
540
- ) {
541
- // This is a property/computed load from a global, that's safe to evaluate as a test expression
542
- break;
543
- }
544
- builder.errors.push({
545
- reason:
546
- "(BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported",
547
- severity: ErrorSeverity.Todo,
548
- nodePath: testExpr,
549
- });
550
- break;
551
- }
552
- default: {
553
- builder.errors.push({
554
- reason:
555
- "(BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported",
556
- severity: ErrorSeverity.Todo,
557
- nodePath: testExpr,
558
- });
559
- }
560
- }
561
- test = lowerExpressionToTemporary(
517
+ test = lowerReorderableExpression(
518
builder,
519
testExpr as NodePath<t.Expression>
520
);
@@ -1830,6 +1786,88 @@ function lowerExpression(
1786
}
1787
}
1788
1789
+/**
1790
+ * There are a few places where we do not preserve original evaluation ordering, such as switch case test values
1791
+ * and default values in destructuring (assignment patterns). In these cases we allow simple expressions whose
1792
+ * evaluation cannot be observed: primitives and arrays/objects whose values are also safely reorderable.
1793
+ */
1794
+function lowerReorderableExpression(
1795
+ builder: HIRBuilder,
1796
+ expr: NodePath<t.Expression>
1797
+): Place {
1798
+ if (isReorderableExpression(builder, expr)) {
1799
+ return lowerExpressionToTemporary(builder, expr);
1800
+ } else {
1801
+ builder.errors.push({
1802
+ reason: `(BuildHIR::node.lowerReorderableExpression) Expression type '${expr.type}' cannot be safely reordered`,
1803
+ severity: ErrorSeverity.Todo,
1804
+ nodePath: expr,
1805
+ });
1806
+ return buildTemporaryPlace(builder, expr.node.loc ?? GeneratedSource);
1807
+ }
1808
+}
1809
+
1810
+function isReorderableExpression(
1811
+ builder: HIRBuilder,
1812
+ expr: NodePath<t.Expression>
1813
+): boolean {
1814
+ switch (expr.node.type) {
1815
+ case "Identifier":
1816
+ case "RegExpLiteral":
1817
+ case "StringLiteral":
1818
+ case "NumericLiteral":
1819
+ case "NullLiteral":
1820
+ case "BooleanLiteral":
1821
+ case "BigIntLiteral": {
1822
+ return true;
1823
+ }
1824
+ case "ArrayExpression": {
1825
+ return (expr as NodePath<t.ArrayExpression>)
1826
+ .get("elements")
1827
+ .every(
1828
+ (element) =>
1829
+ element.isExpression() && isReorderableExpression(builder, element)
1830
+ );
1831
+ }
1832
+ case "ObjectExpression": {
1833
+ return (expr as NodePath<t.ObjectExpression>)
1834
+ .get("properties")
1835
+ .every((property) => {
1836
+ if (!property.isObjectProperty() || property.node.computed) {
1837
+ return false;
1838
+ }
1839
+ const value = property.get("value");
1840
+ return (
1841
+ value.isExpression() && isReorderableExpression(builder, value)
1842
+ );
1843
+ });
1844
+ }
1845
+ case "MemberExpression": {
1846
+ // A common pattern is switch statements where the case test values are properties of a global,
1847
+ // eg `case ProductOptions.Option: { ... }`
1848
+ // We therefore allow expressions where the innermost object is a global identifier, and reject
1849
+ // all other member expressions (for now).
1850
+ const test = expr as NodePath<t.MemberExpression>;
1851
+ let innerObject: NodePath<t.Expression> = test;
1852
+ while (innerObject.isMemberExpression()) {
1853
+ innerObject = innerObject.get("object");
1854
+ }
1855
+ if (
1856
+ innerObject.isIdentifier() &&
1857
+ builder.resolveIdentifier(innerObject) === null // null means global
1858
+ ) {
1859
+ // This is a property/computed load from a global, that's safe to reorder
1860
+ return true;
1861
+ } else {
1862
+ return false;
1863
+ }
1864
+ }
1865
+ default: {
1866
+ return false;
1867
+ }
1868
+ }
1869
+}
1870
+
1871
function lowerArguments(
1872
builder: HIRBuilder,
1873
expr: Array<
@@ -2455,7 +2493,9 @@ function lowerAssignment(
2493
const continuationBlock = builder.reserve(builder.currentBlockKind());
2494
2495
const consequent = builder.enter("value", () => {
2458
- const defaultValue = lowerExpressionToTemporary(
2496
+ // Because we reorder evaluation, we restrict the allowed default values to those where
2497
+ // evaluation order is unobservable
2498
+ const defaultValue = lowerReorderableExpression(
2499
builder,
2500
lvalue.get("right")
2501
);
compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.expect.md
+2
-2
@@ -3,7 +3,7 @@
3
4
```javascript
5
function Component(props) {
6
- const [[x] = [foo()]] = props.y;
6
+ const [[x] = ["default"]] = props.y;
7
return x;
8
}
9
@@ -18,7 +18,7 @@ function Component(props) {
18
const c_0 = $[0] !== t0;
19
let t1;
20
if (c_0) {
21
- t1 = t0 === undefined ? [foo()] : t0;
21
+ t1 = t0 === undefined ? ["default"] : t0;
22
$[0] = t0;
23
$[1] = t1;
24
} else {
compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.js
+1
-1
@@ -1,4 +1,4 @@
1
function Component(props) {
2
- const [[x] = [foo()]] = props.y;
2
+ const [[x] = ["default"]] = props.y;
3
return x;
4
}
compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.expect.md
+2
-2
@@ -5,7 +5,7 @@
5
function Component(props) {
6
let x;
7
if (props.cond) {
8
- [[x] = [foo()]] = props.y;
8
+ [[x] = ["default"]] = props.y;
9
} else {
10
x = props.fallback;
11
}
@@ -25,7 +25,7 @@ function Component(props) {
25
const c_0 = $[0] !== t0;
26
let t1;
27
if (c_0) {
28
- t1 = t0 === undefined ? [foo()] : t0;
28
+ t1 = t0 === undefined ? ["default"] : t0;
29
$[0] = t0;
30
$[1] = t1;
31
} else {
compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.js
+1
-1
@@ -1,7 +1,7 @@
1
function Component(props) {
2
let x;
3
if (props.cond) {
4
- [[x] = [foo()]] = props.y;
4
+ [[x] = ["default"]] = props.y;
5
} else {
6
x = props.fallback;
7
}
compiler/forget/src/__tests__/fixtures/compiler/error.todo-kitchensink.expect.md
+3
-3
@@ -264,7 +264,7 @@ let moduleLocal = false;
264
48 | switch (i) {
265
49 | case 1 + 1: {
266
267
-[ReactForget] TodoError: (BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported
267
+[ReactForget] TodoError: (BuildHIR::node.lowerReorderableExpression) Expression type 'MemberExpression' cannot be safely reordered
268
51 | case foo(): {
269
52 | }
270
> 53 | case x.y: {
@@ -273,7 +273,7 @@ let moduleLocal = false;
273
55 | default: {
274
56 | }
275
276
-[ReactForget] TodoError: (BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported
276
+[ReactForget] TodoError: (BuildHIR::node.lowerReorderableExpression) Expression type 'CallExpression' cannot be safely reordered
277
49 | case 1 + 1: {
278
50 | }
279
> 51 | case foo(): {
@@ -282,7 +282,7 @@ let moduleLocal = false;
282
53 | case x.y: {
283
54 | }
284
285
-[ReactForget] TodoError: (BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported
285
+[ReactForget] TodoError: (BuildHIR::node.lowerReorderableExpression) Expression type 'BinaryExpression' cannot be safely reordered
286
47 |
287
48 | switch (i) {
288
> 49 | case 1 + 1: {