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.

packages/compiler-cli/test/ngtsc/incremental_spec.ts

@@ -251,6 +251,72 @@ runInEachFileSystem(() => {
       expect(written).toContain('/foo_module.js');
     });
 
+    it('should rebuild a component if removed from an NgModule', () => {
+      // This test consists of a component with a dependency (the directive DepDir) provided via an
+      // NgModule. Initially this configuration is built, then the component is removed from its
+      // module (which removes DepDir from the component's scope) and a rebuild is performed.
+      // The compiler should re-emit the component without DepDir in its scope.
+      //
+      // This is a tricky scenario due to the backwards dependency arrow from a component to its
+      // module.
+      env.write('dep.ts', `
+        import {Directive, NgModule} from '@angular/core';
+
+        @Directive({selector: '[dep]'})
+        export class DepDir {}
+
+        @NgModule({
+          declarations: [DepDir],
+          exports: [DepDir],
+        })
+        export class DepModule {}
+      `);
+
+      env.write('cmp.ts', `
+        import {Component} from '@angular/core';
+
+        @Component({
+          selector: 'test-cmp',
+          template: '<div dep></div>',
+        })
+        export class Cmp {}
+      `);
+
+      env.write('module.ts', `
+        import {NgModule} from '@angular/core';
+        import {Cmp} from './cmp';
+        import {DepModule} from './dep';
+
+        @NgModule({
+          declarations: [Cmp],
+          imports: [DepModule],
+        })
+        export class Module {}
+      `);
+
+      env.driveMain();
+      env.flushWrittenFileTracking();
+
+      // Remove the component from the module and recompile.
+      env.write('module.ts', `
+        import {NgModule} from '@angular/core';
+        import {DepModule} from './dep';
+
+        @NgModule({
+          declarations: [],
+          imports: [DepModule],
+        })
+        export class Module {}
+      `);
+
+      env.driveMain();
+
+      // After removing the component from the module, it should have been re-emitted without DepDir
+      // in its scope.
+      expect(env.getFilesWrittenSinceLastFlush()).toContain('/cmp.js');
+      expect(env.getContents('cmp.js')).not.toContain('DepDir');
+    });
+
     it('should rebuild only a Component (but with the correct CompilationScope) and its module if its template has changed',
        () => {
          setupFooBarProgram(env);