From 230255f887edff43d478f22e422546d50e666f50 Mon Sep 17 00:00:00 2001 From: Chuck Jazdzewski Date: Thu, 1 Jun 2017 11:13:50 -0600 Subject: [PATCH] feat(compiler-cli): produce template diagnostics error messages (#17125) Refactoring the compiler to use transformers moves the code generation after type-checking which suppresses the errors TypeScript would generate in the user code. `TypeChecker` currently produces the same factory code that was generated prior the switch to transfomers, getting back the same diagnostics as before. The refactoring will allow the code to diverge from the factory code and allow better diagnostic error messages than was previously possible by type-checking the factories. --- .../src/diagnostics/check_types.ts | 225 ++++++++++++++++++ .../test/diagnostics/check_types_spec.ts | 140 +++++++++++ packages/compiler/src/compiler.ts | 1 + .../compiler/src/output/abstract_emitter.ts | 18 ++ packages/compiler/src/output/ts_emitter.ts | 19 +- packages/compiler/test/aot/test_util.ts | 2 +- 6 files changed, 398 insertions(+), 7 deletions(-) create mode 100644 packages/compiler-cli/src/diagnostics/check_types.ts create mode 100644 packages/compiler-cli/test/diagnostics/check_types_spec.ts diff --git a/packages/compiler-cli/src/diagnostics/check_types.ts b/packages/compiler-cli/src/diagnostics/check_types.ts new file mode 100644 index 0000000000..b7eed5599b --- /dev/null +++ b/packages/compiler-cli/src/diagnostics/check_types.ts @@ -0,0 +1,225 @@ +/** + * @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 {AotCompiler, AotCompilerHost, AotCompilerOptions, EmitterVisitorContext, GeneratedFile, NgAnalyzedModules, ParseSourceSpan, Statement, StaticReflector, TypeScriptEmitter, createAotCompiler} from '@angular/compiler'; +import * as ts from 'typescript'; + +interface FactoryInfo { + source: ts.SourceFile; + context: EmitterVisitorContext; +} + +type FactoryInfoMap = Map; + +export enum DiagnosticKind { + Message, + Warning, + Error, +} + +export interface Diagnostic { + message: string; + kind: DiagnosticKind; + span: ParseSourceSpan; +} + +export class TypeChecker { + private _aotCompiler: AotCompiler|undefined; + private _reflector: StaticReflector|undefined; + private _factories: Map|undefined; + private _factoryNames: string[]|undefined; + private _diagnosticProgram: ts.Program|undefined; + private _diagnosticsByFile: Map|undefined; + + constructor( + private program: ts.Program, private tsOptions: ts.CompilerOptions, + private compilerHost: ts.CompilerHost, private aotCompilerHost: AotCompilerHost, + private aotOptions: AotCompilerOptions, private _analyzedModules?: NgAnalyzedModules, + private _generatedFiles?: GeneratedFile[]) {} + + getDiagnostics(fileName?: string): Diagnostic[] { + return fileName ? + this.diagnosticsByFileName.get(fileName) || [] : + ([] as Diagnostic[]).concat(...Array.from(this.diagnosticsByFileName.values())); + } + + private get analyzedModules(): NgAnalyzedModules { + return this._analyzedModules || (this._analyzedModules = this.aotCompiler.analyzeModulesSync( + this.program.getSourceFiles().map(sf => sf.fileName))); + } + + private get diagnosticsByFileName(): Map { + return this._diagnosticsByFile || this.createDiagnosticsByFile(); + } + + private get diagnosticProgram(): ts.Program { + return this._diagnosticProgram || this.createDiagnosticProgram(); + } + + private get generatedFiles(): GeneratedFile[] { + let result = this._generatedFiles; + if (!result) { + this._generatedFiles = result = this.aotCompiler.emitAllImpls(this.analyzedModules); + } + return result; + } + + private get aotCompiler(): AotCompiler { + return this._aotCompiler || this.createCompilerAndReflector(); + } + + private get reflector(): StaticReflector { + let result = this._reflector; + if (!result) { + this.createCompilerAndReflector(); + result = this._reflector !; + } + return result; + } + + private get factories(): Map { + return this._factories || this.createFactories(); + } + + private get factoryNames(): string[] { + return this._factoryNames || (this.createFactories() && this._factoryNames !); + } + + private createCompilerAndReflector() { + const {compiler, reflector} = createAotCompiler(this.aotCompilerHost, this.aotOptions); + this._reflector = reflector; + return this._aotCompiler = compiler; + } + + private createDiagnosticProgram() { + // Create a program that is all the files from the original program plus the factories. + const existingFiles = this.program.getSourceFiles().map(source => source.fileName); + const host = new TypeCheckingHost(this.compilerHost, this.program, this.factories); + return this._diagnosticProgram = + ts.createProgram([...existingFiles, ...this.factoryNames], this.tsOptions, host); + } + + private createFactories() { + // Create all the factory files with enough information to map the diagnostics reported for the + // created file back to the original source. + const emitter = new TypeScriptEmitter(); + const factorySources = + this.generatedFiles.filter(file => file.stmts != null && file.stmts.length) + .map<[string, FactoryInfo]>( + file => [file.genFileUrl, createFactoryInfo(emitter, file)]); + this._factories = new Map(factorySources); + this._factoryNames = Array.from(this._factories.keys()); + return this._factories; + } + + private createDiagnosticsByFile() { + // Collect all the diagnostics binned by original source file name. + const result = new Map(); + const diagnosticsFor = (fileName: string) => { + let r = result.get(fileName); + if (!r) { + r = []; + result.set(fileName, r); + } + return r; + }; + const program = this.diagnosticProgram; + for (const factoryName of this.factoryNames) { + const sourceFile = program.getSourceFile(factoryName); + for (const diagnostic of this.diagnosticProgram.getSemanticDiagnostics(sourceFile)) { + const span = this.sourceSpanOf(diagnostic.file, diagnostic.start, diagnostic.length); + if (span) { + const fileName = span.start.file.url; + const diagnosticsList = diagnosticsFor(fileName); + diagnosticsList.push({ + message: diagnosticMessageToString(diagnostic.messageText), + kind: diagnosticKindConverter(diagnostic.category), span + }); + } + } + } + return result; + } + + private sourceSpanOf(source: ts.SourceFile, start: number, length: number): ParseSourceSpan|null { + // Find the corresponding TypeScript node + const info = this.factories.get(source.fileName); + if (info) { + const {line, character} = ts.getLineAndCharacterOfPosition(source, start); + return info.context.spanOf(line, character); + } + return null; + } +} + +function diagnosticMessageToString(message: ts.DiagnosticMessageChain | string): string { + return typeof message === 'string' ? message : diagnosticMessageToString(message.messageText); +} + +function diagnosticKindConverter(kind: ts.DiagnosticCategory) { + // The diagnostics kind matches ts.DiagnosticCategory. Review this code if this changes. + return kind as any as DiagnosticKind; +} + +function createFactoryInfo(emitter: TypeScriptEmitter, file: GeneratedFile): FactoryInfo { + const {sourceText, context} = + emitter.emitStatementsAndContext(file.srcFileUrl, file.genFileUrl, file.stmts !); + const source = ts.createSourceFile( + file.genFileUrl, sourceText, ts.ScriptTarget.Latest, /* setParentNodes */ true); + return {source, context}; +} + +class TypeCheckingHost implements ts.CompilerHost { + constructor( + private host: ts.CompilerHost, private originalProgram: ts.Program, + private factories: Map) {} + + getSourceFile( + fileName: string, languageVersion: ts.ScriptTarget, + onError?: ((message: string) => void)): ts.SourceFile { + const originalSource = this.originalProgram.getSourceFile(fileName); + if (originalSource) { + return originalSource; + } + const factoryInfo = this.factories.get(fileName); + if (factoryInfo) { + return factoryInfo.source; + } + return this.host.getSourceFile(fileName, languageVersion, onError); + } + + getDefaultLibFileName(options: ts.CompilerOptions): string { + return this.host.getDefaultLibFileName(options); + } + + writeFile: ts.WriteFileCallback = + () => { throw new Error('Unexpected write in diagnostic program'); } + + getCurrentDirectory(): string { + return this.host.getCurrentDirectory(); + } + + getDirectories(path: string): string[] { return this.host.getDirectories(path); } + + getCanonicalFileName(fileName: string): string { + return this.host.getCanonicalFileName(fileName); + } + + useCaseSensitiveFileNames(): boolean { return this.host.useCaseSensitiveFileNames(); } + + getNewLine(): string { return this.host.getNewLine(); } + + fileExists(fileName: string): boolean { + return this.factories.has(fileName) || this.host.fileExists(fileName); + } + + readFile(fileName: string): string { + const factoryInfo = this.factories.get(fileName); + return (factoryInfo && factoryInfo.source.text) || this.host.readFile(fileName); + } +} diff --git a/packages/compiler-cli/test/diagnostics/check_types_spec.ts b/packages/compiler-cli/test/diagnostics/check_types_spec.ts new file mode 100644 index 0000000000..6ad4e3f8bc --- /dev/null +++ b/packages/compiler-cli/test/diagnostics/check_types_spec.ts @@ -0,0 +1,140 @@ +/** + * @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 {AotCompilerOptions, createAotCompiler} from '@angular/compiler'; +import {EmittingCompilerHost, MockAotCompilerHost, MockCompilerHost, MockData, MockDirectory, MockMetadataBundlerHost, arrayToMockDir, arrayToMockMap, isSource, settings, setup, toMockFileArray} from '@angular/compiler/test/aot/test_util'; +import * as ts from 'typescript'; + +import {Diagnostic, TypeChecker} from '../../src/diagnostics/check_types'; + +function compile( + rootDirs: MockData, options: AotCompilerOptions = {}, + tsOptions: ts.CompilerOptions = {}): Diagnostic[] { + const rootDirArr = toMockFileArray(rootDirs); + const scriptNames = rootDirArr.map(entry => entry.fileName).filter(isSource); + const host = new MockCompilerHost(scriptNames, arrayToMockDir(rootDirArr)); + const aotHost = new MockAotCompilerHost(host); + const tsSettings = {...settings, ...tsOptions}; + const program = ts.createProgram(host.scriptNames.slice(0), tsSettings, host); + const ngChecker = new TypeChecker(program, tsSettings, host, aotHost, options); + return ngChecker.getDiagnostics(); +} + +fdescribe('ng type checker', () => { + let angularFiles = setup(); + + function accept(...files: MockDirectory[]) { + expectNoDiagnostics(compile([angularFiles, QUICKSTART, ...files])); + } + + function reject(message: string | RegExp, ...files: MockDirectory[]) { + const diagnostics = compile([angularFiles, QUICKSTART, ...files]); + if (!diagnostics || !diagnostics.length) { + throw new Error('Expected a diagnostic erorr message'); + } else { + const matches: (d: Diagnostic) => boolean = + typeof message === 'string' ? d => d.message == message : d => message.test(d.message); + const matchingDiagnostics = diagnostics.filter(matches); + if (!matchingDiagnostics || !matchingDiagnostics.length) { + throw new Error( + `Expected a diagnostics matching ${message}, received\n ${diagnostics.map(d => d.message).join('\n ')}`); + } + } + } + + it('should accept unmodified QuickStart', () => { accept(); }); + + describe('with modified quickstart', () => { + function a(template: string) { + accept({quickstart: {app: {'app.component.ts': appComponentSource(template)}}}); + } + + function r(template: string, message: string | RegExp) { + reject(message, {quickstart: {app: {'app.component.ts': appComponentSource(template)}}}); + } + + it('should report an invalid field access', + () => { r('{{fame}}', `Property 'fame' does not exist on type 'AppComponent'.`); }); + it('should reject a reference to a field of a nullable', + () => { r('{{maybePerson.name}}', `Object is possibly 'undefined'.`); }); + it('should accept a reference to a field of a nullable using using non-null-assert', + () => { a('{{maybePerson!.name}}'); }); + it('should accept a safe property access of a nullable person', + () => { a('{{maybePerson?.name}}'); }); + it('should accept a function call', () => { a('{{getName()}}'); }); + it('should reject an invalid method', + () => { r('{{getFame()}}', `Property 'getFame' does not exist on type 'AppComponent'.`); }); + it('should accept a field access of a method result', () => { a('{{getPerson().name}}'); }); + it('should reject an invalid field reference of a method result', + () => { r('{{getPerson().fame}}', `Property 'fame' does not exist on type 'Person'.`); }); + it('should reject an access to a nullable field of a method result', + () => { r('{{getMaybePerson().name}}', `Object is possibly 'undefined'.`); }); + it('should accept a nullable assert of a nullable field refernces of a method result', + () => { a('{{getMaybePerson()!.name}}'); }); + it('should accept a safe property access of a nullable field reference of a method result', + () => { a('{{getMaybePerson()?.name}}'); }); + }); +}); + +function appComponentSource(template: string): string { + return ` + import {Component} from '@angular/core'; + + export interface Person { + name: string; + address: Address; + } + + export interface Address { + street: string; + city: string; + state: string; + zip: string; + } + + @Component({ + template: '${template}' + }) + export class AppComponent { + name = 'Angular'; + person: Person; + people: Person[]; + maybePerson?: Person; + + getName(): string { return this.name; } + getPerson(): Person { return this.person; } + getMaybePerson(): Person | undefined { this.maybePerson; } + } + `; +} + +const QUICKSTART: MockDirectory = { + quickstart: { + app: { + 'app.component.ts': appComponentSource('

Hello {{name}}

'), + 'app.module.ts': ` + import { NgModule } from '@angular/core'; + import { toString } from './utils'; + + import { AppComponent } from './app.component'; + + @NgModule({ + declarations: [ AppComponent ], + bootstrap: [ AppComponent ] + }) + export class AppModule { } + ` + } + } +}; + +function expectNoDiagnostics(diagnostics: Diagnostic[]) { + if (diagnostics && diagnostics.length) { + throw new Error(diagnostics.map(d => `${d.span}: ${d.message}`).join('\n')); + } +} \ No newline at end of file diff --git a/packages/compiler/src/compiler.ts b/packages/compiler/src/compiler.ts index d126a872fc..ae9015597b 100644 --- a/packages/compiler/src/compiler.ts +++ b/packages/compiler/src/compiler.ts @@ -61,6 +61,7 @@ export * from './ml_parser/interpolation_config'; export * from './ml_parser/tags'; export {NgModuleCompiler} from './ng_module_compiler'; export {AssertNotNull, BinaryOperator, BinaryOperatorExpr, BuiltinMethod, BuiltinVar, CastExpr, ClassStmt, CommaExpr, CommentStmt, ConditionalExpr, DeclareFunctionStmt, DeclareVarStmt, ExpressionStatement, ExpressionVisitor, ExternalExpr, ExternalReference, FunctionExpr, IfStmt, InstantiateExpr, InvokeFunctionExpr, InvokeMethodExpr, LiteralArrayExpr, LiteralExpr, LiteralMapExpr, NotExpr, ReadKeyExpr, ReadPropExpr, ReadVarExpr, ReturnStatement, StatementVisitor, ThrowStmt, TryCatchStmt, WriteKeyExpr, WritePropExpr, WriteVarExpr, StmtModifier, Statement} from './output/output_ast'; +export {EmitterVisitorContext} from './output/abstract_emitter'; export * from './output/ts_emitter'; export * from './parse_util'; export * from './schema/dom_element_schema_registry'; diff --git a/packages/compiler/src/output/abstract_emitter.ts b/packages/compiler/src/output/abstract_emitter.ts index 9ec0019dd8..213c69324b 100644 --- a/packages/compiler/src/output/abstract_emitter.ts +++ b/packages/compiler/src/output/abstract_emitter.ts @@ -35,6 +35,7 @@ export class EmitterVisitorContext { private _lines: _EmittedLine[]; private _classes: o.ClassStmt[] = []; + private _preambleLineCount = 0; constructor(private _indent: number) { this._lines = [new _EmittedLine(_indent)]; } @@ -155,6 +156,23 @@ export class EmitterVisitorContext { return map; } + setPreambleLineCount(count: number) { return this._preambleLineCount = count; } + + spanOf(line: number, column: number): ParseSourceSpan|null { + const emittedLine = this._lines[line - this._preambleLineCount]; + if (emittedLine) { + let columnsLeft = column - emittedLine.indent; + for (let partIndex = 0; partIndex < emittedLine.parts.length; partIndex++) { + const part = emittedLine.parts[partIndex]; + if (part.length > columnsLeft) { + return emittedLine.srcSpans[partIndex]; + } + columnsLeft -= part.length; + } + } + return null; + } + private get sourceLines(): _EmittedLine[] { if (this._lines.length && this._lines[this._lines.length - 1].parts.length === 0) { return this._lines.slice(0, -1); diff --git a/packages/compiler/src/output/ts_emitter.ts b/packages/compiler/src/output/ts_emitter.ts index 9a7d74ff75..74f2b8410f 100644 --- a/packages/compiler/src/output/ts_emitter.ts +++ b/packages/compiler/src/output/ts_emitter.ts @@ -37,9 +37,9 @@ export function debugOutputAstAsTypeScript(ast: o.Statement | o.Expression | o.T export class TypeScriptEmitter implements OutputEmitter { - emitStatements( - srcFilePath: string, genFilePath: string, stmts: o.Statement[], - preamble: string = ''): string { + emitStatementsAndContext( + srcFilePath: string, genFilePath: string, stmts: o.Statement[], preamble: string = '', + emitSourceMaps: boolean = true): {sourceText: string, context: EmitterVisitorContext} { const converter = new _TsEmitterVisitor(); const ctx = EmitterVisitorContext.createRoot(); @@ -60,14 +60,21 @@ export class TypeScriptEmitter implements OutputEmitter { `ort * as ${prefix} from '${importedModuleName}';`); }); - const sm = - ctx.toSourceMapGenerator(srcFilePath, genFilePath, preambleLines.length).toJsComment(); + const sm = emitSourceMaps ? + ctx.toSourceMapGenerator(srcFilePath, genFilePath, preambleLines.length).toJsComment() : + ''; const lines = [...preambleLines, ctx.toSource(), sm]; if (sm) { // always add a newline at the end, as some tools have bugs without it. lines.push(''); } - return lines.join('\n'); + ctx.setPreambleLineCount(preambleLines.length); + return {sourceText: lines.join('\n'), context: ctx}; + } + + emitStatements( + srcFilePath: string, genFilePath: string, stmts: o.Statement[], preamble: string = '') { + return this.emitStatementsAndContext(srcFilePath, genFilePath, stmts, preamble).sourceText; } } diff --git a/packages/compiler/test/aot/test_util.ts b/packages/compiler/test/aot/test_util.ts index db91f275f1..1fc4c392c3 100644 --- a/packages/compiler/test/aot/test_util.ts +++ b/packages/compiler/test/aot/test_util.ts @@ -584,7 +584,7 @@ export function expectNoDiagnostics(program: ts.Program) { expectNoDiagnostics(program.getSemanticDiagnostics()); } -function isSource(fileName: string): boolean { +export function isSource(fileName: string): boolean { return !/\.d\.ts$/.test(fileName) && /\.ts$/.test(fileName); }