Skip to content

Commit

Permalink
fix(compiler-cli): avoid duplicate diagnostics about unknown pipes (#…
Browse files Browse the repository at this point in the history
…39517)

TCB generation occasionally transforms binding expressions twice, which can
result in a `BindingPipe` operation being `resolve()`'d multiple times. When
the pipe does not exist, this caused multiple OOB diagnostics to be recorded
about the missing pipe.

This commit fixes the problem by making the OOB recorder track which pipe
expressions have had diagnostics produced already, and only producing them
once per expression.

PR Close #39517
  • Loading branch information
alxhub authored and mhevery committed Nov 6, 2020
1 parent dd28855 commit c68ca49
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 0 deletions.
11 changes: 11 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
Expand Up @@ -73,6 +73,12 @@ export interface OutOfBandDiagnosticRecorder {
export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
private _diagnostics: TemplateDiagnostic[] = [];

/**
* Tracks which `BindingPipe` nodes have already been recorded as invalid, so only one diagnostic
* is ever produced per node.
*/
private recordedPipes = new Set<BindingPipe>();

constructor(private resolver: TemplateSourceResolver) {}

get diagnostics(): ReadonlyArray<TemplateDiagnostic> {
Expand All @@ -90,6 +96,10 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
}

missingPipe(templateId: TemplateId, ast: BindingPipe): void {
if (this.recordedPipes.has(ast)) {
return;
}

const mapping = this.resolver.getSourceMapping(templateId);
const errorMsg = `No pipe found with name '${ast.name}'.`;

Expand All @@ -101,6 +111,7 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
this._diagnostics.push(makeTemplateDiagnostic(
templateId, mapping, sourceSpan, ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.MISSING_PIPE), errorMsg));
this.recordedPipes.add(ast);
}

illegalAssignmentToTemplateVar(
Expand Down
26 changes: 26 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts
Expand Up @@ -210,6 +210,32 @@ runInEachFileSystem(() => {
]);
});

it('does not repeat diagnostics for missing pipes in directive inputs', () => {
// The directive here is structured so that a type constructor is used, which resuts in each
// input binding being processed twice. This results in the 'uppercase' pipe being resolved
// twice, and since it doesn't exist this operation will fail. The test is here to verify that
// failing to resolve the pipe twice only produces a single diagnostic (no duplicates).
const messages = diagnose(
'<div *dir="let x of name | uppercase"></div>', `
class Dir<T> {
dirOf: T;
}
class TestComponent {
name: string;
}`,
[{
type: 'directive',
name: 'Dir',
selector: '[dir]',
inputs: {'dirOf': 'dirOf'},
isGeneric: true,
}]);

expect(messages.length).toBe(1);
expect(messages[0]).toContain(`No pipe found with name 'uppercase'.`);
});

it('does not repeat diagnostics for errors within LHS of safe-navigation operator', () => {
const messages = diagnose(`{{ personn?.name }} {{ personn?.getName() }}`, `
class TestComponent {
Expand Down

0 comments on commit c68ca49

Please sign in to comment.