From 2ef1bb960e2a7eaba4e63ac8f0f595a534e1caa5 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Fri, 15 Dec 2023 15:07:49 -0800 Subject: [PATCH] test(compiler): Add a test for handling of duplicate bindings (#53596) Adds a test for handling of duplicate bindings. Fow now we replicate the TDB behavior in template pipeline, which is: For style and class text attributes, only keep the last one. For all other text attributes, add all of the values to the consts array. PR Close #53596 --- .../attribute_bindings/TEST_CASES.json | 12 ++++++ .../attribute_bindings/duplicate_bindings.js | 19 +++++++++ .../attribute_bindings/duplicate_bindings.ts | 19 +++++++++ .../src/template/pipeline/src/emit.ts | 2 + .../pipeline/src/phases/const_collection.ts | 12 +++++- .../src/phases/deduplicate_text_bindings.ts | 41 +++++++++++++++++++ 6 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/duplicate_bindings.js create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/duplicate_bindings.ts create mode 100644 packages/compiler/src/template/pipeline/src/phases/deduplicate_text_bindings.ts diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/TEST_CASES.json index b028b3300da..a43ab9eb44f 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/TEST_CASES.json @@ -110,6 +110,18 @@ ] } ] + }, + { + "description": "should handle duplicate bindings", + "inputFiles": [ + "duplicate_bindings.ts" + ], + "expectations": [ + { + "failureMessage": "Incorrect handling of duplicate bindings" + } + ], + "focusTest": true } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/duplicate_bindings.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/duplicate_bindings.js new file mode 100644 index 00000000000..a3e52af0460 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/duplicate_bindings.js @@ -0,0 +1,19 @@ +consts: [["aria-label", "hello", "aria-label", "hi"], [2, "height", "0"], [1, "cls2"], [3, "tabindex"]], +template: function MyComponent_Template(rf, ctx) { + if (rf & 1) { + i0.ɵɵelement(0, "div", 0); + i0.ɵɵelementStart(1, "div", 1); + i0.ɵɵelement(2, "div", 2)(3, "div")(4, "div", 3)(5, "div")(6, "div"); + i0.ɵɵelementEnd(); + } + if (rf & 2) { + i0.ɵɵadvance(3); + i0.ɵɵattribute("aria-label", ctx.value1)("aria-label", ctx.value2); + i0.ɵɵadvance(1); + i0.ɵɵproperty("tabindex", ctx.value1)("tabindex", ctx.value2); + i0.ɵɵadvance(1); + i0.ɵɵclassMap(ctx.value2); + i0.ɵɵadvance(1); + i0.ɵɵstyleMap(ctx.value2); + } +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/duplicate_bindings.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/duplicate_bindings.ts new file mode 100644 index 00000000000..fac09c9f199 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/attribute_bindings/duplicate_bindings.ts @@ -0,0 +1,19 @@ +import {Component} from '@angular/core'; + +@Component({ + selector: 'my-component', + standalone: true, + template: ` +
+
+
+
+
+
+
+ `, +}) +export class MyComponent { + value1: any; + value2: any; +} diff --git a/packages/compiler/src/template/pipeline/src/emit.ts b/packages/compiler/src/template/pipeline/src/emit.ts index babf9ccd26c..8bfbcbf9280 100644 --- a/packages/compiler/src/template/pipeline/src/emit.ts +++ b/packages/compiler/src/template/pipeline/src/emit.ts @@ -25,6 +25,7 @@ import {collectElementConsts} from './phases/const_collection'; import {convertI18nBindings} from './phases/convert_i18n_bindings'; import {createDeferDepsFns} from './phases/create_defer_deps_fns'; import {createI18nContexts} from './phases/create_i18n_contexts'; +import {deduplicateTextBindings} from './phases/deduplicate_text_bindings'; import {configureDeferInstructions} from './phases/defer_configs'; import {resolveDeferTargetNames} from './phases/defer_resolve_targets'; import {collapseEmptyInstructions} from './phases/empty_elements'; @@ -92,6 +93,7 @@ const phases: Phase[] = [ {kind: Kind.Tmpl, fn: emitNamespaceChanges}, {kind: Kind.Tmpl, fn: propagateI18nBlocks}, {kind: Kind.Tmpl, fn: wrapI18nIcus}, + {kind: Kind.Both, fn: deduplicateTextBindings}, {kind: Kind.Both, fn: specializeStyleBindings}, {kind: Kind.Both, fn: specializeBindings}, {kind: Kind.Both, fn: extractAttributes}, 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 193579e858c..0db2db1a99e 100644 --- a/packages/compiler/src/template/pipeline/src/phases/const_collection.ts +++ b/packages/compiler/src/template/pipeline/src/phases/const_collection.ts @@ -25,7 +25,8 @@ export function collectElementConsts(job: CompilationJob): void { for (const unit of job.units) { for (const op of unit.create) { if (op.kind === ir.OpKind.ExtractedAttribute) { - const attributes = allElementAttributes.get(op.target) || new ElementAttributes(); + const attributes = + allElementAttributes.get(op.target) || new ElementAttributes(job.compatibility); allElementAttributes.set(op.target, attributes); attributes.add(op.bindingKind, op.name, op.expression, op.trustedValueFn); ir.OpList.remove(op); @@ -102,6 +103,8 @@ class ElementAttributes { return this.byKind.get(ir.BindingKind.I18n) ?? FLYWEIGHT_ARRAY; } + constructor(private compatibility: ir.CompatibilityMode) {} + isKnown(kind: ir.BindingKind, name: string, value: o.Expression|null) { const nameToValue = this.known.get(kind) ?? new Set(); this.known.set(kind, nameToValue); @@ -114,7 +117,12 @@ class ElementAttributes { add(kind: ir.BindingKind, name: string, value: o.Expression|null, trustedValueFn: o.Expression|null): void { - if (this.isKnown(kind, name, value)) { + // In compatibility mode, we allow duplicates for attributes to replicate the behavior in + // TemplateDefinitionBuilder. + const allowDuplicates = this.compatibility === ir.CompatibilityMode.TemplateDefinitionBuilder ? + kind === ir.BindingKind.Attribute : + false; + if (this.isKnown(kind, name, value) && !allowDuplicates) { return; } diff --git a/packages/compiler/src/template/pipeline/src/phases/deduplicate_text_bindings.ts b/packages/compiler/src/template/pipeline/src/phases/deduplicate_text_bindings.ts new file mode 100644 index 00000000000..63f54abc431 --- /dev/null +++ b/packages/compiler/src/template/pipeline/src/phases/deduplicate_text_bindings.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 type {CompilationJob} from '../compilation'; + +/** + * Deduplicate text bindings, e.g.
+ */ +export function deduplicateTextBindings(job: CompilationJob): void { + const seen = new Map>(); + for (const unit of job.units) { + for (const op of unit.update.reversed()) { + if (op.kind === ir.OpKind.Binding && op.isTextAttribute) { + const seenForElement = seen.get(op.target) || new Set(); + if (seenForElement.has(op.name)) { + if (job.compatibility === ir.CompatibilityMode.TemplateDefinitionBuilder) { + // For most duplicated attributes, TemplateDefinitionBuilder lists all of the values in + // the consts array. However, for style and class attributes it only keeps the last one. + // We replicate that behavior here since it has actual consequences for apps with + // duplicate class or style attrs. + if (op.name === 'style' || op.name === 'class') { + ir.OpList.remove(op); + } + } else { + // TODO: Determine the correct behavior. It would probably make sense to merge multiple + // style and class attributes. Alternatively we could just throw an error, as HTML + // doesn't permit duplicate attributes. + } + } + seenForElement.add(op.name); + seen.set(op.target, seenForElement); + } + } + } +}