Skip to content

Commit eeaec92

Browse files
devversionthePunderWoman
devversion
authored and
thePunderWoman
committed
refactor(compiler-cli): ensure FatalDiagnosticError extends Error (#54309)
This helps with the Angular CLI currently swallowing fatal diagnostic errors in ways that are extremely difficult to debug due to workers executing Angular compiler logic. The worker logic, via piscina, is currently not forwarding such Angular errors because those don't extend `Error.` https://github.com/piscinajs/piscina/blob/a7042ea27d129f3cad75c422f5aa92f0663854ee/src/worker.ts#L175 Even with access to these errors by manually forwarding errors, via patching of the Angular CLI, there is no stack trace due to us not using `Error` as base class for fatal diagnostic errors. This commit improves this for future debugging and also better reporting of such errors to our users- if we would accidentally leak one. PR Close #54309
1 parent 40e1edc commit eeaec92

File tree

3 files changed

+31
-24
lines changed

3 files changed

+31
-24
lines changed

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

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,12 @@ runInEachFileSystem(() => {
2222
const {error, program} = createError('', 'nonexistent', 'Error message');
2323
const entrySf = getSourceFileOrError(program, _('/entry.ts'));
2424

25-
if (typeof error.message === 'string') {
25+
if (typeof error.diagnosticMessage === 'string') {
2626
return fail('Created error must have a message chain');
2727
}
28-
expect(error.message.messageText).toBe('Error message');
29-
expect(error.message.next!.length).toBe(1);
30-
expect(error.message.next![0].messageText)
28+
expect(error.diagnosticMessage.messageText).toBe('Error message');
29+
expect(error.diagnosticMessage.next!.length).toBe(1);
30+
expect(error.diagnosticMessage.next![0].messageText)
3131
.toBe(`Value could not be determined statically.`);
3232

3333
expect(error.relatedInformation).toBeDefined();
@@ -44,12 +44,12 @@ runInEachFileSystem(() => {
4444
[{name: _('/foo.ts'), contents: 'export class Foo {}'}]);
4545
const fooSf = getSourceFileOrError(program, _('/foo.ts'));
4646

47-
if (typeof error.message === 'string') {
47+
if (typeof error.diagnosticMessage === 'string') {
4848
return fail('Created error must have a message chain');
4949
}
50-
expect(error.message.messageText).toBe('Error message');
51-
expect(error.message.next!.length).toBe(1);
52-
expect(error.message.next![0].messageText).toBe(`Value is a reference to 'Foo'.`);
50+
expect(error.diagnosticMessage.messageText).toBe('Error message');
51+
expect(error.diagnosticMessage.next!.length).toBe(1);
52+
expect(error.diagnosticMessage.next![0].messageText).toBe(`Value is a reference to 'Foo'.`);
5353

5454
expect(error.relatedInformation).toBeDefined();
5555
expect(error.relatedInformation!.length).toBe(1);
@@ -64,12 +64,12 @@ runInEachFileSystem(() => {
6464
[{name: _('/foo.ts'), contents: 'export default class {}'}]);
6565
const fooSf = getSourceFileOrError(program, _('/foo.ts'));
6666

67-
if (typeof error.message === 'string') {
67+
if (typeof error.diagnosticMessage === 'string') {
6868
return fail('Created error must have a message chain');
6969
}
70-
expect(error.message.messageText).toBe('Error message');
71-
expect(error.message.next!.length).toBe(1);
72-
expect(error.message.next![0].messageText)
70+
expect(error.diagnosticMessage.messageText).toBe('Error message');
71+
expect(error.diagnosticMessage.next!.length).toBe(1);
72+
expect(error.diagnosticMessage.next![0].messageText)
7373
.toBe(`Value is a reference to an anonymous declaration.`);
7474

7575
expect(error.relatedInformation).toBeDefined();
@@ -82,12 +82,13 @@ runInEachFileSystem(() => {
8282
it('should include a representation of the value\'s type', () => {
8383
const {error} = createError('', '{a: 2}', 'Error message');
8484

85-
if (typeof error.message === 'string') {
85+
if (typeof error.diagnosticMessage === 'string') {
8686
return fail('Created error must have a message chain');
8787
}
88-
expect(error.message.messageText).toBe('Error message');
89-
expect(error.message.next!.length).toBe(1);
90-
expect(error.message.next![0].messageText).toBe(`Value is of type '{ a: number }'.`);
88+
expect(error.diagnosticMessage.messageText).toBe('Error message');
89+
expect(error.diagnosticMessage.next!.length).toBe(1);
90+
expect(error.diagnosticMessage.next![0].messageText)
91+
.toBe(`Value is of type '{ a: number }'.`);
9192

9293
expect(error.relatedInformation).not.toBeDefined();
9394
});

‎packages/compiler-cli/src/ngtsc/core/src/compiler.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import ts from 'typescript';
1111
import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, NoopReferencesRegistry, PipeDecoratorHandler, ReferencesRegistry} from '../../annotations';
1212
import {InjectableClassRegistry} from '../../annotations/common';
1313
import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../cycles';
14-
import {COMPILER_ERRORS_WITH_GUIDES, ERROR_DETAILS_PAGE_BASE_URL, ErrorCode, FatalDiagnosticError, ngErrorCode} from '../../diagnostics';
14+
import {COMPILER_ERRORS_WITH_GUIDES, ERROR_DETAILS_PAGE_BASE_URL, ErrorCode, isFatalDiagnosticError, ngErrorCode} from '../../diagnostics';
1515
import {DocEntry, DocsExtractor} from '../../docs';
1616
import {checkForPrivateExports, ReferenceGraph} from '../../entry_point';
1717
import {absoluteFromSourceFile, AbsoluteFsPath, LogicalFileSystem, resolve} from '../../file_system';
@@ -436,7 +436,7 @@ export class NgCompiler {
436436
diagnostics.push(...this.getExtendedTemplateDiagnostics());
437437
}
438438
} catch (err: unknown) {
439-
if (!(err instanceof FatalDiagnosticError)) {
439+
if (!isFatalDiagnosticError(err)) {
440440
throw err;
441441
}
442442
diagnostics.push(err.toDiagnostic());
@@ -466,7 +466,7 @@ export class NgCompiler {
466466
diagnostics.push(...this.getExtendedTemplateDiagnostics(file));
467467
}
468468
} catch (err: unknown) {
469-
if (!(err instanceof FatalDiagnosticError)) {
469+
if (!isFatalDiagnosticError(err)) {
470470
throw err;
471471
}
472472
diagnostics.push(err.toDiagnostic());
@@ -496,7 +496,7 @@ export class NgCompiler {
496496
diagnostics.push(...extendedTemplateChecker.getDiagnosticsForComponent(component));
497497
}
498498
} catch (err: unknown) {
499-
if (!(err instanceof FatalDiagnosticError)) {
499+
if (!isFatalDiagnosticError(err)) {
500500
throw err;
501501
}
502502
diagnostics.push(err.toDiagnostic());

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,19 +11,25 @@ import ts from 'typescript';
1111
import {ErrorCode} from './error_code';
1212
import {ngErrorCode} from './util';
1313

14-
export class FatalDiagnosticError {
14+
export class FatalDiagnosticError extends Error {
1515
constructor(
1616
readonly code: ErrorCode, readonly node: ts.Node,
17-
readonly message: string|ts.DiagnosticMessageChain,
18-
readonly relatedInformation?: ts.DiagnosticRelatedInformation[]) {}
17+
readonly diagnosticMessage: string|ts.DiagnosticMessageChain,
18+
readonly relatedInformation?: ts.DiagnosticRelatedInformation[]) {
19+
super(`FatalDiagnosticError #${code}: ${diagnosticMessage}`);
20+
}
21+
22+
// Trying to hide `.message` from `Error` to encourage users to look
23+
// at `diagnosticMessage` instead.
24+
override message: never = null!;
1925

2026
/**
2127
* @internal
2228
*/
2329
_isFatalDiagnosticError = true;
2430

2531
toDiagnostic(): ts.DiagnosticWithLocation {
26-
return makeDiagnostic(this.code, this.node, this.message, this.relatedInformation);
32+
return makeDiagnostic(this.code, this.node, this.diagnosticMessage, this.relatedInformation);
2733
}
2834
}
2935

0 commit comments

Comments
 (0)