mirror of
https://github.com/angular/angular.git
synced 2026-09-14 13:54:52 +08:00
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.
This commit is contained in:
@@ -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(),
|
||||
},
|
||||
],
|
||||
),
|
||||
|
||||
@@ -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<T extends ErrorCode>(
|
||||
clazz: ts.ClassDeclaration,
|
||||
@@ -377,7 +381,7 @@ export interface TemplateTypeChecker {
|
||||
text: string;
|
||||
start: number;
|
||||
end: number;
|
||||
sourceFile: ts.SourceFile;
|
||||
sourceFile?: ts.SourceFile;
|
||||
}[],
|
||||
): NgTemplateDiagnostic<T>;
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -51,6 +51,10 @@ export interface TemplateContext<Code extends ErrorCode> {
|
||||
/**
|
||||
* 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<Code extends ErrorCode> {
|
||||
text: string;
|
||||
start: number;
|
||||
end: number;
|
||||
sourceFile: ts.SourceFile;
|
||||
sourceFile?: ts.SourceFile;
|
||||
}[],
|
||||
): NgTemplateDiagnostic<Code>;
|
||||
}
|
||||
@@ -80,9 +84,9 @@ export interface TemplateCheckFactory<
|
||||
/**
|
||||
* This abstract class provides a base implementation for the run method.
|
||||
*/
|
||||
export abstract class TemplateCheckWithVisitor<Code extends ErrorCode>
|
||||
implements TemplateCheck<Code>
|
||||
{
|
||||
export abstract class TemplateCheckWithVisitor<
|
||||
Code extends ErrorCode,
|
||||
> implements TemplateCheck<Code> {
|
||||
abstract code: Code;
|
||||
|
||||
/**
|
||||
|
||||
@@ -85,7 +85,7 @@ export class ExtendedTemplateCheckerImpl implements ExtendedTemplateChecker {
|
||||
text: string;
|
||||
start: number;
|
||||
end: number;
|
||||
sourceFile: ts.SourceFile;
|
||||
sourceFile?: ts.SourceFile;
|
||||
}[],
|
||||
): NgTemplateDiagnostic<ErrorCode> => {
|
||||
return this.partialCtx.templateTypeChecker.makeTemplateDiagnostic(
|
||||
|
||||
@@ -908,7 +908,7 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
|
||||
text: string;
|
||||
start: number;
|
||||
end: number;
|
||||
sourceFile: ts.SourceFile;
|
||||
sourceFile?: ts.SourceFile;
|
||||
}[],
|
||||
): NgTemplateDiagnostic<T> {
|
||||
const sfPath = absoluteFromSourceFile(clazz.getSourceFile());
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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',
|
||||
'<FancyButton> @content (icon) {} @content (icon) {} </FancyButton>',
|
||||
);
|
||||
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',
|
||||
'<FancyButton [icon]="myIcon"> @content (icon) {square} </FancyButton>',
|
||||
);
|
||||
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',
|
||||
'<FancyButton [children]="myChildren"> <div>child</div> </FancyButton>',
|
||||
);
|
||||
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(
|
||||
'<div>child</div>',
|
||||
);
|
||||
expect(diags[0].relatedInformation![0].messageText).toEqual(
|
||||
'Child nodes are defined here.',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
it('should detect a duplicate variable declaration', () => {
|
||||
|
||||
Reference in New Issue
Block a user