From e4df686ec66b74f7aacf3436dad41bfcbd7b8487 Mon Sep 17 00:00:00 2001
From: Dylan Hunn <dylhunn@gmail.com>
Date: Thu, 19 Oct 2023 19:45:56 -0700
Subject: [PATCH] refactor(compiler): Order elements before other phases
 (#52289)

Previously, we ran the ordering phase near the end of the compilation. However, this meant that phases like slot assignment and variable offset assignment would happen first, and then the nice, monotonically-increasing orders would be scrambled by the reordering.

It's much more intelligible to order first, and then perform these assignments. However, to make this happen, some modifications to the ordering phase are required. In particular, we can no longer rely on `advance` instructions to break up orderable groups.

PR Close #52289
---
 .../mixed_style_and_class/TEST_CASES.json     |  3 +-
 .../src/template/pipeline/src/emit.ts         |  2 +-
 .../template/pipeline/src/phases/ordering.ts  | 50 +++++++++++--------
 3 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/mixed_style_and_class/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/mixed_style_and_class/TEST_CASES.json
index ebc975bf3d5..c6e627a237b 100644
--- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/mixed_style_and_class/TEST_CASES.json
+++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/mixed_style_and_class/TEST_CASES.json
@@ -33,8 +33,7 @@
           "failureMessage": "Incorrect template",
           "files": ["pipe_bindings_slots.js"]
         }
-      ],
-      "skipForTemplatePipeline": true
+      ]
     },
     {
       "description": "should always generate advance() statements before any styling instructions",
diff --git a/packages/compiler/src/template/pipeline/src/emit.ts b/packages/compiler/src/template/pipeline/src/emit.ts
index 934106152d7..79999ae08b5 100644
--- a/packages/compiler/src/template/pipeline/src/emit.ts
+++ b/packages/compiler/src/template/pipeline/src/emit.ts
@@ -88,6 +88,7 @@ const phases: Phase[] = [
   {kind: Kind.Both, fn: phaseAttributeExtraction},
   {kind: Kind.Both, fn: phaseParseExtractedStyles},
   {kind: Kind.Tmpl, fn: phaseRemoveEmptyBindings},
+  {kind: Kind.Both, fn: phaseOrdering},
   {kind: Kind.Tmpl, fn: phaseConditionals},
   {kind: Kind.Tmpl, fn: phasePipeCreation},
   {kind: Kind.Tmpl, fn: phaseI18nTextExtraction},
@@ -127,7 +128,6 @@ const phases: Phase[] = [
   {kind: Kind.Tmpl, fn: phaseEmptyElements},
   {kind: Kind.Tmpl, fn: phaseNonbindable},
   {kind: Kind.Both, fn: phasePureFunctionExtraction},
-  {kind: Kind.Both, fn: phaseOrdering},
   {kind: Kind.Both, fn: phaseReify},
   {kind: Kind.Both, fn: phaseChaining},
 ];
diff --git a/packages/compiler/src/template/pipeline/src/phases/ordering.ts b/packages/compiler/src/template/pipeline/src/phases/ordering.ts
index 197abe31a19..a4cabc07ebd 100644
--- a/packages/compiler/src/template/pipeline/src/phases/ordering.ts
+++ b/packages/compiler/src/template/pipeline/src/phases/ordering.ts
@@ -58,34 +58,40 @@ export function phaseOrdering(job: CompilationJob) {
     // still have ops pulled at the end, put them back in the correct order.
 
     // Create mode:
-    let opsToOrder = [];
-    for (const op of unit.create) {
-      if (handledOpKinds.has(op.kind)) {
-        opsToOrder.push(op);
-        ir.OpList.remove(op);
-      } else {
-        ir.OpList.insertBefore(reorder(opsToOrder, CREATE_ORDERING), op);
-        opsToOrder = [];
-      }
-    }
-    unit.create.push(reorder(opsToOrder, CREATE_ORDERING));
+    orderWithin(unit.create, CREATE_ORDERING as Array<Rule<ir.CreateOp|ir.UpdateOp>>);
 
 
     // Update mode:
-    opsToOrder = [];
-    for (const op of unit.update) {
-      if (handledOpKinds.has(op.kind)) {
-        opsToOrder.push(op);
-        ir.OpList.remove(op);
-      } else {
-        ir.OpList.insertBefore(reorder(opsToOrder, UPDATE_ORDERING), op);
-        opsToOrder = [];
-      }
-    }
-    unit.update.push(reorder(opsToOrder, UPDATE_ORDERING));
+    orderWithin(unit.update, UPDATE_ORDERING as Array<Rule<ir.CreateOp|ir.UpdateOp>>);
   }
 }
 
+/**
+ * Order all the ops within the specified group.
+ */
+function orderWithin(
+    opList: ir.OpList<ir.CreateOp|ir.UpdateOp>, ordering: Array<Rule<ir.CreateOp|ir.UpdateOp>>) {
+  let opsToOrder = [];
+  // Only reorder ops that target the same xref; do not mix ops that target different xrefs.
+  let firstTargetInGroup: ir.XrefId|null = null;
+  for (const op of opList) {
+    const currentTarget = ir.hasDependsOnSlotContextTrait(op) ? op.target : null;
+    if (!handledOpKinds.has(op.kind) ||
+        (currentTarget !== firstTargetInGroup &&
+         (firstTargetInGroup !== null && currentTarget !== null))) {
+      ir.OpList.insertBefore(reorder(opsToOrder, ordering), op);
+      opsToOrder = [];
+      firstTargetInGroup = null;
+    }
+    if (handledOpKinds.has(op.kind)) {
+      opsToOrder.push(op);
+      ir.OpList.remove(op);
+      firstTargetInGroup = currentTarget ?? firstTargetInGroup;
+    }
+  }
+  opList.push(reorder(opsToOrder, ordering));
+}
+
 /**
  * Reorders the given list of ops according to the ordering defined by `ORDERING`.
  */