diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index 25542002004a..f8537594f17f 100644 --- 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[]; } +/** 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(); + 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): 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 index efe046cad8be..340cf6393f0a 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -7109,6 +7109,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', () => {