8000 refactor(compiler): show meaningful diagnostic when reporting a templ… · angular/angular@4a53b7f · GitHub
[go: up one dir, main page]
Skip to content

Commit 4a53b7f

Browse files
JoostKAndrewKushnir
JoostK
authored and
8000 AndrewKushnir
committed
refactor(compiler): show meaningful diagnostic when reporting a template error fails (#44001)
When the Angular compiler emits a diagnostic in a template file, it forces TypeScript to parse that template. Templates are not TypeScript, so this parse finds a bunch of parsing errors, which Angular then ignores and we show the diagnostic anyways because we have more context. This can lead to strange behavior in TypeScript because templates are so weird that it can break the parser and crash the whole compiler. For example, certain Angular templates can encounter failures fixed by microsoft/TypeScript#45987, which are not easily debuggable and require a TS upgrade to fix. This commit introduces logic to handle the error gracefully, by falling back to report the template error on the component class itself. The diagnostic is extended to still reference the template location and includes the failure's stack trace, to allow the parsing failure to be reported to TypeScript (as parsing should in theory not cause a crash). Closes #43970 PR Close #44001
1 parent 16f96ee commit 4a53b7f

File tree

5 files changed

+131
-10
lines changed

5 files changed

+131
-10
lines changed

‎packages/compiler-cli/src/ngtsc/diagnostics/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
export {COMPILER_ERRORS_WITH_GUIDES} from './src/docs';
10-
export {FatalDiagnosticError, isFatalDiagnosticError, makeDiagnostic, makeDiagnosticChain, makeRelatedInformation} from './src/error';
10+
export {addDiagnosticChain, FatalDiagnosticError, isFatalDiagnosticError, makeDiagnostic, makeDiagnosticChain, makeRelatedInformation} from './src/error';
1111
export {ErrorCode} from './src/error_code';
1212
export {ERROR_DETAILS_PAGE_BASE_URL} from './src/error_details_base_url';
1313
export {ExtendedTemplateDiagnosticName} from './src/extended_template_diagnostic_name';

‎packages/compiler-cli/src/ngtsc/diagnostics/src/error.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,22 @@ export function makeRelatedInformation(
6565
};
6666
}
6767

68+
export function addDiagnosticChain(
69+
messageText: string|ts.DiagnosticMessageChain,
70+
add: ts.DiagnosticMessageChain[]): ts.DiagnosticMessageChain {
71+
if (typeof messageText === 'string') {
72+
return makeDiagnosticChain(messageText, add);
73+
}
74+
75+
if (messageText.next === undefined) {
76+
messageText.next = add;
77+
} else {
78+
messageText.next.push(...add);
79+
}
80+
81+
return messageText;
82+
}
83+
6884
export function isFatalDiagnosticError(err: any): err is FatalDiagnosticError {
6985
return err._isFatalDiagnosticError === true;
7086
}

‎packages/compiler-cli/src/ngtsc/typecheck/diagnostics/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ ts_library(
99
deps = [
1010
"//packages:types",
1111
"//packages/compiler",
12+
"//packages/compiler-cli/src/ngtsc/diagnostics",
1213
"//packages/compiler-cli/src/ngtsc/reflection",
1314
"//packages/compiler-cli/src/ngtsc/typecheck/api",
1415
"@npm//typescript",

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

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import {ParseSourceSpan} from '@angular/compiler';
1010
import ts from 'typescript';
1111

12+
import {addDiagnosticChain, makeDiagnosticChain} from '../../../diagnostics';
1213
import {ExternalTemplateSourceMapping, TemplateDiagnostic, TemplateId, TemplateSourceMapping} from '../../api';
1314

1415
/**
@@ -64,11 +65,6 @@ export function makeTemplateDiagnostic(
6465
const fileName = mapping.type === 'indirect' ?
6566
`${componentSf.fileName} (${componentName} template)` :
6667
(mapping as ExternalTemplateSourceMapping).templateUrl;
67-
// TODO(alxhub): investigate creating a fake `ts.SourceFile` here instead of invoking the TS
68-
// parser against the template (HTML is just really syntactically invalid TypeScript code ;).
69-
// Also investigate caching the file to avoid running the parser multiple times.
70-
const sf = ts.createSourceFile(
71-
fileName, mapping.template, ts.ScriptTarget.Latest, false, ts.ScriptKind.JSX);
7268

7369
let relatedInformation: ts.DiagnosticRelatedInformation[] = [];
7470
if (relatedMessages !== undefined) {
@@ -84,6 +80,32 @@ export function makeTemplateDiagnostic(
8480
}
8581
}
8682

83+
let sf: ts.SourceFile;
84+
try {
85+
sf = parseTemplateAsSourceFile(fileName, mapping.template);
86+
} catch (e) {
87+
const failureChain = makeDiagnosticChain(
88+
`Failed to report an error in '${fileName}' at ${span.start.line + 1}:${
89+
span.start.col + 1}`,
90+
[
91+
makeDiagnosticChain((e as Error)?.stack ?? `${e}`),
92+
]);
93+
return {
94+
source: 'ngtsc',
95+
category,
96+
code,
97+
messageText: addDiagnosticChain(messageText, [failureChain]),
98+
file: componentSf,
99+
componentFile: componentSf,
100+
templateId,
101+
// mapping.node represents either the 'template' or 'templateUrl' expression. getStart()
102+
// and getEnd() are used because they don't include surrounding whitespace.
103+
start: mapping.node.getStart(),
104+
length: mapping.node.getEnd() - mapping.node.getStart(),
105+
relatedInformation,
106+
};
107+
}
108+
87109
relatedInformation.push({
88110
category: ts.DiagnosticCategory.Message,
89111
code: 0,
@@ -113,6 +135,28 @@ export function makeTemplateDiagnostic(
113135
}
114136
}
115137

138+
let parseTemplateAsSourceFileForTest: typeof parseTemplateAsSourceFile|null = null;
139+
140+
export function setParseTemplateAsSourceFileForTest(fn: typeof parseTemplateAsSourceFile): void {
141+
parseTemplateAsSourceFileForTest = fn;
142+
}
143+
144+
export function resetParseTemplateAsSourceFileForTest(): void {
145+
parseTemplateAsSourceFileForTest = null;
146+
}
147+
148+
function parseTemplateAsSourceFile(fileName: string, template: string): ts.SourceFile {
149+
if (parseTemplateAsSourceFileForTest !== null) {
150+
return parseTemplateAsSourceFileForTest(fileName, template);
151+
}
152+
153+
// TODO(alxhub): investigate creating a fake `ts.SourceFile` here instead of invoking the TS
154+
// parser against the template (HTML is just really syntactically invalid TypeScript code ;).
155+
// Also investigate caching the file to avoid running the parser multiple times.
156+
return ts.createSourceFile(
157+
fileName, template, ts.ScriptTarget.Latest, /* setParentNodes */ false, ts.ScriptKind.JSX);
158+
}
159+
116160
export function isTemplateDiagnostic(diagnostic: ts.Diagnostic): diagnostic is TemplateDiagnostic {
117161
return diagnostic.hasOwnProperty('componentFile') &&
118162
ts.isSourceFile((diagnostic as any).componentFile);

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

Lines changed: 64 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import ts from 'typescript';
1111
import {absoluteFrom, getSourceFileOrError} from '../../file_system';
1212
import {runInEachFileSystem, TestFile} from '../../file_system/testing';
1313
import {OptimizeFor, TypeCheckingConfig} from '../api';
14+
import {resetParseTemplateAsSourceFileForTest, setParseTemplateAsSourceFileForTest} from '../diagnostics';
1415
import {ngForDeclaration, ngForDts, ngIfDeclaration, ngIfDts, setup, TestDeclaration} from '../testing';
1516

1617
runInEachFileSystem(() => {
@@ -488,7 +489,8 @@ runInEachFileSystem(() => {
488489
}`);
489490

490491
expect(messages).toEqual([
491-
`TestComponent.html(1, 19): Argument of type 'string | undefined' is not assignable to parameter of type 'string'.`
492+
`TestComponent.html(1, 19): Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
493+
Type 'undefined' is not assignable to type 'string'.`
492494
]);
493495
});
494496

@@ -517,7 +519,8 @@ runInEachFileSystem(() => {
517519
}`);
518520

519521
expect(messages).toEqual([
520-
`TestComponent.html(1, 19): Argument of type 'string | undefined' is not assignable to parameter of type 'string'.`
522+
`TestComponent.html(1, 19): Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
523+
Type 'undefined' is not assignable to type 'string'.`
521524
]);
522525
});
523526
});
@@ -852,6 +855,64 @@ class TestComponent {
852855
]);
853856
});
854857
});
858+
859+
// https://github.com/angular/angular/issues/43970
860+
describe('template parse failures', () => {
861+
afterEach(resetParseTemplateAsSourceFileForTest);
862+
863+
it('baseline test without parse failure', () => {
864+
const messages = diagnose(`<div (click)="test(name)"></div>`, `
865+
export class TestComponent {
866+
name: string | undefined;
867+
test(n: string): void {}
868+
}`);
869+
870+
expect(messages).toEqual([
871+
`TestComponent.html(1, 20): Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
872+
Type 'undefined' is not assignable to type 'string'.`
873+
]);
874+
});
875+
876+
it('should handle TypeScript parse failures gracefully', () => {
877+
setParseTemplateAsSourceFileForTest(() => {
878+
throw new Error('Simulated parse failure');
879+
});
880+
881+
const messages = diagnose(`<div (click)="test(name)"></div>`, `
882+
export class TestComponent {
883+
name: string | undefined;
884+
test(n: string): void {}
885+
}`);
886+
887+
expect(messages.length).toBe(1);
888+
expect(messages[0])
889+
.toContain(
890+
`main.ts(2, 20): Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
891+
Type 'undefined' is not assignable to type 'string'.
892+
Failed to report an error in 'TestComponent.html' at 1:20
893+
Error: Simulated parse failure`);
894+
});
895+
896+
it('should handle non-Error failures gracefully', () => {
897+
setParseTemplateAsSourceFileForTest(() => {
898+
throw 'Simulated parse failure';
899+
});
900+
901+
const messages = diagnose(`<div (click)="test(name)"></div>`, `
902+
export class TestComponent {
903+
name: string | undefined;
904+
test(n: string): void {}
905+
}`);
906+
907+
expect(messages.length).toBe(1);
908+
expect(messages[0])
909+
.toContain(
910+
`main.ts(2, 20): Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
911+
Type 'undefined' is not assignable to type 'string'.
912+
Failed to report an error in 'TestComponent.html' at 1:20
913+
Simulated parse failure`);
914+
});
915+
});
855916
});
856917

857918
function diagnose(
@@ -879,8 +940,7 @@ function diagnose(
879940
const sf = getSourceFileOrError(program, sfPath);
880941
const diagnostics = templateTypeChecker.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram);
881942
return diagnostics.map(diag => {
882-
const text =
883-
typeof diag.messageText === 'string' ? diag.messageText : diag.messageText.messageText;
943+
const text = ts.flattenDiagnosticMessageText(diag.messageText, '\n');
884944
const fileName = diag.file!.fileName;
885945
const {line, character} = ts.getLineAndCharacterOfPosition(diag.file!, diag.start!);
886946
return `${fileName}(${line + 1}, ${character + 1}): ${text}`;

0 commit comments

Comments
 (0)
0