angular · JoostK · Jan 4, 2022
diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md
@@ -43,6 +43,7 @@ export enum ErrorCode {
     IMPORT_CYCLE_DETECTED = 3003,
     IMPORT_GENERATION_FAILURE = 3004,
     INJECTABLE_DUPLICATE_PROV = 9001,
+    INJECTABLE_INHERITS_INVALID_CONSTRUCTOR = 2016,
     INLINE_TCB_REQUIRED = 8900,
     INLINE_TYPE_CTOR_REQUIRED = 8901,
     INVALID_BANANA_IN_BOX = 8101,

diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts
@@ -10,12 +10,13 @@ import ts from 'typescript';
 
 import {ParsedConfiguration} from '../../..';
 import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader} from '../../../src/ngtsc/annotations';
+import {InjectableClassRegistry} from '../../../src/ngtsc/annotations/common';
 import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../../src/ngtsc/cycles';
 import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics';
 import {absoluteFromSourceFile, LogicalFileSystem, ReadonlyFileSystem} from '../../../src/ngtsc/file_system';
 import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, PrivateExportAliasingHost, Reexport, ReferenceEmitter} from '../../../src/ngtsc/imports';
 import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph';
-import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, HostDirectivesResolver, InjectableClassRegistry, LocalMetadataRegistry, ResourceRegistry} from '../../../src/ngtsc/metadata';
+import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, HostDirectivesResolver, LocalMetadataRegistry, ResourceRegistry} from '../../../src/ngtsc/metadata';
 import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
 import {NOOP_PERF_RECORDER} from '../../../src/ngtsc/perf';
 import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver, TypeCheckScopeRegistry} from '../../../src/ngtsc/scope';
@@ -97,16 +98,17 @@ export class DecorationAnalyzer {
       new PartialEvaluator(this.reflectionHost, this.typeChecker, /* dependencyTracker */ null);
   importGraph = new ImportGraph(this.typeChecker, NOOP_PERF_RECORDER);
   cycleAnalyzer = new CycleAnalyzer(this.importGraph);
-  injectableRegistry = new InjectableClassRegistry(this.reflectionHost);
+  injectableRegistry = new InjectableClassRegistry(this.reflectionHost, this.isCore);
   hostDirectivesResolver = new HostDirectivesResolver(this.fullMetaReader);
   typeCheckScopeRegistry = new TypeCheckScopeRegistry(
       this.scopeRegistry, this.fullMetaReader, this.hostDirectivesResolver);
   handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[] = [
     new ComponentDecoratorHandler(
         this.reflectionHost, this.evaluator, this.fullRegistry, this.fullMetaReader,
         this.scopeRegistry, this.dtsModuleScopeResolver, this.scopeRegistry,
-        this.typeCheckScopeRegistry, new ResourceRegistry(), this.isCore, this.resourceManager,
-        this.rootDirs, !!this.compilerOptions.preserveWhitespaces,
+        this.typeCheckScopeRegistry, new ResourceRegistry(), this.isCore,
+        /* strictCtorDeps */ false, this.resourceManager, this.rootDirs,
+        !!this.compilerOptions.preserveWhitespaces,
         /* i18nUseExternalIds */ true, this.bundle.enableI18nLegacyMessageIdFormat,
         /* usePoisonedData */ false,
         /* i18nNormalizeLineEndingsInICUs */ false, this.moduleResolver, this.cycleAnalyzer,
@@ -119,7 +121,7 @@ export class DecorationAnalyzer {
     // clang-format off
     new DirectiveDecoratorHandler(
         this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
-        this.fullMetaReader, this.injectableRegistry, this.refEmitter, this.isCore,
+        this.fullMetaReader, this.injectableRegistry, this.refEmitter, this.isCore, /* strictCtorDeps */ false,
         /* semanticDepGraphUpdater */ null,
         !!this.compilerOptions.annotateForClosureCompiler,
         // In ngcc we want to compile undecorated classes with Angular features. As of
@@ -136,7 +138,7 @@ export class DecorationAnalyzer {
         this.reflectionHost, this.evaluator, this.metaRegistry, this.scopeRegistry,
         this.injectableRegistry, this.isCore, NOOP_PERF_RECORDER),
     new InjectableDecoratorHandler(
-        this.reflectionHost, this.isCore,
+        this.reflectionHost, this.evaluator, this.isCore,
         /* strictCtorDeps */ false, this.injectableRegistry, NOOP_PERF_RECORDER,
         /* errorOnDuplicateProv */ false),
     new NgModuleDecoratorHandler(

diff --git a/packages/compiler-cli/src/ngtsc/annotations/common/index.ts b/packages/compiler-cli/src/ngtsc/annotations/common/index.ts
@@ -11,6 +11,7 @@ export * from './src/di';
 export * from './src/diagnostics';
 export * from './src/evaluation';
 export * from './src/factory';
+export * from './src/injectable_registry';
 export * from './src/metadata';
 export * from './src/references_registry';
 export * from './src/schema';

diff --git a/packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts
@@ -10,13 +10,14 @@ import ts from 'typescript';
 
 import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics';
 import {Reference} from '../../../imports';
-import {HostDirectiveMeta, InjectableClassRegistry, MetadataReader} from '../../../metadata';
+import {HostDirectiveMeta, MetadataReader} from '../../../metadata';
 import {describeResolvedType, DynamicValue, PartialEvaluator, ResolvedValue, traceDynamicValue} from '../../../partial_evaluator';
 import {ClassDeclaration, ReflectionHost} from '../../../reflection';
 import {DeclarationData, LocalModuleScopeRegistry} from '../../../scope';
 import {identifierOfNode} from '../../../util/src/typescript';
 
-import {readBaseClass} from './util';
+import {InjectableClassRegistry} from './injectable_registry';
+import {isAbstractClassDeclaration, readBaseClass} from './util';
 
 
 /**
@@ -102,7 +103,10 @@ export function getProviderDiagnostics(
   const diagnostics: ts.Diagnostic[] = [];
 
   for (const provider of providerClasses) {
-    if (registry.isInjectable(provider.node)) {
+    const injectableMeta = registry.getInjectableMeta(provider.node);
+    if (injectableMeta !== null) {
+      // The provided type is recognized as injectable, so we don't report a diagnostic for this
+      // provider.
       continue;
     }
 
@@ -124,9 +128,9 @@ Either add the @Injectable() decorator to '${
 }
 
 export function getDirectiveDiagnostics(
-    node: ClassDeclaration, reader: MetadataReader, evaluator: PartialEvaluator,
-    reflector: ReflectionHost, scopeRegistry: LocalModuleScopeRegistry,
-    kind: string): ts.Diagnostic[]|null {
+    node: ClassDeclaration, injectableRegistry: InjectableClassRegistry,
+    evaluator: PartialEvaluator, reflector: ReflectionHost, scopeRegistry: LocalModuleScopeRegistry,
+    strictInjectionParameters: boolean, kind: 'Directive'|'Component'): ts.Diagnostic[]|null {
   let diagnostics: ts.Diagnostic[]|null = [];
 
   const addDiagnostics = (more: ts.Diagnostic|ts.Diagnostic[]|null) => {
@@ -147,7 +151,8 @@ export function getDirectiveDiagnostics(
     addDiagnostics(makeDuplicateDeclarationError(node, duplicateDeclarations, kind));
   }
 
-  addDiagnostics(checkInheritanceOfDirective(node, reader, reflector, evaluator));
+  addDiagnostics(checkInheritanceOfInjectable(
+      node, injectableRegistry, reflector, evaluator, strictInjectionParameters, kind));
   return diagnostics;
 }
 
@@ -192,9 +197,42 @@ export function getUndecoratedClassWithAngularFeaturesDiagnostic(node: ClassDecl
           `Angular decorator.`);
 }
 
-export function checkInheritanceOfDirective(
-    node: ClassDeclaration, reader: MetadataReader, reflector: ReflectionHost,
-    evaluator: PartialEvaluator): ts.Diagnostic|null {
+export function checkInheritanceOfInjectable(
+    node: ClassDeclaration, injectableRegistry: InjectableClassRegistry, reflector: ReflectionHost,
+    evaluator: PartialEvaluator, strictInjectionParameters: boolean,
+    kind: 'Directive'|'Component'|'Pipe'|'Injectable'): ts.Diagnostic|null {
+  const classWithCtor = findInheritedCtor(node, injectableRegistry, reflector, evaluator);
+  if (classWithCtor === null || classWithCtor.isCtorValid) {
+    // The class does not inherit a constructor, or the inherited constructor is compatible
+    // with DI; no need to report a diagnostic.
+    return null;
+  }
+
+  if (!classWithCtor.isDecorated) {
+    // The inherited constructor exists in a class that does not have an Angular decorator.
+    // This is an error, as there won't be a factory definition available for DI to invoke
+    // the constructor.
+    return getInheritedUndecoratedCtorDiagnostic(node, classWithCtor.ref, kind);
+  }
+
+  if (!strictInjectionParameters || isAbstractClassDeclaration(node)) {
+    // An invalid constructor is only reported as error under `strictInjectionParameters` and
+    // only for concrete classes; follow the same exclusions for derived types.
+    return null;
+  }
+
+  return getInheritedInvalidCtorDiagnostic(node, classWithCtor.ref, kind);
+}
+
+interface ClassWithCtor {
+  ref: Reference<ClassDeclaration>;
+  isCtorValid: boolean;
+  isDecorated: boolean;
+}
+
+export function findInheritedCtor(
+    node: ClassDeclaration, injectableRegistry: InjectableClassRegistry, reflector: ReflectionHost,
+    evaluator: PartialEvaluator): ClassWithCtor|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.
@@ -211,44 +249,64 @@ export function checkInheritanceOfDirective(
       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;
+    const injectableMeta = injectableRegistry.getInjectableMeta(baseClass.node);
+    if (injectableMeta !== null) {
+      if (injectableMeta.ctorDeps !== null) {
+        // The class has an Angular decorator with a constructor.
+        return {
+          ref: baseClass,
+          isCtorValid: injectableMeta.ctorDeps !== 'invalid',
+          isDecorated: true,
+        };
+      }
+    } else {
+      const baseClassConstructorParams = reflector.getConstructorParameters(baseClass.node);
+      if (baseClassConstructorParams !== null) {
+        // The class is not decorated, but it does have constructor. An undecorated class is only
+        // allowed to have a constructor without parameters, otherwise it is invalid.
+        return {
+          ref: baseClass,
+          isCtorValid: baseClassConstructorParams.length === 0,
+          isDecorated: false,
+        };
+      }
     }
 
     // Go up the chain and continue
-    baseClass = newParentClass;
+    baseClass = readBaseClass(baseClass.node, reflector, evaluator);
   }
 
   return null;
 }
 
+function getInheritedInvalidCtorDiagnostic(
+    node: ClassDeclaration, baseClass: Reference,
+    kind: 'Directive'|'Component'|'Pipe'|'Injectable') {
+  const baseClassName = baseClass.debugName;
+
+  return makeDiagnostic(
+      ErrorCode.INJECTABLE_INHERITS_INVALID_CONSTRUCTOR, node.name,
+      `The ${kind.toLowerCase()} ${node.name.text} inherits its constructor from ${
+          baseClassName}, ` +
+          `but the latter has a constructor parameter that is not compatible with dependency injection. ` +
+          `Either add an explicit constructor to ${node.name.text} or change ${
+              baseClassName}'s constructor to ` +
+          `use parameters that are valid for DI.`);
+}
+
 function getInheritedUndecoratedCtorDiagnostic(
-    node: ClassDeclaration, baseClass: Reference, reader: MetadataReader) {
-  const subclassMeta = reader.getDirectiveMetadata(new Reference(node))!;
-  const dirOrComp = subclassMeta.isComponent ? 'Component' : 'Directive';
+    node: ClassDeclaration, baseClass: Reference,
+    kind: 'Directive'|'Component'|'Pipe'|'Injectable') {
   const baseClassName = baseClass.debugName;
+  const baseNeedsDecorator =
+      kind === 'Component' || kind === 'Directive' ? 'Directive' : 'Injectable';
 
   return makeDiagnostic(
       ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR, node.name,
-      `The ${dirOrComp.toLowerCase()} ${node.name.text} inherits its constructor from ${
+      `The ${kind.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 ` +
+          `resolve the parameters of ${baseClassName}'s constructor. Either add a @${
+              baseNeedsDecorator} decorator ` +
           `to ${baseClassName}, or add an explicit constructor to ${node.name.text}.`);
 }
diff --git a/packages/compiler-cli/src/ngtsc/annotations/common/src/injectable_registry.ts b/packages/compiler-cli/src/ngtsc/annotations/common/src/injectable_registry.ts
@@ -0,0 +1,52 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+import {R3DependencyMetadata} from '@angular/compiler';
+
+import {hasInjectableFields} from '../../../metadata';
+import {ClassDeclaration, ReflectionHost} from '../../../reflection';
+
+import {getConstructorDependencies, unwrapConstructorDependencies} from './di';
+
+
+export interface InjectableMeta {
+  ctorDeps: R3DependencyMetadata[]|'invalid'|null;
+}
+
+/**
+ * Registry that keeps track of classes that can be constructed via dependency injection (e.g.
+ * injectables, directives, pipes).
+ */
+export class InjectableClassRegistry {
+  private classes = new Map<ClassDeclaration, InjectableMeta>();
+
+  constructor(private host: ReflectionHost, private isCore: boolean) {}
+
+  registerInjectable(declaration: ClassDeclaration, meta: InjectableMeta): void {
+    this.classes.set(declaration, meta);
+  }
+
+  getInjectableMeta(declaration: ClassDeclaration): InjectableMeta|null {
+    // Figure out whether the class is injectable based on the registered classes, otherwise
+    // fall back to looking at its members since we might not have been able to register the class
+    // if it was compiled in another compilation unit.
+    if (this.classes.has(declaration)) {
+      return this.classes.get(declaration)!;
+    }
+
+    if (!hasInjectableFields(declaration, this.host)) {
+      return null;
+    }
+
+    const ctorDeps = getConstructorDependencies(declaration, this.host, this.isCore);
+    const meta: InjectableMeta = {
+      ctorDeps: unwrapConstructorDependencies(ctorDeps),
+    };
+    this.classes.set(declaration, meta);
+    return meta;
+  }
+}
diff --git a/packages/compiler-cli/src/ngtsc/annotations/common/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/common/src/util.ts
@@ -400,3 +400,8 @@ export function getOriginNodeForDiagnostics(
     return container;
   }
 }
+
+export function isAbstractClassDeclaration(clazz: ClassDeclaration): boolean {
+  return clazz.modifiers !== undefined &&
+      clazz.modifiers.some(mod => mod.kind === ts.SyntaxKind.AbstractKeyword);
+}