refactor(core): various fixes for missing-injectable migration

integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts

@@ -8,7 +8,7 @@ export class ComponentProvider2 {}
 @Component({
   template: '',
   viewProviders: [ComponentTypeProvider, [
-    {provide: ComponentDontNeedCase, useExisting: ComponentProvider}]
+    {provide: ComponentDontNeedCase, useClass: ComponentProvider}]
   ],
   providers: [ComponentProvider2]
 })
@@ -21,7 +21,7 @@ export class DirectiveProvider {}
 @Directive({
   selector: 'test-dir',
   providers: [DirectiveTypeProvider, [
-    {provide: DirectiveDontNeedCase, useExisting: DirectiveProvider}]
+    {provide: DirectiveDontNeedCase, useClass: DirectiveProvider}]
   ],
 })
 export class ProvidersTestDirective {}

integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts

@@ -11,7 +11,7 @@ export class ComponentProvider2 {}
 @Component({
   template: '',
   viewProviders: [ComponentTypeProvider, [
-    {provide: ComponentDontNeedCase, useExisting: ComponentProvider}]
+    {provide: ComponentDontNeedCase, useClass: ComponentProvider}]
   ],
   providers: [ComponentProvider2]
 })
@@ -26,7 +26,7 @@ export class DirectiveProvider {}
 @Directive({
   selector: 'test-dir',
   providers: [DirectiveTypeProvider, [
-    {provide: DirectiveDontNeedCase, useExisting: DirectiveProvider}]
+    {provide: DirectiveDontNeedCase, useClass: DirectiveProvider}]
   ],
 })
 export class ProvidersTestDirective {}

packages/core/schematics/migrations/missing-injectable/BUILD.bazel

@@ -11,6 +11,7 @@ ts_library(
         "//packages/core/schematics/test:__pkg__",
     ],
     deps = [
+        "//packages/compiler-cli/src/ngtsc/annotations",
         "//packages/compiler-cli/src/ngtsc/imports",
         "//packages/compiler-cli/src/ngtsc/partial_evaluator",
         "//packages/compiler-cli/src/ngtsc/reflection",

packages/core/schematics/migrations/missing-injectable/transform.ts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {forwardRefResolver} from '@angular/compiler-cli/src/ngtsc/annotations/src/util';
 import {Reference} from '@angular/compiler-cli/src/ngtsc/imports';
 import {DynamicValue, PartialEvaluator, ResolvedValue} from '@angular/compiler-cli/src/ngtsc/partial_evaluator';
 import {TypeScriptReflectionHost} from '@angular/compiler-cli/src/ngtsc/reflection';
@@ -67,7 +68,7 @@ export class MissingInjectableTransform {
       return [];
     }
 
-    const evaluatedExpr = this.partialEvaluator.evaluate(module.providersExpr);
+    const evaluatedExpr = this._evaluateExpression(module.providersExpr);
 
     if (!Array.isArray(evaluatedExpr)) {
       return [{
@@ -89,7 +90,7 @@ export class MissingInjectableTransform {
 
     // Migrate "providers" on directives and components if defined.
     if (directive.providersExpr) {
-      const evaluatedExpr = this.partialEvaluator.evaluate(directive.providersExpr);
+      const evaluatedExpr = this._evaluateExpression(directive.providersExpr);
       if (!Array.isArray(evaluatedExpr)) {
         return [
           {node: directive.providersExpr, message: `Providers are not statically analyzable.`}
@@ -100,7 +101,7 @@ export class MissingInjectableTransform {
 
     // Migrate "viewProviders" on components if defined.
     if (directive.viewProvidersExpr) {
-      const evaluatedExpr = this.partialEvaluator.evaluate(directive.viewProvidersExpr);
+      const evaluatedExpr = this._evaluateExpression(directive.viewProvidersExpr);
       if (!Array.isArray(evaluatedExpr)) {
         return [
           {node: directive.viewProvidersExpr, message: `Providers are not statically analyzable.`}
@@ -122,6 +123,14 @@ export class MissingInjectableTransform {
     this.visitedProviderClasses.add(node);
 
     const sourceFile = node.getSourceFile();
+
+    // We cannot migrate provider classes outside of source files. This is because the
+    // migration for third-party library files should happen in "ngcc", and in general
+    // would also involve metadata parsing.
+    if (sourceFile.isDeclarationFile) {
+      return;
+    }
+
     const ngDecorators =
         node.decorators ? getAngularDecorators(this.typeChecker, node.decorators) : null;
 
@@ -150,6 +159,14 @@ export class MissingInjectableTransform {
     }
   }
 
+  /**
+   * Evaluates the given TypeScript expression using the partial evaluator with
+   * the foreign function resolver for handling "forwardRef" calls.
+   */
+  private _evaluateExpression(expr: ts.Expression): ResolvedValue {
+    return this.partialEvaluator.evaluate(expr, forwardRefResolver);
+  }
+
   /**
    * Visits the given resolved value of a provider. Providers can be nested in
    * arrays and we need to recursively walk through the providers to be able to
@@ -160,12 +177,11 @@ export class MissingInjectableTransform {
     if (value instanceof Reference && ts.isClassDeclaration(value.node)) {
       this.migrateProviderClass(value.node, module);
     } else if (value instanceof Map) {
-      if (!value.has('provide') || value.has('useValue') || value.has('useFactory')) {
+      if (!value.has('provide') || value.has('useValue') || value.has('useFactory') ||
+          value.has('useExisting')) {
         return [];
       }
-      if (value.has('useExisting')) {
-        return this._visitProviderResolvedValue(value.get('useExisting') !, module);
-      } else if (value.has('useClass')) {
+      if (value.has('useClass')) {
         return this._visitProviderResolvedValue(value.get('useClass') !, module);
       } else {
         return this._visitProviderResolvedValue(value.get('provide') !, module);

packages/core/schematics/test/google3/missing_injectable_rule_spec.ts

@@ -151,7 +151,7 @@ describe('Google3 missing injectable tslint rule', () => {
       expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceB/);
       expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceC/);
       expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceD/);
-      expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceE/);
+      expect(getFile('/index.ts')).toMatch(/MyServiceD {}\s+export class MyServiceE/);
       expect(getFile('/index.ts')).toMatch(/MyServiceE {}\s+export class MyServiceF/);
       expect(getFile('/index.ts')).toMatch(/MyServiceF {}\s+export class MyServiceG/);
     });

packages/core/schematics/test/missing_injectable_migration_spec.ts

@@ -48,6 +48,10 @@ describe('Missing injectable migration', () => {
     // Switch into the temporary directory path. This allows us to run
     // the schematic against our custom unit test tree.
     shx.cd(tmpDirPath);
+
+    writeFile('/node_modules/@angular/core/index.d.ts', `
+      export declare function forwardRef(fn: Function);
+    `);
   });
 
   afterEach(() => {
@@ -135,6 +139,24 @@ describe('Missing injectable migration', () => {
           .toContain(`{ ${type}, Injectable } from '@angular/core`);
     });
 
+    it(`should migrate object literal provider with forwardRef in ${type}`, async() => {
+      writeFile('/index.ts', `
+        import {${type}, forwardRef} from '@angular/core';
+
+        @${type}({${propName}: [{provide: forwardRef(() => MyService)}]})
+        export class TestClass {}
+
+        export class MyService {}
+      `);
+
+      await runMigration();
+
+      expect(warnOutput.length).toBe(0);
+      expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/);
+      expect(tree.readContent('/index.ts'))
+          .toContain(`{ ${type}, forwardRef, Injectable } from '@angular/core`);
+    });
+
     it(`should not migrate object literal provider with "useValue" in ${type}`, async() => {
       writeFile('/index.ts', `
         import {${type}} from '@angular/core';
@@ -167,24 +189,24 @@ describe('Missing injectable migration', () => {
       expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
     });
 
-    it(`should migrate object literal provider with "useExisting" in ${type}`, async() => {
+    it(`should not migrate object literal provider with "useExisting" in ${type}`, async() => {
       writeFile('/index.ts', `
         import {${type}} from '@angular/core';
 
         export class MyService {}
         export class MyToken {}
 
-        @${type}({${propName}: [{provide: MyToken, useExisting: MyService}]})
+        @${type}({${propName}: [
+          {provide: MyService: useValue: null},
+          {provide: MyToken, useExisting: MyService},
+        ]})
         export class TestClass {}
       `);
 
       await runMigration();
 
       expect(warnOutput.length).toBe(0);
-      expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/);
-      expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/);
-      expect(tree.readContent('/index.ts'))
-          .toContain(`{ ${type}, Injectable } from '@angular/core`);
+      expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
     });
 
     it(`should migrate object literal provider with "useClass" in ${type}`, async() => {
@@ -207,6 +229,41 @@ describe('Missing injectable migration', () => {
           .toContain(`{ ${type}, Injectable } from '@angular/core`);
     });
 
+    it(`should not migrate references for providers with "useExisting" in ${type}, but migrate ` +
+           `existing token if declared in other ${type}`,
+       async() => {
+         writeFile('/index.ts', `
+        import {${type}} from '@angular/core';
+
+        export class MyService {}
+        export class MyToken {}
+
+        @${type}({
+          ${propName}: [
+            {provide: MyToken, useExisting: MyService},
+          ],
+        })
+        export class TestClass {}
+      `);
+
+         writeFile('/other.ts', `
+        import {${type} from '@angular/core';
+        import {MyService} from './index';
+
+        export @${type}({
+          ${propName}: [{provide: MyService}],
+        })
+        export class OtherClass {}
+      `);
+
+         await runMigration();
+
+         expect(warnOutput.length).toBe(0);
+         expect(tree.readContent('/index.ts'))
+             .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class MyService/);
+         expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/);
+       });
+
     it('should not migrate provider which is already decorated with @Injectable', async() => {
       writeFile('/index.ts', `
         import {Injectable, ${type}} from '@angular/core';
@@ -323,7 +380,6 @@ describe('Missing injectable migration', () => {
           .toContain(`{ ${type}, Injectable } from '@angular/core`);
     });
 
-
     it(`should migrate multiple nested providers in same ${type}`, async() => {
       writeFile('/index.ts', `
         import {${type}} from '@angular/core';
@@ -632,5 +688,24 @@ describe('Missing injectable migration', () => {
       expect(tree.readContent('/service.ts'))
           .toMatch(/import { Inject, Injectable } from '@angular\/core';/);
     });
+
+    it('should not migrate provider classes in library type definitions', async() => {
+      writeFile('/node_modules/my-lib/index.d.ts', `
+        export declare class MyService {}
+      `);
+
+      writeFile('/index.ts', `
+        import {MyService} from 'my-lib';
+        import {Pipe, ${type}} from '@angular/core';
+
+        @${type}({${propName}: [MyService]})
+        export class TestClass {}
+      `);
+
+      await runMigration();
+
+      expect(warnOutput.length).toBe(0);
+      expect(tree.readContent('/node_modules/my-lib/index.d.ts')).not.toContain('@Injectable');
+    });
   }
 });