diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index c08d1d2be4033..ca622ccbff6be 100644 --- 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 index a5a20fce19ff0..7f017c6d96a9c 100644 --- 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,7 +98,7 @@ 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); @@ -105,8 +106,9 @@ export class DecorationAnalyzer { 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 index f6f6ba4deea3a..31c9461ece6f8 100644 --- 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 index 8eec9fbd7abf2..9aa5ee6aef93d 100644 --- 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; + 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 new file mode 100644 index 0000000000000..0d1a94ab2aabb --- /dev/null +++ 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(); + + 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 index d6bc9af79c108..b6c1982493e8a 100644 --- 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); +} diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts index 50058c9692fec..bd41257a64be3 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/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, HostDirectivesResolver, InjectableClassRegistry, MatchSource, MetadataReader, MetadataRegistry, MetaKind, PipeMeta, ResourceRegistry} from '../../../metadata'; +import {DirectiveMeta, extractDirectiveTypeCheckMeta, HostDirectivesResolver, MatchSource, 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 {TypeCheckableDirectiveMeta, TypeCheckContext} from '../../../typecheck/a import {ExtendedTemplateChecker} from '../../../typecheck/extended/api'; import {getSourceFile} from '../../../util/src/typescript'; import {Xi18nContext} from '../../../xi18n'; -import {combineResolvers, compileDeclareFactory, compileNgFactoryDefField, compileResults, extractClassMetadata, extractSchemas, findAngularDecorator, forwardRefResolver, getDirectiveDiagnostics, getProviderDiagnostics, isExpressionForwardReference, readBaseClass, resolveEnumValue, resolveImportedFile, resolveLiteral, resolveProvidersRequiringFactory, ResourceLoader, toFactoryMetadata, validateHostDirectives, wrapFunctionExpressionsInParens,} from '../../common'; +import {combineResolvers, compileDeclareFactory, compileNgFactoryDefField, compileResults, extractClassMetadata, extractSchemas, findAngularDecorator, forwardRefResolver, getDirectiveDiagnostics, getProviderDiagnostics, InjectableClassRegistry, isExpressionForwardReference, readBaseClass, resolveEnumValue, resolveImportedFile, resolveLiteral, resolveProvidersRequiringFactory, ResourceLoader, toFactoryMetadata, validateHostDirectives, wrapFunctionExpressionsInParens,} from '../../common'; import {extractDirectiveMetadata, parseFieldArrayValue} from '../../directive'; import {createModuleWithProvidersResolver, 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, - 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, 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, @@ -494,7 +495,9 @@ export class ComponentDecoratorHandler implements }); this.resourceRegistry.registerResources(analysis.resources, node); - this.injectableRegistry.registerInjectable(node); + this.injectableRegistry.registerInjectable(node, { + ctorDeps: analysis.meta.deps, + }); } index( @@ -848,7 +851,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); } diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/component/test/component_spec.ts index 081423b4bf18d..1b4b9244b0007 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/test/component_spec.ts @@ -14,13 +14,13 @@ import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics'; import {absoluteFrom} from '../../../file_system'; import {runInEachFileSystem} from '../../../file_system/testing'; import {ModuleResolver, Reference, ReferenceEmitter} from '../../../imports'; -import {CompoundMetadataReader, DtsMetadataReader, HostDirectivesResolver, InjectableClassRegistry, LocalMetadataRegistry, ResourceRegistry} from '../../../metadata'; +import {CompoundMetadataReader, DtsMetadataReader, HostDirectivesResolver, LocalMetadataRegistry, ResourceRegistry} from '../../../metadata'; import {PartialEvaluator} from '../../../partial_evaluator'; import {NOOP_PERF_RECORDER} from '../../../perf'; import {isNamedClassDeclaration, TypeScriptReflectionHost} from '../../../reflection'; import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver, TypeCheckScopeRegistry} from '../../../scope'; import {getDeclaration, makeProgram} from '../../../testing'; -import {ResourceLoader, ResourceLoaderContext} from '../../common'; +import {InjectableClassRegistry, ResourceLoader, ResourceLoaderContext} from '../../common'; import {ComponentDecoratorHandler} from '../src/handler'; export class StubResourceLoader implements ResourceLoader { @@ -55,7 +55,7 @@ function setup(program: ts.Program, options: ts.CompilerOptions, host: ts.Compil const scopeRegistry = new LocalModuleScopeRegistry( metaRegistry, metaReader, dtsResolver, new ReferenceEmitter([]), null); const refEmitter = new ReferenceEmitter([]); - const injectableRegistry = new InjectableClassRegistry(reflectionHost); + const injectableRegistry = new InjectableClassRegistry(reflectionHost, /* isCore */ false); const resourceRegistry = new ResourceRegistry(); const hostDirectivesResolver = new HostDirectivesResolver(metaReader); const typeCheckScopeRegistry = @@ -73,6 +73,7 @@ function setup(program: ts.Program, options: ts.CompilerOptions, host: ts.Compil typeCheckScopeRegistry, resourceRegistry, /* isCore */ false, + /* strictCtorDeps */ false, resourceLoader, /* rootDirs */['/'], /* defaultPreserveWhitespaces */ false, diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts index e7498d96bbc24..6b7806b7cccf5 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts @@ -11,13 +11,13 @@ import ts from 'typescript'; import {Reference, ReferenceEmitter} from '../../../imports'; import {extractSemanticTypeParameters, SemanticDepGraphUpdater} from '../../../incremental/semantic_graph'; -import {ClassPropertyMapping, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, HostDirectiveMeta, InjectableClassRegistry, MatchSource, MetadataReader, MetadataRegistry, MetaKind} from '../../../metadata'; +import {ClassPropertyMapping, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, HostDirectiveMeta, MatchSource, MetadataReader, MetadataRegistry, MetaKind} from '../../../metadata'; import {PartialEvaluator} from '../../../partial_evaluator'; import {PerfEvent, PerfRecorder} from '../../../perf'; import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, ReflectionHost} from '../../../reflection'; import {LocalModuleScopeRegistry} from '../../../scope'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence, ResolveResult} from '../../../transform'; -import {compileDeclareFactory, compileNgFactoryDefField, compileResults, extractClassMetadata, findAngularDecorator, getDirectiveDiagnostics, getProviderDiagnostics, getUndecoratedClassWithAngularFeaturesDiagnostic, isAngularDecorator, readBaseClass, resolveProvidersRequiringFactory, toFactoryMetadata, validateHostDirectives} from '../../common'; +import {compileDeclareFactory, compileNgFactoryDefField, compileResults, extractClassMetadata, findAngularDecorator, getDirectiveDiagnostics, getProviderDiagnostics, getUndecoratedClassWithAngularFeaturesDiagnostic, InjectableClassRegistry, isAngularDecorator, readBaseClass, resolveProvidersRequiringFactory, toFactoryMetadata, validateHostDirectives} from '../../common'; import {extractDirectiveMetadata} from './shared'; import {DirectiveSymbol} from './symbol'; @@ -53,6 +53,7 @@ export class DirectiveDecoratorHandler implements private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry, private metaReader: MetadataReader, private injectableRegistry: InjectableClassRegistry, private refEmitter: ReferenceEmitter, private isCore: boolean, + private strictCtorDeps: boolean, private semanticDepGraphUpdater: SemanticDepGraphUpdater|null, private annotateForClosureCompiler: boolean, private compileUndecoratedClassesWithAngularFeatures: boolean, private perf: PerfRecorder) {} @@ -161,7 +162,9 @@ export class DirectiveDecoratorHandler implements decorator: analysis.decorator, }); - this.injectableRegistry.registerInjectable(node); + this.injectableRegistry.registerInjectable(node, { + ctorDeps: analysis.meta.deps, + }); } resolve(node: ClassDeclaration, analysis: DirectiveHandlerData, symbol: DirectiveSymbol): @@ -180,7 +183,8 @@ export class DirectiveDecoratorHandler implements } const directiveDiagnostics = getDirectiveDiagnostics( - node, this.metaReader, this.evaluator, this.reflector, this.scopeRegistry, 'Directive'); + node, this.injectableRegistry, this.evaluator, this.reflector, this.scopeRegistry, + this.strictCtorDeps, 'Directive'); if (directiveDiagnostics !== null) { diagnostics.push(...directiveDiagnostics); } diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/annotations/directive/test/BUILD.bazel index c4bff20194e98..a1a27f243d16e 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/directive/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/annotations/directive/test/BUILD.bazel @@ -11,6 +11,7 @@ ts_library( deps = [ "//packages:types", "//packages/compiler", + "//packages/compiler-cli/src/ngtsc/annotations/common", "//packages/compiler-cli/src/ngtsc/annotations/directive", "//packages/compiler-cli/src/ngtsc/file_system", "//packages/compiler-cli/src/ngtsc/file_system/testing", diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/test/directive_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/test/directive_spec.ts index 0da42f349ceaa..8d4b6877cbaf3 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/directive/test/directive_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/directive/test/directive_spec.ts @@ -11,12 +11,13 @@ import ts from 'typescript'; import {absoluteFrom} from '../../../file_system'; import {runInEachFileSystem} from '../../../file_system/testing'; import {ReferenceEmitter} from '../../../imports'; -import {CompoundMetadataReader, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry} from '../../../metadata'; +import {CompoundMetadataReader, DtsMetadataReader, LocalMetadataRegistry} from '../../../metadata'; import {PartialEvaluator} from '../../../partial_evaluator'; import {NOOP_PERF_RECORDER} from '../../../perf'; import {ClassDeclaration, isNamedClassDeclaration, TypeScriptReflectionHost} from '../../../reflection'; import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../../scope'; import {getDeclaration, makeProgram} from '../../../testing'; +import {InjectableClassRegistry} from '../../common'; import {DirectiveDecoratorHandler} from '../index'; runInEachFileSystem(() => { @@ -168,11 +169,12 @@ runInEachFileSystem(() => { const scopeRegistry = new LocalModuleScopeRegistry( metaReader, new CompoundMetadataReader([metaReader, dtsReader]), new MetadataDtsModuleScopeResolver(dtsReader, null), refEmitter, null); - const injectableRegistry = new InjectableClassRegistry(reflectionHost); + const injectableRegistry = new InjectableClassRegistry(reflectionHost, /* isCore */ false); const handler = new DirectiveDecoratorHandler( reflectionHost, evaluator, scopeRegistry, scopeRegistry, metaReader, injectableRegistry, refEmitter, /*isCore*/ false, + /*strictCtorDeps*/ false, /*semanticDepGraphUpdater*/ null, /*annotateForClosureCompiler*/ false, /*detectUndecoratedClassesWithAngularFeatures*/ false, NOOP_PERF_RECORDER); diff --git a/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts index a41961566945e..bff2b5fb7de18 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts @@ -12,7 +12,7 @@ import ts from 'typescript'; import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics'; import {assertSuccessfulReferenceEmit, Reference, ReferenceEmitter} from '../../../imports'; import {isArrayEqual, isReferenceEqual, isSymbolEqual, SemanticReference, SemanticSymbol} from '../../../incremental/semantic_graph'; -import {InjectableClassRegistry, MetadataReader, MetadataRegistry, MetaKind} from '../../../metadata'; +import {MetadataReader, MetadataRegistry, MetaKind} from '../../../metadata'; import {PartialEvaluator, ResolvedValue, SyntheticValue} from '../../../partial_evaluator'; import {PerfEvent, PerfRecorder} from '../../../perf'; import {ClassDeclaration, DeclarationNode, Decorator, isNamedClassDeclaration, ReflectionHost, reflectObjectLiteral} from '../../../reflection'; @@ -21,7 +21,7 @@ import {getDiagnosticNode} from '../../../scope/src/util'; import {FactoryTracker} from '../../../shims/api'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../../transform'; import {getSourceFile} from '../../../util/src/typescript'; -import {combineResolvers, compileDeclareFactory, compileNgFactoryDefField, createValueHasWrongTypeError, extractClassMetadata, extractSchemas, findAngularDecorator, forwardRefResolver, getProviderDiagnostics, getValidConstructorDependencies, isExpressionForwardReference, ReferencesRegistry, resolveProvidersRequiringFactory, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens, wrapTypeReference} from '../../common'; +import {combineResolvers, compileDeclareFactory, compileNgFactoryDefField, createValueHasWrongTypeError, extractClassMetadata, extractSchemas, findAngularDecorator, forwardRefResolver, getProviderDiagnostics, getValidConstructorDependencies, InjectableClassRegistry, isExpressionForwardReference, ReferencesRegistry, resolveProvidersRequiringFactory, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens, wrapTypeReference} from '../../common'; import {createModuleWithProvidersResolver, isResolvedModuleWithProviders} from './module_with_providers'; @@ -475,7 +475,9 @@ export class NgModuleDecoratorHandler implements }); } - this.injectableRegistry.registerInjectable(node); + this.injectableRegistry.registerInjectable(node, { + ctorDeps: analysis.fac.deps, + }); } resolve(node: ClassDeclaration, analysis: Readonly): diff --git a/packages/compiler-cli/src/ngtsc/annotations/ng_module/test/ng_module_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/ng_module/test/ng_module_spec.ts index 0af3c2f3fd1ef..19dbd88583750 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/ng_module/test/ng_module_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/ng_module/test/ng_module_spec.ts @@ -12,13 +12,13 @@ import ts from 'typescript'; import {absoluteFrom} from '../../../file_system'; import {runInEachFileSystem} from '../../../file_system/testing'; import {LocalIdentifierStrategy, ReferenceEmitter} from '../../../imports'; -import {CompoundMetadataReader, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry} from '../../../metadata'; +import {CompoundMetadataReader, DtsMetadataReader, LocalMetadataRegistry} from '../../../metadata'; import {PartialEvaluator} from '../../../partial_evaluator'; import {NOOP_PERF_RECORDER} from '../../../perf'; import {isNamedClassDeclaration, TypeScriptReflectionHost} from '../../../reflection'; import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../../scope'; import {getDeclaration, makeProgram} from '../../../testing'; -import {NoopReferencesRegistry} from '../../common'; +import {InjectableClassRegistry, NoopReferencesRegistry} from '../../common'; import {NgModuleDecoratorHandler} from '../src/handler'; runInEachFileSystem(() => { @@ -67,7 +67,7 @@ runInEachFileSystem(() => { metaRegistry, metaReader, new MetadataDtsModuleScopeResolver(dtsReader, null), new ReferenceEmitter([]), null); const refEmitter = new ReferenceEmitter([new LocalIdentifierStrategy()]); - const injectableRegistry = new InjectableClassRegistry(reflectionHost); + const injectableRegistry = new InjectableClassRegistry(reflectionHost, /* isCore */ false); const handler = new NgModuleDecoratorHandler( reflectionHost, evaluator, metaReader, metaRegistry, scopeRegistry, referencesRegistry, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts index 0d63dcbdb3766..722ddde37687d 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts @@ -9,12 +9,13 @@ import {compileClassMetadata, CompileClassMetadataFn, compileDeclareClassMetadata, compileDeclareInjectableFromMetadata, compileInjectable, createMayBeForwardRefExpression, FactoryTarget, ForwardRefHandling, LiteralExpr, MaybeForwardRefExpression, R3ClassMetadata, R3CompiledExpression, R3DependencyMetadata, R3InjectableMetadata, WrappedNodeExpr} from '@angular/compiler'; import ts from 'typescript'; +import {InjectableClassRegistry, isAbstractClassDeclaration} from '../../annotations/common'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; -import {InjectableClassRegistry} from '../../metadata'; +import {PartialEvaluator} from '../../partial_evaluator'; import {PerfEvent, PerfRecorder} from '../../perf'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; -import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; -import {compileDeclareFactory, CompileFactoryFn, compileNgFactoryDefField, extractClassMetadata, findAngularDecorator, getConstructorDependencies, getValidConstructorDependencies, isAngularCore, toFactoryMetadata, tryUnwrapForwardRef, unwrapConstructorDependencies, validateConstructorDependencies, wrapTypeReference} from '../common'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform'; +import {checkInheritanceOfInjectable, compileDeclareFactory, CompileFactoryFn, compileNgFactoryDefField, extractClassMetadata, findAngularDecorator, getConstructorDependencies, getValidConstructorDependencies, isAngularCore, toFactoryMetadata, tryUnwrapForwardRef, unwrapConstructorDependencies, validateConstructorDependencies, wrapTypeReference,} from '../common'; export interface InjectableHandlerData { meta: R3InjectableMetadata; @@ -29,7 +30,8 @@ export interface InjectableHandlerData { export class InjectableDecoratorHandler implements DecoratorHandler { constructor( - private reflector: ReflectionHost, private isCore: boolean, private strictCtorDeps: boolean, + private reflector: ReflectionHost, private evaluator: PartialEvaluator, + private isCore: boolean, private strictCtorDeps: boolean, private injectableRegistry: InjectableClassRegistry, private perf: PerfRecorder, /** * What to do if the injectable already contains a ɵprov property. @@ -83,8 +85,26 @@ export class InjectableDecoratorHandler implements return null; } - register(node: ClassDeclaration): void { - this.injectableRegistry.registerInjectable(node); + register(node: ClassDeclaration, analysis: InjectableHandlerData): void { + this.injectableRegistry.registerInjectable(node, { + ctorDeps: analysis.ctorDeps, + }); + } + + resolve(node: ClassDeclaration, analysis: Readonly, symbol: null): + ResolveResult { + if (requiresValidCtor(analysis.meta)) { + const diagnostic = checkInheritanceOfInjectable( + node, this.injectableRegistry, this.reflector, this.evaluator, this.strictCtorDeps, + 'Injectable'); + if (diagnostic !== null) { + return { + diagnostics: [diagnostic], + }; + } + } + + return {}; } compileFull(node: ClassDeclaration, analysis: Readonly): CompileResult[] { @@ -244,7 +264,7 @@ function extractInjectableCtorDeps( // To deal with this, @Injectable() without an argument is more lenient, and if the // constructor signature does not work for DI then a factory definition (ɵfac) that throws is // generated. - if (strictCtorDeps) { + if (strictCtorDeps && !isAbstractClassDeclaration(clazz)) { ctorDeps = getValidConstructorDependencies(clazz, reflector, isCore); } else { ctorDeps = @@ -255,9 +275,9 @@ function extractInjectableCtorDeps( } else if (decorator.args.length === 1) { const rawCtorDeps = getConstructorDependencies(clazz, reflector, isCore); - if (strictCtorDeps && meta.useValue === undefined && meta.useExisting === undefined && - meta.useClass === undefined && meta.useFactory === undefined) { - // Since use* was not provided, validate the deps according to strictCtorDeps. + if (strictCtorDeps && !isAbstractClassDeclaration(clazz) && requiresValidCtor(meta)) { + // Since use* was not provided for a concrete class, validate the deps according to + // strictCtorDeps. ctorDeps = validateConstructorDependencies(clazz, rawCtorDeps); } else { ctorDeps = unwrapConstructorDependencies(rawCtorDeps); @@ -267,6 +287,11 @@ function extractInjectableCtorDeps( return ctorDeps; } +function requiresValidCtor(meta: R3InjectableMetadata): boolean { + return meta.useValue === undefined && meta.useExisting === undefined && + meta.useClass === undefined && meta.useFactory === undefined; +} + function getDep(dep: ts.Expression, reflector: ReflectionHost): R3DependencyMetadata { const meta: R3DependencyMetadata = { token: new WrappedNodeExpr(dep), diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts index 5181dc7cb0dd4..c66fece5e5a7a 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts @@ -12,13 +12,13 @@ import ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {Reference} from '../../imports'; import {SemanticSymbol} from '../../incremental/semantic_graph'; -import {InjectableClassRegistry, MetadataRegistry, MetaKind} from '../../metadata'; +import {MetadataRegistry, MetaKind} from '../../metadata'; import {PartialEvaluator} from '../../partial_evaluator'; import {PerfEvent, PerfRecorder} from '../../perf'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; import {LocalModuleScopeRegistry} from '../../scope'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform'; -import {compileDeclareFactory, compileNgFactoryDefField, compileResults, createValueHasWrongTypeError, extractClassMetadata, findAngularDecorator, getValidConstructorDependencies, makeDuplicateDeclarationError, toFactoryMetadata, unwrapExpression, wrapTypeReference} from '../common'; +import {compileDeclareFactory, compileNgFactoryDefField, compileResults, createValueHasWrongTypeError, extractClassMetadata, findAngularDecorator, getValidConstructorDependencies, InjectableClassRegistry, makeDuplicateDeclarationError, toFactoryMetadata, unwrapExpression, wrapTypeReference,} from '../common'; export interface PipeHandlerData { meta: R3PipeMetadata; @@ -164,7 +164,9 @@ export class PipeDecoratorHandler implements decorator: analysis.decorator, }); - this.injectableRegistry.registerInjectable(node); + this.injectableRegistry.registerInjectable(node, { + ctorDeps: analysis.meta.deps, + }); } resolve(node: ClassDeclaration): ResolveResult { diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/annotations/test/BUILD.bazel index bfe1897493bba..2ee2f98ddc49c 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/annotations/test/BUILD.bazel @@ -12,6 +12,7 @@ ts_library( "//packages:types", "//packages/compiler", "//packages/compiler-cli/src/ngtsc/annotations", + "//packages/compiler-cli/src/ngtsc/annotations/common", "//packages/compiler-cli/src/ngtsc/cycles", "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/file_system", diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/injectable_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/injectable_spec.ts index 901463b7e4a71..6ecd3b24ce7ba 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/injectable_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/injectable_spec.ts @@ -5,10 +5,11 @@ * 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 {InjectableClassRegistry} from '../../annotations/common'; import {ErrorCode, FatalDiagnosticError, ngErrorCode} from '../../diagnostics'; import {absoluteFrom} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; -import {InjectableClassRegistry} from '../../metadata'; +import {PartialEvaluator} from '../../partial_evaluator'; import {NOOP_PERF_RECORDER} from '../../perf'; import {isNamedClassDeclaration, TypeScriptReflectionHost} from '../../reflection'; import {getDeclaration, makeProgram} from '../../testing'; @@ -67,9 +68,10 @@ function setupHandler(errorOnDuplicateProv: boolean) { ]); const checker = program.getTypeChecker(); const reflectionHost = new TypeScriptReflectionHost(checker); - const injectableRegistry = new InjectableClassRegistry(reflectionHost); + const injectableRegistry = new InjectableClassRegistry(reflectionHost, /* isCore */ false); + const evaluator = new PartialEvaluator(reflectionHost, checker, null); const handler = new InjectableDecoratorHandler( - reflectionHost, /* isCore */ false, + reflectionHost, evaluator, /* isCore */ false, /* strictCtorDeps */ false, injectableRegistry, NOOP_PERF_RECORDER, errorOnDuplicateProv); const TestClass = getDeclaration(program, ENTRY_FILE, 'TestClass', isNamedClassDeclaration); const ɵprov = reflectionHost.getMembersOfClass(TestClass).find(member => member.name === 'ɵprov'); diff --git a/packages/compiler-cli/src/ngtsc/core/BUILD.bazel b/packages/compiler-cli/src/ngtsc/core/BUILD.bazel index 5bc84e3f1c186..7974be6e2fc4f 100644 --- a/packages/compiler-cli/src/ngtsc/core/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/core/BUILD.bazel @@ -13,6 +13,7 @@ ts_library( "//packages:types", "//packages/compiler", "//packages/compiler-cli/src/ngtsc/annotations", + "//packages/compiler-cli/src/ngtsc/annotations/common", "//packages/compiler-cli/src/ngtsc/cycles", "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/entry_point", diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 6367d9577e504..295116cec7747 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -9,6 +9,7 @@ import ts from 'typescript'; import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, NoopReferencesRegistry, PipeDecoratorHandler, ReferencesRegistry} from '../../annotations'; +import {InjectableClassRegistry} from '../../annotations/common'; import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../cycles'; import {COMPILER_ERRORS_WITH_GUIDES, ERROR_DETAILS_PAGE_BASE_URL, ErrorCode, ngErrorCode} from '../../diagnostics'; import {checkForPrivateExports, ReferenceGraph} from '../../entry_point'; @@ -17,7 +18,7 @@ import {AbsoluteModuleStrategy, AliasingHost, AliasStrategy, DefaultImportTracke import {IncrementalBuildStrategy, IncrementalCompilation, IncrementalState} from '../../incremental'; import {SemanticSymbol} from '../../incremental/semantic_graph'; import {generateAnalysis, IndexedComponent, IndexingContext} from '../../indexer'; -import {ComponentResources, CompoundMetadataReader, CompoundMetadataRegistry, DirectiveMeta, DtsMetadataReader, HostDirectivesResolver, InjectableClassRegistry, LocalMetadataRegistry, MetadataReader, MetadataReaderWithIndex, PipeMeta, ResourceRegistry} from '../../metadata'; +import {ComponentResources, CompoundMetadataReader, CompoundMetadataRegistry, DirectiveMeta, DtsMetadataReader, HostDirectivesResolver, LocalMetadataRegistry, MetadataReader, MetadataReaderWithIndex, PipeMeta, ResourceRegistry} from '../../metadata'; import {PartialEvaluator} from '../../partial_evaluator'; import {ActivePerfRecorder, DelegatingPerfRecorder, PerfCheckpoint, PerfEvent, PerfPhase} from '../../perf'; import {FileUpdate, ProgramDriver, UpdateMode} from '../../program_driver'; @@ -962,6 +963,8 @@ export class NgCompiler { aliasingHost = new UnifiedModulesAliasingHost(this.adapter.unifiedModulesHost); } + const isCore = isAngularCorePackage(this.inputProgram); + const evaluator = new PartialEvaluator(reflector, checker, this.incrementalCompilation.depGraph); const dtsReader = new DtsMetadataReader(checker, reflector); @@ -977,7 +980,7 @@ export class NgCompiler { new CompoundComponentScopeReader([ngModuleScopeRegistry, standaloneScopeReader]); const semanticDepGraphUpdater = this.incrementalCompilation.semanticDepGraphUpdater; const metaRegistry = new CompoundMetadataRegistry([localMetaRegistry, ngModuleScopeRegistry]); - const injectableRegistry = new InjectableClassRegistry(reflector); + const injectableRegistry = new InjectableClassRegistry(reflector, isCore); const hostDirectivesResolver = new HostDirectivesResolver(metaReader); const typeCheckScopeRegistry = @@ -998,8 +1001,6 @@ export class NgCompiler { const dtsTransforms = new DtsTransformRegistry(); - const isCore = isAngularCorePackage(this.inputProgram); - const resourceRegistry = new ResourceRegistry(); // Note: If this compilation builds `@angular/core`, we always build in full compilation @@ -1016,11 +1017,13 @@ export class NgCompiler { CycleHandlingStrategy.UseRemoteScoping : CycleHandlingStrategy.Error; + const strictCtorDeps = this.options.strictInjectionParameters || false; + // Set up the IvyCompilation, which manages state for the Ivy transformer. const handlers: DecoratorHandler[] = [ new ComponentDecoratorHandler( reflector, evaluator, metaRegistry, metaReader, scopeReader, depScopeReader, - ngModuleScopeRegistry, typeCheckScopeRegistry, resourceRegistry, isCore, + ngModuleScopeRegistry, typeCheckScopeRegistry, resourceRegistry, isCore, strictCtorDeps, this.resourceManager, this.adapter.rootDirs, this.options.preserveWhitespaces || false, this.options.i18nUseExternalIds !== false, this.options.enableI18nLegacyMessageIdFormat !== false, this.usePoisonedData, @@ -1034,7 +1037,7 @@ export class NgCompiler { // clang-format off new DirectiveDecoratorHandler( reflector, evaluator, metaRegistry, ngModuleScopeRegistry, metaReader, - injectableRegistry, refEmitter, isCore, semanticDepGraphUpdater, + injectableRegistry, refEmitter, isCore, strictCtorDeps, semanticDepGraphUpdater, this.closureCompilerEnabled, /** compileUndecoratedClassesWithAngularFeatures */ false, this.delegatingPerfRecorder, ) as Readonly>, @@ -1045,7 +1048,7 @@ export class NgCompiler { reflector, evaluator, metaRegistry, ngModuleScopeRegistry, injectableRegistry, isCore, this.delegatingPerfRecorder), new InjectableDecoratorHandler( - reflector, isCore, this.options.strictInjectionParameters || false, injectableRegistry, + reflector, evaluator, isCore, strictCtorDeps, injectableRegistry, this.delegatingPerfRecorder), new NgModuleDecoratorHandler( reflector, evaluator, metaReader, metaRegistry, ngModuleScopeRegistry, referencesRegistry, diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index 1c3a9cdaa5a99..545c3a861d7b4 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -86,6 +86,12 @@ export enum ErrorCode { */ HOST_DIRECTIVE_COMPONENT = 2015, + /** + * Raised when a type with Angular decorator inherits its constructor from a base class + * which has a constructor that is incompatible with Angular DI. + */ + INJECTABLE_INHERITS_INVALID_CONSTRUCTOR = 2016, + SYMBOL_NOT_EXPORTED = 3001, /** * Raised when a relationship between directives and/or pipes would cause a cyclic import to be diff --git a/packages/compiler-cli/src/ngtsc/metadata/index.ts b/packages/compiler-cli/src/ngtsc/metadata/index.ts index c1b367543f965..44d1612592733 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/index.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/index.ts @@ -9,8 +9,8 @@ export * from './src/api'; export {DtsMetadataReader} from './src/dts'; export {flattenInheritedDirectiveMetadata} from './src/inheritance'; -export {CompoundMetadataRegistry, LocalMetadataRegistry, InjectableClassRegistry} from './src/registry'; +export {CompoundMetadataRegistry, LocalMetadataRegistry} from './src/registry'; export {ResourceRegistry, Resource, ComponentResources, isExternalResource, ExternalResource} from './src/resource_registry'; -export {extractDirectiveTypeCheckMeta, CompoundMetadataReader} from './src/util'; +export {extractDirectiveTypeCheckMeta, hasInjectableFields, CompoundMetadataReader} from './src/util'; export {BindingPropertyName, ClassPropertyMapping, ClassPropertyName, InputOrOutput} from './src/property_mapping'; export {HostDirectivesResolver} from './src/host_directives_resolver'; diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts b/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts index 44d2210082741..f0d0abb92c6a6 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts @@ -7,10 +7,9 @@ */ import {Reference} from '../../imports'; -import {ClassDeclaration, ReflectionHost} from '../../reflection'; +import {ClassDeclaration} from '../../reflection'; import {DirectiveMeta, MetadataReaderWithIndex, MetadataRegistry, NgModuleMeta, PipeMeta} from './api'; -import {hasInjectableFields} from './util'; /** * A registry of directive, pipe, and module metadata for types defined in the current compilation @@ -71,24 +70,3 @@ export class CompoundMetadataRegistry implements MetadataRegistry { } } } - -/** - * Registry that keeps track of classes that can be constructed via dependency injection (e.g. - * injectables, directives, pipes). - */ -export class InjectableClassRegistry { - private classes = new Set(); - - constructor(private host: ReflectionHost) {} - - registerInjectable(declaration: ClassDeclaration): void { - this.classes.add(declaration); - } - - isInjectable(declaration: ClassDeclaration): boolean { - // 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 register the class - // if it was compiled already. - return this.classes.has(declaration) || hasInjectableFields(declaration, this.host); - } -} diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 72e2c7dc5cfaa..ec8b0643a2481 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -2732,6 +2732,234 @@ function allTests(os: string) { expect(jsContents) .toMatch(/function Test_Factory\(t\) { i0\.ɵɵinvalidFactory\(\)/ms); }); + + it('should not give a compile-time error if an invalid @Injectable without providedIn is an abstract class', + () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable() + export abstract class Test { + constructor(private notInjectable: string) {} + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents) + .toMatch(/function Test_Factory\(t\) { i0\.ɵɵinvalidFactory\(\)/ms); + }); + + it('should not give a compile-time error if an invalid @Injectable with providedIn is an abstract class', + () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable({ + providedIn: 'root', + }) + export abstract class Test { + constructor(private notInjectable: string) {} + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents) + .toMatch(/function Test_Factory\(t\) { i0\.ɵɵinvalidFactory\(\)/ms); + }); + + it('should give a compile-time error when a derived Directive inherits an invalid constructor', () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Directive} from '@angular/core'; + + @Directive() + export abstract class ParentDir { + constructor(private notInjectable: string) {} + } + + @Directive() + export abstract class AbstractMiddleDir extends ParentDir {} + + @Directive() + export class ConcreteMiddleDir extends AbstractMiddleDir {} + + @Directive() + export class ConcreteDirWithoutCtor extends ConcreteMiddleDir {} + + @Directive() + export class ConcreteDirWithCtor extends ConcreteMiddleDir { + constructor() { + super('correct'); + } + } + + @Directive() + export class ConcreteDerivedDirWithoutCtor extends ConcreteDirWithCtor {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(2); + expect(diags[0].code) + .toBe(ngErrorCode(ErrorCode.INJECTABLE_INHERITS_INVALID_CONSTRUCTOR)); + expect(diags[0].messageText) + .toEqual( + `The directive ConcreteMiddleDir inherits its constructor from ParentDir, but the latter has ` + + `a constructor parameter that is not compatible with dependency injection. Either add an explicit ` + + `constructor to ConcreteMiddleDir or change ParentDir's constructor to use parameters that are ` + + `valid for DI.`); + expect(getDiagnosticSourceCode(diags[0])).toBe('ConcreteMiddleDir'); + + expect(diags[1].code) + .toBe(ngErrorCode(ErrorCode.INJECTABLE_INHERITS_INVALID_CONSTRUCTOR)); + expect(diags[1].messageText) + .toEqual( + `The directive ConcreteDirWithoutCtor inherits its constructor from ParentDir, but the latter ` + + `has a constructor parameter that is not compatible with dependency injection. Either add an ` + + `explicit constructor to ConcreteDirWithoutCtor or change ParentDir's constructor to use ` + + `parameters that are valid for DI.`); + expect(getDiagnosticSourceCode(diags[1])).toBe('ConcreteDirWithoutCtor'); + }); + + it('should give a compile-time error when a derived Injectable inherits an invalid constructor', () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable() + export abstract class ParentService { + constructor(private notInjectable: string) {} + } + + @Injectable() + export abstract class AbstractMiddleService extends ParentService {} + + @Injectable() + export class ConcreteMiddleService extends AbstractMiddleService {} + + @Injectable() + export class ConcreteServiceWithoutCtor extends ConcreteMiddleService {} + + @Injectable() + export class ConcreteServiceWithCtor extends ConcreteMiddleService { + constructor() { + super('correct'); + } + } + + @Injectable() + export class ConcreteDerivedServiceWithoutCtor extends ConcreteServiceWithCtor {} + + @Injectable({ providedIn: 'root' }) + export class ProvidedInRootService extends ParentService {} + + @Injectable({ providedIn: 'root', useFactory: () => null }) + export class UseFactoryService extends ParentService {} + + @Injectable({ providedIn: 'root', useValue: null }) + export class UseValueService extends ParentService {} + + @Injectable({ providedIn: 'root', useClass: ConcreteServiceWithCtor }) + export class UseClassService extends ParentService {} + + @Injectable({ providedIn: 'root', useExisting: ConcreteServiceWithCtor }) + export class UseExistingService extends ParentService {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(3); + expect(diags[0].code) + .toBe(ngErrorCode(ErrorCode.INJECTABLE_INHERITS_INVALID_CONSTRUCTOR)); + expect(diags[0].messageText) + .toEqual( + `The injectable ConcreteMiddleService inherits its constructor from ParentService, but the ` + + `latter has a constructor parameter that is not compatible with dependency injection. Either add ` + + `an explicit constructor to ConcreteMiddleService or change ParentService's constructor to use ` + + `parameters that are valid for DI.`); + expect(getDiagnosticSourceCode(diags[0])).toBe('ConcreteMiddleService'); + + expect(diags[1].code) + .toBe(ngErrorCode(ErrorCode.INJECTABLE_INHERITS_INVALID_CONSTRUCTOR)); + expect(diags[1].messageText) + .toEqual( + `The injectable ConcreteServiceWithoutCtor inherits its constructor from ParentService, but the ` + + `latter has a constructor parameter that is not compatible with dependency injection. Either add ` + + `an explicit constructor to ConcreteServiceWithoutCtor or change ParentService's constructor to ` + + `use parameters that are valid for DI.`); + expect(getDiagnosticSourceCode(diags[1])).toBe('ConcreteServiceWithoutCtor'); + + expect(diags[2].code) + .toBe(ngErrorCode(ErrorCode.INJECTABLE_INHERITS_INVALID_CONSTRUCTOR)); + expect(diags[2].messageText) + .toEqual( + `The injectable ProvidedInRootService inherits its constructor from ParentService, but the ` + + `latter has a constructor parameter that is not compatible with dependency injection. Either add ` + + `an explicit constructor to ProvidedInRootService or change ParentService's constructor to use ` + + `parameters that are valid for DI.`); + expect(getDiagnosticSourceCode(diags[2])).toBe('ProvidedInRootService'); + }); + + it('should give a compile-time error when a derived Directive inherits from a non-decorated class', () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Directive} from '@angular/core'; + + export abstract class ParentClass { + constructor(private notInjectable: string) {} + } + + @Directive() + export abstract class AbstractMiddleDir extends ParentClass {} + + @Directive() + export class ConcreteMiddleDir extends AbstractMiddleDir {} + + @Directive() + export class ConcreteDirWithoutCtor extends ConcreteMiddleDir {} + + @Directive() + export class ConcreteDirWithCtor extends ConcreteMiddleDir { + constructor() { + super('correct'); + } + } + + @Directive() + export class ConcreteDerivedDirWithoutCtor extends ConcreteDirWithCtor {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(3); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR)); + expect(diags[0].messageText) + .toEqual( + `The directive AbstractMiddleDir inherits its constructor from ParentClass, but the latter ` + + `does not have an Angular decorator of its own. Dependency injection will not be able to resolve ` + + `the parameters of ParentClass's constructor. Either add a @Directive decorator to ParentClass, ` + + `or add an explicit constructor to AbstractMiddleDir.`); + expect(getDiagnosticSourceCode(diags[0])).toBe('AbstractMiddleDir'); + + expect(diags[1].code).toBe(ngErrorCode(ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR)); + expect(diags[1].messageText) + .toEqual( + `The directive ConcreteMiddleDir inherits its constructor from ParentClass, but the latter ` + + `does not have an Angular decorator of its own. Dependency injection will not be able to resolve ` + + `the parameters of ParentClass's constructor. Either add a @Directive decorator to ParentClass, or ` + + `add an explicit constructor to ConcreteMiddleDir.`); + expect(getDiagnosticSourceCode(diags[1])).toBe('ConcreteMiddleDir'); + + expect(diags[2].code).toBe(ngErrorCode(ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR)); + expect(diags[2].messageText) + .toEqual( + `The directive ConcreteDirWithoutCtor inherits its constructor from ParentClass, but the latter ` + + `does not have an Angular decorator of its own. Dependency injection will not be able to resolve ` + + `the parameters of ParentClass's constructor. Either add a @Directive decorator to ParentClass, ` + + `or add an explicit constructor to ConcreteDirWithoutCtor.`); + expect(getDiagnosticSourceCode(diags[2])).toBe('ConcreteDirWithoutCtor'); + }); }); describe('with strictInjectionParameters = false', () => { @@ -2768,6 +2996,148 @@ function allTests(os: string) { expect(jsContents) .toContain('Test.ɵfac = function Test_Factory(t) { i0.ɵɵinvalidFactory()'); }); + + it('should compile when a derived Directive inherits an invalid constructor', () => { + env.tsconfig({strictInjectionParameters: false}); + env.write('test.ts', ` + import {Directive} from '@angular/core'; + + @Directive() + export abstract class ParentDir { + constructor(private notInjectable: string) {} + } + + @Directive() + export abstract class AbstractMiddleDir extends ParentDir {} + + @Directive() + export class ConcreteMiddleDir extends AbstractMiddleDir {} + + @Directive() + export class ConcreteDirWithoutCtor extends ConcreteMiddleDir {} + + @Directive() + export class ConcreteDirWithCtor extends ConcreteMiddleDir { + constructor() { + super('correct'); + } + } + + @Directive() + export class ConcreteDerivedDirWithoutCtor extends ConcreteDirWithCtor {} + `); + + env.driveMain(); + }); + + it('should compile when a derived Injectable inherits an invalid constructor', () => { + env.tsconfig({strictInjectionParameters: false}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable() + export abstract class ParentService { + constructor(private notInjectable: string) {} + } + + @Injectable() + export abstract class AbstractMiddleService extends ParentService {} + + @Injectable() + export class ConcreteMiddleService extends AbstractMiddleService {} + + @Injectable() + export class ConcreteServiceWithoutCtor extends ConcreteMiddleService {} + + @Injectable() + export class ConcreteServiceWithCtor extends ConcreteMiddleService { + constructor() { + super('correct'); + } + } + + @Injectable() + export class ConcreteDerivedServiceWithoutCtor extends ConcreteServiceWithCtor {} + + @Injectable({ providedIn: 'root' }) + export class ProvidedInRootService extends ParentService {} + + @Injectable({ providedIn: 'root', useFactory: () => null }) + export class UseFactoryService extends ParentService {} + + @Injectable({ providedIn: 'root', useValue: null }) + export class UseValueService extends ParentService {} + + @Injectable({ providedIn: 'root', useClass: ConcreteServiceWithCtor }) + export class UseClassService extends ParentService {} + + @Injectable({ providedIn: 'root', useExisting: ConcreteServiceWithCtor }) + export class UseExistingService extends ParentService {} + `); + + env.driveMain(); + }); + + it('should give a compile-time error when a derived Directive inherits from a non-decorated class', () => { + // Errors for undecorated base classes should always be reported, even under + // `strictInjectionParameters`. + env.tsconfig({strictInjectionParameters: false}); + env.write('test.ts', ` + import {Directive} from '@angular/core'; + + export abstract class ParentClass { + constructor(private notInjectable: string) {} + } + + @Directive() + export abstract class AbstractMiddleDir extends ParentClass {} + + @Directive() + export class ConcreteMiddleDir extends AbstractMiddleDir {} + + @Directive() + export class ConcreteDirWithoutCtor extends ConcreteMiddleDir {} + + @Directive() + export class ConcreteDirWithCtor extends ConcreteMiddleDir { + constructor() { + super('correct'); + } + } + + @Directive() + export class ConcreteDerivedDirWithoutCtor extends ConcreteDirWithCtor {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(3); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR)); + expect(diags[0].messageText) + .toEqual( + `The directive AbstractMiddleDir inherits its constructor from ParentClass, but the latter ` + + `does not have an Angular decorator of its own. Dependency injection will not be able to resolve ` + + `the parameters of ParentClass's constructor. Either add a @Directive decorator to ParentClass, ` + + `or add an explicit constructor to AbstractMiddleDir.`); + expect(getDiagnosticSourceCode(diags[0])).toBe('AbstractMiddleDir'); + + expect(diags[1].code).toBe(ngErrorCode(ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR)); + expect(diags[1].messageText) + .toEqual( + `The directive ConcreteMiddleDir inherits its constructor from ParentClass, but the latter ` + + `does not have an Angular decorator of its own. Dependency injection will not be able to resolve ` + + `the parameters of ParentClass's constructor. Either add a @Directive decorator to ParentClass, ` + + `or add an explicit constructor to ConcreteMiddleDir.`); + expect(getDiagnosticSourceCode(diags[1])).toBe('ConcreteMiddleDir'); + + expect(diags[2].code).toBe(ngErrorCode(ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR)); + expect(diags[2].messageText) + .toEqual( + `The directive ConcreteDirWithoutCtor inherits its constructor from ParentClass, but the latter ` + + `does not have an Angular decorator of its own. Dependency injection will not be able to resolve ` + + `the parameters of ParentClass's constructor. Either add a @Directive decorator to ParentClass, ` + + `or add an explicit constructor to ConcreteDirWithoutCtor.`); + expect(getDiagnosticSourceCode(diags[2])).toBe('ConcreteDirWithoutCtor'); + }); }); });