feat(ivy): error in ivy when inheriting a ctor from an undecorated base (#34460)

Angular View Engine uses global knowledge to compile the following code:

```typescript
export class Base {
  constructor(private vcr: ViewContainerRef) {}
}

@Directive({...})
export class Dir extends Base {
  // constructor inherited from base
}
```

Here, `Dir` extends `Base` and inherits its constructor. To create a `Dir`
the arguments to this inherited constructor must be obtained via dependency
injection. View Engine is able to generate a correct factory for `Dir` to do
this because via metadata it knows the arguments of `Base`'s constructor,
even if `Base` is declared in a different library.

In Ivy, DI is entirely a runtime concept. Currently `Dir` is compiled with
an ngDirectiveDef field that delegates its factory to `getInheritedFactory`.
This looks for some kind of factory function on `Base`, which comes up
empty. This case looks identical to an inheritance chain with no
constructors, which works today in Ivy.

Both of these cases will now become an error in this commit. If a decorated
class inherits from an undecorated base class, a diagnostic is produced
informing the user of the need to either explicitly declare a constructor or
to decorate the base class.

PR Close #34460

packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts

@@ -100,7 +100,7 @@ export class DecorationAnalyzer {
     // clang-format off
     new DirectiveDecoratorHandler(
         this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
-        NOOP_DEFAULT_IMPORT_RECORDER, this.injectableRegistry, this.isCore,
+        this.fullMetaReader, NOOP_DEFAULT_IMPORT_RECORDER, this.injectableRegistry, this.isCore,
         /* annotateForClosureCompiler */ false) as DecoratorHandler<unknown, unknown, unknown>,
     // clang-format on
     // Pipe handler must be before injectable handler in list so pipe factories are printed

packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts

@@ -207,7 +207,7 @@ runInEachFileSystem(() => {
 
     it('should skip the base class if it is in a different package from the derived class', () => {
       const BASE_FILENAME = _('/node_modules/other-package/index.js');
-      const {program, analysis, errors} = setUpAndAnalyzeProgram([
+      const {errors} = setUpAndAnalyzeProgram([
         {
           name: INDEX_FILENAME,
           contents: `
@@ -232,9 +232,16 @@ runInEachFileSystem(() => {
       `
         }
       ]);
-      expect(errors).toEqual([]);
-      const file = analysis.get(program.getSourceFile(BASE_FILENAME) !);
-      expect(file).toBeUndefined();
+
+      expect(errors.length).toBe(1);
+      expect(errors[0].messageText)
+          .toBe(
+              `The directive DerivedClass inherits its constructor ` +
+              `from BaseClass, but the latter does not have an Angular ` +
+              `decorator of its own. Dependency injection will not be ` +
+              `able to resolve the parameters of BaseClass's ` +
+              `constructor. Either add a @Directive decorator to ` +
+              `BaseClass, or add an explicit constructor to DerivedClass.`);
     });
   });
 

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

@@ -26,7 +26,7 @@ import {tsSourceMapBug29300Fixed} from '../../util/src/ts_source_map_bug_29300';
 import {SubsetOfKeys} from '../../util/src/typescript';
 
 import {ResourceLoader} from './api';
-import {getProviderDiagnostics} from './diagnostics';
+import {getDirectiveDiagnostics, getProviderDiagnostics} from './diagnostics';
 import {extractDirectiveMetadata, parseFieldArrayValue} from './directive';
 import {compileNgFactoryDefField} from './factory';
 import {generateSetClassMetadataCall} from './metadata';
@@ -550,9 +550,10 @@ export class ComponentDecoratorHandler implements
       diagnostics.push(...viewProviderDiagnostics);
     }
 
-    const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node);
-    if (duplicateDeclData !== null) {
-      diagnostics.push(makeDuplicateDeclarationError(node, duplicateDeclData, 'Component'));
+    const directiveDiagnostics = getDirectiveDiagnostics(
+        node, this.metaReader, this.evaluator, this.reflector, this.scopeRegistry, 'Component');
+    if (directiveDiagnostics !== null) {
+      diagnostics.push(...directiveDiagnostics);
     }
 
     if (diagnostics.length > 0) {

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

@@ -10,8 +10,12 @@ import * as ts from 'typescript';
 
 import {ErrorCode, makeDiagnostic} from '../../diagnostics';
 import {Reference} from '../../imports';
-import {InjectableClassRegistry} from '../../metadata';
-import {ClassDeclaration} from '../../reflection';
+import {InjectableClassRegistry, MetadataReader} from '../../metadata';
+import {PartialEvaluator} from '../../partial_evaluator';
+import {ClassDeclaration, ReflectionHost} from '../../reflection';
+import {LocalModuleScopeRegistry} from '../../scope';
+
+import {makeDuplicateDeclarationError, readBaseClass} from './util';
 
 /**
  * Gets the diagnostics for a set of provider classes.
@@ -41,3 +45,90 @@ Either add the @Injectable() decorator to '${provider.node.name.text}', or confi
 
   return diagnostics;
 }
+
+export function getDirectiveDiagnostics(
+    node: ClassDeclaration, reader: MetadataReader, evaluator: PartialEvaluator,
+    reflector: ReflectionHost, scopeRegistry: LocalModuleScopeRegistry,
+    kind: string): ts.Diagnostic[]|null {
+  let diagnostics: ts.Diagnostic[]|null = [];
+
+  const addDiagnostics = (more: ts.Diagnostic | ts.Diagnostic[] | null) => {
+    if (more === null) {
+      return;
+    } else if (diagnostics === null) {
+      diagnostics = Array.isArray(more) ? more : [more];
+    } else if (Array.isArray(more)) {
+      diagnostics.push(...more);
+    } else {
+      diagnostics.push(more);
+    }
+  };
+
+  const duplicateDeclarations = scopeRegistry.getDuplicateDeclarations(node);
+
+  if (duplicateDeclarations !== null) {
+    addDiagnostics(makeDuplicateDeclarationError(node, duplicateDeclarations, kind));
+  }
+
+  addDiagnostics(checkInheritanceOfDirective(node, reader, reflector, evaluator));
+  return diagnostics;
+}
+
+export function checkInheritanceOfDirective(
+    node: ClassDeclaration, reader: MetadataReader, reflector: ReflectionHost,
+    evaluator: PartialEvaluator): ts.Diagnostic|null {
+  if (!reflector.isClass(node) || reflector.getConstructorParameters(node) !== null) {
+    // We should skip nodes that aren't classes. If a constructor exists, then no base class
+    // definition is required on the runtime side - it's legal to inherit from any class.
+    return null;
+  }
+
+  // The extends clause is an expression which can be as dynamic as the user wants. Try to
+  // evaluate it, but fall back on ignoring the clause if it can't be understood. This is a View
+  // Engine compatibility hack: View Engine ignores 'extends' expressions that it cannot understand.
+  let baseClass = readBaseClass(node, reflector, evaluator);
+
+  while (baseClass !== null) {
+    if (baseClass === 'dynamic') {
+      return null;
+    }
+
+    // We can skip the base class if it has metadata.
+    const baseClassMeta = reader.getDirectiveMetadata(baseClass);
+    if (baseClassMeta !== null) {
+      return null;
+    }
+
+    // If the base class has a blank constructor we can skip it since it can't be using DI.
+    const baseClassConstructorParams = reflector.getConstructorParameters(baseClass.node);
+    const newParentClass = readBaseClass(baseClass.node, reflector, evaluator);
+
+    if (baseClassConstructorParams !== null && baseClassConstructorParams.length > 0) {
+      // This class has a non-trivial constructor, that's an error!
+      return getInheritedUndecoratedCtorDiagnostic(node, baseClass, reader);
+    } else if (baseClassConstructorParams !== null || newParentClass === null) {
+      // This class has a trivial constructor, or no constructor + is the
+      // top of the inheritance chain, so it's okay.
+      return null;
+    }
+
+    // Go up the chain and continue
+    baseClass = newParentClass;
+  }
+
+  return null;
+}
+
+function getInheritedUndecoratedCtorDiagnostic(
+    node: ClassDeclaration, baseClass: Reference, reader: MetadataReader) {
+  const subclassMeta = reader.getDirectiveMetadata(new Reference(node)) !;
+  const dirOrComp = subclassMeta.isComponent ? 'Component' : 'Directive';
+  const baseClassName = baseClass.debugName;
+
+  return makeDiagnostic(
+      ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR, node.name,
+      `The ${dirOrComp.toLowerCase()} ${node.name.text} inherits its constructor from ${baseClassName}, ` +
+          `but the latter does not have an Angular decorator of its own. Dependency injection will not be able to ` +
+          `resolve the parameters of ${baseClassName}'s constructor. Either add a @Directive decorator ` +
+          `to ${baseClassName}, or add an explicit constructor to ${node.name.text}.`);
+}

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

@@ -11,17 +11,17 @@ import * as ts from 'typescript';
 
 import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
 import {DefaultImportRecorder, Reference} from '../../imports';
-import {InjectableClassRegistry, MetadataRegistry} from '../../metadata';
+import {InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata';
 import {extractDirectiveGuards} from '../../metadata/src/util';
 import {DynamicValue, EnumValue, PartialEvaluator} from '../../partial_evaluator';
 import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, ReflectionHost, filterToMembersWithDecorator, reflectObjectLiteral} from '../../reflection';
 import {LocalModuleScopeRegistry} from '../../scope';
 import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence, ResolveResult} from '../../transform';
 
-import {getProviderDiagnostics} from './diagnostics';
+import {getDirectiveDiagnostics, getProviderDiagnostics} from './diagnostics';
 import {compileNgFactoryDefField} from './factory';
 import {generateSetClassMetadataCall} from './metadata';
-import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, makeDuplicateDeclarationError, readBaseClass, resolveProvidersRequiringFactory, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util';
+import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, resolveProvidersRequiringFactory, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util';
 
 const EMPTY_OBJECT: {[key: string]: string} = {};
 const FIELD_DECORATORS = [
@@ -46,7 +46,7 @@ export class DirectiveDecoratorHandler implements
   constructor(
       private reflector: ReflectionHost, private evaluator: PartialEvaluator,
       private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry,
-      private defaultImportRecorder: DefaultImportRecorder,
+      private metaReader: MetadataReader, private defaultImportRecorder: DefaultImportRecorder,
       private injectableRegistry: InjectableClassRegistry, private isCore: boolean,
       private annotateForClosureCompiler: boolean) {}
 
@@ -140,10 +140,10 @@ export class DirectiveDecoratorHandler implements
       diagnostics.push(...providerDiagnostics);
     }
 
-    const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node);
-    if (duplicateDeclData !== null) {
-      // This directive was declared twice (or more).
-      diagnostics.push(makeDuplicateDeclarationError(node, duplicateDeclData, 'Directive'));
+    const directiveDiagnostics = getDirectiveDiagnostics(
+        node, this.metaReader, this.evaluator, this.reflector, this.scopeRegistry, 'Directive');
+    if (directiveDiagnostics !== null) {
+      diagnostics.push(...directiveDiagnostics);
     }
 
     return {diagnostics: diagnostics.length > 0 ? diagnostics : undefined};

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

@@ -50,8 +50,8 @@ runInEachFileSystem(() => {
           null);
       const injectableRegistry = new InjectableClassRegistry(reflectionHost);
       const handler = new DirectiveDecoratorHandler(
-          reflectionHost, evaluator, scopeRegistry, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
-          injectableRegistry,
+          reflectionHost, evaluator, scopeRegistry, scopeRegistry, metaReader,
+          NOOP_DEFAULT_IMPORT_RECORDER, injectableRegistry,
           /* isCore */ false, /* annotateForClosureCompiler */ false);
 
       const analyzeDirective = (dirName: string) => {

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

@@ -29,6 +29,12 @@ export enum ErrorCode {
   /** Raised when an undecorated class is passed in as a provider to a module or a directive. */
   UNDECORATED_PROVIDER = 2005,
 
+  /**
+   * Raised when a Directive inherits its constructor from a base class without an Angular
+   * decorator.
+   */
+  DIRECTIVE_INHERITS_UNDECORATED_CTOR = 2006,
+
   SYMBOL_NOT_EXPORTED = 3001,
   SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002,
 
@@ -120,7 +126,7 @@ export enum ErrorCode {
   /**
    * An injectable already has a `ɵprov` property.
    */
-  INJECTABLE_DUPLICATE_PROV = 9001
+  INJECTABLE_DUPLICATE_PROV = 9001,
 }
 
 export function ngErrorCode(code: ErrorCode): number {

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

@@ -665,8 +665,9 @@ export class NgtscProgram implements api.Program {
       // being assignable to `unknown` when wrapped in `Readonly`).
       // clang-format off
       new DirectiveDecoratorHandler(
-          this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.defaultImportTracker, injectableRegistry,
-          this.isCore, this.closureCompilerEnabled) as Readonly<DecoratorHandler<unknown, unknown, unknown>>,
+          this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.metaReader,
+          this.defaultImportTracker, injectableRegistry, this.isCore, this.closureCompilerEnabled
+      ) as Readonly<DecoratorHandler<unknown, unknown, unknown>>,
       // clang-format on
       // Pipe handler must be before injectable handler in list so pipe factories are printed
       // before injectable factories (so injectable factories can delegate to them)

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

@@ -2895,6 +2895,9 @@ runInEachFileSystem(os => {
       env.write(`test.ts`, `
         import {Directive} from '@angular/core';
 
+        @Directive({
+          selector: '[base]',
+        })
         class Base {}
 
         @Directive({
@@ -5131,6 +5134,245 @@ export const Foo = Foo__PRE_R3__;
       });
     });
 
+    describe('inherited directives', () => {
+      beforeEach(() => {
+        env.write('local.ts', `
+          import {Component, Directive, ElementRef} from '@angular/core';
+
+          export class BasePlain {}
+
+          export class BasePlainWithBlankConstructor {
+            constructor() {}
+          }
+
+          export class BasePlainWithConstructorParameters {
+            constructor(elementRef: ElementRef) {}
+          }
+
+          @Component({
+            selector: 'base-cmp',
+            template: 'BaseCmp',
+          })
+          export class BaseCmp {}
+
+          @Directive({
+            selector: '[base]',
+          })
+          export class BaseDir {}
+        `);
+
+        env.write('lib.d.ts', `
+          import {ɵɵComponentDefWithMeta, ɵɵDirectiveDefWithMeta, ElementRef} from '@angular/core';
+
+          export declare class BasePlain {}
+
+          export declare class BasePlainWithBlankConstructor {
+            constructor() {}
+          }
+
+          export declare class BasePlainWithConstructorParameters {
+            constructor(elementRef: ElementRef) {}
+          }
+
+          export declare class BaseCmp {
+            static ɵcmp: ɵɵComponentDefWithMeta<BaseCmp, "base-cmp", never, {}, {}, never>
+          }
+
+          export declare class BaseDir {
+            static ɵdir: ɵɵDirectiveDefWithMeta<BaseDir, '[base]', never, never, never, never>;
+          }
+        `);
+      });
+
+      it('should not error when inheriting a constructor from a decorated directive class', () => {
+        env.tsconfig();
+        env.write('test.ts', `
+          import {Directive, Component} from '@angular/core';
+          import {BaseDir, BaseCmp} from './local';
+
+          @Directive({
+            selector: '[dir]',
+          })
+          export class Dir extends BaseDir {}
+
+          @Component({
+            selector: 'test-cmp',
+            template: 'TestCmp',
+          })
+          export class Cmp extends BaseCmp {}
+        `);
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(0);
+      });
+
+      it('should not error when inheriting a constructor without parameters', () => {
+        env.tsconfig();
+        env.write('test.ts', `
+          import {Directive, Component} from '@angular/core';
+          import {BasePlainWithBlankConstructor} from './local';
+
+          @Directive({
+            selector: '[dir]',
+          })
+          export class Dir extends BasePlainWithBlankConstructor {}
+
+          @Component({
+            selector: 'test-cmp',
+            template: 'TestCmp',
+          })
+          export class Cmp extends BasePlainWithBlankConstructor {}
+        `);
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(0);
+      });
+
+      it('should not error when inheriting from a class without a constructor', () => {
+        env.tsconfig();
+        env.write('test.ts', `
+          import {Directive, Component} from '@angular/core';
+          import {BasePlain} from './local';
+
+          @Directive({
+            selector: '[dir]',
+          })
+          export class Dir extends BasePlain {}
+
+          @Component({
+            selector: 'test-cmp',
+            template: 'TestCmp',
+          })
+          export class Cmp extends BasePlain {}
+        `);
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(0);
+      });
+
+      it('should error when inheriting a constructor from an undecorated class', () => {
+        env.tsconfig();
+        env.write('test.ts', `
+          import {Directive, Component} from '@angular/core';
+          import {BasePlainWithConstructorParameters} from './local';
+
+          @Directive({
+            selector: '[dir]',
+          })
+          export class Dir extends BasePlainWithConstructorParameters {}
+
+          @Component({
+            selector: 'test-cmp',
+            template: 'TestCmp',
+          })
+          export class Cmp extends BasePlainWithConstructorParameters {}
+        `);
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(2);
+        expect(diags[0].messageText).toContain('Dir');
+        expect(diags[0].messageText).toContain('BasePlainWithConstructorParameters');
+        expect(diags[1].messageText).toContain('Cmp');
+        expect(diags[1].messageText).toContain('BasePlainWithConstructorParameters');
+      });
+
+      it('should error when inheriting a constructor from undecorated grand super class', () => {
+        env.tsconfig();
+        env.write('test.ts', `
+          import {Directive, Component} from '@angular/core';
+          import {BasePlainWithConstructorParameters} from './local';
+
+          class Parent extends BasePlainWithConstructorParameters {}
+
+          @Directive({
+            selector: '[dir]',
+          })
+          export class Dir extends Parent {}
+
+          @Component({
+            selector: 'test-cmp',
+            template: 'TestCmp',
+          })
+          export class Cmp extends Parent {}
+        `);
+
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(2);
+        expect(diags[0].messageText).toContain('Dir');
+        expect(diags[0].messageText).toContain('BasePlainWithConstructorParameters');
+        expect(diags[1].messageText).toContain('Cmp');
+        expect(diags[1].messageText).toContain('BasePlainWithConstructorParameters');
+      });
+
+      it('should error when inheriting a constructor from undecorated grand grand super class',
+         () => {
+           env.tsconfig();
+           env.write('test.ts', `
+              import {Directive, Component} from '@angular/core';
+              import {BasePlainWithConstructorParameters} from './local';
+
+              class GrandParent extends BasePlainWithConstructorParameters {}
+
+              class Parent extends GrandParent {}
+
+              @Directive({
+                selector: '[dir]',
+              })
+              export class Dir extends Parent {}
+
+              @Component({
+                selector: 'test-cmp',
+                template: 'TestCmp',
+              })
+              export class Cmp extends Parent {}
+            `);
+
+           const diags = env.driveDiagnostics();
+           expect(diags.length).toBe(2);
+           expect(diags[0].messageText).toContain('Dir');
+           expect(diags[0].messageText).toContain('BasePlainWithConstructorParameters');
+           expect(diags[1].messageText).toContain('Cmp');
+           expect(diags[1].messageText).toContain('BasePlainWithConstructorParameters');
+         });
+
+      it('should not error when inheriting a constructor from decorated directive or component classes in a .d.ts file',
+         () => {
+           env.tsconfig();
+           env.write('test.ts', `
+              import {Component, Directive} from '@angular/core';
+              import {BaseDir, BaseCmp} from './lib';
+
+              @Directive({
+                selector: '[dir]',
+              })
+              export class Dir extends BaseDir {}
+
+              @Component({
+                selector: 'test-cmp',
+                template: 'TestCmp',
+              })
+              export class Cmp extends BaseCmp {}
+           `);
+           const diags = env.driveDiagnostics();
+           expect(diags.length).toBe(0);
+         });
+
+      it('should error when inheriting a constructor from an undecorated class in a .d.ts file',
+         () => {
+           env.tsconfig();
+           env.write('test.ts', `
+              import {Directive} from '@angular/core';
+
+              import {BasePlainWithConstructorParameters} from './lib';
+
+              @Directive({
+                selector: '[dir]',
+              })
+              export class Dir extends BasePlainWithConstructorParameters {}
+            `);
+           const diags = env.driveDiagnostics();
+           expect(diags.length).toBe(1);
+           expect(diags[0].messageText).toContain('Dir');
+           expect(diags[0].messageText).toContain('Base');
+         });
+    });
+
     describe('inline resources', () => {
       it('should process inline <style> tags', () => {
         env.write('test.ts', `