refactor(compiler): reuse temp vars when possible (#51100)

Updates the template pipeline's temporary variables phase to reuse
temporary variables within an expression. The algorithm implemented here
reuses variables more aggressively than TemplateDefinitionBuilder. This
change in behavior is acceptable, as it is unlikely to cause any
failures, and implementing the exact behavior observed in
TemplateDefinitionBuilder would be difficult.

PR Close #51100

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/TEST_CASES.json

@@ -3,9 +3,7 @@
   "cases": [
     {
       "description": "should handle nullish coalescing inside interpolations",
-      "inputFiles": [
-        "nullish_coalescing_interpolation.ts"
-      ],
+      "inputFiles": ["nullish_coalescing_interpolation.ts"],
       "expectations": [
         {
           "files": [
@@ -16,14 +14,11 @@
           ],
           "failureMessage": "Incorrect template"
         }
-      ],
-      "skipForTemplatePipeline": true
+      ]
     },
     {
       "description": "should handle nullish coalescing inside property bindings",
-      "inputFiles": [
-        "nullish_coalescing_property.ts"
-      ],
+      "inputFiles": ["nullish_coalescing_property.ts"],
       "expectations": [
         {
           "files": [
@@ -34,14 +29,11 @@
           ],
           "failureMessage": "Incorrect template"
         }
-      ],
-      "skipForTemplatePipeline": true
+      ]
     },
     {
       "description": "should handle nullish coalescing inside host bindings",
-      "inputFiles": [
-        "nullish_coalescing_host.ts"
-      ],
+      "inputFiles": ["nullish_coalescing_host.ts"],
       "expectations": [
         {
           "files": [

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/TEST_CASES.json

@@ -3,9 +3,7 @@
   "cases": [
     {
       "description": "should handle safe keyed reads inside templates",
-      "inputFiles": [
-        "safe_keyed_read.ts"
-      ],
+      "inputFiles": ["safe_keyed_read.ts"],
       "expectations": [
         {
           "files": [
@@ -20,9 +18,7 @@
     },
     {
       "description": "should handle deep safe property reads inside templates",
-      "inputFiles": [
-        "safe_access_deep.ts"
-      ],
+      "inputFiles": ["safe_access_deep.ts"],
       "expectations": [
         {
           "files": [
@@ -30,21 +26,19 @@
               "expected": "safe_access_deep_template.js",
               "generated": "safe_access_deep.js"
             }
-          ],
-          "failureMessage": "Incorrect template"
+          ]
         }
       ]
     },
     {
       "description": "should handle property reads requiring temporary variables",
-      "inputFiles": [
-        "safe_access_temporaries.ts"
-      ],
+      "inputFiles": ["safe_access_temporaries.ts"],
       "expectations": [
         {
           "files": [
             {
               "expected": "safe_access_temporaries_template.js",
+              "templatePipelineExpected": "safe_access_temporaries_template.pipeline.js",
               "generated": "safe_access_temporaries.js"
             }
           ],
@@ -54,9 +48,7 @@
     },
     {
       "description": "should handle safe method calls inside templates",
-      "inputFiles": [
-        "safe_method_call.ts"
-      ],
+      "inputFiles": ["safe_method_call.ts"],
       "expectations": [
         {
           "files": [
@@ -67,14 +59,11 @@
           ],
           "failureMessage": "Incorrect template"
         }
-      ],
-      "skipForTemplatePipeline": true
+      ]
     },
     {
       "description": "should handle safe calls inside templates",
-      "inputFiles": [
-        "safe_call.ts"
-      ],
+      "inputFiles": ["safe_call.ts"],
       "expectations": [
         {
           "files": [
@@ -90,9 +79,7 @@
     },
     {
       "description": "should handle non-null assertions after a safe access",
-      "inputFiles": [
-        "safe_access_non_null.ts"
-      ],
+      "inputFiles": ["safe_access_non_null.ts"],
       "expectations": [
         {
           "files": [
@@ -107,4 +94,4 @@
       "skipForTemplatePipeline": true
     }
   ]
-}
+}

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/safe_access_temporaries_template.pipeline.js

@@ -0,0 +1,18 @@
+} if (rf & 2) {
+  let $tmp_0_0$;
+  let $tmp_1_0$;
+  let $tmp_2_0$;
+  let $tmp_2_1$;
+  let $tmp_3_0$;
+  let $tmp_3_1$;
+  let $tmp_3_2$;
+  let $tmp_3_3$;
+  i0.ɵɵadvance(1);
+  i0.ɵɵtextInterpolate1("Safe Propery with Calls: ", ($tmp_0_0$ = ctx.p()) == null ? null : ($tmp_0_0$ = $tmp_0_0$.a()) == null ? null : ($tmp_0_0$ = $tmp_0_0$.b()) == null ? null : ($tmp_0_0$ = $tmp_0_0$.c()) == null ? null : $tmp_0_0$.d(), "");
+  i0.ɵɵadvance(2);
+  i0.ɵɵtextInterpolate1("Safe and Unsafe Property with Calls: ", ctx.p == null ? null : ($tmp_1_0$ = ctx.p.a()) == null ? null : ($tmp_1_0$ = $tmp_1_0$.b().c().d()) == null ? null : ($tmp_1_0$ = $tmp_1_0$.e()) == null ? null : $tmp_1_0$.f == null ? null : $tmp_1_0$.f.g.h == null ? null : ($tmp_1_0$ = $tmp_1_0$.f.g.h.i()) == null ? null : ($tmp_1_0$ = $tmp_1_0$.j()) == null ? null : $tmp_1_0$.k().l, "");
+  i0.ɵɵadvance(2);
+  i0.ɵɵtextInterpolate1("Nested Safe with Calls: ", ($tmp_2_0$ = ctx.f1()) == null ? null : $tmp_2_0$[($tmp_2_1$ = ctx.f2()) == null ? null : $tmp_2_1$.a] == null ? null : $tmp_2_0$[($tmp_2_1$ = $tmp_2_1$) == null ? null : $tmp_2_1$.a].b, "");
+  i0.ɵɵadvance(2);
+  i0.ɵɵtextInterpolate1("Deep Nested Safe with Calls: ", ($tmp_3_0$ = ctx.f1()) == null ? null : $tmp_3_0$[($tmp_3_1$ = ctx.f2()) == null ? null : ($tmp_3_2$ = $tmp_3_1$.f3()) == null ? null : $tmp_3_2$[($tmp_3_3$ = ctx.f4()) == null ? null : $tmp_3_3$.f5()]] == null ? null : $tmp_3_0$[($tmp_3_1$ = $tmp_3_1$) == null ? null : ($tmp_3_2$ = $tmp_3_2$) == null ? null : $tmp_3_2$[($tmp_3_3$ = $tmp_3_3$) == null ? null : $tmp_3_3$.f5()]].f6(), "");
+}

packages/compiler/src/output/output_ast.ts

@@ -996,7 +996,7 @@ export class ReadKeyExpr extends Expression {
   }
 
   override clone(): ReadKeyExpr {
-    return new ReadKeyExpr(this.receiver, this.index.clone(), this.type, this.sourceSpan);
+    return new ReadKeyExpr(this.receiver.clone(), this.index.clone(), this.type, this.sourceSpan);
   }
 }
 

packages/compiler/src/template/pipeline/ir/src/expression.ts

@@ -675,7 +675,7 @@ export class AssignTemporaryExpr extends ExpressionBase {
   }
 
   override clone(): AssignTemporaryExpr {
-    const a = new AssignTemporaryExpr(this.expr, this.xref);
+    const a = new AssignTemporaryExpr(this.expr.clone(), this.xref);
     a.name = this.name;
     return a;
   }

packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts

@@ -24,37 +24,56 @@ export function phaseTemporaryVariables(cpl: CompilationJob): void {
     let opCount = 0;
     let generatedStatements: Array<ir.StatementOp<ir.UpdateOp>> = [];
     for (const op of unit.ops()) {
+      // Identify the final time each temp var is read.
+      const finalReads = new Map<ir.XrefId, ir.ReadTemporaryExpr>();
+      ir.visitExpressionsInOp(op, expr => {
+        if (expr instanceof ir.ReadTemporaryExpr) {
+          finalReads.set(expr.xref, expr);
+        }
+      });
+
+      // Name the temp vars, accounting for the fact that a name can be reused after it has been
+      // read for the final time.
       let count = 0;
-      let xrefs = new Set<ir.XrefId>();
-      let defs = new Map<ir.XrefId, string>();
-
+      const assigned = new Set<ir.XrefId>();
+      const released = new Set<ir.XrefId>();
+      const defs = new Map<ir.XrefId, string>();
       ir.visitExpressionsInOp(op, expr => {
-        if (expr instanceof ir.ReadTemporaryExpr || expr instanceof ir.AssignTemporaryExpr) {
-          xrefs.add(expr.xref);
-        }
-      });
-
-      for (const xref of xrefs) {
-        // TODO: Exactly replicate the naming scheme used by `TemplateDefinitionBuilder`. It seems
-        // to rely on an expression index instead of an op index.
-        defs.set(xref, `tmp_${opCount}_${count++}`);
-      }
-
-      ir.visitExpressionsInOp(op, expr => {
-        if (expr instanceof ir.ReadTemporaryExpr || expr instanceof ir.AssignTemporaryExpr) {
-          const name = defs.get(expr.xref);
-          if (name === undefined) {
-            throw new Error('Found xref with unassigned name');
+        if (expr instanceof ir.AssignTemporaryExpr) {
+          if (!assigned.has(expr.xref)) {
+            assigned.add(expr.xref);
+            // TODO: Exactly replicate the naming scheme used by `TemplateDefinitionBuilder`.
+            // It seems to rely on an expression index instead of an op index.
+            defs.set(expr.xref, `tmp_${opCount}_${count++}`);
           }
-          expr.name = name;
+          assignName(defs, expr);
+        } else if (expr instanceof ir.ReadTemporaryExpr) {
+          if (finalReads.get(expr.xref) === expr) {
+            released.add(expr.xref);
+            count--;
+          }
+          assignName(defs, expr);
         }
       });
 
+      // Add declarations for the temp vars.
       generatedStatements.push(
-          ...Array.from(defs.values())
+          ...Array.from(new Set(defs.values()))
               .map(name => ir.createStatementOp<ir.UpdateOp>(new o.DeclareVarStmt(name))));
       opCount++;
     }
     unit.update.prepend(generatedStatements);
   }
 }
+
+/**
+ * Assigns a name to the temporary variable in the given temporary variable expression.
+ */
+function assignName(
+    names: Map<ir.XrefId, string>, expr: ir.AssignTemporaryExpr|ir.ReadTemporaryExpr) {
+  const name = names.get(expr.xref);
+  if (name === undefined) {
+    throw new Error(`Found xref with unassigned name: ${expr.xref}`);
+  }
+  expr.name = name;
+}