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: {