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 !); }