From 3a2b195a58d69685f315ed814c809bc41eee9b85 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 28 Apr 2019 00:26:13 +0200 Subject: [PATCH] feat(ivy): translate type-check diagnostics to their original source (#30181) PR Close #30181 --- .../src/ngtsc/annotations/src/component.ts | 8 +- packages/compiler-cli/src/ngtsc/program.ts | 2 +- .../src/ngtsc/typecheck/src/context.ts | 87 ++++-- .../src/ngtsc/typecheck/src/diagnostics.ts | 176 +++++++++++- .../src/ngtsc/typecheck/src/line_mappings.ts | 63 +++++ .../ngtsc/typecheck/src/type_check_block.ts | 4 +- .../ngtsc/typecheck/test/diagnostics_spec.ts | 257 ++++++++++++++++++ .../src/ngtsc/typecheck/test/test_utils.ts | 232 +++++++++++++--- .../typecheck/test/type_constructor_spec.ts | 11 +- .../src/ngtsc/util/src/typescript.ts | 7 +- packages/compiler-cli/src/perform_compile.ts | 4 +- packages/compiler-cli/test/ngtsc/env.ts | 6 +- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 3 +- .../compiler-cli/test/ngtsc/scope_spec.ts | 8 +- .../test/ngtsc/template_typecheck_spec.ts | 41 +++ packages/compiler/src/render3/view/t2_api.ts | 2 +- .../test/render3/view/binding_spec.ts | 2 +- 17 files changed, 823 insertions(+), 90 deletions(-) create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/src/line_mappings.ts create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index b59a226853..93d8a3ce30 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -34,7 +34,7 @@ const EMPTY_ARRAY: any[] = []; export interface ComponentHandlerData { meta: R3ComponentMetadata; - parsedTemplate: TmplAstNode[]; + parsedTemplate: {nodes: TmplAstNode[]; file: ParseSourceFile}; metadataStmt: Statement|null; parseTemplate: (options?: ParseTemplateOptions) => ParsedTemplate; } @@ -308,7 +308,7 @@ export class ComponentDecoratorHandler implements }, metadataStmt: generateSetClassMetadataCall( node, this.reflector, this.defaultImportRecorder, this.isCore), - parsedTemplate: template.nodes, parseTemplate, + parsedTemplate: template, parseTemplate, }, typeCheck: true, }; @@ -360,7 +360,7 @@ export class ComponentDecoratorHandler implements const extMeta = flattenInheritedDirectiveMetadata(this.metaReader, meta.ref); matcher.addSelectables(CssSelector.parse(meta.selector), extMeta); } - const bound = new R3TargetBinder(matcher).bind({template: meta.parsedTemplate}); + const bound = new R3TargetBinder(matcher).bind({template: meta.parsedTemplate.nodes}); const pipes = new Map>>(); for (const {name, ref} of scope.compilation.pipes) { if (!ts.isClassDeclaration(ref.node)) { @@ -369,7 +369,7 @@ export class ComponentDecoratorHandler implements } pipes.set(name, ref as Reference>); } - ctx.addTemplate(new Reference(node), bound, pipes); + ctx.addTemplate(new Reference(node), bound, pipes, meta.parsedTemplate.file); } } diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index cfcb171f38..fd1219422d 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -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/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index c640d5ec4d..6592958448 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {BoundTarget} from '@angular/compiler'; +import {BoundTarget, ParseLocation, ParseSourceFile, ParseSourceSpan} from '@angular/compiler'; import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../file_system'; @@ -15,8 +15,10 @@ import {ClassDeclaration} from '../../reflection'; import {ImportManager} from '../../translator'; import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from './api'; +import {Diagnostic, SourceLocation, getSourceReferenceName, shouldReportDiagnostic, translateDiagnostic} from './diagnostics'; import {Environment} from './environment'; import {TypeCheckProgramHost} from './host'; +import {computeLineStartsMap, getLineAndCharacterFromPosition} from './line_mappings'; import {generateTypeCheckBlock, requiresInlineTypeCheckBlock} from './type_check_block'; import {TypeCheckFile, typeCheckFilePath} from './type_check_file'; import {generateInlineTypeCtor, requiresInlineTypeCtor} from './type_constructor'; @@ -51,6 +53,13 @@ export class TypeCheckContext { */ private typeCtorPending = new Set(); + /** + * 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. + */ + private templateSources = new Map(); + /** * Record a template for the given component `node`, with a `SelectorMatcher` for directive * matching. @@ -62,7 +71,10 @@ export class TypeCheckContext { addTemplate( ref: Reference>, boundTarget: BoundTarget, - pipes: Map>>): void { + pipes: Map>>, + file: ParseSourceFile): void { + this.templateSources.set(getSourceReferenceName(ref.node), new TemplateSource(file)); + // Get all of the directives used in the template and record type constructors for all of them. for (const dir of boundTarget.getUsedDirectives()) { const dirRef = dir.ref as Reference>; @@ -149,7 +161,7 @@ export class TypeCheckContext { // the source code in between the original chunks. ops.forEach((op, idx) => { const text = op.execute(importManager, sf, this.refEmitter, printer); - code += text + textParts[idx + 1]; + code += '\n\n' + text + textParts[idx + 1]; }); // Write out the imports that need to be added to the beginning of the file. @@ -165,7 +177,7 @@ export class TypeCheckContext { calculateTemplateDiagnostics( originalProgram: ts.Program, originalHost: ts.CompilerHost, originalOptions: ts.CompilerOptions): { - diagnostics: ts.Diagnostic[], + diagnostics: Diagnostic[], program: ts.Program, } { const typeCheckSf = this.typeCheckFile.render(); @@ -189,26 +201,32 @@ export class TypeCheckContext { rootNames: originalProgram.getRootFileNames(), }); - const diagnostics: ts.Diagnostic[] = []; + 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 collectDiagnostics = (diags: readonly ts.Diagnostic[]): void => { + for (const diagnostic of diags) { + if (shouldReportDiagnostic(diagnostic)) { + const translated = translateDiagnostic(diagnostic, resolveSpan); + + if (translated !== null) { + diagnostics.push(translated); + } + } + } + }; + for (const sf of interestingFiles) { - diagnostics.push(...typeCheckProgram.getSemanticDiagnostics(sf)); + collectDiagnostics(typeCheckProgram.getSemanticDiagnostics(sf)); } return { - diagnostics: diagnostics.filter( - (diag: ts.Diagnostic): - boolean => { - if (diag.code === 6133 /* $var is declared but its value is never read. */) { - return false; - } else if (diag.code === 6199 /* All variables are unused. */) { - return false; - } else if ( - diag.code === - 2695 /* Left side of comma operator is unused and has no side effects. */) { - return false; - } - return true; - }), + diagnostics, program: typeCheckProgram, }; } @@ -225,6 +243,35 @@ 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. + */ +class TemplateSource { + private lineStarts: number[]|null = null; + + constructor(private file: ParseSourceFile) {} + + toParseSourceSpan(start: number, end: number): ParseSourceSpan { + const startLoc = this.toParseLocation(start); + const endLoc = this.toParseLocation(end); + return new ParseSourceSpan(startLoc, endLoc); + } + + private toParseLocation(position: number) { + const lineStarts = this.acquireLineStarts(); + const {line, character} = getLineAndCharacterFromPosition(lineStarts, position); + return new ParseLocation(this.file, position, line, character); + } + + private acquireLineStarts(): number[] { + if (this.lineStarts === null) { + this.lineStarts = computeLineStartsMap(this.file.content); + } + return this.lineStarts; + } +} + /** * A code generation operation that needs to happen within a given source file. */ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts index e28c774e2b..3ddb508486 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts @@ -5,11 +5,37 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ -import {ParseSourceSpan, ParseSpan} from '@angular/compiler'; +import {ParseSourceSpan, ParseSpan, Position} from '@angular/compiler'; import * as ts from 'typescript'; import {ClassDeclaration} from '../../reflection'; -import {getSourceFile} from '../../util/src/typescript'; +import {getSourceFile, 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'; +} + +export interface SourceLocation { + sourceReference: string; + start: number; + end: number; +} /** * An `AbsoluteSpan` is the result of translating the `ParseSpan` of `AST` template expression nodes @@ -51,24 +77,156 @@ export function wrapForDiagnostics(expr: ts.Expression): ts.Expression { */ export function addParseSpanInfo(node: ts.Node, span: AbsoluteSpan | ParseSourceSpan): void { let commentText: string; - if (typeof span.start === 'number') { + if (isAbsoluteSpan(span)) { commentText = `${span.start},${span.end}`; } else { - const {start, end} = span as ParseSourceSpan; - commentText = `${start.offset},${end.offset}`; + commentText = `${span.start.offset},${span.end.offset}`; } ts.addSyntheticTrailingComment( node, ts.SyntaxKind.MultiLineCommentTrivia, commentText, /* hasTrailingNewLine */ false); } +function isAbsoluteSpan(span: AbsoluteSpan | ParseSourceSpan): span is AbsoluteSpan { + return typeof span.start === 'number'; +} + /** * Adds a synthetic comment to the function declaration that contains the source location * of the class declaration. */ -export function addSourceInfo( - tcb: ts.FunctionDeclaration, source: ClassDeclaration): void { - const fileName = getSourceFile(source).fileName; - const commentText = `${fileName}#${source.name.text}`; +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}`; +} + +/** + * Determines if the diagnostic should be reported. Some diagnostics are produced because of the + * way TCBs are generated; those diagnostics should not be reported as type check errors of the + * template. + */ +export function shouldReportDiagnostic(diagnostic: ts.Diagnostic): boolean { + const {code} = diagnostic; + if (code === 6133 /* $var is declared but its value is never read. */) { + return false; + } else if (code === 6199 /* All variables are unused. */) { + return false; + } else if (code === 2695 /* Left side of comma operator is unused and has no side effects. */) { + return false; + } + return true; +} + +/** + * Attempts to translate a TypeScript diagnostic produced during template type-checking to their + * location of origin, based on the comments that are emitted in the TCB code. + * + * If the diagnostic could not be translated, `null` is returned to indicate that the diagnostic + * should not be reported at all. This prevents diagnostics from non-TCB code in a user's source + * file from being reported as type-check errors. + */ +export function translateDiagnostic( + diagnostic: ts.Diagnostic, resolveParseSource: (sourceLocation: SourceLocation) => + ParseSourceSpan | null): Diagnostic|null { + if (diagnostic.file === undefined || diagnostic.start === undefined) { + return null; + } + + // Locate the node that the diagnostic is reported on and determine its location in the source. + const node = getTokenAtPosition(diagnostic.file, diagnostic.start); + const sourceLocation = findSourceLocation(node, diagnostic.file); + if (sourceLocation === null) { + return null; + } + + // Now use the external resolver to obtain the full `ParseSourceFile` of the template. + const span = resolveParseSource(sourceLocation); + if (span === null) { + return null; + } + + let messageText: string; + if (typeof diagnostic.messageText === 'string') { + messageText = diagnostic.messageText; + } else { + messageText = diagnostic.messageText.messageText; + } + + return { + source: 'angular', + code: diagnostic.code, + category: diagnostic.category, messageText, span, + }; +} + +function findSourceLocation(node: ts.Node, sourceFile: ts.SourceFile): SourceLocation|null { + // Search for comments until the TCB's function declaration is encountered. + while (node !== undefined && !ts.isFunctionDeclaration(node)) { + const parseSpan = + ts.forEachTrailingCommentRange(sourceFile.text, node.getEnd(), (pos, end, kind) => { + if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { + return null; + } + const commentText = sourceFile.text.substring(pos, end); + return parseParseSpanComment(commentText); + }) || null; + if (parseSpan !== null) { + // Once the positional information has been extracted, search further up the TCB to extract + // the file information that is attached with the TCB's function declaration. + return toSourceLocation(parseSpan, node, sourceFile); + } + + node = node.parent; + } + + return null; +} + +function toSourceLocation( + parseSpan: ParseSpan, node: ts.Node, sourceFile: ts.SourceFile): SourceLocation|null { + // Walk up to the function declaration of the TCB, the file information is attached there. + let tcb = node; + while (!ts.isFunctionDeclaration(tcb)) { + tcb = tcb.parent; + + // Bail once we have reached the root. + if (tcb === undefined) { + return null; + } + } + + const sourceReference = + ts.forEachLeadingCommentRange(sourceFile.text, tcb.getFullStart(), (pos, end, kind) => { + if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { + return null; + } + const commentText = sourceFile.text.substring(pos, end); + return commentText.substring(2, commentText.length - 2); + }) || null; + if (sourceReference === null) { + return null; + } + + return { + sourceReference, + start: parseSpan.start, + end: parseSpan.end, + }; +} + +const parseSpanComment = /^\/\*(\d+),(\d+)\*\/$/; + +function parseParseSpanComment(commentText: string): ParseSpan|null { + const match = commentText.match(parseSpanComment); + if (match === null) { + return null; + } + + return {start: +match[1], end: +match[2]}; +} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/line_mappings.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/line_mappings.ts new file mode 100644 index 0000000000..8ff8f10cec --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/line_mappings.ts @@ -0,0 +1,63 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +const LF_CHAR = 10; +const CR_CHAR = 13; +const LINE_SEP_CHAR = 8232; +const PARAGRAPH_CHAR = 8233; + +/** Gets the line and character for the given position from the line starts map. */ +export function getLineAndCharacterFromPosition(lineStartsMap: number[], position: number) { + const lineIndex = findClosestLineStartPosition(lineStartsMap, position); + return {character: position - lineStartsMap[lineIndex], line: lineIndex}; +} + +/** + * Computes the line start map of the given text. This can be used in order to + * retrieve the line and character of a given text position index. + */ +export function computeLineStartsMap(text: string): number[] { + const result: number[] = [0]; + let pos = 0; + while (pos < text.length) { + const char = text.charCodeAt(pos++); + // Handles the "CRLF" line break. In that case we peek the character + // after the "CR" and check if it is a line feed. + if (char === CR_CHAR) { + if (text.charCodeAt(pos) === LF_CHAR) { + pos++; + } + result.push(pos); + } else if (char === LF_CHAR || char === LINE_SEP_CHAR || char === PARAGRAPH_CHAR) { + result.push(pos); + } + } + result.push(pos); + return result; +} + +/** Finds the closest line start for the given position. */ +function findClosestLineStartPosition( + linesMap: T[], position: T, low = 0, high = linesMap.length - 1) { + while (low <= high) { + const pivotIdx = Math.floor((low + high) / 2); + const pivotEl = linesMap[pivotIdx]; + + if (pivotEl === position) { + return pivotIdx; + } else if (position > pivotEl) { + low = pivotIdx + 1; + } else { + high = pivotIdx - 1; + } + } + + // In case there was no exact match, return the closest "lower" line index. We also + // subtract the index by one because want the index of the previous line start. + return low - 1; +} 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 70dd1f829a..41615b54cd 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,7 +13,7 @@ import {Reference} from '../../imports'; import {ClassDeclaration} from '../../reflection'; import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api'; -import {addParseSpanInfo, addSourceInfo, toAbsoluteSpan, wrapForDiagnostics} from './diagnostics'; +import {addParseSpanInfo, addSourceReferenceName, toAbsoluteSpan, wrapForDiagnostics} from './diagnostics'; import {Environment} from './environment'; import {astToTypescript} from './expression'; import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateVariable, tsDeclareVariable} from './ts_util'; @@ -60,7 +60,7 @@ export function generateTypeCheckBlock( /* parameters */ paramList, /* type */ undefined, /* body */ body); - addSourceInfo(fnDecl, ref.node); + addSourceReferenceName(fnDecl, ref.node); 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 new file mode 100644 index 0000000000..ac09f5f86f --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts @@ -0,0 +1,257 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {TestFile, runInEachFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; +import * as ts from 'typescript'; + +import {Diagnostic} from '../src/diagnostics'; + +import {NGFOR_DECLARATION, TestDeclaration, ngForDts, typecheck} from './test_utils'; + +runInEachFileSystem(() => { + describe('template diagnostics', () => { + it('works for directive bindings', () => { + const messages = diagnose( + `
`, ` + class Dir { + input: number; + } + class TestComponent { + person: { + name: string; + }; + }`, + [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + exportAs: ['dir'], + inputs: {input: 'input'}, + }]); + + expect(messages).toEqual( + [`synthetic.html(9, 30): Type 'string' is not assignable to type 'number | undefined'.`]); + }); + + it('infers type of template variables', () => { + const messages = diagnose( + `
{{ render(idx) }}
`, ` + class TestComponent { + persons: {}[]; + + render(input: string): string { return input; } + }`, + [NGFOR_DECLARATION], [ngForDts()]); + + expect(messages).toEqual([ + `synthetic.html(61, 64): Argument of type 'number' is not assignable to parameter of type 'string'.`, + ]); + }); + + it('infers any type when generic type inference fails', () => { + const messages = diagnose( + `
{{ render(person.namme) }}
`, ` + class TestComponent { + persons: any; + + render(input: string): string { return input; } + }`, + [NGFOR_DECLARATION], [ngForDts()]); + + expect(messages).toEqual([]); + }); + + it('infers type of element references', () => { + const messages = diagnose( + `
{{ render(el) }}
`, ` + class Dir { + value: number; + } + class TestComponent { + render(input: string): string { return input; } + }`, + [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + exportAs: ['dir'], + }]); + + expect(messages).toEqual([ + `synthetic.html(23, 25): Argument of type 'HTMLDivElement' is not assignable to parameter of type 'string'.`, + ]); + }); + + it('infers type of directive references', () => { + const messages = diagnose( + `
{{ render(dir) }}
`, ` + class Dir { + value: number; + } + class TestComponent { + render(input: string): string { return input; } + }`, + [{ + type: 'directive', + name: 'Dir', + selector: '[dir]', + exportAs: ['dir'], + }]); + + expect(messages).toEqual([ + `synthetic.html(30, 33): Argument of type 'Dir' is not assignable to parameter of type 'string'.`, + ]); + }); + + it('infers TemplateRef for ng-template references', () => { + const messages = diagnose(`{{ render(tmpl) }}`, ` + class TestComponent { + render(input: string): string { return input; } + }`); + + expect(messages).toEqual([ + `synthetic.html(29, 33): Argument of type 'TemplateRef' is not assignable to parameter of type 'string'.`, + ]); + }); + + it('infers type of template context', () => { + const messages = diagnose( + `
{{ person.namme }}
`, ` + class TestComponent { + persons: { + name: string; + }[]; + }`, + [NGFOR_DECLARATION], [ngForDts()]); + + expect(messages).toEqual([ + `synthetic.html(39, 52): Property 'namme' does not exist on type '{ name: string; }'. Did you mean 'name'?`, + ]); + }); + + it('interprets interpolation as strings', () => { + const messages = diagnose(`
`, ` + class TestComponent { + person: {}; + }`); + + expect(messages).toEqual([]); + }); + + it('checks bindings to regular element', () => { + const messages = diagnose(``, ` + class TestComponent { + src: string; + height: number; + }`); + + 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'?`, + ]); + }); + + it('produces diagnostics for pipes', () => { + const messages = diagnose( + `
{{ person.name | pipe:person.age:1 }}
`, ` + class Pipe { + transform(value: string, a: string, b: string): string { return a + b; } + } + class TestComponent { + person: { + name: string; + age: number; + }; + }`, + [{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'.`, + ]); + }); + + it('does not produce diagnostics for user code', () => { + const messages = diagnose(`{{ person.name }}`, ` + class TestComponent { + person: { + name: string; + }; + render(input: string): number { return input; } // <-- type error here should not be reported + }`); + + expect(messages).toEqual([]); + }); + + describe('strict null checks', () => { + it('produces diagnostic for unchecked property access', () => { + const messages = + diagnose(`
`, ` + export class TestComponent { + person: { + address?: { + street: string; + }; + }; + }`); + + expect(messages).toEqual([`synthetic.html(25, 46): Object is possibly 'undefined'.`]); + }); + + it('does not produce diagnostic for checked property access', () => { + const messages = diagnose( + `
`, ` + export class TestComponent { + person: { + address?: { + street: string; + }; + }; + }`); + + expect(messages).toEqual([]); + }); + }); + + it('computes line and column offsets', () => { + const diagnostics = typecheck( + ` +
+ +
+`, + ` +class TestComponent { + src: string; + height: number; +}`); + + expect(diagnostics.length).toBe(2); + expect(formatSpan(diagnostics[0])).toBe('2:14, 2:18'); + expect(formatSpan(diagnostics[1])).toBe('3:17, 3:23'); + }); + }); +}); + +function diagnose( + template: string, source: string, declarations?: TestDeclaration[], + additionalSources: TestFile[] = []): string[] { + const diagnostics = typecheck(template, source, declarations, additionalSources); + return diagnostics.map(diagnostic => { + const span = diagnostic.span !; + return `${span.start.file.url}(${span.start.offset}, ${span.end.offset}): ${diagnostic.messageText}`; + }); +} + +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 3ab0df0c4e..5b142bf72f 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -6,21 +6,128 @@ * found in the LICENSE file at https://angular.io/license */ -import {CssSelector, R3TargetBinder, SelectorMatcher, parseTemplate} from '@angular/compiler'; +import {CssSelector, ParseSourceFile, R3TargetBinder, SelectorMatcher, parseTemplate} from '@angular/compiler'; import * as ts from 'typescript'; -import {Reference} from '../../imports'; -import {ClassDeclaration, isNamedClassDeclaration} from '../../reflection'; +import {AbsoluteFsPath, LogicalFileSystem, absoluteFrom} from '../../file_system'; +import {TestFile} from '../../file_system/testing'; +import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, Reference, ReferenceEmitter} from '../../imports'; +import {ClassDeclaration, TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection'; +import {makeProgram} from '../../testing'; +import {getRootDirs} from '../../util/src/typescript'; import {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'; +export function typescriptLibDts(): TestFile { + return { + name: absoluteFrom('/lib.d.ts'), + contents: ` + type Partial = { [P in keyof T]?: T[P]; }; + type Pick = { [P in K]: T[P]; }; + type NonNullable = T extends null | undefined ? never : T; + + // The following native type declarations are required for proper type inference + declare interface Function { + call(...args: any[]): any; + } + declare interface Array { + length: number; + } + declare interface String { + length: number; + } + + declare interface HTMLElement {} + declare interface HTMLDivElement extends HTMLElement {} + declare interface HTMLImageElement extends HTMLElement { + src: string; + alt: string; + width: number; + height: number; + } + declare interface HTMLQuoteElement extends HTMLElement { + cite: string; + } + declare interface HTMLElementTagNameMap { + "blockquote": HTMLQuoteElement; + "div": HTMLDivElement; + "img": HTMLImageElement; + } + declare interface Document { + createElement(tagName: K): HTMLElementTagNameMap[K]; + createElement(tagName: string): HTMLElement; + } + declare const document: Document; + ` + }; +} + +export function angularCoreDts(): TestFile { + return { + name: absoluteFrom('/node_modules/@angular/core/index.d.ts'), + contents: ` + export declare class TemplateRef { + abstract readonly elementRef: unknown; + abstract createEmbeddedView(context: C): unknown; + } + ` + }; +} + +export const NGFOR_DECLARATION: TestDeclaration = { + type: 'directive', + file: 'ngfor.d.ts', + selector: '[ngForOf]', + name: 'NgForOf', + inputs: {ngForOf: 'ngForOf'}, + hasNgTemplateContextGuard: true, +}; + +export function ngForDts(): TestFile { + return { + name: absoluteFrom('/ngfor.d.ts'), + contents: ` + export declare class NgForOf { + ngForOf: T[]; + ngForTrackBy: TrackByFunction; + static ngTemplateContextGuard(dir: NgForOf, ctx: any): ctx is NgForOfContext; + } + + export interface TrackByFunction { + (index: number, item: T): any; + } + + export declare class NgForOfContext { + $implicit: T; + index: number; + count: number; + readonly odd: boolean; + readonly even: boolean; + readonly first: boolean; + readonly last: boolean; + }`, + }; +} + +export const ALL_ENABLED_CONFIG: TypeCheckingConfig = { + applyTemplateContextGuards: true, + checkQueries: false, + checkTemplateBodies: true, + checkTypeOfBindings: true, + checkTypeOfPipes: true, + strictSafeNavigationTypes: true, +}; + // Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead. export type TestDirective = Partial>>& - {selector: string, name: string, type: 'directive'}; + {selector: string, name: string, file?: string, type: 'directive'}; export type TestPipe = { name: string, + file?: string, pipeName: string, type: 'pipe', }; @@ -35,38 +142,13 @@ export function tcb( const sf = ts.createSourceFile('synthetic.ts', code, ts.ScriptTarget.Latest, true); const clazz = getClass(sf, 'Test'); - const {nodes} = parseTemplate(template, 'synthetic.html'); - const matcher = new SelectorMatcher(); - - for (const decl of declarations) { - if (decl.type !== 'directive') { - continue; - } - const selector = CssSelector.parse(decl.selector); - const meta: TypeCheckableDirectiveMeta = { - name: decl.name, - ref: new Reference(getClass(sf, decl.name)), - exportAs: decl.exportAs || null, - hasNgTemplateContextGuard: decl.hasNgTemplateContextGuard || false, - inputs: decl.inputs || {}, - isComponent: decl.isComponent || false, - ngTemplateGuards: decl.ngTemplateGuards || [], - outputs: decl.outputs || {}, - queries: decl.queries || [], - }; - matcher.addSelectables(selector, meta); - } + const templateUrl = 'synthetic.html'; + const {nodes} = parseTemplate(template, templateUrl); + const {matcher, pipes} = prepareDeclarations(declarations, decl => getClass(sf, decl.name)); const binder = new R3TargetBinder(matcher); const boundTarget = binder.bind({template: nodes}); - const pipes = new Map>>(); - for (const decl of declarations) { - if (decl.type === 'pipe') { - pipes.set(decl.pipeName, new Reference(getClass(sf, decl.name))); - } - } - const meta: TypeCheckBlockMetadata = {boundTarget, pipes}; config = config || { @@ -89,7 +171,91 @@ export function tcb( return res.replace(/\s+/g, ' '); } -function getClass(sf: ts.SourceFile, name: string): ClassDeclaration { +export function typecheck( + template: string, source: string, declarations: TestDeclaration[] = [], + additionalSources: {name: AbsoluteFsPath; contents: string}[] = []): Diagnostic[] { + const typeCheckFilePath = absoluteFrom('/_typecheck_.ts'); + const files = [ + typescriptLibDts(), + angularCoreDts(), + // Add the typecheck file to the program, as the typecheck program is created with the + // assumption that the typecheck file was already a root file in the original program. + {name: typeCheckFilePath, contents: 'export const TYPECHECK = true;'}, + {name: absoluteFrom('/main.ts'), contents: source}, + ...additionalSources, + ]; + const {program, host, options} = makeProgram(files, {strictNullChecks: true}, undefined, false); + const sf = program.getSourceFile('main.ts') !; + const checker = program.getTypeChecker(); + const logicalFs = new LogicalFileSystem(getRootDirs(host, options)); + const emitter = new ReferenceEmitter([ + new LocalIdentifierStrategy(), + new AbsoluteModuleStrategy( + program, checker, options, host, new TypeScriptReflectionHost(checker)), + new LogicalProjectStrategy(checker, logicalFs), + ]); + const ctx = new TypeCheckContext(ALL_ENABLED_CONFIG, emitter, typeCheckFilePath); + + const templateUrl = 'synthetic.html'; + const templateFile = new ParseSourceFile(template, templateUrl); + const {nodes, errors} = parseTemplate(template, templateUrl); + if (errors !== undefined) { + throw new Error('Template parse errors: \n' + errors.join('\n')); + } + + const {matcher, pipes} = prepareDeclarations(declarations, decl => { + let declFile = sf; + if (decl.file !== undefined) { + declFile = program.getSourceFile(decl.file) !; + if (declFile === undefined) { + throw new Error(`Unable to locate ${decl.file} for ${decl.type} ${decl.name}`); + } + } + return getClass(declFile, decl.name); + }); + const binder = new R3TargetBinder(matcher); + const boundTarget = binder.bind({template: nodes}); + const clazz = new Reference(getClass(sf, 'TestComponent')); + + ctx.addTemplate(clazz, boundTarget, pipes, templateFile); + return ctx.calculateTemplateDiagnostics(program, host, options).diagnostics; +} + +function prepareDeclarations( + declarations: TestDeclaration[], + resolveDeclaration: (decl: TestDeclaration) => ClassDeclaration) { + const matcher = new SelectorMatcher(); + for (const decl of declarations) { + if (decl.type !== 'directive') { + continue; + } + + const selector = CssSelector.parse(decl.selector); + const meta: TypeCheckableDirectiveMeta = { + name: decl.name, + ref: new Reference(resolveDeclaration(decl)), + exportAs: decl.exportAs || null, + hasNgTemplateContextGuard: decl.hasNgTemplateContextGuard || false, + inputs: decl.inputs || {}, + isComponent: decl.isComponent || false, + ngTemplateGuards: decl.ngTemplateGuards || [], + outputs: decl.outputs || {}, + queries: decl.queries || [], + }; + matcher.addSelectables(selector, meta); + } + + const pipes = new Map>>(); + for (const decl of declarations) { + if (decl.type === 'pipe') { + pipes.set(decl.pipeName, new Reference(resolveDeclaration(decl))); + } + } + + return {matcher, pipes}; +} + +export function getClass(sf: ts.SourceFile, name: string): ClassDeclaration { for (const stmt of sf.statements) { if (isNamedClassDeclaration(stmt) && stmt.name.text === name) { return stmt; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts index 3b631bb92c..36744e9b42 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts @@ -12,17 +12,8 @@ import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, import {TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection'; import {getDeclaration, makeProgram} from '../../testing'; import {getRootDirs} from '../../util/src/typescript'; -import {TypeCheckingConfig} from '../src/api'; import {TypeCheckContext} from '../src/context'; - -const ALL_ENABLED_CONFIG: TypeCheckingConfig = { - applyTemplateContextGuards: true, - checkQueries: false, - checkTemplateBodies: true, - checkTypeOfBindings: true, - checkTypeOfPipes: true, - strictSafeNavigationTypes: true, -}; +import {ALL_ENABLED_CONFIG} from './test_utils'; runInEachFileSystem(() => { describe('ngtsc typechecking', () => { diff --git a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts index 58f89ac34b..63b3686f0b 100644 --- a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts @@ -53,6 +53,11 @@ export function getSourceFileOrNull(program: ts.Program, fileName: AbsoluteFsPat } +export function getTokenAtPosition(sf: ts.SourceFile, pos: number): ts.Node { + // getTokenAtPosition is part of TypeScript's private API. + return (ts as any).getTokenAtPosition(sf, pos); +} + export function identifierOfNode(decl: ts.Node & {name?: ts.Node}): ts.Identifier|null { if (decl.name !== undefined && ts.isIdentifier(decl.name)) { return decl.name; @@ -113,4 +118,4 @@ export function resolveModuleName( return ts.resolveModuleName(moduleName, containingFile, compilerOptions, compilerHost) .resolvedModule; } -} \ No newline at end of file +} diff --git a/packages/compiler-cli/src/perform_compile.ts b/packages/compiler-cli/src/perform_compile.ts index f7ea7bbb59..57ba6bf78b 100644 --- a/packages/compiler-cli/src/perform_compile.ts +++ b/packages/compiler-cli/src/perform_compile.ts @@ -72,11 +72,11 @@ export function formatDiagnostic( result += `${formatDiagnosticPosition(diagnostic.position, host)}: `; } if (diagnostic.span && diagnostic.span.details) { - result += `: ${diagnostic.span.details}, ${diagnostic.messageText}${newLine}`; + result += `${diagnostic.span.details}, ${diagnostic.messageText}${newLine}`; } else if (diagnostic.chain) { result += `${flattenDiagnosticMessageChain(diagnostic.chain, host)}.${newLine}`; } else { - result += `: ${diagnostic.messageText}${newLine}`; + result += `${diagnostic.messageText}${newLine}`; } return result; } diff --git a/packages/compiler-cli/test/ngtsc/env.ts b/packages/compiler-cli/test/ngtsc/env.ts index 1f1639688d..51dab03618 100644 --- a/packages/compiler-cli/test/ngtsc/env.ts +++ b/packages/compiler-cli/test/ngtsc/env.ts @@ -7,6 +7,7 @@ */ import {CustomTransformers, Program} from '@angular/compiler-cli'; +import * as api from '@angular/compiler-cli/src/transformers/api'; import * as ts from 'typescript'; import {createCompilerHost, createProgram} from '../../ngtools2'; @@ -185,9 +186,8 @@ export class NgtscTestEnvironment { /** * Run the compiler to completion, and return any `ts.Diagnostic` errors that may have occurred. */ - driveDiagnostics(): ReadonlyArray { - // Cast is safe as ngtsc mode only produces ts.Diagnostics. - return mainDiagnosticsForTest(['-p', this.basePath]) as ReadonlyArray; + driveDiagnostics(): ReadonlyArray { + return mainDiagnosticsForTest(['-p', this.basePath]); } driveRoutes(entryPoint?: string): LazyRoute[] { diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 7ef050c285..20acac08a1 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -2979,7 +2979,8 @@ runInEachFileSystem(os => { 'entrypoint.'); // Verify that the error is for the correct class. - const id = expectTokenAtPosition(errors[0].file !, errors[0].start !, ts.isIdentifier); + const error = errors[0] as ts.Diagnostic; + const id = expectTokenAtPosition(error.file !, error.start !, ts.isIdentifier); expect(id.text).toBe('Dir'); expect(ts.isClassDeclaration(id.parent)).toBe(true); }); diff --git a/packages/compiler-cli/test/ngtsc/scope_spec.ts b/packages/compiler-cli/test/ngtsc/scope_spec.ts index b4b0edc454..7b47e4cc11 100644 --- a/packages/compiler-cli/test/ngtsc/scope_spec.ts +++ b/packages/compiler-cli/test/ngtsc/scope_spec.ts @@ -5,10 +5,13 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ + +import {Diagnostic} from '@angular/compiler-cli'; import * as ts from 'typescript'; import {ErrorCode, ngErrorCode} from '../../src/ngtsc/diagnostics'; import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; +import {getTokenAtPosition} from '../../src/ngtsc/util/src/typescript'; import {loadStandardTestFiles} from '../helpers/src/mock_file_loading'; import {NgtscTestEnvironment} from './env'; @@ -179,11 +182,12 @@ runInEachFileSystem(() => { }); function diagnosticToNode( - diag: ts.Diagnostic, guard: (node: ts.Node) => node is T): T { + diagnostic: ts.Diagnostic | Diagnostic, guard: (node: ts.Node) => node is T): T { + const diag = diagnostic as ts.Diagnostic; if (diag.file === undefined) { throw new Error(`Expected ts.Diagnostic to have a file source`); } - const node = (ts as any).getTokenAtPosition(diag.file, diag.start) as ts.Node; + const node = getTokenAtPosition(diag.file, diag.start !); expect(guard(node)).toBe(true); return node as T; } diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 705df88ff6..b98da28b22 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ +import {Diagnostic} from '@angular/compiler-cli'; import * as ts from 'typescript'; import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; @@ -171,6 +172,7 @@ 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'); }); it('should accept an NgFor iteration over an any-typed value', () => { @@ -271,6 +273,7 @@ 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'); }); it('should property type-check a microsyntax variable with the same name as the expression', @@ -334,10 +337,48 @@ export declare class CommonModule { // 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[1].messageText) .toBe(`Type 'number' is not assignable to type 'boolean | undefined'.`); + expect(formatSpan(diags[1])).toEqual('/test.ts: 19:43, 19:58'); + }); + + it('should report diagnostics for external template files', () => { + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'test', + templateUrl: './template.html', + }) + export class TestCmp { + user: {name: string}[]; + } + + @NgModule({ + declarations: [TestCmp], + }) + export class Module {} + `); + env.write('template.html', `
+ {{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'); }); }); }); + +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}`; +} diff --git a/packages/compiler/src/render3/view/t2_api.ts b/packages/compiler/src/render3/view/t2_api.ts index 8b5990e52d..ed651075b8 100644 --- a/packages/compiler/src/render3/view/t2_api.ts +++ b/packages/compiler/src/render3/view/t2_api.ts @@ -7,9 +7,9 @@ */ import {AST} from '../../expression_parser/ast'; - import {BoundAttribute, BoundEvent, Element, Node, Reference, Template, TextAttribute, Variable} from '../r3_ast'; + /* * t2 is the replacement for the `TemplateDefinitionBuilder`. It handles the operations of * analyzing Angular templates, extracting semantic info, and ultimately producing a template diff --git a/packages/compiler/test/render3/view/binding_spec.ts b/packages/compiler/test/render3/view/binding_spec.ts index 946bac59df..b7e6fcd310 100644 --- a/packages/compiler/test/render3/view/binding_spec.ts +++ b/packages/compiler/test/render3/view/binding_spec.ts @@ -78,4 +78,4 @@ describe('t2 binding', () => { expect(elDirectives.length).toBe(1); expect(elDirectives[0].name).toBe('Dir'); }); -}); \ No newline at end of file +});