fix(ivy): correctly emit component when it's removed from its module (#34912)

This commit fixes a bug in the incremental rebuild engine of ngtsc, where if
a component was removed from its NgModule, it would not be properly
re-emitted.

The bug stemmed from the fact that whether to emit a file was a decision
based purely on the updated dependency graph, which captures the dependency
structure of the rebuild program. This graph has no edge from the component
to its former module (as it was removed, of course), so the compiler
erroneously decides not to emit the component.

The bug here is that the compiler does know, from the previous dependency
graph, that the component file has logically changed, since its previous
dependency (the module file) has changed. This information was not carried
forward into the set of files which need to be emitted, because it was
assumed that the updated dependency graph was a more accurate source of that
information.

With this commit, the set of files which need emit is pre-populated with the
set of logically changed files, to cover edge cases like this.

Fixes #34813

PR Close #34912

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

@@ -88,9 +88,9 @@ On every invocation, the compiler receives (or can easily determine) several pie
 
 With this information, the compiler can perform rebuild optimizations:
 
-1. The compiler uses the last good compilation's dependency graph to determine which parts of its analysis work can be reused.
+1. The compiler uses the last good compilation's dependency graph to determine which parts of its analysis work can be reused, and an initial set of files which need to be re-emitted.
 2. The compiler analyzes the rest of the program and generates an updated dependency graph, which describes the relationships between files in the program as they are currently.
-3. Based on this graph, the compiler can make a determination for each TS file whether it needs to be re-emitted or can safely be skipped. This produces a set called `pendingEmit` of every file which requires a re-emit.
+3. Based on this graph, the compiler can make a final determination for each TS file whether it needs to be re-emitted or can safely be skipped. This produces a set called `pendingEmit` of every file which requires a re-emit.
 4. The compiler cycles through the files and emits those which are necessary, removing them from `pendingEmit`.
 
 Theoretically, after this process `pendingEmit` should be empty. As a precaution against errors which might happen in the future, `pendingEmit` is also passed into future compilations, so any files which previously were determined to need an emit (but have not been successfully produced yet) will be retried on subsequent compilations. This is mostly relevant if a client of `ngtsc` attempts to implement emit-on-error functionality.
@@ -113,6 +113,12 @@ After analysis is successfully performed, the compiler uses its dependency graph
 
 If a new build is started after a successful build, only `pendingEmit` from the `AnalyzedBuildState` needs to be merged into the new build's `PendingBuildState`.
 
+## Component to NgModule dependencies
+
+The dependency of a component on its NgModule is slightly problematic, because its arrow is in the opposite direction of the source dependency (which is from NgModule to the component, via `declarations`). This creates a scenario where, if the NgModule is changed to no longer include the component, the component still needs to be re-emitted because the module has changed.
+
+This is one of very few cases where `pendingEmit` must be populated with the logical changes from the previous program (those files determined to be changed in step 1 under "Tracking of changes" above), and cannot simply be created from the current dependency graph.
+
 # What optimizations are possible in the future?
 
 There is plenty of room for improvement here, with diminishing returns for the work involved.

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

@@ -127,6 +127,16 @@ export class IncrementalDriver implements IncrementalBuild<ClassRecord> {
       for (const fileName of state.changedTsPaths) {
         logicalChanges.add(fileName);
       }
+
+      // Any logically changed files need to be re-emitted. Most of the time this would happen
+      // regardless because the new dependency graph would _also_ identify the file as stale.
+      // However there are edge cases such as removing a component from an NgModule without adding
+      // it to another one, where the previous graph identifies the file as logically changed, but
+      // the new graph (which does not have that edge) fails to identify that the file should be
+      // re-emitted.
+      for (const change of logicalChanges) {
+        state.pendingEmit.add(change);
+      }
     }
 
     // `state` now reflects the initial pending state of the current compilation.