Skip to content

Commit

Permalink
refactor(compiler-cli): add TemplateId to template diagnostics (ang…
Browse files Browse the repository at this point in the history
…ular#38105)

Previously, a stable template id was implemented for each component in a
file. This commit adds this id to each `TemplateDiagnostic` generated from
the template type-checker, so it can potentially be used for filtration.

PR Close angular#38105
  • Loading branch information
alxhub authored and Splaktar committed Aug 8, 2020
1 parent 0314fd4 commit 4096bbf
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 33 deletions.
9 changes: 5 additions & 4 deletions packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {ImportManager} from '../../translator';
import {ComponentToShimMappingStrategy, TemplateSourceMapping, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata, TypeCheckContext, TypeCheckingConfig, TypeCtorMetadata} from '../api';

import {TemplateDiagnostic} from './diagnostics';
import {DomSchemaChecker, RegistryDomSchemaChecker} from './dom';
import {Environment} from './environment';
import {OutOfBandDiagnosticRecorder, OutOfBandDiagnosticRecorderImpl} from './oob';
Expand All @@ -34,7 +35,7 @@ export interface ShimTypeCheckingData {
*
* Some diagnostics are produced during creation time and are tracked here.
*/
genesisDiagnostics: ts.Diagnostic[];
genesisDiagnostics: TemplateDiagnostic[];

/**
* Whether any inline operations for the input file were required to generate this shim.
Expand Down Expand Up @@ -182,7 +183,6 @@ export class TypeCheckContextImpl implements TypeCheckContext {
}

const sfPath = absoluteFromSourceFile(ref.node.getSourceFile());

const overrideTemplate = this.host.getTemplateOverride(sfPath, ref.node);
if (overrideTemplate !== null) {
template = overrideTemplate;
Expand Down Expand Up @@ -231,12 +231,13 @@ export class TypeCheckContextImpl implements TypeCheckContext {
// and inlining would be required.

// Record diagnostics to indicate the issues with this template.
const templateId = fileData.sourceManager.getTemplateId(ref.node);
if (tcbRequiresInline) {
shimData.oobRecorder.requiresInlineTcb(ref.node);
shimData.oobRecorder.requiresInlineTcb(templateId, ref.node);
}

if (missingInlines.length > 0) {
shimData.oobRecorder.requiresInlineTypeConstructors(ref.node, missingInlines);
shimData.oobRecorder.requiresInlineTypeConstructors(templateId, ref.node, missingInlines);
}

// Checking this template would be unsupported, so don't try.
Expand Down
17 changes: 13 additions & 4 deletions packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ export interface TemplateDiagnostic extends ts.Diagnostic {
* The component with the template that resulted in this diagnostic.
*/
componentFile: ts.SourceFile;

/**
* The template id of the component that resulted in this diagnostic.
*/
templateId: TemplateId;
}

/**
Expand Down Expand Up @@ -119,7 +124,7 @@ export function shouldReportDiagnostic(diagnostic: ts.Diagnostic): boolean {
* file from being reported as type-check errors.
*/
export function translateDiagnostic(
diagnostic: ts.Diagnostic, resolver: TemplateSourceResolver): ts.Diagnostic|null {
diagnostic: ts.Diagnostic, resolver: TemplateSourceResolver): TemplateDiagnostic|null {
if (diagnostic.file === undefined || diagnostic.start === undefined) {
return null;
}
Expand All @@ -139,7 +144,8 @@ export function translateDiagnostic(

const mapping = resolver.getSourceMapping(sourceLocation.id);
return makeTemplateDiagnostic(
mapping, span, diagnostic.category, diagnostic.code, diagnostic.messageText);
sourceLocation.id, mapping, span, diagnostic.category, diagnostic.code,
diagnostic.messageText);
}

export function findTypeCheckBlock(file: ts.SourceFile, id: TemplateId): ts.Node|null {
Expand All @@ -155,8 +161,9 @@ export function findTypeCheckBlock(file: ts.SourceFile, id: TemplateId): ts.Node
* Constructs a `ts.Diagnostic` for a given `ParseSourceSpan` within a template.
*/
export function makeTemplateDiagnostic(
mapping: TemplateSourceMapping, span: ParseSourceSpan, category: ts.DiagnosticCategory,
code: number, messageText: string|ts.DiagnosticMessageChain, relatedMessage?: {
templateId: TemplateId, mapping: TemplateSourceMapping, span: ParseSourceSpan,
category: ts.DiagnosticCategory, code: number, messageText: string|ts.DiagnosticMessageChain,
relatedMessage?: {
text: string,
span: ParseSourceSpan,
}): TemplateDiagnostic {
Expand All @@ -182,6 +189,7 @@ export function makeTemplateDiagnostic(
messageText,
file: mapping.node.getSourceFile(),
componentFile: mapping.node.getSourceFile(),
templateId,
start: span.start.offset,
length: span.end.offset - span.start.offset,
relatedInformation,
Expand Down Expand Up @@ -233,6 +241,7 @@ export function makeTemplateDiagnostic(
messageText,
file: sf,
componentFile: componentSf,
templateId,
start: span.start.offset,
length: span.end.offset - span.start.offset,
// Show a secondary message indicating the component whose template contains the error.
Expand Down
12 changes: 6 additions & 6 deletions packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import * as ts from 'typescript';
import {ErrorCode, ngErrorCode} from '../../diagnostics';
import {TemplateId} from '../api';

import {makeTemplateDiagnostic, TemplateSourceResolver} from './diagnostics';
import {makeTemplateDiagnostic, TemplateDiagnostic, TemplateSourceResolver} from './diagnostics';

const REGISTRY = new DomElementSchemaRegistry();
const REMOVE_XHTML_REGEX = /^:xhtml:/;
Expand All @@ -31,7 +31,7 @@ export interface DomSchemaChecker {
* Get the `ts.Diagnostic`s that have been generated via `checkElement` and `checkProperty` calls
* thus far.
*/
readonly diagnostics: ReadonlyArray<ts.Diagnostic>;
readonly diagnostics: ReadonlyArray<TemplateDiagnostic>;

/**
* Check a non-Angular element and record any diagnostics about it.
Expand Down Expand Up @@ -64,9 +64,9 @@ export interface DomSchemaChecker {
* maintained by the Angular team via extraction from a browser IDL.
*/
export class RegistryDomSchemaChecker implements DomSchemaChecker {
private _diagnostics: ts.Diagnostic[] = [];
private _diagnostics: TemplateDiagnostic[] = [];

get diagnostics(): ReadonlyArray<ts.Diagnostic> {
get diagnostics(): ReadonlyArray<TemplateDiagnostic> {
return this._diagnostics;
}

Expand All @@ -93,7 +93,7 @@ export class RegistryDomSchemaChecker implements DomSchemaChecker {
}

const diag = makeTemplateDiagnostic(
mapping, element.sourceSpan, ts.DiagnosticCategory.Error,
id, mapping, element.sourceSpan, ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.SCHEMA_INVALID_ELEMENT), errorMsg);
this._diagnostics.push(diag);
}
Expand Down Expand Up @@ -123,7 +123,7 @@ export class RegistryDomSchemaChecker implements DomSchemaChecker {
}

const diag = makeTemplateDiagnostic(
mapping, span, ts.DiagnosticCategory.Error,
id, mapping, span, ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.SCHEMA_INVALID_ATTRIBUTE), errorMsg);
this._diagnostics.push(diag);
}
Expand Down
47 changes: 30 additions & 17 deletions packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {ErrorCode, makeDiagnostic, makeRelatedInformation, ngErrorCode} from '..
import {ClassDeclaration} from '../../reflection';
import {TemplateId} from '../api';

import {makeTemplateDiagnostic, TemplateSourceResolver} from './diagnostics';
import {makeTemplateDiagnostic, TemplateDiagnostic, TemplateSourceResolver} from './diagnostics';



Expand All @@ -27,7 +27,7 @@ import {makeTemplateDiagnostic, TemplateSourceResolver} from './diagnostics';
* recorder for later display.
*/
export interface OutOfBandDiagnosticRecorder {
readonly diagnostics: ReadonlyArray<ts.Diagnostic>;
readonly diagnostics: ReadonlyArray<TemplateDiagnostic>;

/**
* Reports a `#ref="target"` expression in the template for which a target directive could not be
Expand Down Expand Up @@ -63,17 +63,18 @@ export interface OutOfBandDiagnosticRecorder {
duplicateTemplateVar(
templateId: TemplateId, variable: TmplAstVariable, firstDecl: TmplAstVariable): void;

requiresInlineTcb(node: ClassDeclaration): void;
requiresInlineTcb(templateId: TemplateId, node: ClassDeclaration): void;

requiresInlineTypeConstructors(node: ClassDeclaration, directives: ClassDeclaration[]): void;
requiresInlineTypeConstructors(
templateId: TemplateId, node: ClassDeclaration, directives: ClassDeclaration[]): void;
}

export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
private _diagnostics: ts.Diagnostic[] = [];
private _diagnostics: TemplateDiagnostic[] = [];

constructor(private resolver: TemplateSourceResolver) {}

get diagnostics(): ReadonlyArray<ts.Diagnostic> {
get diagnostics(): ReadonlyArray<TemplateDiagnostic> {
return this._diagnostics;
}

Expand All @@ -83,7 +84,7 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor

const errorMsg = `No directive found with exportAs '${value}'.`;
this._diagnostics.push(makeTemplateDiagnostic(
mapping, ref.valueSpan || ref.sourceSpan, ts.DiagnosticCategory.Error,
templateId, mapping, ref.valueSpan || ref.sourceSpan, ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.MISSING_REFERENCE_TARGET), errorMsg));
}

Expand All @@ -97,8 +98,8 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
`Assertion failure: no SourceLocation found for usage of pipe '${ast.name}'.`);
}
this._diagnostics.push(makeTemplateDiagnostic(
mapping, sourceSpan, ts.DiagnosticCategory.Error, ngErrorCode(ErrorCode.MISSING_PIPE),
errorMsg));
templateId, mapping, sourceSpan, ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.MISSING_PIPE), errorMsg));
}

illegalAssignmentToTemplateVar(
Expand All @@ -113,7 +114,7 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
throw new Error(`Assertion failure: no SourceLocation found for property binding.`);
}
this._diagnostics.push(makeTemplateDiagnostic(
mapping, sourceSpan, ts.DiagnosticCategory.Error,
templateId, mapping, sourceSpan, ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.WRITE_TO_READ_ONLY_VARIABLE), errorMsg, {
text: `The variable ${assignment.name} is declared here.`,
span: target.valueSpan || target.sourceSpan,
Expand All @@ -132,20 +133,21 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
//
// TODO(alxhub): allocate to a tighter span once one is available.
this._diagnostics.push(makeTemplateDiagnostic(
mapping, variable.sourceSpan, ts.DiagnosticCategory.Error,
templateId, mapping, variable.sourceSpan, ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.DUPLICATE_VARIABLE_DECLARATION), errorMsg, {
text: `The variable '${firstDecl.name}' was first declared here.`,
span: firstDecl.sourceSpan,
}));
}

requiresInlineTcb(node: ClassDeclaration): void {
this._diagnostics.push(makeDiagnostic(
ErrorCode.INLINE_TCB_REQUIRED, node.name,
requiresInlineTcb(templateId: TemplateId, node: ClassDeclaration): void {
this._diagnostics.push(makeInlineDiagnostic(
templateId, ErrorCode.INLINE_TCB_REQUIRED, node.name,
`This component requires inline template type-checking, which is not supported by the current environment.`));
}

requiresInlineTypeConstructors(node: ClassDeclaration, directives: ClassDeclaration[]): void {
requiresInlineTypeConstructors(
templateId: TemplateId, node: ClassDeclaration, directives: ClassDeclaration[]): void {
let message: string;
if (directives.length > 1) {
message =
Expand All @@ -155,9 +157,20 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
`This component uses a directive which requires an inline type constructor, which is not supported by the current environment.`;
}

this._diagnostics.push(makeDiagnostic(
ErrorCode.INLINE_TYPE_CTOR_REQUIRED, node.name, message,
this._diagnostics.push(makeInlineDiagnostic(
templateId, ErrorCode.INLINE_TYPE_CTOR_REQUIRED, node.name, message,
directives.map(
dir => makeRelatedInformation(dir.name, `Requires an inline type constructor.`))));
}
}

function makeInlineDiagnostic(
templateId: TemplateId, code: ErrorCode.INLINE_TCB_REQUIRED|ErrorCode.INLINE_TYPE_CTOR_REQUIRED,
node: ts.Node, messageText: string|ts.DiagnosticMessageChain,
relatedInformation?: ts.DiagnosticRelatedInformation[]): TemplateDiagnostic {
return {
...makeDiagnostic(code, node, messageText, relatedInformation),
componentFile: node.getSourceFile(),
templateId,
};
}
5 changes: 3 additions & 2 deletions packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckContext} from '..
import {TemplateId, TemplateSourceMapping, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata, TypeCheckingConfig, UpdateMode} from '../api/api';
import {ReusedProgramStrategy} from '../src/augmented_program';
import {TemplateTypeCheckerImpl} from '../src/checker';
import {TemplateDiagnostic} from '../src/diagnostics';
import {DomSchemaChecker} from '../src/dom';
import {Environment} from '../src/environment';
import {OutOfBandDiagnosticRecorder} from '../src/oob';
Expand Down Expand Up @@ -487,7 +488,7 @@ class FakeEnvironment /* implements Environment */ {
}

export class NoopSchemaChecker implements DomSchemaChecker {
get diagnostics(): ReadonlyArray<ts.Diagnostic> {
get diagnostics(): ReadonlyArray<TemplateDiagnostic> {
return [];
}

Expand All @@ -498,7 +499,7 @@ export class NoopSchemaChecker implements DomSchemaChecker {
}

export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
get diagnostics(): ReadonlyArray<ts.Diagnostic> {
get diagnostics(): ReadonlyArray<TemplateDiagnostic> {
return [];
}
missingReferenceTarget(): void {}
Expand Down

0 comments on commit 4096bbf

Please sign in to comment.