From 6d434cc777112e261159c39dc2a2dae5710b5e58 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 27 Mar 2023 10:34:11 -0700 Subject: [PATCH] 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. --- compiler/forget/src/HIR/BuildHIR.ts | 132 ++++++++++++------ .../destructuring-array-default.expect.md | 4 +- .../compiler/destructuring-array-default.js | 2 +- ...cturing-assignment-array-default.expect.md | 4 +- .../destructuring-assignment-array-default.js | 2 +- .../compiler/error.todo-kitchensink.expect.md | 6 +- 6 files changed, 95 insertions(+), 55 deletions(-) diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 819bd27811..0a11edeb54 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -514,51 +514,7 @@ function lowerStatement( }); let test: Place | null = null; if (testExpr.node != null) { - switch (testExpr.node.type) { - case "Identifier": - case "StringLiteral": - case "NumericLiteral": - case "NullLiteral": - case "BooleanLiteral": - case "BigIntLiteral": { - // ok - break; - } - case "MemberExpression": { - // A common pattern is switch statements where the case test values are properties of a global, - // eg `case ProductOptions.Option: { ... }` - // We therefore allow expressions where the innermost object is a global identifier, and reject - // all other member expressions (for now). - const test = testExpr as NodePath; - let innerObject: NodePath = test; - while (innerObject.isMemberExpression()) { - innerObject = innerObject.get("object"); - } - if ( - innerObject.isIdentifier() && - builder.resolveIdentifier(innerObject) === null // null means global - ) { - // This is a property/computed load from a global, that's safe to evaluate as a test expression - break; - } - builder.errors.push({ - reason: - "(BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported", - severity: ErrorSeverity.Todo, - nodePath: testExpr, - }); - break; - } - default: { - builder.errors.push({ - reason: - "(BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported", - severity: ErrorSeverity.Todo, - nodePath: testExpr, - }); - } - } - test = lowerExpressionToTemporary( + test = lowerReorderableExpression( builder, testExpr as NodePath ); @@ -1830,6 +1786,88 @@ function lowerExpression( } } +/** + * There are a few places where we do not preserve original evaluation ordering, such as switch case test values + * and default values in destructuring (assignment patterns). In these cases we allow simple expressions whose + * evaluation cannot be observed: primitives and arrays/objects whose values are also safely reorderable. + */ +function lowerReorderableExpression( + builder: HIRBuilder, + expr: NodePath +): Place { + if (isReorderableExpression(builder, expr)) { + return lowerExpressionToTemporary(builder, expr); + } else { + builder.errors.push({ + reason: `(BuildHIR::node.lowerReorderableExpression) Expression type '${expr.type}' cannot be safely reordered`, + severity: ErrorSeverity.Todo, + nodePath: expr, + }); + return buildTemporaryPlace(builder, expr.node.loc ?? GeneratedSource); + } +} + +function isReorderableExpression( + builder: HIRBuilder, + expr: NodePath +): boolean { + switch (expr.node.type) { + case "Identifier": + case "RegExpLiteral": + case "StringLiteral": + case "NumericLiteral": + case "NullLiteral": + case "BooleanLiteral": + case "BigIntLiteral": { + return true; + } + case "ArrayExpression": { + return (expr as NodePath) + .get("elements") + .every( + (element) => + element.isExpression() && isReorderableExpression(builder, element) + ); + } + case "ObjectExpression": { + return (expr as NodePath) + .get("properties") + .every((property) => { + if (!property.isObjectProperty() || property.node.computed) { + return false; + } + const value = property.get("value"); + return ( + value.isExpression() && isReorderableExpression(builder, value) + ); + }); + } + case "MemberExpression": { + // A common pattern is switch statements where the case test values are properties of a global, + // eg `case ProductOptions.Option: { ... }` + // We therefore allow expressions where the innermost object is a global identifier, and reject + // all other member expressions (for now). + const test = expr as NodePath; + let innerObject: NodePath = test; + while (innerObject.isMemberExpression()) { + innerObject = innerObject.get("object"); + } + if ( + innerObject.isIdentifier() && + builder.resolveIdentifier(innerObject) === null // null means global + ) { + // This is a property/computed load from a global, that's safe to reorder + return true; + } else { + return false; + } + } + default: { + return false; + } + } +} + function lowerArguments( builder: HIRBuilder, expr: Array< @@ -2455,7 +2493,9 @@ function lowerAssignment( const continuationBlock = builder.reserve(builder.currentBlockKind()); const consequent = builder.enter("value", () => { - const defaultValue = lowerExpressionToTemporary( + // Because we reorder evaluation, we restrict the allowed default values to those where + // evaluation order is unobservable + const defaultValue = lowerReorderableExpression( builder, lvalue.get("right") ); diff --git a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.expect.md index 0fa78caecc..35d3dd2185 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.expect.md @@ -3,7 +3,7 @@ ```javascript function Component(props) { - const [[x] = [foo()]] = props.y; + const [[x] = ["default"]] = props.y; return x; } @@ -18,7 +18,7 @@ function Component(props) { const c_0 = $[0] !== t0; let t1; if (c_0) { - t1 = t0 === undefined ? [foo()] : t0; + t1 = t0 === undefined ? ["default"] : t0; $[0] = t0; $[1] = t1; } else { diff --git a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.js b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.js index eb00011bc0..a15539f441 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.js +++ b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-array-default.js @@ -1,4 +1,4 @@ function Component(props) { - const [[x] = [foo()]] = props.y; + const [[x] = ["default"]] = props.y; return x; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.expect.md index 5b10ade0d0..f6e7f6f8bd 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.expect.md @@ -5,7 +5,7 @@ function Component(props) { let x; if (props.cond) { - [[x] = [foo()]] = props.y; + [[x] = ["default"]] = props.y; } else { x = props.fallback; } @@ -25,7 +25,7 @@ function Component(props) { const c_0 = $[0] !== t0; let t1; if (c_0) { - t1 = t0 === undefined ? [foo()] : t0; + t1 = t0 === undefined ? ["default"] : t0; $[0] = t0; $[1] = t1; } else { diff --git a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.js b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.js index 3c5acd33a9..331ece2dc2 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.js +++ b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-assignment-array-default.js @@ -1,7 +1,7 @@ function Component(props) { let x; if (props.cond) { - [[x] = [foo()]] = props.y; + [[x] = ["default"]] = props.y; } else { x = props.fallback; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.todo-kitchensink.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.todo-kitchensink.expect.md index f099a8347e..de37287be2 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.todo-kitchensink.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.todo-kitchensink.expect.md @@ -264,7 +264,7 @@ let moduleLocal = false; 48 | switch (i) { 49 | case 1 + 1: { -[ReactForget] TodoError: (BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported +[ReactForget] TodoError: (BuildHIR::node.lowerReorderableExpression) Expression type 'MemberExpression' cannot be safely reordered 51 | case foo(): { 52 | } > 53 | case x.y: { @@ -273,7 +273,7 @@ let moduleLocal = false; 55 | default: { 56 | } -[ReactForget] TodoError: (BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported +[ReactForget] TodoError: (BuildHIR::node.lowerReorderableExpression) Expression type 'CallExpression' cannot be safely reordered 49 | case 1 + 1: { 50 | } > 51 | case foo(): { @@ -282,7 +282,7 @@ let moduleLocal = false; 53 | case x.y: { 54 | } -[ReactForget] TodoError: (BuildHIR::lowerStatement) Switch case test values must be identifiers or primitives, compound values are not yet supported +[ReactForget] TodoError: (BuildHIR::node.lowerReorderableExpression) Expression type 'BinaryExpression' cannot be safely reordered 47 | 48 | switch (i) { > 49 | case 1 + 1: {