Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
crisbeto wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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)) {
Expand Down
20 changes: 8 additions & 12 deletions packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())');
});
Expand All @@ -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;');
Expand All @@ -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;');
Expand All @@ -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)');
});
Expand All @@ -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));');
});
});
});
34 changes: 34 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down