From 957be960d28f4f6e8df695d98e2ed117e8451f88 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Thu, 26 Oct 2017 15:24:54 -0700 Subject: [PATCH] fix(compiler): recover from structural errors in watch mode (#19953) This also changes the compiler so that we throw less often on structural changes and produce a meaningful state in the `ng.Program` in case of errors. Related to #19951 PR Close #19953 --- packages/compiler-cli/src/perform_watch.ts | 4 ++ .../compiler-cli/src/transformers/program.ts | 69 ++++++++++--------- packages/compiler-cli/test/ngc_spec.ts | 2 +- .../compiler-cli/test/perform_watch_spec.ts | 58 +++++++++++++--- packages/compiler-cli/test/test_support.ts | 4 +- .../test/transformers/program_spec.ts | 62 +++++++++++++++++ packages/compiler/src/aot/static_reflector.ts | 2 +- 7 files changed, 157 insertions(+), 44 deletions(-) diff --git a/packages/compiler-cli/src/perform_watch.ts b/packages/compiler-cli/src/perform_watch.ts index c5acc570f0..2943bc7f5c 100644 --- a/packages/compiler-cli/src/perform_watch.ts +++ b/packages/compiler-cli/src/perform_watch.ts @@ -191,6 +191,10 @@ export function performWatchCompilation(host: PerformWatchHost): }; } ingoreFilesForWatch.clear(); + const oldProgram = cachedProgram; + // We clear out the `cachedProgram` here as a + // program can only be used as `oldProgram` 1x + cachedProgram = undefined; const compileResult = performCompilation({ rootNames: cachedOptions.rootNames, options: cachedOptions.options, diff --git a/packages/compiler-cli/src/transformers/program.ts b/packages/compiler-cli/src/transformers/program.ts index ac9a9bb1ef..27bf90cf7c 100644 --- a/packages/compiler-cli/src/transformers/program.ts +++ b/packages/compiler-cli/src/transformers/program.ts @@ -175,15 +175,17 @@ class AngularCompilerProgram implements Program { if (this._analyzedModules) { throw new Error('Angular structure already loaded'); } - const {tmpProgram, sourceFiles, rootNames} = this._createProgramWithBasicStubs(); - return this.compiler.loadFilesAsync(sourceFiles) - .catch(this.catchAnalysisError.bind(this)) - .then(analyzedModules => { - if (this._analyzedModules) { - throw new Error('Angular structure loaded both synchronously and asynchronsly'); - } - this._updateProgramWithTypeCheckStubs(tmpProgram, analyzedModules, rootNames); - }); + return Promise.resolve() + .then(() => { + const {tmpProgram, sourceFiles, rootNames} = this._createProgramWithBasicStubs(); + return this.compiler.loadFilesAsync(sourceFiles).then(analyzedModules => { + if (this._analyzedModules) { + throw new Error('Angular structure loaded both synchronously and asynchronsly'); + } + this._updateProgramWithTypeCheckStubs(tmpProgram, analyzedModules, rootNames); + }); + }) + .catch(e => this._createProgramOnError(e)); } listLazyRoutes(route?: string): LazyRoute[] { @@ -402,14 +404,13 @@ class AngularCompilerProgram implements Program { if (this._analyzedModules) { return; } - const {tmpProgram, sourceFiles, rootNames} = this._createProgramWithBasicStubs(); - let analyzedModules: NgAnalyzedModules|null; try { - analyzedModules = this.compiler.loadFilesSync(sourceFiles); + const {tmpProgram, sourceFiles, rootNames} = this._createProgramWithBasicStubs(); + const analyzedModules = this.compiler.loadFilesSync(sourceFiles); + this._updateProgramWithTypeCheckStubs(tmpProgram, analyzedModules, rootNames); } catch (e) { - analyzedModules = this.catchAnalysisError(e); + this._createProgramOnError(e); } - this._updateProgramWithTypeCheckStubs(tmpProgram, analyzedModules, rootNames); } private _createCompiler() { @@ -457,7 +458,7 @@ class AngularCompilerProgram implements Program { let rootNames = this.rootNames; if (this.options.generateCodeForLibraries !== false) { - // if we should generateCodeForLibraries, enver include + // if we should generateCodeForLibraries, never include // generated files in the program as otherwise we will // ovewrite them and typescript will report the error // TS5055: Cannot write file ... because it would overwrite input file. @@ -482,23 +483,21 @@ class AngularCompilerProgram implements Program { } private _updateProgramWithTypeCheckStubs( - tmpProgram: ts.Program, analyzedModules: NgAnalyzedModules|null, rootNames: string[]) { - this._analyzedModules = analyzedModules || emptyModules; - if (analyzedModules) { - tmpProgram.getSourceFiles().forEach(sf => { - if (sf.fileName.endsWith('.ngfactory.ts')) { - const {generate, baseFileName} = this.hostAdapter.shouldGenerateFile(sf.fileName); - if (generate) { - // Note: ! is ok as hostAdapter.shouldGenerateFile will always return a basefileName - // for .ngfactory.ts files. - const genFile = this.compiler.emitTypeCheckStub(sf.fileName, baseFileName !); - if (genFile) { - this.hostAdapter.updateGeneratedFile(genFile); - } + tmpProgram: ts.Program, analyzedModules: NgAnalyzedModules, rootNames: string[]) { + this._analyzedModules = analyzedModules; + tmpProgram.getSourceFiles().forEach(sf => { + if (sf.fileName.endsWith('.ngfactory.ts')) { + const {generate, baseFileName} = this.hostAdapter.shouldGenerateFile(sf.fileName); + if (generate) { + // Note: ! is ok as hostAdapter.shouldGenerateFile will always return a basefileName + // for .ngfactory.ts files. + const genFile = this.compiler.emitTypeCheckStub(sf.fileName, baseFileName !); + if (genFile) { + this.hostAdapter.updateGeneratedFile(genFile); } } - }); - } + } + }); this._tsProgram = ts.createProgram(rootNames, this.options, this.hostAdapter, tmpProgram); // Note: the new ts program should be completely reusable by TypeScript as: // - we cache all the files in the hostAdapter @@ -509,7 +508,13 @@ class AngularCompilerProgram implements Program { } } - private catchAnalysisError(e: any): NgAnalyzedModules|null { + private _createProgramOnError(e: any) { + // Still fill the analyzedModules and the tsProgram + // so that we don't cause other errors for users who e.g. want to emit the ngProgram. + this._analyzedModules = emptyModules; + this.oldTsProgram = undefined; + this._hostAdapter.isSourceFile = () => false; + this._tsProgram = ts.createProgram(this.rootNames, this.options, this.hostAdapter); if (isSyntaxError(e)) { const parserErrors = getParseErrors(e); if (parserErrors && parserErrors.length) { @@ -533,7 +538,7 @@ class AngularCompilerProgram implements Program { } ]; } - return null; + return; } throw e; } diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts index bf90700146..58911fcd67 100644 --- a/packages/compiler-cli/test/ngc_spec.ts +++ b/packages/compiler-cli/test/ngc_spec.ts @@ -1464,7 +1464,7 @@ describe('ngc transformer command-line', () => { const messages: string[] = []; const exitCode = main(['-p', path.join(basePath, 'src/tsconfig.json')], message => messages.push(message)); - expect(exitCode).toBe(2, 'Compile was expected to fail'); + expect(exitCode).toBe(1, 'Compile was expected to fail'); expect(messages[0]).toContain(['Tagged template expressions are not supported in metadata']); }); diff --git a/packages/compiler-cli/test/perform_watch_spec.ts b/packages/compiler-cli/test/perform_watch_spec.ts index ae0184e2c2..4cd19b7c6a 100644 --- a/packages/compiler-cli/test/perform_watch_spec.ts +++ b/packages/compiler-cli/test/perform_watch_spec.ts @@ -105,6 +105,47 @@ describe('perform watch', () => { expect(getSourceFileSpy !).toHaveBeenCalledWith(mainTsPath, ts.ScriptTarget.ES5); expect(getSourceFileSpy !).toHaveBeenCalledWith(utilTsPath, ts.ScriptTarget.ES5); }); + + it('should recover from static analysis errors', () => { + const config = createConfig(); + const host = new MockWatchHost(config); + + const okFileContent = ` + import {NgModule} from '@angular/core'; + + @NgModule() + export class MyModule {} + `; + const errorFileContent = ` + import {NgModule} from '@angular/core'; + + @NgModule(() => (1===1 ? null as any : null as any)) + export class MyModule {} + `; + const indexTsPath = path.resolve(testSupport.basePath, 'src', 'index.ts'); + + testSupport.write(indexTsPath, okFileContent); + + performWatchCompilation(host); + expectNoDiagnostics(config.options, host.diagnostics); + + // Do it multiple times as the watch mode switches internal modes. + // E.g. from regular compile to using summaries, ... + for (let i = 0; i < 3; i++) { + host.diagnostics = []; + testSupport.write(indexTsPath, okFileContent); + host.triggerFileChange(FileChangeEvent.Change, indexTsPath); + expectNoDiagnostics(config.options, host.diagnostics); + + host.diagnostics = []; + testSupport.write(indexTsPath, errorFileContent); + host.triggerFileChange(FileChangeEvent.Change, indexTsPath); + + const errDiags = host.diagnostics.filter(d => d.category === ts.DiagnosticCategory.Error); + expect(errDiags.length).toBe(1); + expect(errDiags[0].messageText).toContain('Function calls are not supported.'); + } + }); }); function createModuleAndCompSource(prefix: string, template: string = prefix + 'template') { @@ -122,7 +163,8 @@ function createModuleAndCompSource(prefix: string, template: string = prefix + ' } class MockWatchHost { - timeoutListeners: Array<(() => void)|null> = []; + nextTimeoutListenerId = 1; + timeoutListeners: {[id: string]: (() => void)} = {}; fileChangeListeners: Array<((event: FileChangeEvent, fileName: string) => void)|null> = []; diagnostics: ng.Diagnostics = []; constructor(public config: ng.ParsedConfiguration) {} @@ -141,16 +183,16 @@ class MockWatchHost { close: () => this.fileChangeListeners[id] = null, }; } - setTimeout(callback: () => void, ms: number): any { - const id = this.timeoutListeners.length; - this.timeoutListeners.push(callback); + setTimeout(callback: () => void): any { + const id = this.nextTimeoutListenerId++; + this.timeoutListeners[id] = callback; return id; } - clearTimeout(timeoutId: any): void { this.timeoutListeners[timeoutId] = null; } + clearTimeout(timeoutId: any): void { delete this.timeoutListeners[timeoutId]; } flushTimeouts() { - this.timeoutListeners.forEach(cb => { - if (cb) cb(); - }); + const listeners = this.timeoutListeners; + this.timeoutListeners = {}; + Object.keys(listeners).forEach(id => listeners[id]()); } triggerFileChange(event: FileChangeEvent, fileName: string) { this.fileChangeListeners.forEach(listener => { diff --git a/packages/compiler-cli/test/test_support.ts b/packages/compiler-cli/test/test_support.ts index 122bb64492..c64be8b91f 100644 --- a/packages/compiler-cli/test/test_support.ts +++ b/packages/compiler-cli/test/test_support.ts @@ -64,10 +64,10 @@ export function setup(): TestSupport { function write(fileName: string, content: string) { const dir = path.dirname(fileName); if (dir != '.') { - const newDir = path.join(basePath, dir); + const newDir = path.resolve(basePath, dir); if (!fs.existsSync(newDir)) fs.mkdirSync(newDir); } - fs.writeFileSync(path.join(basePath, fileName), content, {encoding: 'utf-8'}); + fs.writeFileSync(path.resolve(basePath, fileName), content, {encoding: 'utf-8'}); } function writeFiles(...mockDirs: {[fileName: string]: string}[]) { diff --git a/packages/compiler-cli/test/transformers/program_spec.ts b/packages/compiler-cli/test/transformers/program_spec.ts index 8bc67cfbf5..ecb379fa21 100644 --- a/packages/compiler-cli/test/transformers/program_spec.ts +++ b/packages/compiler-cli/test/transformers/program_spec.ts @@ -889,4 +889,66 @@ describe('ng program', () => { .toContain( `src/main.html(1,1): error TS100: Property 'nonExistent' does not exist on type 'MyComp'.`); }); + + describe('errors', () => { + const fileWithStructuralError = ` + import {NgModule} from '@angular/core'; + + @NgModule(() => (1===1 ? null as any : null as any)) + export class MyModule {} + `; + const fileWithGoodContent = ` + import {NgModule} from '@angular/core'; + + @NgModule() + export class MyModule {} + `; + + it('should not throw on structural errors but collect them', () => { + testSupport.write('src/index.ts', fileWithStructuralError); + + const options = testSupport.createCompilerOptions(); + const host = ng.createCompilerHost({options}); + const program = ng.createProgram( + {rootNames: [path.resolve(testSupport.basePath, 'src/index.ts')], options, host}); + + const structuralErrors = program.getNgStructuralDiagnostics(); + expect(structuralErrors.length).toBe(1); + expect(structuralErrors[0].messageText).toContain('Function calls are not supported.'); + }); + + it('should not throw on structural errors but collect them (loadNgStructureAsync)', (done) => { + testSupport.write('src/index.ts', fileWithStructuralError); + + const options = testSupport.createCompilerOptions(); + const host = ng.createCompilerHost({options}); + const program = ng.createProgram( + {rootNames: [path.resolve(testSupport.basePath, 'src/index.ts')], options, host}); + program.loadNgStructureAsync().then(() => { + const structuralErrors = program.getNgStructuralDiagnostics(); + expect(structuralErrors.length).toBe(1); + expect(structuralErrors[0].messageText).toContain('Function calls are not supported.'); + done(); + }); + }); + + it('should be able to use a program with structural errors as oldProgram', () => { + testSupport.write('src/index.ts', fileWithStructuralError); + + const options = testSupport.createCompilerOptions(); + const host = ng.createCompilerHost({options}); + const program1 = ng.createProgram( + {rootNames: [path.resolve(testSupport.basePath, 'src/index.ts')], options, host}); + expect(program1.getNgStructuralDiagnostics().length).toBe(1); + + testSupport.write('src/index.ts', fileWithGoodContent); + const program2 = ng.createProgram({ + rootNames: [path.resolve(testSupport.basePath, 'src/index.ts')], + options, + host, + oldProgram: program1 + }); + expectNoDiagnosticsInProgram(options, program2); + }); + }); }); diff --git a/packages/compiler/src/aot/static_reflector.ts b/packages/compiler/src/aot/static_reflector.ts index daf6b8ac0d..5300134cc1 100644 --- a/packages/compiler/src/aot/static_reflector.ts +++ b/packages/compiler/src/aot/static_reflector.ts @@ -752,7 +752,7 @@ class PopulatedScope extends BindingScope { } function positionalError(message: string, fileName: string, line: number, column: number): Error { - const result = new Error(message); + const result = syntaxError(message); (result as any).fileName = fileName; (result as any).line = line; (result as any).column = column;