From 5645641bbc8c929bcb8fc56f7d4bdee75dae86c6 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 16 Oct 2024 21:42:15 +0200 Subject: [PATCH 1/2] fix(compiler-cli): report when NgModule imports or exports itself 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. --- .../compiler-cli/src/ngtsc/scope/src/local.ts | 45 +++++++-- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 99 +++++++++++++++++++ 2 files changed, 137 insertions(+), 7 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index 25542002004a..8e0b3e8f467b 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -175,9 +175,12 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop * defined, or the string `'error'` if the scope contained errors. */ getScopeOfModule(clazz: ClassDeclaration): LocalModuleScope | null { - return this.moduleToRef.has(clazz) - ? this.getScopeOfModuleReference(this.moduleToRef.get(clazz)!) - : null; + if (this.moduleToRef.has(clazz)) { + const ref = this.moduleToRef.get(clazz)!; + return this.getScopeOfModuleReference(ref, ref); + } + + return null; } /** @@ -243,7 +246,10 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop /** * Implementation of `getScopeOfModule` which accepts a reference to a class. */ - private getScopeOfModuleReference(ref: Reference): LocalModuleScope | null { + private getScopeOfModuleReference( + ref: Reference, + lookupRoot: Reference, + ): LocalModuleScope | null { if (this.cache.has(ref.node)) { return this.cache.get(ref.node)!; } @@ -299,7 +305,19 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop // 1) process imports. for (const decl of ngModule.imports) { - const importScope = this.getExportedScope(decl, diagnostics, ref.node, 'import'); + if (decl.node === lookupRoot.node) { + diagnostics.push( + makeDiagnostic( + ErrorCode.NGMODULE_INVALID_IMPORT, + getDiagnosticNode(decl, ngModule.rawImports), + 'NgModule cannot import itself', + ), + ); + isPoisoned = true; + continue; + } + + const importScope = this.getExportedScope(decl, lookupRoot, diagnostics, ref.node, 'import'); if (importScope !== null) { if (importScope === 'invalid' || importScope.exported.isPoisoned) { // An import was an NgModule but contained errors of its own. Record this as an error too, @@ -425,8 +443,20 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop // export maps. Directives/pipes are different - they might be exports of declared types or // imported types. for (const decl of ngModule.exports) { + if (decl.node === lookupRoot.node) { + diagnostics.push( + makeDiagnostic( + ErrorCode.NGMODULE_INVALID_EXPORT, + getDiagnosticNode(decl, ngModule.rawExports), + 'NgModule cannot export itself', + ), + ); + isPoisoned = true; + continue; + } + // Attempt to resolve decl as an NgModule. - const exportScope = this.getExportedScope(decl, diagnostics, ref.node, 'export'); + const exportScope = this.getExportedScope(decl, lookupRoot, diagnostics, ref.node, 'export'); if (exportScope === 'invalid' || (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 @@ -541,6 +571,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop */ private getExportedScope( ref: Reference, + lookupRoot: Reference, diagnostics: ts.Diagnostic[], ownerForErrors: DeclarationNode, type: 'import' | 'export', @@ -566,7 +597,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop return this.dependencyScopeReader.resolve(ref); } else { // The NgModule is declared locally in the current program. Resolve it from the registry. - return this.getScopeOfModuleReference(ref); + return this.getScopeOfModuleReference(ref, lookupRoot); } } diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index efe046cad8be..2e64a3d33ce1 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -7109,6 +7109,105 @@ 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 cannot import itself'); + }); + + 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 cannot export itself'); + }); + + 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 cannot import itself'); + 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 cannot import itself'); + }); }); describe('when processing external directives', () => { From 071df4595cc2f75626a8b3704d45ad08320fb047 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 17 Oct 2024 21:15:44 +0200 Subject: [PATCH 2/2] fixup! fix(compiler-cli): report when NgModule imports or exports itself --- .../compiler-cli/src/ngtsc/scope/src/local.ts | 104 ++++++++++-------- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 36 +++++- 2 files changed, 89 insertions(+), 51 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index 8e0b3e8f467b..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". @@ -175,12 +181,9 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop * defined, or the string `'error'` if the scope contained errors. */ getScopeOfModule(clazz: ClassDeclaration): LocalModuleScope | null { - if (this.moduleToRef.has(clazz)) { - const ref = this.moduleToRef.get(clazz)!; - return this.getScopeOfModuleReference(ref, ref); - } - - return null; + return this.moduleToRef.has(clazz) + ? this.getScopeOfModuleReference(this.moduleToRef.get(clazz)!) + : null; } /** @@ -246,14 +249,17 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop /** * Implementation of `getScopeOfModule` which accepts a reference to a class. */ - private getScopeOfModuleReference( - ref: Reference, - lookupRoot: Reference, - ): LocalModuleScope | null { + 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; @@ -305,28 +311,24 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop // 1) process imports. for (const decl of ngModule.imports) { - if (decl.node === lookupRoot.node) { - diagnostics.push( - makeDiagnostic( - ErrorCode.NGMODULE_INVALID_IMPORT, - getDiagnosticNode(decl, ngModule.rawImports), - 'NgModule cannot import itself', - ), - ); - isPoisoned = true; - continue; - } - - const importScope = this.getExportedScope(decl, lookupRoot, diagnostics, ref.node, 'import'); + 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; } } @@ -443,28 +445,24 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop // export maps. Directives/pipes are different - they might be exports of declared types or // imported types. for (const decl of ngModule.exports) { - if (decl.node === lookupRoot.node) { - diagnostics.push( - makeDiagnostic( - ErrorCode.NGMODULE_INVALID_EXPORT, - getDiagnosticNode(decl, ngModule.rawExports), - 'NgModule cannot export itself', - ), - ); - isPoisoned = true; - continue; - } - // Attempt to resolve decl as an NgModule. - const exportScope = this.getExportedScope(decl, lookupRoot, diagnostics, ref.node, 'export'); - if (exportScope === 'invalid' || (exportScope !== null && exportScope.exported.isPoisoned)) { + const exportScope = this.getExportedScope(decl, diagnostics, ref.node, 'export'); + 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) { @@ -571,11 +569,10 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop */ private getExportedScope( ref: Reference, - lookupRoot: Reference, 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)) { @@ -596,8 +593,21 @@ 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, lookupRoot); + 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 2e64a3d33ce1..340cf6393f0a 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -7123,7 +7123,7 @@ runInEachFileSystem((os: string) => { const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); - expect(diags[0].messageText).toBe('NgModule cannot import itself'); + expect(diags[0].messageText).toBe('NgModule "import" field contains a cycle'); }); it('should report if an NgModule exports itself', () => { @@ -7139,7 +7139,7 @@ runInEachFileSystem((os: string) => { const diags = env.driveDiagnostics(); expect(diags.length).toBe(1); - expect(diags[0].messageText).toBe('NgModule cannot export itself'); + expect(diags[0].messageText).toBe('NgModule "export" field contains a cycle'); }); it('should report if an NgModule imports itself transitively', () => { @@ -7181,7 +7181,7 @@ runInEachFileSystem((os: string) => { expect(diags[0].messageText).toBe( 'This import contains errors, which may affect components that depend on this NgModule.', ); - expect(diags[1].messageText).toBe('NgModule cannot import itself'); + 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.', ); @@ -7206,7 +7206,35 @@ runInEachFileSystem((os: string) => { expect(diags[0].messageText).toBe( 'This import contains errors, which may affect components that depend on this NgModule.', ); - expect(diags[1].messageText).toBe('NgModule cannot import itself'); + 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'); }); });