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

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.

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:

packages/compiler-cli/src/ngtsc/imports/src/references.ts

@@ -135,6 +135,29 @@ export class Reference<T extends ts.Node = ts.Node> {
         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<T> {
     const ref = new Reference(this.node, this.bestGuessOwningModule);
     ref.identifiers = [...this.identifiers];

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;
       }