From 047488c5d86b39b87d3d7497c5ff720d2adf81c3 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 17 Dec 2019 13:29:15 -0800 Subject: [PATCH] refactor(ivy): move NgModule declaration checks to the 'scope' package (#34460) Previously each NgModule trait checked its own scope for valid declarations during 'resolve'. This worked, but caused the LocalModuleScopeRegistry to declare that NgModule scopes were valid even if they contained invalid declarations. This commit moves the generation of diagnostic errors to the LocalModuleScopeRegistry where it belongs. Now the registry can consider an NgModule's scope to be invalid if it contains invalid declarations. PR Close #34460 --- .../src/ngtsc/annotations/src/ng_module.ts | 22 ++------------ .../src/ngtsc/annotations/src/util.ts | 30 +------------------ .../src/ngtsc/imports/src/references.ts | 23 ++++++++++++++ .../compiler-cli/src/ngtsc/scope/src/local.ts | 9 ++++-- 4 files changed, 33 insertions(+), 51 deletions(-) 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 7e43c198ee..6b5b356311 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -22,7 +22,7 @@ import {getSourceFile} from '../../util/src/typescript'; import {generateSetClassMetadataCall} from './metadata'; import {ReferencesRegistry} from './references_registry'; -import {combineResolvers, findAngularDecorator, forwardRefResolver, getReferenceOriginForDiagnostics, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; +import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; export interface NgModuleAnalysis { mod: R3NgModuleMetadata; @@ -119,7 +119,7 @@ export class NgModuleDecoratorHandler implements // Look through the declarations to make sure they're all a part of the current compilation. for (const ref of declarationRefs) { if (ref.node.getSourceFile().isDeclarationFile) { - const errorNode: ts.Expression = getReferenceOriginForDiagnostics(ref, rawDeclarations); + const errorNode: ts.Expression = ref.getOriginForDiagnostics(rawDeclarations); diagnostics.push(makeDiagnostic( ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode, @@ -317,24 +317,6 @@ export class NgModuleDecoratorHandler implements injectorImports: [], }; - for (const decl of analysis.declarations) { - if (this.metaReader.getDirectiveMetadata(decl) === null && - this.metaReader.getPipeMetadata(decl) === null) { - // This declaration is neither a directive(or component) nor a pipe, so produce a diagnostic - // for it. - - // Locate the error on the 'declarations' field of the NgModule decorator to start. - const errorNode: ts.Expression = - getReferenceOriginForDiagnostics(decl, analysis.rawDeclarations !); - diagnostics.push(makeDiagnostic( - ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode, - `The class '${decl.node.name.text}' is listed in the declarations of the NgModule '${node.name.text}', but is not a directive, a component, or a pipe. - -Either remove it from the NgModule's declarations, or add an appropriate Angular decorator.`, - [{node: decl.node.name, messageText: `'${decl.node.name.text}' is declared here.`}])); - } - } - if (scope !== null) { // Using the scope information, extend the injector's imports using the modules that are // specified as module exports. diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index 989275dfbf..3d730de347 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -378,33 +378,6 @@ export function wrapFunctionExpressionsInParens(expression: ts.Expression): ts.E return ts.transform(expression, [parensWrapperTransformerFactory]).transformed[0]; } -/** - * Given a 'container' expression and a `Reference` extracted from that container, produce a - * `ts.Expression` to use in a diagnostic which best indicates the position within the container - * expression that generated the `Reference`. - * - * For example, given a `Reference` to the class 'Bar' and the containing expression: - * `[Foo, Bar, Baz]`, this function would attempt to return the `ts.Identifier` for `Bar` within the - * array. This could be used to produce a nice diagnostic context: - * - * ```text - * [Foo, Bar, Baz] - * ~~~ - * ``` - * - * If no specific node can be found, then the `fallback` expression is used, which defaults to the - * entire containing expression. - */ -export function getReferenceOriginForDiagnostics( - ref: Reference, container: ts.Expression, fallback: ts.Expression = container): ts.Expression { - const id = ref.getIdentityInExpression(container); - if (id !== null) { - return id; - } - - return fallback; -} - /** * Create a `ts.Diagnostic` which indicates the given class is part of the declarations of two or * more NgModules. @@ -421,8 +394,7 @@ export function makeDuplicateDeclarationError( } // Try to find the reference to the declaration within the declarations array, to hang the // error there. If it can't be found, fall back on using the NgModule's name. - const contextNode = - getReferenceOriginForDiagnostics(decl.ref, decl.rawDeclarations, decl.ngModule.name); + const contextNode = decl.ref.getOriginForDiagnostics(decl.rawDeclarations, decl.ngModule.name); context.push({ node: contextNode, messageText: diff --git a/packages/compiler-cli/src/ngtsc/imports/src/references.ts b/packages/compiler-cli/src/ngtsc/imports/src/references.ts index f50dd581b3..f2432d9c3f 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/references.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/references.ts @@ -135,6 +135,29 @@ export class Reference { null; } + /** + * Given the 'container' expression from which this `Reference` was extracted, produce a + * `ts.Expression` to use in a diagnostic which best indicates the position within the container + * expression that generated the `Reference`. + * + * For example, given a `Reference` to the class 'Bar' and the containing expression: + * `[Foo, Bar, Baz]`, this function would attempt to return the `ts.Identifier` for `Bar` within + * the array. This could be used to produce a nice diagnostic context: + * + * ```text + * [Foo, Bar, Baz] + * ~~~ + * ``` + * + * If no specific node can be found, then the `fallback` expression is used, which defaults to the + * entire containing expression. + */ + getOriginForDiagnostics(container: ts.Expression, fallback: ts.Expression = container): + ts.Expression { + const id = this.getIdentityInExpression(container); + return id !== null ? id : fallback; + } + cloneWithAlias(alias: Expression): Reference { const ref = new Reference(this.node, this.bestGuessOwningModule); ref.identifiers = [...this.identifiers]; diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index 7d1d1f4a9e..b55cca14e7 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -298,8 +298,13 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop } else if (pipe !== null) { compilationPipes.set(decl.node, {...pipe, ref: decl}); } else { - // TODO(alxhub): produce a ts.Diagnostic. This can't be an error right now since some - // ngtools tests rely on analysis of broken components. + const errorNode = decl.getOriginForDiagnostics(ngModule.rawDeclarations !); + diagnostics.push(makeDiagnostic( + ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode, + `The class '${decl.node.name.text}' is listed in the declarations of the NgModule '${ngModule.ref.node.name.text}', but is not a directive, a component, or a pipe. + +Either remove it from the NgModule's declarations, or add an appropriate Angular decorator.`, + [{node: decl.node.name, messageText: `'${decl.node.name.text}' is declared here.`}])); continue; }