fix(compiler-cli): exclude abstract classes from `strictInjectionPara…

…meters` 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.

BREAKING CHANGE: Invalid constructors for DI may now report compilation errors

When a class inherits its constructor from a base class, the compiler may now
report an error when that constructor cannot be used for DI purposes. This may
either be because the base class is missing an Angular decorator such as
`@Injectable()` or `@Directive()`, or because the constructor contains parameters
which do not have an associated token (such as primitive types like `string`).
These situations used to behave unexpectedly at runtime, where the class may be
constructed without any of its constructor parameters, so this is now reported
as an error during compilation.

Any new errors that may be reported because of this change can be resolved either
by decorating the base class from which the constructor is inherited, or by adding
an explicit constructor to the class for which the error is reported.

Closes #37914

PR Close #44615

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,

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(

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,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}.`);
 }

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;
+  }
+}

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);
+}