fix(ivy): always re-analyze the program during incremental rebuilds (#33862)

Previously, the ngtsc compiler attempted to reuse analysis work from the
previous program during an incremental build. To do this, it had to prove
that the work was safe to reuse - that no changes made to the new program
would invalidate the previous analysis.

The implementation of this had a significant design flaw: if the previous
program had errors, the previous analysis would be missing significant
information, and the dependency graph extracted from it would not be
sufficient to determine which files should be re-analyzed to fill in the
gaps. This often meant that the build output after an error was resolved
would be wholly incorrect.

This commit switches ngtsc to take a simpler approach to incremental
rebuilds. Instead of attempting to reuse prior analysis work, the entire
program is re-analyzed with each compilation. This is actually not as
expensive as one might imagine - analysis is a fairly small part of overall
compilation time.

Based on the dependency graph extracted during this analysis, the compiler
then can make accurate decisions on whether to emit specific files. A new
suite of tests is added to validate behavior in the presence of source code
level errors.

This new approach is dramatically simpler than the previous algorithm, and
should always produce correct results for a semantically correct program.s

Fixes #32388
Fixes #32214

PR Close #33862

Author: alxhub

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

@@ -2,7 +2,7 @@
 
 This package contains logic related to incremental compilation in ngtsc.
 
-In particular, it tracks metadata for `ts.SourceFile`s in between compilations, so the compiler can make intelligent decisions about when to skip certain operations and rely on previously analyzed data.
+In particular, it tracks dependencies between `ts.SourceFile`s, so the compiler can make intelligent decisions about when it's safe to skip certain operations.
 
 # How does incremental compilation work?
 
@@ -14,30 +14,36 @@ This information is leveraged in two major ways:
 
 1) The previous `ts.Program` itself is used to create the next `ts.Program`, allowing TypeScript internally to leverage information from the previous compile in much the same way.
 
-2) An `IncrementalState` instance is constructed from the previous compilation's `IncrementalState` and its `ts.Program`.
+2) An `IncrementalState` instance is constructed from the old and new `ts.Program`s.
 
-After this initialization, the `IncrementalState` contains the knowledge from the previous compilation which will be used to optimize the next one.
+The compiler then proceeds normally, analyzing all of the Angular code within the program. As a part of this process, the compiler maps out all of the dependencies between files in the `IncrementalState`.
 
-# What optimizations can be made?
+# What optimizations are made?
 
-Currently, ngtsc makes a decision to skip the emit of a file if it can prove that the contents of the file will not have changed. To prove this, two conditions must be true.
+ngtsc makes a decision to skip the emit of a file if it can prove that the contents of the file will not have changed. To prove this, two conditions must be true.
 
 * The input file itself must not have changed since the previous compilation.
 
-* As a result of analyzing the file, no dependencies must exist where the output of compilation could vary depending on the contents of any other file.
+* None of the files on which the input file is dependent have changed since the previous compilation.
 
-The second condition is challenging, as Angular allows statically evaluated expressions in lots of contexts that could result in changes from file to file. For example, the `name` of an `@Pipe` could be a reference to a constant in a different file.
+The second condition is challenging to prove, as Angular allows statically evaluated expressions in lots of contexts that could result in changes from file to file. For example, the `name` of an `@Pipe` could be a reference to a constant in a different file. As part of analyzing the program, the compiler keeps track of such dependencies in order to answer this question.
 
-Therefore, only two types of files meet these conditions and can be optimized today:
-
-* Files with no Angular decorated classes at all.
-
-* Files with only `@Injectable`s.
+The emit of a file is the most expensive part of TypeScript/Angular compilation, so skipping emits when they are not necessary is one of the most valuable things the compiler can do to improve incremental build performance.
 
 # What optimizations are possible in the future?
 
 There is plenty of room for improvement here, with diminishing returns for the work involved.
 
-* The compiler could track the dependencies of each file being compiled, and know whether an `@Pipe` gets its name from a second file, for example. This is sufficient to skip the analysis and emit of more files when none of the dependencies have changed.
+## Optimization of re-analysis
+
+Currently, the compiler re-analyzes the entire `ts.Program` on each compilation. Under certain circumstances it may be possible for the compiler to reuse parts of the previous compilation's analysis rather than repeat the work, if it can be proven to be safe.
+
+## Semantic dependency tracking
+
+Currently the compiler tracks dependencies only at the file level, and will re-emit dependent files if they _may_ have been affected by a change. Often times a change though does _not_ require updates to dependent files.
+
+For example, today a component's `NgModule` and all of the other components which consume that module's export scope are considered to depend on the component file itself. If the component's template changes, this triggers a re-emit of not only the component's file, but the entire chain of its NgModule and that module's export scope. This happens even though the template of a component _does not have any impact_ on any components which consume it - these other emits are deeply unnecessary.
+
+In contrast, if the component's _selector_ changes, then all those dependent files do need to be updated since their `directiveDefs` might have changed.
 
-* The compiler could also perform analysis on files which _have_ changed dependencies, and skip emit if the analysis indicates nothing has changed which would affect the file being emitted.
+Currently the compiler does not distinguish these two cases, and conservatively always re-emits the entire NgModule chain. It would be possible to break the dependency graph down into finer-grained nodes and distinguish between updates that affect the component, vs updates that affect its dependents. This would be a huge win, but is exceedingly complex.

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

@@ -8,47 +8,33 @@
 
 import * as ts from 'typescript';
 
-import {Reference} from '../../imports';
-import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata';
 import {DependencyTracker} from '../../partial_evaluator';
-import {ClassDeclaration} from '../../reflection';
-import {ComponentScopeReader, ComponentScopeRegistry, LocalModuleScope} from '../../scope';
 import {ResourceDependencyRecorder} from '../../util/src/resource_recorder';
 
 /**
  * Accumulates state between compilations.
  */
-export class IncrementalState implements DependencyTracker, MetadataReader, MetadataRegistry,
-    ResourceDependencyRecorder, ComponentScopeRegistry, ComponentScopeReader {
+export class IncrementalState implements DependencyTracker, ResourceDependencyRecorder {
   private constructor(
       private unchangedFiles: Set<ts.SourceFile>,
       private metadata: Map<ts.SourceFile, FileMetadata>,
       private modifiedResourceFiles: Set<string>|null) {}
 
   static reconcile(
-      previousState: IncrementalState, oldProgram: ts.Program, newProgram: ts.Program,
+      oldProgram: ts.Program, newProgram: ts.Program,
       modifiedResourceFiles: Set<string>|null): IncrementalState {
     const unchangedFiles = new Set<ts.SourceFile>();
     const metadata = new Map<ts.SourceFile, FileMetadata>();
     const oldFiles = new Set<ts.SourceFile>(oldProgram.getSourceFiles());
-    const newFiles = new Set<ts.SourceFile>(newProgram.getSourceFiles());
 
     // Compute the set of files that are unchanged (both in themselves and their dependencies).
     for (const newFile of newProgram.getSourceFiles()) {
-      if (oldFiles.has(newFile)) {
-        const oldDeps = previousState.getFileDependencies(newFile);
-        if (oldDeps.every(oldDep => newFiles.has(oldDep))) {
-          // The file and its dependencies are unchanged.
-          unchangedFiles.add(newFile);
-          // Copy over its metadata too
-          const meta = previousState.metadata.get(newFile);
-          if (meta) {
-            metadata.set(newFile, meta);
-          }
-        }
-      } else if (newFile.isDeclarationFile) {
-        // A typings file has changed so trigger a full rebuild of the Angular analyses
+      if (newFile.isDeclarationFile && !oldFiles.has(newFile)) {
+        // Bail out and re-emit everything if a .d.ts file has changed - currently the compiler does
+        // not track dependencies into .d.ts files very well.
         return IncrementalState.fresh();
+      } else if (oldFiles.has(newFile)) {
+        unchangedFiles.add(newFile);
       }
     }
 
@@ -60,8 +46,13 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta
         new Set<ts.SourceFile>(), new Map<ts.SourceFile, FileMetadata>(), null);
   }
 
-  safeToSkip(sf: ts.SourceFile): boolean|Promise<boolean> {
-    return this.unchangedFiles.has(sf) && !this.hasChangedResourceDependencies(sf);
+  safeToSkip(sf: ts.SourceFile): boolean {
+    // It's safe to skip emitting a file if:
+    // 1) it hasn't changed
+    // 2) none if its resource dependencies have changed
+    // 3) none of its source dependencies have changed
+    return this.unchangedFiles.has(sf) && !this.hasChangedResourceDependencies(sf) &&
+        this.getFileDependencies(sf).every(dep => this.unchangedFiles.has(dep));
   }
 
   trackFileDependency(dep: ts.SourceFile, src: ts.SourceFile) {
@@ -84,93 +75,11 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta
     return Array.from(meta.fileDependencies);
   }
 
-  getNgModuleMetadata(ref: Reference<ClassDeclaration>): NgModuleMeta|null {
-    if (!this.metadata.has(ref.node.getSourceFile())) {
-      return null;
-    }
-    const metadata = this.metadata.get(ref.node.getSourceFile()) !;
-    if (!metadata.ngModuleMeta.has(ref.node)) {
-      return null;
-    }
-    return metadata.ngModuleMeta.get(ref.node) !;
-  }
-
-  registerNgModuleMetadata(meta: NgModuleMeta): void {
-    const metadata = this.ensureMetadata(meta.ref.node.getSourceFile());
-    metadata.ngModuleMeta.set(meta.ref.node, meta);
-  }
-
-  getDirectiveMetadata(ref: Reference<ClassDeclaration>): DirectiveMeta|null {
-    if (!this.metadata.has(ref.node.getSourceFile())) {
-      return null;
-    }
-    const metadata = this.metadata.get(ref.node.getSourceFile()) !;
-    if (!metadata.directiveMeta.has(ref.node)) {
-      return null;
-    }
-    return metadata.directiveMeta.get(ref.node) !;
-  }
-
-  registerDirectiveMetadata(meta: DirectiveMeta): void {
-    const metadata = this.ensureMetadata(meta.ref.node.getSourceFile());
-    metadata.directiveMeta.set(meta.ref.node, meta);
-  }
-
-  getPipeMetadata(ref: Reference<ClassDeclaration>): PipeMeta|null {
-    if (!this.metadata.has(ref.node.getSourceFile())) {
-      return null;
-    }
-    const metadata = this.metadata.get(ref.node.getSourceFile()) !;
-    if (!metadata.pipeMeta.has(ref.node)) {
-      return null;
-    }
-    return metadata.pipeMeta.get(ref.node) !;
-  }
-
-  registerPipeMetadata(meta: PipeMeta): void {
-    const metadata = this.ensureMetadata(meta.ref.node.getSourceFile());
-    metadata.pipeMeta.set(meta.ref.node, meta);
-  }
-
   recordResourceDependency(file: ts.SourceFile, resourcePath: string): void {
     const metadata = this.ensureMetadata(file);
     metadata.resourcePaths.add(resourcePath);
   }
 
-  registerComponentScope(clazz: ClassDeclaration, scope: LocalModuleScope): void {
-    const metadata = this.ensureMetadata(clazz.getSourceFile());
-    metadata.componentScope.set(clazz, scope);
-  }
-
-  getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
-    if (!this.metadata.has(clazz.getSourceFile())) {
-      return null;
-    }
-    const metadata = this.metadata.get(clazz.getSourceFile()) !;
-    if (!metadata.componentScope.has(clazz)) {
-      return null;
-    }
-    return metadata.componentScope.get(clazz) !;
-  }
-
-  setComponentAsRequiringRemoteScoping(clazz: ClassDeclaration): void {
-    const metadata = this.ensureMetadata(clazz.getSourceFile());
-    metadata.remoteScoping.add(clazz);
-  }
-
-  getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null {
-    // TODO: https://angular-team.atlassian.net/browse/FW-1501
-    // Handle the incremental build case where a component requires remote scoping.
-    // This means that if the the component's template changes, it requires the module to be
-    // re-emitted.
-    // Also, we need to make sure the cycle detector works well across rebuilds.
-    if (!this.metadata.has(clazz.getSourceFile())) {
-      return null;
-    }
-    const metadata = this.metadata.get(clazz.getSourceFile()) !;
-    return metadata.remoteScoping.has(clazz);
-  }
-
   private ensureMetadata(sf: ts.SourceFile): FileMetadata {
     const metadata = this.metadata.get(sf) || new FileMetadata();
     this.metadata.set(sf, metadata);
@@ -194,9 +103,4 @@ class FileMetadata {
   /** A set of source files that this file depends upon. */
   fileDependencies = new Set<ts.SourceFile>();
   resourcePaths = new Set<string>();
-  directiveMeta = new Map<ClassDeclaration, DirectiveMeta>();
-  ngModuleMeta = new Map<ClassDeclaration, NgModuleMeta>();
-  pipeMeta = new Map<ClassDeclaration, PipeMeta>();
-  componentScope = new Map<ClassDeclaration, LocalModuleScope>();
-  remoteScoping = new Set<ClassDeclaration>();
 }

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

@@ -28,7 +28,7 @@ import {NOOP_PERF_RECORDER, PerfRecorder, PerfTracker} from './perf';
 import {TypeScriptReflectionHost} from './reflection';
 import {HostResourceLoader} from './resource_loader';
 import {NgModuleRouteAnalyzer, entryPointKeyFor} from './routing';
-import {CompoundComponentScopeReader, LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from './scope';
+import {ComponentScopeReader, CompoundComponentScopeReader, LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from './scope';
 import {FactoryGenerator, FactoryInfo, GeneratedShimsHostWrapper, ShimGenerator, SummaryGenerator, TypeCheckShimGenerator, generatedFactoryTransform} from './shims';
 import {ivySwitchTransform} from './switch';
 import {IvyCompilation, declarationTransformFactory, ivyTransformFactory} from './transform';
@@ -186,8 +186,7 @@ export class NgtscProgram implements api.Program {
       this.incrementalState = IncrementalState.fresh();
     } else {
       this.incrementalState = IncrementalState.reconcile(
-          oldProgram.incrementalState, oldProgram.reuseTsProgram, this.tsProgram,
-          this.modifiedResourceFiles);
+          oldProgram.reuseTsProgram, this.tsProgram, this.modifiedResourceFiles);
     }
   }
 
@@ -309,13 +308,11 @@ export class NgtscProgram implements api.Program {
     if (this.compilation === undefined) {
       const analyzeSpan = this.perfRecorder.start('analyze');
       this.compilation = this.makeCompilation();
-      this.tsProgram.getSourceFiles()
-          .filter(file => !file.fileName.endsWith('.d.ts'))
-          .forEach(file => {
-            const analyzeFileSpan = this.perfRecorder.start('analyzeFile', file);
-            this.compilation !.analyzeSync(file);
-            this.perfRecorder.stop(analyzeFileSpan);
-          });
+      this.tsProgram.getSourceFiles().filter(file => !file.isDeclarationFile).forEach(file => {
+        const analyzeFileSpan = this.perfRecorder.start('analyzeFile', file);
+        this.compilation !.analyzeSync(file);
+        this.perfRecorder.stop(analyzeFileSpan);
+      });
       this.perfRecorder.stop(analyzeSpan);
       this.compilation.resolve();
     }
@@ -576,13 +573,12 @@ export class NgtscProgram implements api.Program {
     const evaluator = new PartialEvaluator(this.reflector, checker, this.incrementalState);
     const dtsReader = new DtsMetadataReader(checker, this.reflector);
     const localMetaRegistry = new LocalMetadataRegistry();
-    const localMetaReader = new CompoundMetadataReader([localMetaRegistry, this.incrementalState]);
+    const localMetaReader: MetadataReader = localMetaRegistry;
     const depScopeReader = new MetadataDtsModuleScopeResolver(dtsReader, this.aliasingHost);
     const scopeRegistry = new LocalModuleScopeRegistry(
-        localMetaReader, depScopeReader, this.refEmitter, this.aliasingHost, this.incrementalState);
-    const scopeReader = new CompoundComponentScopeReader([scopeRegistry, this.incrementalState]);
-    const metaRegistry =
-        new CompoundMetadataRegistry([localMetaRegistry, scopeRegistry, this.incrementalState]);
+        localMetaReader, depScopeReader, this.refEmitter, this.aliasingHost);
+    const scopeReader: ComponentScopeReader = scopeRegistry;
+    const metaRegistry = new CompoundMetadataRegistry([localMetaRegistry, scopeRegistry]);
 
     this.metaReader = new CompoundMetadataReader([localMetaReader, dtsReader]);
 

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

@@ -7,6 +7,6 @@
  */
 
 export {ExportScope, ScopeData} from './src/api';
-export {ComponentScopeReader, ComponentScopeRegistry, CompoundComponentScopeReader} from './src/component_scope';
+export {ComponentScopeReader, CompoundComponentScopeReader} from './src/component_scope';
 export {DtsModuleScopeResolver, MetadataDtsModuleScopeResolver} from './src/dependency';
 export {LocalModuleScope, LocalModuleScopeRegistry, LocalNgModuleData} from './src/local';

packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts

@@ -8,14 +8,6 @@
 import {ClassDeclaration} from '../../reflection';
 import {LocalModuleScope} from './local';
 
-/**
- * Register information about the compilation scope of components.
- */
-export interface ComponentScopeRegistry {
-  registerComponentScope(clazz: ClassDeclaration, scope: LocalModuleScope): void;
-  setComponentAsRequiringRemoteScoping(clazz: ClassDeclaration): void;
-}
-
 /**
  * Read information about the compilation scope of components.
  */
@@ -24,17 +16,6 @@ export interface ComponentScopeReader {
   getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null;
 }
 
-/**
- * A noop registry that doesn't do anything.
- *
- * This can be used in tests and cases where we don't care about the compilation scopes
- * being registered.
- */
-export class NoopComponentScopeRegistry implements ComponentScopeRegistry {
-  registerComponentScope(clazz: ClassDeclaration, scope: LocalModuleScope): void {}
-  setComponentAsRequiringRemoteScoping(clazz: ClassDeclaration): void {}
-}
-
 /**
  * A `ComponentScopeReader` that reads from an ordered set of child readers until it obtains the
  * requested scope.

packages/compiler-cli/src/ngtsc/scope/src/local.ts

@@ -16,7 +16,7 @@ import {ClassDeclaration} from '../../reflection';
 import {identifierOfNode, nodeNameForError} from '../../util/src/typescript';
 
 import {ExportScope, ScopeData} from './api';
-import {ComponentScopeReader, ComponentScopeRegistry, NoopComponentScopeRegistry} from './component_scope';
+import {ComponentScopeReader} from './component_scope';
 import {DtsModuleScopeResolver} from './dependency';
 
 export interface LocalNgModuleData {
@@ -104,8 +104,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
 
   constructor(
       private localReader: MetadataReader, private dependencyScopeReader: DtsModuleScopeResolver,
-      private refEmitter: ReferenceEmitter, private aliasingHost: AliasingHost|null,
-      private componentScopeRegistry: ComponentScopeRegistry = new NoopComponentScopeRegistry()) {}
+      private refEmitter: ReferenceEmitter, private aliasingHost: AliasingHost|null) {}
 
   /**
    * Add an NgModule's data to the registry.
@@ -126,9 +125,6 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
     const scope = !this.declarationToModule.has(clazz) ?
         null :
         this.getScopeOfModule(this.declarationToModule.get(clazz) !);
-    if (scope !== null) {
-      this.componentScopeRegistry.registerComponentScope(clazz, scope);
-    }
     return scope;
   }
 
@@ -357,7 +353,6 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
    */
   setComponentAsRequiringRemoteScoping(node: ClassDeclaration): void {
     this.remoteScoping.add(node);
-    this.componentScopeRegistry.setComponentAsRequiringRemoteScoping(node);
   }
 
   /**

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

@@ -173,9 +173,6 @@ export class IvyCompilation {
   private analyze(sf: ts.SourceFile, preanalyze: true): Promise<void>|undefined;
   private analyze(sf: ts.SourceFile, preanalyze: boolean): Promise<void>|undefined {
     const promises: Promise<void>[] = [];
-    if (this.incrementalState.safeToSkip(sf)) {
-      return;
-    }
     const analyzeClass = (node: ClassDeclaration): void => {
       const ivyClass = this.detectHandlersForClass(node);