Skip to content

Commit eb625d3

Browse files
crisbetodylhunn
crisbeto
authored and
dylhunn
committed
fix(compiler): declare for loop aliases in addition to new name (#54942)
Currently when aliasing a `for` loop variable with `let`, we replace the variable's old name with the new one. Since users have found this to be confusing, these changes switch to a model where the variable is available both under the original name and the new one. Fixes #52528. PR Close #54942
1 parent 76b921e commit eb625d3

26 files changed

+389
-143
lines changed

‎packages/compiler-cli/src/ngtsc/indexer/src/template.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ class TemplateVisitor extends TmplAstRecursiveVisitor {
271271

272272
override visitForLoopBlock(block: TmplAstForLoopBlock): void {
273273
block.item.visit(this);
274-
this.visitAll(Object.values(block.contextVariables));
274+
this.visitAll(block.contextVariables);
275275
this.visitExpression(block.expression);
276276
this.visitAll(block.children);
277277
block.empty?.visit(this);

‎packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ class TemplateVisitor<Code extends ErrorCode> extends RecursiveAstVisitor implem
195195

196196
visitForLoopBlock(block: TmplAstForLoopBlock): void {
197197
block.item.visit(this);
198-
this.visitAllNodes(Object.values(block.contextVariables));
198+
this.visitAllNodes(block.contextVariables);
199199
this.visitAst(block.expression);
200200
this.visitAllNodes(block.children);
201201
block.empty?.visit(this);

‎packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -353,11 +353,13 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
353353
throw new Error(`Assertion failure: no SourceLocation found for property read.`);
354354
}
355355

356+
const messageVars = [block.item, ...block.contextVariables.filter(v => v.value === '$index')]
357+
.map(v => `'${v.name}'`)
358+
.join(', ');
356359
const message =
357360
`Cannot access '${access.name}' inside of a track expression. ` +
358-
`Only '${block.item.name}', '${
359-
block.contextVariables.$index
360-
.name}' and properties on the containing component are available to this expression.`;
361+
`Only ${
362+
messageVars} and properties on the containing component are available to this expression.`;
361363

362364
this._diagnostics.push(makeTemplateDiagnostic(
363365
templateId, this.resolver.getSourceMapping(templateId), sourceSpan,

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1816,12 +1816,13 @@ class Scope {
18161816
addParseSpanInfo(loopInitializer, scopedNode.item.sourceSpan);
18171817
scope.varMap.set(scopedNode.item, loopInitializer);
18181818

1819-
for (const [name, variable] of Object.entries(scopedNode.contextVariables)) {
1820-
if (!this.forLoopContextVariableTypes.has(name)) {
1821-
throw new Error(`Unrecognized for loop context variable ${name}`);
1819+
for (const variable of scopedNode.contextVariables) {
1820+
if (!this.forLoopContextVariableTypes.has(variable.value)) {
1821+
throw new Error(`Unrecognized for loop context variable ${variable.name}`);
18221822
}
18231823

1824-
const type = ts.factory.createKeywordTypeNode(this.forLoopContextVariableTypes.get(name)!);
1824+
const type =
1825+
ts.factory.createKeywordTypeNode(this.forLoopContextVariableTypes.get(variable.value)!);
18251826
this.registerVariable(
18261827
scope, variable, new TcbBlockImplicitVariableOp(tcb, scope, type, variable));
18271828
}
@@ -2761,18 +2762,26 @@ class TcbEventHandlerTranslator extends TcbExpressionTranslator {
27612762
}
27622763

27632764
class TcbForLoopTrackTranslator extends TcbExpressionTranslator {
2765+
private allowedVariables: Set<TmplAstVariable>;
2766+
27642767
constructor(tcb: Context, scope: Scope, private block: TmplAstForLoopBlock) {
27652768
super(tcb, scope);
2769+
2770+
// Tracking expressions are only allowed to read the `$index`,
2771+
// the item and properties off the component instance.
2772+
this.allowedVariables = new Set([block.item]);
2773+
for (const variable of block.contextVariables) {
2774+
if (variable.value === '$index') {
2775+
this.allowedVariables.add(variable);
2776+
}
2777+
}
27662778
}
27672779

27682780
protected override resolve(ast: AST): ts.Expression|null {
27692781
if (ast instanceof PropertyRead && ast.receiver instanceof ImplicitReceiver) {
27702782
const target = this.tcb.boundTarget.getExpressionTarget(ast);
27712783

2772-
// Tracking expressions are only allowed to read the `$index`,
2773-
// the item and properties off the component instance.
2774-
if (target !== null && target !== this.block.item &&
2775-
target !== this.block.contextVariables.$index) {
2784+
if (target !== null && !this.allowedVariables.has(target)) {
27762785
this.tcb.oobRecorder.illegalForLoopTrackAccess(this.tcb.id, this.block, ast);
27772786
}
27782787
}

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1710,15 +1710,16 @@ describe('type check blocks', () => {
17101710
expect(result).toContain('"" + (_t2) + (_t3) + (_t4) + (_t5) + (_t6) + (_t7)');
17111711
});
17121712

1713-
it('should read an implicit variable from the component scope if it is aliased', () => {
1713+
it('should read both implicit variables and their alias at the same time', () => {
17141714
const TEMPLATE = `
17151715
@for (item of items; track item; let i = $index) { {{$index}} {{i}} }
17161716
`;
17171717

17181718
const result = tcb(TEMPLATE);
17191719
expect(result).toContain('for (const _t1 of ((this).items)!) {');
17201720
expect(result).toContain('var _t2 = null! as number;');
1721-
expect(result).toContain('"" + (((this).$index)) + (_t2)');
1721+
expect(result).toContain('var _t3 = null! as number;');
1722+
expect(result).toContain('"" + (_t2) + (_t3)');
17221723
});
17231724

17241725
it('should read variable from a parent for loop', () => {

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -477,17 +477,18 @@ runInEachFileSystem(() => {
477477
});
478478

479479
it('finds symbols for $index variable', () => {
480-
const iVar = forLoopNode.contextVariables.$index;
480+
const iVar = forLoopNode.contextVariables.find(v => v.name === '$index')!;
481481
const iSymbol = templateTypeChecker.getSymbolOfNode(iVar, cmp)!;
482-
expectIndexSymbol(iSymbol);
482+
expect(iVar).toBeTruthy();
483+
expectIndexSymbol(iSymbol, '$index');
483484
});
484485

485486
it('finds symbol when using the index in the body', () => {
486487
const innerElementNodes =
487488
onlyAstElements((forLoopNode.children[0] as TmplAstElement).children);
488489
const indexSymbol =
489490
templateTypeChecker.getSymbolOfNode(innerElementNodes[0].inputs[0].value, cmp)!;
490-
expectIndexSymbol(indexSymbol);
491+
expectIndexSymbol(indexSymbol, 'i');
491492
});
492493

493494
function expectUserSymbol(userSymbol: Symbol) {
@@ -497,12 +498,15 @@ runInEachFileSystem(() => {
497498
expect((userSymbol).declaration).toEqual(forLoopNode.item);
498499
}
499500

500-
function expectIndexSymbol(indexSymbol: Symbol) {
501+
function expectIndexSymbol(indexSymbol: Symbol, localName: string) {
502+
const indexVar =
503+
forLoopNode.contextVariables.find(v => v.value === '$index' && v.name === localName)!;
501504
assertVariableSymbol(indexSymbol);
505+
expect(indexVar).toBeTruthy();
502506
expect(indexSymbol.tsSymbol)
503507
.toBeNull(); // implicit variable doesn't have a TS definition location
504508
expect(program.getTypeChecker().typeToString(indexSymbol.tsType!)).toEqual('number');
505-
expect((indexSymbol).declaration).toEqual(forLoopNode.contextVariables.$index);
509+
expect((indexSymbol).declaration).toEqual(indexVar);
506510
}
507511
});
508512
});

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,6 +1143,75 @@ export declare class MyApp {
11431143
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
11441144
}
11451145

1146+
/****************************************************************************************************
1147+
* PARTIAL FILE: for_both_aliased_and_original_variables.js
1148+
****************************************************************************************************/
1149+
import { Component } from '@angular/core';
1150+
import * as i0 from "@angular/core";
1151+
export class MyApp {
1152+
constructor() {
1153+
this.message = 'hello';
1154+
this.items = [];
1155+
}
1156+
}
1157+
MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component });
1158+
MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, selector: "ng-component", ngImport: i0, template: `
1159+
<div>
1160+
{{message}}
1161+
@for (item of items; track item; let idx = $index, f = $first; let l = $last, ev = $even, o = $odd; let co = $count) {
1162+
Original index: {{$index}}
1163+
Original first: {{$first}}
1164+
Original last: {{$last}}
1165+
Original even: {{$even}}
1166+
Original odd: {{$odd}}
1167+
Original count: {{$count}}
1168+
<hr>
1169+
Aliased index: {{idx}}
1170+
Aliased first: {{f}}
1171+
Aliased last: {{l}}
1172+
Aliased even: {{ev}}
1173+
Aliased odd: {{o}}
1174+
Aliased count: {{co}}
1175+
}
1176+
</div>
1177+
`, isInline: true });
1178+
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
1179+
type: Component,
1180+
args: [{
1181+
template: `
1182+
<div>
1183+
{{message}}
1184+
@for (item of items; track item; let idx = $index, f = $first; let l = $last, ev = $even, o = $odd; let co = $count) {
1185+
Original index: {{$index}}
1186+
Original first: {{$first}}
1187+
Original last: {{$last}}
1188+
Original even: {{$even}}
1189+
Original odd: {{$odd}}
1190+
Original count: {{$count}}
1191+
<hr>
1192+
Aliased index: {{idx}}
1193+
Aliased first: {{f}}
1194+
Aliased last: {{l}}
1195+
Aliased even: {{ev}}
1196+
Aliased odd: {{o}}
1197+
Aliased count: {{co}}
1198+
}
1199+
</div>
1200+
`,
1201+
}]
1202+
}] });
1203+
1204+
/****************************************************************************************************
1205+
* PARTIAL FILE: for_both_aliased_and_original_variables.d.ts
1206+
****************************************************************************************************/
1207+
import * as i0 from "@angular/core";
1208+
export declare class MyApp {
1209+
message: string;
1210+
items: never[];
1211+
static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
1212+
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
1213+
}
1214+
11461215
/****************************************************************************************************
11471216
* PARTIAL FILE: nested_for_template_variables.js
11481217
****************************************************************************************************/

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,21 @@
301301
}
302302
]
303303
},
304+
{
305+
"description": "should generate a for block with references both to original and aliased template variables",
306+
"inputFiles": ["for_both_aliased_and_original_variables.ts"],
307+
"expectations": [
308+
{
309+
"files": [
310+
{
311+
"expected": "for_both_aliased_and_original_variables_template.js",
312+
"generated": "for_both_aliased_and_original_variables.js"
313+
}
314+
],
315+
"failureMessage": "Incorrect template"
316+
}
317+
]
318+
},
304319
{
305320
"description": "should be able to refer to aliased template variables in nested for blocks",
306321
"inputFiles": ["nested_for_template_variables.ts"],

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_aliased_template_variables_template.pipeline.js‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,9 @@ function MyApp_For_3_Template(rf, ctx) {
33
$r3$.ɵɵtext(0);
44
}
55
if (rf & 2) {
6-
const $idx_r2$ = ctx.$index;
7-
const $co_r3$ = ctx.$count;
8-
const $idx_2_r2$ = ctx.$index;
9-
const $co_2_r3$ = ctx.$count;
10-
$r3$.ɵɵtextInterpolate6(" Index: ", $idx_r2$, " First: ", $idx_2_r2$ === 0, " Last: ", $idx_2_r2$ === $co_2_r3$ - 1, " Even: ", $idx_2_r2$ % 2 === 0, " Odd: ", $idx_2_r2$ % 2 !== 0, " Count: ", $co_r3$, " ");
6+
const $idx_r2$ = ctx.$index;
7+
const $co_r2$ = ctx.$count;
8+
$r3$.ɵɵtextInterpolate6(" Index: ", $idx_r2$, " First: ", $idx_r2$ === 0, " Last: ", $idx_r2$ === $co_r2$ - 1, " Even: ", $idx_r2$ % 2 === 0, " Odd: ", $idx_r2$ % 2 !== 0, " Count: ", $co_r2$, " ");
119
}
1210
}
1311
…

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_both_aliased_and_original_variables.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import {Component} from '@angular/core';
2+
3+
@Component({
4+
template: `
5+
<div>
6+
{{message}}
7+
@for (item of items; track item; let idx = $index, f = $first; let l = $last, ev = $even, o = $odd; let co = $count) {
8+
Original index: {{$index}}
9+
Original first: {{$first}}
10+
Original last: {{$last}}
11+
Original even: {{$even}}
12+
Original odd: {{$odd}}
13+
Original count: {{$count}}
14+
<hr>
15+
Aliased index: {{idx}}
16+
Aliased first: {{f}}
17+
Aliased last: {{l}}
18+
Aliased even: {{ev}}
19+
Aliased odd: {{o}}
20+
Aliased count: {{co}}
21+
}
22+
</div>
23+
`,
24+
})
25+
export class MyApp {
26+
message = 'hello';
27+
items = [];
28+
}

0 commit comments

Comments
 (0)