fix(compiler-cli): ensure the compiler tracks ts.Programs correctly (…

…#41291)

`NgCompiler` previously had a notion of the "next" `ts.Program`, which
served two purposes:

* it allowed a client using the `ts.createProgram` API to query for the
  latest program produced by the previous `NgCompiler`, as a starting
  point for building the _next_ program that incorporated any new user
  changes.

* it allowed the old `NgCompiler` to be queried for the `ts.Program` on
  which all prior state is based, which is needed to compute the delta
  from the new program to ultimately determine how much of the prior
  state can be reused.

This system contained a flaw: it relied on the `NgCompiler` knowing when
the `ts.Program` would be changed. This works fine for changes that
originate in `NgCompiler` APIs, but a client of the `TemplateTypeChecker`
may use that API in ways that create new `ts.Program`s without the
`NgCompiler`'s knowledge. This caused the `NgCompiler`'s concept of the
"next" program to get out of sync, causing incorrectness in future
incremental analysis.

This refactoring cleans up the compiler's `ts.Program` management in
several ways:

* `TypeCheckingProgramStrategy`, the API which controls `ts.Program`
  updating, is renamed to the `ProgramDriver` and extracted to a separate
  ngtsc package.

* It loses its responsibility of determining component shim filenames. That
  functionality now lives exclusively in the template type-checking package.

* The "next" `ts.Program` concept is renamed to the "current" program, as
  the "next" name was misleading in several ways.

* `NgCompiler` now wraps the `ProgramDriver` used in the
  `TemplateTypeChecker` to know when a new `ts.Program` is created,
  regardless of which API drove the creation, which actually fixes the bug.

PR Close #41291

packages/compiler-cli/BUILD.bazel

@@ -33,6 +33,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/incremental",
         "//packages/compiler-cli/src/ngtsc/indexer",
         "//packages/compiler-cli/src/ngtsc/perf",
+        "//packages/compiler-cli/src/ngtsc/program_driver",
         "//packages/compiler-cli/src/ngtsc/reflection",
         "//packages/compiler-cli/src/ngtsc/shims",
         "//packages/compiler-cli/src/ngtsc/translator",

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

@@ -26,6 +26,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/modulewithproviders",
         "//packages/compiler-cli/src/ngtsc/partial_evaluator",
         "//packages/compiler-cli/src/ngtsc/perf",
+        "//packages/compiler-cli/src/ngtsc/program_driver",
         "//packages/compiler-cli/src/ngtsc/reflection",
         "//packages/compiler-cli/src/ngtsc/resource",
         "//packages/compiler-cli/src/ngtsc/routing",

packages/compiler-cli/src/ngtsc/core/test/BUILD.bazel

@@ -15,6 +15,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/file_system",
         "//packages/compiler-cli/src/ngtsc/file_system/testing",
         "//packages/compiler-cli/src/ngtsc/incremental",
+        "//packages/compiler-cli/src/ngtsc/program_driver",
         "//packages/compiler-cli/src/ngtsc/reflection",
         "//packages/compiler-cli/src/ngtsc/typecheck",
         "//packages/compiler-cli/src/ngtsc/typecheck/api",

packages/compiler-cli/src/ngtsc/core/test/compiler_test.ts

@@ -11,9 +11,9 @@ import * as ts from 'typescript';
 import {absoluteFrom as _, FileSystem, getFileSystem, getSourceFileOrError, NgtscCompilerHost, setFileSystem} from '../../file_system';
 import {runInEachFileSystem} from '../../file_system/testing';
 import {IncrementalBuildStrategy, NoopIncrementalBuildStrategy} from '../../incremental';
+import {ProgramDriver, TsCreateProgramDriver} from '../../program_driver';
 import {ClassDeclaration, isNamedClassDeclaration} from '../../reflection';
-import {ReusedProgramStrategy} from '../../typecheck';
-import {OptimizeFor, TypeCheckingProgramStrategy} from '../../typecheck/api';
+import {OptimizeFor} from '../../typecheck/api';
 
 import {NgCompilerOptions} from '../api';
 
@@ -22,7 +22,7 @@ import {NgCompilerHost} from '../src/host';
 
 function makeFreshCompiler(
     host: NgCompilerHost, options: NgCompilerOptions, program: ts.Program,
-    programStrategy: TypeCheckingProgramStrategy, incrementalStrategy: IncrementalBuildStrategy,
+    programStrategy: ProgramDriver, incrementalStrategy: IncrementalBuildStrategy,
     enableTemplateTypeChecker: boolean, usePoisonedData: boolean): NgCompiler {
   const ticket = freshCompilationTicket(
       program, options, incrementalStrategy, programStrategy, /* perfRecorder */ null,
@@ -61,7 +61,7 @@ runInEachFileSystem(() => {
       const host = NgCompilerHost.wrap(baseHost, [COMPONENT], options, /* oldProgram */ null);
       const program = ts.createProgram({host, options, rootNames: host.inputFiles});
       const compiler = makeFreshCompiler(
-          host, options, program, new ReusedProgramStrategy(program, host, options, []),
+          host, options, program, new TsCreateProgramDriver(program, host, options, []),
           new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
           /* usePoisonedData */ false);
 
@@ -113,7 +113,7 @@ runInEachFileSystem(() => {
         const CmpC = getClass(getSourceFileOrError(program, cmpCFile), 'CmpC');
 
         const compiler = makeFreshCompiler(
-            host, options, program, new ReusedProgramStrategy(program, host, options, []),
+            host, options, program, new TsCreateProgramDriver(program, host, options, []),
             new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
             /* usePoisonedData */ false);
         const components = compiler.getComponentsWithTemplateFile(templateFile);
@@ -165,7 +165,7 @@ runInEachFileSystem(() => {
         const CmpA = getClass(getSourceFileOrError(program, cmpAFile), 'CmpA');
         const CmpC = getClass(getSourceFileOrError(program, cmpCFile), 'CmpC');
         const compiler = makeFreshCompiler(
-            host, options, program, new ReusedProgramStrategy(program, host, options, []),
+            host, options, program, new TsCreateProgramDriver(program, host, options, []),
             new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
             /* usePoisonedData */ false);
         const components = compiler.getComponentsWithStyleFile(styleFile);
@@ -199,7 +199,7 @@ runInEachFileSystem(() => {
         const program = ts.createProgram({host, options, rootNames: host.inputFiles});
         const CmpA = getClass(getSourceFileOrError(program, cmpAFile), 'CmpA');
         const compiler = makeFreshCompiler(
-            host, options, program, new ReusedProgramStrategy(program, host, options, []),
+            host, options, program, new TsCreateProgramDriver(program, host, options, []),
             new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
             /* usePoisonedData */ false);
         const resources = compiler.getComponentResources(CmpA);
@@ -235,7 +235,7 @@ runInEachFileSystem(() => {
         const program = ts.createProgram({host, options, rootNames: host.inputFiles});
         const CmpA = getClass(getSourceFileOrError(program, cmpAFile), 'CmpA');
         const compiler = makeFreshCompiler(
-            host, options, program, new ReusedProgramStrategy(program, host, options, []),
+            host, options, program, new TsCreateProgramDriver(program, host, options, []),
             new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
             /* usePoisonedData */ false);
         const resources = compiler.getComponentResources(CmpA);
@@ -267,7 +267,7 @@ runInEachFileSystem(() => {
         const host = NgCompilerHost.wrap(baseHost, [COMPONENT], options, /* oldProgram */ null);
         const program = ts.createProgram({host, options, rootNames: host.inputFiles});
         const compiler = makeFreshCompiler(
-            host, options, program, new ReusedProgramStrategy(program, host, options, []),
+            host, options, program, new TsCreateProgramDriver(program, host, options, []),
             new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
             /* usePoisonedData */ false);
 
@@ -301,7 +301,7 @@ runInEachFileSystem(() => {
         const host = NgCompilerHost.wrap(baseHost, [COMPONENT], options, /* oldProgram */ null);
         const program = ts.createProgram({host, options, rootNames: host.inputFiles});
         const compilerA = makeFreshCompiler(
-            host, options, program, new ReusedProgramStrategy(program, host, options, []),
+            host, options, program, new TsCreateProgramDriver(program, host, options, []),
             new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false,
             /* usePoisonedData */ false);
 

packages/compiler-cli/src/ngtsc/program.ts

@@ -18,9 +18,9 @@ import {absoluteFrom, AbsoluteFsPath, getFileSystem} from './file_system';
 import {TrackedIncrementalBuildStrategy} from './incremental';
 import {IndexedComponent} from './indexer';
 import {ActivePerfRecorder, PerfCheckpoint as PerfCheckpoint, PerfEvent, PerfPhase} from './perf';
+import {TsCreateProgramDriver} from './program_driver';
 import {DeclarationNode} from './reflection';
 import {retagAllTsFiles, untagAllTsFiles} from './shims';
-import {ReusedProgramStrategy} from './typecheck';
 import {OptimizeFor} from './typecheck/api';
 
 
@@ -95,7 +95,7 @@ export class NgtscProgram implements api.Program {
     // the program.
     untagAllTsFiles(this.tsProgram);
 
-    const reusedProgramStrategy = new ReusedProgramStrategy(
+    const programDriver = new TsCreateProgramDriver(
         this.tsProgram, this.host, this.options, this.host.shimExtensionPrefixes);
 
     this.incrementalStrategy = oldProgram !== undefined ?
@@ -114,14 +114,14 @@ export class NgtscProgram implements api.Program {
     let ticket: CompilationTicket;
     if (oldProgram === undefined) {
       ticket = freshCompilationTicket(
-          this.tsProgram, options, this.incrementalStrategy, reusedProgramStrategy, perfRecorder,
+          this.tsProgram, options, this.incrementalStrategy, programDriver, perfRecorder,
           /* enableTemplateTypeChecker */ false, /* usePoisonedData */ false);
     } else {
       ticket = incrementalFromCompilerTicket(
           oldProgram.compiler,
           this.tsProgram,
           this.incrementalStrategy,
-          reusedProgramStrategy,
+          programDriver,
           modifiedResourceFiles,
           perfRecorder,
       );
@@ -223,7 +223,7 @@ export class NgtscProgram implements api.Program {
     const diagnostics = sf === undefined ?
         this.compiler.getDiagnostics() :
         this.compiler.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram);
-    this.reuseTsProgram = this.compiler.getNextProgram();
+    this.reuseTsProgram = this.compiler.getCurrentProgram();
     return diagnostics;
   }
 

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

@@ -0,0 +1,17 @@
+load("//tools:defaults.bzl", "ts_library")
+
+package(default_visibility = ["//visibility:public"])
+
+ts_library(
+    name = "program_driver",
+    srcs = ["index.ts"] + glob([
+        "src/*.ts",
+    ]),
+    module_name = "@angular/compiler-cli/src/ngtsc/program_driver",
+    deps = [
+        "//packages/compiler-cli/src/ngtsc/file_system",
+        "//packages/compiler-cli/src/ngtsc/shims",
+        "//packages/compiler-cli/src/ngtsc/util",
+        "@npm//typescript",
+    ],
+)

packages/compiler-cli/src/ngtsc/program_driver/README.md

@@ -0,0 +1,18 @@
+# The Program Driver interface
+
+`ProgramDriver` is a small but important interface which allows the template type-checking machinery to request changes to the current `ts.Program`, and to receive a new `ts.Program` with those changes applied. This is used to add template type-checking code to the current `ts.Program`, eventually allowing for diagnostics to be produced within that code. This operation is abstracted behind this interface because different clients create `ts.Program`s differently. The Language Service, for example, creates `ts.Program`s from the current editor state on request, while the TS compiler API creates them explicitly.
+
+When running using the TS APIs, it's important that each new `ts.Program` be created incrementally from the previous `ts.Program`. Under a normal compilation, this means that programs alternate between template type checking programs and user programs:
+
+* `ts.Program#1` is created from user input (the user's source files).
+* `ts.Program#2` is created incrementally on top of #1 for template type-checking, and adds private TCB code.
+* `ts.Program#3` is created incrementally on top of #2 when the user makes changes to files on disk (incremental build).
+* `ts.Program#4` is created incrementally on top of #3 to adjust template type-checking code according to the user's changes.
+
+The `TsCreateProgramDriver` performs this operation for template type-checking `ts.Program`s built by the command-line compiler or by the CLI. The latest template type-checking program is then exposed via the `NgCompiler`'s `getCurrentProgram()` operation, and new user programs are expected to be created incrementally on top of the previous template type-checking program.
+
+## Programs and the compiler as a service
+
+Not all clients of the compiler follow the incremental tick-tock scenario above. When the compiler is used as a service, new `ts.Program`s may be generated in response to various queries, either directly to `NgCompiler` or via the `TemplateTypeChecker`. Internally, the compiler will use the current `ProgramDriver` to create these additional `ts.Program`s as needed.
+
+Incremental builds (new user code changes) may also require changing the `ts.Program`, using the compiler's incremental ticket process. If the `TsCreateProgramDriver` is used, the client is responsible for ensuring that any new incremental `ts.Program`s are created on top of the current program from the previous compilation, which can be obtained via `NgCompiler`'s `getCurrentProgram()`.

packages/compiler-cli/src/ngtsc/program_driver/index.ts

@@ -0,0 +1,10 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+export * from './src/api';
+export {TsCreateProgramDriver} from './src/ts_create_program_driver';

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

@@ -0,0 +1,44 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import * as ts from 'typescript';
+import {AbsoluteFsPath} from '../../file_system';
+
+export interface ProgramDriver {
+  /**
+   * Whether this strategy supports modifying user files (inline modifications) in addition to
+   * modifying type-checking shims.
+   */
+  readonly supportsInlineOperations: boolean;
+
+  /**
+   * Retrieve the latest version of the program, containing all the updates made thus far.
+   */
+  getProgram(): ts.Program;
+
+  /**
+   * Incorporate a set of changes to either augment or completely replace the type-checking code
+   * included in the type-checking program.
+   */
+  updateFiles(contents: Map<AbsoluteFsPath, string>, updateMode: UpdateMode): void;
+}
+
+export enum UpdateMode {
+  /**
+   * A complete update creates a completely new overlay of type-checking code on top of the user's
+   * original program, which doesn't include type-checking code from previous calls to
+   * `updateFiles`.
+   */
+  Complete,
+
+  /**
+   * An incremental update changes the contents of some files in the type-checking program without
+   * reverting any prior changes.
+   */
+  Incremental,
+}

packages/compiler-cli/src/ngtsc/typecheck/src/host.ts → packages/compiler-cli/src/ngtsc/program_driver/src/ts_create_program_driver.ts

@@ -8,9 +8,20 @@
 
 import * as ts from 'typescript';
 
-import {copyFileShimData, ShimReferenceTagger} from '../../shims';
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {AbsoluteFsPath} from '../../file_system';
+import {copyFileShimData, retagAllTsFiles, ShimReferenceTagger, untagAllTsFiles} from '../../shims';
 import {RequiredDelegations} from '../../util/src/typescript';
 
+import {ProgramDriver, UpdateMode} from './api';
+
 /**
  * Delegates all methods of `ts.CompilerHost` to a delegate, with the exception of
  * `getSourceFile`, `fileExists` and `writeFile` which are implemented in `TypeCheckProgramHost`.
@@ -51,10 +62,9 @@ export class DelegatingCompilerHost implements
 }
 
 /**
- * A `ts.CompilerHost` which augments source files with type checking code from a
- * `TypeCheckContext`.
+ * A `ts.CompilerHost` which augments source files.
  */
-export class TypeCheckProgramHost extends DelegatingCompilerHost {
+class UpdatedProgramHost extends DelegatingCompilerHost {
   /**
    * Map of source file names to `ts.SourceFile` instances.
    */
@@ -63,12 +73,12 @@ export class TypeCheckProgramHost extends DelegatingCompilerHost {
   /**
    * The `ShimReferenceTagger` responsible for tagging `ts.SourceFile`s loaded via this host.
    *
-   * The `TypeCheckProgramHost` is used in the creation of a new `ts.Program`. Even though this new
+   * The `UpdatedProgramHost` is used in the creation of a new `ts.Program`. Even though this new
    * program is based on a prior one, TypeScript will still start from the root files and enumerate
    * all source files to include in the new program.  This means that just like during the original
    * program's creation, these source files must be tagged with references to per-file shims in
    * order for those shims to be loaded, and then cleaned up afterwards. Thus the
-   * `TypeCheckProgramHost` has its own `ShimReferenceTagger` to perform this function.
+   * `UpdatedProgramHost` has its own `ShimReferenceTagger` to perform this function.
    */
   private shimTagger = new ShimReferenceTagger(this.shimExtensionPrefixes);
 
@@ -127,3 +137,72 @@ export class TypeCheckProgramHost extends DelegatingCompilerHost {
     return this.sfMap.has(fileName) || this.delegate.fileExists(fileName);
   }
 }
+
+
+/**
+ * Updates a `ts.Program` instance with a new one that incorporates specific changes, using the
+ * TypeScript compiler APIs for incremental program creation.
+ */
+export class TsCreateProgramDriver implements ProgramDriver {
+  /**
+   * A map of source file paths to replacement `ts.SourceFile`s for those paths.
+   *
+   * Effectively, this tracks the delta between the user's program (represented by the
+   * `originalHost`) and the template type-checking program being managed.
+   */
+  private sfMap = new Map<string, ts.SourceFile>();
+
+  private program: ts.Program = this.originalProgram;
+
+  constructor(
+      private originalProgram: ts.Program, private originalHost: ts.CompilerHost,
+      private options: ts.CompilerOptions, private shimExtensionPrefixes: string[]) {}
+
+  readonly supportsInlineOperations = true;
+
+  getProgram(): ts.Program {
+    return this.program;
+  }
+
+  updateFiles(contents: Map<AbsoluteFsPath, string>, updateMode: UpdateMode): void {
+    if (contents.size === 0) {
+      // No changes have been requested. Is it safe to skip updating entirely?
+      // If UpdateMode is Incremental, then yes. If UpdateMode is Complete, then it's safe to skip
+      // only if there are no active changes already (that would be cleared by the update).
+
+      if (updateMode !== UpdateMode.Complete || this.sfMap.size === 0) {
+        // No changes would be made to the `ts.Program` anyway, so it's safe to do nothing here.
+        return;
+      }
+    }
+
+    if (updateMode === UpdateMode.Complete) {
+      this.sfMap.clear();
+    }
+
+    for (const [filePath, text] of contents.entries()) {
+      this.sfMap.set(filePath, ts.createSourceFile(filePath, text, ts.ScriptTarget.Latest, true));
+    }
+
+    const host = new UpdatedProgramHost(
+        this.sfMap, this.originalProgram, this.originalHost, this.shimExtensionPrefixes);
+    const oldProgram = this.program;
+
+    // Retag the old program's `ts.SourceFile`s with shim tags, to allow TypeScript to reuse the
+    // most data.
+    retagAllTsFiles(oldProgram);
+
+    this.program = ts.createProgram({
+      host,
+      rootNames: this.program.getRootFileNames(),
+      options: this.options,
+      oldProgram,
+    });
+    host.postProgramCreationCleanup();
+
+    // And untag them afterwards. We explicitly untag both programs here, because the oldProgram
+    // may still be used for emit and needs to not contain tags.
+    untagAllTsFiles(this.program);
+    untagAllTsFiles(oldProgram);
+  }
+}