From de5c9ca8e9a026ad752aab348bd137f647cc3cc9 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sat, 16 Dec 2023 19:24:16 +0100 Subject: [PATCH] fix(compiler): correctly intercept index in loop tracking function (#53604) The for loop tracking function doesn't allow references to local template variables, aside from `$index` and the item which are passed in as parameters. We enforce this by rewriting all variable references to the components scope. The problem is that the logic that rewrites the references first walks the view tree and then checks if the variable is `$index` or the item. This is problematic in nested for loops, because it'll find the `$index` of the parent. These changes resolve the issue by checking for `$index` and the item first. Fixes #53600. PR Close #53604 --- .../GOLDEN_PARTIAL.js | 51 +++++++++++++++++++ .../TEST_CASES.json | 17 +++++++ .../nested_for_tracking_function.ts | 19 +++++++ .../nested_for_tracking_function_template.js | 44 ++++++++++++++++ .../compiler/src/render3/view/template.ts | 12 ++--- 5 files changed, 137 insertions(+), 6 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function.ts create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function_template.js diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/GOLDEN_PARTIAL.js index ea5163c8421..ca395e33ba2 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/GOLDEN_PARTIAL.js @@ -1926,3 +1926,54 @@ export declare class MyApp { static ɵcmp: i0.ɵɵComponentDeclaration; } +/**************************************************************************************************** + * PARTIAL FILE: nested_for_tracking_function.js + ****************************************************************************************************/ +import { Component } from '@angular/core'; +import * as i0 from "@angular/core"; +export class MyApp { + constructor() { + this.items = []; + this.trackByGrandparent = (item, index) => index; + this.trackByParent = (item, index) => index; + this.trackByChild = (item, index) => index; + } +} +MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component }); +MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, selector: "ng-component", ngImport: i0, template: ` + @for (grandparent of items; track trackByGrandparent(grandparent, $index)) { + @for (parent of grandparent.items; track trackByParent(parent, $index)) { + @for (child of parent.items; track trackByChild(child, $index)) { + + } + } + } + `, isInline: true }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{ + type: Component, + args: [{ + template: ` + @for (grandparent of items; track trackByGrandparent(grandparent, $index)) { + @for (parent of grandparent.items; track trackByParent(parent, $index)) { + @for (child of parent.items; track trackByChild(child, $index)) { + + } + } + } + `, + }] + }] }); + +/**************************************************************************************************** + * PARTIAL FILE: nested_for_tracking_function.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +export declare class MyApp { + items: any[]; + trackByGrandparent: (item: any, index: number) => number; + trackByParent: (item: any, index: number) => number; + trackByChild: (item: any, index: number) => number; + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵcmp: i0.ɵɵComponentDeclaration; +} + diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/TEST_CASES.json index 8e5b68d6c1d..b3f688adde1 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/TEST_CASES.json @@ -617,6 +617,23 @@ } ], "skipForTemplatePipeline": true + }, + { + "description": "should generate tracking function in a nested for loop", + "inputFiles": [ + "nested_for_tracking_function.ts" + ], + "expectations": [ + { + "files": [ + { + "expected": "nested_for_tracking_function_template.js", + "generated": "nested_for_tracking_function.js" + } + ], + "failureMessage": "Incorrect template" + } + ] } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function.ts new file mode 100644 index 00000000000..9d01b5e5898 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function.ts @@ -0,0 +1,19 @@ +import {Component} from '@angular/core'; + +@Component({ + template: ` + @for (grandparent of items; track trackByGrandparent(grandparent, $index)) { + @for (parent of grandparent.items; track trackByParent(parent, $index)) { + @for (child of parent.items; track trackByChild(child, $index)) { + + } + } + } + `, +}) +export class MyApp { + items: any[] = []; + trackByGrandparent = (item: any, index: number) => index; + trackByParent = (item: any, index: number) => index; + trackByChild = (item: any, index: number) => index; +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function_template.js new file mode 100644 index 00000000000..81eac05a0b2 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function_template.js @@ -0,0 +1,44 @@ +function _forTrack0($index, $item) { + return this.trackByGrandparent($item, $index); +} + +function _forTrack1($index, $item) { + return this.trackByParent($item, $index); +} + +function _forTrack2($index, $item) { + return this.trackByChild($item, $index); +} + +function MyApp_For_1_For_1_For_1_Template(rf, ctx) {} + +function MyApp_For_1_For_1_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵrepeaterCreate(0, MyApp_For_1_For_1_For_1_Template, 0, 0, null, null, _forTrack2, true); + } + if (rf & 2) { + const $parent_r7$ = ctx.$implicit; + $r3$.ɵɵrepeater($parent_r7$.items); + } +} + +function MyApp_For_1_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵrepeaterCreate(0, MyApp_For_1_For_1_Template, 2, 0, null, null, _forTrack1, true); + } + if (rf & 2) { + const $grandparent_r1$ = ctx.$implicit; + $r3$.ɵɵrepeater($grandparent_r1$.items); + } +} + +… + +function MyApp_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵrepeaterCreate(0, MyApp_For_1_Template, 2, 0, null, null, _forTrack0, true); + } + if (rf & 2) { + $r3$.ɵɵrepeater(ctx.items); + } +} diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 1a2093ae421..f0186462094 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -2550,11 +2550,16 @@ export class BindingScope implements LocalResolver { class TrackByBindingScope extends BindingScope { private componentAccessCount = 0; - constructor(parentScope: BindingScope, private globalAliases: Record) { + constructor(parentScope: BindingScope, private globalOverrides: Record) { super(parentScope.bindingLevel + 1, parentScope); } override get(name: string): o.Expression|null { + // Intercept any overridden globals. + if (this.globalOverrides.hasOwnProperty(name)) { + return o.variable(this.globalOverrides[name]); + } + let current: BindingScope|null = this.parent; // Prevent accesses of template variables outside the `for` loop. @@ -2565,11 +2570,6 @@ class TrackByBindingScope extends BindingScope { current = current.parent; } - // Intercept any aliased globals. - if (this.globalAliases[name]) { - return o.variable(this.globalAliases[name]); - } - // When the component scope is accessed, we redirect it through `this`. this.componentAccessCount++; return o.variable('this').prop(name);