fix(ivy): align NgModule assertions with ViewEngine (#31543)

Aligns Ivy's `NgModule` assertion messages with the ones from `ViewEngine` and adds a few that hadn't been implemented.

PR Close #31543

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

@@ -109,6 +109,11 @@ export function compileNgModuleDefs(
     configurable: true,
     get: () => {
       if (ngModuleDef === null) {
+        if (ngDevMode && ngModule.imports && ngModule.imports.indexOf(moduleType) > -1) {
+          // We need to assert this immediately, because allowing it to continue will cause it to
+          // go into an infinite loop before we've reached the point where we throw all the errors.
+          throw new Error(`'${stringifyForError(moduleType)}' module can't import itself`);
+        }
         ngModuleDef = getCompilerFacade().compileNgModule(
             angularCoreEnv, `ng:///${moduleType.name}/ngModuleDef.js`, {
               type: moduleType,
@@ -156,17 +161,28 @@ export function compileNgModuleDefs(
 }
 
 function verifySemanticsOfNgModuleDef(
-    moduleType: NgModuleType, allowDuplicateDeclarationsInRoot: boolean): void {
+    moduleType: NgModuleType, allowDuplicateDeclarationsInRoot: boolean,
+    importingModule?: NgModuleType): void {
   if (verifiedNgModule.get(moduleType)) return;
   verifiedNgModule.set(moduleType, true);
   moduleType = resolveForwardRef(moduleType);
-  const ngModuleDef = getNgModuleDef(moduleType, true);
+  let ngModuleDef: NgModuleDef<any>;
+  if (importingModule) {
+    ngModuleDef = getNgModuleDef(moduleType) !;
+    if (!ngModuleDef) {
+      throw new Error(
+          `Unexpected value '${moduleType.name}' imported by the module '${importingModule.name}'. Please add a @NgModule annotation.`);
+    }
+  } else {
+    ngModuleDef = getNgModuleDef(moduleType, true);
+  }
   const errors: string[] = [];
   const declarations = maybeUnwrapFn(ngModuleDef.declarations);
   const imports = maybeUnwrapFn(ngModuleDef.imports);
-  flatten(imports)
-      .map(unwrapModuleWithProvidersImports)
-      .forEach(mod => verifySemanticsOfNgModuleDef(mod, false));
+  flatten(imports).map(unwrapModuleWithProvidersImports).forEach(mod => {
+    verifySemanticsOfNgModuleImport(mod, moduleType);
+    verifySemanticsOfNgModuleDef(mod, false, moduleType);
+  });
   const exports = maybeUnwrapFn(ngModuleDef.exports);
   declarations.forEach(verifyDeclarationsHaveDefinitions);
   const combinedDeclarations: Type<any>[] = [
@@ -180,9 +196,10 @@ function verifySemanticsOfNgModuleDef(
   const ngModule = getAnnotation<NgModule>(moduleType, 'NgModule');
   if (ngModule) {
     ngModule.imports &&
-        flatten(ngModule.imports)
-            .map(unwrapModuleWithProvidersImports)
-            .forEach(mod => verifySemanticsOfNgModuleDef(mod, false));
+        flatten(ngModule.imports).map(unwrapModuleWithProvidersImports).forEach(mod => {
+          verifySemanticsOfNgModuleImport(mod, moduleType);
+          verifySemanticsOfNgModuleDef(mod, false, moduleType);
+        });
     ngModule.bootstrap && ngModule.bootstrap.forEach(verifyCorrectBootstrapType);
     ngModule.bootstrap && ngModule.bootstrap.forEach(verifyComponentIsPartOfNgModule);
     ngModule.entryComponents && ngModule.entryComponents.forEach(verifyComponentIsPartOfNgModule);
@@ -260,6 +277,20 @@ function verifySemanticsOfNgModuleDef(
       }
     }
   }
+
+  function verifySemanticsOfNgModuleImport(type: Type<any>, importingModule: Type<any>) {
+    type = resolveForwardRef(type);
+
+    if (getComponentDef(type) || getDirectiveDef(type)) {
+      throw new Error(
+          `Unexpected directive '${type.name}' imported by the module '${importingModule.name}'. Please add a @NgModule annotation.`);
+    }
+
+    if (getPipeDef(type)) {
+      throw new Error(
+          `Unexpected pipe '${type.name}' imported by the module '${importingModule.name}'. Please add a @NgModule annotation.`);
+    }
+  }
 }
 
 function unwrapModuleWithProvidersImports(

packages/core/test/acceptance/integration_spec.ts

@@ -6,12 +6,12 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import {CommonModule} from '@angular/common';
-import {Component, ContentChild, Directive, ElementRef, EventEmitter, HostBinding, HostListener, Input, NgModule, OnInit, Output, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core';
+import {Component, ContentChild, Directive, ElementRef, EventEmitter, HostBinding, HostListener, Input, NgModule, OnInit, Output, Pipe, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core';
 import {ngDevModeResetPerfCounters} from '@angular/core/src/util/ng_dev_mode';
 import {TestBed} from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
-import {onlyInIvy} from '@angular/private/testing';
+import {ivyEnabled, onlyInIvy} from '@angular/private/testing';
 
 describe('acceptance integration tests', () => {
   function stripHtmlComments(str: string) { return str.replace(/<!--[\s\S]*?-->/g, ''); }
@@ -1401,6 +1401,111 @@ describe('acceptance integration tests', () => {
     });
   });
 
+  describe('NgModule assertions', () => {
+    it('should throw with descriptive error message when a module imports itself', () => {
+      @Component({template: ''})
+      class FixtureComponent {
+      }
+
+      @NgModule({imports: [SomeModule], declarations: [FixtureComponent]})
+      class SomeModule {
+      }
+      expect(() => {
+        TestBed.configureTestingModule({imports: [SomeModule]}).createComponent(FixtureComponent);
+      }).toThrowError(`'SomeModule' module can't import itself`);
+    });
+
+    it('should throw with descriptive error message when a directive is passed to imports', () => {
+      @Component({template: ''})
+      class SomeComponent {
+      }
+
+      @NgModule({imports: [SomeComponent]})
+      class ModuleWithImportedComponent {
+      }
+      expect(() => {
+        TestBed.configureTestingModule({imports: [ModuleWithImportedComponent]})
+            .createComponent(SomeComponent);
+      })
+          .toThrowError(
+              `Unexpected directive 'SomeComponent' imported by the module 'ModuleWithImportedComponent'. Please add a @NgModule annotation.`);
+    });
+
+    it('should throw with descriptive error message when a pipe is passed to imports', () => {
+      @Component({template: ''})
+      class FixtureComponent {
+      }
+      @Pipe({name: 'somePipe'})
+      class SomePipe {
+      }
+      @NgModule({imports: [SomePipe], declarations: [FixtureComponent]})
+      class ModuleWithImportedPipe {
+      }
+      expect(() => {
+        TestBed.configureTestingModule({imports: [ModuleWithImportedPipe]})
+            .createComponent(FixtureComponent);
+      })
+          .toThrowError(
+              `Unexpected pipe 'SomePipe' imported by the module 'ModuleWithImportedPipe'. Please add a @NgModule annotation.`);
+    });
+
+    it('should throw with descriptive error message when a module is passed to declarations', () => {
+      @Component({template: ''})
+      class FixtureComponent {
+      }
+      @NgModule({})
+      class SomeModule {
+      }
+      @NgModule({declarations: [SomeModule, FixtureComponent]})
+      class ModuleWithDeclaredModule {
+      }
+
+      // The error is almost the same in Ivy and ViewEngine, however since Ivy's
+      // message is more correct it doesn't make sense to align it ViewEngine.
+      const expectedErrorMessage = ivyEnabled ?
+          `Unexpected value 'SomeModule' declared by the module 'ModuleWithDeclaredModule'. Please add a @Pipe/@Directive/@Component annotation.` :
+          `Unexpected module 'SomeModule' declared by the module 'ModuleWithDeclaredModule'. Please add a @Pipe/@Directive/@Component annotation.`;
+
+      expect(() => {
+        TestBed.configureTestingModule({imports: [ModuleWithDeclaredModule]})
+            .createComponent(FixtureComponent);
+      }).toThrowError(expectedErrorMessage);
+    });
+
+    it('should throw with descriptive error message when a declaration is missing annotation', () => {
+      @Component({template: ''})
+      class FixtureComponent {
+      }
+      class SomeClass {}
+      @NgModule({declarations: [SomeClass, FixtureComponent]})
+      class SomeModule {
+      }
+      expect(() => {
+        TestBed.configureTestingModule({imports: [SomeModule]}).createComponent(FixtureComponent);
+      })
+          .toThrowError(
+              `Unexpected value 'SomeClass' declared by the module 'SomeModule'. Please add a @Pipe/@Directive/@Component annotation.`);
+    });
+
+    it('should throw with descriptive error message when an imported module is missing annotation',
+       () => {
+         @Component({template: ''})
+         class FixtureComponent {
+         }
+         class SomeModule {}
+         @NgModule({imports: [SomeModule], declarations: [FixtureComponent]})
+         class ModuleWithImportedModule {
+         }
+         expect(() => {
+           TestBed.configureTestingModule({imports: [ModuleWithImportedModule]})
+               .createComponent(FixtureComponent);
+         })
+             .toThrowError(
+                 `Unexpected value 'SomeModule' imported by the module 'ModuleWithImportedModule'. Please add a @NgModule annotation.`);
+       });
+
+  });
+
   it('should only call inherited host listeners once', () => {
     let clicks = 0;