From aa6bb8ee95bb4e6d530ac4b74e346612928bca6e Mon Sep 17 00:00:00 2001 From: Dylan Hunn Date: Thu, 28 Sep 2023 12:44:25 -0700 Subject: [PATCH] refactor(compiler): Fix a bug in which temporaries were being declared in the wrong places (#51950) 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 #51950 --- .../nullish_coalescing/TEST_CASES.json | 15 ++- .../src/phases/temporary_variables.ts | 106 +++++++++++------- 2 files changed, 73 insertions(+), 48 deletions(-) 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 index 3bfadccf874..a7f78af7cf3 100644 --- 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 + ] } ] } \ No newline at end of file diff --git a/packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts b/packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts index 5fcef42358c..dd7b0edf0c0 100644 --- 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,51 +21,71 @@ import {CompilationJob, ComponentCompilationJob} from '../compilation'; */ export function phaseTemporaryVariables(cpl: CompilationJob): void { for (const unit of cpl.units) { - let opCount = 0; - let generatedStatements: Array> = []; - for (const op of unit.ops()) { - // Identify the final time each temp var is read. - const finalReads = new Map(); - 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; - const assigned = new Set(); - const released = new Set(); - const defs = new Map(); - 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); - } - }); - - // Add declarations for the temp vars. - generatedStatements.push( - ...Array.from(new Set(defs.values())) - .map(name => ir.createStatementOp(new o.DeclareVarStmt(name)))); - opCount++; - } - unit.update.prepend(generatedStatements); + unit.create.prepend(generateTemporaries(unit.create) as Array>); + unit.update.prepend(generateTemporaries(unit.update) as Array>); } } +function generateTemporaries(ops: ir.OpList): + Array> { + let opCount = 0; + let generatedStatements: Array> = []; + + // 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.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(); + const released = new Set(); + const defs = new Map(); + 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(new o.DeclareVarStmt(name)))); + opCount++; + + if (op.kind === ir.OpKind.Listener) { + op.handlerOps.prepend(generateTemporaries(op.handlerOps) as ir.UpdateOp[]); + } + } + + return generatedStatements; +} + /** * Assigns a name to the temporary variable in the given temporary variable expression. */