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

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<string>();
-    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;
   }
 
   /**

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));

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', '<div></div>');
     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);
   });