Skip to content

Commit a46292a

Browse files
leonsenftleonsenft
committed
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.
1 parent 83f7695 commit a46292a

8 files changed

Lines changed: 170 additions & 29 deletions

File tree

‎packages/compiler-cli/src/ngtsc/annotations/component/src/foreign_component.ts‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,6 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
166166
text: 'Child nodes are defined here.',
167167
start: firstChild.sourceSpan.start.offset,
168168
end: firstChild.sourceSpan.end.offset,
169-
sourceFile: this.sourceMapping.node.getSourceFile(),
170169
},
171170
],
172171
),
@@ -307,7 +306,6 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
307306
text: `The @content block '${block.name}' was first defined here.`,
308307
start: firstDecl.sourceSpan.start.offset,
309308
end: firstDecl.sourceSpan.end.offset,
310-
sourceFile: this.sourceMapping.node.getSourceFile(),
311309
},
312310
],
313311
),
@@ -340,7 +338,6 @@ class ForeignComponentFeatureAnalyzer extends TmplAstRecursiveVisitor {
340338
text: `The property '${block.name}' is defined here.`,
341339
start: conflict.sourceSpan.start.offset,
342340
end: conflict.sourceSpan.end.offset,
343-
sourceFile: this.sourceMapping.node.getSourceFile(),
344341
},
345342
],
346343
),

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,10 @@ export interface TemplateTypeChecker {
366366

367367
/**
368368
* Constructs a `ts.Diagnostic` for a given `ParseSourceSpan` within a template.
369+
*
370+
* @param relatedInformation Optional list of secondary related messages:
371+
* - Omit `sourceFile` when `start` and `end` offsets are locations within the template itself.
372+
* - Specify `sourceFile` only when referencing a separate file (e.g. directive class declaration).
369373
*/
370374
makeTemplateDiagnostic<T extends ErrorCode>(
371375
clazz: ts.ClassDeclaration,
@@ -377,7 +381,7 @@ export interface TemplateTypeChecker {
377381
text: string;
378382
start: number;
379383
end: number;
380-
sourceFile: ts.SourceFile;
384+
sourceFile?: ts.SourceFile;
381385
}[],
382386
): NgTemplateDiagnostic<T>;
383387
}

‎packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts‎

Lines changed: 43 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,19 @@ interface DeprecatedDiagnosticInfo {
2525
/**
2626
* Constructs a `ts.Diagnostic` for a given `ParseSourceSpan` within a template.
2727
*
28+
* @param id The unique type-check ID for the component.
29+
* @param mapping The source mapping for the template (direct, indirect, or external).
30+
* @param span The source span within the template where the diagnostic occurred.
31+
* @param category The diagnostic category (Error, Warning, Suggestion, Message).
32+
* @param code The numeric Angular error code.
33+
* @param messageText The primary diagnostic message.
34+
* @param relatedMessages Optional list of secondary related messages:
35+
* - Omit `sourceFile` (leave `undefined`) when `start` and `end` offsets correspond to
36+
* positions within the template itself. The diagnostic will automatically associate them
37+
* with the template source file (such as the parsed external HTML file or inline template node).
38+
* - Specify `sourceFile` only when the message points to an external file (e.g., a component,
39+
* directive, or pipe TypeScript declaration file) where `start` and `end` are offsets within
40+
* that specific source file.
2841
* @param deprecatedDiagInfo Optional information about deprecation and related messages.
2942
*/
3043
export function makeTemplateDiagnostic(
@@ -38,7 +51,7 @@ export function makeTemplateDiagnostic(
3851
text: string;
3952
start: number;
4053
end: number;
41-
sourceFile: ts.SourceFile;
54+
sourceFile?: ts.SourceFile;
4255
}[],
4356
deprecatedDiagInfo?: DeprecatedDiagnosticInfo,
4457
): TemplateDiagnostic {
@@ -49,7 +62,7 @@ export function makeTemplateDiagnostic(
4962
relatedInformation.push({
5063
category: ts.DiagnosticCategory.Message,
5164
code: 0,
52-
file: relatedMessage.sourceFile,
65+
file: relatedMessage.sourceFile ?? mapping.node.getSourceFile(),
5366
start: relatedMessage.start,
5467
length: relatedMessage.end - relatedMessage.start,
5568
messageText: relatedMessage.text,
@@ -89,24 +102,24 @@ export function makeTemplateDiagnostic(
89102
? `${componentSf.fileName} (${componentName} template)`
90103
: mapping.templateUrl;
91104

92-
let relatedInformation: ts.DiagnosticRelatedInformation[] = [];
93-
if (relatedMessages !== undefined) {
94-
for (const relatedMessage of relatedMessages) {
95-
relatedInformation.push({
96-
category: ts.DiagnosticCategory.Message,
97-
code: 0,
98-
file: relatedMessage.sourceFile,
99-
start: relatedMessage.start,
100-
length: relatedMessage.end - relatedMessage.start,
101-
messageText: relatedMessage.text,
102-
});
103-
}
104-
}
105-
106105
let sf: ts.SourceFile;
107106
try {
108107
sf = getParsedTemplateSourceFile(fileName, mapping);
109108
} catch (e) {
109+
let relatedInformation: ts.DiagnosticRelatedInformation[] = [];
110+
if (relatedMessages !== undefined) {
111+
for (const relatedMessage of relatedMessages) {
112+
relatedInformation.push({
113+
category: ts.DiagnosticCategory.Message,
114+
code: 0,
115+
file: relatedMessage.sourceFile ?? componentSf,
116+
start: relatedMessage.start,
117+
length: relatedMessage.end - relatedMessage.start,
118+
messageText: relatedMessage.text,
119+
});
120+
}
121+
}
122+
110123
const failureChain = makeDiagnosticChain(
111124
`Failed to report an error in '${fileName}' at ${span.start.line + 1}:${
112125
span.start.col + 1
@@ -130,6 +143,20 @@ export function makeTemplateDiagnostic(
130143
};
131144
}
132145

146+
let relatedInformation: ts.DiagnosticRelatedInformation[] = [];
147+
if (relatedMessages !== undefined) {
148+
for (const relatedMessage of relatedMessages) {
149+
relatedInformation.push({
150+
category: ts.DiagnosticCategory.Message,
151+
code: 0,
152+
file: relatedMessage.sourceFile ?? sf,
153+
start: relatedMessage.start,
154+
length: relatedMessage.end - relatedMessage.start,
155+
messageText: relatedMessage.text,
156+
});
157+
}
158+
}
159+
133160
let typeForMessage: string;
134161

135162
if (category === ts.DiagnosticCategory.Warning) {

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ export interface TemplateContext<Code extends ErrorCode> {
5151
/**
5252
* Creates a template diagnostic with the given information for the template being processed and
5353
* using the diagnostic category configured for the extended template diagnostic.
54+
*
55+
* @param relatedInformation Optional list of secondary related messages:
56+
* - Omit `sourceFile` when `start` and `end` offsets are locations within the template itself.
57+
* - Specify `sourceFile` only when referencing a separate file (e.g. directive class declaration).
5458
*/
5559
makeTemplateDiagnostic(
5660
sourceSpan: ParseSourceSpan,
@@ -59,7 +63,7 @@ export interface TemplateContext<Code extends ErrorCode> {
5963
text: string;
6064
start: number;
6165
end: number;
62-
sourceFile: ts.SourceFile;
66+
sourceFile?: ts.SourceFile;
6367
}[],
6468
): NgTemplateDiagnostic<Code>;
6569
}
@@ -80,9 +84,9 @@ export interface TemplateCheckFactory<
8084
/**
8185
* This abstract class provides a base implementation for the run method.
8286
*/
83-
export abstract class TemplateCheckWithVisitor<Code extends ErrorCode>
84-
implements TemplateCheck<Code>
85-
{
87+
export abstract class TemplateCheckWithVisitor<
88+
Code extends ErrorCode,
89+
> implements TemplateCheck<Code> {
8690
abstract code: Code;
8791

8892
/**

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ export class ExtendedTemplateCheckerImpl implements ExtendedTemplateChecker {
8585
text: string;
8686
start: number;
8787
end: number;
88-
sourceFile: ts.SourceFile;
88+
sourceFile?: ts.SourceFile;
8989
}[],
9090
): NgTemplateDiagnostic<ErrorCode> => {
9191
return this.partialCtx.templateTypeChecker.makeTemplateDiagnostic(

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -908,7 +908,7 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
908908
text: string;
909909
start: number;
910910
end: number;
911-
sourceFile: ts.SourceFile;
911+
sourceFile?: ts.SourceFile;
912912
}[],
913913
): NgTemplateDiagnostic<T> {
914914
const sfPath = absoluteFromSourceFile(clazz.getSourceFile());

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -312,8 +312,12 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
312312
const errorMsg = `The property and event halves of the two-way binding '${input.name}' are not bound to the same target.
313313
Find more at ${DOC_PAGE_BASE_URL}/guide/templates/two-way-binding`;
314314

315-
const relatedMessages: {text: string; start: number; end: number; sourceFile: ts.SourceFile}[] =
316-
[];
315+
const relatedMessages: {
316+
text: string;
317+
start: number;
318+
end: number;
319+
sourceFile?: ts.SourceFile;
320+
}[] = [];
317321

318322
if (inputConsumer.ref.nodeNameSpan && inputConsumer.ref.nodeFilePath) {
319323
const sf = this.getSourceFile(inputConsumer.ref.nodeFilePath);
@@ -336,7 +340,6 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
336340
text: message,
337341
start: outputConsumer.sourceSpan.start.offset + 1,
338342
end: outputConsumer.sourceSpan.start.offset + outputConsumer.name.length + 1,
339-
sourceFile: mapping.node.getSourceFile(),
340343
});
341344
} else {
342345
if (outputConsumer.ref.nodeNameSpan && outputConsumer.ref.nodeFilePath) {

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

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2659,6 +2659,112 @@ runInEachFileSystem(() => {
26592659
'Child nodes are defined here.',
26602660
);
26612661
});
2662+
2663+
it('should detect duplicate @content blocks in an external template', () => {
2664+
env.write(
2665+
'test.ts',
2666+
`
2667+
${foreignSetupCode}
2668+
2669+
@Component({
2670+
selector: 'test',
2671+
templateUrl: './test.html',
2672+
foreignImports: [frameworkImport(FancyButton)],
2673+
})
2674+
export class TestCmp {}
2675+
`,
2676+
);
2677+
env.write(
2678+
'test.html',
2679+
'<FancyButton> @content (icon) {} @content (icon) {} </FancyButton>',
2680+
);
2681+
const diags = env.driveDiagnostics();
2682+
expect(diags.length).toEqual(1);
2683+
expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.CONFLICTING_CONTENT_DECLARATION));
2684+
expect(diags[0].file?.fileName).toMatch(/test\.html$/);
2685+
expect(getSourceCodeForDiagnostic(diags[0])).toEqual('@content (icon) {}');
2686+
expect(diags[0].relatedInformation).toBeDefined();
2687+
expect(diags[0].relatedInformation!.length).toEqual(2);
2688+
expect(diags[0].relatedInformation![0].file?.fileName).toMatch(/test\.html$/);
2689+
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toEqual(
2690+
'@content (icon) {}',
2691+
);
2692+
expect(diags[0].relatedInformation![0].messageText).toEqual(
2693+
"The @content block 'icon' was first defined here.",
2694+
);
2695+
});
2696+
2697+
it('should detect a conflict between a @content block and property binding in an external template', () => {
2698+
env.write(
2699+
'test.ts',
2700+
`
2701+
${foreignSetupCode}
2702+
2703+
@Component({
2704+
selector: 'test',
2705+
templateUrl: './test.html',
2706+
foreignImports: [frameworkImport(FancyButton)],
2707+
})
2708+
export class TestCmp {
2709+
myIcon = document.createTextNode('circle');
2710+
}
2711+
`,
2712+
);
2713+
env.write(
2714+
'test.html',
2715+
'<FancyButton [icon]="myIcon"> @content (icon) {square} </FancyButton>',
2716+
);
2717+
const diags = env.driveDiagnostics();
2718+
expect(diags.length).toEqual(1);
2719+
expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.CONFLICTING_CONTENT_AND_PROPERTY));
2720+
expect(diags[0].file?.fileName).toMatch(/test\.html$/);
2721+
expect(getSourceCodeForDiagnostic(diags[0])).toEqual('@content (icon) {square}');
2722+
expect(diags[0].relatedInformation).toBeDefined();
2723+
expect(diags[0].relatedInformation!.length).toEqual(2);
2724+
expect(diags[0].relatedInformation![0].file?.fileName).toMatch(/test\.html$/);
2725+
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toEqual(
2726+
'[icon]="myIcon"',
2727+
);
2728+
expect(diags[0].relatedInformation![0].messageText).toEqual(
2729+
"The property 'icon' is defined here.",
2730+
);
2731+
});
2732+
2733+
it('should detect a conflict between implicit children and [children] binding in an external template', () => {
2734+
env.write(
2735+
'test.ts',
2736+
`
2737+
${foreignSetupCode}
2738+
2739+
@Component({
2740+
selector: 'test',
2741+
templateUrl: './test.html',
2742+
foreignImports: [frameworkImport(FancyButton)],
2743+
})
2744+
export class TestCmp {
2745+
myChildren = [];
2746+
}
2747+
`,
2748+
);
2749+
env.write(
2750+
'test.html',
2751+
'<FancyButton [children]="myChildren"> <div>child</div> </FancyButton>',
2752+
);
2753+
const diags = env.driveDiagnostics();
2754+
expect(diags.length).toEqual(1);
2755+
expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.CONFLICTING_CONTENT_AND_PROPERTY));
2756+
expect(diags[0].file?.fileName).toMatch(/test\.html$/);
2757+
expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[children]="myChildren"');
2758+
expect(diags[0].relatedInformation).toBeDefined();
2759+
expect(diags[0].relatedInformation!.length).toEqual(2);
2760+
expect(diags[0].relatedInformation![0].file?.fileName).toMatch(/test\.html$/);
2761+
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toEqual(
2762+
'<div>child</div>',
2763+
);
2764+
expect(diags[0].relatedInformation![0].messageText).toEqual(
2765+
'Child nodes are defined here.',
2766+
);
2767+
});
26622768
});
26632769

26642770
it('should detect a duplicate variable declaration', () => {

0 commit comments

Comments
 (0)