From c3f1cf9e62bd4f263c75eb44b829c08b3f8352bd Mon Sep 17 00:00:00 2001
From: crisbeto <crisbeto@abv.bg>
Date: Tue, 15 Oct 2019 22:29:42 +0200
Subject: [PATCH 1/2] fix(ivy): throw better error for missing generic type in
 ModuleWithProviders

Currently if a `ModuleWithProviders` is missng its generic type, we throw a cryptic error like:

```
error TS-991010: Value at position 3 in the NgModule.imports of TodosModule is not a reference: [object Object]
```

These changes add a better error to make it easier to debug.
---
 .../src/ngtsc/annotations/src/ng_module.ts    | 24 ++++++++++++-------
 .../src/ngtsc/diagnostics/src/code.ts         |  8 ++++++-
 .../test/ngtsc/fake_core/index.ts             |  3 ++-
 .../compiler-cli/test/ngtsc/ngtsc_spec.ts     | 24 +++++++++++++++++++
 4 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
index ea2c2b142a5b8..4a776fc6054b4 100644
--- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
+++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
@@ -89,7 +89,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
     }
 
     const moduleResolvers = combineResolvers([
-      ref => this._extractModuleFromModuleWithProvidersFn(ref.node),
+      ref => this._extractModuleFromModuleWithProvidersFn(ref.node, name),
       forwardRefResolver,
     ]);
 
@@ -353,12 +353,12 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
    * Given a `FunctionDeclaration`, `MethodDeclaration` or `FunctionExpression`, check if it is
    * typed as a `ModuleWithProviders` and return an expression referencing the module if available.
    */
-  private _extractModuleFromModuleWithProvidersFn(node: ts.FunctionDeclaration|
-                                                  ts.MethodDeclaration|
-                                                  ts.FunctionExpression): ts.Expression|null {
+  private _extractModuleFromModuleWithProvidersFn(
+      node: ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression,
+      className: string): ts.Expression|null {
     const type = node.type || null;
-    return type &&
-        (this._reflectModuleFromTypeParam(type) || this._reflectModuleFromLiteralType(type));
+    return type && (this._reflectModuleFromTypeParam(type, node, className) ||
+                    this._reflectModuleFromLiteralType(type));
   }
 
   /**
@@ -367,7 +367,9 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
    * @param type The type to reflect on.
    * @returns the identifier of the NgModule type if found, or null otherwise.
    */
-  private _reflectModuleFromTypeParam(type: ts.TypeNode): ts.Expression|null {
+  private _reflectModuleFromTypeParam(
+      type: ts.TypeNode, node: ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression,
+      className: string): ts.Expression|null {
     // Examine the type of the function to see if it's a ModuleWithProviders reference.
     if (!ts.isTypeReferenceNode(type)) {
       return null;
@@ -395,7 +397,13 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
 
     // If there's no type parameter specified, bail.
     if (type.typeArguments === undefined || type.typeArguments.length !== 1) {
-      return null;
+      const providerName = ts.isMethodDeclaration(node) && ts.isClassDeclaration(node.parent) ?
+          node.parent.name :
+          node.name;
+      throw new FatalDiagnosticError(
+          ErrorCode.NGMODULE_MODULE_WITH_PROVIDERS_MISSING_GENERIC, type,
+          `ModuleWithProviders annotation of ${providerName ? providerName.getText() : 'provider'} ` +
+              `is missing its generic type argument in NgModule of ${className}`);
     }
 
     const arg = type.typeArguments[0];
diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts
index 85be9dbf473d9..7534049f181fe 100644
--- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts
+++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts
@@ -58,6 +58,12 @@ export enum ErrorCode {
    */
   NGMODULE_INVALID_REEXPORT = 6004,
 
+  /**
+   * Raised when a `ModuleWithProviders` with a missing
+   * generic type argument is passed into an `NgModule`.
+   */
+  NGMODULE_MODULE_WITH_PROVIDERS_MISSING_GENERIC = 6005,
+
   /**
    * Raised when ngcc tries to inject a synthetic decorator over one that already exists.
    */
@@ -87,4 +93,4 @@ export enum ErrorCode {
 
 export function ngErrorCode(code: ErrorCode): number {
   return parseInt('-99' + code);
-}
\ No newline at end of file
+}
diff --git a/packages/compiler-cli/test/ngtsc/fake_core/index.ts b/packages/compiler-cli/test/ngtsc/fake_core/index.ts
index ddc353bf51600..496d8183587b9 100644
--- a/packages/compiler-cli/test/ngtsc/fake_core/index.ts
+++ b/packages/compiler-cli/test/ngtsc/fake_core/index.ts
@@ -44,7 +44,8 @@ export const Output = callablePropDecorator();
 export const ViewChild = callablePropDecorator();
 export const ViewChildren = callablePropDecorator();
 
-export type ModuleWithProviders<T> = any;
+// T defaults to `any` to reflect what is currently in core.
+export type ModuleWithProviders<T = any> = any;
 
 export class ChangeDetectorRef {}
 export class ElementRef {}
diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
index 321b44d78f4be..d5b7a51172219 100644
--- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
+++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
@@ -1469,6 +1469,30 @@ runInEachFileSystem(os => {
                 'i0.ɵɵNgModuleDefWithMeta<TestModule, never, [typeof i1.RouterModule], never>');
       });
 
+      it('should throw if ModuleWithProviders is missing its generic type argument', () => {
+        env.write(`test.ts`, `
+          import {NgModule} from '@angular/core';
+          import {RouterModule} from 'router';
+
+          @NgModule({imports: [RouterModule.forRoot()]})
+          export class TestModule {}
+        `);
+
+        env.write('node_modules/router/index.d.ts', `
+          import {ModuleWithProviders, ɵɵNgModuleDefWithMeta} from '@angular/core';
+
+          declare class RouterModule {
+            static forRoot(): ModuleWithProviders;
+            static ɵmod: ɵɵNgModuleDefWithMeta<RouterModule, never, never, never>;
+          }
+        `);
+
+        const errors = env.driveDiagnostics();
+        expect(trim(errors[0].messageText as string))
+            .toContain(
+                `ModuleWithProviders annotation of RouterModule is missing its generic type argument in NgModule of TestModule`);
+      });
+
       it('should extract the generic type if it is provided as qualified type name', () => {
         env.write(`test.ts`, `
         import {NgModule} from '@angular/core';

From 9e218540d4257a330ba8d8606a0add0f37e4be57 Mon Sep 17 00:00:00 2001
From: crisbeto <crisbeto@abv.bg>
Date: Fri, 18 Oct 2019 19:25:07 +0200
Subject: [PATCH 2/2] fixup! fix(ivy): throw better error for missing generic
 type in ModuleWithProviders

---
 .../src/ngtsc/annotations/src/ng_module.ts    | 28 ++++++++++---------
 .../compiler-cli/test/ngtsc/ngtsc_spec.ts     |  5 ++--
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
index 4a776fc6054b4..7a814e7bec225 100644
--- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
+++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
@@ -89,7 +89,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
     }
 
     const moduleResolvers = combineResolvers([
-      ref => this._extractModuleFromModuleWithProvidersFn(ref.node, name),
+      ref => this._extractModuleFromModuleWithProvidersFn(ref.node),
       forwardRefResolver,
     ]);
 
@@ -353,12 +353,12 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
    * Given a `FunctionDeclaration`, `MethodDeclaration` or `FunctionExpression`, check if it is
    * typed as a `ModuleWithProviders` and return an expression referencing the module if available.
    */
-  private _extractModuleFromModuleWithProvidersFn(
-      node: ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression,
-      className: string): ts.Expression|null {
+  private _extractModuleFromModuleWithProvidersFn(node: ts.FunctionDeclaration|
+                                                  ts.MethodDeclaration|
+                                                  ts.FunctionExpression): ts.Expression|null {
     const type = node.type || null;
-    return type && (this._reflectModuleFromTypeParam(type, node, className) ||
-                    this._reflectModuleFromLiteralType(type));
+    return type &&
+        (this._reflectModuleFromTypeParam(type, node) || this._reflectModuleFromLiteralType(type));
   }
 
   /**
@@ -368,8 +368,8 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
    * @returns the identifier of the NgModule type if found, or null otherwise.
    */
   private _reflectModuleFromTypeParam(
-      type: ts.TypeNode, node: ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression,
-      className: string): ts.Expression|null {
+      type: ts.TypeNode,
+      node: ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression): ts.Expression|null {
     // Examine the type of the function to see if it's a ModuleWithProviders reference.
     if (!ts.isTypeReferenceNode(type)) {
       return null;
@@ -397,13 +397,15 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
 
     // If there's no type parameter specified, bail.
     if (type.typeArguments === undefined || type.typeArguments.length !== 1) {
-      const providerName = ts.isMethodDeclaration(node) && ts.isClassDeclaration(node.parent) ?
-          node.parent.name :
-          node.name;
+      const parent =
+          ts.isMethodDeclaration(node) && ts.isClassDeclaration(node.parent) ? node.parent : null;
+      const symbolName = (parent && parent.name ? parent.name.getText() + '.' : '') +
+          (node.name ? node.name.getText() : 'anonymous');
       throw new FatalDiagnosticError(
           ErrorCode.NGMODULE_MODULE_WITH_PROVIDERS_MISSING_GENERIC, type,
-          `ModuleWithProviders annotation of ${providerName ? providerName.getText() : 'provider'} ` +
-              `is missing its generic type argument in NgModule of ${className}`);
+          `${symbolName} returns a ModuleWithProviders type without a generic type argument. ` +
+              `Please add a generic type argument to the ModuleWithProviders type. If this ` +
+              `occurrence is in library code you don't control, please contact the library authors.`);
     }
 
     const arg = type.typeArguments[0];
diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
index d5b7a51172219..7e349632d7058 100644
--- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
+++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
@@ -1486,11 +1486,12 @@ runInEachFileSystem(os => {
             static ɵmod: ɵɵNgModuleDefWithMeta<RouterModule, never, never, never>;
           }
         `);
-
         const errors = env.driveDiagnostics();
         expect(trim(errors[0].messageText as string))
             .toContain(
-                `ModuleWithProviders annotation of RouterModule is missing its generic type argument in NgModule of TestModule`);
+                `RouterModule.forRoot returns a ModuleWithProviders type without a generic type argument. ` +
+                `Please add a generic type argument to the ModuleWithProviders type. If this ` +
+                `occurrence is in library code you don't control, please contact the library authors.`);
       });
 
       it('should extract the generic type if it is provided as qualified type name', () => {