refactor(compiler-cli): update type check generation code to use new …

…import manager (#54983)

Updates the type-check block generation code (also for inline type check
blocks) to use the new import manager.

This is now a requirement because the translator utilities from the
reference emit environment expect an import manager that follows the
new contract established via `ImportGenerator<TFile, TExpression>`.

For type check files, we can simply print new imports as we don't expect
existing imports to be updated. That is because type check files do not
have any _original_ source files (or in practice— those are empty).

For type check blocks inline, or constructors, imports _may_ be re-used.
This is great as it helps fixing some incrementality bugs that we were
seeing in the type check code. That is, sometimes the type check block
code may generate imports conditionally for e.g. `TemplateRef`, or
animations. Those then **prevent** incremental re-use if TCB code
switches between those continously. We tried to account for that with
signal inputs by always pre-generating such imports. This fixed the
issue for type-check files, but for inline type check blocks this is
different as we would introduce new imports in user code that would then
be changed back in subsequential edit iterations. See:
#53521 (review).

In practice, the assumption was that we would be fine since user code is
most likely containing imports to `@angular/core` already. That is a
true assumption, but unfortunately it doesn't help with incremental
re-use because TypeScript's structural change detection does not dedupe
and expects 1:1 exact imports from their old source files.

microsoft/TypeScript#56845

To improve incremental re-use for the type check integration, we should
re-use original source file imports when possible. This commit enables
this.

To update imports and execute inline operations, we are now uisng
`magic-string` (which is then bundled) as it simplifies the string
manipulatuons.

PR Close #54983

package.json

@@ -118,7 +118,7 @@
     "karma-jasmine": "^5.0.0",
     "karma-requirejs": "^1.1.0",
     "karma-sourcemap-loader": "^0.4.0",
-    "magic-string": "0.30.7",
+    "magic-string": "^0.30.8",
     "memo-decorator": "^2.0.1",
     "ngx-flamegraph": "0.0.12",
     "ngx-progressbar": "^11.1.0",

packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel

@@ -26,6 +26,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/typecheck/diagnostics",
         "//packages/compiler-cli/src/ngtsc/util",
         "@npm//@types/node",
+        "@npm//magic-string",
         "@npm//typescript",
     ],
 )

packages/compiler-cli/src/ngtsc/typecheck/src/context.ts

@@ -7,16 +7,17 @@
  */
 
 import {BoundTarget, ParseError, ParseSourceFile, R3TargetBinder, SchemaMetadata, TmplAstNode} from '@angular/compiler';
+import MagicString from 'magic-string';
 import ts from 'typescript';
 
 import {ErrorCode, ngErrorCode} from '../../../../src/ngtsc/diagnostics';
 import {absoluteFromSourceFile, AbsoluteFsPath} from '../../file_system';
-import {NoopImportRewriter, Reference, ReferenceEmitter} from '../../imports';
+import {Reference, ReferenceEmitter} from '../../imports';
 import {PipeMeta} from '../../metadata';
 import {PerfEvent, PerfRecorder} from '../../perf';
 import {FileUpdate} from '../../program_driver';
 import {ClassDeclaration, ReflectionHost} from '../../reflection';
-import {ImportManager} from '../../translator';
+import {ImportManagerV2} from '../../translator';
 import {TemplateDiagnostic, TemplateId, TemplateSourceMapping, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata, TypeCheckContext, TypeCheckingConfig, TypeCtorMetadata} from '../api';
 import {makeTemplateDiagnostic} from '../diagnostics';
 
@@ -27,7 +28,6 @@ import {ReferenceEmitEnvironment} from './reference_emit_environment';
 import {TypeCheckShimGenerator} from './shim';
 import {TemplateSourceManager} from './source';
 import {requiresInlineTypeCheckBlock, TcbInliningRequirement} from './tcb_util';
-import {getImportString} from './ts_util';
 import {generateTypeCheckBlock, TcbGenericContextBehavior} from './type_check_block';
 import {TypeCheckFile} from './type_check_file';
 import {generateInlineTypeCtor, requiresInlineTypeCtor} from './type_constructor';
@@ -358,34 +358,61 @@ export class TypeCheckContextImpl implements TypeCheckContext {
       return null;
     }
 
-    // Imports may need to be added to the file to support type-checking of directives used in the
-    // template within it.
-    const importManager = new ImportManager(new NoopImportRewriter(), '_i');
-
-    // Each Op has a splitPoint index into the text where it needs to be inserted. Split the
-    // original source text into chunks at these split points, where code will be inserted between
-    // the chunks.
-    const ops = this.opMap.get(sf)!.sort(orderOps);
-    const textParts = splitStringAtPoints(sf.text, ops.map(op => op.splitPoint));
-
     // Use a `ts.Printer` to generate source code.
     const printer = ts.createPrinter({omitTrailingSemicolon: true});
 
-    // Begin with the initial section of the code text.
-    let code = textParts[0];
-
-    // Process each operation and use the printer to generate source code for it, inserting it into
-    // the source code in between the original chunks.
-    ops.forEach((op, idx) => {
-      const text = op.execute(importManager, sf, this.refEmitter, printer);
-      code += '\n\n' + text + textParts[idx + 1];
+    // Imports may need to be added to the file to support type-checking of directives
+    // used in the template within it.
+    const importManager = new ImportManagerV2({
+      // This minimizes noticeable changes with older versions of `ImportManager`.
+      forceGenerateNamespacesForNewImports: true,
+      // Type check block code affects code completion and fix suggestions.
+      // We want to encourage single quotes for now, like we always did.
+      shouldUseSingleQuotes: () => true,
     });
 
-    // Write out the imports that need to be added to the beginning of the file.
-    let imports = importManager.getAllImports(sf.fileName).map(getImportString).join('\n');
-    code = imports + '\n' + code;
+    // Execute ops.
+    // Each Op has a splitPoint index into the text where it needs to be inserted.
+    const updates: {pos: number, deletePos?: number, text: string}[] =
+        this.opMap.get(sf)!.map(op => {
+          return {
+            pos: op.splitPoint,
+            text: op.execute(importManager, sf, this.refEmitter, printer),
+          };
+        });
+
+    const {newImports, updatedImports} = importManager.finalize();
+
+    // Capture new imports
+    if (newImports.has(sf.fileName)) {
+      newImports.get(sf.fileName)!.forEach(newImport => {
+        updates.push({
+          pos: 0,
+          text: printer.printNode(ts.EmitHint.Unspecified, newImport, sf),
+        });
+      });
+    }
+
+    // Capture updated imports
+    for (const [oldBindings, newBindings] of updatedImports.entries()) {
+      if (oldBindings.getSourceFile() !== sf) {
+        throw new Error('Unexpected updates to unrelated source files.');
+      }
+      updates.push({
+        pos: oldBindings.getStart(),
+        deletePos: oldBindings.getEnd(),
+        text: printer.printNode(ts.EmitHint.Unspecified, newBindings, sf),
+      });
+    }
 
-    return code;
+    const result = new MagicString(sf.text, {filename: sf.fileName});
+    for (const update of updates) {
+      if (update.deletePos !== undefined) {
+        result.remove(update.pos, update.deletePos);
+      }
+      result.appendLeft(update.pos, update.text);
+    }
+    return result.toString();
   }
 
   finalize(): Map<AbsoluteFsPath, FileUpdate> {
@@ -510,8 +537,9 @@ interface Op {
   /**
    * Execute the operation and return the generated code as text.
    */
-  execute(im: ImportManager, sf: ts.SourceFile, refEmitter: ReferenceEmitter, printer: ts.Printer):
-      string;
+  execute(
+      im: ImportManagerV2, sf: ts.SourceFile, refEmitter: ReferenceEmitter,
+      printer: ts.Printer): string;
 }
 
 /**
@@ -531,16 +559,18 @@ class InlineTcbOp implements Op {
     return this.ref.node.end + 1;
   }
 
-  execute(im: ImportManager, sf: ts.SourceFile, refEmitter: ReferenceEmitter, printer: ts.Printer):
-      string {
+  execute(
+      im: ImportManagerV2, sf: ts.SourceFile, refEmitter: ReferenceEmitter,
+      printer: ts.Printer): string {
     const env = new Environment(this.config, im, refEmitter, this.reflector, sf);
     const fnName = ts.factory.createIdentifier(`_tcb_${this.ref.node.pos}`);
 
-    // Inline TCBs should copy any generic type parameter nodes directly, as the TCB code is inlined
-    // into the class in a context where that will always be legal.
+    // Inline TCBs should copy any generic type parameter nodes directly, as the TCB code is
+    // inlined into the class in a context where that will always be legal.
     const fn = generateTypeCheckBlock(
         env, this.ref, fnName, this.meta, this.domSchemaChecker, this.oobRecorder,
         TcbGenericContextBehavior.CopyClassNodes);
+
     return printer.printNode(ts.EmitHint.Unspecified, fn, sf);
   }
 }
@@ -560,8 +590,9 @@ class TypeCtorOp implements Op {
     return this.ref.node.end - 1;
   }
 
-  execute(im: ImportManager, sf: ts.SourceFile, refEmitter: ReferenceEmitter, printer: ts.Printer):
-      string {
+  execute(
+      im: ImportManagerV2, sf: ts.SourceFile, refEmitter: ReferenceEmitter,
+      printer: ts.Printer): string {
     const emitEnv = new ReferenceEmitEnvironment(im, refEmitter, this.reflector, sf);
     const tcb = generateInlineTypeCtor(emitEnv, this.ref.node, this.meta);
     return printer.printNode(ts.EmitHint.Unspecified, tcb, sf);

packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts

@@ -10,7 +10,7 @@ import ts from 'typescript';
 
 import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitter} from '../../imports';
 import {ClassDeclaration, ReflectionHost} from '../../reflection';
-import {ImportManager, translateExpression} from '../../translator';
+import {ImportManagerV2, translateExpression} from '../../translator';
 import {TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from '../api';
 
 import {ReferenceEmitEnvironment} from './reference_emit_environment';
@@ -42,7 +42,7 @@ export class Environment extends ReferenceEmitEnvironment {
   protected pipeInstStatements: ts.Statement[] = [];
 
   constructor(
-      readonly config: TypeCheckingConfig, importManager: ImportManager,
+      readonly config: TypeCheckingConfig, importManager: ImportManagerV2,
       refEmitter: ReferenceEmitter, reflector: ReflectionHost, contextFile: ts.SourceFile) {
     super(importManager, refEmitter, reflector, contextFile);
   }

packages/compiler-cli/src/ngtsc/typecheck/src/reference_emit_environment.ts

@@ -11,7 +11,7 @@ import ts from 'typescript';
 
 import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitKind, ReferenceEmitter} from '../../imports';
 import {ReflectionHost} from '../../reflection';
-import {ImportManager, translateExpression, translateType} from '../../translator';
+import {ImportManagerV2, translateExpression, translateType} from '../../translator';
 
 /**
  * An environment for a given source file that can be used to emit references.
@@ -21,7 +21,7 @@ import {ImportManager, translateExpression, translateType} from '../../translato
  */
 export class ReferenceEmitEnvironment {
   constructor(
-      readonly importManager: ImportManager, protected refEmitter: ReferenceEmitter,
+      readonly importManager: ImportManagerV2, protected refEmitter: ReferenceEmitter,
       readonly reflector: ReflectionHost, public contextFile: ts.SourceFile) {}
 
   canReferenceType(

packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts

@@ -19,17 +19,21 @@ import {ReferenceEmitEnvironment} from './reference_emit_environment';
 import {TypeParameterEmitter} from './type_parameter_emitter';
 
 /**
- * External modules that always should exist for type check blocks and
- * file hosting inline type constructors.
+ * External modules/identifiers that always should exist for type check
+ * block files.
  *
  * Importing the modules in preparation helps ensuring a stable import graph
  * that would not degrade TypeScript's incremental program structure re-use.
+ *
+ * Note: For inline type check blocks, or type constructors, we cannot add preparation
+ * imports, but ideally the required modules are already imported and can be re-used
+ * to not incur a structural TypeScript program re-use discarding.
  */
-const TCB_FILE_IMPORT_GRAPH_PREPARE_MODULES = [
+const TCB_FILE_IMPORT_GRAPH_PREPARE_IDENTIFIERS = [
   // Imports may be added for signal input checking. We wouldn't want to change the
   // import graph for incremental compilations when suddenly a signal input is used,
   // or removed.
-  R3Identifiers.InputSignalBrandWriteType.moduleName,
+  R3Identifiers.InputSignalBrandWriteType,
 ];
 
 /**
@@ -194,8 +198,12 @@ function getTemplateId(
  * import graph changes whenever e.g. a signal input is introduced in user code.
  */
 export function ensureTypeCheckFilePreparationImports(env: ReferenceEmitEnvironment): void {
-  for (const moduleName of TCB_FILE_IMPORT_GRAPH_PREPARE_MODULES) {
-    env.importManager.generateNamespaceImport(moduleName);
+  for (const identifier of TCB_FILE_IMPORT_GRAPH_PREPARE_IDENTIFIERS) {
+    env.importManager.addImport({
+      exportModuleSpecifier: identifier.moduleName,
+      exportSymbolName: identifier.name,
+      requestedFile: env.contextFile,
+    });
   }
 }
 

packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts

@@ -8,10 +8,7 @@
 
 import ts from 'typescript';
 
-import {Import} from '../../translator';
-
 import {addExpressionIdentifier, ExpressionIdentifier} from './comments';
-import {wrapForTypeChecker} from './diagnostics';
 
 
 
@@ -163,11 +160,3 @@ export function tsNumericExpression(value: number): ts.NumericLiteral|ts.PrefixU
 
   return ts.factory.createNumericLiteral(value);
 }
-
-export function getImportString(imp: Import): string {
-  if (imp.qualifier === null) {
-    return `import from '${imp.specifier}';`;
-  } else {
-    return `import * as ${imp.qualifier.text} from '${imp.specifier}';`;
-  }
-}

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts

@@ -8,16 +8,15 @@
 import ts from 'typescript';
 
 import {AbsoluteFsPath, join} from '../../file_system';
-import {NoopImportRewriter, Reference, ReferenceEmitter} from '../../imports';
+import {Reference, ReferenceEmitter} from '../../imports';
 import {ClassDeclaration, ReflectionHost} from '../../reflection';
-import {ImportManager} from '../../translator';
+import {ImportManagerV2} from '../../translator';
 import {TypeCheckBlockMetadata, TypeCheckingConfig} from '../api';
 
 import {DomSchemaChecker} from './dom';
 import {Environment} from './environment';
 import {OutOfBandDiagnosticRecorder} from './oob';
 import {ensureTypeCheckFilePreparationImports} from './tcb_util';
-import {getImportString} from './ts_util';
 import {generateTypeCheckBlock, TcbGenericContextBehavior} from './type_check_block';
 
 
@@ -38,7 +37,14 @@ export class TypeCheckFile extends Environment {
       readonly fileName: AbsoluteFsPath, config: TypeCheckingConfig, refEmitter: ReferenceEmitter,
       reflector: ReflectionHost, compilerHost: Pick<ts.CompilerHost, 'getCanonicalFileName'>) {
     super(
-        config, new ImportManager(new NoopImportRewriter(), 'i'), refEmitter, reflector,
+        config, new ImportManagerV2({
+          // This minimizes noticeable changes with older versions of `ImportManager`.
+          forceGenerateNamespacesForNewImports: true,
+          // Type check block code affects code completion and fix suggestions.
+          // We want to encourage single quotes for now, like we always did.
+          shouldUseSingleQuotes: () => true
+        }),
+        refEmitter, reflector,
         ts.createSourceFile(
             compilerHost.getCanonicalFileName(fileName), '', ts.ScriptTarget.Latest, true));
   }
@@ -60,11 +66,21 @@ export class TypeCheckFile extends Environment {
     // import to e.g. `@angular/core` always exists to guarantee a stable graph.
     ensureTypeCheckFilePreparationImports(this);
 
-    let source: string = this.importManager.getAllImports(this.contextFile.fileName)
-                             .map(getImportString)
-                             .join('\n') +
-        '\n\n';
+    const importChanges = this.importManager.finalize();
+    if (importChanges.updatedImports.size > 0) {
+      throw new Error(
+          'AssertionError: Expected no imports to be updated for a new type check file.');
+    }
+
     const printer = ts.createPrinter({removeComments});
+    let source = '';
+
+    const newImports = importChanges.newImports.get(this.contextFile.fileName);
+    if (newImports !== undefined) {
+      source += newImports.map(i => printer.printNode(ts.EmitHint.Unspecified, i, this.contextFile))
+                    .join('\n');
+    }
+
     source += '\n';
     for (const stmt of this.pipeInstStatements) {
       source += printer.printNode(ts.EmitHint.Unspecified, stmt, this.contextFile) + '\n';

yarn.lock

@@ -386,7 +386,6 @@
 
 "@angular/build-tooling@https://github.com/angular/dev-infra-private-build-tooling-builds.git#65d002a534a74daa3c9bd26bdea5a092cbd519df":
   version "0.0.0-5774b71c01a55c4c998f858ee37d3b77ae704c31"
-  uid "65d002a534a74daa3c9bd26bdea5a092cbd519df"
   resolved "https://github.com/angular/dev-infra-private-build-tooling-builds.git#65d002a534a74daa3c9bd26bdea5a092cbd519df"
   dependencies:
     "@angular-devkit/build-angular" "17.2.0-rc.0"
@@ -495,7 +494,6 @@
 
 "@angular/docs@https://github.com/angular/dev-infra-private-docs-builds.git#82c4573f5c9d4fb864271a1c74259fc251457e2f":
   version "0.0.0-5774b71c01a55c4c998f858ee37d3b77ae704c31"
-  uid "82c4573f5c9d4fb864271a1c74259fc251457e2f"
   resolved "https://github.com/angular/dev-infra-private-docs-builds.git#82c4573f5c9d4fb864271a1c74259fc251457e2f"
   dependencies:
     "@angular/cdk" "17.3.0-next.0"
@@ -4876,8 +4874,7 @@
     "@types/glob" "*"
     "@types/node" "*"
 
-"@types/selenium-webdriver4@npm:@types/selenium-webdriver@4.1.21", "@types/selenium-webdriver@^4.1.21":
-  name "@types/selenium-webdriver4"
+"@types/selenium-webdriver4@npm:@types/selenium-webdriver@4.1.21":
   version "4.1.21"
   resolved "https://registry.yarnpkg.com/@types/selenium-webdriver/-/selenium-webdriver-4.1.21.tgz#79fe31faf9953a4143c3e32944d98d5146bbe185"
   integrity sha512-QGURnImvxYlIQz5DVhvHdqpYNLBjhJ2Vm+cnQI2G9QZzkWlZm0LkLcvDcHp+qE6N2KBz4CeuvXgPO7W3XQ0Tyw==
@@ -4894,6 +4891,14 @@
   resolved "https://registry.yarnpkg.com/@types/selenium-webdriver/-/selenium-webdriver-3.0.25.tgz#7982bf5add9539b4e953512354055545e8508cff"
   integrity sha512-gkb8rx0kjHuaVUzIfKa1tVVjnEmjYLufDmoOHKPsaoRraTcNjV9W9ruFWi3Xv821c7M5gZVmrGOT8BJYsY/acQ==
 
+"@types/selenium-webdriver@^4.1.21":
+  name "@types/selenium-webdriver4"
+  version "4.1.22"
+  resolved "https://registry.yarnpkg.com/@types/selenium-webdriver/-/selenium-webdriver-4.1.22.tgz#344519b90727eb713e1ce6d2e0198eb0b4f8f316"
+  integrity sha512-MCL4l7q8dwxejr2Q2NXLyNwHWMPdlWE0Kpn6fFwJtvkJF7PTkG5jkvbH/X1IAAQxgt/L1dA8u2GtDeekvSKvOA==
+  dependencies:
+    "@types/ws" "*"
+
 "@types/semver@^7.3.4":
   version "7.5.4"
   resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.5.4.tgz#0a41252ad431c473158b22f9bfb9a63df7541cff"
@@ -12480,7 +12485,7 @@ magic-string@0.30.7:
   dependencies:
     "@jridgewell/sourcemap-codec" "^1.4.15"
 
-magic-string@0.30.8:
+magic-string@0.30.8, magic-string@^0.30.8:
   version "0.30.8"
   resolved "https://registry.yarnpkg.com/magic-string/-/magic-string-0.30.8.tgz#14e8624246d2bedba70d5462aa99ac9681844613"
   integrity sha512-ISQTe55T2ao7XtlAStud6qwYPZjE4GK1S/BeVPus4jrq6JuOnQ00YKQC581RWhR122W7msZV263KzVeLoqidyQ==