refactor(compiler): Fix a bug in which temporaries were being declared in the wrong places (angular#51950)

dylhunn · alxhub · commit aa6bb8ee95bb · 2023-10-04T09:00:54.000-07:00
Previously, we always generated temporary variable declarations at the beginning of each view's update block. This is wrong, for two reasons: 1. Temporaries can be used in the create block 2. When listeners use temporaries, we should declare them inside the listener. Now, we always place temporaries at the beginning of the enclosing OpList, and recursively try to generate them when we find a listener. PR Close angular#51950
diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/TEST_CASES.json
@@ -3,7 +3,9 @@
   "cases": [
     {
       "description": "should handle nullish coalescing inside interpolations",
-      "inputFiles": ["nullish_coalescing_interpolation.ts"],
+      "inputFiles": [
+        "nullish_coalescing_interpolation.ts"
+      ],
       "expectations": [
         {
           "files": [
@@ -18,7 +20,9 @@
     },
     {
       "description": "should handle nullish coalescing inside property bindings",
-      "inputFiles": ["nullish_coalescing_property.ts"],
+      "inputFiles": [
+        "nullish_coalescing_property.ts"
+      ],
       "expectations": [
         {
           "files": [
@@ -33,7 +37,9 @@
     },
     {
       "description": "should handle nullish coalescing inside host bindings",
-      "inputFiles": ["nullish_coalescing_host.ts"],
+      "inputFiles": [
+        "nullish_coalescing_host.ts"
+      ],
       "expectations": [
         {
           "files": [
@@ -44,8 +50,7 @@
           ],
           "failureMessage": "Incorrect host bindings"
         }
-      ],
-      "skipForTemplatePipeline": true
+      ]
     }
   ]
 }
diff --git a/packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts b/packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts
@@ -8,7 +8,7 @@
 
 import * as o from '../../../../output/output_ast';
 import * as ir from '../../ir';
-import {CompilationJob, ComponentCompilationJob} from '../compilation';
+import type {CompilationJob, CompilationUnit} from '../compilation';
 
 /**
  * Find all assignments and usages of temporary variables, which are linked to each other with cross
@@ -21,49 +21,69 @@ import {CompilationJob, ComponentCompilationJob} from '../compilation';
  */
 export function phaseTemporaryVariables(cpl: CompilationJob): void {
   for (const unit of cpl.units) {
-    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);
-        }
-      });
+    unit.create.prepend(generateTemporaries(unit.create) as Array<ir.StatementOp<ir.CreateOp>>);
+    unit.update.prepend(generateTemporaries(unit.update) as Array<ir.StatementOp<ir.UpdateOp>>);
+  }
+}
+
+function generateTemporaries(ops: ir.OpList<ir.CreateOp|ir.UpdateOp>):
+    Array<ir.StatementOp<ir.CreateOp|ir.UpdateOp>> {
+  let opCount = 0;
+  let generatedStatements: Array<ir.StatementOp<ir.UpdateOp>> = [];
 
-      // 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;
-      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.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++}`);
-          }
-          assignName(defs, expr);
-        } else if (expr instanceof ir.ReadTemporaryExpr) {
-          if (finalReads.get(expr.xref) === expr) {
-            released.add(expr.xref);
-            count--;
-          }
-          assignName(defs, expr);
+  // For each op, search for any variables that are assigned or read. For each variable, generate a
+  // name and produce a `DeclareVarStmt` to the beginning of the block.
+  for (const op of ops) {
+    // Identify the final time each temp var is read.
+    const finalReads = new Map<ir.XrefId, ir.ReadTemporaryExpr>();
+    ir.visitExpressionsInOp(op, (expr, flag) => {
+      if (flag & ir.VisitorContextFlag.InChildOperation) {
+        return;
+      }
+      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;
+    const assigned = new Set<ir.XrefId>();
+    const released = new Set<ir.XrefId>();
+    const defs = new Map<ir.XrefId, string>();
+    ir.visitExpressionsInOp(op, (expr, flag) => {
+      if (flag & ir.VisitorContextFlag.InChildOperation) {
+        return;
+      }
+      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++}`);
+        }
+        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(new Set(defs.values()))
-              .map(name => ir.createStatementOp<ir.UpdateOp>(new o.DeclareVarStmt(name))));
-      opCount++;
+    // Add declarations for the temp vars.
+    generatedStatements.push(
+        ...Array.from(new Set(defs.values()))
+            .map(name => ir.createStatementOp<ir.UpdateOp>(new o.DeclareVarStmt(name))));
+    opCount++;
+
+    if (op.kind === ir.OpKind.Listener) {
+      op.handlerOps.prepend(generateTemporaries(op.handlerOps) as ir.UpdateOp[]);
     }
-    unit.update.prepend(generatedStatements);
   }
+
+  return generatedStatements;
 }
 
 /**

Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,9 @@`
`3`	`3`	`"cases": [`
`4`	`4`	`{`
`5`	`5`	`"description": "should handle nullish coalescing inside interpolations",`
`6`		`- "inputFiles": ["nullish_coalescing_interpolation.ts"],`
	`6`	`+ "inputFiles": [`
	`7`	`+ "nullish_coalescing_interpolation.ts"`
	`8`	`+ ],`
`7`	`9`	`"expectations": [`
`8`	`10`	`{`
`9`	`11`	`"files": [`
`@@ -18,7 +20,9 @@`
`18`	`20`	`},`
`19`	`21`	`{`
`20`	`22`	`"description": "should handle nullish coalescing inside property bindings",`
`21`		`- "inputFiles": ["nullish_coalescing_property.ts"],`
	`23`	`+ "inputFiles": [`
	`24`	`+ "nullish_coalescing_property.ts"`
	`25`	`+ ],`
`22`	`26`	`"expectations": [`
`23`	`27`	`{`
`24`	`28`	`"files": [`
`@@ -33,7 +37,9 @@`
`33`	`37`	`},`
`34`	`38`	`{`
`35`	`39`	`"description": "should handle nullish coalescing inside host bindings",`
`36`		`- "inputFiles": ["nullish_coalescing_host.ts"],`
	`40`	`+ "inputFiles": [`
	`41`	`+ "nullish_coalescing_host.ts"`
	`42`	`+ ],`
`37`	`43`	`"expectations": [`
`38`	`44`	`{`
`39`	`45`	`"files": [`
`@@ -44,8 +50,7 @@`
`44`	`50`	`],`
`45`	`51`	`"failureMessage": "Incorrect host bindings"`
`46`	`52`	`}`
`47`		`- ],`
`48`		`- "skipForTemplatePipeline": true`
	`53`	`+ ]`
`49`	`54`	`}`
`50`	`55`	`]`
`51`	`56`	`}`