From 24177e973bf696fe661f1f28dfdc412c19db889c Mon Sep 17 00:00:00 2001 From: Dylan Hunn Date: Wed, 6 Dec 2023 15:40:32 -0800 Subject: [PATCH] refactor(compiler): Keep a TemplateKind on various binding ops (#53405) Previously, binding ops only knew whether they applied to a structural template (and even this was actually very misleading!). Now, binding ops have full information about what kind of template they apply to, if any (e.g. plain template, structural template, etc). Additionally, each binding knows whether it `IsStructuralTemplateAttribute`, which is a property of the binding rather than the template target. In the future, we should refactor this to unify the various flags that can describe binding types, as well as the flags that describe template targets, into a single and comprehensive field on binding ops. PR Close #53405 --- .../element_attributes/TEST_CASES.json | 20 ++++-- ...late_interpolation_structural_template.js} | 0 ...erpolation_structural_template.pipeline.js | 29 ++++++++ ... => ng-template_interpolation_template.js} | 0 ...emplate_interpolation_template.pipeline.js | 18 +++++ .../template/pipeline/ir/src/ops/update.ts | 66 +++++++++++-------- .../src/template/pipeline/src/ingest.ts | 30 +++++---- .../src/phases/attribute_extraction.ts | 7 +- .../src/phases/binding_specialization.ts | 7 +- .../pipeline/src/phases/const_collection.ts | 1 + 10 files changed, 130 insertions(+), 48 deletions(-) rename packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/{ng-template_interpolation_structural.js => ng-template_interpolation_structural_template.js} (100%) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_structural_template.pipeline.js rename packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/{ng-template_interpolation.js => ng-template_interpolation_template.js} (100%) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_template.pipeline.js diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/TEST_CASES.json index bcedae737ae..a084126685a 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/TEST_CASES.json @@ -54,10 +54,16 @@ "extraChecks": [ "verifyPlaceholdersIntegrity", "verifyUniqueConsts" + ], + "files": [ + { + "expected": "ng-template_interpolation_template.js", + "templatePipelineExpected": "ng-template_interpolation_template.pipeline.js", + "generated": "ng-template_interpolation.js" + } ] } - ], - "skipForTemplatePipeline": true + ] }, { "description": "should support i18n attributes with interpolations on explicit elements with structural directives", @@ -69,10 +75,16 @@ "extraChecks": [ "verifyPlaceholdersIntegrity", "verifyUniqueConsts" + ], + "files": [ + { + "expected": "ng-template_interpolation_structural_template.js", + "templatePipelineExpected": "ng-template_interpolation_structural_template.pipeline.js", + "generated": "ng-template_interpolation_structural.js" + } ] } - ], - "skipForTemplatePipeline": true + ] }, { "description": "should not create translations for empty attributes", diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_structural.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_structural_template.js similarity index 100% rename from packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_structural.js rename to packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_structural_template.js diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_structural_template.pipeline.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_structural_template.pipeline.js new file mode 100644 index 00000000000..6a37a0e961f --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_structural_template.pipeline.js @@ -0,0 +1,29 @@ +function MyComponent_0_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵtemplate(0, MyComponent_0_ng_template_0_Template, 0, 0, "ng-template", 2); + $r3$.ɵɵi18nAttributes(1, 0); + } + if (rf & 2) { + const $ctx_r2$ = $r3$.ɵɵnextContext(); + $r3$.ɵɵi18nExp($ctx_r2$.name); + $r3$.ɵɵi18nApply(1); + } + } + … + consts: () => { + __i18nMsg__('Hello {$interpolation}', [['interpolation', String.raw`\uFFFD0\uFFFD`]], {original_code: {'interpolation': '{{ name }}'}}, {}) + return [ + ["title", $i18n_0$], + [__AttributeMarker.Template__, "ngIf"], + [__AttributeMarker.Bindings__, "title"] + ]; + }, + template: function MyComponent_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵtemplate(0, MyComponent_0_Template, 2, 1, null, 1); + } + if (rf & 2) { + $r3$.ɵɵproperty("ngIf", true); + } + }, + \ No newline at end of file diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_template.js similarity index 100% rename from packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation.js rename to packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_template.js diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_template.pipeline.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_template.pipeline.js new file mode 100644 index 00000000000..db47a1ed314 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/element_attributes/ng-template_interpolation_template.pipeline.js @@ -0,0 +1,18 @@ +consts: () => { + __i18nMsg__('Hello {$interpolation}', [['interpolation', String.raw`\uFFFD0\uFFFD`]], {original_code: {'interpolation': '{{ name }}'}}, {}) + return [ + ["title", $i18n_0$], + [__AttributeMarker.Bindings__, "title"] + ]; + }, + template: function MyComponent_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵtemplate(0, MyComponent_ng_template_0_Template, 0, 0, "ng-template", 1); + $r3$.ɵɵi18nAttributes(1, 0); + } + if (rf & 2) { + $r3$.ɵɵi18nExp(ctx.name); + $r3$.ɵɵi18nApply(1); + } + } + \ No newline at end of file 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 7521901d7b4..a0c469d213c 100644 --- a/packages/compiler/src/template/pipeline/ir/src/ops/update.ts +++ b/packages/compiler/src/template/pipeline/ir/src/ops/update.ts @@ -10,7 +10,7 @@ import {SecurityContext} from '../../../../../core'; import * as i18n from '../../../../../i18n/i18n_ast'; import * as o from '../../../../../output/output_ast'; import {ParseSourceSpan} from '../../../../../parse_util'; -import {BindingKind, I18nExpressionFor, I18nParamResolutionTime, OpKind} from '../enums'; +import {BindingKind, I18nExpressionFor, I18nParamResolutionTime, OpKind, TemplateKind} from '../enums'; import type {ConditionalCaseExpr} from '../expression'; import {SlotHandle} from '../handle'; import {Op, XrefId} from '../operations'; @@ -121,10 +121,12 @@ export interface BindingOp extends Op { */ isTextAttribute: boolean; + isStructuralTemplateAttribute: boolean; + /** * Whether this binding is on a structural template. */ - isStructuralTemplate: boolean; + templateKind: TemplateKind|null; i18nContext: XrefId|null; i18nMessage: i18n.Message|null; @@ -138,8 +140,8 @@ export interface BindingOp extends Op { export function createBindingOp( target: XrefId, kind: BindingKind, name: string, expression: o.Expression|Interpolation, unit: string|null, securityContext: SecurityContext, isTextAttribute: boolean, - isStructuralTemplate: boolean, i18nMessage: i18n.Message|null, - sourceSpan: ParseSourceSpan): BindingOp { + isStructuralTemplateAttribute: boolean, templateKind: TemplateKind|null, + i18nMessage: i18n.Message|null, sourceSpan: ParseSourceSpan): BindingOp { return { kind: OpKind.Binding, bindingKind: kind, @@ -149,7 +151,8 @@ export function createBindingOp( unit, securityContext, isTextAttribute, - isStructuralTemplate: isStructuralTemplate, + isStructuralTemplateAttribute, + templateKind, i18nContext: null, i18nMessage, sourceSpan, @@ -193,10 +196,13 @@ export interface PropertyOp extends Op, ConsumesVarsTrait, DependsOnSl */ sanitizer: o.Expression|null; + isStructuralTemplateAttribute: boolean; + /** - * Whether this binding is on a structural template. + * The kind of template targeted by the binding, or null if this binding does not target a + * template. */ - isStructuralTemplate: boolean; + templateKind: TemplateKind|null; i18nContext: XrefId|null; i18nMessage: i18n.Message|null; @@ -209,7 +215,8 @@ export interface PropertyOp extends Op, ConsumesVarsTrait, DependsOnSl */ export function createPropertyOp( target: XrefId, name: string, expression: o.Expression|Interpolation, - isAnimationTrigger: boolean, securityContext: SecurityContext, isStructuralTemplate: boolean, + isAnimationTrigger: boolean, securityContext: SecurityContext, + isStructuralTemplateAttribute: boolean, templateKind: TemplateKind|null, i18nContext: XrefId|null, i18nMessage: i18n.Message|null, sourceSpan: ParseSourceSpan): PropertyOp { return { @@ -220,7 +227,8 @@ export function createPropertyOp( isAnimationTrigger, securityContext, sanitizer: null, - isStructuralTemplate, + isStructuralTemplateAttribute, + templateKind, i18nContext, i18nMessage, sourceSpan, @@ -424,10 +432,13 @@ export interface AttributeOp extends Op { */ isTextAttribute: boolean; + isStructuralTemplateAttribute: boolean; + /** - * Whether this binding is on a structural template. + * The kind of template targeted by the binding, or null if this binding does not target a + * template. */ - isStructuralTemplate: boolean; + templateKind: TemplateKind|null; /** * The i18n context, if this is an i18n attribute. @@ -444,7 +455,8 @@ export interface AttributeOp extends Op { */ export function createAttributeOp( target: XrefId, name: string, expression: o.Expression|Interpolation, - securityContext: SecurityContext, isTextAttribute: boolean, isStructuralTemplate: boolean, + securityContext: SecurityContext, isTextAttribute: boolean, + isStructuralTemplateAttribute: boolean, templateKind: TemplateKind|null, i18nMessage: i18n.Message|null, sourceSpan: ParseSourceSpan): AttributeOp { return { kind: OpKind.Attribute, @@ -454,7 +466,8 @@ export function createAttributeOp( securityContext, sanitizer: null, isTextAttribute, - isStructuralTemplate, + isStructuralTemplateAttribute, + templateKind, i18nContext: null, i18nMessage, sourceSpan, @@ -510,7 +523,8 @@ export interface ConditionalOp extends Op, DependsOnSlotContextOp targetSlot: SlotHandle; /** - * The main test expression (for a switch), or `null` (for an if, which has no test expression). + * The main test expression (for a switch), or `null` (for an if, which has no test + * expression). */ test: o.Expression|null; @@ -527,8 +541,8 @@ export interface ConditionalOp extends Op, DependsOnSlotContextOp processed: o.Expression|null; /** - * Control flow conditionals can accept a context value (this is a result of specifying an alias). - * This expression will be passed to the conditional instruction's context parameter. + * Control flow conditionals can accept a context value (this is a result of specifying an + * alias). This expression will be passed to the conditional instruction's context parameter. */ contextValue: o.Expression|null; @@ -626,8 +640,8 @@ export function createDeferWhenOp( /** * An op that represents an expression in an i18n message. * - * TODO: This can represent expressions used in both i18n attributes and normal i18n content. We may - * want to split these into two different op types, deriving from the same base class. + * TODO: This can represent expressions used in both i18n attributes and normal i18n content. We + * may want to split these into two different op types, deriving from the same base class. */ export interface I18nExpressionOp extends Op, ConsumesVarsTrait, DependsOnSlotContextOpTrait { @@ -641,11 +655,11 @@ export interface I18nExpressionOp extends Op, ConsumesVarsTrait, /** * The Xref of the op that we need to `advance` to. * - * In an i18n block, this is initially the i18n start op, but will eventually correspond to the - * final slot consumer in the owning i18n block. - * TODO: We should make text i18nExpressions target the i18nEnd instruction, instead the last slot - * consumer in the i18n block. This makes them resilient to that last consumer being deleted. (Or - * new slot consumers being added!) + * In an i18n block, this is initially the i18n start op, but will eventually correspond to + * the final slot consumer in the owning i18n block. + * TODO: We should make text i18nExpressions target the i18nEnd instruction, instead the last + * slot consumer in the i18n block. This makes them resilient to that last consumer being + * deleted. (Or new slot consumers being added!) * * In an i18n attribute, this is the xref of the corresponding elementStart/element. */ @@ -733,9 +747,9 @@ export interface I18nApplyOp extends Op { owner: XrefId; /** - * A handle for the slot that i18n apply instruction should apply to. In an i18n block, this is - * the slot of the i18n block this expression belongs to. In an i18n attribute, this is the slot - * of the corresponding i18nAttributes instruction. + * A handle for the slot that i18n apply instruction should apply to. In an i18n block, this + * is the slot of the i18n block this expression belongs to. In an i18n attribute, this is the + * slot of the corresponding i18nAttributes instruction. */ handle: SlotHandle; diff --git a/packages/compiler/src/template/pipeline/src/ingest.ts b/packages/compiler/src/template/pipeline/src/ingest.ts index de38e78e9b7..a4ee92c1ae6 100644 --- a/packages/compiler/src/template/pipeline/src/ingest.ts +++ b/packages/compiler/src/template/pipeline/src/ingest.ts @@ -92,7 +92,7 @@ export function ingestHostProperty( job.root.xref, bindingKind, property.name, expression, null, SecurityContext .NONE /* TODO: what should we pass as security context? Passing NONE for now. */, - isTextAttribute, false, /* TODO: How do Host bindings handle i18n attrs? */ null, + isTextAttribute, false, null, /* TODO: How do Host bindings handle i18n attrs? */ null, property.sourceSpan)); } @@ -100,6 +100,7 @@ export function ingestHostAttribute( job: HostBindingCompilationJob, name: string, value: o.Expression): void { const attrBinding = ir.createBindingOp( job.root.xref, ir.BindingKind.Attribute, name, value, null, SecurityContext.NONE, true, false, + null, /* TODO */ null, /* TODO: host attribute source spans */ null!); job.root.update.push(attrBinding); @@ -165,7 +166,7 @@ function ingestElement(unit: ViewCompilationUnit, element: t.Element): void { element.startSourceSpan); unit.create.push(startOp); - ingestBindings(unit, startOp, element); + ingestBindings(unit, startOp, element, null); ingestReferences(startOp, element); // Start i18n, if needed, goes after the element create and bindings, but before the nodes @@ -220,7 +221,7 @@ function ingestTemplate(unit: ViewCompilationUnit, tmpl: t.Template): void { i18nPlaceholder, tmpl.startSourceSpan); unit.create.push(templateOp); - ingestBindings(unit, templateOp, tmpl); + ingestBindings(unit, templateOp, tmpl, templateKind); ingestReferences(templateOp, tmpl); ingestNodes(childView, tmpl.children); @@ -251,7 +252,7 @@ function ingestContent(unit: ViewCompilationUnit, content: t.Content): void { for (const attr of content.attributes) { ingestBinding( unit, op.xref, attr.name, o.literal(attr.value), e.BindingType.Attribute, null, - SecurityContext.NONE, attr.sourceSpan, BindingFlags.TextValue, attr.i18n); + SecurityContext.NONE, attr.sourceSpan, BindingFlags.TextValue, null, attr.i18n); } unit.create.push(op); } @@ -754,7 +755,8 @@ function isPlainTemplate(tmpl: t.Template) { * to their IR representation. */ function ingestBindings( - unit: ViewCompilationUnit, op: ir.ElementOpBase, element: t.Element|t.Template): void { + unit: ViewCompilationUnit, op: ir.ElementOpBase, element: t.Element|t.Template, + templateKind: ir.TemplateKind|null): void { let flags: BindingFlags = BindingFlags.None; let hasI18nAttributes = false; @@ -771,12 +773,12 @@ function ingestBindings( ingestBinding( unit, op.xref, attr.name, o.literal(attr.value), e.BindingType.Attribute, null, SecurityContext.NONE, attr.sourceSpan, templateAttrFlags | BindingFlags.TextValue, - attr.i18n); + templateKind, attr.i18n); hasI18nAttributes ||= attr.i18n !== undefined; } else { ingestBinding( unit, op.xref, attr.name, attr.value, attr.type, attr.unit, attr.securityContext, - attr.sourceSpan, templateAttrFlags, attr.i18n); + attr.sourceSpan, templateAttrFlags, templateKind, attr.i18n); hasI18nAttributes ||= attr.i18n !== undefined; } } @@ -788,13 +790,14 @@ function ingestBindings( // `BindingType.Attribute`. ingestBinding( unit, op.xref, attr.name, o.literal(attr.value), e.BindingType.Attribute, null, - SecurityContext.NONE, attr.sourceSpan, flags | BindingFlags.TextValue, attr.i18n); + SecurityContext.NONE, attr.sourceSpan, flags | BindingFlags.TextValue, templateKind, + attr.i18n); hasI18nAttributes ||= attr.i18n !== undefined; } for (const input of element.inputs) { ingestBinding( unit, op.xref, input.name, input.value, input.type, input.unit, input.securityContext, - input.sourceSpan, flags, input.i18n); + input.sourceSpan, flags, templateKind, input.i18n); hasI18nAttributes ||= input.i18n !== undefined; } @@ -888,7 +891,8 @@ enum BindingFlags { function ingestBinding( view: ViewCompilationUnit, xref: ir.XrefId, name: string, value: e.AST|o.Expression, type: e.BindingType, unit: string|null, securityContext: SecurityContext, - sourceSpan: ParseSourceSpan, flags: BindingFlags, i18nMeta: i18n.I18nMeta|undefined): void { + sourceSpan: ParseSourceSpan, flags: BindingFlags, templateKind: ir.TemplateKind|null, + i18nMeta: i18n.I18nMeta|undefined): void { if (value instanceof e.ASTWithSource) { value = value.ast; } @@ -926,7 +930,8 @@ function ingestBinding( const kind: ir.BindingKind = BINDING_KINDS.get(type)!; view.update.push(ir.createBindingOp( xref, kind, name, expression, unit, securityContext, !!(flags & BindingFlags.TextValue), - !!(flags & BindingFlags.IsStructuralTemplateAttribute), i18nMeta ?? null, sourceSpan)); + !!(flags & BindingFlags.IsStructuralTemplateAttribute), templateKind, i18nMeta ?? null, + sourceSpan)); } /** @@ -1026,7 +1031,8 @@ function ingestControlFlowInsertionPoint( for (const attr of root.attributes) { ingestBinding( unit, xref, attr.name, o.literal(attr.value), e.BindingType.Attribute, null, - SecurityContext.NONE, attr.sourceSpan, BindingFlags.TextValue, attr.i18n); + SecurityContext.NONE, attr.sourceSpan, BindingFlags.TextValue, ir.TemplateKind.Block, + attr.i18n); } const tagName = root instanceof t.Element ? root.name : root.tagName; diff --git a/packages/compiler/src/template/pipeline/src/phases/attribute_extraction.ts b/packages/compiler/src/template/pipeline/src/phases/attribute_extraction.ts index 9c555002c7e..df0f062aa53 100644 --- a/packages/compiler/src/template/pipeline/src/phases/attribute_extraction.ts +++ b/packages/compiler/src/template/pipeline/src/phases/attribute_extraction.ts @@ -26,11 +26,11 @@ export function extractAttributes(job: CompilationJob): void { case ir.OpKind.Property: if (!op.isAnimationTrigger) { let bindingKind: ir.BindingKind; - if (op.i18nMessage !== null) { + if (op.i18nMessage !== null && op.templateKind === null) { // If the binding has an i18n context, it is an i18n attribute, and should have that // kind in the consts array. bindingKind = ir.BindingKind.I18n; - } else if (op.isStructuralTemplate) { + } else if (op.isStructuralTemplateAttribute) { // TODO: How do i18n attributes on templates work?! bindingKind = ir.BindingKind.Template; } else { @@ -119,7 +119,8 @@ function extractAttributeOp( if (extractable) { const extractedAttributeOp = ir.createExtractedAttributeOp( - op.target, op.isStructuralTemplate ? ir.BindingKind.Template : ir.BindingKind.Attribute, + op.target, + op.isStructuralTemplateAttribute ? ir.BindingKind.Template : ir.BindingKind.Attribute, op.name, op.expression, op.i18nContext, op.i18nMessage); if (unit.job.kind === CompilationJobKind.Host) { // This attribute will apply to the enclosing host binding compilation unit, so order doesn't diff --git a/packages/compiler/src/template/pipeline/src/phases/binding_specialization.ts b/packages/compiler/src/template/pipeline/src/phases/binding_specialization.ts index 5baa6df5b74..da30e372227 100644 --- a/packages/compiler/src/template/pipeline/src/phases/binding_specialization.ts +++ b/packages/compiler/src/template/pipeline/src/phases/binding_specialization.ts @@ -48,7 +48,8 @@ export function specializeBindings(job: CompilationJob): void { op, ir.createAttributeOp( op.target, op.name, op.expression, op.securityContext, op.isTextAttribute, - op.isStructuralTemplate, op.i18nMessage, op.sourceSpan)); + op.isStructuralTemplateAttribute, op.templateKind, op.i18nMessage, + op.sourceSpan)); } break; case ir.BindingKind.Property: @@ -64,8 +65,8 @@ export function specializeBindings(job: CompilationJob): void { op, ir.createPropertyOp( op.target, op.name, op.expression, op.bindingKind === ir.BindingKind.Animation, - op.securityContext, op.isStructuralTemplate, op.i18nContext, op.i18nMessage, - op.sourceSpan)); + op.securityContext, op.isStructuralTemplateAttribute, op.templateKind, + op.i18nContext, op.i18nMessage, op.sourceSpan)); } break; diff --git a/packages/compiler/src/template/pipeline/src/phases/const_collection.ts b/packages/compiler/src/template/pipeline/src/phases/const_collection.ts index c15b9001d8c..893fe0e3a67 100644 --- a/packages/compiler/src/template/pipeline/src/phases/const_collection.ts +++ b/packages/compiler/src/template/pipeline/src/phases/const_collection.ts @@ -105,6 +105,7 @@ class ElementAttributes { return; } this.known.add(name); + // TODO: Can this be its own phase if (name === 'ngProjectAs') { if (value === null || !(value instanceof o.LiteralExpr) || (value.value == null) || (typeof value.value?.toString() !== 'string')) {