Skip to content

Commit bc54687

Browse files
JoostKthePunderWoman
JoostK
authored and
thePunderWoman
committed
fix(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. 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
1 parent 48b354a commit bc54687

File tree

23 files changed

+634
-113
lines changed

23 files changed

+634
-113
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ export enum ErrorCode {
4343
IMPORT_CYCLE_DETECTED = 3003,
4444
IMPORT_GENERATION_FAILURE = 3004,
4545
INJECTABLE_DUPLICATE_PROV = 9001,
46+
INJECTABLE_INHERITS_INVALID_CONSTRUCTOR = 2016,
4647
INLINE_TCB_REQUIRED = 8900,
4748
INLINE_TYPE_CTOR_REQUIRED = 8901,
4849
INVALID_BANANA_IN_BOX = 8101,

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,13 @@ import ts from 'typescript';
1010

1111
import {ParsedConfiguration} from '../../..';
1212
import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader} from '../../../src/ngtsc/annotations';
13+
import {InjectableClassRegistry} from '../../../src/ngtsc/annotations/common';
1314
import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../../src/ngtsc/cycles';
1415
import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics';
1516
import {absoluteFromSourceFile, LogicalFileSystem, ReadonlyFileSystem} from '../../../src/ngtsc/file_system';
1617
import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, PrivateExportAliasingHost, Reexport, ReferenceEmitter} from '../../../src/ngtsc/imports';
1718
import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph';
18-
import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, HostDirectivesResolver, InjectableClassRegistry, LocalMetadataRegistry, ResourceRegistry} from '../../../src/ngtsc/metadata';
19+
import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, HostDirectivesResolver, LocalMetadataRegistry, ResourceRegistry} from '../../../src/ngtsc/metadata';
1920
import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
2021
import {NOOP_PERF_RECORDER} from '../../../src/ngtsc/perf';
2122
import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver, TypeCheckScopeRegistry} from '../../../src/ngtsc/scope';
@@ -97,16 +98,17 @@ export class DecorationAnalyzer {
9798
new PartialEvaluator(this.reflectionHost, this.typeChecker, /* dependencyTracker */ null);
9899
importGraph = new ImportGraph(this.typeChecker, NOOP_PERF_RECORDER);
99100
cycleAnalyzer = new CycleAnalyzer(this.importGraph);
100-
injectableRegistry = new InjectableClassRegistry(this.reflectionHost);
101+
injectableRegistry = new InjectableClassRegistry(this.reflectionHost, this.isCore);
101102
hostDirectivesResolver = new HostDirectivesResolver(this.fullMetaReader);
102103
typeCheckScopeRegistry = new TypeCheckScopeRegistry(
103104
this.scopeRegistry, this.fullMetaReader, this.hostDirectivesResolver);
104105
handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[] = [
105106
new ComponentDecoratorHandler(
106107
this.reflectionHost, this.evaluator, this.fullRegistry, this.fullMetaReader,
107108
this.scopeRegistry, this.dtsModuleScopeResolver, this.scopeRegistry,
108-
this.typeCheckScopeRegistry, new ResourceRegistry(), this.isCore, this.resourceManager,
109-
this.rootDirs, !!this.compilerOptions.preserveWhitespaces,
109+
this.typeCheckScopeRegistry, new ResourceRegistry(), this.isCore,
110+
/* strictCtorDeps */ false, this.resourceManager, this.rootDirs,
111+
!!this.compilerOptions.preserveWhitespaces,
110112
/* i18nUseExternalIds */ true, this.bundle.enableI18nLegacyMessageIdFormat,
111113
/* usePoisonedData */ false,
112114
/* i18nNormalizeLineEndingsInICUs */ false, this.moduleResolver, this.cycleAnalyzer,
@@ -119,7 +121,7 @@ export class DecorationAnalyzer {
119121
// clang-format off
120122
new DirectiveDecoratorHandler(
121123
this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
122-
this.fullMetaReader, this.injectableRegistry, this.refEmitter, this.isCore,
124+
this.fullMetaReader, this.injectableRegistry, this.refEmitter, this.isCore, /* strictCtorDeps */ false,
123125
/* semanticDepGraphUpdater */ null,
124126
!!this.compilerOptions.annotateForClosureCompiler,
125127
// In ngcc we want to compile undecorated classes with Angular features. As of
@@ -136,7 +138,7 @@ export class DecorationAnalyzer {
136138
this.reflectionHost, this.evaluator, this.metaRegistry, this.scopeRegistry,
137139
this.injectableRegistry, this.isCore, NOOP_PERF_RECORDER),
138140
new InjectableDecoratorHandler(
139-
this.reflectionHost, this.isCore,
141+
this.reflectionHost, this.evaluator, this.isCore,
140142
/* strictCtorDeps */ false, this.injectableRegistry, NOOP_PERF_RECORDER,
141143
/* errorOnDuplicateProv */ false),
142144
new NgModuleDecoratorHandler(

‎packages/compiler-cli/src/ngtsc/annotations/common/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export * from './src/di';
1111
export * from './src/diagnostics';
1212
export * from './src/evaluation';
1313
export * from './src/factory';
14+
export * from './src/injectable_registry';
1415
export * from './src/metadata';
1516
export * from './src/references_registry';
1617
export * from './src/schema';

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

Lines changed: 92 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,14 @@ import ts from 'typescript';
1010

1111
import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics';
1212
import {Reference} from '../../../imports';
13-
import {HostDirectiveMeta, InjectableClassRegistry, MetadataReader} from '../../../metadata';
13+
import {HostDirectiveMeta, MetadataReader} from '../../../metadata';
1414
import {describeResolvedType, DynamicValue, PartialEvaluator, ResolvedValue, traceDynamicValue} from '../../../partial_evaluator';
1515
import {ClassDeclaration, ReflectionHost} from '../../../reflection';
1616
import {DeclarationData, LocalModuleScopeRegistry} from '../../../scope';
1717
import {identifierOfNode} from '../../../util/src/typescript';
1818

19-
import {readBaseClass} from './util';
19+
import {InjectableClassRegistry} from './injectable_registry';
20+
import {isAbstractClassDeclaration, readBaseClass} from './util';
2021

2122

2223
/**
@@ -102,7 +103,10 @@ export function getProviderDiagnostics(
102103
const diagnostics: ts.Diagnostic[] = [];
103104

104105
for (const provider of providerClasses) {
105-
if (registry.isInjectable(provider.node)) {
106+
const injectableMeta = registry.getInjectableMeta(provider.node);
107+
if (injectableMeta !== null) {
108+
// The provided type is recognized as injectable, so we don't report a diagnostic for this
109+
// provider.
106110
continue;
107111
}
108112

@@ -124,9 +128,9 @@ Either add the @Injectable() decorator to '${
124128
}
125129

126130
export function getDirectiveDiagnostics(
127-
node: ClassDeclaration, reader: MetadataReader, evaluator: PartialEvaluator,
128-
reflector: ReflectionHost, scopeRegistry: LocalModuleScopeRegistry,
129-
kind: string): ts.Diagnostic[]|null {
131+
node: ClassDeclaration, injectableRegistry: InjectableClassRegistry,
132+
evaluator: PartialEvaluator, reflector: ReflectionHost, scopeRegistry: LocalModuleScopeRegistry,
133+
strictInjectionParameters: boolean, kind: 'Directive'|'Component'): ts.Diagnostic[]|null {
130134
let diagnostics: ts.Diagnostic[]|null = [];
131135

132136
const addDiagnostics = (more: ts.Diagnostic|ts.Diagnostic[]|null) => {
@@ -147,7 +151,8 @@ export function getDirectiveDiagnostics(
147151
addDiagnostics(makeDuplicateDeclarationError(node, duplicateDeclarations, kind));
148152
}
149153

150-
addDiagnostics(checkInheritanceOfDirective(node, reader, reflector, evaluator));
154+
addDiagnostics(checkInheritanceOfInjectable(
155+
node, injectableRegistry, reflector, evaluator, strictInjectionParameters, kind));
151156
return diagnostics;
152157
}
153158

@@ -192,9 +197,42 @@ export function getUndecoratedClassWithAngularFeaturesDiagnostic(node: ClassDecl
192197
`Angular decorator.`);
193198
}
194199

195-
export function checkInheritanceOfDirective(
196-
node: ClassDeclaration, reader: MetadataReader, reflector: ReflectionHost,
197-
evaluator: PartialEvaluator): ts.Diagnostic|null {
200+
export function checkInheritanceOfInjectable(
201+
node: ClassDeclaration, injectableRegistry: InjectableClassRegistry, reflector: ReflectionHost,
202+
evaluator: PartialEvaluator, strictInjectionParameters: boolean,
203+
kind: 'Directive'|'Component'|'Pipe'|'Injectable'): ts.Diagnostic|null {
204+
const classWithCtor = findInheritedCtor(node, injectableRegistry, reflector, evaluator);
205+
if (classWithCtor === null || classWithCtor.isCtorValid) {
206+
// The class does not inherit a constructor, or the inherited constructor is compatible
207+
// with DI; no need to report a diagnostic.
208+
return null;
209+
}
210+
211+
if (!classWithCtor.isDecorated) {
212+
// The inherited constructor exists in a class that does not have an Angular decorator.
213+
// This is an error, as there won't be a factory definition available for DI to invoke
214+
// the constructor.
215+
return getInheritedUndecoratedCtorDiagnostic(node, classWithCtor.ref, kind);
216+
}
217+
218+
if (!strictInjectionParameters || isAbstractClassDeclaration(node)) {
219+
// An invalid constructor is only reported as error under `strictInjectionParameters` and
220+
// only for concrete classes; follow the same exclusions for derived types.
221+
return null;
222+
}
223+
224+
return getInheritedInvalidCtorDiagnostic(node, classWithCtor.ref, kind);
225+
}
226+
227+
interface ClassWithCtor {
228+
ref: Reference<ClassDeclaration>;
229+
isCtorValid: boolean;
230+
isDecorated: boolean;
231+
}
232+
233+
export function findInheritedCtor(
234+
node: ClassDeclaration, injectableRegistry: InjectableClassRegistry, reflector: ReflectionHost,
235+
evaluator: PartialEvaluator): ClassWithCtor|null {
198236
if (!reflector.isClass(node) || reflector.getConstructorParameters(node) !== null) {
199237
// We should skip nodes that aren't classes. If a constructor exists, then no base class
200238
// definition is required on the runtime side - it's legal to inherit from any class.
@@ -211,44 +249,64 @@ export function checkInheritanceOfDirective(
211249
return null;
212250
}
213251

214-
// We can skip the base class if it has metadata.
215-
const baseClassMeta = reader.getDirectiveMetadata(baseClass);
216-
if (baseClassMeta !== null) {
217-
return null;
218-
}
219-
220-
// If the base class has a blank constructor we can skip it since it can't be using DI.
221-
const baseClassConstructorParams = reflector.getConstructorParameters(baseClass.node);
222-
const newParentClass = readBaseClass(baseClass.node, reflector, evaluator);
223-
224-
if (baseClassConstructorParams !== null && baseClassConstructorParams.length > 0) {
225-
// This class has a non-trivial constructor, that's an error!
226-
return getInheritedUndecoratedCtorDiagnostic(node, baseClass, reader);
227-
} else if (baseClassConstructorParams !== null || newParentClass === null) {
228-
// This class has a trivial constructor, or no constructor + is the
229-
// top of the inheritance chain, so it's okay.
230-
return null;
252+
const injectableMeta = injectableRegistry.getInjectableMeta(baseClass.node);
253+
if (injectableMeta !== null) {
254+
if (injectableMeta.ctorDeps !== null) {
255+
// The class has an Angular decorator with a constructor.
256+
return {
257+
ref: baseClass,
258+
isCtorValid: injectableMeta.ctorDeps !== 'invalid',
259+
isDecorated: true,
260+
};
261+
}
262+
} else {
263+
const baseClassConstructorParams = reflector.getConstructorParameters(baseClass.node);
264+
if (baseClassConstructorParams !== null) {
265+
// The class is not decorated, but it does have constructor. An undecorated class is only
266+
// allowed to have a constructor without parameters, otherwise it is invalid.
267+
return {
268+
ref: baseClass,
269+
isCtorValid: baseClassConstructorParams.length === 0,
270+
isDecorated: false,
271+
};
272+
}
231273
}
232274

233275
// Go up the chain and continue
234-
baseClass = newParentClass;
276+
baseClass = readBaseClass(baseClass.node, reflector, evaluator);
235277
}
236278

237279
return null;
238280
}
239281

282+
function getInheritedInvalidCtorDiagnostic(
283+
node: ClassDeclaration, baseClass: Reference,
284+
kind: 'Directive'|'Component'|'Pipe'|'Injectable') {
285+
const baseClassName = baseClass.debugName;
286+
287+
return makeDiagnostic(
288+
ErrorCode.INJECTABLE_INHERITS_INVALID_CONSTRUCTOR, node.name,
289+
`The ${kind.toLowerCase()} ${node.name.text} inherits its constructor from ${
290+
baseClassName}, ` +
291+
`but the latter has a constructor parameter that is not compatible with dependency injection. ` +
292+
`Either add an explicit constructor to ${node.name.text} or change ${
293+
baseClassName}'s constructor to ` +
294+
`use parameters that are valid for DI.`);
295+
}
296+
240297
function getInheritedUndecoratedCtorDiagnostic(
241-
node: ClassDeclaration, baseClass: Reference, reader: MetadataReader) {
242-
const subclassMeta = reader.getDirectiveMetadata(new Reference(node))!;
243-
const dirOrComp = subclassMeta.isComponent ? 'Component' : 'Directive';
298+
node: ClassDeclaration, baseClass: Reference,
299+
kind: 'Directive'|'Component'|'Pipe'|'Injectable') {
244300
const baseClassName = baseClass.debugName;
301+
const baseNeedsDecorator =
302+
kind === 'Component' || kind === 'Directive' ? 'Directive' : 'Injectable';
245303

246304
return makeDiagnostic(
247305
ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR, node.name,
248-
`The ${dirOrComp.toLowerCase()} ${node.name.text} inherits its constructor from ${
306+
`The ${kind.toLowerCase()} ${node.name.text} inherits its constructor from ${
249307
baseClassName}, ` +
250308
`but the latter does not have an Angular decorator of its own. Dependency injection will not be able to ` +
251-
`resolve the parameters of ${
252-
baseClassName}'s constructor. Either add a @Directive decorator ` +
309+
`resolve the parameters of ${baseClassName}'s constructor. Either add a @${
310+
baseNeedsDecorator} decorator ` +
253311
`to ${baseClassName}, or add an explicit constructor to ${node.name.text}.`);
254312
}

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
import {R3DependencyMetadata} from '@angular/compiler';
9+
10+
import {hasInjectableFields} from '../../../metadata';
11+
import {ClassDeclaration, ReflectionHost} from '../../../reflection';
12+
13+
import {getConstructorDependencies, unwrapConstructorDependencies} from './di';
14+
15+
16+
export interface InjectableMeta {
17+
ctorDeps: R3DependencyMetadata[]|'invalid'|null;
18+
}
19+
20+
/**
21+
* Registry that keeps track of classes that can be constructed via dependency injection (e.g.
22+
* injectables, directives, pipes).
23+
*/
24+
export class InjectableClassRegistry {
25+
private classes = new Map<ClassDeclaration, InjectableMeta>();
26+
27+
constructor(private host: ReflectionHost, private isCore: boolean) {}
28+
29+
registerInjectable(declaration: ClassDeclaration, meta: InjectableMeta): void {
30+
this.classes.set(declaration, meta);
31+
}
32+
33+
getInjectableMeta(declaration: ClassDeclaration): InjectableMeta|null {
34+
// Figure out whether the class is injectable based on the registered classes, otherwise
35+
// fall back to looking at its members since we might not have been able to register the class
36+
// if it was compiled in another compilation unit.
37+
if (this.classes.has(declaration)) {
38+
return this.classes.get(declaration)!;
39+
}
40+
41+
if (!hasInjectableFields(declaration, this.host)) {
42+
return null;
43+
}
44+
45+
const ctorDeps = getConstructorDependencies(declaration, this.host, this.isCore);
46+
const meta: InjectableMeta = {
47+
ctorDeps: unwrapConstructorDependencies(ctorDeps),
48+
};
49+
this.classes.set(declaration, meta);
50+
return meta;
51+
}
52+
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,3 +400,8 @@ export function getOriginNodeForDiagnostics(
400400
return container;
401401
}
402402
}
403+
404+
export function isAbstractClassDeclaration(clazz: ClassDeclaration): boolean {
405+
return clazz.modifiers !== undefined &&
406+
clazz.modifiers.some(mod => mod.kind === ts.SyntaxKind.AbstractKeyword);
407+
}

0 commit comments

Comments
 (0)