From 9882a8215ad3b049f70d5a7eb7074a5a51006863 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 | 49 +++++++++++++------ packages/language-service/test/test_utils.ts | 4 +- .../test/typescript_host_spec.ts | 8 ++- 3 files changed, 41 insertions(+), 20 deletions(-) diff --git a/packages/language-service/src/typescript_host.ts b/packages/language-service/src/typescript_host.ts index 87f51c4dffc3e..85a40d40c1b26 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); + } + } + + // 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); } } - // 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); + for (const fileName of filesChangedOrRemoved) { + const symbols = this.staticSymbolResolver.invalidateFile(fileName); this.reflector.invalidateSymbols(symbols); - }); + } - return false; + // 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 554202af649cb..7973e2e37bda9 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 7aa0512cf6ed4..b7a0a53d168e0 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); });