fix(ivy): validate the NgModule declarations field

This commit adds three previously missing validations to
NgModule.declarations:

1. It checks that declared classes are actually within the current
   compilation.

2. It checks that declared classes are directives, components, or pipes.

3. It checks that classes are declared in at most one NgModule.

packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts

@@ -96,14 +96,15 @@ export class DecorationAnalyzer {
     // See the note in ngtsc about why this cast is needed.
     // clang-format off
     new DirectiveDecoratorHandler(
-        this.reflectionHost, this.evaluator, this.fullRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
-        this.isCore, /* annotateForClosureCompiler */ false) as DecoratorHandler<unknown, unknown, unknown>,
+        this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
+        NOOP_DEFAULT_IMPORT_RECORDER, this.isCore,
+        /* annotateForClosureCompiler */ false) as DecoratorHandler<unknown, unknown, unknown>,
     // clang-format on
     // Pipe handler must be before injectable handler in list so pipe factories are printed
     // before injectable factories (so injectable factories can delegate to them)
     new PipeDecoratorHandler(
-        this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
-        this.isCore),
+        this.reflectionHost, this.evaluator, this.metaRegistry, this.scopeRegistry,
+        NOOP_DEFAULT_IMPORT_RECORDER, this.isCore),
     new InjectableDecoratorHandler(
         this.reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore,
         /* strictCtorDeps */ false, /* errorOnDuplicateProv */ false),

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

@@ -29,7 +29,7 @@ import {ResourceLoader} from './api';
 import {extractDirectiveMetadata, parseFieldArrayValue} from './directive';
 import {compileNgFactoryDefField} from './factory';
 import {generateSetClassMetadataCall} from './metadata';
-import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, readBaseClass, unwrapExpression, wrapFunctionExpressionsInParens} from './util';
+import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, makeDuplicateDeclarationError, readBaseClass, unwrapExpression, wrapFunctionExpressionsInParens} from './util';
 
 const EMPTY_MAP = new Map<string, Expression>();
 const EMPTY_ARRAY: any[] = [];
@@ -385,6 +385,14 @@ export class ComponentDecoratorHandler implements
 
   resolve(node: ClassDeclaration, analysis: Readonly<ComponentAnalysisData>):
       ResolveResult<ComponentResolutionData> {
+    const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node);
+    if (duplicateDeclData !== null) {
+      // This component was declared twice (or more).
+      return {
+        diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Component')],
+      };
+    }
+
     const context = node.getSourceFile();
     // Check whether this component was registered with an NgModule. If so, it should be compiled
     // under that module's compilation scope.

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

@@ -15,11 +15,12 @@ import {MetadataRegistry} from '../../metadata';
 import {extractDirectiveGuards} from '../../metadata/src/util';
 import {DynamicValue, EnumValue, PartialEvaluator} from '../../partial_evaluator';
 import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, ReflectionHost, filterToMembersWithDecorator, reflectObjectLiteral} from '../../reflection';
-import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence} from '../../transform';
+import {LocalModuleScopeRegistry} from '../../scope';
+import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence, ResolveResult} from '../../transform';
 
 import {compileNgFactoryDefField} from './factory';
 import {generateSetClassMetadataCall} from './metadata';
-import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens} from './util';
+import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, makeDuplicateDeclarationError, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens} from './util';
 
 const EMPTY_OBJECT: {[key: string]: string} = {};
 const FIELD_DECORATORS = [
@@ -41,8 +42,9 @@ export class DirectiveDecoratorHandler implements
     DecoratorHandler<Decorator|null, DirectiveHandlerData, unknown> {
   constructor(
       private reflector: ReflectionHost, private evaluator: PartialEvaluator,
-      private metaRegistry: MetadataRegistry, private defaultImportRecorder: DefaultImportRecorder,
-      private isCore: boolean, private annotateForClosureCompiler: boolean) {}
+      private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry,
+      private defaultImportRecorder: DefaultImportRecorder, private isCore: boolean,
+      private annotateForClosureCompiler: boolean) {}
 
   readonly precedence = HandlerPrecedence.PRIMARY;
   readonly name = DirectiveDecoratorHandler.name;
@@ -115,6 +117,18 @@ export class DirectiveDecoratorHandler implements
     });
   }
 
+  resolve(node: ClassDeclaration): ResolveResult<unknown> {
+    const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node);
+    if (duplicateDeclData !== null) {
+      // This directive was declared twice (or more).
+      return {
+        diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Directive')],
+      };
+    }
+
+    return {};
+  }
+
   compile(
       node: ClassDeclaration, analysis: Readonly<DirectiveHandlerData>,
       resolution: Readonly<unknown>, pool: ConstantPool): CompileResult[] {

packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts

@@ -9,7 +9,7 @@
 import {CUSTOM_ELEMENTS_SCHEMA, Expression, ExternalExpr, InvokeFunctionExpr, LiteralArrayExpr, LiteralExpr, NO_ERRORS_SCHEMA, R3Identifiers, R3InjectorMetadata, R3NgModuleMetadata, R3Reference, STRING_TYPE, SchemaMetadata, Statement, WrappedNodeExpr, compileInjector, compileNgModule} from '@angular/compiler';
 import * as ts from 'typescript';
 
-import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
+import {ErrorCode, FatalDiagnosticError, makeDiagnostic} from '../../diagnostics';
 import {DefaultImportRecorder, Reference, ReferenceEmitter} from '../../imports';
 import {MetadataReader, MetadataRegistry} from '../../metadata';
 import {PartialEvaluator, ResolvedValue} from '../../partial_evaluator';
@@ -22,13 +22,14 @@ import {getSourceFile} from '../../util/src/typescript';
 
 import {generateSetClassMetadataCall} from './metadata';
 import {ReferencesRegistry} from './references_registry';
-import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens} from './util';
+import {combineResolvers, findAngularDecorator, forwardRefResolver, getReferenceOriginForDiagnostics, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens} from './util';
 
 export interface NgModuleAnalysis {
   mod: R3NgModuleMetadata;
   inj: R3InjectorMetadata;
   metadataStmt: Statement|null;
   declarations: Reference<ClassDeclaration>[];
+  rawDeclarations: ts.Expression|null;
   schemas: SchemaMetadata[];
   imports: Reference<ClassDeclaration>[];
   exports: Reference<ClassDeclaration>[];
@@ -104,13 +105,37 @@ export class NgModuleDecoratorHandler implements
       forwardRefResolver,
     ]);
 
+    const diagnostics: ts.Diagnostic[] = [];
+
     // Extract the module declarations, imports, and exports.
     let declarationRefs: Reference<ClassDeclaration>[] = [];
+    let rawDeclarations: ts.Expression|null = null;
     if (ngModule.has('declarations')) {
-      const expr = ngModule.get('declarations') !;
-      const declarationMeta = this.evaluator.evaluate(expr, forwardRefResolver);
-      declarationRefs = this.resolveTypeList(expr, declarationMeta, name, 'declarations');
+      rawDeclarations = ngModule.get('declarations') !;
+      const declarationMeta = this.evaluator.evaluate(rawDeclarations, forwardRefResolver);
+      declarationRefs =
+          this.resolveTypeList(rawDeclarations, declarationMeta, name, 'declarations');
+
+      // 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);
+
+          diagnostics.push(makeDiagnostic(
+              ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode,
+              `Cannot declare '${ref.node.name.text}' in an NgModule as it's not a part of the current compilation.`,
+              [{
+                node: ref.node.name,
+                messageText: `'${ref.node.name.text}' is declared here.`,
+              }]));
+        }
+      }
+    }
+
+    if (diagnostics.length > 0) {
+      return {diagnostics};
     }
+
     let importRefs: Reference<ClassDeclaration>[] = [];
     let rawImports: ts.Expression|null = null;
     if (ngModule.has('imports')) {
@@ -245,7 +270,7 @@ export class NgModuleDecoratorHandler implements
         schemas: schemas,
         mod: ngModuleDef,
         inj: ngInjectorDef,
-        declarations: declarationRefs,
+        declarations: declarationRefs, rawDeclarations,
         imports: importRefs,
         exports: exportRefs,
         metadataStmt: generateSetClassMetadataCall(
@@ -266,6 +291,7 @@ export class NgModuleDecoratorHandler implements
       declarations: analysis.declarations,
       imports: analysis.imports,
       exports: analysis.exports,
+      rawDeclarations: analysis.rawDeclarations,
     });
 
     if (this.factoryTracker !== null) {
@@ -276,11 +302,35 @@ export class NgModuleDecoratorHandler implements
   resolve(node: ClassDeclaration, analysis: Readonly<NgModuleAnalysis>):
       ResolveResult<NgModuleResolution> {
     const scope = this.scopeRegistry.getScopeOfModule(node);
-    const diagnostics = this.scopeRegistry.getDiagnosticsOfModule(node) || undefined;
+    const diagnostics: ts.Diagnostic[] = [];
+
+    const scopeDiagnostics = this.scopeRegistry.getDiagnosticsOfModule(node);
+    if (scopeDiagnostics !== null) {
+      diagnostics.push(...scopeDiagnostics);
+    }
+
     const data: NgModuleResolution = {
       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.
@@ -302,12 +352,15 @@ export class NgModuleDecoratorHandler implements
       }
     }
 
+    if (diagnostics.length > 0) {
+      return {diagnostics};
+    }
+
     if (scope === null || scope.reexports === null) {
-      return {data, diagnostics};
+      return {data};
     } else {
       return {
         data,
-        diagnostics,
         reexports: scope.reexports,
       };
     }

packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts

@@ -14,11 +14,12 @@ import {DefaultImportRecorder, Reference} from '../../imports';
 import {MetadataRegistry} from '../../metadata';
 import {PartialEvaluator} from '../../partial_evaluator';
 import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection';
-import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform';
+import {LocalModuleScopeRegistry} from '../../scope';
+import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform';
 
 import {compileNgFactoryDefField} from './factory';
 import {generateSetClassMetadataCall} from './metadata';
-import {findAngularDecorator, getValidConstructorDependencies, unwrapExpression} from './util';
+import {findAngularDecorator, getValidConstructorDependencies, makeDuplicateDeclarationError, unwrapExpression} from './util';
 
 export interface PipeHandlerData {
   meta: R3PipeMetadata;
@@ -28,8 +29,8 @@ export interface PipeHandlerData {
 export class PipeDecoratorHandler implements DecoratorHandler<Decorator, PipeHandlerData, unknown> {
   constructor(
       private reflector: ReflectionHost, private evaluator: PartialEvaluator,
-      private metaRegistry: MetadataRegistry, private defaultImportRecorder: DefaultImportRecorder,
-      private isCore: boolean) {}
+      private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry,
+      private defaultImportRecorder: DefaultImportRecorder, private isCore: boolean) {}
 
   readonly precedence = HandlerPrecedence.PRIMARY;
   readonly name = PipeDecoratorHandler.name;
@@ -115,6 +116,18 @@ export class PipeDecoratorHandler implements DecoratorHandler<Decorator, PipeHan
     this.metaRegistry.registerPipeMetadata({ref, name: analysis.meta.pipeName});
   }
 
+  resolve(node: ClassDeclaration): ResolveResult<unknown> {
+    const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node);
+    if (duplicateDeclData !== null) {
+      // This pipe was declared twice (or more).
+      return {
+        diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Pipe')],
+      };
+    }
+
+    return {};
+  }
+
   compile(node: ClassDeclaration, analysis: Readonly<PipeHandlerData>): CompileResult[] {
     const meta = analysis.meta;
     const res = compilePipeFromMetadata(meta);

packages/compiler-cli/src/ngtsc/annotations/src/util.ts

@@ -9,10 +9,11 @@
 import {Expression, ExternalExpr, R3DependencyMetadata, R3Reference, R3ResolvedDependencyType, WrappedNodeExpr} from '@angular/compiler';
 import * as ts from 'typescript';
 
-import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
+import {ErrorCode, FatalDiagnosticError, makeDiagnostic} from '../../diagnostics';
 import {DefaultImportRecorder, ImportMode, Reference, ReferenceEmitter} from '../../imports';
 import {ForeignFunctionResolver, PartialEvaluator} from '../../partial_evaluator';
 import {ClassDeclaration, CtorParameter, Decorator, Import, ReflectionHost, TypeValueReference, isNamedClassDeclaration} from '../../reflection';
+import {DeclarationData} from '../../scope';
 
 export enum ConstructorDepErrorKind {
   NO_SUITABLE_TOKEN,
@@ -375,4 +376,62 @@ const parensWrapperTransformerFactory: ts.TransformerFactory<ts.Expression> =
  */
 export function wrapFunctionExpressionsInParens(expression: ts.Expression): ts.Expression {
   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.
+ *
+ * The resulting `ts.Diagnostic` will have a context entry for each NgModule showing the point where
+ * the directive/pipe exists in its `declarations` (if possible).
+ */
+export function makeDuplicateDeclarationError(
+    node: ClassDeclaration, data: DeclarationData[], kind: string): ts.Diagnostic {
+  const context: {node: ts.Node; messageText: string;}[] = [];
+  for (const decl of data) {
+    if (decl.rawDeclarations === null) {
+      continue;
+    }
+    // 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);
+    context.push({
+      node: contextNode,
+      messageText:
+          `'${node.name.text}' is listed in the declarations of the NgModule '${decl.ngModule.name.text}'.`,
+    });
+  }
+
+  // Finally, produce the diagnostic.
+  return makeDiagnostic(
+      ErrorCode.NGMODULE_DECLARATION_NOT_UNIQUE, node.name,
+      `The ${kind} '${node.name.text}' is declared by more than one NgModule.`, context);
+}

packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts

@@ -49,7 +49,7 @@ runInEachFileSystem(() => {
           metaReader, new MetadataDtsModuleScopeResolver(dtsReader, null), new ReferenceEmitter([]),
           null);
       const handler = new DirectiveDecoratorHandler(
-          reflectionHost, evaluator, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
+          reflectionHost, evaluator, scopeRegistry, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
           /* isCore */ false, /* annotateForClosureCompiler */ false);
 
       const analyzeDirective = (dirName: string) => {

packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts

@@ -70,6 +70,11 @@ export enum ErrorCode {
    */
   NGMODULE_REEXPORT_NAME_COLLISION = 6006,
 
+  /**
+   * Raised when a directive/pipe is part of the declarations of two or more NgModules.
+   */
+  NGMODULE_DECLARATION_NOT_UNIQUE = 6007,
+
   /**
    * Raised when ngcc tries to inject a synthetic decorator over one that already exists.
    */

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

@@ -115,6 +115,26 @@ export class Reference<T extends ts.Node = ts.Node> {
     return this.identifiers.find(id => id.getSourceFile() === context) || null;
   }
 
+  /**
+   * Get a `ts.Identifier` for this `Reference` that exists within the given expression.
+   *
+   * This is very useful for producing `ts.Diagnostic`s that reference `Reference`s that were
+   * extracted from some larger expression, as it can be used to pinpoint the `ts.Identifier` within
+   * the expression from which the `Reference` originated.
+   */
+  getIdentityInExpression(expr: ts.Expression): ts.Identifier|null {
+    const sf = expr.getSourceFile();
+    return this.identifiers.find(id => {
+      if (id.getSourceFile() !== sf) {
+        return false;
+      }
+
+      // This identifier is a match if its position lies within the given expression.
+      return id.pos >= expr.pos && id.end <= expr.end;
+    }) ||
+        null;
+  }
+
   cloneWithAlias(alias: Expression): Reference<T> {
     const ref = new Reference(this.node, this.bestGuessOwningModule);
     ref.identifiers = [...this.identifiers];