perf(compiler-cli): detect semantic changes and their effect on an in…

…cremental rebuild

In Angular programs, changing a file may require other files to be
emitted as well due to implicit NgModule dependencies. For example, if
the selector of a directive is changed then all components that have
that directive in their compilation scope need to be recompiled, as the
change of selector may affect the directive matching results.

Until now, the compiler solved this problem using a single dependency
graph. The implicit NgModule dependencies were represented in this
graph, such that a changed file would correctly also cause other files
to be re-emitted. This approach is limited in a few ways:

1. The file dependency graph is used to determine whether it is safe to
   reuse the analysis data of an Angular decorated class. This analysis
   data is invariant to unrelated changes to the NgModule scope, but
   because the single dependency graph also tracked the implicit
   NgModule dependencies the compiler had to consider analysis data as
   stale far more often than necessary.
2. It is typical for a change to e.g. a directive to not affect its
   public API—its selector, inputs, outputs, or exportAs clause—in which
   case there is no need to re-emit all declarations in scope, as their
   compilation output wouldn't have changed.

This commit implements a mechanism by which the compiler is able to
determine the impact of a change by comparing it to the prior
compilation. To achieve this, a new graph is maintained that tracks all
public API information of all Angular decorated symbols. During an
incremental compilation this information is compared to the information
that was captured in the most recently succeeded compilation. This
determines the exact impact of the changes to the public API, which
is then used to determine which files need to be re-emitted.

Note that the file dependency graph remains, as it is still used to
track the dependencies of analysis data. This graph does no longer track
the implicit NgModule dependencies, which allows for better reuse of
analysis data.

This commit also fixes an incorrectness where a change to a declaration
in NgModule `A` would not cause the declarations in NgModules that
import from NgModule `A` to be re-emitted. This was intentionally
incorrect as otherwise the performance of incremental rebuilds would
have been far worse. This is no longer a concern, as the compiler is now
able to only re-emit when actually necessary.

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

@@ -14,6 +14,7 @@ import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../../src/ng
 import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics';
 import {absoluteFromSourceFile, LogicalFileSystem, ReadonlyFileSystem} from '../../../src/ngtsc/file_system';
 import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, NOOP_DEFAULT_IMPORT_RECORDER, PrivateExportAliasingHost, Reexport, ReferenceEmitter} from '../../../src/ngtsc/imports';
+import {NOOP_COMPONENT_RESOLUTION_REGISTRY} from '../../../src/ngtsc/incremental/api';
 import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry, ResourceRegistry} from '../../../src/ngtsc/metadata';
 import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
 import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver, TypeCheckScopeRegistry} from '../../../src/ngtsc/scope';
@@ -102,8 +103,11 @@ export class DecorationAnalyzer {
         /* usePoisonedData */ false,
         /* i18nNormalizeLineEndingsInICUs */ false, this.moduleResolver, this.cycleAnalyzer,
         CycleHandlingStrategy.UseRemoteScoping, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER,
-        NOOP_DEPENDENCY_TRACKER, this.injectableRegistry,
+        NOOP_DEPENDENCY_TRACKER,
+        this.injectableRegistry,
+        NOOP_COMPONENT_RESOLUTION_REGISTRY,
         !!this.compilerOptions.annotateForClosureCompiler),
+
     // See the note in ngtsc about why this cast is needed.
     // clang-format off
     new DirectiveDecoratorHandler(

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

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import {AbsoluteFsPath, isLocalRelativePath, relative} from '../../../src/ngtsc/file_system';
-import {DependencyTracker} from '../../../src/ngtsc/incremental/api';
+import {ComponentResolutionRegistry, DependencyTracker} from '../../../src/ngtsc/incremental/api';
 
 export function isWithinPackage(packagePath: AbsoluteFsPath, filePath: AbsoluteFsPath): boolean {
   const relativePath = relative(packagePath, filePath);
@@ -16,8 +16,6 @@ export function isWithinPackage(packagePath: AbsoluteFsPath, filePath: AbsoluteF
 class NoopDependencyTracker implements DependencyTracker {
   addDependency(): void {}
   addResourceDependency(): void {}
-  addTransitiveDependency(): void {}
-  addTransitiveResources(): void {}
   recordDependencyAnalysisFailure(): void {}
 }
 

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

@@ -13,7 +13,7 @@ import {Cycle, CycleAnalyzer, CycleHandlingStrategy} from '../../cycles';
 import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics';
 import {absoluteFrom, relative} from '../../file_system';
 import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from '../../imports';
-import {DependencyTracker} from '../../incremental/api';
+import {ComponentResolutionRegistry, DependencyTracker} from '../../incremental/api';
 import {IndexingContext} from '../../indexer';
 import {ClassPropertyMapping, ComponentResources, DirectiveMeta, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry, Resource, ResourceRegistry} from '../../metadata';
 import {EnumValue, PartialEvaluator, ResolvedValue} from '../../partial_evaluator';
@@ -131,6 +131,7 @@ export class ComponentDecoratorHandler implements
       private defaultImportRecorder: DefaultImportRecorder,
       private depTracker: DependencyTracker|null,
       private injectableRegistry: InjectableClassRegistry,
+      private componentResolutionRegistry: ComponentResolutionRegistry,
       private annotateForClosureCompiler: boolean) {}
 
   private literalCache = new Map<Decorator, ts.ObjectLiteralExpression>();
@@ -538,7 +539,7 @@ export class ComponentDecoratorHandler implements
       const bound = binder.bind({template: metadata.template.nodes});
 
       // The BoundTarget knows which directives and pipes matched the template.
-      type UsedDirective = R3UsedDirectiveMetadata&{ref: Reference};
+      type UsedDirective = R3UsedDirectiveMetadata&{ref: Reference<ClassDeclaration>};
       const usedDirectives: UsedDirective[] = bound.getUsedDirectives().map(directive => {
         return {
           ref: directive.ref,
@@ -551,7 +552,7 @@ export class ComponentDecoratorHandler implements
         };
       });
 
-      type UsedPipe = {ref: Reference, pipeName: string, expression: Expression};
+      type UsedPipe = {ref: Reference<ClassDeclaration>, pipeName: string, expression: Expression};
       const usedPipes: UsedPipe[] = [];
       for (const pipeName of bound.getUsedPipes()) {
         if (!pipes.has(pipeName)) {
@@ -582,7 +583,8 @@ export class ComponentDecoratorHandler implements
         }
       }
 
-      if (cyclesFromDirectives.size === 0 && cyclesFromPipes.size === 0) {
+      const cycleDetected = cyclesFromDirectives.size !== 0 || cyclesFromPipes.size !== 0;
+      if (!cycleDetected) {
         // No cycle was detected. Record the imports that need to be created in the cycle detector
         // so that future cyclic import checks consider their production.
         for (const {type} of usedDirectives) {
@@ -630,6 +632,10 @@ export class ComponentDecoratorHandler implements
               relatedMessages);
         }
       }
+
+      this.componentResolutionRegistry.register(
+          node, usedDirectives.map(dir => dir.ref.node), usedPipes.map(pipe => pipe.ref.node),
+          cycleDetected);
     }
 
     const diagnostics: ts.Diagnostic[] = [];

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

@@ -17,6 +17,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/file_system",
         "//packages/compiler-cli/src/ngtsc/file_system/testing",
         "//packages/compiler-cli/src/ngtsc/imports",
+        "//packages/compiler-cli/src/ngtsc/incremental:api",
         "//packages/compiler-cli/src/ngtsc/metadata",
         "//packages/compiler-cli/src/ngtsc/partial_evaluator",
         "//packages/compiler-cli/src/ngtsc/reflection",

packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts

@@ -14,6 +14,7 @@ import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
 import {absoluteFrom} from '../../file_system';
 import {runInEachFileSystem} from '../../file_system/testing';
 import {ModuleResolver, NOOP_DEFAULT_IMPORT_RECORDER, ReferenceEmitter} from '../../imports';
+import {NOOP_COMPONENT_RESOLUTION_REGISTRY} from '../../incremental/api';
 import {CompoundMetadataReader, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry, ResourceRegistry} from '../../metadata';
 import {PartialEvaluator} from '../../partial_evaluator';
 import {isNamedClassDeclaration, TypeScriptReflectionHost} from '../../reflection';
@@ -78,6 +79,7 @@ function setup(program: ts.Program, options: ts.CompilerOptions, host: ts.Compil
       NOOP_DEFAULT_IMPORT_RECORDER,
       /* depTracker */ null,
       injectableRegistry,
+      NOOP_COMPONENT_RESOLUTION_REGISTRY,
       /* annotateForClosureCompiler */ false,
   );
   return {reflectionHost, handler};

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

@@ -19,9 +19,11 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/file_system",
         "//packages/compiler-cli/src/ngtsc/imports",
         "//packages/compiler-cli/src/ngtsc/incremental",
+        "//packages/compiler-cli/src/ngtsc/incremental:api",
         "//packages/compiler-cli/src/ngtsc/indexer",
         "//packages/compiler-cli/src/ngtsc/metadata",
         "//packages/compiler-cli/src/ngtsc/modulewithproviders",
+        "//packages/compiler-cli/src/ngtsc/ngmodule_semantics",
         "//packages/compiler-cli/src/ngtsc/partial_evaluator",
         "//packages/compiler-cli/src/ngtsc/perf",
         "//packages/compiler-cli/src/ngtsc/reflection",

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

@@ -16,9 +16,11 @@ import {checkForPrivateExports, ReferenceGraph} from '../../entry_point';
 import {LogicalFileSystem, resolve} from '../../file_system';
 import {AbsoluteModuleStrategy, AliasingHost, AliasStrategy, DefaultImportTracker, ImportRewriter, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, NoopImportRewriter, PrivateExportAliasingHost, R3SymbolsImportRewriter, Reference, ReferenceEmitStrategy, ReferenceEmitter, RelativePathStrategy, UnifiedModulesAliasingHost, UnifiedModulesStrategy} from '../../imports';
 import {IncrementalBuildStrategy, IncrementalDriver} from '../../incremental';
+import {ComponentResolutionRegistry} from '../../incremental/api';
 import {generateAnalysis, IndexedComponent, IndexingContext} from '../../indexer';
 import {ComponentResources, CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry, MetadataReader, ResourceRegistry} from '../../metadata';
 import {ModuleWithProvidersScanner} from '../../modulewithproviders';
+import {SemanticDepGraphAdapter} from '../../ngmodule_semantics';
 import {PartialEvaluator} from '../../partial_evaluator';
 import {NOOP_PERF_RECORDER, PerfRecorder} from '../../perf';
 import {DeclarationNode, isNamedClassDeclaration, TypeScriptReflectionHost} from '../../reflection';
@@ -634,8 +636,6 @@ export class NgCompiler {
   private resolveCompilation(traitCompiler: TraitCompiler): void {
     traitCompiler.resolve();
 
-    this.recordNgModuleScopeDependencies();
-
     // At this point, analysis is complete and the compiler can now calculate which files need to
     // be emitted, so do that.
     this.incrementalDriver.recordSuccessfulAnalysis(traitCompiler);
@@ -808,74 +808,6 @@ export class NgCompiler {
     return this.nonTemplateDiagnostics;
   }
 
-  /**
-   * Reifies the inter-dependencies of NgModules and the components within their compilation scopes
-   * into the `IncrementalDriver`'s dependency graph.
-   */
-  private recordNgModuleScopeDependencies() {
-    const recordSpan = this.perfRecorder.start('recordDependencies');
-    const depGraph = this.incrementalDriver.depGraph;
-
-    for (const scope of this.compilation!.scopeRegistry!.getCompilationScopes()) {
-      const file = scope.declaration.getSourceFile();
-      const ngModuleFile = scope.ngModule.getSourceFile();
-
-      // A change to any dependency of the declaration causes the declaration to be invalidated,
-      // which requires the NgModule to be invalidated as well.
-      depGraph.addTransitiveDependency(ngModuleFile, file);
-
-      // A change to the NgModule file should cause the declaration itself to be invalidated.
-      depGraph.addDependency(file, ngModuleFile);
-
-      const meta =
-          this.compilation!.metaReader.getDirectiveMetadata(new Reference(scope.declaration));
-      if (meta !== null && meta.isComponent) {
-        // If a component's template changes, it might have affected the import graph, and thus the
-        // remote scoping feature which is activated in the event of potential import cycles. Thus,
-        // the module depends not only on the transitive dependencies of the component, but on its
-        // resources as well.
-        depGraph.addTransitiveResources(ngModuleFile, file);
-
-        // A change to any directive/pipe in the compilation scope should cause the component to be
-        // invalidated.
-        for (const directive of scope.directives) {
-          // When a directive in scope is updated, the component needs to be recompiled as e.g. a
-          // selector may have changed.
-          depGraph.addTransitiveDependency(file, directive.ref.node.getSourceFile());
-        }
-        for (const pipe of scope.pipes) {
-          // When a pipe in scope is updated, the component needs to be recompiled as e.g. the
-          // pipe's name may have changed.
-          depGraph.addTransitiveDependency(file, pipe.ref.node.getSourceFile());
-        }
-
-        // Components depend on the entire export scope. In addition to transitive dependencies on
-        // all directives/pipes in the export scope, they also depend on every NgModule in the
-        // scope, as changes to a module may add new directives/pipes to the scope.
-        for (const depModule of scope.ngModules) {
-          // There is a correctness issue here. To be correct, this should be a transitive
-          // dependency on the depModule file, since the depModule's exports might change via one of
-          // its dependencies, even if depModule's file itself doesn't change. However, doing this
-          // would also trigger recompilation if a non-exported component or directive changed,
-          // which causes performance issues for rebuilds.
-          //
-          // Given the rebuild issue is an edge case, currently we err on the side of performance
-          // instead of correctness. A correct and performant design would distinguish between
-          // changes to the depModule which affect its export scope and changes which do not, and
-          // only add a dependency for the former. This concept is currently in development.
-          //
-          // TODO(alxhub): fix correctness issue by understanding the semantics of the dependency.
-          depGraph.addDependency(file, depModule.getSourceFile());
-        }
-      } else {
-        // Directives (not components) and pipes only depend on the NgModule which directly declares
-        // them.
-        depGraph.addDependency(file, ngModuleFile);
-      }
-    }
-    this.perfRecorder.stop(recordSpan);
-  }
-
   private scanForMwp(sf: ts.SourceFile): void {
     this.compilation!.mwpScanner.scan(sf, {
       addTypeReplacement: (node: ts.Declaration, type: Type): void => {
@@ -955,9 +887,14 @@ export class NgCompiler {
     const scopeRegistry =
         new LocalModuleScopeRegistry(localMetaReader, depScopeReader, refEmitter, aliasingHost);
     const scopeReader: ComponentScopeReader = scopeRegistry;
-    const metaRegistry = new CompoundMetadataRegistry([localMetaRegistry, scopeRegistry]);
+    const semanticDepGraphUpdater = this.incrementalDriver.getSemanticDepGraphUpdater();
+    const semanticDepRegistry = new SemanticDepGraphAdapter(semanticDepGraphUpdater);
+    const metaRegistry =
+        new CompoundMetadataRegistry([localMetaRegistry, scopeRegistry, semanticDepRegistry]);
     const injectableRegistry = new InjectableClassRegistry(reflector);
 
+    const componentResolutionRegistry: ComponentResolutionRegistry = semanticDepGraphUpdater;
+
     const metaReader = new CompoundMetadataReader([localMetaReader, dtsReader]);
     const typeCheckScopeRegistry = new TypeCheckScopeRegistry(scopeReader, metaReader);
 
@@ -1005,7 +942,10 @@ export class NgCompiler {
           this.options.enableI18nLegacyMessageIdFormat !== false, this.usePoisonedData,
           this.options.i18nNormalizeLineEndingsInICUs, this.moduleResolver, this.cycleAnalyzer,
           cycleHandlingStrategy, refEmitter, defaultImportTracker, this.incrementalDriver.depGraph,
-          injectableRegistry, this.closureCompilerEnabled),
+          injectableRegistry,
+          componentResolutionRegistry,
+          this.closureCompilerEnabled),
+
       // TODO(alxhub): understand why the cast here is necessary (something to do with `null`
       // not being assignable to `unknown` when wrapped in `Readonly`).
       // clang-format off

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

@@ -12,6 +12,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/file_system",
         "//packages/compiler-cli/src/ngtsc/imports",
         "//packages/compiler-cli/src/ngtsc/metadata",
+        "//packages/compiler-cli/src/ngtsc/ngmodule_semantics",
         "//packages/compiler-cli/src/ngtsc/partial_evaluator",
         "//packages/compiler-cli/src/ngtsc/reflection",
         "//packages/compiler-cli/src/ngtsc/scope",
@@ -27,6 +28,7 @@ ts_library(
     srcs = ["api.ts"],
     deps = [
         "//packages/compiler-cli/src/ngtsc/file_system",
+        "//packages/compiler-cli/src/ngtsc/reflection",
         "@npm//typescript",
     ],
 )

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

@@ -8,6 +8,7 @@
 
 import * as ts from 'typescript';
 import {AbsoluteFsPath} from '../file_system';
+import {ClassDeclaration, DeclarationNode} from '../reflection';
 
 /**
  * Interface of the incremental build engine.
@@ -49,22 +50,6 @@ export interface DependencyTracker<T extends {fileName: string} = ts.SourceFile>
    */
   addResourceDependency(from: T, on: AbsoluteFsPath): void;
 
-  /**
-   * Record that the file `from` depends on the file `on` as well as `on`'s direct dependencies.
-   *
-   * This operation is reified immediately, so if future dependencies are added to `on` they will
-   * not automatically be added to `from`.
-   */
-  addTransitiveDependency(from: T, on: T): void;
-
-  /**
-   * Record that the file `from` depends on the resource dependencies of `resourcesOf`.
-   *
-   * This operation is reified immediately, so if future resource dependencies are added to
-   * `resourcesOf` they will not automatically be added to `from`.
-   */
-  addTransitiveResources(from: T, resourcesOf: T): void;
-
   /**
    * Record that the given file contains unresolvable dependencies.
    *
@@ -73,3 +58,29 @@ export interface DependencyTracker<T extends {fileName: string} = ts.SourceFile>
    */
   recordDependencyAnalysisFailure(file: T): void;
 }
+
+/**
+ * Captures the resolution data of components to be able determine if a component's emit is
+ * affected by the changes that were made in an incremental rebuild.
+ */
+export interface ComponentResolutionRegistry {
+  /**
+   * Registers a component's usages in the registry.
+   *
+   * @param component The component declaration to register.
+   * @param usedDirectives The declarations of the directives that are used by the component.
+   * @param usedPipes The declarations of the pipes that are used by the component.
+   * @param isRemotelyScoped Whether the component requires remote scoping.
+   */
+  register(
+      component: ClassDeclaration, usedDirectives: ClassDeclaration[],
+      usedPipes: ClassDeclaration[], isRemotelyScoped: boolean): void;
+}
+
+/**
+ * A `ComponentResolutionRegistry` that does nothing, to be used when the information that is being
+ * registered is not used.
+ */
+export const NOOP_COMPONENT_RESOLUTION_REGISTRY: ComponentResolutionRegistry = {
+  register() {}
+};

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

@@ -35,24 +35,6 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
     this.nodeFor(from).usesResources.add(resource);
   }
 
-  addTransitiveDependency(from: T, on: T): void {
-    const nodeFrom = this.nodeFor(from);
-    nodeFrom.dependsOn.add(on.fileName);
-
-    const nodeOn = this.nodeFor(on);
-    for (const dep of nodeOn.dependsOn) {
-      nodeFrom.dependsOn.add(dep);
-    }
-  }
-
-  addTransitiveResources(from: T, resourcesOf: T): void {
-    const nodeFrom = this.nodeFor(from);
-    const nodeOn = this.nodeFor(resourcesOf);
-    for (const dep of nodeOn.usesResources) {
-      nodeFrom.usesResources.add(dep);
-    }
-  }
-
   recordDependencyAnalysisFailure(file: T): void {
     this.nodeFor(file).failedAnalysis = true;
   }
@@ -63,10 +45,6 @@ export class FileDependencyGraph<T extends {fileName: string} = ts.SourceFile> i
     return node ? [...node.usesResources] : [];
   }
 
-  isStale(sf: T, changedTsPaths: Set<string>, changedResources: Set<AbsoluteFsPath>): boolean {
-    return isLogicallyChanged(sf, this.nodeFor(sf), changedTsPaths, EMPTY_SET, changedResources);
-  }
-
   /**
    * Update the current dependency graph from a previous one, incorporating a set of physical
    * changes.
@@ -160,5 +138,3 @@ interface FileNode {
   usesResources: Set<AbsoluteFsPath>;
   failedAnalysis: boolean;
 }
-
-const EMPTY_SET: ReadonlySet<any> = new Set<any>();