Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5645641
fix(compiler-cli): report when NgModule imports or exports itself
crisbeto Oct 16, 2024
071df45
fixup! fix(compiler-cli): report when NgModule imports or exports itself
crisbeto Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 50 additions & 9 deletions packages/compiler-cli/src/ngtsc/scope/src/local.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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".
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)) {
Expand All @@ -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);
}
Expand Down
127 changes: 127 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down