feat(core): undecorated-classes-with-decorated-fields migration should handle classes with lifecycle hooks (#36921)

As of v10, undecorated classes using Angular features are no longer
supported. In v10, we plan on removing the undecorated classes
compatibility code in ngtsc. This means that old patterns for
undecorated classes will result in compilation errors.

We had a migration for this in v9 already, but it looks like
the migration does not handle cases where classes uses lifecycle
hooks. This is handled in the ngtsc compatibility code, and we
should handle it similarly in migrations too.

This has not been outlined in the migration plan initially,
but an appendix has been added for v10 to the plan document.

https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA?both.

Note: The migration is unable to determine whether a given undecorated
class that only defines `ngOnDestroy` is a directive or an actual
service. This means that in some cases the migration cannot do
more than adding a TODO and printing an failure.

Certainly there are more ways to determine the type of such classes,
but it would involve metadata and NgModule analysis. This is out of
scope for this migration.

PR Close #36921

packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/README.md

@@ -1,7 +1,11 @@
 ## Undecorated classes with decorated fields migration
 
-Automatically adds a `Directive` decorator to undecorated classes that have fields with Angular
-decorators. Also adds the relevant imports, if necessary.
+Automatically adds a `Directive` decorator to undecorated classes that use Angular features. A
+class is considered using Angular features if a class member is decorated (e.g. `@Input()`), or
+if the class defines any lifecycle hooks.
+
+This matches the undecorated classes compatibility logic in ngtsc that will be removed
+as part of v10 so that the new mental model is enforced.    
 
 #### Before
 ```ts

packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/google3/tslint_update_recorder.ts

@@ -16,6 +16,12 @@ export class TslintUpdateRecorder implements UpdateRecorder {
 
   constructor(private ruleName: string, private sourceFile: ts.SourceFile) {}
 
+  addClassTodo(node: ts.ClassDeclaration, message: string) {
+    this.failures.push(new RuleFailure(
+        this.sourceFile, node.getStart(), 0, message, this.ruleName,
+        Replacement.appendText(node.getStart(), `// TODO: ${message}`)));
+  }
+
   /** Adds the specified decorator to the given class declaration. */
   addClassDecorator(node: ts.ClassDeclaration, decoratorText: string) {
     // Adding a decorator should be the last replacement. Replacements/rule failures

packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/index.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Rule, SchematicsException, Tree,} from '@angular-devkit/schematics';
+import {Rule, SchematicContext, SchematicsException, Tree,} from '@angular-devkit/schematics';
 import {relative} from 'path';
 import * as ts from 'typescript';
 
@@ -21,10 +21,11 @@ import {UpdateRecorder} from './update_recorder';
  * https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA
  */
 export default function(): Rule {
-  return (tree: Tree) => {
+  return (tree: Tree, ctx: SchematicContext) => {
     const {buildPaths, testPaths} = getProjectTsConfigPaths(tree);
     const basePath = process.cwd();
     const allPaths = [...buildPaths, ...testPaths];
+    const failures: string[] = [];
 
     if (!allPaths.length) {
       throw new SchematicsException(
@@ -32,12 +33,20 @@ export default function(): Rule {
     }
 
     for (const tsconfigPath of allPaths) {
-      runUndecoratedClassesMigration(tree, tsconfigPath, basePath);
+      failures.push(...runUndecoratedClassesMigration(tree, tsconfigPath, basePath));
+    }
+
+    if (failures.length) {
+      ctx.logger.info('Could not migrate all undecorated classes that use Angular features.');
+      ctx.logger.info('Please manually fix the following failures:');
+      failures.forEach(message => ctx.logger.warn(`⮑   ${message}`));
     }
   };
 }
 
-function runUndecoratedClassesMigration(tree: Tree, tsconfigPath: string, basePath: string) {
+function runUndecoratedClassesMigration(
+    tree: Tree, tsconfigPath: string, basePath: string): string[] {
+  const failures: string[] = [];
   const {program} = createMigrationProgram(tree, tsconfigPath, basePath);
   const typeChecker = program.getTypeChecker();
   const sourceFiles = program.getSourceFiles().filter(
@@ -47,7 +56,13 @@ function runUndecoratedClassesMigration(tree: Tree, tsconfigPath: string, basePa
       new UndecoratedClassesWithDecoratedFieldsTransform(typeChecker, getUpdateRecorder);
 
   // Migrate all source files in the project.
-  transform.migrate(sourceFiles);
+  transform.migrate(sourceFiles).forEach(({node, message}) => {
+    const nodeSourceFile = node.getSourceFile();
+    const relativeFilePath = relative(basePath, nodeSourceFile.fileName);
+    const {line, character} =
+        ts.getLineAndCharacterOfPosition(node.getSourceFile(), node.getStart());
+    failures.push(`${relativeFilePath}@${line + 1}:${character + 1}: ${message}`);
+  });
 
   // Record the changes collected in the import manager.
   transform.recordChanges();
@@ -57,6 +72,8 @@ function runUndecoratedClassesMigration(tree: Tree, tsconfigPath: string, basePa
   // file in order to avoid shifted character offsets.
   updateRecorders.forEach(recorder => recorder.commitUpdate());
 
+  return failures;
+
   /** Gets the update recorder for the specified source file. */
   function getUpdateRecorder(sourceFile: ts.SourceFile): UpdateRecorder {
     if (updateRecorders.has(sourceFile)) {
@@ -64,6 +81,9 @@ function runUndecoratedClassesMigration(tree: Tree, tsconfigPath: string, basePa
     }
     const treeRecorder = tree.beginUpdate(relative(basePath, sourceFile.fileName));
     const recorder: UpdateRecorder = {
+      addClassTodo(node: ts.ClassDeclaration, message: string) {
+        treeRecorder.insertRight(node.getStart(), `// TODO: ${message}\n`);
+      },
       addClassDecorator(node: ts.ClassDeclaration, text: string) {
         // New imports should be inserted at the left while decorators should be inserted
         // at the right in order to ensure that imports are inserted before the decorator

packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/transform.ts

@@ -14,9 +14,40 @@ import {ImportManager} from '../../utils/import_manager';
 import {getAngularDecorators, NgDecorator} from '../../utils/ng_decorators';
 import {findBaseClassDeclarations} from '../../utils/typescript/find_base_classes';
 import {unwrapExpression} from '../../utils/typescript/functions';
+import {getPropertyNameText} from '../../utils/typescript/property_name';
 
 import {UpdateRecorder} from './update_recorder';
 
+/**
+ * Set of known decorators that indicate that the current class needs a directive
+ * definition. These decorators are always specific to directives.
+ */
+const DIRECTIVE_FIELD_DECORATORS = new Set([
+  'Input', 'Output', 'ViewChild', 'ViewChildren', 'ContentChild', 'ContentChildren', 'HostBinding',
+  'HostListener'
+]);
+
+/**
+ * Set of known lifecycle hooks that indicate that the current class needs a directive
+ * definition. These lifecycle hooks are always specific to directives.
+ */
+const DIRECTIVE_LIFECYCLE_HOOKS = new Set([
+  'ngOnChanges', 'ngOnInit', 'ngDoCheck', 'ngAfterViewInit', 'ngAfterViewChecked',
+  'ngAfterContentInit', 'ngAfterContentChecked'
+]);
+
+/**
+ * Set of known lifecycle hooks that indicate that a given class uses Angular
+ * features, but it's ambiguous whether it is a directive or service.
+ */
+const AMBIGUOUS_LIFECYCLE_HOOKS = new Set(['ngOnDestroy']);
+
+/** Describes how a given class is used in the context of Angular. */
+enum ClassKind {
+  DIRECTIVE,
+  AMBIGUOUS,
+  UNKNOWN,
+}
 
 /** Analyzed class declaration. */
 interface AnalyzedClass {
@@ -24,8 +55,13 @@ interface AnalyzedClass {
   isDirectiveOrComponent: boolean;
   /** Whether the class is an abstract directive. */
   isAbstractDirective: boolean;
-  /** Whether the class uses any Angular features. */
-  usesAngularFeatures: boolean;
+  /** Kind of the given class in terms of Angular. */
+  kind: ClassKind;
+}
+
+interface AnalysisFailure {
+  node: ts.Node;
+  message: string;
 }
 
 export class UndecoratedClassesWithDecoratedFieldsTransform {
@@ -40,11 +76,15 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
 
   /**
    * Migrates the specified source files. The transform adds the abstract `@Directive`
-   * decorator to classes that have Angular field decorators but are not decorated.
-   * https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA
+   * decorator to undecorated classes that use Angular features. Class members which
+   * are decorated with any Angular decorator, or class members for lifecycle hooks are
+   * indicating that a given class uses Angular features. https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA
    */
-  migrate(sourceFiles: ts.SourceFile[]) {
-    this._findUndecoratedAbstractDirectives(sourceFiles).forEach(node => {
+  migrate(sourceFiles: ts.SourceFile[]): AnalysisFailure[] {
+    const {result, ambiguous} = this._findUndecoratedAbstractDirectives(sourceFiles);
+
+
+    result.forEach(node => {
       const sourceFile = node.getSourceFile();
       const recorder = this.getUpdateRecorder(sourceFile);
       const directiveExpr =
@@ -53,6 +93,25 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
       recorder.addClassDecorator(
           node, this.printer.printNode(ts.EmitHint.Unspecified, decoratorExpr, sourceFile));
     });
+
+    // Ambiguous classes clearly use Angular features, but the migration is unable to
+    // determine whether the class is used as directive, service or pipe. The migration
+    // could potentially determine the type by checking NgModule definitions or inheritance
+    // of other known declarations, but this is out of scope and a TODO/failure is sufficient.
+    return Array.from(ambiguous).reduce((failures, node) => {
+      const sourceFile = node.getSourceFile();
+      const recorder = this.getUpdateRecorder(sourceFile);
+
+      // Add a TODO to the class that uses Angular features but is not decorated.
+      recorder.addClassTodo(node, `Add Angular decorator.`);
+
+      // Add an error for the class that will be printed in the `ng update` output.
+      return failures.concat({
+        node,
+        message: 'Class uses Angular features but cannot be migrated automatically. Please ' +
+            'add an appropriate Angular decorator.'
+      });
+    }, [] as AnalysisFailure[]);
   }
 
   /** Records all changes that were made in the import manager. */
@@ -60,19 +119,24 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
     this.importManager.recordChanges();
   }
 
-  /** Finds undecorated abstract directives in the specified source files. */
+  /**
+   * Finds undecorated abstract directives in the specified source files. Also returns
+   * a set of undecorated classes which could not be detected as guaranteed abstract
+   * directives. Those are ambiguous and could be either Directive, Pipe or service.
+   */
   private _findUndecoratedAbstractDirectives(sourceFiles: ts.SourceFile[]) {
     const result = new Set<ts.ClassDeclaration>();
     const undecoratedClasses = new Set<ts.ClassDeclaration>();
     const nonAbstractDirectives = new WeakSet<ts.ClassDeclaration>();
     const abstractDirectives = new WeakSet<ts.ClassDeclaration>();
+    const ambiguous = new Set<ts.ClassDeclaration>();
 
     const visitNode = (node: ts.Node) => {
       node.forEachChild(visitNode);
       if (!ts.isClassDeclaration(node)) {
         return;
       }
-      const {isDirectiveOrComponent, isAbstractDirective, usesAngularFeatures} =
+      const {isDirectiveOrComponent, isAbstractDirective, kind} =
           this._analyzeClassDeclaration(node);
       if (isDirectiveOrComponent) {
         if (isAbstractDirective) {
@@ -80,10 +144,13 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
         } else {
           nonAbstractDirectives.add(node);
         }
-      } else if (usesAngularFeatures) {
+      } else if (kind === ClassKind.DIRECTIVE) {
         abstractDirectives.add(node);
         result.add(node);
       } else {
+        if (kind === ClassKind.AMBIGUOUS) {
+          ambiguous.add(node);
+        }
         undecoratedClasses.add(node);
       }
     };
@@ -96,18 +163,21 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
       for (const {node: baseClass} of findBaseClassDeclarations(node, this.typeChecker)) {
         // If the undecorated class inherits from a non-abstract directive, skip the current
         // class. We do this because undecorated classes which inherit metadata from non-abstract
-        // directives are handle in the `undecorated-classes-with-di` migration that copies
+        // directives are handled in the `undecorated-classes-with-di` migration that copies
         // inherited metadata into an explicit decorator.
         if (nonAbstractDirectives.has(baseClass)) {
           break;
         } else if (abstractDirectives.has(baseClass)) {
           result.add(node);
+          // In case the undecorated class previously could not be detected as directive,
+          // remove it from the ambiguous set as we now know that it's a guaranteed directive.
+          ambiguous.delete(node);
           break;
         }
       }
     });
 
-    return result;
+    return {result, ambiguous};
   }
 
   /**
@@ -116,9 +186,9 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
    */
   private _analyzeClassDeclaration(node: ts.ClassDeclaration): AnalyzedClass {
     const ngDecorators = node.decorators && getAngularDecorators(this.typeChecker, node.decorators);
-    const usesAngularFeatures = this._hasAngularDecoratedClassMember(node);
+    const kind = this._determineClassKind(node);
     if (ngDecorators === undefined || ngDecorators.length === 0) {
-      return {isDirectiveOrComponent: false, isAbstractDirective: false, usesAngularFeatures};
+      return {isDirectiveOrComponent: false, isAbstractDirective: false, kind};
     }
     const directiveDecorator = ngDecorators.find(({name}) => name === 'Directive');
     const componentDecorator = ngDecorators.find(({name}) => name === 'Component');
@@ -127,7 +197,7 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
     return {
       isDirectiveOrComponent: !!directiveDecorator || !!componentDecorator,
       isAbstractDirective,
-      usesAngularFeatures,
+      kind,
     };
   }
 
@@ -152,8 +222,41 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
     return selector == null;
   }
 
-  private _hasAngularDecoratedClassMember(node: ts.ClassDeclaration): boolean {
-    return node.members.some(
-        m => m.decorators && getAngularDecorators(this.typeChecker, m.decorators).length !== 0);
+  /**
+   * Determines the kind of a given class in terms of Angular. The method checks
+   * whether the given class has members that indicate the use of Angular features.
+   * e.g. lifecycle hooks or decorated members like `@Input` or `@Output` are
+   * considered Angular features..
+   */
+  private _determineClassKind(node: ts.ClassDeclaration): ClassKind {
+    let usage = ClassKind.UNKNOWN;
+
+    for (const member of node.members) {
+      const propertyName = member.name !== undefined ? getPropertyNameText(member.name) : null;
+
+      // If the class declares any of the known directive lifecycle hooks, we can
+      // immediately exit the loop as the class is guaranteed to be a directive.
+      if (propertyName !== null && DIRECTIVE_LIFECYCLE_HOOKS.has(propertyName)) {
+        return ClassKind.DIRECTIVE;
+      }
+
+      const ngDecorators = member.decorators !== undefined ?
+          getAngularDecorators(this.typeChecker, member.decorators) :
+          [];
+      for (const {name} of ngDecorators) {
+        if (DIRECTIVE_FIELD_DECORATORS.has(name)) {
+          return ClassKind.DIRECTIVE;
+        }
+      }
+
+      // If the class declares any of the lifecycle hooks that do not guarantee that
+      // the given class is a directive, update the kind and continue looking for other
+      // members that would unveil a more specific kind (i.e. being a directive).
+      if (propertyName !== null && AMBIGUOUS_LIFECYCLE_HOOKS.has(propertyName)) {
+        usage = ClassKind.AMBIGUOUS;
+      }
+    }
+
+    return usage;
   }
 }

packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/update_recorder.ts

@@ -15,5 +15,6 @@ import {ImportManagerUpdateRecorder} from '../../utils/import_manager';
  */
 export interface UpdateRecorder extends ImportManagerUpdateRecorder {
   addClassDecorator(node: ts.ClassDeclaration, text: string): void;
+  addClassTodo(node: ts.ClassDeclaration, message: string): void;
   commitUpdate(): void;
 }

packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts

@@ -18,6 +18,7 @@ describe('Undecorated classes with decorated fields migration', () => {
   let tree: UnitTestTree;
   let tmpDirPath: string;
   let previousWorkingDir: string;
+  let warnings: string[];
 
   beforeEach(() => {
     runner = new SchematicTestRunner('test', require.resolve('../migrations.json'));
@@ -29,6 +30,13 @@ describe('Undecorated classes with decorated fields migration', () => {
       projects: {t: {architect: {build: {options: {tsConfig: './tsconfig.json'}}}}}
     }));
 
+    warnings = [];
+    runner.logger.subscribe(entry => {
+      if (entry.level === 'warn') {
+        warnings.push(entry.message);
+      }
+    });
+
     previousWorkingDir = shx.pwd();
     tmpDirPath = getSystemPath(host.root);
 
@@ -228,6 +236,45 @@ describe('Undecorated classes with decorated fields migration', () => {
     expect(tree.readContent('/index.ts')).toContain(`@Directive()\nexport class Base {`);
   });
 
+  it('should migrate undecorated class that uses "ngOnChanges" lifecycle hook',
+     () => assertLifecycleHookMigrated('ngOnChanges'));
+  it('should migrate undecorated class that uses "ngOnInit" lifecycle hook',
+     () => assertLifecycleHookMigrated('ngOnInit'));
+  it('should migrate undecorated class that uses "ngDoCheck" lifecycle hook',
+     () => assertLifecycleHookMigrated('ngDoCheck'));
+  it('should migrate undecorated class that uses "ngAfterViewInit" lifecycle hook',
+     () => assertLifecycleHookMigrated('ngAfterViewInit'));
+  it('should migrate undecorated class that uses "ngAfterViewChecked" lifecycle hook',
+     () => assertLifecycleHookMigrated('ngAfterViewChecked'));
+  it('should migrate undecorated class that uses "ngAfterContentInit" lifecycle hook',
+     () => assertLifecycleHookMigrated('ngAfterContentInit'));
+  it('should migrate undecorated class that uses "ngAfterContentChecked" lifecycle hook',
+     () => assertLifecycleHookMigrated('ngAfterContentChecked'));
+
+  it(`should report an error and add a TODO for undecorated classes that only define ` +
+         `the "ngOnDestroy" lifecycle hook`,
+     async () => {
+       writeFile('/index.ts', `
+      import { Input } from '@angular/core';
+
+      export class SomeClassWithAngularFeatures {
+        ngOnDestroy() {
+          // noop for testing
+        }
+      }
+    `);
+
+       await runMigration();
+
+       expect(warnings.length).toBe(1);
+       expect(warnings[0])
+           .toMatch(
+               'index.ts@4:7: Class uses Angular features but cannot be migrated automatically. ' +
+               'Please add an appropriate Angular decorator.');
+       expect(tree.readContent('/index.ts'))
+           .toMatch(/TODO: Add Angular decorator\.\nexport class SomeClassWithAngularFeatures {/);
+     });
+
   it('should add @Directive to undecorated derived classes of a migrated class', async () => {
     writeFile('/index.ts', `
       import { Input, Directive, NgModule } from '@angular/core';
@@ -314,6 +361,22 @@ describe('Undecorated classes with decorated fields migration', () => {
     expect(error).toBe(null);
   });
 
+  async function assertLifecycleHookMigrated(lifecycleHookName: string) {
+    writeFile('/index.ts', `
+      import { Input } from '@angular/core';
+
+      export class SomeClassWithAngularFeatures {
+        ${lifecycleHookName}() {
+          // noop for testing
+        }
+      }
+    `);
+
+    await runMigration();
+    expect(tree.readContent('/index.ts'))
+        .toContain(`@Directive()\nexport class SomeClassWithAngularFeatures {`);
+  }
+
   function writeFile(filePath: string, contents: string) {
     host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents));
   }