fix(compiler-cli): correct incremental behavior even with broken imports (#39923)

When the compiler is invoked via ngc or the Angular CLI, its APIs are used
under the assumption that Angular analysis/diagnostics are only requested if
the program has no TypeScript-level errors. A result of this assumption is
that the incremental engine has not needed to resolve changes via its
dependency graph when the program contained broken imports, since broken
imports are a TypeScript error.

The Angular Language Service for Ivy is using the compiler as a backend, and
exercising its incremental compilation APIs without enforcing this
assumption. As a result, the Language Service has run into issues where
broken imports cause incremental compilation to fail and produce incorrect
results.

This commit introduces a mechanism within the compiler to keep track of
files for which dependency analysis has failed, and to always treat such
files as potentially affected by future incremental steps. This is tested
via the Language Service infrastructure to ensure that the compiler is doing
the right thing in the case of invalid imports.

PR Close #39923

Author: alxhub

packages/compiler-cli/ngcc/src/analysis/util.ts

@@ -18,6 +18,7 @@ class NoopDependencyTracker implements DependencyTracker {
   addResourceDependency(): void {}
   addTransitiveDependency(): void {}
   addTransitiveResources(): void {}
+  recordDependencyAnalysisFailure(): void {}
 }
 
 export const NOOP_DEPENDENCY_TRACKER: DependencyTracker = new NoopDependencyTracker();

packages/compiler-cli/src/ngtsc/incremental/api.ts

@@ -64,4 +64,12 @@ export interface DependencyTracker<T extends {fileName: string} = ts.SourceFile>
    * `resourcesOf` they will not automatically be added to `from`.
    */
   addTransitiveResources(from: T, resourcesOf: T): void;
+
+  /**
+   * Record that the given file contains unresolvable dependencies.
+   *
+   * In practice, this means that the dependency graph cannot provide insight into the effects of
+   * future changes on that file.
+   */
+  recordDependencyAnalysisFailure(file: T): void;
 }

packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts

@@ -53,6 +53,10 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
     }
   }
 
+  recordDependencyAnalysisFailure(file: T): void {
+    this.nodeFor(file).failedAnalysis = true;
+  }
+
   getResourceDependencies(from: T): AbsoluteFsPath[] {
     const node = this.nodes.get(from);
 
@@ -97,6 +101,7 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
         this.nodes.set(sf, {
           dependsOn: new Set(node.dependsOn),
           usesResources: new Set(node.usesResources),
+          failedAnalysis: false,
         });
       }
     }
@@ -109,6 +114,7 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
       this.nodes.set(sf, {
         dependsOn: new Set<string>(),
         usesResources: new Set<AbsoluteFsPath>(),
+        failedAnalysis: false,
       });
     }
     return this.nodes.get(sf)!;
@@ -122,6 +128,12 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
 function isLogicallyChanged<T extends {fileName: string}>(
     sf: T, node: FileNode, changedTsPaths: ReadonlySet<string>, deletedTsPaths: ReadonlySet<string>,
     changedResources: ReadonlySet<AbsoluteFsPath>): boolean {
+  // A file is assumed to have logically changed if its dependencies could not be determined
+  // accurately.
+  if (node.failedAnalysis) {
+    return true;
+  }
+
   // A file is logically changed if it has physically changed itself (including being deleted).
   if (changedTsPaths.has(sf.fileName) || deletedTsPaths.has(sf.fileName)) {
     return true;
@@ -146,6 +158,7 @@ function isLogicallyChanged<T extends {fileName: string}>(
 interface FileNode {
   dependsOn: Set<string>;
   usesResources: Set<AbsoluteFsPath>;
+  failedAnalysis: boolean;
 }
 
 const EMPTY_SET: ReadonlySet<any> = new Set<any>();

packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts

@@ -220,6 +220,15 @@ export class StaticInterpreter {
       if (node.originalKeywordKind === ts.SyntaxKind.UndefinedKeyword) {
         return undefined;
       } else {
+        // Check if the symbol here is imported.
+        if (this.dependencyTracker !== null && this.host.getImportOfIdentifier(node) !== null) {
+          // It was, but no declaration for the node could be found. This means that the dependency
+          // graph for the current file cannot be properly updated to account for this (broken)
+          // import. Instead, the originating file is reported as failing dependency analysis,
+          // ensuring that future compilations will always attempt to re-resolve the previously
+          // broken identifier.
+          this.dependencyTracker.recordDependencyAnalysisFailure(context.originatingFile);
+        }
         return DynamicValue.fromUnknownIdentifier(node);
       }
     }

packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts

@@ -991,4 +991,5 @@ const fakeDepTracker: DependencyTracker = {
   addResourceDependency: () => undefined,
   addTransitiveDependency: () => undefined,
   addTransitiveResources: () => undefined,
+  recordDependencyAnalysisFailure: () => undefined,
 };

packages/language-service/ivy/test/compiler_spec.ts

@@ -60,4 +60,72 @@ describe('language-service/compiler integration', () => {
     expect(diags.map(diag => diag.messageText))
         .toContain(`Type 'number' is not assignable to type 'string'.`);
   });
+
+  it('should handle broken imports during incremental build steps', () => {
+    // This test validates that the compiler's incremental APIs correctly handle a broken import
+    // when invoked via the Language Service. Testing this via the LS is important as only the LS
+    // requests Angular analysis in the presence of TypeScript-level errors. In the case of broken
+    // imports this distinction is especially important: Angular's incremental analysis is
+    // built on the the compiler's dependency graph, and this graph must be able to function even
+    // with broken imports.
+    //
+    // The test works by creating a component/module pair where the module imports and declares a
+    // component from a separate file. That component is initially not exported, meaning the
+    // module's import is broken. Angular will correctly complain that the NgModule is declaring a
+    // value which is not statically analyzable.
+    //
+    // Then, the component file is fixed to properly export the component class, and an incremental
+    // build step is performed. The compiler should recognize that the module's previous analysis
+    // is stale, even though it was not able to fully understand the import during the first pass.
+
+    const moduleFile = absoluteFrom('/mod.ts');
+    const componentFile = absoluteFrom('/cmp.ts');
+
+    const componentSource = (isExported: boolean): string => `
+      import {Component} from '@angular/core';
+
+      @Component({
+        selector: 'some-cmp',
+        template: 'Not important',
+      })
+      ${isExported ? 'export' : ''} class Cmp {}
+    `;
+
+    const env = LanguageServiceTestEnvironment.setup([
+      {
+        name: moduleFile,
+        contents: `
+          import {NgModule} from '@angular/core';
+    
+          import {Cmp} from './cmp';
+    
+          @NgModule({
+            declarations: [Cmp],
+          })
+          export class Mod {}
+        `,
+        isRoot: true,
+      },
+      {
+        name: componentFile,
+        contents: componentSource(/* start with component not exported */ false),
+        isRoot: true,
+      }
+    ]);
+
+    // Angular should be complaining about the module not being understandable.
+    const programBefore = env.tsLS.getProgram()!;
+    const moduleSfBefore = programBefore.getSourceFile(moduleFile)!;
+    const ngDiagsBefore = env.ngLS.compilerFactory.getOrCreate().getDiagnostics(moduleSfBefore);
+    expect(ngDiagsBefore.length).toBe(1);
+
+    // Fix the import.
+    env.updateFile(componentFile, componentSource(/* properly export the component */ true));
+
+    // Angular should stop complaining about the NgModule.
+    const programAfter = env.tsLS.getProgram()!;
+    const moduleSfAfter = programAfter.getSourceFile(moduleFile)!;
+    const ngDiagsAfter = env.ngLS.compilerFactory.getOrCreate().getDiagnostics(moduleSfAfter);
+    expect(ngDiagsAfter.length).toBe(0);
+  });
 });

packages/language-service/ivy/test/env.ts

@@ -55,8 +55,8 @@ export interface TemplateOverwriteResult {
 
 export class LanguageServiceTestEnvironment {
   private constructor(
-      private tsLS: ts.LanguageService, readonly ngLS: LanguageService,
-      readonly host: MockServerHost) {}
+      readonly tsLS: ts.LanguageService, readonly ngLS: LanguageService,
+      readonly projectService: ts.server.ProjectService, readonly host: MockServerHost) {}
 
   static setup(files: TestFile[], options: TestableOptions = {}): LanguageServiceTestEnvironment {
     const fs = getFileSystem();
@@ -106,7 +106,7 @@ export class LanguageServiceTestEnvironment {
     const tsLS = project.getLanguageService();
 
     const ngLS = new LanguageService(project, tsLS);
-    return new LanguageServiceTestEnvironment(tsLS, ngLS, host);
+    return new LanguageServiceTestEnvironment(tsLS, ngLS, projectService, host);
   }
 
   getClass(fileName: AbsoluteFsPath, className: string): ts.ClassDeclaration {
@@ -136,6 +136,17 @@ export class LanguageServiceTestEnvironment {
     return {cursor, nodes, component, text};
   }
 
+  updateFile(fileName: AbsoluteFsPath, contents: string): void {
+    const scriptInfo = this.projectService.getScriptInfo(fileName);
+    if (scriptInfo === undefined) {
+      throw new Error(`Could not find a file named ${fileName}`);
+    }
+
+    // Get the current contents to find the length
+    const len = scriptInfo.getSnapshot().getLength();
+    scriptInfo.editContent(0, len, contents);
+  }
+
   expectNoSourceDiagnostics(): void {
     const program = this.tsLS.getProgram();
     if (program === undefined) {