fix(ivy): validate the NgModule declarations field (#34404)

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.

PR Close #34404

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.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
-        this.isCore, /* annotateForClosureCompiler */ false) as DecoratorHandler<unknown, unknown, unknown>,
+        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.reflectionHost, this.evaluator, this.metaRegistry, this.scopeRegistry,
-        this.isCore),
+        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 metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry,
-      private isCore: boolean, private annotateForClosureCompiler: boolean) {}
+      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') !;
+      rawDeclarations = ngModule.get('declarations') !;
-      const declarationMeta = this.evaluator.evaluate(expr, forwardRefResolver);
+      const declarationMeta = this.evaluator.evaluate(rawDeclarations, forwardRefResolver);
-      declarationRefs = this.resolveTypeList(expr, declarationMeta, name, 'declarations');
+      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 metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry,
-      private isCore: boolean) {}
+      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];

packages/compiler-cli/src/ngtsc/metadata/src/api.ts

@@ -7,10 +7,12 @@
  */
 
 import {DirectiveMeta as T2DirectiveMeta, SchemaMetadata} from '@angular/compiler';
+import * as ts from 'typescript';
 
 import {Reference} from '../../imports';
 import {ClassDeclaration} from '../../reflection';
 
+
 /**
  * Metadata collected for an `NgModule`.
  */
@@ -20,6 +22,14 @@ export interface NgModuleMeta {
   imports: Reference<ClassDeclaration>[];
   exports: Reference<ClassDeclaration>[];
   schemas: SchemaMetadata[];
+
+  /**
+   * The raw `ts.Expression` which gave rise to `declarations`, if one exists.
+   *
+   * If this is `null`, then either no declarations exist, or no expression was available (likely
+   * because the module came from a .d.ts file).
+   */
+  rawDeclarations: ts.Expression|null;
 }
 
 /**

packages/compiler-cli/src/ngtsc/metadata/src/dts.ts

@@ -55,6 +55,7 @@ export class DtsMetadataReader implements MetadataReader {
       imports: extractReferencesFromType(
           this.checker, importMetadata, ref.ownedByModuleGuess, resolutionContext),
       schemas: [],
+      rawDeclarations: null,
     };
   }
 

packages/compiler-cli/src/ngtsc/program.ts

@@ -661,13 +661,16 @@ export class NgtscProgram implements api.Program {
           this.incrementalDriver.depGraph, this.closureCompilerEnabled),
       // TODO(alxhub): understand why the cast here is necessary (something to do with `null` not
       // being assignable to `unknown` when wrapped in `Readonly`).
+      // clang-format off
       new DirectiveDecoratorHandler(
-          this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore,
+          this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.defaultImportTracker,
-          this.closureCompilerEnabled) as Readonly<DecoratorHandler<unknown, unknown, unknown>>,
+          this.isCore, this.closureCompilerEnabled) as Readonly<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.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore),
+          this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.defaultImportTracker,
+          this.isCore),
       new InjectableDecoratorHandler(
           this.reflector, this.defaultImportTracker, this.isCore,
           this.options.strictInjectionParameters || false),

packages/compiler-cli/src/ngtsc/scope/index.ts

@@ -9,4 +9,4 @@
 export {ExportScope, ScopeData} from './src/api';
 export {ComponentScopeReader, CompoundComponentScopeReader} from './src/component_scope';
 export {DtsModuleScopeResolver, MetadataDtsModuleScopeResolver} from './src/dependency';
-export {LocalModuleScope, LocalModuleScopeRegistry, LocalNgModuleData} from './src/local';
+export {DeclarationData, LocalModuleScope, LocalModuleScopeRegistry, LocalNgModuleData} from './src/local';

packages/compiler-cli/src/ngtsc/scope/src/local.ts

@@ -75,7 +75,14 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
    * contain directives. This doesn't cause any problems but isn't useful as there is no concept of
    * a directive's compilation scope.
    */
-  private declarationToModule = new Map<ClassDeclaration, ClassDeclaration>();
+  private declarationToModule = new Map<ClassDeclaration, DeclarationData>();
+
+  /**
+   * This maps from the directive/pipe class to a map of data for each NgModule that declares the
+   * directive/pipe. This data is needed to produce an error for the given class.
+   */
+  private duplicateDeclarations =
+      new Map<ClassDeclaration, Map<ClassDeclaration, DeclarationData>>();
 
   private moduleToRef = new Map<ClassDeclaration, Reference<ClassDeclaration>>();
 
@@ -111,9 +118,12 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
    */
   registerNgModuleMetadata(data: NgModuleMeta): void {
     this.assertCollecting();
+    const ngModule = data.ref.node;
     this.moduleToRef.set(data.ref.node, data.ref);
+    // Iterate over the module's declarations, and add them to declarationToModule. If duplicates
+    // are found, they're instead tracked in duplicateDeclarations.
     for (const decl of data.declarations) {
-      this.declarationToModule.set(decl.node, data.ref.node);
+      this.registerDeclarationOfModule(ngModule, decl, data.rawDeclarations);
     }
   }
 
@@ -124,10 +134,25 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
   getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
     const scope = !this.declarationToModule.has(clazz) ?
         null :
-        this.getScopeOfModule(this.declarationToModule.get(clazz) !);
+        this.getScopeOfModule(this.declarationToModule.get(clazz) !.ngModule);
     return scope;
   }
 
+  /**
+   * If `node` is declared in more than one NgModule (duplicate declaration), then get the
+   * `DeclarationData` for each offending declaration.
+   *
+   * Ordinarily a class is only declared in one NgModule, in which case this function returns
+   * `null`.
+   */
+  getDuplicateDeclarations(node: ClassDeclaration): DeclarationData[]|null {
+    if (!this.duplicateDeclarations.has(node)) {
+      return null;
+    }
+
+    return Array.from(this.duplicateDeclarations.get(node) !.values());
+  }
+
   /**
    * Collects registered data for a module and its directives/pipes and convert it into a full
    * `LocalModuleScope`.
@@ -165,15 +190,51 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
    */
   getCompilationScopes(): CompilationScope[] {
     const scopes: CompilationScope[] = [];
-    this.declarationToModule.forEach((ngModule, declaration) => {
+    this.declarationToModule.forEach((declData, declaration) => {
-      const scope = this.getScopeOfModule(ngModule);
+      const scope = this.getScopeOfModule(declData.ngModule);
       if (scope !== null) {
-        scopes.push({declaration, ngModule, ...scope.compilation});
+        scopes.push({declaration, ngModule: declData.ngModule, ...scope.compilation});
       }
     });
     return scopes;
   }
 
+  private registerDeclarationOfModule(
+      ngModule: ClassDeclaration, decl: Reference<ClassDeclaration>,
+      rawDeclarations: ts.Expression|null): void {
+    const declData: DeclarationData = {
+      ngModule,
+      ref: decl, rawDeclarations,
+    };
+
+    // First, check for duplicate declarations of the same directive/pipe.
+    if (this.duplicateDeclarations.has(decl.node)) {
+      // This directive/pipe has already been identified as being duplicated. Add this module to the
+      // map of modules for which a duplicate declaration exists.
+      this.duplicateDeclarations.get(decl.node) !.set(ngModule, declData);
+    } else if (
+        this.declarationToModule.has(decl.node) &&
+        this.declarationToModule.get(decl.node) !.ngModule !== ngModule) {
+      // This directive/pipe is already registered as declared in another module. Mark it as a
+      // duplicate instead.
+      const duplicateDeclMap = new Map<ClassDeclaration, DeclarationData>();
+      const firstDeclData = this.declarationToModule.get(decl.node) !;
+
+      // Being detected as a duplicate means there are two NgModules (for now) which declare this
+      // directive/pipe. Add both of them to the duplicate tracking map.
+      duplicateDeclMap.set(firstDeclData.ngModule, firstDeclData);
+      duplicateDeclMap.set(ngModule, declData);
+      this.duplicateDeclarations.set(decl.node, duplicateDeclMap);
+
+      // Remove the directive/pipe from `declarationToModule` as it's a duplicate declaration, and
+      // therefore not valid.
+      this.declarationToModule.delete(decl.node);
+    } else {
+      // This is the first declaration of this directive/pipe, so map it.
+      this.declarationToModule.set(decl.node, declData);
+    }
+  }
+
   /**
    * Implementation of `getScopeOfModule` which accepts a reference to a class and differentiates
    * between:
@@ -514,3 +575,9 @@ function reexportCollision(
         {node: refB.node.name, messageText: childMessageText},
       ]);
 }
+
+export interface DeclarationData {
+  ngModule: ClassDeclaration;
+  ref: Reference;
+  rawDeclarations: ts.Expression|null;
+}

packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts

@@ -53,6 +53,7 @@ describe('LocalModuleScopeRegistry', () => {
       declarations: [Dir1, Dir2, Pipe1],
       exports: [Dir1, Pipe1],
       schemas: [],
+      rawDeclarations: null,
     });
 
     const scope = scopeRegistry.getScopeOfModule(Module.node) !;
@@ -69,6 +70,7 @@ describe('LocalModuleScopeRegistry', () => {
       declarations: [DirA],
       exports: [],
       schemas: [],
+      rawDeclarations: null,
     });
     metaRegistry.registerNgModuleMetadata({
       ref: new Reference(ModuleB.node),
@@ -76,6 +78,7 @@ describe('LocalModuleScopeRegistry', () => {
       declarations: [DirB],
       imports: [],
       schemas: [],
+      rawDeclarations: null,
     });
     metaRegistry.registerNgModuleMetadata({
       ref: new Reference(ModuleC.node),
@@ -83,6 +86,7 @@ describe('LocalModuleScopeRegistry', () => {
       exports: [DirCE],
       imports: [],
       schemas: [],
+      rawDeclarations: null,
     });
 
     const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !;
@@ -99,6 +103,7 @@ describe('LocalModuleScopeRegistry', () => {
       imports: [],
       declarations: [],
       schemas: [],
+      rawDeclarations: null,
     });
     metaRegistry.registerNgModuleMetadata({
       ref: new Reference(ModuleB.node),
@@ -106,6 +111,7 @@ describe('LocalModuleScopeRegistry', () => {
       exports: [Dir],
       imports: [],
       schemas: [],
+      rawDeclarations: null,
     });
 
     const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !;
@@ -122,6 +128,7 @@ describe('LocalModuleScopeRegistry', () => {
       imports: [ModuleB, ModuleC],
       exports: [DirA, DirA, DirB, ModuleB],
       schemas: [],
+      rawDeclarations: null,
     });
     metaRegistry.registerNgModuleMetadata({
       ref: new Reference(ModuleB.node),
@@ -129,6 +136,7 @@ describe('LocalModuleScopeRegistry', () => {
       imports: [],
       exports: [DirB],
       schemas: [],
+      rawDeclarations: null,
     });
     metaRegistry.registerNgModuleMetadata({
       ref: new Reference(ModuleC.node),
@@ -136,6 +144,7 @@ describe('LocalModuleScopeRegistry', () => {
       imports: [],
       exports: [ModuleB],
       schemas: [],
+      rawDeclarations: null,
     });
 
     const scope = scopeRegistry.getScopeOfModule(ModuleA.node) !;
@@ -159,6 +168,7 @@ describe('LocalModuleScopeRegistry', () => {
       imports: [],
       declarations: [DirInModule],
       schemas: [],
+      rawDeclarations: null,
     });
 
     const scope = scopeRegistry.getScopeOfModule(Module.node) !;
@@ -174,6 +184,7 @@ describe('LocalModuleScopeRegistry', () => {
       imports: [ModuleB],
       declarations: [],
       schemas: [],
+      rawDeclarations: null,
     });
     metaRegistry.registerNgModuleMetadata({
       ref: new Reference(ModuleB.node),
@@ -181,6 +192,7 @@ describe('LocalModuleScopeRegistry', () => {
       exports: [Dir],
       imports: [],
       schemas: [],
+      rawDeclarations: null,
     });
 
     const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !;
@@ -196,6 +208,7 @@ describe('LocalModuleScopeRegistry', () => {
       imports: [],
       declarations: [],
       schemas: [],
+      rawDeclarations: null,
     });
     metaRegistry.registerNgModuleMetadata({
       ref: new Reference(ModuleB.node),
@@ -203,6 +216,7 @@ describe('LocalModuleScopeRegistry', () => {
       exports: [Dir],
       imports: [],
       schemas: [],
+      rawDeclarations: null,
     });
 
     expect(scopeRegistry.getScopeOfModule(ModuleA.node)).toBe(null);

packages/compiler-cli/test/ngtsc/scope_spec.ts

@@ -28,6 +28,129 @@ runInEachFileSystem(() => {
     });
 
     describe('diagnostics', () => {
+      describe('declarations', () => {
+        it('should detect when a random class is declared', () => {
+          env.write('test.ts', `
+            import {NgModule} from '@angular/core';
+
+            export class RandomClass {}
+
+            @NgModule({
+              declarations: [RandomClass],
+            })
+            export class Module {}
+          `);
+
+          const diags = env.driveDiagnostics();
+          expect(diags.length).toBe(1);
+          const node = diagnosticToNode(diags[0], ts.isIdentifier);
+          expect(node.text).toEqual('RandomClass');
+          expect(diags[0].messageText).toContain('is not a directive, a component, or a pipe.');
+        });
+
+        it('should detect when a declaration lives outside the current compilation', () => {
+          env.write('dir.d.ts', `
+            import {ɵɵDirectiveDefWithMeta} from '@angular/core';
+
+            export declare class ExternalDir {
+              static ɵdir: ɵɵDirectiveDefWithMeta<ExternalDir, '[test]', never, never, never, never>;
+            }
+          `);
+          env.write('test.ts', `
+            import {NgModule} from '@angular/core';
+            import {ExternalDir} from './dir';
+
+            @NgModule({
+              declarations: [ExternalDir],
+            })
+            export class Module {}
+          `);
+          const diags = env.driveDiagnostics();
+          expect(diags.length).toBe(1);
+          const node = diagnosticToNode(diags[0], ts.isIdentifier);
+          expect(node.text).toEqual('ExternalDir');
+          expect(diags[0].messageText).toContain(`not a part of the current compilation`);
+        });
+
+        it('should detect when a declaration is shared between two modules', () => {
+          env.write('test.ts', `
+            import {Directive, NgModule} from '@angular/core';
+
+            @Directive({selector: '[test]'})
+            export class TestDir {}
+
+            @NgModule({
+              declarations: [TestDir]
+            })
+            export class ModuleA {}
+
+            @NgModule({
+              declarations: [TestDir],
+            })
+            export class ModuleB {}
+          `);
+          const diags = env.driveDiagnostics();
+          expect(diags.length).toBe(1);
+          const node = findContainingClass(diagnosticToNode(diags[0], ts.isIdentifier));
+          expect(node.name !.text).toEqual('TestDir');
+
+          const relatedNodes = new Set(diags[0].relatedInformation !.map(
+              related =>
+                  findContainingClass(diagnosticToNode(related, ts.isIdentifier)).name !.text));
+          expect(relatedNodes).toContain('ModuleA');
+          expect(relatedNodes).toContain('ModuleB');
+          expect(relatedNodes.size).toBe(2);
+        });
+
+        it('should detect when a declaration is repeated within the same module', () => {
+          env.write('test.ts', `
+            import {Directive, NgModule} from '@angular/core';
+
+            @Directive({selector: '[test]'})
+            export class TestDir {}
+
+
+            @NgModule({
+              declarations: [TestDir, TestDir],
+            })
+            export class Module {}
+          `);
+
+          const diags = env.driveDiagnostics();
+          expect(diags.length).toBe(0);
+        });
+
+        it('should detect when a declaration is shared between two modules, and is repeated within them',
+           () => {
+             env.write('test.ts', `
+              import {Directive, NgModule} from '@angular/core';
+
+              @Directive({selector: '[test]'})
+              export class TestDir {}
+
+              @NgModule({
+                declarations: [TestDir, TestDir]
+              })
+              export class ModuleA {}
+
+              @NgModule({
+                declarations: [TestDir, TestDir],
+              })
+              export class ModuleB {}
+            `);
+             const diags = env.driveDiagnostics();
+             expect(diags.length).toBe(1);
+             const node = findContainingClass(diagnosticToNode(diags[0], ts.isIdentifier));
+             expect(node.name !.text).toEqual('TestDir');
+
+             const relatedNodes = new Set(diags[0].relatedInformation !.map(
+                 related =>
+                     findContainingClass(diagnosticToNode(related, ts.isIdentifier)).name !.text));
+             expect(relatedNodes).toContain('ModuleA');
+             expect(relatedNodes).toContain('ModuleB');
+             expect(relatedNodes.size).toBe(2);
+           });
+      });
       describe('imports', () => {
         it('should emit imports in a pure function call', () => {
           env.write('test.ts', `
@@ -182,8 +305,9 @@ runInEachFileSystem(() => {
   });
 
   function diagnosticToNode<T extends ts.Node>(
-      diagnostic: ts.Diagnostic | Diagnostic, guard: (node: ts.Node) => node is T): T {
+      diagnostic: ts.Diagnostic | Diagnostic | ts.DiagnosticRelatedInformation,
-    const diag = diagnostic as ts.Diagnostic;
+      guard: (node: ts.Node) => node is T): T {
+    const diag = diagnostic as ts.Diagnostic | ts.DiagnosticRelatedInformation;
     if (diag.file === undefined) {
       throw new Error(`Expected ts.Diagnostic to have a file source`);
     }
@@ -192,3 +316,14 @@ runInEachFileSystem(() => {
     return node as T;
   }
 });
+
+function findContainingClass(node: ts.Node): ts.ClassDeclaration {
+  while (!ts.isClassDeclaration(node)) {
+    if (node.parent && node.parent !== node) {
+      node = node.parent;
+    } else {
+      throw new Error('Expected node to have a ClassDeclaration parent');
+    }
+  }
+  return node;
+}