From c37ec8b2557e2f224fd7720586e5528d013050fc Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Fri, 8 Mar 2019 11:32:49 -0800 Subject: [PATCH] fix(ivy): produce ts.Diagnostics for NgModule scope errors (#29191) Previously, when the NgModule scope resolver discovered semantic errors within a users NgModules, it would throw assertion errors. TODOs in the codebase indicated these should become ts.Diagnostics eventually. Besides producing better-looking errors, there is another reason to make this change asap: these assertions were shadowing actual errors, via an interesting mechanism: 1) a component would produce a ts.Diagnostic during its analyze() step 2) as a result, it wouldn't register component metadata with the scope resolver 3) the NgModule for the component references it in exports, which was detected as an invalid export (no metadata registering it as a component). 4) the resulting assertion error would crash the compiler, hiding the real cause of the problem (an invalid component). This commit should mitigate this problem by converting scoping errors to proper ts.Diagnostics. Additionally, we should consider registering some marker indicating a class is a directive/component/pipe without actually requiring full metadata to be produced for it, which would allow suppression of errors like "invalid export" for such invalid types. PR Close #29191 --- .../src/ngtsc/annotations/src/ng_module.ts | 36 +++-- .../src/ngtsc/diagnostics/index.ts | 2 +- .../src/ngtsc/diagnostics/src/code.ts | 23 ++- .../src/ngtsc/diagnostics/src/error.ts | 22 +-- .../compiler-cli/src/ngtsc/scope/BUILD.bazel | 1 + .../compiler-cli/src/ngtsc/scope/src/local.ts | 149 ++++++++++++++++-- .../src/ngtsc/scope/test/local_spec.ts | 8 +- .../src/ngtsc/transform/src/api.ts | 5 +- .../src/ngtsc/transform/src/compilation.ts | 27 +++- packages/compiler-cli/test/ngtsc/BUILD.bazel | 1 + .../compiler-cli/test/ngtsc/scope_spec.ts | 134 ++++++++++++++++ 11 files changed, 358 insertions(+), 50 deletions(-) create mode 100644 packages/compiler-cli/test/ngtsc/scope_spec.ts 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; +}