From c5a75fd80722fc760cf6e968880e586ddeac04b1 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Wed, 13 Nov 2019 12:21:04 -0800 Subject: [PATCH] fix(language-service): Recompute analyzed modules only when source files change (#33806) This commit fixes a bug brought up by @andrius-pra whereby the language service host would recompute the analyzed modules even when none of the source files changes. This is due to a bug in our unit test that precludes non-TS files from incrementing the project version. Consequently, when the external template is updated, the program remains the same. With the bug fixed, the next step is to figure out if any source files have been added / changed / removed since the last computation. The previously analyzed could be safely retained only when none of these operations happen. PR Close #33806 --- .../language-service/src/typescript_host.ts | 51 ++++++++++++------- packages/language-service/test/test_utils.ts | 4 +- .../test/typescript_host_spec.ts | 8 ++- 3 files changed, 42 insertions(+), 21 deletions(-) diff --git a/packages/language-service/src/typescript_host.ts b/packages/language-service/src/typescript_host.ts index 87f51c4dff..85a40d40c1 100644 --- a/packages/language-service/src/typescript_host.ts +++ b/packages/language-service/src/typescript_host.ts @@ -195,7 +195,8 @@ export class TypeScriptServiceHost implements LanguageServiceHost { } /** - * Checks whether the program has changed, and invalidate caches if it has. + * Checks whether the program has changed, and invalidate static symbols in + * the source files that have changed. * Returns true if modules are up-to-date, false otherwise. * This should only be called by getAnalyzedModules(). */ @@ -206,30 +207,46 @@ export class TypeScriptServiceHost implements LanguageServiceHost { } this.lastProgram = program; - // Invalidate file that have changed in the static symbol resolver + // Even though the program has changed, it could be the case that none of + // the source files have changed. If all source files remain the same, then + // program is still up-to-date, and we should not invalidate caches. + let filesAdded = 0; + const filesChangedOrRemoved: string[] = []; + + // Check if any source files have been added / changed since last computation. const seen = new Set(); - for (const sourceFile of program.getSourceFiles()) { - const fileName = sourceFile.fileName; + for (const {fileName} of program.getSourceFiles()) { seen.add(fileName); const version = this.tsLsHost.getScriptVersion(fileName); const lastVersion = this.fileVersions.get(fileName); - this.fileVersions.set(fileName, version); - // Should not invalidate file on the first encounter or if file hasn't changed - if (lastVersion !== undefined && version !== lastVersion) { - const symbols = this.staticSymbolResolver.invalidateFile(fileName); - this.reflector.invalidateSymbols(symbols); + if (lastVersion === undefined) { + filesAdded++; + this.fileVersions.set(fileName, version); + } else if (version !== lastVersion) { + filesChangedOrRemoved.push(fileName); // changed + this.fileVersions.set(fileName, version); } } - // Remove file versions that are no longer in the program and invalidate them. - const missing = Array.from(this.fileVersions.keys()).filter(f => !seen.has(f)); - missing.forEach(f => { - this.fileVersions.delete(f); - const symbols = this.staticSymbolResolver.invalidateFile(f); - this.reflector.invalidateSymbols(symbols); - }); + // Check if any source files have been removed since last computation. + for (const [fileName] of this.fileVersions) { + if (!seen.has(fileName)) { + filesChangedOrRemoved.push(fileName); // removed + // Because Maps are iterated in insertion order, it is safe to delete + // entries from the same map while iterating. + // See https://stackoverflow.com/questions/35940216 and + // https://www.ecma-international.org/ecma-262/10.0/index.html#sec-map.prototype.foreach + this.fileVersions.delete(fileName); + } + } - return false; + for (const fileName of filesChangedOrRemoved) { + const symbols = this.staticSymbolResolver.invalidateFile(fileName); + this.reflector.invalidateSymbols(symbols); + } + + // Program is up-to-date iff no files are added, changed, or removed. + return filesAdded === 0 && filesChangedOrRemoved.length === 0; } /** diff --git a/packages/language-service/test/test_utils.ts b/packages/language-service/test/test_utils.ts index 554202af64..7973e2e37b 100644 --- a/packages/language-service/test/test_utils.ts +++ b/packages/language-service/test/test_utils.ts @@ -114,9 +114,7 @@ export class MockTypescriptHost implements ts.LanguageServiceHost { override(fileName: string, content: string) { this.scriptVersion.set(fileName, (this.scriptVersion.get(fileName) || 0) + 1); - if (fileName.endsWith('.ts')) { - this.projectVersion++; - } + this.projectVersion++; if (content) { this.overrides.set(fileName, content); this.overrideDirectory.add(path.dirname(fileName)); diff --git a/packages/language-service/test/typescript_host_spec.ts b/packages/language-service/test/typescript_host_spec.ts index 7aa0512cf6..b7a0a53d16 100644 --- a/packages/language-service/test/typescript_host_spec.ts +++ b/packages/language-service/test/typescript_host_spec.ts @@ -50,7 +50,7 @@ describe('TypeScriptServiceHost', () => { expect(analyzedModules.files.length).toBe(0); expect(analyzedModules.ngModules.length).toBe(0); expect(analyzedModules.ngModuleByPipeOrDirective.size).toBe(0); - expect(analyzedModules.symbolsMissingModule).toEqual([]); + expect(analyzedModules.symbolsMissingModule).toBeUndefined(); }); it('should clear the caches if new script is added', () => { @@ -166,8 +166,14 @@ describe('TypeScriptServiceHost', () => { const tsLS = ts.createLanguageService(tsLSHost); const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS); const oldModules = ngLSHost.getAnalyzedModules(); + const oldProgram = ngLSHost.program; tsLSHost.override('/app/test.ng', '
'); const newModules = ngLSHost.getAnalyzedModules(); + const newProgram = ngLSHost.program; + // Assert that the program has changed because external template was updated + expect(newProgram).not.toBe(oldProgram); + // But, analyzed modules should remain the same because none of the source + // files have changed. expect(newModules).toBe(oldModules); });