diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 1293a676e2..50b9a338c9 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -328,6 +328,11 @@ export class ComponentDecoratorHandler implements const scope = this.scopeReader.getScopeForComponent(node); const selector = analysis.meta.selector; const matcher = new SelectorMatcher(); + if (scope === 'error') { + // Don't bother indexing components which had erroneous scopes. + return null; + } + if (scope !== null) { for (const directive of scope.compilation.directives) { if (directive.selector !== null) { @@ -360,6 +365,11 @@ export class ComponentDecoratorHandler implements let schemas: SchemaMetadata[] = []; const scope = this.scopeReader.getScopeForComponent(node); + if (scope === 'error') { + // Don't type-check components that had errors in their scopes. + return; + } + if (scope !== null) { for (const meta of scope.compilation.directives) { if (meta.selector !== null) { @@ -405,7 +415,7 @@ export class ComponentDecoratorHandler implements wrapDirectivesAndPipesInClosure: false, }; - if (scope !== null) { + if (scope !== null && scope !== 'error') { // Replace the empty components and directives from the analyze() step with a fully expanded // scope. This is possible now because during resolve() the whole compilation unit has been // fully analyzed. diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index 6b5b356311..6f4cb73016 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -317,7 +317,7 @@ export class NgModuleDecoratorHandler implements injectorImports: [], }; - if (scope !== null) { + if (scope !== null && scope !== 'error') { // Using the scope information, extend the injector's imports using the modules that are // specified as module exports. const context = getSourceFile(node); @@ -342,7 +342,7 @@ export class NgModuleDecoratorHandler implements return {diagnostics}; } - if (scope === null || scope.reexports === null) { + if (scope === null || scope === 'error' || scope.reexports === null) { return {data}; } else { return { @@ -370,9 +370,10 @@ export class NgModuleDecoratorHandler implements for (const decl of analysis.declarations) { if (this.scopeRegistry.getRequiresRemoteScope(decl.node)) { const scope = this.scopeRegistry.getScopeOfModule(ts.getOriginalNode(node) as typeof node); - if (scope === null) { + if (scope === null || scope === 'error') { continue; } + const directives = scope.compilation.directives.map( directive => this.refEmitter.emit(directive.ref, context)); const pipes = scope.compilation.pipes.map(pipe => this.refEmitter.emit(pipe.ref, context)); diff --git a/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts b/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts index 55107c18f0..77a28aa499 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts @@ -12,7 +12,7 @@ import {LocalModuleScope} from './local'; * Read information about the compilation scope of components. */ export interface ComponentScopeReader { - getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null; + getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error'; getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null; } @@ -26,7 +26,7 @@ export interface ComponentScopeReader { export class CompoundComponentScopeReader implements ComponentScopeReader { constructor(private readers: ComponentScopeReader[]) {} - getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null { + getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error' { for (const reader of this.readers) { const meta = reader.getScopeForComponent(clazz); if (meta !== null) { diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index b55cca14e7..95a4728fbd 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -109,6 +109,15 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop */ private scopeErrors = new Map(); + /** + * Tracks which NgModules are unreliable due to errors within their declarations. + * + * This provides a unified view of which modules have errors, across all of the different + * diagnostic categories that can be produced. Theoretically this can be inferred from the other + * properties of this class, but is tracked explicitly to simplify the logic. + */ + private taintedModules = new Set(); + constructor( private localReader: MetadataReader, private dependencyScopeReader: DtsModuleScopeResolver, private refEmitter: ReferenceEmitter, private aliasingHost: AliasingHost|null) {} @@ -131,7 +140,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop registerPipeMetadata(pipe: PipeMeta): void {} - getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null { + getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error' { const scope = !this.declarationToModule.has(clazz) ? null : this.getScopeOfModule(this.declarationToModule.get(clazz) !.ngModule); @@ -158,15 +167,20 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop * `LocalModuleScope`. * * This method implements the logic of NgModule imports and exports. It returns the - * `LocalModuleScope` for the given NgModule if one can be produced, and `null` if no scope is - * available or the scope contains errors. + * `LocalModuleScope` for the given NgModule if one can be produced, `null` if no scope was ever + * defined, or the string `'error'` if the scope contained errors. */ - getScopeOfModule(clazz: ClassDeclaration): LocalModuleScope|null { + getScopeOfModule(clazz: ClassDeclaration): LocalModuleScope|'error'|null { const scope = this.moduleToRef.has(clazz) ? this.getScopeOfModuleReference(this.moduleToRef.get(clazz) !) : null; - // Translate undefined -> null. - return scope !== undefined ? scope : null; + // If the NgModule class is marked as tainted, consider it an error. + if (this.taintedModules.has(clazz)) { + return 'error'; + } + + // Translate undefined -> 'error'. + return scope !== undefined ? scope : 'error'; } /** @@ -192,7 +206,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop const scopes: CompilationScope[] = []; this.declarationToModule.forEach((declData, declaration) => { const scope = this.getScopeOfModule(declData.ngModule); - if (scope !== null) { + if (scope !== null && scope !== 'error') { scopes.push({declaration, ngModule: declData.ngModule, ...scope.compilation}); } }); @@ -220,6 +234,10 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop const duplicateDeclMap = new Map(); const firstDeclData = this.declarationToModule.get(decl.node) !; + // Mark both modules as tainted, since their declarations are missing a component. + this.taintedModules.add(firstDeclData.ngModule); + this.taintedModules.add(ngModule); + // Being detected as a duplicate means there are two NgModules (for now) which declare this // directive/pipe. Add both of them to the duplicate tracking map. duplicateDeclMap.set(firstDeclData.ngModule, firstDeclData); @@ -298,6 +316,8 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop } else if (pipe !== null) { compilationPipes.set(decl.node, {...pipe, ref: decl}); } else { + this.taintedModules.add(ngModule.ref.node); + const errorNode = decl.getOriginForDiagnostics(ngModule.rawDeclarations !); diagnostics.push(makeDiagnostic( ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode, @@ -390,8 +410,8 @@ Either remove it from the NgModule's declarations, or add an appropriate Angular // Save the errors for retrieval. this.scopeErrors.set(ref.node, diagnostics); - // Return undefined to indicate the scope is invalid. - this.cache.set(ref.node, undefined); + // Mark this module as being tainted. + this.taintedModules.add(ref.node); return undefined; } diff --git a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts index e6235501e5..05d06dda5e 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -13,7 +13,7 @@ import {CompoundMetadataRegistry, DirectiveMeta, LocalMetadataRegistry, Metadata import {ClassDeclaration} from '../../reflection'; import {ScopeData} from '../src/api'; import {DtsModuleScopeResolver} from '../src/dependency'; -import {LocalModuleScopeRegistry} from '../src/local'; +import {LocalModuleScope, LocalModuleScopeRegistry} from '../src/local'; function registerFakeRefs(registry: MetadataRegistry): {[name: string]: Reference} { @@ -56,7 +56,7 @@ describe('LocalModuleScopeRegistry', () => { rawDeclarations: null, }); - const scope = scopeRegistry.getScopeOfModule(Module.node) !; + const scope = scopeRegistry.getScopeOfModule(Module.node) as LocalModuleScope; expect(scopeToRefs(scope.compilation)).toEqual([Dir1, Dir2, Pipe1]); expect(scopeToRefs(scope.exported)).toEqual([Dir1, Pipe1]); }); @@ -89,7 +89,7 @@ describe('LocalModuleScopeRegistry', () => { rawDeclarations: null, }); - const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !; + const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope; expect(scopeToRefs(scopeA.compilation)).toEqual([DirA, DirB, DirCE]); expect(scopeToRefs(scopeA.exported)).toEqual([]); }); @@ -114,7 +114,7 @@ describe('LocalModuleScopeRegistry', () => { rawDeclarations: null, }); - const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !; + const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope; expect(scopeToRefs(scopeA.compilation)).toEqual([]); expect(scopeToRefs(scopeA.exported)).toEqual([Dir]); }); @@ -147,7 +147,7 @@ describe('LocalModuleScopeRegistry', () => { rawDeclarations: null, }); - const scope = scopeRegistry.getScopeOfModule(ModuleA.node) !; + const scope = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope; expect(scopeToRefs(scope.compilation)).toEqual([DirA, DirB]); expect(scopeToRefs(scope.exported)).toEqual([DirA, DirB]); }); @@ -171,7 +171,7 @@ describe('LocalModuleScopeRegistry', () => { rawDeclarations: null, }); - const scope = scopeRegistry.getScopeOfModule(Module.node) !; + const scope = scopeRegistry.getScopeOfModule(Module.node) as LocalModuleScope; expect(scope.compilation.directives[0].ref.getIdentityIn(idSf)).toBe(id); }); @@ -195,7 +195,7 @@ describe('LocalModuleScopeRegistry', () => { rawDeclarations: null, }); - const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !; + const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope; expect(scopeToRefs(scopeA.exported)).toEqual([Dir]); }); @@ -219,7 +219,7 @@ describe('LocalModuleScopeRegistry', () => { rawDeclarations: null, }); - expect(scopeRegistry.getScopeOfModule(ModuleA.node)).toBe(null); + expect(scopeRegistry.getScopeOfModule(ModuleA.node)).toBe('error'); // ModuleA should have associated diagnostics as it exports `Dir` without declaring it. expect(scopeRegistry.getDiagnosticsOfModule(ModuleA.node)).not.toBeNull(); diff --git a/packages/compiler-cli/test/ngtsc/scope_spec.ts b/packages/compiler-cli/test/ngtsc/scope_spec.ts index 22300e35cd..9165104706 100644 --- a/packages/compiler-cli/test/ngtsc/scope_spec.ts +++ b/packages/compiler-cli/test/ngtsc/scope_spec.ts @@ -301,6 +301,87 @@ runInEachFileSystem(() => { expect(diagnosticToNode(error, ts.isIdentifier).text).toEqual('Dir'); }); }); + + it('should not produce component template type-check errors if its module is invalid', () => { + env.tsconfig({'fullTemplateTypeCheck': true}); + + // Set up 3 files, each of which declare an NgModule that's invalid in some way. This will + // produce a bunch of diagnostics related to the issues with the modules. Each module also + // declares a component with a template that references a element. This test + // verifies that none of the produced diagnostics mention this nonexistent element, since + // no template type-checking should be performed for a component that's part of an invalid + // NgModule. + + // This NgModule declares something which isn't a directive/pipe. + env.write('invalid-declaration.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: '', + }) + export class TestCmp {} + + export class NotACmp {} + + @NgModule({declarations: [TestCmp, NotACmp]}) + export class Module {} + `); + + // This NgModule imports something which isn't an NgModule. + env.write('invalid-import.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: '', + }) + export class TestCmp {} + + export class NotAModule {} + + @NgModule({ + declarations: [TestCmp], + imports: [NotAModule], + }) + export class Module {} + `); + + // This NgModule imports a DepModule which itself is invalid (it declares something which + // isn't a directive/pipe). + env.write('transitive-error-in-import.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: '', + }) + export class TestCmp {} + + export class NotACmp {} + + @NgModule({ + declarations: [NotACmp], + exports: [NotACmp], + }) + export class DepModule {} + + @NgModule({ + declarations: [TestCmp], + imports: [DepModule], + }) + export class Module {} + `); + + for (const diag of env.driveDiagnostics()) { + // None of the diagnostics should be related to the fact that the component uses an + // unknown element, because in all cases the component's scope was invalid. + expect(diag.messageText) + .not.toContain( + 'doesnt-exist', + 'Template type-checking ran for a component, when it shouldn\'t have.'); + } + }); }); });