From a46292af260f241da2f9f4e4cbb7d40b14a678f1 Mon Sep 17 00:00:00 2001 From: leonsenft Date: Tue, 25 Aug 2026 22:02:28 +0000 Subject: [PATCH] fix(compiler-cli): default template diagnostic related message source file to template For external templates (using `templateUrl`), primary diagnostics are reported against the synthetic `ts.SourceFile` representing the HTML template document. However, secondary related messages (such as those in `foreign_component.ts` and `oob.ts`) were explicitly passing the component's TypeScript file as `sourceFile`. Because the character offsets (`start` and `end`) originate from the HTML template AST, associating them with the TypeScript source file caused IDEs and CLI diagnostics to map HTML offsets onto the `.ts` file, resulting in corrupt or out-of-bounds source locations. This commit resolves the issue by: 1. Making `sourceFile` optional in `makeTemplateDiagnostic` and related checker interfaces (`TemplateTypeChecker`, `TemplateContext`). 2. Defaulting `relatedMessage.sourceFile` to the template's source file (`sf` for external/indirect templates, or the component `.ts` file for direct inline templates) when not explicitly provided. 3. Removing explicit `sourceFile: this.sourceMapping.node.getSourceFile()` mappings from `foreign_component.ts` and DOM element checks in `oob.ts`, allowing them to automatically resolve to the template file. 4. Adding unit test coverage for external templates encountering foreign component conflicts with related messages. --- .../component/src/foreign_component.ts | 3 - .../src/ngtsc/typecheck/api/checker.ts | 6 +- .../typecheck/diagnostics/src/diagnostic.ts | 59 +++++++--- .../src/ngtsc/typecheck/extended/api/api.ts | 12 +- .../extended/src/extended_template_checker.ts | 2 +- .../src/ngtsc/typecheck/src/checker.ts | 2 +- .../src/ngtsc/typecheck/src/oob.ts | 9 +- .../test/ngtsc/template_typecheck_spec.ts | 106 ++++++++++++++++++ 8 files changed, 170 insertions(+), 29 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/foreign_component.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/foreign_component.ts index 6e6bedc560e..c099651bf6a 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/foreign_component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/foreign_component.ts @@ -166,7 +166,6 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor { text: 'Child nodes are defined here.', start: firstChild.sourceSpan.start.offset, end: firstChild.sourceSpan.end.offset, - sourceFile: this.sourceMapping.node.getSourceFile(), }, ], ), @@ -307,7 +306,6 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor { text: `The @content block '${block.name}' was first defined here.`, start: firstDecl.sourceSpan.start.offset, end: firstDecl.sourceSpan.end.offset, - sourceFile: this.sourceMapping.node.getSourceFile(), }, ], ), @@ -340,7 +338,6 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor { text: `The property '${block.name}' is defined here.`, start: conflict.sourceSpan.start.offset, end: conflict.sourceSpan.end.offset, - sourceFile: this.sourceMapping.node.getSourceFile(), }, ], ), diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts index 6051c688c34..dd900eb2d1d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts @@ -366,6 +366,10 @@ export interface TemplateTypeChecker { /** * Constructs a `ts.Diagnostic` for a given `ParseSourceSpan` within a template. + * + * @param relatedInformation Optional list of secondary related messages: + * - Omit `sourceFile` when `start` and `end` offsets are locations within the template itself. + * - Specify `sourceFile` only when referencing a separate file (e.g. directive class declaration). */ makeTemplateDiagnostic( clazz: ts.ClassDeclaration, @@ -377,7 +381,7 @@ export interface TemplateTypeChecker { text: string; start: number; end: number; - sourceFile: ts.SourceFile; + sourceFile?: ts.SourceFile; }[], ): NgTemplateDiagnostic; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts b/packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts index 6882987df25..5af6e81aec9 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts @@ -25,6 +25,19 @@ interface DeprecatedDiagnosticInfo { /** * Constructs a `ts.Diagnostic` for a given `ParseSourceSpan` within a template. * + * @param id The unique type-check ID for the component. + * @param mapping The source mapping for the template (direct, indirect, or external). + * @param span The source span within the template where the diagnostic occurred. + * @param category The diagnostic category (Error, Warning, Suggestion, Message). + * @param code The numeric Angular error code. + * @param messageText The primary diagnostic message. + * @param relatedMessages Optional list of secondary related messages: + * - Omit `sourceFile` (leave `undefined`) when `start` and `end` offsets correspond to + * positions within the template itself. The diagnostic will automatically associate them + * with the template source file (such as the parsed external HTML file or inline template node). + * - Specify `sourceFile` only when the message points to an external file (e.g., a component, + * directive, or pipe TypeScript declaration file) where `start` and `end` are offsets within + * that specific source file. * @param deprecatedDiagInfo Optional information about deprecation and related messages. */ export function makeTemplateDiagnostic( @@ -38,7 +51,7 @@ export function makeTemplateDiagnostic( text: string; start: number; end: number; - sourceFile: ts.SourceFile; + sourceFile?: ts.SourceFile; }[], deprecatedDiagInfo?: DeprecatedDiagnosticInfo, ): TemplateDiagnostic { @@ -49,7 +62,7 @@ export function makeTemplateDiagnostic( relatedInformation.push({ category: ts.DiagnosticCategory.Message, code: 0, - file: relatedMessage.sourceFile, + file: relatedMessage.sourceFile ?? mapping.node.getSourceFile(), start: relatedMessage.start, length: relatedMessage.end - relatedMessage.start, messageText: relatedMessage.text, @@ -89,24 +102,24 @@ export function makeTemplateDiagnostic( ? `${componentSf.fileName} (${componentName} template)` : mapping.templateUrl; - let relatedInformation: ts.DiagnosticRelatedInformation[] = []; - if (relatedMessages !== undefined) { - for (const relatedMessage of relatedMessages) { - relatedInformation.push({ - category: ts.DiagnosticCategory.Message, - code: 0, - file: relatedMessage.sourceFile, - start: relatedMessage.start, - length: relatedMessage.end - relatedMessage.start, - messageText: relatedMessage.text, - }); - } - } - let sf: ts.SourceFile; try { sf = getParsedTemplateSourceFile(fileName, mapping); } catch (e) { + let relatedInformation: ts.DiagnosticRelatedInformation[] = []; + if (relatedMessages !== undefined) { + for (const relatedMessage of relatedMessages) { + relatedInformation.push({ + category: ts.DiagnosticCategory.Message, + code: 0, + file: relatedMessage.sourceFile ?? componentSf, + start: relatedMessage.start, + length: relatedMessage.end - relatedMessage.start, + messageText: relatedMessage.text, + }); + } + } + const failureChain = makeDiagnosticChain( `Failed to report an error in '${fileName}' at ${span.start.line + 1}:${ span.start.col + 1 @@ -130,6 +143,20 @@ export function makeTemplateDiagnostic( }; } + let relatedInformation: ts.DiagnosticRelatedInformation[] = []; + if (relatedMessages !== undefined) { + for (const relatedMessage of relatedMessages) { + relatedInformation.push({ + category: ts.DiagnosticCategory.Message, + code: 0, + file: relatedMessage.sourceFile ?? sf, + start: relatedMessage.start, + length: relatedMessage.end - relatedMessage.start, + messageText: relatedMessage.text, + }); + } + } + let typeForMessage: string; if (category === ts.DiagnosticCategory.Warning) { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts index 686f2a7653e..0fbad79f783 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts @@ -51,6 +51,10 @@ export interface TemplateContext { /** * Creates a template diagnostic with the given information for the template being processed and * using the diagnostic category configured for the extended template diagnostic. + * + * @param relatedInformation Optional list of secondary related messages: + * - Omit `sourceFile` when `start` and `end` offsets are locations within the template itself. + * - Specify `sourceFile` only when referencing a separate file (e.g. directive class declaration). */ makeTemplateDiagnostic( sourceSpan: ParseSourceSpan, @@ -59,7 +63,7 @@ export interface TemplateContext { text: string; start: number; end: number; - sourceFile: ts.SourceFile; + sourceFile?: ts.SourceFile; }[], ): NgTemplateDiagnostic; } @@ -80,9 +84,9 @@ export interface TemplateCheckFactory< /** * This abstract class provides a base implementation for the run method. */ -export abstract class TemplateCheckWithVisitor - implements TemplateCheck -{ +export abstract class TemplateCheckWithVisitor< + Code extends ErrorCode, +> implements TemplateCheck { abstract code: Code; /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts index 1892a506be9..30ada0aaa8f 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts @@ -85,7 +85,7 @@ export class ExtendedTemplateCheckerImpl implements ExtendedTemplateChecker { text: string; start: number; end: number; - sourceFile: ts.SourceFile; + sourceFile?: ts.SourceFile; }[], ): NgTemplateDiagnostic => { return this.partialCtx.templateTypeChecker.makeTemplateDiagnostic( diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts index 4393912ac33..10d710da29c 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts @@ -908,7 +908,7 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { text: string; start: number; end: number; - sourceFile: ts.SourceFile; + sourceFile?: ts.SourceFile; }[], ): NgTemplateDiagnostic { const sfPath = absoluteFromSourceFile(clazz.getSourceFile()); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts index 418726b458f..09acae1a006 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts @@ -312,8 +312,12 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor const errorMsg = `The property and event halves of the two-way binding '${input.name}' are not bound to the same target. Find more at ${DOC_PAGE_BASE_URL}/guide/templates/two-way-binding`; - const relatedMessages: {text: string; start: number; end: number; sourceFile: ts.SourceFile}[] = - []; + const relatedMessages: { + text: string; + start: number; + end: number; + sourceFile?: ts.SourceFile; + }[] = []; if (inputConsumer.ref.nodeNameSpan && inputConsumer.ref.nodeFilePath) { const sf = this.getSourceFile(inputConsumer.ref.nodeFilePath); @@ -336,7 +340,6 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor text: message, start: outputConsumer.sourceSpan.start.offset + 1, end: outputConsumer.sourceSpan.start.offset + outputConsumer.name.length + 1, - sourceFile: mapping.node.getSourceFile(), }); } else { if (outputConsumer.ref.nodeNameSpan && outputConsumer.ref.nodeFilePath) { diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 03603abcac6..f693bb3d853 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -2659,6 +2659,112 @@ runInEachFileSystem(() => { 'Child nodes are defined here.', ); }); + + it('should detect duplicate @content blocks in an external template', () => { + env.write( + 'test.ts', + ` + ${foreignSetupCode} + + @Component({ + selector: 'test', + templateUrl: './test.html', + foreignImports: [frameworkImport(FancyButton)], + }) + export class TestCmp {} + `, + ); + env.write( + 'test.html', + ' @content (icon) {} @content (icon) {} ', + ); + const diags = env.driveDiagnostics(); + expect(diags.length).toEqual(1); + expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.CONFLICTING_CONTENT_DECLARATION)); + expect(diags[0].file?.fileName).toMatch(/test\.html$/); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('@content (icon) {}'); + expect(diags[0].relatedInformation).toBeDefined(); + expect(diags[0].relatedInformation!.length).toEqual(2); + expect(diags[0].relatedInformation![0].file?.fileName).toMatch(/test\.html$/); + expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toEqual( + '@content (icon) {}', + ); + expect(diags[0].relatedInformation![0].messageText).toEqual( + "The @content block 'icon' was first defined here.", + ); + }); + + it('should detect a conflict between a @content block and property binding in an external template', () => { + env.write( + 'test.ts', + ` + ${foreignSetupCode} + + @Component({ + selector: 'test', + templateUrl: './test.html', + foreignImports: [frameworkImport(FancyButton)], + }) + export class TestCmp { + myIcon = document.createTextNode('circle'); + } + `, + ); + env.write( + 'test.html', + ' @content (icon) {square} ', + ); + const diags = env.driveDiagnostics(); + expect(diags.length).toEqual(1); + expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.CONFLICTING_CONTENT_AND_PROPERTY)); + expect(diags[0].file?.fileName).toMatch(/test\.html$/); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('@content (icon) {square}'); + expect(diags[0].relatedInformation).toBeDefined(); + expect(diags[0].relatedInformation!.length).toEqual(2); + expect(diags[0].relatedInformation![0].file?.fileName).toMatch(/test\.html$/); + expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toEqual( + '[icon]="myIcon"', + ); + expect(diags[0].relatedInformation![0].messageText).toEqual( + "The property 'icon' is defined here.", + ); + }); + + it('should detect a conflict between implicit children and [children] binding in an external template', () => { + env.write( + 'test.ts', + ` + ${foreignSetupCode} + + @Component({ + selector: 'test', + templateUrl: './test.html', + foreignImports: [frameworkImport(FancyButton)], + }) + export class TestCmp { + myChildren = []; + } + `, + ); + env.write( + 'test.html', + '
child
', + ); + const diags = env.driveDiagnostics(); + expect(diags.length).toEqual(1); + expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.CONFLICTING_CONTENT_AND_PROPERTY)); + expect(diags[0].file?.fileName).toMatch(/test\.html$/); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[children]="myChildren"'); + expect(diags[0].relatedInformation).toBeDefined(); + expect(diags[0].relatedInformation!.length).toEqual(2); + expect(diags[0].relatedInformation![0].file?.fileName).toMatch(/test\.html$/); + expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toEqual( + '
child
', + ); + expect(diags[0].relatedInformation![0].messageText).toEqual( + 'Child nodes are defined here.', + ); + }); }); it('should detect a duplicate variable declaration', () => {