From 1b624643cf9c24d28238a095fd7ed0b0501d0111 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sat, 2 Dec 2023 09:25:22 +0100 Subject: [PATCH] fix(compiler-cli): avoid conflicts with built-in global variables in for loop blocks 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. --- .../ngtsc/typecheck/src/type_check_block.ts | 24 ++++++++----- .../typecheck/test/type_check_block_spec.ts | 20 +++++------ .../test/ngtsc/template_typecheck_spec.ts | 34 +++++++++++++++++++ 3 files changed, 58 insertions(+), 20 deletions(-) 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 index 531f5a6c3b73..5cab0f599f4d 100644 --- 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(); + private varMap = new Map(); /** * 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 index 04acceb7582c..51a1eedf427c 100644 --- 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 index aa4383013882..fa336c5cd50b 100644 --- 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) { + + } + \`, + }) + 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', () => {