From 06f6dec7aae107065d2501a7145ae4e905de06e6 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 13 May 2026 09:59:57 +0200 Subject: [PATCH] fix(compiler): type check invalid for loops Currently if a `@for` loop doesn't have a `track` expression we don't produce an AST for it at all which means no type checking and language service support for it. These changes make it so we produce the AST anyways since it gives the user more tools to resolve the issue (e.g. autocompletion when writing the `track` expression). --- ...ecker__get_symbol_of_template_node_spec.ts | 2 +- .../test/ngtsc/template_typecheck_spec.ts | 26 +++++++++ packages/compiler/src/render3/r3_ast.ts | 4 +- .../compiler/src/render3/r3_control_flow.ts | 53 ++++++++++--------- .../compiler/src/render3/view/t2_binder.ts | 2 +- .../src/template/pipeline/src/ingest.ts | 15 +++++- .../compiler/src/typecheck/ops/for_block.ts | 18 +++++-- .../render3/r3_template_transform_spec.ts | 5 +- .../safe-optional-chaining/migration.ts | 2 +- .../template_reference_visitor.ts | 6 ++- .../src/quick_info_built_ins.ts | 1 + .../language-service/src/template_target.ts | 2 +- 12 files changed, 98 insertions(+), 38 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts index 0815589d850..5e942e9bdb0 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts @@ -573,7 +573,7 @@ runInEachFileSystem(() => { }); it('should retrieve a symbol for the track expression', () => { - const userSymbol = templateTypeChecker.getSymbolOfNode(forLoopNode.trackBy.ast, cmp)!; + const userSymbol = templateTypeChecker.getSymbolOfNode(forLoopNode.trackBy!.ast, cmp)!; expectUserSymbol(userSymbol); }); diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index fe4860f9c99..44addc76d16 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -6573,6 +6573,32 @@ suppress `Type 'number' is not assignable to type 'string'.`, ]); }); + + it('should type check a @for loop without a `track` expression', () => { + env.write( + 'test.ts', + ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + @for (item of items) { + {{does_not_exist}} + } + \`, + }) + export class Main { + items = []; + } + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.map((d) => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([ + `Property 'does_not_exist' does not exist on type 'Main'.`, + `@for loop must have a "track" expression`, + ]); + }); }); describe('control flow content projection diagnostics', () => { diff --git a/packages/compiler/src/render3/r3_ast.ts b/packages/compiler/src/render3/r3_ast.ts index bd6cb2988d9..8c7fa4a7909 100644 --- a/packages/compiler/src/render3/r3_ast.ts +++ b/packages/compiler/src/render3/r3_ast.ts @@ -489,8 +489,8 @@ export class ForLoopBlock extends BlockNode implements Node { constructor( public item: Variable, public expression: ASTWithSource, - public trackBy: ASTWithSource, - public trackKeywordSpan: ParseSourceSpan, + public trackBy: ASTWithSource | null, + public trackKeywordSpan: ParseSourceSpan | null, public contextVariables: Variable[], public children: Node[], public empty: ForLoopBlockEmpty | null, diff --git a/packages/compiler/src/render3/r3_control_flow.ts b/packages/compiler/src/render3/r3_control_flow.ts index 0f37877ffb0..98ea387c7e3 100644 --- a/packages/compiler/src/render3/r3_control_flow.ts +++ b/packages/compiler/src/render3/r3_control_flow.ts @@ -183,35 +183,40 @@ export function createForLoop( } if (params !== null) { + // The `for` block has a main span that includes the `empty` branch. For only the span of the + // main `for` body, use `mainSourceSpan`. + const endSpan = empty?.endSourceSpan ?? ast.endSourceSpan; + const sourceSpan = new ParseSourceSpan( + ast.sourceSpan.start, + endSpan?.end ?? ast.sourceSpan.end, + ); + let trackExpression: ASTWithSource | null; + let trackKeywordSpan: ParseSourceSpan | null; + if (params.trackBy === null) { - // TODO: We should not fail here, and instead try to produce some AST for the language - // service. + trackExpression = trackKeywordSpan = null; errors.push(new ParseError(ast.startSourceSpan, '@for loop must have a "track" expression')); } else { - // The `for` block has a main span that includes the `empty` branch. For only the span of the - // main `for` body, use `mainSourceSpan`. - const endSpan = empty?.endSourceSpan ?? ast.endSourceSpan; - const sourceSpan = new ParseSourceSpan( - ast.sourceSpan.start, - endSpan?.end ?? ast.sourceSpan.end, - ); + trackExpression = params.trackBy.expression; + trackKeywordSpan = params.trackBy.keywordSpan; validateTrackByExpression(params.trackBy.expression, params.trackBy.keywordSpan, errors); - node = new t.ForLoopBlock( - params.itemName, - params.expression, - params.trackBy.expression, - params.trackBy.keywordSpan, - params.context, - html.visitAll(visitor, ast.children, ast.children), - empty, - sourceSpan, - ast.sourceSpan, - ast.startSourceSpan, - endSpan, - ast.nameSpan, - ast.i18n, - ); } + + node = new t.ForLoopBlock( + params.itemName, + params.expression, + trackExpression, + trackKeywordSpan, + params.context, + html.visitAll(visitor, ast.children, ast.children), + empty, + sourceSpan, + ast.sourceSpan, + ast.startSourceSpan, + endSpan, + ast.nameSpan, + ast.i18n, + ); } return {node, errors}; diff --git a/packages/compiler/src/render3/view/t2_binder.ts b/packages/compiler/src/render3/view/t2_binder.ts index b6eb3a18bb3..38d23acefdb 100644 --- a/packages/compiler/src/render3/view/t2_binder.ts +++ b/packages/compiler/src/render3/view/t2_binder.ts @@ -994,7 +994,7 @@ class TemplateBinder extends CombinedRecursiveAstVisitor { } else if (nodeOrNodes instanceof ForLoopBlock) { this.visitNode(nodeOrNodes.item); nodeOrNodes.contextVariables.forEach((v) => this.visitNode(v)); - nodeOrNodes.trackBy.visit(this); + nodeOrNodes.trackBy?.visit(this); nodeOrNodes.children.forEach(this.visitNode); this.nestingLevel.set(nodeOrNodes, this.level); } else if (nodeOrNodes instanceof DeferredBlock) { diff --git a/packages/compiler/src/template/pipeline/src/ingest.ts b/packages/compiler/src/template/pipeline/src/ingest.ts index 74ab2a5e179..f4cb0d9f1ed 100644 --- a/packages/compiler/src/template/pipeline/src/ingest.ts +++ b/packages/compiler/src/template/pipeline/src/ingest.ts @@ -935,8 +935,19 @@ function ingestForBlock(unit: ViewCompilationUnit, forBlock: t.ForLoopBlock): vo } } - const sourceSpan = convertSourceSpan(forBlock.trackBy.span, forBlock.sourceSpan); - const track = convertAst(forBlock.trackBy, unit.job, sourceSpan); + let track: o.Expression; + + if (forBlock.trackBy === null) { + // `@for` without a `track` is invalid and it produces a parser error. + // Put a placeholder here so we don't need to account for it throughout the pipeline. + track = o.variable('$index'); + } else { + track = convertAst( + forBlock.trackBy, + unit.job, + convertSourceSpan(forBlock.trackBy.span, forBlock.sourceSpan), + ); + } ingestNodes(repeaterView, forBlock.children); diff --git a/packages/compiler/src/typecheck/ops/for_block.ts b/packages/compiler/src/typecheck/ops/for_block.ts index 34b9ae52772..b5850f103c9 100644 --- a/packages/compiler/src/typecheck/ops/for_block.ts +++ b/packages/compiler/src/typecheck/ops/for_block.ts @@ -48,11 +48,21 @@ export class TcbForOfOp extends TcbOp { const expression = new TcbExpr( `${tcbExpression(this.block.expression, this.tcb, this.scope).print()}!`, ); - const trackTranslator = new TcbForLoopTrackTranslator(this.tcb, loopScope, this.block); - const trackExpression = trackTranslator.translate(this.block.trackBy); - const block = getStatementsBlock([...loopScope.render(), trackExpression]); + + let statements: TcbExpr[]; + + if (this.block.trackBy === null) { + statements = loopScope.render(); + } else { + const trackTranslator = new TcbForLoopTrackTranslator(this.tcb, loopScope, this.block); + const trackExpression = trackTranslator.translate(this.block.trackBy); + statements = [...loopScope.render(), trackExpression]; + } + this.scope.addStatement( - new TcbExpr(`for (${initializer.print()} of ${expression.print()}) {\n${block} }`), + new TcbExpr( + `for (${initializer.print()} of ${expression.print()}) {\n${getStatementsBlock(statements)} }`, + ), ); return null; } diff --git a/packages/compiler/test/render3/r3_template_transform_spec.ts b/packages/compiler/test/render3/r3_template_transform_spec.ts index 4f969463059..b30b5ee1023 100644 --- a/packages/compiler/test/render3/r3_template_transform_spec.ts +++ b/packages/compiler/test/render3/r3_template_transform_spec.ts @@ -119,7 +119,10 @@ class R3AstHumanizer implements t.Visitor { } visitForLoopBlock(block: t.ForLoopBlock): void { - const result: any[] = ['ForLoopBlock', unparse(block.expression), unparse(block.trackBy)]; + const result: any[] = ['ForLoopBlock', unparse(block.expression)]; + if (block.trackBy !== null) { + result.push(unparse(block.trackBy)); + } this.result.push(result); this.visitAll([[block.item], block.contextVariables, block.children]); block.empty?.visit(this); diff --git a/packages/core/schematics/migrations/safe-optional-chaining/migration.ts b/packages/core/schematics/migrations/safe-optional-chaining/migration.ts index 1a72bf5975e..b116a5e1132 100644 --- a/packages/core/schematics/migrations/safe-optional-chaining/migration.ts +++ b/packages/core/schematics/migrations/safe-optional-chaining/migration.ts @@ -703,7 +703,7 @@ class TmplVisitor extends TmplAstRecursiveVisitor { override visitForLoopBlock(block: TmplAstForLoopBlock) { block.expression.visit(this.exprMigrator, false); - block.trackBy.visit(this.exprMigrator, false); + block.trackBy?.visit(this.exprMigrator, false); super.visitForLoopBlock(block); } diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts b/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts index c22b6d2c0f2..e33e71658ba 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts @@ -175,7 +175,11 @@ export class TemplateReferenceVisitor< override visitForLoopBlock(block: TmplAstForLoopBlock): void { this.checkExpressionForReferencedFields(block, block.expression); - this.checkExpressionForReferencedFields(block, block.trackBy); + + if (block.trackBy !== null) { + this.checkExpressionForReferencedFields(block, block.trackBy); + } + super.visitForLoopBlock(block); } diff --git a/packages/language-service/src/quick_info_built_ins.ts b/packages/language-service/src/quick_info_built_ins.ts index 4da50627212..9996b6b1e66 100644 --- a/packages/language-service/src/quick_info_built_ins.ts +++ b/packages/language-service/src/quick_info_built_ins.ts @@ -127,6 +127,7 @@ export function createQuickInfoForBuiltIn( partSpan = node.nameSpan; } else if ( node instanceof TmplAstForLoopBlock && + node.trackKeywordSpan !== null && isWithin(cursorPositionInTemplate, node.trackKeywordSpan) ) { partSpan = node.trackKeywordSpan; diff --git a/packages/language-service/src/template_target.ts b/packages/language-service/src/template_target.ts index f4fffd8eb6f..17282274f64 100644 --- a/packages/language-service/src/template_target.ts +++ b/packages/language-service/src/template_target.ts @@ -679,7 +679,7 @@ class TemplateTargetVisitor implements TmplAstVisitor { this.visit(block.item); this.visitAll(block.contextVariables); this.visitBinding(block.expression); - this.visitBinding(block.trackBy); + block.trackBy && this.visitBinding(block.trackBy); this.visitAll(block.children); block.empty && this.visit(block.empty); }