feat(compiler-cli): exclude abstract classes from strictInjectionParameters requirement (#44615)

In AOT compilations, the `strictInjectionParameters` compiler option can
be enabled to report errors when an `@Injectable` annotated class has a
constructor with parameters that do not provide an injection token, e.g.
only a primitive type or interface.

Since Ivy it's become required that any class with Angular behavior
(e.g. the `ngOnDestroy` lifecycle hook) is decorated using an Angular
decorator, which meant that `@Injectable()` may need to have been added
to abstract base classes. Doing so would then report an error if
`strictInjectionParameters` is enabled, if the abstract class has an
incompatible constructor for DI purposes. This may be fine though, as
a subclass may call the constructor explicitly without relying on
Angular's DI mechanism.

Therefore, this commit excludes abstract classes from the
`strictInjectionParameters` check. This avoids an error from being
reported at compile time. If the constructor ends up being used by
Angular's DI system at runtime, then the factory function of the
abstract class will throw an error by means of the `ɵɵinvalidFactory`
instruction.

In addition to the runtime error, this commit also analyzes the inheritance
chain of an injectable without a constructor to verify that their inherited
constructor is valid.

Closes #37914

PR Close #44615

Author: JoostK

goldens/public-api/compiler-cli/error_code.md

@@ -42,6 +42,7 @@ export enum ErrorCode {
     IMPORT_CYCLE_DETECTED = 3003,
     IMPORT_GENERATION_FAILURE = 3004,
     INJECTABLE_DUPLICATE_PROV = 9001,
+    INJECTABLE_INHERITS_INVALID_CONSTRUCTOR = 2013,
     INLINE_TCB_REQUIRED = 8900,
     INLINE_TYPE_CTOR_REQUIRED = 8901,
     INVALID_BANANA_IN_BOX = 8101,

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, InjectableClassRegistry, LocalMetadataRegistry, ResourceRegistry} from '../../../src/ngtsc/metadata';
+import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, 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,14 +98,15 @@ 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);
   typeCheckScopeRegistry = new TypeCheckScopeRegistry(this.scopeRegistry, this.fullMetaReader);
   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,
@@ -117,7 +119,7 @@ export class DecorationAnalyzer {
     // clang-format off
     new DirectiveDecoratorHandler(
         this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
-        this.fullMetaReader, this.injectableRegistry, this.isCore,
+        this.fullMetaReader, this.injectableRegistry, this.isCore, /* strictCtorDeps */ false,
         /* semanticDepGraphUpdater */ null,
         !!this.compilerOptions.annotateForClosureCompiler,
         // In ngcc we want to compile undecorated classes with Angular features. As of
@@ -134,7 +136,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(

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';

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

@@ -10,13 +10,13 @@ import ts from 'typescript';
 
 import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics';
 import {Reference} from '../../../imports';
-import {InjectableClassRegistry, 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 +102,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 +127,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 +150,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;
 }
 
@@ -159,9 +163,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.
@@ -178,44 +215,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}.`);
 }

packages/compiler-cli/src/ngtsc/annotations/common/src/injectable_registry.ts

@@ -0,0 +1,50 @@
+/**
+ * @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);
+    return {
+      ctorDeps: unwrapConstructorDependencies(ctorDeps),
+    };
+  }
+}

packages/compiler-cli/src/ngtsc/annotations/common/src/util.ts

@@ -380,3 +380,8 @@ export function resolveImportedFile(
   // Figure out what file is being imported.
   return moduleResolver.resolveModule(expr.value.moduleName!, origin.fileName);
 }
+
+export function isAbstractClassDeclaration(clazz: ClassDeclaration): boolean {
+  return clazz.modifiers !== undefined &&
+      clazz.modifiers.some(mod => mod.kind === ts.SyntaxKind.AbstractKeyword);
+}

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

@@ -16,7 +16,7 @@ import {assertSuccessfulReferenceEmit, ImportedFile, ModuleResolver, Reference,
 import {DependencyTracker} from '../../../incremental/api';
 import {extractSemanticTypeParameters, SemanticDepGraphUpdater} from '../../../incremental/semantic_graph';
 import {IndexingContext} from '../../../indexer';
-import {DirectiveMeta, extractDirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry, MetaKind, PipeMeta, ResourceRegistry} from '../../../metadata';
+import {DirectiveMeta, extractDirectiveTypeCheckMeta, MetadataReader, MetadataRegistry, MetaKind, PipeMeta, ResourceRegistry} from '../../../metadata';
 import {PartialEvaluator} from '../../../partial_evaluator';
 import {PerfEvent, PerfRecorder} from '../../../perf';
 import {ClassDeclaration, DeclarationNode, Decorator, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
@@ -26,7 +26,7 @@ import {TypeCheckContext} from '../../../typecheck/api';
 import {ExtendedTemplateChecker} from '../../../typecheck/extended/api';
 import {getSourceFile} from '../../../util/src/typescript';
 import {Xi18nContext} from '../../../xi18n';
-import {compileDeclareFactory, compileNgFactoryDefField, compileResults, extractClassMetadata, extractSchemas, findAngularDecorator, getDirectiveDiagnostics, getProviderDiagnostics, isExpressionForwardReference, readBaseClass, resolveEnumValue, resolveImportedFile, resolveLiteral, resolveProvidersRequiringFactory, ResourceLoader, toFactoryMetadata, wrapFunctionExpressionsInParens} from '../../common';
+import {compileDeclareFactory, compileNgFactoryDefField, compileResults, extractClassMetadata, extractSchemas, findAngularDecorator, getDirectiveDiagnostics, getProviderDiagnostics, InjectableClassRegistry, isExpressionForwardReference, readBaseClass, resolveEnumValue, resolveImportedFile, resolveLiteral, resolveProvidersRequiringFactory, ResourceLoader, toFactoryMetadata, wrapFunctionExpressionsInParens} from '../../common';
 import {extractDirectiveMetadata, parseFieldArrayValue} from '../../directive';
 import {NgModuleSymbol} from '../../ng_module';
 
@@ -51,12 +51,13 @@ export class ComponentDecoratorHandler implements
       private scopeRegistry: LocalModuleScopeRegistry,
       private typeCheckScopeRegistry: TypeCheckScopeRegistry,
       private resourceRegistry: ResourceRegistry, private isCore: boolean,
-      private resourceLoader: ResourceLoader, private rootDirs: ReadonlyArray<string>,
-      private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean,
-      private enableI18nLegacyMessageIdFormat: boolean, private usePoisonedData: boolean,
-      private i18nNormalizeLineEndingsInICUs: boolean, private moduleResolver: ModuleResolver,
-      private cycleAnalyzer: CycleAnalyzer, private cycleHandlingStrategy: CycleHandlingStrategy,
-      private refEmitter: ReferenceEmitter, private depTracker: DependencyTracker|null,
+      private strictCtorDeps: boolean, private resourceLoader: ResourceLoader,
+      private rootDirs: ReadonlyArray<string>, private defaultPreserveWhitespaces: boolean,
+      private i18nUseExternalIds: boolean, private enableI18nLegacyMessageIdFormat: boolean,
+      private usePoisonedData: boolean, private i18nNormalizeLineEndingsInICUs: boolean,
+      private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer,
+      private cycleHandlingStrategy: CycleHandlingStrategy, private refEmitter: ReferenceEmitter,
+      private depTracker: DependencyTracker|null,
       private injectableRegistry: InjectableClassRegistry,
       private semanticDepGraphUpdater: SemanticDepGraphUpdater|null,
       private annotateForClosureCompiler: boolean, private perf: PerfRecorder) {}
@@ -477,7 +478,9 @@ export class ComponentDecoratorHandler implements
     });
 
     this.resourceRegistry.registerResources(analysis.resources, node);
-    this.injectableRegistry.registerInjectable(node);
+    this.injectableRegistry.registerInjectable(node, {
+      ctorDeps: analysis.meta.deps,
+    });
   }
 
   index(
@@ -814,7 +817,8 @@ export class ComponentDecoratorHandler implements
     }
 
     const directiveDiagnostics = getDirectiveDiagnostics(
-        node, this.metaReader, this.evaluator, this.reflector, this.scopeRegistry, 'Component');
+        node, this.injectableRegistry, this.evaluator, this.reflector, this.scopeRegistry,
+        this.strictCtorDeps, 'Component');
     if (directiveDiagnostics !== null) {
       diagnostics.push(...directiveDiagnostics);
     }