From 0287b234eac30dc454b46ccfc8223fdda5f74abb Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 1 Aug 2019 15:01:55 -0700 Subject: [PATCH] feat(ivy): convert all ngtsc diagnostics to ts.Diagnostics (#31952) Historically, the Angular Compiler has produced both native TypeScript diagnostics (called ts.Diagnostics) and its own internal Diagnostic format (called an api.Diagnostic). This was done because TypeScript ts.Diagnostics cannot be produced for files not in the ts.Program, and template type- checking diagnostics are naturally produced for external .html template files. This design isn't optimal for several reasons: 1) Downstream tooling (such as the CLI) must support multiple formats of diagnostics, adding to the maintenance burden. 2) ts.Diagnostics have gotten a lot better in recent releases, with support for suggested changes, highlighting of the code in question, etc. None of these changes have been of any benefit for api.Diagnostics, which have continued to be reported in a very primitive fashion. 3) A future plugin model will not support anything but ts.Diagnostics, so generating api.Diagnostics is a blocker for ngtsc-as-a-plugin. 4) The split complicates both the typings and the testing of ngtsc. To fix this issue, this commit changes template type-checking to produce ts.Diagnostics instead. Instead of reporting a special kind of diagnostic for external template files, errors in a template are always reported in a ts.Diagnostic that highlights the portion of the template which contains the error. When this template text is distinct from the source .ts file (for example, when the template is parsed from an external resource file), additional contextual information links the error back to the originating component. A template error can thus be reported in 3 separate ways, depending on how the template was configured: 1) For inline template strings which can be directly mapped to offsets in the TS code, ts.Diagnostics point to real ranges in the source. This is the case if an inline template is used with a string literal or a "no-substitution" string. For example: ```typescript @Component({..., template: `

Bar: {{baz}}

`}) export class TestCmp { bar: string; } ``` The above template contains an error (no 'baz' property of `TestCmp`). The error produced by TS will look like: ```

Bar: {{baz}}

~~~ test.ts:2:11 - error TS2339: Property 'baz' does not exist on type 'TestCmp'. Did you mean 'bar'? ``` 2) For template strings which cannot be directly mapped to offsets in the TS code, a logical offset into the template string will be included in the error message. For example: ```typescript const SOME_TEMPLATE = '

Bar: {{baz}}

'; @Component({..., template: SOME_TEMPLATE}) export class TestCmp { bar: string; } ``` Because the template is a reference to another variable and is not an inline string constant, the compiler will not be able to use "absolute" positions when parsing the template. As a result, errors will report logical offsets into the template string: ```

Bar: {{baz}}

~~~ test.ts (TestCmp template):2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'. test.ts:3:28 @Component({..., template: TEMPLATE}) ~~~~~~~~ Error occurs in the template of component TestCmp. ``` This error message uses logical offsets into the template string, and also gives a reference to the `TEMPLATE` expression from which the template was parsed. This helps in locating the component which contains the error. 3) For external templates (templateUrl), the error message is delivered within the HTML template file (testcmp.html) instead, and additional information contextualizes the error on the templateUrl expression from which the template file was determined: ```

Bar: {{baz}}

~~~ testcmp.html:2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'. test.ts:10:31 @Component({..., templateUrl: './testcmp.html'}) ~~~~~~~~~~~~~~~~ Error occurs in the template of component TestCmp. ``` PR Close #31952 --- .../src/ngtsc/annotations/src/component.ts | 142 +++++++++++++++--- packages/compiler-cli/src/ngtsc/program.ts | 8 +- .../src/ngtsc/typecheck/src/api.ts | 51 +++++++ .../src/ngtsc/typecheck/src/context.ts | 52 ++++--- .../src/ngtsc/typecheck/src/diagnostics.ts | 122 +++++++++------ .../ngtsc/typecheck/src/type_check_block.ts | 5 +- .../ngtsc/typecheck/test/diagnostics_spec.ts | 48 +++--- .../src/ngtsc/typecheck/test/test_utils.ts | 18 ++- packages/compiler-cli/test/ngtsc/env.ts | 5 +- .../test/ngtsc/template_typecheck_spec.ts | 119 ++++++++++----- 10 files changed, 407 insertions(+), 163 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 5f18e37ffd..55c511ee2b 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -20,7 +20,7 @@ import {EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; import {ComponentScopeReader, LocalModuleScopeRegistry} from '../../scope'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform'; -import {TypeCheckContext} from '../../typecheck'; +import {TemplateSourceMapping, TypeCheckContext} from '../../typecheck'; import {NoopResourceDependencyRecorder, ResourceDependencyRecorder} from '../../util/src/resource_recorder'; import {tsSourceMapBug29300Fixed} from '../../util/src/ts_source_map_bug_29300'; @@ -34,7 +34,8 @@ const EMPTY_ARRAY: any[] = []; export interface ComponentHandlerData { meta: R3ComponentMetadata; - parsedTemplate: {nodes: TmplAstNode[]; file: ParseSourceFile}; + parsedTemplate: ParsedTemplate; + templateSourceMapping: TemplateSourceMapping; metadataStmt: Statement|null; parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate; } @@ -63,7 +64,7 @@ export class ComponentDecoratorHandler implements * any potential tags which might need to be loaded. This cache ensures that work is not * thrown away, and the parsed template is reused during the analyze phase. */ - private preanalyzeTemplateCache = new Map(); + private preanalyzeTemplateCache = new Map(); readonly precedence = HandlerPrecedence.PRIMARY; @@ -174,18 +175,22 @@ export class ComponentDecoratorHandler implements // Extract a closure of the template parsing code so that it can be reparsed with different // options if needed, like in the indexing pipeline. let parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate; + // Track the origin of the template to determine how the ParseSourceSpans should be interpreted. + let templateSourceMapping: TemplateSourceMapping; if (this.preanalyzeTemplateCache.has(node)) { // The template was parsed in preanalyze. Use it and delete it to save memory. const template = this.preanalyzeTemplateCache.get(node) !; this.preanalyzeTemplateCache.delete(node); - // A pre-analyzed template cannot be reparsed. Pre-analysis is never run with the indexing - // pipeline. - parseTemplate = (options?: ParseTemplateOptions) => { - if (options !== undefined) { - throw new Error(`Cannot reparse a pre-analyzed template with new options`); - } - return template; + parseTemplate = template.parseTemplate; + + // A pre-analyzed template is always an external mapping. + templateSourceMapping = { + type: 'external', + componentClass: node, + node: component.get('templateUrl') !, + template: template.template, + templateUrl: template.templateUrl, }; } else { // The template was not already parsed. Either there's a templateUrl, or an inline template. @@ -203,6 +208,13 @@ export class ComponentDecoratorHandler implements parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate( component, templateStr, sourceMapUrl(templateUrl), /* templateRange */ undefined, /* escapedString */ false, options); + templateSourceMapping = { + type: 'external', + componentClass: node, + node: templateUrlExpr, + template: templateStr, + templateUrl: templateUrl, + }; } else { // Expect an inline template to be present. const inlineTemplate = this._extractInlineTemplate(component, containingFile); @@ -214,6 +226,20 @@ export class ComponentDecoratorHandler implements const {templateStr, templateUrl, templateRange, escapedString} = inlineTemplate; parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate( component, templateStr, templateUrl, templateRange, escapedString, options); + if (escapedString) { + templateSourceMapping = { + type: 'direct', + node: + component.get('template') !as(ts.StringLiteral | ts.NoSubstitutionTemplateLiteral), + }; + } else { + templateSourceMapping = { + type: 'indirect', + node: component.get('template') !, + componentClass: node, + template: templateStr, + }; + } } } const template = parseTemplate(); @@ -308,7 +334,7 @@ export class ComponentDecoratorHandler implements }, metadataStmt: generateSetClassMetadataCall( node, this.reflector, this.defaultImportRecorder, this.isCore), - parsedTemplate: template, parseTemplate, + parsedTemplate: template, parseTemplate, templateSourceMapping, }, typeCheck: true, }; @@ -354,8 +380,22 @@ export class ComponentDecoratorHandler implements return; } - const pipes = new Map>>(); + // There are issues with parsing the template under certain configurations (namely with + // `preserveWhitespaces: false`) which cause inaccurate positional information within the + // template AST, particularly within interpolation expressions. + // + // To work around this, the template is re-parsed with settings that guarantee the spans are as + // accurate as possible. This is only a temporary solution until the whitespace removal step can + // be rewritten as a transform against the expression AST instead of against the HTML AST. + // + // TODO(alxhub): remove this when whitespace removal no longer corrupts span information. + const template = meta.parseTemplate({ + preserveWhitespaces: true, + leadingTriviaChars: [], + }); + const matcher = new SelectorMatcher(); + const pipes = new Map>>(); const scope = this.scopeReader.getScopeForComponent(node); if (scope !== null) { @@ -372,8 +412,8 @@ export class ComponentDecoratorHandler implements } } - const bound = new R3TargetBinder(matcher).bind({template: meta.parsedTemplate.nodes}); - ctx.addTemplate(new Reference(node), bound, pipes, meta.parsedTemplate.file); + const bound = new R3TargetBinder(matcher).bind({template: template.nodes}); + ctx.addTemplate(new Reference(node), bound, pipes, meta.templateSourceMapping, template.file); } resolve(node: ClassDeclaration, analysis: ComponentHandlerData): ResolveResult { @@ -561,10 +601,12 @@ export class ComponentDecoratorHandler implements return templatePromise.then(() => { const templateStr = this.resourceLoader.load(resourceUrl); this.resourceDependencies.recordResourceDependency(node.getSourceFile(), resourceUrl); - const template = this._parseTemplate( - component, templateStr, sourceMapUrl(resourceUrl), /* templateRange */ undefined, - /* escapedString */ false); - this.preanalyzeTemplateCache.set(node, template); + const parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate( + component, templateStr, sourceMapUrl(resourceUrl), + /* templateRange */ undefined, + /* escapedString */ false, options); + const template = parseTemplate(); + this.preanalyzeTemplateCache.set(node, {...template, parseTemplate}); return template; }); } else { @@ -579,9 +621,10 @@ export class ComponentDecoratorHandler implements } const {templateStr, templateUrl, escapedString, templateRange} = inlineTemplate; - const template = - this._parseTemplate(component, templateStr, templateUrl, templateRange, escapedString); - this.preanalyzeTemplateCache.set(node, template); + const parseTemplate = (options?: ParseTemplateOptions) => this._parseTemplate( + component, templateStr, templateUrl, templateRange, escapedString, options); + const template = parseTemplate(); + this.preanalyzeTemplateCache.set(node, {...template, parseTemplate}); return Promise.resolve(template); } } @@ -656,6 +699,7 @@ export class ComponentDecoratorHandler implements interpolationConfig: interpolation, range: templateRange, escapedString, ...options, }), + template: templateStr, templateUrl, isInline: component.has('template'), file: new ParseSourceFile(templateStr, templateUrl), }; @@ -713,12 +757,66 @@ function sourceMapUrl(resourceUrl: string): string { } } -interface ParsedTemplate { + +/** + * Information about the template which was extracted during parsing. + * + * This contains the actual parsed template as well as any metadata collected during its parsing, + * some of which might be useful for re-parsing the template with different options. + */ +export interface ParsedTemplate { + /** + * The `InterpolationConfig` specified by the user. + */ interpolation: InterpolationConfig; + + /** + * A full path to the file which contains the template. + * + * This can be either the original .ts file if the template is inline, or the .html file if an + * external file was used. + */ + templateUrl: string; + + /** + * The string contents of the template. + * + * This is the "logical" template string, after expansion of any escaped characters (for inline + * templates). This may differ from the actual template bytes as they appear in the .ts file. + */ + template: string; + + /** + * Any errors from parsing the template the first time. + */ errors?: ParseError[]|undefined; + + /** + * The actual parsed template nodes. + */ nodes: TmplAstNode[]; + + /** + * Any styleUrls extracted from the metadata. + */ styleUrls: string[]; + + /** + * Any inline styles extracted from the metadata. + */ styles: string[]; + + /** + * Whether the template was inline. + */ isInline: boolean; + + /** + * The `ParseSourceFile` for the template. + */ file: ParseSourceFile; } + +interface PreanalyzedTemplate extends ParsedTemplate { + parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate; +} \ No newline at end of file diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index c360bb52b8..bc74db24b5 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -175,7 +175,7 @@ export class NgtscProgram implements api.Program { } getNgOptionDiagnostics(cancellationToken?: ts.CancellationToken| - undefined): ReadonlyArray { + undefined): ReadonlyArray { return this.constructionDiagnostics; } @@ -197,8 +197,8 @@ export class NgtscProgram implements api.Program { } getNgSemanticDiagnostics( - fileName?: string|undefined, cancellationToken?: ts.CancellationToken| - undefined): ReadonlyArray { + fileName?: string|undefined, + cancellationToken?: ts.CancellationToken|undefined): ReadonlyArray { const compilation = this.ensureAnalyzed(); const diagnostics = [...compilation.diagnostics, ...this.getTemplateDiagnostics()]; if (this.entryPoint !== null && this.exportReferenceGraph !== null) { @@ -381,7 +381,7 @@ export class NgtscProgram implements api.Program { return ((opts && opts.mergeEmitResultsCallback) || mergeEmitResults)(emitResults); } - private getTemplateDiagnostics(): ReadonlyArray { + private getTemplateDiagnostics(): ReadonlyArray { // Skip template type-checking if it's disabled. if (this.options.ivyTemplateTypeCheck === false && this.options.fullTemplateTypeCheck !== true) { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts index 5d251b5fd1..ce0ae95d33 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts @@ -29,6 +29,13 @@ export interface TypeCheckableDirectiveMeta extends DirectiveMeta { * for that component. */ export interface TypeCheckBlockMetadata { + /** + * A unique identifier for the class which gave rise to this TCB. + * + * This can be used to map errors back to the `ts.ClassDeclaration` for the component. + */ + id: string; + /** * Semantic information about the template of the component. */ @@ -104,3 +111,47 @@ export interface TypeCheckingConfig { */ checkQueries: false; } + + +export type TemplateSourceMapping = + DirectTemplateSourceMapping | IndirectTemplateSourceMapping | ExternalTemplateSourceMapping; + +/** + * A mapping to an inline template in a TS file. + * + * `ParseSourceSpan`s for this template should be accurate for direct reporting in a TS error + * message. + */ +export interface DirectTemplateSourceMapping { + type: 'direct'; + node: ts.StringLiteral|ts.NoSubstitutionTemplateLiteral; +} + +/** + * A mapping to a template which is still in a TS file, but where the node positions in any + * `ParseSourceSpan`s are not accurate for one reason or another. + * + * This can occur if the template expression was interpolated in a way where the compiler could not + * construct a contiguous mapping for the template string. The `node` refers to the `template` + * expression. + */ +export interface IndirectTemplateSourceMapping { + type: 'indirect'; + componentClass: ClassDeclaration; + node: ts.Expression; + template: string; +} + +/** + * A mapping to a template declared in an external HTML file, where node positions in + * `ParseSourceSpan`s represent accurate offsets into the external file. + * + * In this case, the given `node` refers to the `templateUrl` expression. + */ +export interface ExternalTemplateSourceMapping { + type: 'external'; + componentClass: ClassDeclaration; + node: ts.Expression; + template: string; + templateUrl: string; +} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index 6592958448..cd8c6ca513 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -14,8 +14,8 @@ import {NoopImportRewriter, Reference, ReferenceEmitter} from '../../imports'; import {ClassDeclaration} from '../../reflection'; import {ImportManager} from '../../translator'; -import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from './api'; -import {Diagnostic, SourceLocation, getSourceReferenceName, shouldReportDiagnostic, translateDiagnostic} from './diagnostics'; +import {TemplateSourceMapping, TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from './api'; +import {SourceLocation, TcbSourceResolver, shouldReportDiagnostic, translateDiagnostic} from './diagnostics'; import {Environment} from './environment'; import {TypeCheckProgramHost} from './host'; import {computeLineStartsMap, getLineAndCharacterFromPosition} from './line_mappings'; @@ -53,10 +53,12 @@ export class TypeCheckContext { */ private typeCtorPending = new Set(); + + private nextTcbId: number = 1; /** - * This map keeps track of all template sources that have been type-checked by the reference name - * that is attached to a TCB's function declaration as leading trivia. This enables translation - * of diagnostics produced for TCB code to their source location in the template. + * This map keeps track of all template sources that have been type-checked by the id that is + * attached to a TCB's function declaration as leading trivia. This enables translation of + * diagnostics produced for TCB code to their source location in the template. */ private templateSources = new Map(); @@ -72,8 +74,9 @@ export class TypeCheckContext { ref: Reference>, boundTarget: BoundTarget, pipes: Map>>, - file: ParseSourceFile): void { - this.templateSources.set(getSourceReferenceName(ref.node), new TemplateSource(file)); + sourceMapping: TemplateSourceMapping, file: ParseSourceFile): void { + const id = `tcb${this.nextTcbId++}`; + this.templateSources.set(id, new TemplateSource(sourceMapping, file)); // Get all of the directives used in the template and record type constructors for all of them. for (const dir of boundTarget.getUsedDirectives()) { @@ -96,13 +99,14 @@ export class TypeCheckContext { } } + if (requiresInlineTypeCheckBlock(ref.node)) { // This class didn't meet the requirements for external type checking, so generate an inline // TCB for the class. - this.addInlineTypeCheckBlock(ref, {boundTarget, pipes}); + this.addInlineTypeCheckBlock(ref, {id, boundTarget, pipes}); } else { // The class can be type-checked externally as normal. - this.typeCheckFile.addTypeCheckBlock(ref, {boundTarget, pipes}); + this.typeCheckFile.addTypeCheckBlock(ref, {id, boundTarget, pipes}); } } @@ -177,7 +181,7 @@ export class TypeCheckContext { calculateTemplateDiagnostics( originalProgram: ts.Program, originalHost: ts.CompilerHost, originalOptions: ts.CompilerOptions): { - diagnostics: Diagnostic[], + diagnostics: ts.Diagnostic[], program: ts.Program, } { const typeCheckSf = this.typeCheckFile.render(); @@ -201,18 +205,27 @@ export class TypeCheckContext { rootNames: originalProgram.getRootFileNames(), }); - const diagnostics: Diagnostic[] = []; - const resolveSpan = (sourceLocation: SourceLocation): ParseSourceSpan | null => { - if (!this.templateSources.has(sourceLocation.sourceReference)) { - return null; - } - const templateSource = this.templateSources.get(sourceLocation.sourceReference) !; - return templateSource.toParseSourceSpan(sourceLocation.start, sourceLocation.end); + const tcbResolver: TcbSourceResolver = { + getSourceMapping: (id: string): TemplateSourceMapping => { + if (!this.templateSources.has(id)) { + throw new Error(`Unexpected unknown TCB ID: ${id}`); + } + return this.templateSources.get(id) !.mapping; + }, + sourceLocationToSpan: (location: SourceLocation): ParseSourceSpan | null => { + if (!this.templateSources.has(location.id)) { + return null; + } + const templateSource = this.templateSources.get(location.id) !; + return templateSource.toParseSourceSpan(location.start, location.end); + }, }; + + const diagnostics: ts.Diagnostic[] = []; const collectDiagnostics = (diags: readonly ts.Diagnostic[]): void => { for (const diagnostic of diags) { if (shouldReportDiagnostic(diagnostic)) { - const translated = translateDiagnostic(diagnostic, resolveSpan); + const translated = translateDiagnostic(diagnostic, tcbResolver); if (translated !== null) { diagnostics.push(translated); @@ -243,6 +256,7 @@ export class TypeCheckContext { } } + /** * Represents the source of a template that was processed during type-checking. This information is * used when translating parse offsets in diagnostics back to their original line/column location. @@ -250,7 +264,7 @@ export class TypeCheckContext { class TemplateSource { private lineStarts: number[]|null = null; - constructor(private file: ParseSourceFile) {} + constructor(readonly mapping: TemplateSourceMapping, private file: ParseSourceFile) {} toParseSourceSpan(start: number, end: number): ParseSourceSpan { const startLoc = this.toParseLocation(start); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts index 3ddb508486..08b0a5e650 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts @@ -8,35 +8,33 @@ import {ParseSourceSpan, ParseSpan, Position} from '@angular/compiler'; import * as ts from 'typescript'; -import {ClassDeclaration} from '../../reflection'; -import {getSourceFile, getTokenAtPosition} from '../../util/src/typescript'; +import {getTokenAtPosition} from '../../util/src/typescript'; -/** - * FIXME: Taken from packages/compiler-cli/src/transformers/api.ts to prevent circular dep, - * modified to account for new span notation. - */ -export interface DiagnosticMessageChain { - messageText: string; - position?: Position; - next?: DiagnosticMessageChain; -} - -export interface Diagnostic { - messageText: string; - span?: ParseSourceSpan; - position?: Position; - chain?: DiagnosticMessageChain; - category: ts.DiagnosticCategory; - code: number; - source: 'angular'; -} +import {ExternalTemplateSourceMapping, TemplateSourceMapping} from './api'; export interface SourceLocation { - sourceReference: string; + id: string; start: number; end: number; } +/** + * Adapter interface which allows the template type-checking diagnostics code to interpret offsets + * in a TCB and map them back to original locations in the template. + */ +export interface TcbSourceResolver { + /** + * For the given template id, retrieve the original source mapping which describes how the offsets + * in the template should be interpreted. + */ + getSourceMapping(id: string): TemplateSourceMapping; + + /** + * Convert a location extracted from a TCB into a `ParseSourceSpan` if possible. + */ + sourceLocationToSpan(location: SourceLocation): ParseSourceSpan|null; +} + /** * An `AbsoluteSpan` is the result of translating the `ParseSpan` of `AST` template expression nodes * to their absolute positions, as the `ParseSpan` is always relative to the start of the @@ -95,15 +93,8 @@ function isAbsoluteSpan(span: AbsoluteSpan | ParseSourceSpan): span is AbsoluteS * Adds a synthetic comment to the function declaration that contains the source location * of the class declaration. */ -export function addSourceReferenceName( - tcb: ts.FunctionDeclaration, source: ClassDeclaration): void { - const commentText = getSourceReferenceName(source); - ts.addSyntheticLeadingComment(tcb, ts.SyntaxKind.MultiLineCommentTrivia, commentText, true); -} - -export function getSourceReferenceName(source: ClassDeclaration): string { - const fileName = getSourceFile(source).fileName; - return `${fileName}#${source.name.text}`; +export function addSourceId(tcb: ts.FunctionDeclaration, id: string): void { + ts.addSyntheticLeadingComment(tcb, ts.SyntaxKind.MultiLineCommentTrivia, id, true); } /** @@ -132,8 +123,7 @@ export function shouldReportDiagnostic(diagnostic: ts.Diagnostic): boolean { * file from being reported as type-check errors. */ export function translateDiagnostic( - diagnostic: ts.Diagnostic, resolveParseSource: (sourceLocation: SourceLocation) => - ParseSourceSpan | null): Diagnostic|null { + diagnostic: ts.Diagnostic, resolver: TcbSourceResolver): ts.Diagnostic|null { if (diagnostic.file === undefined || diagnostic.start === undefined) { return null; } @@ -146,7 +136,7 @@ export function translateDiagnostic( } // Now use the external resolver to obtain the full `ParseSourceFile` of the template. - const span = resolveParseSource(sourceLocation); + const span = resolver.sourceLocationToSpan(sourceLocation); if (span === null) { return null; } @@ -158,11 +148,59 @@ export function translateDiagnostic( messageText = diagnostic.messageText.messageText; } - return { - source: 'angular', - code: diagnostic.code, - category: diagnostic.category, messageText, span, - }; + const mapping = resolver.getSourceMapping(sourceLocation.id); + if (mapping.type === 'direct') { + // For direct mappings, the error is shown inline as ngtsc was able to pinpoint a string + // constant within the `@Component` decorator for the template. This allows us to map the error + // directly into the bytes of the source file. + return { + source: 'ngtsc', + file: mapping.node.getSourceFile(), + start: span.start.offset, + length: span.end.offset - span.start.offset, + code: diagnostic.code, messageText, + category: diagnostic.category, + }; + } else if (mapping.type === 'indirect' || mapping.type === 'external') { + // For indirect mappings (template was declared inline, but ngtsc couldn't map it directly + // to a string constant in the decorator), the component's file name is given with a suffix + // indicating it's not the TS file being displayed, but a template. + // For external temoplates, the HTML filename is used. + const componentSf = mapping.componentClass.getSourceFile(); + const componentName = mapping.componentClass.name.text; + // TODO(alxhub): remove cast when TS in g3 supports this narrowing. + const fileName = mapping.type === 'indirect' ? + `${componentSf.fileName} (${componentName} template)` : + (mapping as ExternalTemplateSourceMapping).templateUrl; + // TODO(alxhub): investigate creating a fake `ts.SourceFile` here instead of invoking the TS + // parser against the template (HTML is just really syntactically invalid TypeScript code ;). + // Also investigate caching the file to avoid running the parser multiple times. + const sf = ts.createSourceFile( + fileName, mapping.template, ts.ScriptTarget.Latest, false, ts.ScriptKind.JSX); + + return { + source: 'ngtsc', + file: sf, + start: span.start.offset, + length: span.end.offset - span.start.offset, + messageText: diagnostic.messageText, + category: diagnostic.category, + code: diagnostic.code, + // Show a secondary message indicating the component whose template contains the error. + relatedInformation: [{ + category: ts.DiagnosticCategory.Message, + code: 0, + file: componentSf, + // mapping.node represents either the 'template' or 'templateUrl' expression. getStart() + // and getEnd() are used because they don't include surrounding whitespace. + start: mapping.node.getStart(), + length: mapping.node.getEnd() - mapping.node.getStart(), + messageText: `Error occurs in the template of component ${componentName}.`, + }], + }; + } else { + throw new Error(`Unexpected source mapping type: ${(mapping as {type: string}).type}`); + } } function findSourceLocation(node: ts.Node, sourceFile: ts.SourceFile): SourceLocation|null { @@ -201,7 +239,7 @@ function toSourceLocation( } } - const sourceReference = + const id = ts.forEachLeadingCommentRange(sourceFile.text, tcb.getFullStart(), (pos, end, kind) => { if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { return null; @@ -209,12 +247,12 @@ function toSourceLocation( const commentText = sourceFile.text.substring(pos, end); return commentText.substring(2, commentText.length - 2); }) || null; - if (sourceReference === null) { + if (id === null) { return null; } return { - sourceReference, + id, start: parseSpan.start, end: parseSpan.end, }; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index 41615b54cd..04669baeb7 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -13,12 +13,13 @@ import {Reference} from '../../imports'; import {ClassDeclaration} from '../../reflection'; import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api'; -import {addParseSpanInfo, addSourceReferenceName, toAbsoluteSpan, wrapForDiagnostics} from './diagnostics'; +import {addParseSpanInfo, addSourceId, toAbsoluteSpan, wrapForDiagnostics} from './diagnostics'; import {Environment} from './environment'; import {astToTypescript} from './expression'; import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateVariable, tsDeclareVariable} from './ts_util'; + /** * Given a `ts.ClassDeclaration` for a component, and metadata regarding that component, compose a * "type check block" function. @@ -60,7 +61,7 @@ export function generateTypeCheckBlock( /* parameters */ paramList, /* type */ undefined, /* body */ body); - addSourceReferenceName(fnDecl, ref.node); + addSourceId(fnDecl, meta.id); return fnDecl; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts index ea31cff717..5b133cb3f5 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts @@ -9,8 +9,6 @@ import {TestFile, runInEachFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; import * as ts from 'typescript'; -import {Diagnostic} from '../src/diagnostics'; - import {TestDeclaration, ngForDeclaration, ngForDts, typecheck} from './test_utils'; runInEachFileSystem(() => { @@ -35,7 +33,7 @@ runInEachFileSystem(() => { }]); expect(messages).toEqual( - [`synthetic.html(9, 30): Type 'string' is not assignable to type 'number | undefined'.`]); + [`synthetic.html(1, 10): Type 'string' is not assignable to type 'number | undefined'.`]); }); it('infers type of template variables', () => { @@ -49,7 +47,7 @@ runInEachFileSystem(() => { [ngForDeclaration()], [ngForDts()]); expect(messages).toEqual([ - `synthetic.html(61, 64): Argument of type 'number' is not assignable to parameter of type 'string'.`, + `synthetic.html(1, 62): Argument of type 'number' is not assignable to parameter of type 'string'.`, ]); }); @@ -83,7 +81,7 @@ runInEachFileSystem(() => { }]); expect(messages).toEqual([ - `synthetic.html(23, 25): Argument of type 'HTMLDivElement' is not assignable to parameter of type 'string'.`, + `synthetic.html(1, 24): Argument of type 'HTMLDivElement' is not assignable to parameter of type 'string'.`, ]); }); @@ -104,7 +102,7 @@ runInEachFileSystem(() => { }]); expect(messages).toEqual([ - `synthetic.html(30, 33): Argument of type 'Dir' is not assignable to parameter of type 'string'.`, + `synthetic.html(1, 31): Argument of type 'Dir' is not assignable to parameter of type 'string'.`, ]); }); @@ -115,7 +113,7 @@ runInEachFileSystem(() => { }`); expect(messages).toEqual([ - `synthetic.html(29, 33): Argument of type 'TemplateRef' is not assignable to parameter of type 'string'.`, + `synthetic.html(1, 30): Argument of type 'TemplateRef' is not assignable to parameter of type 'string'.`, ]); }); @@ -130,7 +128,7 @@ runInEachFileSystem(() => { [ngForDeclaration()], [ngForDts()]); expect(messages).toEqual([ - `synthetic.html(39, 52): Property 'namme' does not exist on type '{ name: string; }'. Did you mean 'name'?`, + `synthetic.html(1, 40): Property 'namme' does not exist on type '{ name: string; }'. Did you mean 'name'?`, ]); }); @@ -151,8 +149,8 @@ runInEachFileSystem(() => { }`); expect(messages).toEqual([ - `synthetic.html(5, 17): Property 'srcc' does not exist on type 'HTMLImageElement'. Did you mean 'src'?`, - `synthetic.html(28, 34): Property 'heihgt' does not exist on type 'TestComponent'. Did you mean 'height'?`, + `synthetic.html(1, 6): Property 'srcc' does not exist on type 'HTMLImageElement'. Did you mean 'src'?`, + `synthetic.html(1, 29): Property 'heihgt' does not exist on type 'TestComponent'. Did you mean 'height'?`, ]); }); @@ -171,7 +169,7 @@ runInEachFileSystem(() => { [{type: 'pipe', name: 'Pipe', pipeName: 'pipe'}]); expect(messages).toEqual([ - `synthetic.html(27, 37): Argument of type 'number' is not assignable to parameter of type 'string'.`, + `synthetic.html(1, 28): Argument of type 'number' is not assignable to parameter of type 'string'.`, ]); }); @@ -199,7 +197,7 @@ runInEachFileSystem(() => { }; }`); - expect(messages).toEqual([`synthetic.html(25, 46): Object is possibly 'undefined'.`]); + expect(messages).toEqual([`synthetic.html(1, 26): Object is possibly 'undefined'.`]); }); it('does not produce diagnostic for checked property access', () => { @@ -218,7 +216,7 @@ runInEachFileSystem(() => { }); it('computes line and column offsets', () => { - const diagnostics = typecheck( + const messages = diagnose( `
{ - const span = diagnostic.span !; - return `${span.start.file.url}(${span.start.offset}, ${span.end.offset}): ${diagnostic.messageText}`; + return diagnostics.map(diag => { + const text = + typeof diag.messageText === 'string' ? diag.messageText : diag.messageText.messageText; + const fileName = diag.file !.fileName; + const {line, character} = ts.getLineAndCharacterOfPosition(diag.file !, diag.start !); + return `${fileName}(${line + 1}, ${character + 1}): ${text}`; }); } - -function formatSpan(diagostic: ts.Diagnostic | Diagnostic): string { - if (diagostic.source !== 'angular') { - return ''; - } - const diag = diagostic as Diagnostic; - return `${diag.span!.start.line}:${diag.span!.start.col}, ${diag.span!.end.line}:${diag.span!.end.col}`; -} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index ed8e5772de..a0876867e2 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -15,9 +15,8 @@ import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, import {ClassDeclaration, TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection'; import {makeProgram} from '../../testing'; import {getRootDirs} from '../../util/src/typescript'; -import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig} from '../src/api'; +import {TemplateSourceMapping, TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig} from '../src/api'; import {TypeCheckContext} from '../src/context'; -import {Diagnostic} from '../src/diagnostics'; import {Environment} from '../src/environment'; import {generateTypeCheckBlock} from '../src/type_check_block'; @@ -151,7 +150,7 @@ export function tcb( const binder = new R3TargetBinder(matcher); const boundTarget = binder.bind({template: nodes}); - const meta: TypeCheckBlockMetadata = {boundTarget, pipes}; + const meta: TypeCheckBlockMetadata = {boundTarget, pipes, id: 'tcb'}; config = config || { applyTemplateContextGuards: true, @@ -175,7 +174,7 @@ export function tcb( export function typecheck( template: string, source: string, declarations: TestDeclaration[] = [], - additionalSources: {name: AbsoluteFsPath; contents: string}[] = []): Diagnostic[] { + additionalSources: {name: AbsoluteFsPath; contents: string}[] = []): ts.Diagnostic[] { const typeCheckFilePath = absoluteFrom('/_typecheck_.ts'); const files = [ typescriptLibDts(), @@ -219,7 +218,16 @@ export function typecheck( const boundTarget = binder.bind({template: nodes}); const clazz = new Reference(getClass(sf, 'TestComponent')); - ctx.addTemplate(clazz, boundTarget, pipes, templateFile); + const sourceMapping: TemplateSourceMapping = { + type: 'external', + template, + templateUrl, + componentClass: clazz.node, + // Use the class's name for error mappings. + node: clazz.node.name, + }; + + ctx.addTemplate(clazz, boundTarget, pipes, sourceMapping, templateFile); return ctx.calculateTemplateDiagnostics(program, host, options).diagnostics; } diff --git a/packages/compiler-cli/test/ngtsc/env.ts b/packages/compiler-cli/test/ngtsc/env.ts index 51dab03618..445c70e569 100644 --- a/packages/compiler-cli/test/ngtsc/env.ts +++ b/packages/compiler-cli/test/ngtsc/env.ts @@ -186,8 +186,9 @@ export class NgtscTestEnvironment { /** * Run the compiler to completion, and return any `ts.Diagnostic` errors that may have occurred. */ - driveDiagnostics(): ReadonlyArray { - return mainDiagnosticsForTest(['-p', this.basePath]); + driveDiagnostics(): ReadonlyArray { + // ngtsc only produces ts.Diagnostic messages. + return mainDiagnosticsForTest(['-p', this.basePath]) as ts.Diagnostic[]; } driveRoutes(entryPoint?: string): LazyRoute[] { diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index ed4fe3b5ae..64a9f3f8ba 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -6,9 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ -import {Diagnostic} from '@angular/compiler-cli'; import * as ts from 'typescript'; +import {absoluteFrom as _} from '../../src/ngtsc/file_system'; import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; import {loadStandardTestFiles} from '../helpers/src/mock_file_loading'; @@ -171,8 +171,10 @@ export declare class CommonModule { const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); - expect(diags[0].messageText).toContain('does_not_exist'); - expect(formatSpan(diags[0])).toEqual('/test.ts: 6:51, 6:70'); + expect(diags[0].messageText) + .toEqual(`Property 'does_not_exist' does not exist on type '{ name: string; }'.`); + expect(diags[0].start).toBe(199); + expect(diags[0].length).toBe(19); }); it('should accept an NgFor iteration over an any-typed value', () => { @@ -272,8 +274,9 @@ export declare class CommonModule { const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); - expect(diags[0].messageText).toContain('does_not_exist'); - expect(formatSpan(diags[0])).toEqual('/test.ts: 6:51, 6:70'); + expect(diags[0].messageText).toEqual(`Property 'does_not_exist' does not exist on type 'T'.`); + expect(diags[0].start).toBe(206); + expect(diags[0].length).toBe(19); }); it('should property type-check a microsyntax variable with the same name as the expression', @@ -333,52 +336,88 @@ export declare class CommonModule { const diags = env.driveDiagnostics(); expect(diags.length).toBe(2); - - // Error from the binding to [fromBase]. expect(diags[0].messageText) .toBe(`Type 'number' is not assignable to type 'string | undefined'.`); - expect(formatSpan(diags[0])).toEqual('/test.ts: 19:28, 19:42'); - - // Error from the binding to [fromChild]. + expect(diags[0].start).toEqual(386); + expect(diags[0].length).toEqual(14); expect(diags[1].messageText) .toBe(`Type 'number' is not assignable to type 'boolean | undefined'.`); - expect(formatSpan(diags[1])).toEqual('/test.ts: 19:43, 19:58'); + expect(diags[1].start).toEqual(401); + expect(diags[1].length).toEqual(15); }); - it('should report diagnostics for external template files', () => { - env.write('test.ts', ` - import {Component, NgModule} from '@angular/core'; + describe('error locations', () => { + it('should be correct for direct templates', () => { + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'test', + template: \`

+ {{user.does_not_exist}} +

\`, + }) + export class TestCmp { + user: {name: string}[]; + }`); - @Component({ - selector: 'test', - templateUrl: './template.html', - }) - export class TestCmp { - user: {name: string}[]; - } + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].file !.fileName).toBe(_('/test.ts')); + expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist'); + }); - @NgModule({ - declarations: [TestCmp], - }) - export class Module {} - `); - env.write('template.html', `
- {{user.does_not_exist}} -
`); + it('should be correct for indirect templates', () => { + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + + const TEMPLATE = \`

+ {{user.does_not_exist}} +

\`; - const diags = env.driveDiagnostics(); - expect(diags.length).toBe(1); - expect(diags[0].messageText).toContain('does_not_exist'); - expect(formatSpan(diags[0])).toEqual('/template.html: 1:14, 1:33'); + @Component({ + selector: 'test', + template: TEMPLATE, + }) + export class TestCmp { + user: {name: string}[]; + }`); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].file !.fileName).toBe(_('/test.ts') + ' (TestCmp template)'); + expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist'); + expect(getSourceCodeForDiagnostic(diags[0].relatedInformation ![0])).toBe('TEMPLATE'); + }); + + it('should be correct for external templates', () => { + env.write('template.html', `

+ {{user.does_not_exist}} +

`); + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + + + @Component({ + selector: 'test', + templateUrl: './template.html', + }) + export class TestCmp { + user: {name: string}[]; + }`); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].file !.fileName).toBe(_('/template.html')); + expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist'); + expect(getSourceCodeForDiagnostic(diags[0].relatedInformation ![0])) + .toBe(`'./template.html'`); + }); }); }); }); -function formatSpan(diagnostic: ts.Diagnostic | Diagnostic): string { - if (diagnostic.source !== 'angular') { - return ''; - } - const span = (diagnostic as Diagnostic).span !; - const fileName = span.start.file.url.replace(/^C:\//, '/'); - return `${fileName}: ${span.start.line}:${span.start.col}, ${span.end.line}:${span.end.col}`; +function getSourceCodeForDiagnostic(diag: ts.Diagnostic): string { + const text = diag.file !.text; + return text.substr(diag.start !, diag.length !); }