feat(core): undecorated-classes migration should handle derived abstract classes (#35339)

In version 10, undecorated base classes that use Angular features need
to be decorated explicitly with `@Directive()`. Additionally, derived
classes of abstract directives need to be decorated.

The migration already handles this for undecorated classes that are
not explicitly decorated, but since in V9, abstract directives can be
used, we also need to handle this for explicitly decorated abstract
directives. e.g.

```
@Directive()
export class Base {...}

// needs to be decorated by migration when updating from v9 to v10
export class Wrapped extends Base {}

@Component(...)
export class Cmp extends Wrapped {}
```

PR Close #35339

integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields.ts

@@ -1,4 +1,12 @@
-import {Component, ElementRef, HostBinding, HostListener, Input, NgModule} from '@angular/core';
+import {
+  Component,
+  Directive,
+  ElementRef,
+  HostBinding,
+  HostListener,
+  Input,
+  NgModule
+} from '@angular/core';
 
 export class NonAngularBaseClass {
   greet() {}
@@ -45,3 +53,10 @@ export class WrappedMyComp extends MyComp {}
 
 @NgModule({declarations: [MyComp, WrappedMyComp]})
 export class TestModule {}
+
+@Directive({selector: null})
+export class AbstractDir {}
+
+export class DerivedAbstractDir extends AbstractDir {}
+
+export class WrappedDerivedAbstractDir extends DerivedAbstractDir {}

integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields_expected.ts

@@ -1,4 +1,12 @@
-import { Component, ElementRef, HostBinding, HostListener, Input, NgModule, Directive } from '@angular/core';
+import {
+  Component,
+  Directive,
+  ElementRef,
+  HostBinding,
+  HostListener,
+  Input,
+  NgModule
+} from '@angular/core';
 
 export class NonAngularBaseClass {
   greet() {}
@@ -55,3 +63,12 @@ export class WrappedMyComp extends MyComp {}
 
 @NgModule({declarations: [MyComp, WrappedMyComp]})
 export class TestModule {}
+
+@Directive({selector: null})
+export class AbstractDir {}
+
+@Directive()
+export class DerivedAbstractDir extends AbstractDir {}
+
+@Directive()
+export class WrappedDerivedAbstractDir extends DerivedAbstractDir {}

packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/BUILD.bazel

@@ -11,6 +11,8 @@ ts_library(
         "//packages/core/schematics/test:__pkg__",
     ],
     deps = [
+        "//packages/compiler-cli/src/ngtsc/partial_evaluator",
+        "//packages/compiler-cli/src/ngtsc/reflection",
         "//packages/core/schematics/utils",
         "@npm//@angular-devkit/schematics",
         "@npm//@types/node",

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

@@ -6,17 +6,33 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {PartialEvaluator} from '@angular/compiler-cli/src/ngtsc/partial_evaluator';
+import {TypeScriptReflectionHost, reflectObjectLiteral} from '@angular/compiler-cli/src/ngtsc/reflection';
 import * as ts from 'typescript';
 
 import {ImportManager} from '../../utils/import_manager';
-import {getAngularDecorators} from '../../utils/ng_decorators';
+import {NgDecorator, getAngularDecorators} from '../../utils/ng_decorators';
 import {findBaseClassDeclarations} from '../../utils/typescript/find_base_classes';
+import {unwrapExpression} from '../../utils/typescript/functions';
 
 import {UpdateRecorder} from './update_recorder';
 
+
+/** Analyzed class declaration. */
+interface AnalyzedClass {
+  /** Whether the class is decorated with @Directive or @Component. */
+  isDirectiveOrComponent: boolean;
+  /** Whether the class is an abstract directive. */
+  isAbstractDirective: boolean;
+  /** Whether the class uses any Angular features. */
+  usesAngularFeatures: boolean;
+}
+
 export class UndecoratedClassesWithDecoratedFieldsTransform {
   private printer = ts.createPrinter();
   private importManager = new ImportManager(this.getUpdateRecorder, this.printer);
+  private reflectionHost = new TypeScriptReflectionHost(this.typeChecker);
+  private partialEvaluator = new PartialEvaluator(this.reflectionHost, this.typeChecker, null);
 
   constructor(
       private typeChecker: ts.TypeChecker,
@@ -28,7 +44,7 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
    * https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA
    */
   migrate(sourceFiles: ts.SourceFile[]) {
-    this._findUndecoratedDirectives(sourceFiles).forEach(node => {
+    this._findUndecoratedAbstractDirectives(sourceFiles).forEach(node => {
       const sourceFile = node.getSourceFile();
       const recorder = this.getUpdateRecorder(sourceFile);
       const directiveExpr =
@@ -42,54 +58,96 @@ export class UndecoratedClassesWithDecoratedFieldsTransform {
   /** Records all changes that were made in the import manager. */
   recordChanges() { this.importManager.recordChanges(); }
 
-  /** Finds undecorated directives in the specified source files. */
+  /** Finds undecorated abstract directives in the specified source files. */
-  private _findUndecoratedDirectives(sourceFiles: ts.SourceFile[]) {
+  private _findUndecoratedAbstractDirectives(sourceFiles: ts.SourceFile[]) {
-    const typeChecker = this.typeChecker;
+    const result = new Set<ts.ClassDeclaration>();
-    const undecoratedDirectives = new Set<ts.ClassDeclaration>();
     const undecoratedClasses = new Set<ts.ClassDeclaration>();
-    const decoratedDirectives = new WeakSet<ts.ClassDeclaration>();
+    const nonAbstractDirectives = new WeakSet<ts.ClassDeclaration>();
+    const abstractDirectives = new WeakSet<ts.ClassDeclaration>();
 
     const visitNode = (node: ts.Node) => {
       node.forEachChild(visitNode);
       if (!ts.isClassDeclaration(node)) {
         return;
       }
-      const ngDecorators = node.decorators && getAngularDecorators(typeChecker, node.decorators);
+      const {isDirectiveOrComponent, isAbstractDirective, usesAngularFeatures} =
-      const isDirectiveOrComponent = ngDecorators !== undefined &&
+          this._analyzeClassDeclaration(node);
-          ngDecorators.some(({name}) => name === 'Directive' || name === 'Component');
       if (isDirectiveOrComponent) {
-        decoratedDirectives.add(node);
+        if (isAbstractDirective) {
-      } else {
+          abstractDirectives.add(node);
-        if (this._hasAngularDecoratedClassMember(node)) {
-          undecoratedDirectives.add(node);
         } else {
-          undecoratedClasses.add(node);
+          nonAbstractDirectives.add(node);
         }
+      } else if (usesAngularFeatures) {
+        abstractDirectives.add(node);
+        result.add(node);
+      } else {
+        undecoratedClasses.add(node);
       }
     };
 
     sourceFiles.forEach(sourceFile => sourceFile.forEachChild(visitNode));
 
-    // We collected all class declarations that use Angular features but are not decorated. For
+    // We collected all undecorated class declarations which inherit from abstract directives.
-    // such undecorated directives, the derived classes also need to be migrated. To achieve this,
+    // For such abstract directives, the derived classes also need to be migrated.
-    // we walk through all undecorated classes and mark those which extend from an undecorated
-    // directive as undecorated directive too.
     undecoratedClasses.forEach(node => {
       for (const {node: baseClass} of findBaseClassDeclarations(node, this.typeChecker)) {
-        // If the undecorated class inherits from a decorated directive, skip the current class.
+        // If the undecorated class inherits from a non-abstract directive, skip the current
-        // We do this because undecorated classes which inherit from directives/components are
+        // class. We do this because undecorated classes which inherit metadata from non-abstract
-        // handled in the `undecorated-classes-with-di` migration which copies inherited metadata.
+        // directives are handle in the `undecorated-classes-with-di` migration that copies
-        if (decoratedDirectives.has(baseClass)) {
+        // inherited metadata into an explicit decorator.
+        if (nonAbstractDirectives.has(baseClass)) {
           break;
-        } else if (undecoratedDirectives.has(baseClass)) {
+        } else if (abstractDirectives.has(baseClass)) {
-          undecoratedDirectives.add(node);
+          result.add(node);
-          undecoratedClasses.delete(node);
           break;
         }
       }
     });
 
-    return undecoratedDirectives;
+    return result;
+  }
+
+  /**
+   * Analyzes the given class declaration by determining whether the class
+   * is a directive, is an abstract directive, or uses Angular features.
+   */
+  private _analyzeClassDeclaration(node: ts.ClassDeclaration): AnalyzedClass {
+    const ngDecorators = node.decorators && getAngularDecorators(this.typeChecker, node.decorators);
+    const usesAngularFeatures = this._hasAngularDecoratedClassMember(node);
+    if (ngDecorators === undefined || ngDecorators.length === 0) {
+      return {isDirectiveOrComponent: false, isAbstractDirective: false, usesAngularFeatures};
+    }
+    const directiveDecorator = ngDecorators.find(({name}) => name === 'Directive');
+    const componentDecorator = ngDecorators.find(({name}) => name === 'Component');
+    const isAbstractDirective =
+        directiveDecorator !== undefined && this._isAbstractDirective(directiveDecorator);
+    return {
+      isDirectiveOrComponent: !!directiveDecorator || !!componentDecorator,
+      isAbstractDirective,
+      usesAngularFeatures,
+    };
+  }
+
+  /**
+  * Checks whether the given decorator resolves to an abstract directive. An directive is
+  * considered "abstract" if there is no selector specified.
+  */
+  private _isAbstractDirective({node}: NgDecorator): boolean {
+    const metadataArgs = node.expression.arguments;
+    if (metadataArgs.length === 0) {
+      return true;
+    }
+    const metadataExpr = unwrapExpression(metadataArgs[0]);
+    if (!ts.isObjectLiteralExpression(metadataExpr)) {
+      return false;
+    }
+    const metadata = reflectObjectLiteral(metadataExpr);
+    if (!metadata.has('selector')) {
+      return false;
+    }
+    const selector = this.partialEvaluator.evaluate(metadata.get('selector') !);
+    return selector == null;
   }
 
   private _hasAngularDecoratedClassMember(node: ts.ClassDeclaration): boolean {

packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts

@@ -259,6 +259,39 @@ describe('Undecorated classes with decorated fields migration', () => {
     expect(fileContent).toMatch(/}\s+export class MyCompWrapped/);
   });
 
+  it('should add @Directive to derived undecorated classes of abstract directives', async() => {
+    writeFile('/index.ts', `
+      import { Input, Directive, NgModule } from '@angular/core';
+
+      @Directive()
+      export class Base {
+        // ...
+      }
+      
+      export class DerivedA extends Base {}
+      export class DerivedB extends DerivedA {}
+      export class DerivedC extends DerivedB {}
+      
+      @Directive({selector: 'my-comp'})
+      export class MyComp extends DerivedC {}
+      
+      export class MyCompWrapped extends MyComp {}
+      
+      @NgModule({declarations: [MyComp, MyCompWrapped]})
+      export class AppModule {} 
+    `);
+
+    await runMigration();
+    const fileContent = tree.readContent('/index.ts');
+    expect(fileContent).toContain(`import { Input, Directive, NgModule } from '@angular/core';`);
+    expect(fileContent).toMatch(/core';\s+@Directive\(\)\s+export class Base/);
+    expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedA/);
+    expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedB/);
+    expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedC/);
+    expect(fileContent).toMatch(/}\s+@Directive\(\{selector: 'my-comp'}\)\s+export class MyComp/);
+    expect(fileContent).toMatch(/}\s+export class MyCompWrapped/);
+  });
+
   function writeFile(filePath: string, contents: string) {
     host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents));
   }

packages/core/schematics/utils/typescript/functions.ts

@@ -17,11 +17,11 @@ export function isFunctionLikeDeclaration(node: ts.Node): node is ts.FunctionLik
 
 /**
  * Unwraps a given expression TypeScript node. Expressions can be wrapped within multiple
- * parentheses. e.g. "(((({exp}))))()". The function should return the TypeScript node
+ * parentheses or as expression. e.g. "(((({exp}))))()". The function should return the
- * referring to the inner expression. e.g "exp".
+ * TypeScript node referring to the inner expression. e.g "exp".
  */
 export function unwrapExpression(node: ts.Expression | ts.ParenthesizedExpression): ts.Expression {
-  if (ts.isParenthesizedExpression(node)) {
+  if (ts.isParenthesizedExpression(node) || ts.isAsExpression(node)) {
     return unwrapExpression(node.expression);
   } else {
     return node;