From 3744930728bfe6a08c382b25834e258bc44bc07f Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Sun, 4 Jun 2023 21:41:27 -0400
Subject: [PATCH] Fix last(?) order of evaluation bug

Not to get ahead of myself (sorry i had to), but i think this is the last order
of evaluation bug. At least it's the last one we know of[1]. Per the previous
PR, the issue is that constant propagation can copy the last value of a sequence
expression to where the sequence is used, leaving the original sequence
expression out of order after other instructions are moved around. We fix that
here by explicitly skipping constant propagation for the last value of a
sequence block.

[1] There are some places where we _would_ have evaluation order bugs if we
allowed arbitrary expressions, but we explicitly limit the expressions we allow
in those places. For the curious: switch test case values and destructuring
default values.
---
 compiler/forget/src/HIR/BuildHIR.ts                       | 2 +-
 compiler/forget/src/HIR/HIR.ts                            | 2 +-
 compiler/forget/src/Optimization/ConstantPropagation.ts   | 8 +++++++-
 ...expect.md => computed-call-evaluation-order.expect.md} | 5 +++--
 ...luation-order.js => computed-call-evaluation-order.js} | 0
 5 files changed, 12 insertions(+), 5 deletions(-)
 rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.computed-call-evaluation-order.expect.md => computed-call-evaluation-order.expect.md} (90%)
 rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.computed-call-evaluation-order.js => computed-call-evaluation-order.js} (100%)

diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts
index 74d80d3119..d6d002e8c4 100644
--- a/compiler/forget/src/HIR/BuildHIR.ts
+++ b/compiler/forget/src/HIR/BuildHIR.ts
@@ -1171,7 +1171,7 @@ function lowerExpression(
       const continuationBlock = builder.reserve(builder.currentBlockKind());
       const place = buildTemporaryPlace(builder, exprLoc);
 
-      const sequenceBlock = builder.enter("value", (_) => {
+      const sequenceBlock = builder.enter("sequence", (_) => {
         let last: Place | null = null;
         for (const item of expr.get("expressions")) {
           last = lowerExpressionToTemporary(builder, item);
diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts
index 923ae83ca1..b02016ebd6 100644
--- a/compiler/forget/src/HIR/HIR.ts
+++ b/compiler/forget/src/HIR/HIR.ts
@@ -250,7 +250,7 @@ export type HIR = {
  * an exception occurs, therefore the block model only represents explicit throw
  * statements and not implicit exceptions which may occur.
  */
-export type BlockKind = "block" | "value" | "loop";
+export type BlockKind = "block" | "value" | "loop" | "sequence";
 export type BasicBlock = {
   kind: BlockKind;
   id: BlockId;
diff --git a/compiler/forget/src/Optimization/ConstantPropagation.ts b/compiler/forget/src/Optimization/ConstantPropagation.ts
index 05403c4cd9..a7b1a9ecd1 100644
--- a/compiler/forget/src/Optimization/ConstantPropagation.ts
+++ b/compiler/forget/src/Optimization/ConstantPropagation.ts
@@ -113,7 +113,13 @@ function applyConstantPropagation(fn: HIRFunction): boolean {
       }
     }
 
-    for (const instr of block.instructions) {
+    for (let i = 0; i < block.instructions.length; i++) {
+      if (block.kind === "sequence" && i === block.instructions.length - 1) {
+        // evaluating the last value of a value block can break order of evaluation,
+        // skip these instructions
+        continue;
+      }
+      const instr = block.instructions[i]!;
       const value = evaluateInstruction(constants, instr);
       if (value !== null) {
         constants.set(instr.lvalue.identifier.id, value);
diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/computed-call-evaluation-order.expect.md
similarity index 90%
rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md
rename to compiler/forget/src/__tests__/fixtures/compiler/computed-call-evaluation-order.expect.md
index 868e6c1784..57e297c305 100644
--- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md
+++ b/compiler/forget/src/__tests__/fixtures/compiler/computed-call-evaluation-order.expect.md
@@ -46,8 +46,9 @@ function Component() {
   if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
     x = { f: t1 };
 
-    console.log("B"), "f";
-    (console.log("A"), x).f((changeF(x), console.log("arg"), 1));
+    (console.log("A"), x)[(console.log("B"), "f")](
+      (changeF(x), console.log("arg"), 1)
+    );
     $[2] = x;
   } else {
     x = $[2];
diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.js b/compiler/forget/src/__tests__/fixtures/compiler/computed-call-evaluation-order.js
similarity index 100%
rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.js
rename to compiler/forget/src/__tests__/fixtures/compiler/computed-call-evaluation-order.js