fix(compiler-cli): avoid conflicts with built-in global variables in for loop blocks (#53319)

crisbeto · dylhunn · commit d7a83f95213c · 2023-12-04T21:45:17.000-08:00
Currently we generate the following TCB for a `@for` loop: ```ts // @for (item of items; track item) {...} for (const item of this.items) { var _t1 = item; // Do things with `_t1` } ``` This is problematic if the item name is the same as a global variable (e.g. `document`), because when the TCB has references to that variable (e.g. `document.createElement`), it'll find the loop initializer instead of the global variable. These changes fix the issue by generating the following instead: ```ts for (const _t1 of this.items) { // Do things with `_t1` } ``` Fixes #53293. PR Close #53319
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
@@ -1538,8 +1538,13 @@ class TcbForOfOp extends TcbOp {
 
   override execute(): null {
     const loopScope = Scope.forNodes(this.tcb, this.scope, this.block, this.block.children, null);
+    const initializerId = loopScope.resolve(this.block.item);
+    if (!ts.isIdentifier(initializerId)) {
+      throw new Error(
+          `Could not resolve for loop variable ${this.block.item.name} to an identifier`);
+    }
     const initializer = ts.factory.createVariableDeclarationList(
-        [ts.factory.createVariableDeclaration(this.block.item.name)], ts.NodeFlags.Const);
+        [ts.factory.createVariableDeclaration(initializerId)], ts.NodeFlags.Const);
     // It's common to have a for loop over a nullable value (e.g. produced by the `async` pipe).
     // Add a non-null expression to allow such values to be assigned.
     const expression = ts.factory.createNonNullExpression(
@@ -1656,9 +1661,10 @@ class Scope {
 
   /**
    * Map of variables declared on the template that created this `Scope` (represented by
-   * `TmplAstVariable` nodes) to the index of their `TcbVariableOp`s in the `opQueue`.
+   * `TmplAstVariable` nodes) to the index of their `TcbVariableOp`s in the `opQueue`, or to
+   * pre-resolved variable identifiers.
    */
-  private varMap = new Map<TmplAstVariable, number>();
+  private varMap = new Map<TmplAstVariable, number|ts.Identifier>();
 
   /**
    * Statements for this template.
@@ -1730,10 +1736,11 @@ class Scope {
                 tcb, scope, tcbExpression(expression, tcb, scope), expressionAlias));
       }
     } else if (scopedNode instanceof TmplAstForLoopBlock) {
-      this.registerVariable(
-          scope, scopedNode.item,
-          new TcbBlockVariableOp(
-              tcb, scope, ts.factory.createIdentifier(scopedNode.item.name), scopedNode.item));
+      // Register the variable for the loop so it can be resolved by
+      // children. It'll be declared once the loop is created.
+      const loopInitializer = tcb.allocateId();
+      addParseSpanInfo(loopInitializer, scopedNode.item.sourceSpan);
+      scope.varMap.set(scopedNode.item, loopInitializer);
 
       for (const [name, variable] of Object.entries(scopedNode.contextVariables)) {
         if (!this.forLoopContextVariableTypes.has(name)) {
@@ -1865,7 +1872,8 @@ class Scope {
     } else if (ref instanceof TmplAstVariable && this.varMap.has(ref)) {
       // Resolving a context variable for this template.
       // Execute the `TcbVariableOp` associated with the `TmplAstVariable`.
-      return this.resolveOp(this.varMap.get(ref)!);
+      const opIndexOrNode = this.varMap.get(ref)!;
+      return typeof opIndexOrNode === 'number' ? this.resolveOp(opIndexOrNode) : opIndexOrNode;
     } else if (
         ref instanceof TmplAstTemplate && directive === undefined &&
         this.templateCtxOpMap.has(ref)) {
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
@@ -1545,7 +1545,7 @@ describe('type check blocks', () => {
       `;
 
       const result = tcb(TEMPLATE);
-      expect(result).toContain('for (const item of ((this).items)!) { var _t1 = item;');
+      expect(result).toContain('for (const _t1 of ((this).items)!) {');
       expect(result).toContain('"" + ((this).main(_t1))');
       expect(result).toContain('"" + ((this).empty())');
     });
@@ -1558,7 +1558,7 @@ describe('type check blocks', () => {
       `;
 
       const result = tcb(TEMPLATE);
-      expect(result).toContain('for (const item of ((this).items)!) { var _t1 = item;');
+      expect(result).toContain('for (const _t1 of ((this).items)!) {');
       expect(result).toContain('var _t2: number = null!;');
       expect(result).toContain('var _t3: boolean = null! as boolean;');
       expect(result).toContain('var _t4: boolean = null! as boolean;');
@@ -1576,7 +1576,7 @@ describe('type check blocks', () => {
       `;
 
       const result = tcb(TEMPLATE);
-      expect(result).toContain('for (const item of ((this).items)!) { var _t1 = item;');
+      expect(result).toContain('for (const _t1 of ((this).items)!) {');
       expect(result).toContain('var _t2: number = null!;');
       expect(result).toContain('var _t3: boolean = null! as boolean;');
       expect(result).toContain('var _t4: boolean = null! as boolean;');
@@ -1592,7 +1592,7 @@ describe('type check blocks', () => {
       `;
 
       const result = tcb(TEMPLATE);
-      expect(result).toContain('for (const item of ((this).items)!) { var _t1 = item;');
+      expect(result).toContain('for (const _t1 of ((this).items)!) {');
       expect(result).toContain('var _t2: number = null!;');
       expect(result).toContain('"" + (((this).$index)) + (_t2)');
     });
@@ -1609,20 +1609,16 @@ describe('type check blocks', () => {
       `;
 
       const result = tcb(TEMPLATE);
-      expect(result).toContain(
-          'for (const item of ((this).items)!) { var _t1 = item; var _t2: number = null!;');
+      expect(result).toContain('for (const _t1 of ((this).items)!) { var _t2: number = null!;');
       expect(result).toContain('"" + (_t1) + (_t2)');
-      expect(result).toContain(
-          'for (const inner of ((_t1).items)!) { var _t8 = inner; var _t9: number = null!;');
+      expect(result).toContain('for (const _t8 of ((_t1).items)!) { var _t9: number = null!;');
       expect(result).toContain('"" + (_t1) + (_t2) + (_t8) + (_t9)');
     });
 
     it('should generate the tracking expression of a for loop', () => {
       const result = tcb(`@for (item of items; track trackingFn($index, item, prop)) {}`);
-
-      expect(result).toContain(
-          'for (const item of ((this).items)!) { var _t1: number = null!; var _t2 = item;');
-      expect(result).toContain('(this).trackingFn(_t1, _t2, ((this).prop));');
+      expect(result).toContain('for (const _t1 of ((this).items)!) { var _t2: number = null!;');
+      expect(result).toContain('(this).trackingFn(_t2, _t1, ((this).prop));');
     });
   });
 });
diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
@@ -5019,6 +5019,40 @@ suppress
           `Type 'number' must have a '[Symbol.iterator]()' method that returns an iterator.`
         ]);
       });
+
+      it('should check for loop variables with the same name as built-in globals', () => {
+        // strictTemplates are necessary so the event listener is checked.
+        env.tsconfig({strictTemplates: true});
+        env.write('test.ts', `
+          import {Component, Directive, Input} from '@angular/core';
+
+          @Directive({
+            standalone: true,
+            selector: '[dir]'
+          })
+          export class Dir {
+            @Input('dir') value!: string;
+          }
+
+          @Component({
+            standalone: true,
+            imports: [Dir],
+            template: \`
+              @for (document of documents; track document) {
+                <button [dir]="document" (click)="$event.stopPropagation()"></button>
+              }
+            \`,
+          })
+          export class Main {
+            documents = [1, 2, 3];
+          }
+        `);
+
+        const diags = env.driveDiagnostics();
+        expect(diags.map(d => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
+          `Type 'number' is not assignable to type 'string'.`,
+        ]);
+      });
     });
 
     describe('control flow content projection diagnostics', () => {
