fix(ivy): check semantics of NgModule for consistency (angular#27604)

`NgModule` requires that `Component`s/`Directive`s/`Pipe`s are listed in
declarations, and that each `Component`s/`Directive`s/`Pipe` is declared
in exactly one `NgModule`. This change adds runtime checks to ensure
that these sementics are true at runtime.

There will need to be seperate set of checks for the AoT path of the
codebase to verify that same set of semantics hold. Due to current
design there does not seem to be an easy way to share the two checks
because JIT deal with references where as AoT deals with AST nodes.

PR Close angular#27604

Author: mhevery

packages/core/src/core_render3_private_export.ts

@@ -128,6 +128,7 @@ export {
   compileNgModule as ɵcompileNgModule,
   compileNgModuleDefs as ɵcompileNgModuleDefs,
   patchComponentDefWithScope as ɵpatchComponentDefWithScope,
+  resetCompiledComponents as ɵresetCompiledComponents,
 } from './render3/jit/module';
 export {
   compilePipe as ɵcompilePipe,
@@ -224,6 +225,8 @@ export {
   SWITCH_RENDERER2_FACTORY__POST_R3__ as ɵSWITCH_RENDERER2_FACTORY__POST_R3__,
 } from './render/api';
 
+export {getModuleFactory__POST_R3__ as ɵgetModuleFactory__POST_R3__} from './linker/ng_module_factory_loader';
+
 export {
   publishGlobalUtil as ɵpublishGlobalUtil,
   publishDefaultGlobalUtils as ɵpublishDefaultGlobalUtils

packages/core/src/di/defs.ts

@@ -168,7 +168,7 @@ export function defineInjector(options: {factory: () => any, providers?: any[],
  * @param type type which may have `ngInjectableDef`
  */
 export function getInjectableDef<T>(type: any): InjectableDef<T>|null {
-  return type.hasOwnProperty(NG_INJECTABLE_DEF) ? (type as any)[NG_INJECTABLE_DEF] : null;
+  return type && type.hasOwnProperty(NG_INJECTABLE_DEF) ? (type as any)[NG_INJECTABLE_DEF] : null;
 }
 
 /**
@@ -177,5 +177,5 @@ export function getInjectableDef<T>(type: any): InjectableDef<T>|null {
  * @param type type which may have `ngInjectorDef`
  */
 export function getInjectorDef<T>(type: any): InjectorDef<T>|null {
-  return type.hasOwnProperty(NG_INJECTOR_DEF) ? (type as any)[NG_INJECTOR_DEF] : null;
+  return type && type.hasOwnProperty(NG_INJECTOR_DEF) ? (type as any)[NG_INJECTOR_DEF] : null;
 }

packages/core/src/di/r3_injector.ts

@@ -111,8 +111,9 @@ export class R3Injector {
     const dedupStack: InjectorType<any>[] = [];
     deepForEach([def], injectorDef => this.processInjectorType(injectorDef, [], dedupStack));
 
-    additionalProviders &&
-        deepForEach(additionalProviders, provider => this.processProvider(provider));
+    additionalProviders && deepForEach(
+                               additionalProviders, provider => this.processProvider(
+                                                        provider, def, additionalProviders));
 
 
     // Make sure the INJECTOR token provides this injector.
@@ -270,25 +271,31 @@ export class R3Injector {
     }
 
     // Next, include providers listed on the definition itself.
-    if (def.providers != null && !isDuplicate) {
-      deepForEach(def.providers, provider => this.processProvider(provider));
+    const defProviders = def.providers;
+    if (defProviders != null && !isDuplicate) {
+      const injectorType = defOrWrappedDef as InjectorType<any>;
+      deepForEach(
+          defProviders, provider => this.processProvider(provider, injectorType, defProviders));
     }
 
     // Finally, include providers from an InjectorDefTypeWithProviders if there was one.
-    deepForEach(providers, provider => this.processProvider(provider));
+    const ngModuleType = (defOrWrappedDef as InjectorTypeWithProviders<any>).ngModule;
+    deepForEach(providers, provider => this.processProvider(provider, ngModuleType, providers));
   }
 
   /**
    * Process a `SingleProvider` and add it.
    */
-  private processProvider(provider: SingleProvider): void {
+  private processProvider(
+      provider: SingleProvider, ngModuleType: InjectorType<any>, providers: any[]): void {
     // Determine the token from the provider. Either it's its own token, or has a {provide: ...}
     // property.
     provider = resolveForwardRef(provider);
-    let token: any = isTypeProvider(provider) ? provider : resolveForwardRef(provider.provide);
+    let token: any =
+        isTypeProvider(provider) ? provider : resolveForwardRef(provider && provider.provide);
 
     // Construct a `Record` for the provider.
-    const record = providerToRecord(provider);
+    const record = providerToRecord(provider, ngModuleType, providers);
 
     if (!isTypeProvider(provider) && provider.multi === true) {
       // If the provider indicates that it's a multi-provider, process it specially.
@@ -317,7 +324,7 @@ export class R3Injector {
 
   private hydrate<T>(token: Type<T>|InjectionToken<T>, record: Record<T>): T {
     if (record.value === CIRCULAR) {
-      throw new Error(`Circular dep for ${stringify(token)}`);
+      throw new Error(`Cannot instantiate cyclic dependency! ${stringify(token)}`);
     } else if (record.value === NOT_YET) {
       record.value = CIRCULAR;
       record.value = record.factory !();
@@ -345,20 +352,25 @@ function injectableDefOrInjectorDefFactory(token: Type<any>| InjectionToken<any>
     const injectorDef = getInjectorDef(token as InjectorType<any>);
     if (injectorDef !== null) {
       return injectorDef.factory;
-    }
-
-    if (token instanceof InjectionToken) {
+    } else if (token instanceof InjectionToken) {
       throw new Error(`Token ${stringify(token)} is missing an ngInjectableDef definition.`);
+    } else if (token instanceof Function) {
+      const paramLength = token.length;
+      if (paramLength > 0) {
+        const args: string[] = new Array(paramLength).fill('?');
+        throw new Error(
+            `Can't resolve all parameters for ${stringify(token)}: (${args.join(', ')}).`);
+      }
+      return () => new (token as Type<any>)();
     }
-    // TODO(alxhub): there should probably be a strict mode which throws here instead of assuming a
-    // no-args constructor.
-    return () => new (token as Type<any>)();
+    throw new Error('unreachable');
   }
   return injectableDef.factory;
 }
 
-function providerToRecord(provider: SingleProvider): Record<any> {
-  let factory: (() => any)|undefined = providerToFactory(provider);
+function providerToRecord(
+    provider: SingleProvider, ngModuleType: InjectorType<any>, providers: any[]): Record<any> {
+  let factory: (() => any)|undefined = providerToFactory(provider, ngModuleType, providers);
   if (isValueProvider(provider)) {
     return makeRecord(undefined, provider.useValue);
   } else {
@@ -371,7 +383,8 @@ function providerToRecord(provider: SingleProvider): Record<any> {
  *
  * @param provider provider to convert to factory
  */
-export function providerToFactory(provider: SingleProvider): () => any {
+export function providerToFactory(
+    provider: SingleProvider, ngModuleType?: InjectorType<any>, providers?: any[]): () => any {
   let factory: (() => any)|undefined = undefined;
   if (isTypeProvider(provider)) {
     return injectableDefOrInjectorDefFactory(resolveForwardRef(provider));
@@ -384,7 +397,18 @@ export function providerToFactory(provider: SingleProvider): () => any {
       factory = () => provider.useFactory(...injectArgs(provider.deps || []));
     } else {
       const classRef = resolveForwardRef(
-          (provider as StaticClassProvider | ClassProvider).useClass || provider.provide);
+          provider &&
+          ((provider as StaticClassProvider | ClassProvider).useClass || provider.provide));
+      if (!classRef) {
+        let ngModuleDetail = '';
+        if (ngModuleType && providers) {
+          const providerDetail = providers.map(v => v == provider ? '?' + provider + '?' : '...');
+          ngModuleDetail =
+              ` - only instances of Provider and Type are allowed, got: [${providerDetail.join(', ')}]`;
+        }
+        throw new Error(
+            `Invalid provider for the NgModule '${stringify(ngModuleType)}'` + ngModuleDetail);
+      }
       if (hasDeps(provider)) {
         factory = () => new (classRef)(...injectArgs(provider.deps));
       } else {
@@ -409,15 +433,15 @@ function deepForEach<T>(input: (T | any[])[], fn: (value: T) => void): void {
 }
 
 function isValueProvider(value: SingleProvider): value is ValueProvider {
-  return USE_VALUE in value;
+  return value && typeof value == 'object' && USE_VALUE in value;
 }
 
 function isExistingProvider(value: SingleProvider): value is ExistingProvider {
-  return !!(value as ExistingProvider).useExisting;
+  return !!(value && (value as ExistingProvider).useExisting);
 }
 
 function isFactoryProvider(value: SingleProvider): value is FactoryProvider {
-  return !!(value as FactoryProvider).useFactory;
+  return !!(value && (value as FactoryProvider).useFactory);
 }
 
 export function isTypeProvider(value: SingleProvider): value is TypeProvider {

packages/core/src/linker/ng_module_factory_loader.ts

@@ -6,6 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {NgModuleFactory as R3NgModuleFactory, NgModuleType} from '../render3/ng_module_ref';
+import {Type} from '../type';
+import {stringify} from '../util';
 import {NgModuleFactory} from './ng_module_factory';
 
 /**
@@ -17,23 +20,50 @@ export abstract class NgModuleFactoryLoader {
   abstract load(path: string): Promise<NgModuleFactory<any>>;
 }
 
-let moduleFactories = new Map<string, NgModuleFactory<any>>();
+/**
+ * Map of module-id to the corresponding NgModule.
+ * - In pre Ivy we track NgModuleFactory,
+ * - In post Ivy we track the NgModuleType
+ */
+const modules = new Map<string, NgModuleFactory<any>|NgModuleType>();
 
 /**
  * Registers a loaded module. Should only be called from generated NgModuleFactory code.
  * @publicApi
  */
 export function registerModuleFactory(id: string, factory: NgModuleFactory<any>) {
-  const existing = moduleFactories.get(id);
-  if (existing) {
-    throw new Error(`Duplicate module registered for ${id
-                    } - ${existing.moduleType.name} vs ${factory.moduleType.name}`);
+  const existing = modules.get(id) as NgModuleFactory<any>;
+  assertNotExisting(id, existing && existing.moduleType);
+  modules.set(id, factory);
+}
+
+function assertNotExisting(id: string, type: Type<any>| null): void {
+  if (type) {
+    throw new Error(
+        `Duplicate module registered for ${id} - ${stringify(type)} vs ${stringify(type.name)}`);
   }
-  moduleFactories.set(id, factory);
 }
 
-export function clearModulesForTest() {
-  moduleFactories = new Map<string, NgModuleFactory<any>>();
+export function registerNgModuleType(id: string, ngModuleType: NgModuleType) {
+  const existing = modules.get(id) as NgModuleType | null;
+  assertNotExisting(id, existing);
+  modules.set(id, ngModuleType);
+}
+
+export function clearModulesForTest(): void {
+  modules.clear();
+}
+
+export function getModuleFactory__PRE_R3__(id: string): NgModuleFactory<any> {
+  const factory = modules.get(id) as NgModuleFactory<any>| null;
+  if (!factory) throw noModuleError(id);
+  return factory;
+}
+
+export function getModuleFactory__POST_R3__(id: string): NgModuleFactory<any> {
+  const type = modules.get(id) as NgModuleType | null;
+  if (!type) throw noModuleError(id);
+  return new R3NgModuleFactory(type);
 }
 
 /**
@@ -42,8 +72,8 @@ export function clearModulesForTest() {
  * cannot be found.
  * @publicApi
  */
-export function getModuleFactory(id: string): NgModuleFactory<any> {
-  const factory = moduleFactories.get(id);
-  if (!factory) throw new Error(`No module with ID ${id} loaded`);
-  return factory;
+export const getModuleFactory: (id: string) => NgModuleFactory<any> = getModuleFactory__PRE_R3__;
+
+function noModuleError(id: string, ): Error {
+  return new Error(`No module with ID ${id} loaded`);
 }

packages/core/src/metadata/ng_module.ts

@@ -10,6 +10,7 @@ import {ApplicationRef} from '../application_ref';
 import {InjectorType, defineInjector} from '../di/defs';
 import {Provider} from '../di/provider';
 import {convertInjectableProviderToFactory} from '../di/util';
+import {NgModuleType} from '../render3';
 import {compileNgModule as render3CompileNgModule} from '../render3/jit/module';
 import {Type} from '../type';
 import {TypeDecorator, makeDecorator} from '../util/decorators';
@@ -337,7 +338,7 @@ export const NgModule: NgModuleDecorator = makeDecorator(
      * * The `imports` and `exports` options bring in members from other modules, and make
      * this module's members available to others.
      */
-    (type: Type<any>, meta: NgModule) => SWITCH_COMPILE_NGMODULE(type, meta));
+    (type: NgModuleType, meta: NgModule) => SWITCH_COMPILE_NGMODULE(type, meta));
 
 /**
  * @description

packages/core/src/render3/definition.ts

@@ -13,7 +13,7 @@ import {Provider} from '../di/provider';
 import {NgModuleDef} from '../metadata/ng_module';
 import {ViewEncapsulation} from '../metadata/view';
 import {Mutable, Type} from '../type';
-import {noSideEffects} from '../util';
+import {noSideEffects, stringify} from '../util';
 
 import {NG_COMPONENT_DEF, NG_DIRECTIVE_DEF, NG_MODULE_DEF, NG_PIPE_DEF} from './fields';
 import {BaseDef, ComponentDef, ComponentDefFeature, ComponentQuery, ComponentTemplate, ComponentType, DirectiveDef, DirectiveDefFeature, DirectiveType, DirectiveTypesOrFactory, HostBindingsFunction, PipeDef, PipeType, PipeTypesOrFactory} from './interfaces/definition';
@@ -651,6 +651,12 @@ export function getPipeDef<T>(type: any): PipeDef<T>|null {
   return (type as any)[NG_PIPE_DEF] || null;
 }
 
-export function getNgModuleDef<T>(type: any): NgModuleDef<T>|null {
-  return (type as any)[NG_MODULE_DEF] || null;
+export function getNgModuleDef<T>(type: any, throwNotFound: true): NgModuleDef<T>;
+export function getNgModuleDef<T>(type: any): NgModuleDef<T>|null;
+export function getNgModuleDef<T>(type: any, throwNotFound?: boolean): NgModuleDef<T>|null {
+  const ngModuleDef = (type as any)[NG_MODULE_DEF] || null;
+  if (!ngModuleDef && throwNotFound === true) {
+    throw new Error(`Type ${stringify(type)} does not have 'ngModuleDef' property.`);
+  }
+  return ngModuleDef;
 }

packages/core/src/render3/jit/compiler_facade_interface.ts

@@ -101,8 +101,8 @@ export interface R3InjectorMetadataFacade {
   name: string;
   type: any;
   deps: R3DependencyMetadataFacade[]|null;
-  providers: any;
-  imports: any;
+  providers: any[];
+  imports: any[];
 }
 
 export interface R3DirectiveMetadataFacade {

packages/core/src/render3/jit/directive.ts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {ComponentType} from '..';
 import {Query} from '../../metadata/di';
 import {Component, Directive} from '../../metadata/directives';
 import {componentNeedsResolution, maybeQueueResolutionOfComponentResources} from '../../metadata/resource_loading';
@@ -58,7 +59,7 @@ export function compileComponent(type: Type<any>, metadata: Component): void {
           preserveWhitespaces: metadata.preserveWhitespaces || false,
           styles: metadata.styles || EMPTY_ARRAY,
           animations: metadata.animations,
-          viewQueries: extractQueriesMetadata(getReflect().propMetadata(type), isViewQuery),
+          viewQueries: extractQueriesMetadata(type, getReflect().propMetadata(type), isViewQuery),
           directives: [],
           pipes: new Map(),
           encapsulation: metadata.encapsulation || ViewEncapsulation.Emulated,
@@ -108,7 +109,7 @@ export function compileDirective(type: Type<any>, directive: Directive): void {
   Object.defineProperty(type, NG_DIRECTIVE_DEF, {
     get: () => {
       if (ngDirectiveDef === null) {
-        const facade = directiveMetadata(type, directive);
+        const facade = directiveMetadata(type as ComponentType<any>, directive);
         ngDirectiveDef = getCompilerFacade().compileDirective(
             angularCoreEnv, `ng://${type && type.name}/ngDirectiveDef.js`, facade);
       }
@@ -141,7 +142,7 @@ function directiveMetadata(type: Type<any>, metadata: Directive): R3DirectiveMet
     propMetadata: propMetadata,
     inputs: metadata.inputs || EMPTY_ARRAY,
     outputs: metadata.outputs || EMPTY_ARRAY,
-    queries: extractQueriesMetadata(propMetadata, isContentQuery),
+    queries: extractQueriesMetadata(type, propMetadata, isContentQuery),
     lifecycle: {
       usesOnChanges: type.prototype.ngOnChanges !== undefined,
     },
@@ -168,13 +169,18 @@ export function convertToR3QueryMetadata(propertyName: string, ann: Query): R3Qu
   };
 }
 function extractQueriesMetadata(
-    propMetadata: {[key: string]: any[]},
+    type: Type<any>, propMetadata: {[key: string]: any[]},
     isQueryAnn: (ann: any) => ann is Query): R3QueryMetadataFacade[] {
   const queriesMeta: R3QueryMetadataFacade[] = [];
   for (const field in propMetadata) {
     if (propMetadata.hasOwnProperty(field)) {
       propMetadata[field].forEach(ann => {
         if (isQueryAnn(ann)) {
+          if (!ann.selector) {
+            throw new Error(
+                `Can't construct a query for the property "${field}" of ` +
+                `"${stringify(type)}" since the query selector wasn't defined.`);
+          }
           queriesMeta.push(convertToR3QueryMetadata(field, ann));
         }
       });