fix(compiler-cli): report when NgModule imports or exports itself (#58231)

crisbeto · devversion · commit b0ab653965cf · 2024-10-18T09:26:45.000Z
Reports a diagnostic if an NgModule refers to itself in its `imports` or `exports`. Previously the compiler would go into an infinite loop. Fixes #58224. PR Close #58231
diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts
@@ -45,6 +45,9 @@ export interface LocalNgModuleData {
   exports: Reference<ClassDeclaration>[];
 }
 
+/** Value used to mark a module whose scope is in the process of being resolved. */
+const IN_PROGRESS_RESOLUTION = {};
+
 /**
  * A registry which collects information about NgModules, Directives, Components, and Pipes which
  * are local (declared in the ts.Program being compiled), and can produce `LocalModuleScope`s
@@ -96,7 +99,10 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
    * A cache of calculated `LocalModuleScope`s for each NgModule declared in the current program.
 
    */
-  private cache = new Map<ClassDeclaration, LocalModuleScope | null>();
+  private cache = new Map<
+    ClassDeclaration,
+    LocalModuleScope | typeof IN_PROGRESS_RESOLUTION | null
+  >();
 
   /**
    * Tracks the `RemoteScope` for components requiring "remote scoping".
@@ -245,9 +251,15 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
    */
   private getScopeOfModuleReference(ref: Reference<ClassDeclaration>): LocalModuleScope | null {
     if (this.cache.has(ref.node)) {
-      return this.cache.get(ref.node)!;
+      const cachedValue = this.cache.get(ref.node);
+
+      if (cachedValue !== IN_PROGRESS_RESOLUTION) {
+        return cachedValue as LocalModuleScope | null;
+      }
     }
 
+    this.cache.set(ref.node, IN_PROGRESS_RESOLUTION);
+
     // Seal the registry to protect the integrity of the `LocalModuleScope` cache.
     this.sealed = true;
 
@@ -301,14 +313,22 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
     for (const decl of ngModule.imports) {
       const importScope = this.getExportedScope(decl, diagnostics, ref.node, 'import');
       if (importScope !== null) {
-        if (importScope === 'invalid' || importScope.exported.isPoisoned) {
+        if (
+          importScope === 'invalid' ||
+          importScope === 'cycle' ||
+          importScope.exported.isPoisoned
+        ) {
           // An import was an NgModule but contained errors of its own. Record this as an error too,
           // because this scope is always going to be incorrect if one of its imports could not be
           // read.
-          diagnostics.push(invalidTransitiveNgModuleRef(decl, ngModule.rawImports, 'import'));
           isPoisoned = true;
 
-          if (importScope === 'invalid') {
+          // Prevent the module from reporting a diagnostic about itself when there's a cycle.
+          if (importScope !== 'cycle') {
+            diagnostics.push(invalidTransitiveNgModuleRef(decl, ngModule.rawImports, 'import'));
+          }
+
+          if (importScope === 'invalid' || importScope === 'cycle') {
             continue;
           }
         }
@@ -427,14 +447,22 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
     for (const decl of ngModule.exports) {
       // Attempt to resolve decl as an NgModule.
       const exportScope = this.getExportedScope(decl, diagnostics, ref.node, 'export');
-      if (exportScope === 'invalid' || (exportScope !== null && exportScope.exported.isPoisoned)) {
+      if (
+        exportScope === 'invalid' ||
+        exportScope === 'cycle' ||
+        (exportScope !== null && exportScope.exported.isPoisoned)
+      ) {
         // An export was an NgModule but contained errors of its own. Record this as an error too,
         // because this scope is always going to be incorrect if one of its exports could not be
         // read.
-        diagnostics.push(invalidTransitiveNgModuleRef(decl, ngModule.rawExports, 'export'));
         isPoisoned = true;
 
-        if (exportScope === 'invalid') {
+        // Prevent the module from reporting a diagnostic about itself when there's a cycle.
+        if (exportScope !== 'cycle') {
+          diagnostics.push(invalidTransitiveNgModuleRef(decl, ngModule.rawExports, 'export'));
+        }
+
+        if (exportScope === 'invalid' || exportScope === 'cycle') {
           continue;
         }
       } else if (exportScope !== null) {
@@ -544,7 +572,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
     diagnostics: ts.Diagnostic[],
     ownerForErrors: DeclarationNode,
     type: 'import' | 'export',
-  ): ExportScope | null | 'invalid' {
+  ): ExportScope | null | 'invalid' | 'cycle' {
     if (ref.node.getSourceFile().isDeclarationFile) {
       // The NgModule is declared in a .d.ts file. Resolve it with the `DependencyScopeReader`.
       if (!ts.isClassDeclaration(ref.node)) {
@@ -565,6 +593,19 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
       }
       return this.dependencyScopeReader.resolve(ref);
     } else {
+      if (this.cache.get(ref.node) === IN_PROGRESS_RESOLUTION) {
+        diagnostics.push(
+          makeDiagnostic(
+            type === 'import'
+              ? ErrorCode.NGMODULE_INVALID_IMPORT
+              : ErrorCode.NGMODULE_INVALID_EXPORT,
+            identifierOfNode(ref.node) || ref.node,
+            `NgModule "${type}" field contains a cycle`,
+          ),
+        );
+        return 'cycle';
+      }
+
       // The NgModule is declared locally in the current program. Resolve it from the registry.
       return this.getScopeOfModuleReference(ref);
     }
diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
@@ -6936,6 +6936,133 @@ runInEachFileSystem((os: string) => {
           'Is it missing an @NgModule annotation?',
         );
       });
+
+      it('should report if an NgModule imports itself', () => {
+        env.write(
+          'test.ts',
+          `
+          import {NgModule} from '@angular/core';
+
+          @NgModule({imports: [MyModule]})
+          export class MyModule {}
+        `,
+        );
+
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(1);
+        expect(diags[0].messageText).toBe('NgModule "import" field contains a cycle');
+      });
+
+      it('should report if an NgModule exports itself', () => {
+        env.write(
+          'test.ts',
+          `
+          import {NgModule} from '@angular/core';
+
+          @NgModule({exports: [MyModule]})
+          export class MyModule {}
+        `,
+        );
+
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(1);
+        expect(diags[0].messageText).toBe('NgModule "export" field contains a cycle');
+      });
+
+      it('should report if an NgModule imports itself transitively', () => {
+        env.write(
+          'dep-2.ts',
+          `
+          import {NgModule} from '@angular/core';
+          import {MyModule} from './test';
+
+          @NgModule({imports: [MyModule]})
+          export class DepModule2 {}
+        `,
+        );
+
+        env.write(
+          'dep.ts',
+          `
+          import {NgModule} from '@angular/core';
+          import {DepModule2} from './dep-2';
+
+          @NgModule({imports: [DepModule2]})
+          export class DepModule {}
+        `,
+        );
+
+        env.write(
+          'test.ts',
+          `
+          import {NgModule} from '@angular/core';
+          import {DepModule} from './dep';
+
+          @NgModule({imports: [DepModule]})
+          export class MyModule {}
+        `,
+        );
+
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(3);
+        expect(diags[0].messageText).toBe(
+          'This import contains errors, which may affect components that depend on this NgModule.',
+        );
+        expect(diags[1].messageText).toBe('NgModule "import" field contains a cycle');
+        expect(diags[2].messageText).toBe(
+          'This import contains errors, which may affect components that depend on this NgModule.',
+        );
+      });
+
+      it('should report if an NgModule imports itself via a forwardRef', () => {
+        env.write(
+          'test.ts',
+          `
+          import {NgModule, forwardRef} from '@angular/core';
+
+          @NgModule({imports: [forwardRef(() => MyModule)]})
+          export class DepModule {}
+
+          @NgModule({imports: [DepModule]})
+          export class MyModule {}
+        `,
+        );
+
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(2);
+        expect(diags[0].messageText).toBe(
+          'This import contains errors, which may affect components that depend on this NgModule.',
+        );
+        expect(diags[1].messageText).toBe('NgModule "import" field contains a cycle');
+      });
+
+      it('should report if an NgModule imports itself via a forwardRef', () => {
+        env.write(
+          'test.ts',
+          `
+          import {NgModule, forwardRef} from '@angular/core';
+
+          @NgModule({imports: [forwardRef(() => ModB)]})
+          class ModA {}
+
+          @NgModule({imports: [forwardRef(() => ModC)]})
+          class ModB {}
+
+          @NgModule({imports: [ModB]})
+          class ModC {}
+        `,
+        );
+
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(3);
+        expect(diags[0].messageText).toBe(
+          'This import contains errors, which may affect components that depend on this NgModule.',
+        );
+        expect(diags[1].messageText).toBe(
+          'This import contains errors, which may affect components that depend on this NgModule.',
+        );
+        expect(diags[2].messageText).toBe('NgModule "import" field contains a cycle');
+      });
     });
 
     describe('when processing external directives', () => {
