From 1fa5020739666a783eca156e6c1921bd41813703 Mon Sep 17 00:00:00 2001 From: Dylan Hunn Date: Thu, 19 Oct 2023 17:32:45 -0700 Subject: [PATCH] refactor(compiler): Imitate TemplateDefinitionBuilder's variable offset assignment order (#52289) Many instructions consume variable slots, which are used to persist data between update runs. For top-level instructions, the offset into the variable data array is implicitly advanced, because those instructions always run. However, instructions in non-top-level expressions cannot be assumed to run every time, because they might be conditionally executed. Therefore, they cannot implicitly advance the offset into the variable data, and must be given an explicitly assigned variable offset. TemplateDefinitionBuilder assigned offsets top-to-bottom for all instructions *except* pure functions. Pure functions would be assigned offsets lazily, on a second pass. Template Pipeline can now imitate this behavior, when in compatibility mode: pure functions are assigned offsets on a second pass. This also makes the "variadic var offsets" phase unnecessary -- the new approach is more general and correct. PR Close #52289 --- .../elements/TEST_CASES.json | 3 +- .../src/template/pipeline/src/emit.ts | 2 - .../phases/align_pipe_variadic_var_offset.ts | 51 ------------------- .../pipeline/src/phases/var_counting.ts | 28 ++++++++++ 4 files changed, 29 insertions(+), 55 deletions(-) delete mode 100644 packages/compiler/src/template/pipeline/src/phases/align_pipe_variadic_var_offset.ts diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/TEST_CASES.json index 73b58809717..aeed83f7aa7 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/TEST_CASES.json @@ -194,8 +194,7 @@ ], "failureMessage": "Incorrect generated template." } - ], - "skipForTemplatePipeline": true + ] }, { "description": "should reserve slots for pure functions in host binding function", diff --git a/packages/compiler/src/template/pipeline/src/emit.ts b/packages/compiler/src/template/pipeline/src/emit.ts index 31b62ec6077..934106152d7 100644 --- a/packages/compiler/src/template/pipeline/src/emit.ts +++ b/packages/compiler/src/template/pipeline/src/emit.ts @@ -13,7 +13,6 @@ import * as ir from '../ir'; import {CompilationJob, CompilationJobKind as Kind, type ComponentCompilationJob, type HostBindingCompilationJob, type ViewCompilationUnit} from './compilation'; -import {phaseAlignPipeVariadicVarOffset} from './phases/align_pipe_variadic_var_offset'; import {phaseFindAnyCasts} from './phases/any_cast'; import {phaseApplyI18nExpressions} from './phases/apply_i18n_expressions'; import {phaseAssignI18nSlotDependencies} from './phases/assign_i18n_slot_dependencies'; @@ -128,7 +127,6 @@ const phases: Phase[] = [ {kind: Kind.Tmpl, fn: phaseEmptyElements}, {kind: Kind.Tmpl, fn: phaseNonbindable}, {kind: Kind.Both, fn: phasePureFunctionExtraction}, - {kind: Kind.Tmpl, fn: phaseAlignPipeVariadicVarOffset}, {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/align_pipe_variadic_var_offset.ts b/packages/compiler/src/template/pipeline/src/phases/align_pipe_variadic_var_offset.ts deleted file mode 100644 index 1ef13e2d235..00000000000 --- a/packages/compiler/src/template/pipeline/src/phases/align_pipe_variadic_var_offset.ts +++ /dev/null @@ -1,51 +0,0 @@ -/** - * @license - * Copyright Google LLC All Rights Reserved. - * - * Use of this source code is governed by an MIT-style license that can be - * found in the LICENSE file at https://angular.io/license - */ - -import * as ir from '../../ir'; - -import type {CompilationJob} from '../compilation'; -import {varsUsedByIrExpression} from './var_counting'; - -export function phaseAlignPipeVariadicVarOffset(job: CompilationJob): void { - for (const unit of job.units) { - for (const op of unit.update) { - ir.visitExpressionsInOp(op, expr => { - if (!(expr instanceof ir.PipeBindingVariadicExpr)) { - return expr; - } - - if (!(expr.args instanceof ir.PureFunctionExpr)) { - return expr; - } - - if (expr.varOffset === null || expr.args.varOffset === null) { - throw new Error(`Must run after variable counting`); - } - - // The structure of this variadic pipe expression is: - // PipeBindingVariadic(#, Y, PureFunction(X, ...ARGS)) - // Where X and Y are the slot offsets for the variables used by these operations, and Y > X. - - // In `TemplateDefinitionBuilder` the PipeBindingVariadic variable slots are allocated - // before the PureFunction slots, which is unusually out-of-order. - // - // To maintain identical output for the tests in question, we adjust the variable offsets of - // these two calls to emulate TDB's behavior. This is not perfect, because the ARGS of the - // PureFunction call may also allocate slots which by TDB's ordering would come after X, and - // we don't account for that. Still, this should be enough to pass the existing pipe tests. - - // Put the PipeBindingVariadic vars where the PureFunction vars were previously allocated. - expr.varOffset = expr.args.varOffset; - - // Put the PureFunction vars following the PipeBindingVariadic vars. - expr.args.varOffset = expr.varOffset + varsUsedByIrExpression(expr); - return undefined; - }); - } - } -} diff --git a/packages/compiler/src/template/pipeline/src/phases/var_counting.ts b/packages/compiler/src/template/pipeline/src/phases/var_counting.ts index f1a81afdfd8..7a5477543ba 100644 --- a/packages/compiler/src/template/pipeline/src/phases/var_counting.ts +++ b/packages/compiler/src/template/pipeline/src/phases/var_counting.ts @@ -34,6 +34,14 @@ export function phaseVarCounting(job: CompilationJob): void { return; } + // TemplateDefinitionBuilder assigns variable offsets for everything but pure functions + // first, and then assigns offsets to pure functions lazily. We emulate that behavior by + // assigning offsets in two passes instead of one, only in compatibility mode. + if (job.compatibility === ir.CompatibilityMode.TemplateDefinitionBuilder && + expr instanceof ir.PureFunctionExpr) { + return; + } + // Some expressions require knowledge of the number of variable slots consumed. if (ir.hasUsesVarOffsetTrait(expr)) { expr.varOffset = varCount; @@ -45,6 +53,26 @@ export function phaseVarCounting(job: CompilationJob): void { }); } + // Compatiblity mode pass for pure function offsets (as explained above). + if (job.compatibility === ir.CompatibilityMode.TemplateDefinitionBuilder) { + for (const op of unit.ops()) { + ir.visitExpressionsInOp(op, expr => { + if (!ir.isIrExpression(expr) || !(expr instanceof ir.PureFunctionExpr)) { + return; + } + + // Some expressions require knowledge of the number of variable slots consumed. + if (ir.hasUsesVarOffsetTrait(expr)) { + expr.varOffset = varCount; + } + + if (ir.hasConsumesVarsTrait(expr)) { + varCount += varsUsedByIrExpression(expr); + } + }); + } + } + unit.vars = varCount; }