refactor(compiler): computed for loop variables exposed on the wrong scope (#51618)

Fixes that the computed for loop variables (e.g. $first and $last) were exposed on the parent scope instead of the for loop scope.

PR Close #51618

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/GOLDEN_PARTIAL.js

@@ -1192,3 +1192,54 @@ export declare class MyApp {
     static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
 }
 
+/****************************************************************************************************
+ * PARTIAL FILE: for_template_variables_scope.js
+ ****************************************************************************************************/
+import { Component } from '@angular/core';
+import * as i0 from "@angular/core";
+export class MyApp {
+    constructor() {
+        this.message = 'hello';
+        this.items = [];
+    }
+}
+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: "14.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, selector: "ng-component", ngImport: i0, template: `
+    {{$index}} {{$count}} {{$first}} {{$last}}
+
+    {#for item of items; track item}
+      {{$index}} {{$count}} {{$first}} {{$last}}
+    {/for}
+
+    {{$index}} {{$count}} {{$first}} {{$last}}
+  `, isInline: true });
+i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
+            type: Component,
+            args: [{
+                    template: `
+    {{$index}} {{$count}} {{$first}} {{$last}}
+
+    {#for item of items; track item}
+      {{$index}} {{$count}} {{$first}} {{$last}}
+    {/for}
+
+    {{$index}} {{$count}} {{$first}} {{$last}}
+  `,
+                }]
+        }] });
+
+/****************************************************************************************************
+ * PARTIAL FILE: for_template_variables_scope.d.ts
+ ****************************************************************************************************/
+import * as i0 from "@angular/core";
+export declare class MyApp {
+    message: string;
+    items: never[];
+    $index: any;
+    $count: any;
+    $first: any;
+    $last: any;
+    static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
+    static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
+}
+

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/TEST_CASES.json

@@ -506,6 +506,27 @@
         }
       ],
       "skipForTemplatePipeline": true
+    },
+    {
+      "description": "should not expose for loop variables to the surrounding scope",
+      "angularCompilerOptions": {
+        "_enabledBlockTypes": ["for"]
+      },
+      "inputFiles": [
+        "for_template_variables_scope.ts"
+      ],
+      "expectations": [
+        {
+          "files": [
+            {
+              "expected": "for_template_variables_scope_template.js",
+              "generated": "for_template_variables_scope.js"
+            }
+          ],
+          "failureMessage": "Incorrect template"
+        }
+      ],
+      "skipForTemplatePipeline": true
     }
   ]
 }

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_template_variables_scope.ts

@@ -0,0 +1,23 @@
+import {Component} from '@angular/core';
+
+@Component({
+  template: `
+    {{$index}} {{$count}} {{$first}} {{$last}}
+
+    {#for item of items; track item}
+      {{$index}} {{$count}} {{$first}} {{$last}}
+    {/for}
+
+    {{$index}} {{$count}} {{$first}} {{$last}}
+  `,
+})
+export class MyApp {
+  message = 'hello';
+  items = [];
+
+  // These variables are defined so that the template type checker doesn't raise an error.
+  $index: any;
+  $count: any;
+  $first: any;
+  $last: any;
+}

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_template_variables_scope_template.js

@@ -0,0 +1,24 @@
+function MyApp_For_2_Template(rf, ctx) {
+  if (rf & 1) {
+    $r3$.ɵɵtext(0);
+  }
+  if (rf & 2) {
+    const $index_r2 = ctx.$index;
+    const $count_r3 = ctx.$count;
+    $r3$.ɵɵtextInterpolate4(" ", $index_r2, " ", $count_r3, " ", $index_r2 === 0, " ", $index_r2 === $count_r3 - 1, " ");
+  }
+}
+…
+function MyApp_Template(rf, ctx) {
+  if (rf & 1) {
+    $r3$.ɵɵtext(0);
+    $r3$.ɵɵrepeaterCreate(1, MyApp_For_2_Template, 1, 4, $r3$.ɵɵrepeaterTrackByIdentity);
+    $r3$.ɵɵtext(3);
+  }
+  if (rf & 2) {
+    $r3$.ɵɵtextInterpolate4(" ", ctx.$index, " ", ctx.$count, " ", ctx.$first, " ", ctx.$last, " ");
+    $r3$.ɵɵrepeater(1, ctx.items);
+    $r3$.ɵɵadvance(3);
+    $r3$.ɵɵtextInterpolate4(" ", ctx.$index, " ", ctx.$count, " ", ctx.$first, " ", ctx.$last, " ");
+  }
+}

packages/compiler/src/render3/view/template.ts

@@ -158,7 +158,8 @@ function createComponentDefConsts(): ComponentDefConsts {
 
 class TemplateData {
   constructor(
-      readonly name: string, readonly index: number, private visitor: TemplateDefinitionBuilder) {}
+      readonly name: string, readonly index: number, readonly scope: BindingScope,
+      private visitor: TemplateDefinitionBuilder) {}
 
   getConstCount() {
     return this.visitor.getConstCount();
@@ -958,7 +959,7 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
       }
     });
 
-    return new TemplateData(name, index, visitor);
+    return new TemplateData(name, index, visitor._bindingScope, visitor);
   }
 
   private createEmbeddedTemplateFn(
@@ -1428,8 +1429,13 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
     // Allocate one slot for the repeater metadata. The slots for the primary and empty block
     // are implicitly inferred by the runtime to index + 1 and index + 2.
     const blockIndex = this.allocateDataSlot();
-    const templateVariables = this.createForLoopVariables(block);
-    const primaryData = this.prepareEmbeddedTemplateFn(block.children, '_For', templateVariables);
+    const primaryData = this.prepareEmbeddedTemplateFn(block.children, '_For', [
+      new t.Variable(block.itemName, '$implicit', block.sourceSpan, block.sourceSpan),
+      new t.Variable(
+          getLoopLocalName(block, '$index'), '$index', block.sourceSpan, block.sourceSpan),
+      new t.Variable(
+          getLoopLocalName(block, '$count'), '$count', block.sourceSpan, block.sourceSpan),
+    ]);
     const emptyData = block.empty === null ?
         null :
         this.prepareEmbeddedTemplateFn(block.empty.children, '_ForEmpty');
@@ -1437,6 +1443,8 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
     const value = block.expression.visit(this._valueConverter);
     this.allocateBindingSlots(value);
 
+    this.registerComputedLoopVariables(block, primaryData.scope);
+
     // `repeaterCreate(0, ...)`
     this.creationInstruction(block.sourceSpan, R3.repeaterCreate, () => {
       const params = [
@@ -1462,32 +1470,27 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
         () => [o.literal(blockIndex), this.convertPropertyBinding(value)]);
   }
 
-  private createForLoopVariables(block: t.ForLoopBlock): t.Variable[] {
+  private registerComputedLoopVariables(block: t.ForLoopBlock, bindingScope: BindingScope): void {
     const indexLocalName = getLoopLocalName(block, '$index');
     const countLocalName = getLoopLocalName(block, '$count');
+    const level = bindingScope.bindingLevel;
 
-    this._bindingScope.set(
-        this.level, getLoopLocalName(block, '$odd'),
+    bindingScope.set(
+        level, getLoopLocalName(block, '$odd'),
         scope => scope.get(indexLocalName)!.modulo(o.literal(2)).notIdentical(o.literal(0)));
 
-    this._bindingScope.set(
-        this.level, getLoopLocalName(block, '$even'),
+    bindingScope.set(
+        level, getLoopLocalName(block, '$even'),
         scope => scope.get(indexLocalName)!.modulo(o.literal(2)).identical(o.literal(0)));
 
-    this._bindingScope.set(
-        this.level, getLoopLocalName(block, '$first'),
+    bindingScope.set(
+        level, getLoopLocalName(block, '$first'),
         scope => scope.get(indexLocalName)!.identical(o.literal(0)));
 
-    this._bindingScope.set(
-        this.level, getLoopLocalName(block, '$last'),
+    bindingScope.set(
+        level, getLoopLocalName(block, '$last'),
         scope =>
             scope.get(indexLocalName)!.identical(scope.get(countLocalName)!.minus(o.literal(1))));
-
-    return [
-      new t.Variable(block.itemName, '$implicit', block.sourceSpan, block.sourceSpan),
-      new t.Variable(indexLocalName, '$index', block.sourceSpan, block.sourceSpan),
-      new t.Variable(countLocalName, '$count', block.sourceSpan, block.sourceSpan),
-    ];
   }
 
   private createTrackByFunction(block: t.ForLoopBlock): o.Expression {