feat(compiler-cli): report error if undecorated class with Angular features is discovered (#36921)

Previously in v9, we deprecated the pattern of undecorated base classes
that rely on Angular features. We ran a migration for this in version 9
and will run the same on in version 10 again.

To ensure that projects do not regress and start using the unsupported
pattern again, we report an error in ngtsc if such undecorated classes
are discovered.

We keep the compatibility code enabled in ngcc so that libraries
can be still be consumed, even if they have not been migrated yet.

Resolves FW-2130.

PR Close #36921

packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts

@@ -78,6 +78,14 @@ export function getDirectiveDiagnostics(
   return diagnostics;
 }
 
+export function getUndecoratedClassWithAngularFeaturesDiagnostic(node: ClassDeclaration):
+    ts.Diagnostic {
+  return makeDiagnostic(
+      ErrorCode.UNDECORATED_CLASS_USING_ANGULAR_FEATURES, node.name,
+      `Class is using Angular features but is not decorated. Please add an explicit ` +
+          `Angular decorator.`);
+}
+
 export function checkInheritanceOfDirective(
     node: ClassDeclaration, reader: MetadataReader, reflector: ReflectionHost,
     evaluator: PartialEvaluator): ts.Diagnostic|null {

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

@@ -18,7 +18,7 @@ import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, filterToMembe
 import {LocalModuleScopeRegistry} from '../../scope';
 import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence, ResolveResult} from '../../transform';
 
-import {getDirectiveDiagnostics, getProviderDiagnostics} from './diagnostics';
+import {getDirectiveDiagnostics, getProviderDiagnostics, getUndecoratedClassWithAngularFeaturesDiagnostic} from './diagnostics';
 import {compileNgFactoryDefField} from './factory';
 import {generateSetClassMetadataCall} from './metadata';
 import {createSourceSpan, findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, resolveProvidersRequiringFactory, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util';
@@ -48,28 +48,20 @@ export class DirectiveDecoratorHandler implements
       private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry,
       private metaReader: MetadataReader, private defaultImportRecorder: DefaultImportRecorder,
       private injectableRegistry: InjectableClassRegistry, private isCore: boolean,
-      private annotateForClosureCompiler: boolean) {}
+      private annotateForClosureCompiler: boolean,
+      private compileUndecoratedClassesWithAngularFeatures: boolean) {}
 
   readonly precedence = HandlerPrecedence.PRIMARY;
   readonly name = DirectiveDecoratorHandler.name;
 
   detect(node: ClassDeclaration, decorators: Decorator[]|null):
       DetectResult<Decorator|null>|undefined {
-    // If the class is undecorated, check if any of the fields have Angular decorators or lifecycle
-    // hooks, and if they do, label the class as an abstract directive.
+    // If a class is undecorated but uses Angular features, we detect it as an
+    // abstract directive. This is an unsupported pattern as of v10, but we want
+    // to still detect these patterns so that we can report diagnostics, or compile
+    // them for backwards compatibility in ngcc.
     if (!decorators) {
-      const angularField = this.reflector.getMembersOfClass(node).find(member => {
-        if (!member.isStatic && member.kind === ClassMemberKind.Method &&
-            LIFECYCLE_HOOKS.has(member.name)) {
-          return true;
-        }
-        if (member.decorators) {
-          return member.decorators.some(
-              decorator => FIELD_DECORATORS.some(
-                  decoratorName => isAngularDecorator(decorator, decoratorName, this.isCore)));
-        }
-        return false;
-      });
+      const angularField = this.findClassFieldWithAngularFeatures(node);
       return angularField ? {trigger: angularField.node, decorator: null, metadata: null} :
                             undefined;
     } else {
@@ -80,6 +72,14 @@ export class DirectiveDecoratorHandler implements
 
   analyze(node: ClassDeclaration, decorator: Readonly<Decorator|null>, flags = HandlerFlags.NONE):
       AnalysisOutput<DirectiveHandlerData> {
+    // Skip processing of the class declaration if compilation of undecorated classes
+    // with Angular features is disabled. Previously in ngtsc, such classes have always
+    // been processed, but we want to enforce a consistent decorator mental model.
+    // See: https://v9.angular.io/guide/migration-undecorated-classes.
+    if (this.compileUndecoratedClassesWithAngularFeatures === false && decorator === null) {
+      return {diagnostics: [getUndecoratedClassWithAngularFeaturesDiagnostic(node)]};
+    }
+
     const directiveResult = extractDirectiveMetadata(
         node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore,
         flags, this.annotateForClosureCompiler);
@@ -167,6 +167,27 @@ export class DirectiveDecoratorHandler implements
       }
     ];
   }
+
+  /**
+   * Checks if a given class uses Angular features and returns the TypeScript node
+   * that indicated the usage. Classes are considered using Angular features if they
+   * contain class members that are either decorated with a known Angular decorator,
+   * or if they correspond to a known Angular lifecycle hook.
+   */
+  private findClassFieldWithAngularFeatures(node: ClassDeclaration): ClassMember|undefined {
+    return this.reflector.getMembersOfClass(node).find(member => {
+      if (!member.isStatic && member.kind === ClassMemberKind.Method &&
+          LIFECYCLE_HOOKS.has(member.name)) {
+        return true;
+      }
+      if (member.decorators) {
+        return member.decorators.some(
+            decorator => FIELD_DECORATORS.some(
+                decoratorName => isAngularDecorator(decorator, decoratorName, this.isCore)));
+      }
+      return false;
+    });
+  }
 }
 
 /**

packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts

@@ -99,7 +99,8 @@ runInEachFileSystem(() => {
     const handler = new DirectiveDecoratorHandler(
         reflectionHost, evaluator, scopeRegistry, scopeRegistry, metaReader,
         NOOP_DEFAULT_IMPORT_RECORDER, injectableRegistry, /*isCore*/ false,
-        /*annotateForClosureCompiler*/ false);
+        /*annotateForClosureCompiler*/ false,
+        /*detectUndecoratedClassesWithAngularFeatures*/ false);
 
     const DirNode = getDeclaration(program, _('/entry.ts'), dirName, isNamedClassDeclaration);
 

packages/compiler-cli/src/ngtsc/core/src/compiler.ts

@@ -704,7 +704,11 @@ export class NgCompiler {
       // clang-format off
         new DirectiveDecoratorHandler(
             reflector, evaluator, metaRegistry, scopeRegistry, metaReader,
-            defaultImportTracker, injectableRegistry, isCore, this.closureCompilerEnabled
+            defaultImportTracker, injectableRegistry, isCore, this.closureCompilerEnabled,
+            // In ngtsc we no longer want to compile undecorated classes with Angular features.
+            // Migrations for these patterns ran as part of `ng update` and we want to ensure
+            // that projects do not regress. See https://hackmd.io/@alx/ryfYYuvzH for more details.
+            /* compileUndecoratedClassesWithAngularFeatures */ false
         ) as Readonly<DecoratorHandler<unknown, unknown, unknown>>,
       // clang-format on
       // Pipe handler must be before injectable handler in list so pipe factories are printed

packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts

@@ -38,6 +38,12 @@ export enum ErrorCode {
    */
   DIRECTIVE_INHERITS_UNDECORATED_CTOR = 2006,
 
+  /**
+   * Raised when an undecorated class that is using Angular features
+   * has been discovered.
+   */
+  UNDECORATED_CLASS_USING_ANGULAR_FEATURES = 2007,
+
   SYMBOL_NOT_EXPORTED = 3001,
   SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002,