Visitar URL original
fix(compiler): stop suppressing non-iterable values in `@for` expression · angular/angular@0ee0a16 · GitHub
Skip to content

Commit 0ee0a16

Browse files
atscottthePunderWoman
atscott
authored and
thePunderWoman
committed
fix(compiler): stop suppressing non-iterable values in @for expression
When generating type check blocks for `@for` loops, a non-null assertion is appended to the collection expression. If the collection expression is a compound expression (e.g. binary or logical operations like a && b), the lack of outer parentheses caused the ! to bind only to the rightmost operand (a && b!), leading to typecheck errors such as TS2532. This wraps the expression via .wrapForTypeChecker() before appending !. Template type checking for compound expressions in `@for` loops may surface new type errors Template type checking for `@for` loops now more accurately checks the entire collection expression when compound expressions (such as `(x | async) || []` or logical/ternary operations) are used. Previously, certain invalid expressions in `@for` loops could slip through template type checking unnoticed. As a result, you may see new TypeScript diagnostics (such as `TS2488: Type '{}' must have a '[Symbol.iterator]()' method that returns an iterator`) if the expression does not evaluate to an iterable type. For example, when a component property `x` is typed as `any`, `(x | async)` produces `unknown | null`, which causes `(x | async) || []` to type check as `{}` rather than an iterable array. To fix these errors, ensure that values referenced in the template are strongly typed on the component class (for example, declaring `x: Observable<Item[]>` instead of `any`), or use `$any(...)` in the template (for example, `$any(x | async) || []`).
1 parent 451098c commit 0ee0a16

3 files changed

Lines changed: 224 additions & 10 deletions

File tree

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

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2349,7 +2349,7 @@ describe('type check blocks', () => {
23492349
`;
23502350

23512351
const result = tcb(TEMPLATE);
2352-
expect(result).toContain('for (const _t1 of ((this).items)!) {');
2352+
expect(result).toContain('for (const _t1 of (((this).items))!) {');
23532353
expect(result).toContain('"" + ((this).main(_t1))');
23542354
expect(result).toContain('"" + ((this).empty())');
23552355
});
@@ -2362,7 +2362,7 @@ describe('type check blocks', () => {
23622362
`;
23632363

23642364
const result = tcb(TEMPLATE);
2365-
expect(result).toContain('for (const _t1 of ((this).items)!) {');
2365+
expect(result).toContain('for (const _t1 of (((this).items))!) {');
23662366
expect(result).toContain('var _t2 = null! as number;');
23672367
expect(result).toContain('var _t3 = null! as boolean;');
23682368
expect(result).toContain('var _t4 = null! as boolean;');
@@ -2380,7 +2380,7 @@ describe('type check blocks', () => {
23802380
`;
23812381

23822382
const result = tcb(TEMPLATE);
2383-
expect(result).toContain('for (const _t1 of ((this).items)!) {');
2383+
expect(result).toContain('for (const _t1 of (((this).items))!) {');
23842384
expect(result).toContain('var _t2 = null! as number;');
23852385
expect(result).toContain('var _t3 = null! as boolean;');
23862386
expect(result).toContain('var _t4 = null! as boolean;');
@@ -2396,7 +2396,7 @@ describe('type check blocks', () => {
23962396
`;
23972397

23982398
const result = tcb(TEMPLATE);
2399-
expect(result).toContain('for (const _t1 of ((this).items)!) {');
2399+
expect(result).toContain('for (const _t1 of (((this).items))!) {');
24002400
expect(result).toContain('var _t2 = null! as number;');
24012401
expect(result).toContain('var _t3 = null! as number;');
24022402
expect(result).toContain('"" + (_t2) + (_t3)');
@@ -2414,15 +2414,15 @@ describe('type check blocks', () => {
24142414
`;
24152415

24162416
const result = tcb(TEMPLATE);
2417-
expect(result).toContain('for (const _t1 of ((this).items)!) { var _t2 = null! as number;');
2417+
expect(result).toContain('for (const _t1 of (((this).items))!) { var _t2 = null! as number;');
24182418
expect(result).toContain('"" + (_t1) + (_t2)');
2419-
expect(result).toContain('for (const _t3 of ((_t1).items)!) { var _t4 = null! as number;');
2419+
expect(result).toContain('for (const _t3 of (((_t1).items))!) { var _t4 = null! as number;');
24202420
expect(result).toContain('"" + (_t1) + ((_t2)) + (_t3) + (_t4)');
24212421
});
24222422

24232423
it('should generate the tracking expression of a for loop', () => {
24242424
const result = tcb(`@for (item of items; track trackingFn($index, item, prop)) {}`);
2425-
expect(result).toContain('for (const _t1 of ((this).items)!) { var _t2 = null! as number;');
2425+
expect(result).toContain('for (const _t1 of (((this).items))!) { var _t2 = null! as number;');
24262426
expect(result).toContain('(this).trackingFn(_t2, _t1, ((this).prop));');
24272427
});
24282428

@@ -2436,10 +2436,23 @@ describe('type check blocks', () => {
24362436
`;
24372437

24382438
const result = tcb(TEMPLATE, undefined, {checkControlFlowBodies: false});
2439-
expect(result).toContain('for (const _t1 of ((this).items)!) {');
2439+
expect(result).toContain('for (const _t1 of (((this).items))!) {');
24402440
expect(result).not.toContain('.main');
24412441
expect(result).not.toContain('.empty');
24422442
});
2443+
2444+
it('should wrap compound expressions in parentheses before appending non-null assertion', () => {
2445+
const TEMPLATE = `
2446+
@for (item of items && items.slice(0, 10); track item) {
2447+
{{item}}
2448+
}
2449+
`;
2450+
2451+
const result = tcb(TEMPLATE);
2452+
expect(result).toContain(
2453+
'for (const _t1 of ((((this).items)) && ((((this).items)).slice(0, 10)))!) {',
2454+
);
2455+
});
24432456
});
24442457

24452458
describe('let declarations', () => {

‎packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts‎

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7345,6 +7345,206 @@ suppress
73457345
]);
73467346
});
73477347

7348+
it('should report diagnostics within sub-expressions of compound @for loop expressions', () => {
7349+
env.write(
7350+
'test.ts',
7351+
`
7352+
import {Component} from '@angular/core';
7353+
7354+
@Component({
7355+
template: \`
7356+
@for (item of items && does_not_exist; track item) {
7357+
{{item}}
7358+
}
7359+
\`,
7360+
})
7361+
export class Main {
7362+
items = [1, 2, 3];
7363+
}
7364+
`,
7365+
);
7366+
7367+
const diags = env.driveDiagnostics();
7368+
expect(diags.map((d) => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
7369+
"Property 'does_not_exist' does not exist on type 'Main'.",
7370+
]);
7371+
});
7372+
7373+
it('should report diagnostics on invalid arguments in compound @for loop expressions', () => {
7374+
env.write(
7375+
'test.ts',
7376+
`
7377+
import {Component} from '@angular/core';
7378+
7379+
@Component({
7380+
template: \`
7381+
@for (item of items && items.slice('not_a_number'); track item) {
7382+
{{item}}
7383+
}
7384+
\`,
7385+
})
7386+
export class Main {
7387+
items = [1, 2, 3];
7388+
}
7389+
`,
7390+
);
7391+
7392+
const diags = env.driveDiagnostics();
7393+
expect(diags.map((d) => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
7394+
"Argument of type 'string' is not assignable to parameter of type 'number'.",
7395+
]);
7396+
});
7397+
7398+
it('should report diagnostics on ternary expressions in @for loop expressions', () => {
7399+
env.write(
7400+
'test.ts',
7401+
`
7402+
import {Component} from '@angular/core';
7403+
7404+
@Component({
7405+
template: \`
7406+
@for (item of condition ? not_found : items; track item) {
7407+
{{item}}
7408+
}
7409+
\`,
7410+
})
7411+
export class Main {
7412+
condition = true;
7413+
items = [1, 2, 3];
7414+
}
7415+
`,
7416+
);
7417+
7418+
const diags = env.driveDiagnostics();
7419+
expect(diags.map((d) => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
7420+
"Property 'not_found' does not exist on type 'Main'.",
7421+
]);
7422+
});
7423+
7424+
it('should report diagnostics on nested property reads in @for loop expressions', () => {
7425+
env.write(
7426+
'test.ts',
7427+
`
7428+
import {Component} from '@angular/core';
7429+
7430+
@Component({
7431+
template: \`
7432+
@for (item of nested.does_not_exist; track item) {
7433+
{{item}}
7434+
}
7435+
\`,
7436+
})
7437+
export class Main {
7438+
nested = {a: 1};
7439+
}
7440+
`,
7441+
);
7442+
7443+
const diags = env.driveDiagnostics();
7444+
expect(diags.map((d) => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
7445+
"Property 'does_not_exist' does not exist on type '{ a: number; }'.",
7446+
]);
7447+
});
7448+
7449+
it('should report diagnostics when calling functions with invalid arguments in @for loop expressions', () => {
7450+
env.write(
7451+
'test.ts',
7452+
`
7453+
import {Component} from '@angular/core';
7454+
7455+
@Component({
7456+
template: \`
7457+
@for (item of getItems('invalid'); track item) {
7458+
{{item}}
7459+
}
7460+
\`,
7461+
})
7462+
export class Main {
7463+
getItems(count: number): string[] {
7464+
return [];
7465+
}
7466+
}
7467+
`,
7468+
);
7469+
7470+
const diags = env.driveDiagnostics();
7471+
expect(diags.map((d) => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
7472+
"Argument of type 'string' is not assignable to parameter of type 'number'.",
7473+
]);
7474+
});
7475+
7476+
it('should report diagnostics when iterating over an un-narrowed async pipe result with empty array fallback', () => {
7477+
env.write(
7478+
'test.ts',
7479+
`
7480+
import {Component, Pipe} from '@angular/core';
7481+
7482+
interface Subscribable<T> {
7483+
subscribe(observer: any): any;
7484+
}
7485+
7486+
@Pipe({name: 'async'})
7487+
export class AsyncPipe {
7488+
transform<T>(value: Subscribable<T> | Promise<T> | null | undefined): T | null {
7489+
return null;
7490+
}
7491+
}
7492+
7493+
@Component({
7494+
template: \`
7495+
@for (a of (x | async) || []; track a) {
7496+
{{a}}
7497+
}
7498+
\`,
7499+
imports: [AsyncPipe],
7500+
})
7501+
export class Main {
7502+
x: any;
7503+
}
7504+
`,
7505+
);
7506+
7507+
const diags = env.driveDiagnostics();
7508+
expect(diags.map((d) => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
7509+
`Type '{}' must have a '[Symbol.iterator]()' method that returns an iterator.`,
7510+
]);
7511+
});
7512+
7513+
it('should not report diagnostics when iterating over a typed async pipe result with empty array fallback', () => {
7514+
env.write(
7515+
'test.ts',
7516+
`
7517+
import {Component, Pipe} from '@angular/core';
7518+
7519+
interface Subscribable<T> {
7520+
subscribe(observer: any): any;
7521+
}
7522+
7523+
@Pipe({name: 'async'})
7524+
export class AsyncPipe {
7525+
transform<T>(value: Subscribable<T> | Promise<T> | null | undefined): T | null {
7526+
return null;
7527+
}
7528+
}
7529+
7530+
@Component({
7531+
template: \`
7532+
@for (a of (x | async) || []; track a) {
7533+
{{a}}
7534+
}
7535+
\`,
7536+
imports: [AsyncPipe],
7537+
})
7538+
export class Main {
7539+
x!: Subscribable<number[]>;
7540+
}
7541+
`,
7542+
);
7543+
7544+
const diags = env.driveDiagnostics();
7545+
expect(diags.map((d) => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([]);
7546+
});
7547+
73487548
it('should check for loop variables with the same name as built-in globals', () => {
73497549
// strictTemplates are necessary so the event listener is checked.
73507550
env.tsconfig({strictTemplates: true});

‎packages/compiler/src/typecheck/ops/for_block.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,9 @@ export class TcbForOfOp extends TcbOp {
4545

4646
// It's common to have a for loop over a nullable value (e.g. produced by the `async` pipe).
4747
// Add a non-null expression to allow such values to be assigned.
48-
const expression = new TcbExpr(
49-
`${tcbExpression(this.block.expression, this.tcb, this.scope).print()}!`,
48+
const expr = tcbExpression(this.block.expression, this.tcb, this.scope).wrapForTypeChecker();
49+
const expression = new TcbExpr(`${expr.print()}!`).addParseSpanInfo(
50+
this.block.expression.sourceSpan,
5051
);
5152

5253
let statements: TcbExpr[];

0 commit comments

Comments
 (0)