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 0dce76e968..828c3ee344 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -60,6 +60,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler { + const name = node.name !.text; if (decorator.args === null || decorator.args.length > 1) { throw new FatalDiagnosticError( ErrorCode.DECORATOR_ARITY_WRONG, decorator.node, @@ -93,28 +94,28 @@ export class NgModuleDecoratorHandler implements DecoratorHandler[] = []; let rawImports: ts.Expression|null = null; if (ngModule.has('imports')) { rawImports = ngModule.get('imports') !; const importsMeta = this.evaluator.evaluate(rawImports, moduleResolvers); - imports = this.resolveTypeList(rawImports, importsMeta, 'imports'); + imports = this.resolveTypeList(rawImports, importsMeta, name, 'imports'); } let exports: Reference[] = []; let rawExports: ts.Expression|null = null; if (ngModule.has('exports')) { rawExports = ngModule.get('exports') !; const exportsMeta = this.evaluator.evaluate(rawExports, moduleResolvers); - exports = this.resolveTypeList(rawExports, exportsMeta, 'exports'); + exports = this.resolveTypeList(rawExports, exportsMeta, name, 'exports'); this.referencesRegistry.add(node, ...exports); } let bootstrap: Reference[] = []; if (ngModule.has('bootstrap')) { const expr = ngModule.get('bootstrap') !; const bootstrapMeta = this.evaluator.evaluate(expr, forwardRefResolver); - bootstrap = this.resolveTypeList(expr, bootstrapMeta, 'bootstrap'); + bootstrap = this.resolveTypeList(expr, bootstrapMeta, name, 'bootstrap'); } // Register this module's information with the LocalModuleScopeRegistry. This ensures that @@ -156,12 +157,11 @@ export class NgModuleDecoratorHandler implements DecoratorHandler[] { + private resolveTypeList( + expr: ts.Node, resolvedList: ResolvedValue, className: string, + arrayName: string): Reference[] { const refList: Reference[] = []; if (!Array.isArray(resolvedList)) { throw new FatalDiagnosticError( - ErrorCode.VALUE_HAS_WRONG_TYPE, expr, `Expected array when reading property ${name}`); + ErrorCode.VALUE_HAS_WRONG_TYPE, expr, + `Expected array when reading property ${arrayName}`); } resolvedList.forEach((entry, idx) => { @@ -341,17 +345,19 @@ export class NgModuleDecoratorHandler implements DecoratorHandler(); + private cache = new Map(); /** * Tracks whether a given component requires "remote scoping". @@ -88,6 +96,11 @@ export class LocalModuleScopeRegistry { */ private remoteScoping = new Set(); + /** + * Tracks errors accumulated in the processing of scopes for each module declaration. + */ + private scopeErrors = new Map(); + constructor( private dependencyScopeReader: DtsModuleScopeResolver, private refEmitter: ReferenceEmitter, private aliasGenerator: AliasGenerator|null) {} @@ -124,15 +137,43 @@ export class LocalModuleScopeRegistry { * Collects registered data for a module and its directives/pipes and convert it into a full * `LocalModuleScope`. * - * This method implements the logic of NgModule imports and exports. + * 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. */ getScopeOfModule(clazz: ts.Declaration): LocalModuleScope|null { + const scope = this.getScopeOfModuleInternal(clazz); + // Translate undefined -> null. + return scope !== undefined ? scope : null; + } + + /** + * Retrieves any `ts.Diagnostic`s produced during the calculation of the `LocalModuleScope` for + * the given NgModule, or `null` if no errors were present. + */ + getDiagnosticsOfModule(clazz: ts.Declaration): ts.Diagnostic[]|null { + // Required to ensure the errors are populated for the given class. If it has been processed + // before, this will be a no-op due to the scope cache. + this.getScopeOfModule(clazz); + + if (this.scopeErrors.has(clazz)) { + return this.scopeErrors.get(clazz) !; + } else { + return null; + } + } + + /** + * Implementation of `getScopeOfModule` which differentiates between no scope being available + * (returns `null`) and a scope being produced with errors (returns `undefined`). + */ + private getScopeOfModuleInternal(clazz: ts.Declaration): LocalModuleScope|null|undefined { // Seal the registry to protect the integrity of the `LocalModuleScope` cache. this.sealed = true; // Look for cached data if available. if (this.cache.has(clazz)) { - return this.cache.get(clazz) !; + return this.cache.get(clazz); } // `clazz` should be an NgModule previously added to the registry. If not, a scope for it @@ -142,6 +183,10 @@ export class LocalModuleScopeRegistry { } const ngModule = this.ngModuleData.get(clazz) !; + // Errors produced during computation of the scope are recorded here. At the end, if this array + // isn't empty then `undefined` will be cached and returned to indicate this scope is invalid. + const diagnostics: ts.Diagnostic[] = []; + // At this point, the goal is to produce two distinct transitive sets: // - the directives and pipes which are visible to components declared in the NgModule. // - the directives and pipes which are exported to any NgModules which import this one. @@ -189,10 +234,17 @@ export class LocalModuleScopeRegistry { // 2) process imports. for (const decl of ngModule.imports) { - const importScope = this.getExportedScope(decl); + const importScope = this.getExportedScope(decl, diagnostics, clazz, 'import'); if (importScope === null) { - // TODO(alxhub): produce a ts.Diagnostic - throw new Error(`Unknown import: ${decl.debugName}`); + // An import wasn't an NgModule, so record an error. + diagnostics.push(invalidRef(clazz, decl, 'import')); + continue; + } else if (importScope === undefined) { + // 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(clazz, decl, 'import')); + continue; } for (const directive of importScope.exported.directives) { compilationDirectives.set(directive.ref.node, directive); @@ -209,8 +261,14 @@ export class LocalModuleScopeRegistry { // imported types. for (const decl of ngModule.exports) { // Attempt to resolve decl as an NgModule. - const importScope = this.getExportedScope(decl); - if (importScope !== null) { + const importScope = this.getExportedScope(decl, diagnostics, clazz, 'export'); + if (importScope === undefined) { + // 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(clazz, decl, 'export')); + continue; + } else if (importScope !== null) { // decl is an NgModule. for (const directive of importScope.exported.directives) { exportDirectives.set(directive.ref.node, directive); @@ -228,8 +286,12 @@ export class LocalModuleScopeRegistry { exportPipes.set(decl.node, pipe); } else { // decl is an unknown export. - // TODO(alxhub): produce a ts.Diagnostic - throw new Error(`Unknown export: ${decl.debugName}`); + if (this.directiveData.has(decl.node) || this.pipeData.has(decl.node)) { + diagnostics.push(invalidReexport(clazz, decl)); + } else { + diagnostics.push(invalidRef(clazz, decl, 'export')); + } + continue; } } @@ -272,7 +334,17 @@ export class LocalModuleScopeRegistry { } } + // Check if this scope had any errors during production. + if (diagnostics.length > 0) { + // Cache undefined, to mark the fact that the scope is invalid. + this.cache.set(clazz, undefined); + // Save the errors for retrieval. + this.scopeErrors.set(clazz, diagnostics); + + // Return undefined to indicate the scope is invalid. + return undefined; + } // Finally, produce the `LocalModuleScope` with both the compilation and export scopes. const scope = { @@ -302,18 +374,30 @@ export class LocalModuleScopeRegistry { * * The NgModule in question may be declared locally in the current ts.Program, or it may be * declared in a .d.ts file. + * + * This function will return `null` if no scope could be found, or `undefined` if an invalid scope + * was found. It can also contribute diagnostics of its own by adding to the given `diagnostics` + * array parameter. */ - private getExportedScope(ref: Reference): ExportScope|null { + private getExportedScope( + ref: Reference, diagnostics: ts.Diagnostic[], ownerForErrors: ts.Declaration, + type: 'import'|'export'): ExportScope|null|undefined { if (ref.node.getSourceFile().isDeclarationFile) { // The NgModule is declared in a .d.ts file. Resolve it with the `DependencyScopeReader`. if (!ts.isClassDeclaration(ref.node)) { - // TODO(alxhub): produce a ts.Diagnostic - throw new Error(`Reference to an NgModule ${ref.debugName} which isn't a class?`); + // The NgModule is in a .d.ts file but is not declared as a ts.ClassDeclaration. This is an + // error in the .d.ts metadata. + const code = type === 'import' ? ErrorCode.NGMODULE_INVALID_IMPORT : + ErrorCode.NGMODULE_INVALID_EXPORT; + diagnostics.push(makeDiagnostic( + code, identifierOfNode(ref.node) || ref.node, + `Appears in the NgModule.${type}s of ${nodeNameForError(ownerForErrors)}, but could not be resolved to an NgModule`)); + return undefined; } return this.dependencyScopeReader.resolve(ref as Reference); } else { // The NgModule is declared locally in the current program. Resolve it from the registry. - return this.getScopeOfModule(ref.node); + return this.getScopeOfModuleInternal(ref.node); } } @@ -323,3 +407,40 @@ export class LocalModuleScopeRegistry { } } } + +/** + * Produce a `ts.Diagnostic` for an invalid import or export from an NgModule. + */ +function invalidRef( + clazz: ts.Declaration, decl: Reference, + type: 'import' | 'export'): ts.Diagnostic { + const code = + type === 'import' ? ErrorCode.NGMODULE_INVALID_IMPORT : ErrorCode.NGMODULE_INVALID_EXPORT; + const resolveTarget = type === 'import' ? 'NgModule' : 'NgModule, Component, Directive, or Pipe'; + return makeDiagnostic( + code, identifierOfNode(decl.node) || decl.node, + `Appears in the NgModule.${type}s of ${nodeNameForError(clazz)}, but could not be resolved to an ${resolveTarget} class`); +} + +/** + * Produce a `ts.Diagnostic` for an import or export which itself has errors. + */ +function invalidTransitiveNgModuleRef( + clazz: ts.Declaration, decl: Reference, + type: 'import' | 'export'): ts.Diagnostic { + const code = + type === 'import' ? ErrorCode.NGMODULE_INVALID_IMPORT : ErrorCode.NGMODULE_INVALID_EXPORT; + return makeDiagnostic( + code, identifierOfNode(decl.node) || decl.node, + `Appears in the NgModule.${type}s of ${nodeNameForError(clazz)}, but itself has errors`); +} + +/** + * Produce a `ts.Diagnostic` for an exported directive or pipe which was not declared or imported + * by the NgModule in question. + */ +function invalidReexport(clazz: ts.Declaration, decl: Reference): ts.Diagnostic { + return makeDiagnostic( + ErrorCode.NGMODULE_INVALID_REEXPORT, identifierOfNode(decl.node) || decl.node, + `Present in the NgModule.exports of ${nodeNameForError(clazz)} but neither declared nor imported`); +} 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 6a38e632d3..10b39c5d8b 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -126,7 +126,13 @@ describe('LocalModuleScopeRegistry', () => { registry.registerNgModule(ModuleA.node, {exports: [Dir], imports: [], declarations: []}); registry.registerNgModule(ModuleB.node, {declarations: [Dir], exports: [Dir], imports: []}); - expect(() => registry.getScopeOfModule(ModuleA.node)).toThrow(); + expect(registry.getScopeOfModule(ModuleA.node)).toBe(null); + + // ModuleA should have associated diagnostics as it exports `Dir` without declaring it. + expect(registry.getDiagnosticsOfModule(ModuleA.node)).not.toBeNull(); + + // ModuleB should have no diagnostics as it correctly declares `Dir`. + expect(registry.getDiagnosticsOfModule(ModuleB.node)).toBeNull(); }); }); diff --git a/packages/compiler-cli/src/ngtsc/transform/src/api.ts b/packages/compiler-cli/src/ngtsc/transform/src/api.ts index 1120661ae1..ef5c8aa216 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/api.ts @@ -123,4 +123,7 @@ export interface CompileResult { type: Type; } -export interface ResolveResult { reexports?: Reexport[]; } +export interface ResolveResult { + reexports?: Reexport[]; + diagnostics?: ts.Diagnostic[]; +} diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index 240fcc077e..e3b3416466 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -247,15 +247,26 @@ export class IvyCompilation { for (const match of ivyClass.matchedHandlers) { if (match.handler.resolve !== undefined && match.analyzed !== null && match.analyzed.analysis !== undefined) { - const res = match.handler.resolve(node, match.analyzed.analysis); - if (res.reexports !== undefined) { - const fileName = node.getSourceFile().fileName; - if (!this.reexportMap.has(fileName)) { - this.reexportMap.set(fileName, new Map()); + try { + const res = match.handler.resolve(node, match.analyzed.analysis); + if (res.reexports !== undefined) { + const fileName = node.getSourceFile().fileName; + if (!this.reexportMap.has(fileName)) { + this.reexportMap.set(fileName, new Map()); + } + const fileReexports = this.reexportMap.get(fileName) !; + for (const reexport of res.reexports) { + fileReexports.set(reexport.asAlias, [reexport.fromModule, reexport.symbolName]); + } } - const fileReexports = this.reexportMap.get(fileName) !; - for (const reexport of res.reexports) { - fileReexports.set(reexport.asAlias, [reexport.fromModule, reexport.symbolName]); + if (res.diagnostics !== undefined) { + this._diagnostics.push(...res.diagnostics); + } + } catch (err) { + if (err instanceof FatalDiagnosticError) { + this._diagnostics.push(err.toDiagnostic()); + } else { + throw err; } } } diff --git a/packages/compiler-cli/test/ngtsc/BUILD.bazel b/packages/compiler-cli/test/ngtsc/BUILD.bazel index 880234345e..e5bfd3f84f 100644 --- a/packages/compiler-cli/test/ngtsc/BUILD.bazel +++ b/packages/compiler-cli/test/ngtsc/BUILD.bazel @@ -7,6 +7,7 @@ ts_library( deps = [ "//packages/compiler", "//packages/compiler-cli", + "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/routing", "//packages/compiler-cli/src/ngtsc/util", "//packages/compiler-cli/test:test_utils", diff --git a/packages/compiler-cli/test/ngtsc/scope_spec.ts b/packages/compiler-cli/test/ngtsc/scope_spec.ts new file mode 100644 index 0000000000..07588c4484 --- /dev/null +++ b/packages/compiler-cli/test/ngtsc/scope_spec.ts @@ -0,0 +1,134 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import * as ts from 'typescript'; + +import {ErrorCode, ngErrorCode} from '../../src/ngtsc/diagnostics'; + +import {NgtscTestEnvironment} from './env'; + +describe('ngtsc module scopes', () => { + let env !: NgtscTestEnvironment; + beforeEach(() => { + env = NgtscTestEnvironment.setup(); + env.tsconfig(); + }); + + describe('diagnostics', () => { + describe('imports', () => { + it('should produce an error when an invalid class is imported', () => { + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + + class NotAModule {} + + @NgModule({imports: [NotAModule]}) + class IsAModule {} + `); + const [error] = env.driveDiagnostics(); + expect(error).not.toBeUndefined(); + expect(error.messageText).toContain('IsAModule'); + expect(error.messageText).toContain('NgModule.imports'); + expect(error.code).toEqual(ngErrorCode(ErrorCode.NGMODULE_INVALID_IMPORT)); + expect(diagnosticToNode(error, ts.isIdentifier).text).toEqual('NotAModule'); + }); + + it('should produce an error when a non-class is imported from a .d.ts dependency', () => { + env.write('dep.d.ts', `export declare let NotAClass: Function;`); + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + import {NotAClass} from './dep'; + + @NgModule({imports: [NotAClass]}) + class IsAModule {} + `); + const [error] = env.driveDiagnostics(); + expect(error).not.toBeUndefined(); + expect(error.messageText).toContain('IsAModule'); + expect(error.messageText).toContain('NgModule.imports'); + expect(error.code).toEqual(ngErrorCode(ErrorCode.VALUE_HAS_WRONG_TYPE)); + expect(diagnosticToNode(error, ts.isIdentifier).text).toEqual('NotAClass'); + }); + }); + + describe('exports', () => { + it('should produce an error when a non-NgModule class is exported', () => { + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + + class NotAModule {} + + @NgModule({exports: [NotAModule]}) + class IsAModule {} + `); + const [error] = env.driveDiagnostics(); + expect(error).not.toBeUndefined(); + expect(error.messageText).toContain('IsAModule'); + expect(error.messageText).toContain('NgModule.exports'); + expect(error.code).toEqual(ngErrorCode(ErrorCode.NGMODULE_INVALID_EXPORT)); + expect(diagnosticToNode(error, ts.isIdentifier).text).toEqual('NotAModule'); + }); + + it('should produce a transitive error when an invalid NgModule is exported', () => { + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + + export class NotAModule {} + + @NgModule({ + imports: [NotAModule], + }) + class InvalidModule {} + + @NgModule({exports: [InvalidModule]}) + class IsAModule {} + `); + + // Find the diagnostic referencing InvalidModule, which should have come from IsAModule. + const error = env.driveDiagnostics().find( + error => diagnosticToNode(error, ts.isIdentifier).text === 'InvalidModule'); + if (error === undefined) { + return fail('Expected to find a diagnostic referencing InvalidModule'); + } + expect(error.messageText).toContain('IsAModule'); + expect(error.messageText).toContain('NgModule.exports'); + expect(error.code).toEqual(ngErrorCode(ErrorCode.NGMODULE_INVALID_EXPORT)); + }); + }); + + describe('re-exports', () => { + it('should produce an error when a non-declared/imported class is re-exported', () => { + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + @Directive({selector: 'test'}) + class Dir {} + + @NgModule({exports: [Dir]}) + class IsAModule {} + `); + const [error] = env.driveDiagnostics(); + expect(error).not.toBeUndefined(); + expect(error.messageText).toContain('IsAModule'); + expect(error.messageText).toContain('NgModule.exports'); + expect(error.code).toEqual(ngErrorCode(ErrorCode.NGMODULE_INVALID_REEXPORT)); + expect(diagnosticToNode(error, ts.isIdentifier).text).toEqual('Dir'); + }); + }); + }); +}); + +function diagnosticToNode( + diag: ts.Diagnostic, guard: (node: ts.Node) => node is T): T { + if (diag.file === undefined) { + throw new Error(`Expected ts.Diagnostic to have a file source`); + } + const node = (ts as any).getTokenAtPosition(diag.file, diag.start) as ts.Node; + expect(guard(node)).toBe(true); + return node as T; +}