fix(compiler-cli): prevent eliding default imports in incremental recompilations (#41557)

The Angular compiler has to actively keep default import statements
alive if they were only used in type-only positions, but have been
emitted as value expressions for DI purposes. A problem occurred in
incremental recompilations, where the relationship between an identifier
usage and its corresponding default import would not be considered. This
could result in the removal of the default import statement and caused
a `ReferenceError` at runtime.

This commit fixes the issue by storing the association from an
identifier to its default import declaration on the source file itself,
instead of within the `DefaultImportTracker` instance. The
`DefaultImportTracker` instance is only valid for a single compilation,
whereas the association from an identifier to a default import
declaration is valid as long as the `ts.SourceFile` is the same
instance.

A subsequent commit refactor the `DefaultImportTracker` to no longer
be responsible for registering the association, as its lifetime is
conceptually too short to do so.

Fixes #41377

PR Close #41557

packages/compiler-cli/src/ngtsc/imports/src/default.ts

@@ -48,6 +48,12 @@ export const NOOP_DEFAULT_IMPORT_RECORDER: DefaultImportRecorder = {
   recordUsedIdentifier: (id: ts.Identifier) => void{},
 };
 
+const ImportDeclarationMapping = Symbol('ImportDeclarationMapping');
+
+interface SourceFileWithImportDeclarationMapping extends ts.SourceFile {
+  [ImportDeclarationMapping]?: Map<ts.Identifier, ts.ImportDeclaration>;
+}
+
 /**
  * TypeScript has trouble with generating default imports inside of transformers for some module
  * formats. The issue is that for the statement:
@@ -79,38 +85,31 @@ export const NOOP_DEFAULT_IMPORT_RECORDER: DefaultImportRecorder = {
  * "import * as X" style imports for those, and the "X" identifier survives transformation.
  */
 export class DefaultImportTracker implements DefaultImportRecorder {
-  /**
-   * A `Map` which tracks the `Map` of default import `ts.Identifier`s to their
-   * `ts.ImportDeclaration`s. These declarations are not guaranteed to be used.
-   */
-  private sourceFileToImportMap =
-      new Map<ts.SourceFile, Map<ts.Identifier, ts.ImportDeclaration>>();
-
   /**
    * A `Map` which tracks the `Set` of `ts.ImportDeclaration`s for default imports that were used in
    * a given `ts.SourceFile` and need to be preserved.
    */
   private sourceFileToUsedImports = new Map<ts.SourceFile, Set<ts.ImportDeclaration>>();
   recordImportedIdentifier(id: ts.Identifier, decl: ts.ImportDeclaration): void {
-    const sf = getSourceFile(id);
+    const sf = getSourceFile(id) as SourceFileWithImportDeclarationMapping;
-    if (!this.sourceFileToImportMap.has(sf)) {
+    if (sf[ImportDeclarationMapping] === undefined) {
-      this.sourceFileToImportMap.set(sf, new Map<ts.Identifier, ts.ImportDeclaration>());
+      sf[ImportDeclarationMapping] = new Map<ts.Identifier, ts.ImportDeclaration>();
     }
-    this.sourceFileToImportMap.get(sf)!.set(id, decl);
+    sf[ImportDeclarationMapping]!.set(id, decl);
   }
 
   recordUsedIdentifier(id: ts.Identifier): void {
-    const sf = getSourceFile(id);
+    const sf = getSourceFile(id) as SourceFileWithImportDeclarationMapping;
-    if (!this.sourceFileToImportMap.has(sf)) {
+    const identifierToDeclaration = sf[ImportDeclarationMapping];
+    if (identifierToDeclaration === undefined) {
       // The identifier's source file has no registered default imports at all.
       return;
     }
-    const identiferToDeclaration = this.sourceFileToImportMap.get(sf)!;
+    if (!identifierToDeclaration.has(id)) {
-    if (!identiferToDeclaration.has(id)) {
       // The identifier isn't from a registered default import.
       return;
     }
-    const decl = identiferToDeclaration.get(id)!;
+    const decl = identifierToDeclaration.get(id)!;
 
     // Add the default import declaration to the set of used import declarations for the file.
     if (!this.sourceFileToUsedImports.has(sf)) {
@@ -182,7 +181,6 @@ export class DefaultImportTracker implements DefaultImportRecorder {
 
     // Save memory - there's no need to keep these around once the transform has run for the given
     // file.
-    this.sourceFileToImportMap.delete(originalSf);
     this.sourceFileToUsedImports.delete(originalSf);
 
     return ts.updateSourceFileNode(sf, statements);

packages/compiler-cli/test/ngtsc/incremental_spec.ts

@@ -605,6 +605,67 @@ runInEachFileSystem(() => {
         expect(env.driveDiagnostics().length).toBe(1);
       });
 
+      it('should retain default imports that have been converted into a value expression', () => {
+        // This test defines the component `TestCmp` that has a default-imported class as
+        // constructor parameter, and uses `TestDir` in its template. An incremental compilation
+        // updates `TestDir` and changes its inputs, thereby triggering re-emit of `TestCmp` without
+        // performing re-analysis of `TestCmp`. The output of the re-emitted file for `TestCmp`
+        // should continue to have retained the default import.
+        env.write('service.ts', `
+          import {Injectable} from '@angular/core';
+
+          @Injectable({ providedIn: 'root' })
+          export default class DefaultService {}
+        `);
+        env.write('cmp.ts', `
+          import {Component, Directive} from '@angular/core';
+          import DefaultService from './service';
+
+          @Component({
+            template: '<div dir></div>',
+          })
+          export class TestCmp {
+            constructor(service: DefaultService) {}
+          }
+        `);
+        env.write('dir.ts', `
+          import {Directive} from '@angular/core';
+
+          @Directive({ selector: '[dir]' })
+          export class TestDir {}
+        `);
+        env.write('mod.ts', `
+          import {NgModule} from '@angular/core';
+          import {TestDir} from './dir';
+          import {TestCmp} from './cmp';
+
+          @NgModule({ declarations: [TestDir, TestCmp] })
+          export class TestMod {}
+        `);
+
+        env.driveMain();
+        env.flushWrittenFileTracking();
+
+        // Update `TestDir` to change its inputs, triggering a re-emit of `TestCmp` that uses
+        // `TestDir`.
+        env.write('dir.ts', `
+          import {Directive} from '@angular/core';
+
+          @Directive({ selector: '[dir]', inputs: ['added'] })
+          export class TestDir {}
+        `);
+        env.driveMain();
+
+        // Verify that `TestCmp` was indeed re-emitted.
+        const written = env.getFilesWrittenSinceLastFlush();
+        expect(written).toContain('/dir.js');
+        expect(written).toContain('/cmp.js');
+
+        // Verify that the default import is still present.
+        const content = env.getContents('cmp.js');
+        expect(content).toContain(`import DefaultService from './service';`);
+      });
+
       it('should recompile when a remote change happens to a scope', () => {
         // The premise of this test is that the component Cmp has a template error (a binding to an
         // unknown property). Cmp is in ModuleA, which imports ModuleB, which declares Dir that has