From 859b6c298fbf3b2e8d2a2f2a0fcd4e537efcc860 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Mon, 25 Sep 2023 20:36:36 -0700 Subject: [PATCH] refactor(compiler): Additional fixes for pipes in i18n (#51876) Adds a new test to verify pipe behavior in i18n blocks, and makes serveral fixes to allow the test to pass. PR Close #51876 --- .../r3_view_compiler_i18n/TEST_CASES.json | 14 +++ .../r3_view_compiler_i18n/multiple_pipes.js | 31 +++++ .../r3_view_compiler_i18n/multiple_pipes.ts | 39 +++++++ .../template/pipeline/ir/src/ops/update.ts | 19 ++- .../src/template/pipeline/src/emit.ts | 2 + .../src/phases/apply_i18n_expressions.ts | 41 +++++++ .../template/pipeline/src/phases/chaining.ts | 27 ++--- .../src/phases/i18n_text_extraction.ts | 10 +- .../src/phases/resolve_i18n_placeholders.ts | 108 +++++++++--------- 9 files changed, 210 insertions(+), 81 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/multiple_pipes.js create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/multiple_pipes.ts create mode 100644 packages/compiler/src/template/pipeline/src/phases/apply_i18n_expressions.ts diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/TEST_CASES.json index d49c427d374..de6daa170b6 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/TEST_CASES.json @@ -15,6 +15,20 @@ } ], "skipForTemplatePipeline": true + }, + { + "description": "should support i18n message with multiple pipes", + "inputFiles": [ + "multiple_pipes.ts" + ], + "expectations": [ + { + "extraChecks": [ + "verifyPlaceholdersIntegrity", + "verifyUniqueConsts" + ] + } + ] } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/multiple_pipes.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/multiple_pipes.js new file mode 100644 index 00000000000..3612f92ff77 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/multiple_pipes.js @@ -0,0 +1,31 @@ +consts: function() { + __i18nMsg__('{$interpolation} and {$interpolation_1}', [['interpolation', String.raw`\uFFFD0\uFFFD`], ['interpolation_1', String.raw`\uFFFD1\uFFFD`]], {original_code: {'interpolation': '{{ valueA | pipeA }}', 'interpolation_1': '{{ valueB | pipeB }}'}}, {}) + __i18nMsg__('{$startTagSpan}{$interpolation}{$closeTagSpan} and {$interpolation_1} {$startTagSpan}and {$interpolation_2}{$closeTagSpan}', [['closeTagSpan', String.raw`[\uFFFD/#6\uFFFD|\uFFFD/#9\uFFFD]`], ['interpolation', String.raw`\uFFFD0\uFFFD`], ['interpolation_1', String.raw`\uFFFD1\uFFFD`], ['interpolation_2', String.raw`\uFFFD2\uFFFD`], ['startTagSpan', String.raw`[\uFFFD#6\uFFFD|\uFFFD#9\uFFFD]`]], {original_code: {'closeTagSpan': '', 'interpolation': '{{ valueA | pipeA }}', 'interpolation_1': '{{ valueB | pipeB }}', 'interpolation_2': '{{ valueC | pipeC }}', 'startTagSpan': ''}}, {}) + … +}, +template: function MyComponent_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵelementStart(0, "div"); + $r3$.ɵɵi18n(1, 0); + $r3$.ɵɵpipe(2, "pipeA"); + $r3$.ɵɵpipe(3, "pipeB"); + $r3$.ɵɵelementEnd(); + $r3$.ɵɵelementStart(4, "div"); + $r3$.ɵɵi18nStart(5, 1); + $r3$.ɵɵelement(6, "span"); + $r3$.ɵɵpipe(7, "pipeA"); + $r3$.ɵɵpipe(8, "pipeB"); + $r3$.ɵɵelement(9, "span"); + $r3$.ɵɵpipe(10, "pipeC"); + $r3$.ɵɵi18nEnd(); + $r3$.ɵɵelementEnd(); + } + if (rf & 2) { + $r3$.ɵɵadvance(3); + $r3$.ɵɵi18nExp($r3$.ɵɵpipeBind1(2, …, ctx.valueA))($r3$.ɵɵpipeBind1(3, …, ctx.valueB)); + $r3$.ɵɵi18nApply(1); + $r3$.ɵɵadvance(7); + $r3$.ɵɵi18nExp($r3$.ɵɵpipeBind1(7, …, ctx.valueA))($r3$.ɵɵpipeBind1(8, …, ctx.valueB))($r3$.ɵɵpipeBind1(10, …, ctx.valueC)); + $r3$.ɵɵi18nApply(5); + } +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/multiple_pipes.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/multiple_pipes.ts new file mode 100644 index 00000000000..1dc0c9f432f --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/multiple_pipes.ts @@ -0,0 +1,39 @@ +import {Component, NgModule, Pipe, PipeTransform} from '@angular/core'; + +@Component({ + selector: 'my-component', + template: ` +
{{ valueA | pipeA }} and {{ valueB | pipeB }}
+
{{ valueA | pipeA }} and {{ valueB | pipeB }} and {{ valueC | pipeC }}
+`, +}) +export class MyComponent { + valueA: 0; + valueB: 0; + valueC: 0; +} + +@Pipe({name: 'pipeA'}) +export class PipeA implements PipeTransform { + transform() { + return null; + } +} + +@Pipe({name: 'pipeB'}) +export class PipeB implements PipeTransform { + transform() { + return null; + } +} + +@Pipe({name: 'pipeC'}) +export class PipeC implements PipeTransform { + transform() { + return null; + } +} + +@NgModule({declarations: [MyComponent, PipeA, PipeB, PipeC]}) +export class MyModule { +} diff --git a/packages/compiler/src/template/pipeline/ir/src/ops/update.ts b/packages/compiler/src/template/pipeline/ir/src/ops/update.ts index 47493969b42..5790b1373dc 100644 --- a/packages/compiler/src/template/pipeline/ir/src/ops/update.ts +++ b/packages/compiler/src/template/pipeline/ir/src/ops/update.ts @@ -542,6 +542,11 @@ export interface I18nExpressionOp extends Op, ConsumesVarsTrait, */ expression: o.Expression; + /** + * The i18n placeholder associated with this expression. + */ + i18nPlaceholder: i18n.Placeholder; + sourceSpan: ParseSourceSpan; } @@ -549,11 +554,13 @@ export interface I18nExpressionOp extends Op, ConsumesVarsTrait, * Create an i18n expression op. */ export function createI18nExpressionOp( - target: XrefId, expression: o.Expression, sourceSpan: ParseSourceSpan): I18nExpressionOp { + target: XrefId, expression: o.Expression, i18nPlaceholder: i18n.Placeholder, + sourceSpan: ParseSourceSpan): I18nExpressionOp { return { kind: OpKind.I18nExpression, target, expression, + i18nPlaceholder, sourceSpan, ...NEW_OP, ...TRAIT_CONSUMES_VARS, @@ -577,24 +584,16 @@ export interface I18nApplyOp extends Op, UsesSlotIndexTrait { */ slot: number|null; - /** - * The i18n placeholders associated with this operation. - */ - i18nPlaceholders: i18n.Placeholder[]; - sourceSpan: ParseSourceSpan; } /** * Creates an op to apply i18n expression ops. */ -export function createI18nApplyOp( - target: XrefId, i18nPlaceholders: i18n.Placeholder[], - sourceSpan: ParseSourceSpan): I18nApplyOp { +export function createI18nApplyOp(target: XrefId, sourceSpan: ParseSourceSpan): I18nApplyOp { return { kind: OpKind.I18nApply, target, - i18nPlaceholders, sourceSpan, ...NEW_OP, ...TRAIT_USES_SLOT_INDEX, diff --git a/packages/compiler/src/template/pipeline/src/emit.ts b/packages/compiler/src/template/pipeline/src/emit.ts index 4a2eaa9239e..39b4817d4e9 100644 --- a/packages/compiler/src/template/pipeline/src/emit.ts +++ b/packages/compiler/src/template/pipeline/src/emit.ts @@ -14,6 +14,7 @@ import {CompilationJob, CompilationJobKind as Kind, type ComponentCompilationJob import {phaseAlignPipeVariadicVarOffset} from './phases/align_pipe_variadic_var_offset'; import {phaseFindAnyCasts} from './phases/any_cast'; +import {phaseApplyI18nExpressions} from './phases/apply_i18n_expressions'; import {phaseAttributeExtraction} from './phases/attribute_extraction'; import {phaseBindingSpecialization} from './phases/binding_specialization'; import {phaseChaining} from './phases/chaining'; @@ -81,6 +82,7 @@ const phases: Phase[] = [ {kind: Kind.Tmpl, fn: phaseNoListenersOnTemplates}, {kind: Kind.Tmpl, fn: phasePipeCreation}, {kind: Kind.Tmpl, fn: phaseI18nTextExtraction}, + {kind: Kind.Tmpl, fn: phaseApplyI18nExpressions}, {kind: Kind.Tmpl, fn: phasePipeVariadic}, {kind: Kind.Both, fn: phasePureLiteralStructures}, {kind: Kind.Tmpl, fn: phaseGenerateProjectionDef}, diff --git a/packages/compiler/src/template/pipeline/src/phases/apply_i18n_expressions.ts b/packages/compiler/src/template/pipeline/src/phases/apply_i18n_expressions.ts new file mode 100644 index 00000000000..9e17bf47bf9 --- /dev/null +++ b/packages/compiler/src/template/pipeline/src/phases/apply_i18n_expressions.ts @@ -0,0 +1,41 @@ +/** + * @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 {CompilationJob} from '../compilation'; + + +/** + * Adds apply operations after i18n expressions. + */ +export function phaseApplyI18nExpressions(job: CompilationJob): void { + for (const unit of job.units) { + for (const op of unit.update) { + // Only add apply after expressions that are not followed by more expressions. + if (op.kind === ir.OpKind.I18nExpression && needsApplication(op)) { + // TODO: what should be the source span for the apply op? + ir.OpList.insertAfter(ir.createI18nApplyOp(op.target, null!), op); + } + } + } +} + +/** + * Checks whether the given expression op needs to be followed with an apply op. + */ +function needsApplication(op: ir.I18nExpressionOp) { + // If the next op is not another expression, we need to apply. + if (op.next?.kind !== ir.OpKind.I18nExpression) { + return true; + } + // If the next op is an expression targeting a different i18n block, we need to apply. + if (op.next.target !== op.target) { + return true; + } + return false; +} diff --git a/packages/compiler/src/template/pipeline/src/phases/chaining.ts b/packages/compiler/src/template/pipeline/src/phases/chaining.ts index 1bb976c6db7..374b07299cc 100644 --- a/packages/compiler/src/template/pipeline/src/phases/chaining.ts +++ b/packages/compiler/src/template/pipeline/src/phases/chaining.ts @@ -12,14 +12,20 @@ import * as ir from '../../ir'; import {CompilationJob} from '../compilation'; const CHAINABLE = new Set([ - R3.elementStart, - R3.elementEnd, - R3.element, - R3.property, - R3.hostProperty, - R3.syntheticHostProperty, - R3.styleProp, R3.attribute, + R3.classProp, + R3.element, + R3.elementContainer, + R3.elementContainerEnd, + R3.elementContainerStart, + R3.elementEnd, + R3.elementStart, + R3.hostProperty, + R3.i18nExp, + R3.listener, + R3.listener, + R3.property, + R3.styleProp, R3.stylePropInterpolate1, R3.stylePropInterpolate2, R3.stylePropInterpolate3, @@ -29,13 +35,8 @@ const CHAINABLE = new Set([ R3.stylePropInterpolate7, R3.stylePropInterpolate8, R3.stylePropInterpolateV, - R3.classProp, - R3.listener, - R3.elementContainerStart, - R3.elementContainerEnd, - R3.elementContainer, - R3.listener, R3.syntheticHostListener, + R3.syntheticHostProperty, R3.templateCreate, ]); diff --git a/packages/compiler/src/template/pipeline/src/phases/i18n_text_extraction.ts b/packages/compiler/src/template/pipeline/src/phases/i18n_text_extraction.ts index c7f1510cd1c..35a0b91c6fc 100644 --- a/packages/compiler/src/template/pipeline/src/phases/i18n_text_extraction.ts +++ b/packages/compiler/src/template/pipeline/src/phases/i18n_text_extraction.ts @@ -46,12 +46,14 @@ export function phaseI18nTextExtraction(job: CompilationJob): void { const i18nBlockId = textNodes.get(op.target)!; const ops: ir.UpdateOp[] = []; - for (const expr of op.interpolation.expressions) { - ops.push( - ir.createI18nExpressionOp(i18nBlockId, expr, expr.sourceSpan ?? op.sourceSpan)); + for (let i = 0; i < op.interpolation.expressions.length; i++) { + const expr = op.interpolation.expressions[i]; + const placeholder = op.i18nPlaceholders[i]; + ops.push(ir.createI18nExpressionOp( + i18nBlockId, expr, placeholder, expr.sourceSpan ?? op.sourceSpan)); } if (ops.length > 0) { - ops.push(ir.createI18nApplyOp(i18nBlockId, op.i18nPlaceholders, op.sourceSpan)); + // ops.push(ir.createI18nApplyOp(i18nBlockId, op.i18nPlaceholders, op.sourceSpan)); } ir.OpList.replaceWithMany(op as ir.UpdateOp, ops); break; diff --git a/packages/compiler/src/template/pipeline/src/phases/resolve_i18n_placeholders.ts b/packages/compiler/src/template/pipeline/src/phases/resolve_i18n_placeholders.ts index e07c7ed5129..d89f9cd9b9e 100644 --- a/packages/compiler/src/template/pipeline/src/phases/resolve_i18n_placeholders.ts +++ b/packages/compiler/src/template/pipeline/src/phases/resolve_i18n_placeholders.ts @@ -15,89 +15,69 @@ import {CompilationJob} from '../compilation'; */ const ESCAPE = '\uFFFD'; -/** - * Represents a placeholder value mapping on an I18nStartOp. - */ -interface PlaceholderValue { - op: ir.I18nStartOp; - - placeholder: string; - - value: o.Expression; -} - /** * Resolve the placeholders in i18n messages. */ export function phaseResolveI18nPlaceholders(job: CompilationJob) { for (const unit of job.units) { const i18nOps = new Map(); - let currentOp: ir.I18nStartOp|null = null; - let startTags: PlaceholderValue[] = []; - let closeTags: PlaceholderValue[] = []; + let startTagSlots = new Map(); + let closeTagSlots = new Map(); + let currentI18nOp: ir.I18nStartOp|null = null; - // Fill in values for tag name placeholders. + // Record slots for tag name placeholders. for (const op of unit.create) { switch (op.kind) { case ir.OpKind.I18nStart: - i18nOps.set(op.xref, op); - currentOp = op; - break; case ir.OpKind.I18n: + // Initialize collected slots for a new i18n block. i18nOps.set(op.xref, op); + currentI18nOp = op.kind === ir.OpKind.I18nStart ? op : null; + startTagSlots = new Map(); + closeTagSlots = new Map(); break; case ir.OpKind.I18nEnd: - currentOp = null; + // Add values for tag placeholders. + if (currentI18nOp === null) { + throw Error('Missing corresponding i18n start op for i18n end op'); + } + for (const [placeholder, slots] of startTagSlots) { + currentI18nOp.params.set(placeholder, serializeSlots(slots, true)); + } + for (const [placeholder, slots] of closeTagSlots) { + currentI18nOp.params.set(placeholder, serializeSlots(slots, false)); + } + currentI18nOp = null; break; case ir.OpKind.Element: case ir.OpKind.ElementStart: + // Record slots for tag placeholders. if (op.i18nPlaceholder != undefined) { - if (currentOp === null) { + if (currentI18nOp === null) { throw Error('i18n tag placeholder should only occur inside an i18n block'); } - // In order to add the keys in the same order as TemplateDefinitionBuilder, we - // separately track the start and close tag placeholders. - // TODO: when TemplateDefinitionBuilder compatibility is not required, we can just add - // both keys directly to the map here. - startTags.push({ - op: currentOp, - placeholder: op.i18nPlaceholder.startName, - value: o.literal(`${ESCAPE}#${op.slot}${ESCAPE}`) - }); - closeTags.push({ - op: currentOp, - placeholder: op.i18nPlaceholder.closeName, - value: o.literal(`${ESCAPE}/#${op.slot}${ESCAPE}`) - }); + if (!op.slot) { + throw Error('Slots should be allocated before i18n placeholder resolution'); + } + const {startName, closeName} = op.i18nPlaceholder; + addTagSlot(startTagSlots, startName, op.slot); + addTagSlot(closeTagSlots, closeName, op.slot); } break; } } - // Add the start tags in the order we encountered them, to match TemplateDefinitionBuilder. - for (const {op, placeholder, value} of startTags) { - op.params.set(placeholder, value); - } - // Add the close tags in reverse order that we encountered the start tags, to match - // TemplateDefinitionBuilder. - for (let i = closeTags.length - 1; i >= 0; i--) { - const {op, placeholder, value} = closeTags[i]; - op.params.set(placeholder, value); - } - // Fill in values for each of the expression placeholders applied in i18nApply operations. const i18nBlockPlaceholderIndices = new Map(); for (const op of unit.update) { - if (op.kind === ir.OpKind.I18nApply) { - for (const placeholder of op.i18nPlaceholders) { - const i18nOp = i18nOps.get(op.target); - let index = i18nBlockPlaceholderIndices.get(op.target) || 0; - if (!i18nOp) { - throw Error('Cannot find corresponding i18nStart for i18nApply'); - } - i18nOp.params.set(placeholder.name, o.literal(`${ESCAPE}${index++}${ESCAPE}`)); - i18nBlockPlaceholderIndices.set(op.target, index); + if (op.kind === ir.OpKind.I18nExpression) { + const i18nOp = i18nOps.get(op.target); + let index = i18nBlockPlaceholderIndices.get(op.target) || 0; + if (!i18nOp) { + throw Error('Cannot find corresponding i18nStart for i18nExpr'); } + i18nOp.params.set(op.i18nPlaceholder.name, o.literal(`${ESCAPE}${index++}${ESCAPE}`)); + i18nBlockPlaceholderIndices.set(op.target, index); } } @@ -111,3 +91,23 @@ export function phaseResolveI18nPlaceholders(job: CompilationJob) { } } } + +/** + * Updates the given slots map with the specified slot. + */ +function addTagSlot(tagSlots: Map, placeholder: string, slot: number) { + const slots = tagSlots.get(placeholder) || []; + slots.push(slot); + tagSlots.set(placeholder, slots); +} + +/** + * Serializes a list of slots to a string literal expression. + */ +function serializeSlots(slots: number[], start: boolean): o.Expression { + const slotStrings = slots.map(slot => `${ESCAPE}${start ? '' : '/'}#${slot}${ESCAPE}`); + if (slotStrings.length === 1) { + return o.literal(slotStrings[0]); + } + return o.literal(`[${slotStrings.join('|')}]`); +}