From dcc8ff4ce7444c29892f6cd82f60fbf88bebcf95 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Wed, 11 Dec 2019 17:59:05 +0100 Subject: [PATCH] feat(ivy): throw compilation error when providing undecorated classes (#34460) Adds a compilation error if the consumer tries to pass in an undecorated class into the `providers` of an `NgModule`, or the `providers`/`viewProviders` arrays of a `Directive`/`Component`. PR Close #34460 --- .../ngcc/src/analysis/decoration_analyzer.ts | 15 +- .../src/ngtsc/annotations/src/component.ts | 90 ++++- .../src/ngtsc/annotations/src/diagnostics.ts | 43 +++ .../src/ngtsc/annotations/src/directive.ts | 45 ++- .../src/ngtsc/annotations/src/injectable.ts | 4 + .../src/ngtsc/annotations/src/ng_module.ts | 28 +- .../src/ngtsc/annotations/src/pipe.ts | 7 +- .../src/ngtsc/annotations/src/util.ts | 44 +++ .../ngtsc/annotations/test/component_spec.ts | 5 +- .../ngtsc/annotations/test/directive_spec.ts | 4 +- .../ngtsc/annotations/test/injectable_spec.ts | 4 +- .../ngtsc/annotations/test/ng_module_spec.ts | 5 +- .../src/ngtsc/diagnostics/src/code.ts | 3 + .../compiler-cli/src/ngtsc/metadata/index.ts | 2 +- .../src/ngtsc/metadata/src/registry.ts | 22 +- .../src/ngtsc/metadata/src/util.ts | 7 + packages/compiler-cli/src/ngtsc/program.ts | 13 +- .../src/ngtsc/transform/src/compilation.ts | 2 +- .../test/ngtsc/fake_core/index.ts | 2 + .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 328 ++++++++++++++++++ 20 files changed, 613 insertions(+), 60 deletions(-) create mode 100644 packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 9c799d0ae2..1f745f4618 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -7,12 +7,13 @@ */ import {ConstantPool} from '@angular/compiler'; import * as ts from 'typescript'; + import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader} from '../../../src/ngtsc/annotations'; import {CycleAnalyzer, ImportGraph} from '../../../src/ngtsc/cycles'; import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics'; import {FileSystem, LogicalFileSystem, absoluteFrom, dirname, resolve} from '../../../src/ngtsc/file_system'; import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, NOOP_DEFAULT_IMPORT_RECORDER, PrivateExportAliasingHost, Reexport, ReferenceEmitter} from '../../../src/ngtsc/imports'; -import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, LocalMetadataRegistry} from '../../../src/ngtsc/metadata'; +import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry} from '../../../src/ngtsc/metadata'; import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator'; import {ClassDeclaration} from '../../../src/ngtsc/reflection'; import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../../src/ngtsc/scope'; @@ -30,6 +31,7 @@ import {AnalyzedClass, AnalyzedFile, CompiledClass, CompiledFile, DecorationAnal import {NOOP_DEPENDENCY_TRACKER, analyzeDecorators, isWithinPackage} from './util'; + /** * Simple class that resolves and loads files directly from the filesystem. */ @@ -85,6 +87,7 @@ export class DecorationAnalyzer { new PartialEvaluator(this.reflectionHost, this.typeChecker, /* dependencyTracker */ null); importGraph = new ImportGraph(this.moduleResolver); cycleAnalyzer = new CycleAnalyzer(this.importGraph); + injectableRegistry = new InjectableClassRegistry(this.reflectionHost); handlers: DecoratorHandler[] = [ new ComponentDecoratorHandler( this.reflectionHost, this.evaluator, this.fullRegistry, this.fullMetaReader, @@ -92,28 +95,28 @@ export class DecorationAnalyzer { /* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true, this.bundle.enableI18nLegacyMessageIdFormat, this.moduleResolver, this.cycleAnalyzer, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, - NOOP_DEPENDENCY_TRACKER, /* annotateForClosureCompiler */ false), + NOOP_DEPENDENCY_TRACKER, this.injectableRegistry, /* annotateForClosureCompiler */ false), // See the note in ngtsc about why this cast is needed. // clang-format off new DirectiveDecoratorHandler( this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry, - NOOP_DEFAULT_IMPORT_RECORDER, this.isCore, + NOOP_DEFAULT_IMPORT_RECORDER, this.injectableRegistry, this.isCore, /* annotateForClosureCompiler */ false) as DecoratorHandler, // clang-format on // Pipe handler must be before injectable handler in list so pipe factories are printed // before injectable factories (so injectable factories can delegate to them) new PipeDecoratorHandler( this.reflectionHost, this.evaluator, this.metaRegistry, this.scopeRegistry, - NOOP_DEFAULT_IMPORT_RECORDER, this.isCore), + NOOP_DEFAULT_IMPORT_RECORDER, this.injectableRegistry, this.isCore), new InjectableDecoratorHandler( this.reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore, - /* strictCtorDeps */ false, /* errorOnDuplicateProv */ false), + /* strictCtorDeps */ false, this.injectableRegistry, /* errorOnDuplicateProv */ false), new NgModuleDecoratorHandler( this.reflectionHost, this.evaluator, this.fullMetaReader, this.fullRegistry, this.scopeRegistry, this.referencesRegistry, this.isCore, /* routeAnalyzer */ null, this.refEmitter, /* factoryTracker */ null, NOOP_DEFAULT_IMPORT_RECORDER, - /* annotateForClosureCompiler */ false), + /* annotateForClosureCompiler */ false, this.injectableRegistry), ]; migrations: Migration[] = [ new UndecoratedParentMigration(), diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 50b9a338c9..a9993158ca 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -15,7 +15,7 @@ import {absoluteFrom, relative} from '../../file_system'; import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; import {DependencyTracker} from '../../incremental/api'; import {IndexingContext} from '../../indexer'; -import {DirectiveMeta, MetadataReader, MetadataRegistry, extractDirectiveGuards} from '../../metadata'; +import {DirectiveMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry, extractDirectiveGuards} from '../../metadata'; import {flattenInheritedDirectiveMetadata} from '../../metadata/src/inheritance'; import {EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; @@ -26,10 +26,11 @@ import {tsSourceMapBug29300Fixed} from '../../util/src/ts_source_map_bug_29300'; import {SubsetOfKeys} from '../../util/src/typescript'; import {ResourceLoader} from './api'; +import {getProviderDiagnostics} from './diagnostics'; import {extractDirectiveMetadata, parseFieldArrayValue} from './directive'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, makeDuplicateDeclarationError, readBaseClass, unwrapExpression, wrapFunctionExpressionsInParens} from './util'; +import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, makeDuplicateDeclarationError, readBaseClass, resolveProvidersRequiringFactory, unwrapExpression, wrapFunctionExpressionsInParens} from './util'; const EMPTY_MAP = new Map(); const EMPTY_ARRAY: any[] = []; @@ -53,6 +54,18 @@ export interface ComponentAnalysisData { guards: ReturnType; template: ParsedTemplateWithSource; metadataStmt: Statement|null; + + /** + * Providers extracted from the `providers` field of the component annotation which will require + * an Angular factory definition at runtime. + */ + providersRequiringFactory: Set>|null; + + /** + * Providers extracted from the `viewProviders` field of the component annotation which will + * require an Angular factory definition at runtime. + */ + viewProvidersRequiringFactory: Set>|null; } export type ComponentResolutionData = Pick; @@ -71,7 +84,9 @@ export class ComponentDecoratorHandler implements private enableI18nLegacyMessageIdFormat: boolean, private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer, private refEmitter: ReferenceEmitter, private defaultImportRecorder: DefaultImportRecorder, - private depTracker: DependencyTracker|null, private annotateForClosureCompiler: boolean) {} + private depTracker: DependencyTracker|null, + private injectableRegistry: InjectableClassRegistry, + private annotateForClosureCompiler: boolean) {} private literalCache = new Map(); private elementSchemaRegistry = new DomElementSchemaRegistry(); @@ -186,12 +201,27 @@ export class ComponentDecoratorHandler implements } }, undefined) !; - const viewProviders: Expression|null = component.has('viewProviders') ? - new WrappedNodeExpr( - this.annotateForClosureCompiler ? - wrapFunctionExpressionsInParens(component.get('viewProviders') !) : - component.get('viewProviders') !) : - null; + + // Note that we could technically combine the `viewProvidersRequiringFactory` and + // `providersRequiringFactory` into a single set, but we keep the separate so that + // we can distinguish where an error is coming from when logging the diagnostics in `resolve`. + let viewProvidersRequiringFactory: Set>|null = null; + let providersRequiringFactory: Set>|null = null; + let wrappedViewProviders: Expression|null = null; + + if (component.has('viewProviders')) { + const viewProviders = component.get('viewProviders') !; + viewProvidersRequiringFactory = + resolveProvidersRequiringFactory(viewProviders, this.reflector, this.evaluator); + wrappedViewProviders = new WrappedNodeExpr( + this.annotateForClosureCompiler ? wrapFunctionExpressionsInParens(viewProviders) : + viewProviders); + } + + if (component.has('providers')) { + providersRequiringFactory = resolveProvidersRequiringFactory( + component.get('providers') !, this.reflector, this.evaluator); + } // Parse the template. // If a preanalyze phase was executed, the template may already exist in parsed form, so check @@ -290,7 +320,7 @@ export class ComponentDecoratorHandler implements // These will be replaced during the compilation step, after all `NgModule`s have been // analyzed and the full compilation scope for the component can be realized. animations, - viewProviders, + viewProviders: wrappedViewProviders, i18nUseExternalIds: this.i18nUseExternalIds, relativeContextFilePath, }, guards: extractDirectiveGuards(node, this.reflector), @@ -298,6 +328,8 @@ export class ComponentDecoratorHandler implements node, this.reflector, this.defaultImportRecorder, this.isCore, this.annotateForClosureCompiler), template, + providersRequiringFactory, + viewProvidersRequiringFactory, }, }; if (changeDetection !== null) { @@ -321,6 +353,8 @@ export class ComponentDecoratorHandler implements isComponent: true, baseClass: analysis.baseClass, ...analysis.guards, }); + + this.injectableRegistry.registerInjectable(node); } index( @@ -395,14 +429,6 @@ export class ComponentDecoratorHandler implements resolve(node: ClassDeclaration, analysis: Readonly): ResolveResult { - const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node); - if (duplicateDeclData !== null) { - // This component was declared twice (or more). - return { - diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Component')], - }; - } - const context = node.getSourceFile(); // Check whether this component was registered with an NgModule. If so, it should be compiled // under that module's compilation scope. @@ -505,6 +531,34 @@ export class ComponentDecoratorHandler implements this.scopeRegistry.setComponentAsRequiringRemoteScoping(node); } } + + const diagnostics: ts.Diagnostic[] = []; + + if (analysis.providersRequiringFactory !== null && + analysis.meta.providers instanceof WrappedNodeExpr) { + const providerDiagnostics = getProviderDiagnostics( + analysis.providersRequiringFactory, analysis.meta.providers !.node, + this.injectableRegistry); + diagnostics.push(...providerDiagnostics); + } + + if (analysis.viewProvidersRequiringFactory !== null && + analysis.meta.viewProviders instanceof WrappedNodeExpr) { + const viewProviderDiagnostics = getProviderDiagnostics( + analysis.viewProvidersRequiringFactory, analysis.meta.viewProviders !.node, + this.injectableRegistry); + diagnostics.push(...viewProviderDiagnostics); + } + + const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node); + if (duplicateDeclData !== null) { + diagnostics.push(makeDuplicateDeclarationError(node, duplicateDeclData, 'Component')); + } + + if (diagnostics.length > 0) { + return {diagnostics}; + } + return {data}; } diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts new file mode 100644 index 0000000000..dc2f39eefd --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts @@ -0,0 +1,43 @@ +/** + * @license + * Copyright Google Inc. 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 * as ts from 'typescript'; + +import {ErrorCode, makeDiagnostic} from '../../diagnostics'; +import {Reference} from '../../imports'; +import {InjectableClassRegistry} from '../../metadata'; +import {ClassDeclaration} from '../../reflection'; + +/** + * Gets the diagnostics for a set of provider classes. + * @param providerClasses Classes that should be checked. + * @param providersDeclaration Node that declares the providers array. + * @param registry Registry that keeps track of the registered injectable classes. + */ +export function getProviderDiagnostics( + providerClasses: Set>, providersDeclaration: ts.Expression, + registry: InjectableClassRegistry): ts.Diagnostic[] { + const diagnostics: ts.Diagnostic[] = []; + + for (const provider of providerClasses) { + if (registry.isInjectable(provider.node)) { + continue; + } + + const contextNode = provider.getOriginForDiagnostics(providersDeclaration); + diagnostics.push(makeDiagnostic( + ErrorCode.UNDECORATED_PROVIDER, contextNode, + `The class '${provider.node.name.text}' cannot be created via dependency injection, as it does not have an Angular decorator. This will result in an error at runtime. + +Either add the @Injectable() decorator to '${provider.node.name.text}', or configure a different provider (such as a provider with 'useFactory'). +`, + [{node: provider.node, messageText: `'${provider.node.name.text}' is declared here.`}])); + } + + return diagnostics; +} diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 77f1918fb0..e6a8206199 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -11,16 +11,17 @@ import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {DefaultImportRecorder, Reference} from '../../imports'; -import {MetadataRegistry} from '../../metadata'; +import {InjectableClassRegistry, MetadataRegistry} from '../../metadata'; import {extractDirectiveGuards} from '../../metadata/src/util'; import {DynamicValue, EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, ReflectionHost, filterToMembersWithDecorator, reflectObjectLiteral} from '../../reflection'; import {LocalModuleScopeRegistry} from '../../scope'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence, ResolveResult} from '../../transform'; +import {getProviderDiagnostics} from './diagnostics'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, makeDuplicateDeclarationError, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; +import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, makeDuplicateDeclarationError, readBaseClass, resolveProvidersRequiringFactory, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; const EMPTY_OBJECT: {[key: string]: string} = {}; const FIELD_DECORATORS = [ @@ -37,13 +38,16 @@ export interface DirectiveHandlerData { guards: ReturnType; meta: R3DirectiveMetadata; metadataStmt: Statement|null; + providersRequiringFactory: Set>|null; } + export class DirectiveDecoratorHandler implements DecoratorHandler { constructor( private reflector: ReflectionHost, private evaluator: PartialEvaluator, private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry, - private defaultImportRecorder: DefaultImportRecorder, private isCore: boolean, + private defaultImportRecorder: DefaultImportRecorder, + private injectableRegistry: InjectableClassRegistry, private isCore: boolean, private annotateForClosureCompiler: boolean) {} readonly precedence = HandlerPrecedence.PRIMARY; @@ -88,6 +92,12 @@ export class DirectiveDecoratorHandler implements return {}; } + let providersRequiringFactory: Set>|null = null; + if (directiveResult !== undefined && directiveResult.decorator.has('providers')) { + providersRequiringFactory = resolveProvidersRequiringFactory( + directiveResult.decorator.get('providers') !, this.reflector, this.evaluator); + } + return { analysis: { meta: analysis, @@ -95,7 +105,7 @@ export class DirectiveDecoratorHandler implements node, this.reflector, this.defaultImportRecorder, this.isCore, this.annotateForClosureCompiler), baseClass: readBaseClass(node, this.reflector, this.evaluator), - guards: extractDirectiveGuards(node, this.reflector), + guards: extractDirectiveGuards(node, this.reflector), providersRequiringFactory } }; } @@ -115,18 +125,28 @@ export class DirectiveDecoratorHandler implements isComponent: false, baseClass: analysis.baseClass, ...analysis.guards, }); + + this.injectableRegistry.registerInjectable(node); } - resolve(node: ClassDeclaration): ResolveResult { + resolve(node: ClassDeclaration, analysis: DirectiveHandlerData): ResolveResult { + const diagnostics: ts.Diagnostic[] = []; + + if (analysis.providersRequiringFactory !== null && + analysis.meta.providers instanceof WrappedNodeExpr) { + const providerDiagnostics = getProviderDiagnostics( + analysis.providersRequiringFactory, analysis.meta.providers !.node, + this.injectableRegistry); + diagnostics.push(...providerDiagnostics); + } + const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node); if (duplicateDeclData !== null) { // This directive was declared twice (or more). - return { - diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Directive')], - }; + diagnostics.push(makeDuplicateDeclarationError(node, duplicateDeclData, 'Directive')); } - return {}; + return {diagnostics: diagnostics.length > 0 ? diagnostics : undefined}; } compile( @@ -160,10 +180,8 @@ export function extractDirectiveMetadata( clazz: ClassDeclaration, decorator: Readonly, reflector: ReflectionHost, evaluator: PartialEvaluator, defaultImportRecorder: DefaultImportRecorder, isCore: boolean, flags: HandlerFlags, annotateForClosureCompiler: boolean, - defaultSelector: string | null = null): { - decorator: Map, - metadata: R3DirectiveMetadata, -}|undefined { + defaultSelector: string | null = + null): {decorator: Map, metadata: R3DirectiveMetadata}|undefined { let directive: Map; if (decorator === null || decorator.args === null || decorator.args.length === 0) { directive = new Map(); @@ -390,7 +408,6 @@ export function extractQueriesFromDecorator( view: R3QueryMetadata[], } { const content: R3QueryMetadata[] = [], view: R3QueryMetadata[] = []; - const expr = unwrapExpression(queryData); if (!ts.isObjectLiteralExpression(queryData)) { throw new Error(`queries metadata must be an object literal`); } diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts index 93717607b5..978bf693e2 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts @@ -11,6 +11,7 @@ import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {DefaultImportRecorder} from '../../imports'; +import {InjectableClassRegistry} from '../../metadata'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; @@ -33,6 +34,7 @@ export class InjectableDecoratorHandler implements constructor( private reflector: ReflectionHost, private defaultImportRecorder: DefaultImportRecorder, private isCore: boolean, private strictCtorDeps: boolean, + private injectableRegistry: InjectableClassRegistry, /** * What to do if the injectable already contains a ɵprov property. * @@ -80,6 +82,8 @@ export class InjectableDecoratorHandler implements }; } + register(node: ClassDeclaration): void { this.injectableRegistry.registerInjectable(node); } + compile(node: ClassDeclaration, analysis: Readonly): CompileResult[] { const res = compileIvyInjectable(analysis.meta); const statements = res.statements; diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index 6f4cb73016..1abeb1d9e8 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -11,8 +11,8 @@ import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError, makeDiagnostic} from '../../diagnostics'; import {DefaultImportRecorder, Reference, ReferenceEmitter} from '../../imports'; -import {MetadataReader, MetadataRegistry} from '../../metadata'; -import {PartialEvaluator, ResolvedValue} from '../../partial_evaluator'; +import {InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; +import {PartialEvaluator, ResolvedValue, ResolvedValueArray} from '../../partial_evaluator'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral, typeNodeToValueExpr} from '../../reflection'; import {NgModuleRouteAnalyzer} from '../../routing'; import {LocalModuleScopeRegistry, ScopeData} from '../../scope'; @@ -20,9 +20,10 @@ import {FactoryTracker} from '../../shims'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform'; import {getSourceFile} from '../../util/src/typescript'; +import {getProviderDiagnostics} from './diagnostics'; import {generateSetClassMetadataCall} from './metadata'; import {ReferencesRegistry} from './references_registry'; -import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; +import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, resolveProvidersRequiringFactory, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; export interface NgModuleAnalysis { mod: R3NgModuleMetadata; @@ -35,6 +36,8 @@ export interface NgModuleAnalysis { exports: Reference[]; id: Expression|null; factorySymbolName: string; + providersRequiringFactory: Set>|null; + providers: ts.Expression|null; } export interface NgModuleResolution { injectorImports: Expression[]; } @@ -54,7 +57,8 @@ export class NgModuleDecoratorHandler implements private routeAnalyzer: NgModuleRouteAnalyzer|null, private refEmitter: ReferenceEmitter, private factoryTracker: FactoryTracker|null, private defaultImportRecorder: DefaultImportRecorder, - private annotateForClosureCompiler: boolean, private localeId?: string) {} + private annotateForClosureCompiler: boolean, + private injectableRegistry: InjectableClassRegistry, private localeId?: string) {} readonly precedence = HandlerPrecedence.PRIMARY; readonly name = NgModuleDecoratorHandler.name; @@ -240,7 +244,7 @@ export class NgModuleDecoratorHandler implements }; const rawProviders = ngModule.has('providers') ? ngModule.get('providers') ! : null; - const providers = rawProviders !== null ? + const wrapperProviders = rawProviders !== null ? new WrappedNodeExpr( this.annotateForClosureCompiler ? wrapFunctionExpressionsInParens(rawProviders) : rawProviders) : @@ -264,7 +268,7 @@ export class NgModuleDecoratorHandler implements internalType, deps: getValidConstructorDependencies( node, this.reflector, this.defaultImportRecorder, this.isCore), - providers, + providers: wrapperProviders, imports: injectorImports, }; @@ -277,6 +281,10 @@ export class NgModuleDecoratorHandler implements declarations: declarationRefs, rawDeclarations, imports: importRefs, exports: exportRefs, + providers: rawProviders, + providersRequiringFactory: rawProviders ? + resolveProvidersRequiringFactory(rawProviders, this.reflector, this.evaluator) : + null, metadataStmt: generateSetClassMetadataCall( node, this.reflector, this.defaultImportRecorder, this.isCore, this.annotateForClosureCompiler), @@ -301,6 +309,8 @@ export class NgModuleDecoratorHandler implements if (this.factoryTracker !== null) { this.factoryTracker.track(node.getSourceFile(), analysis.factorySymbolName); } + + this.injectableRegistry.registerInjectable(node); } resolve(node: ClassDeclaration, analysis: Readonly): @@ -313,6 +323,12 @@ export class NgModuleDecoratorHandler implements diagnostics.push(...scopeDiagnostics); } + if (analysis.providersRequiringFactory !== null) { + const providerDiagnostics = getProviderDiagnostics( + analysis.providersRequiringFactory, analysis.providers !, this.injectableRegistry); + diagnostics.push(...providerDiagnostics); + } + const data: NgModuleResolution = { injectorImports: [], }; diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts index 0210aba663..c328cde05c 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts @@ -11,7 +11,7 @@ import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {DefaultImportRecorder, Reference} from '../../imports'; -import {MetadataRegistry} from '../../metadata'; +import {InjectableClassRegistry, MetadataRegistry} from '../../metadata'; import {PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; import {LocalModuleScopeRegistry} from '../../scope'; @@ -30,7 +30,8 @@ export class PipeDecoratorHandler implements DecoratorHandler): void { const ref = new Reference(node); this.metaRegistry.registerPipeMetadata({ref, name: analysis.meta.pipeName}); + + this.injectableRegistry.registerInjectable(node); } resolve(node: ClassDeclaration): ResolveResult { diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index 3d730de347..5bce8ff698 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -408,6 +408,50 @@ export function makeDuplicateDeclarationError( `The ${kind} '${node.name.text}' is declared by more than one NgModule.`, context); } +/** + * Resolves the given `rawProviders` into `ClassDeclarations` and returns + * a set containing those that are known to require a factory definition. + * @param rawProviders Expression that declared the providers array in the source. + */ +export function resolveProvidersRequiringFactory( + rawProviders: ts.Expression, reflector: ReflectionHost, + evaluator: PartialEvaluator): Set> { + const providers = new Set>(); + const resolvedProviders = evaluator.evaluate(rawProviders); + + if (!Array.isArray(resolvedProviders)) { + return providers; + } + + resolvedProviders.forEach(function processProviders(provider) { + let tokenClass: Reference|null = null; + + if (Array.isArray(provider)) { + // If we ran into an array, recurse into it until we've resolve all the classes. + provider.forEach(processProviders); + } else if (provider instanceof Reference) { + tokenClass = provider; + } else if (provider instanceof Map && provider.has('useClass') && !provider.has('deps')) { + const useExisting = provider.get('useClass') !; + if (useExisting instanceof Reference) { + tokenClass = useExisting; + } + } + + if (tokenClass !== null && reflector.isClass(tokenClass.node)) { + const constructorParameters = reflector.getConstructorParameters(tokenClass.node); + + // Note that we only want to capture providers with a non-trivial constructor, + // because they're the ones that might be using DI and need to be decorated. + if (constructorParameters !== null && constructorParameters.length > 0) { + providers.add(tokenClass as Reference); + } + } + }); + + return providers; +} + /** * Create an R3Reference for a class. * diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts index 09e9908179..02772389e3 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts @@ -10,7 +10,7 @@ import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {absoluteFrom} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; import {ModuleResolver, NOOP_DEFAULT_IMPORT_RECORDER, ReferenceEmitter} from '../../imports'; -import {CompoundMetadataReader, DtsMetadataReader, LocalMetadataRegistry} from '../../metadata'; +import {CompoundMetadataReader, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry} from '../../metadata'; import {PartialEvaluator} from '../../partial_evaluator'; import {TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection'; import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../scope'; @@ -59,13 +59,14 @@ runInEachFileSystem(() => { new ReferenceEmitter([]), null); const metaReader = new CompoundMetadataReader([metaRegistry, dtsReader]); const refEmitter = new ReferenceEmitter([]); + const injectableRegistry = new InjectableClassRegistry(reflectionHost); const handler = new ComponentDecoratorHandler( reflectionHost, evaluator, metaRegistry, metaReader, scopeRegistry, scopeRegistry, /* isCore */ false, new NoopResourceLoader(), /* rootDirs */[''], /* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true, /* enableI18nLegacyMessageIdFormat */ false, moduleResolver, cycleAnalyzer, refEmitter, - NOOP_DEFAULT_IMPORT_RECORDER, /* depTracker */ null, + NOOP_DEFAULT_IMPORT_RECORDER, /* depTracker */ null, injectableRegistry, /* annotateForClosureCompiler */ false); const TestCmp = getDeclaration(program, _('/entry.ts'), 'TestCmp', isNamedClassDeclaration); const detected = handler.detect(TestCmp, reflectionHost.getDecoratorsOfDeclaration(TestCmp)); diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts index 25588c6bec..9d7feef0d9 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts @@ -8,7 +8,7 @@ import {absoluteFrom} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; import {NOOP_DEFAULT_IMPORT_RECORDER, ReferenceEmitter} from '../../imports'; -import {DtsMetadataReader, LocalMetadataRegistry} from '../../metadata'; +import {DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry} from '../../metadata'; import {PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection'; import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../scope'; @@ -48,8 +48,10 @@ runInEachFileSystem(() => { const scopeRegistry = new LocalModuleScopeRegistry( metaReader, new MetadataDtsModuleScopeResolver(dtsReader, null), new ReferenceEmitter([]), null); + const injectableRegistry = new InjectableClassRegistry(reflectionHost); const handler = new DirectiveDecoratorHandler( reflectionHost, evaluator, scopeRegistry, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER, + injectableRegistry, /* isCore */ false, /* annotateForClosureCompiler */ false); const analyzeDirective = (dirName: string) => { 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 29b2134f84..fed17853d4 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/injectable_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/injectable_spec.ts @@ -9,6 +9,7 @@ import {ErrorCode, FatalDiagnosticError, ngErrorCode} from '../../diagnostics'; import {absoluteFrom} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; import {NOOP_DEFAULT_IMPORT_RECORDER} from '../../imports'; +import {InjectableClassRegistry} from '../../metadata'; import {TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection'; import {getDeclaration, makeProgram} from '../../testing'; import {InjectableDecoratorHandler} from '../src/injectable'; @@ -67,9 +68,10 @@ function setupHandler(errorOnDuplicateProv: boolean) { ]); const checker = program.getTypeChecker(); const reflectionHost = new TypeScriptReflectionHost(checker); + const injectableRegistry = new InjectableClassRegistry(reflectionHost); const handler = new InjectableDecoratorHandler( reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, /* isCore */ false, - /* strictCtorDeps */ false, errorOnDuplicateProv); + /* strictCtorDeps */ false, injectableRegistry, errorOnDuplicateProv); const TestClass = getDeclaration(program, ENTRY_FILE, 'TestClass', isNamedClassDeclaration); const ɵprov = reflectionHost.getMembersOfClass(TestClass).find(member => member.name === 'ɵprov'); if (ɵprov === undefined) { diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/ng_module_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/ng_module_spec.ts index 5f54d69ed7..2e89ddca33 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/ng_module_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/ng_module_spec.ts @@ -12,7 +12,7 @@ import * as ts from 'typescript'; import {absoluteFrom} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; import {LocalIdentifierStrategy, NOOP_DEFAULT_IMPORT_RECORDER, ReferenceEmitter} from '../../imports'; -import {CompoundMetadataReader, DtsMetadataReader, LocalMetadataRegistry} from '../../metadata'; +import {CompoundMetadataReader, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry} from '../../metadata'; import {PartialEvaluator} from '../../partial_evaluator'; import {TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection'; import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../scope'; @@ -66,11 +66,12 @@ runInEachFileSystem(() => { metaRegistry, new MetadataDtsModuleScopeResolver(dtsReader, null), new ReferenceEmitter([]), null); const refEmitter = new ReferenceEmitter([new LocalIdentifierStrategy()]); + const injectableRegistry = new InjectableClassRegistry(reflectionHost); const handler = new NgModuleDecoratorHandler( reflectionHost, evaluator, metaReader, metaRegistry, scopeRegistry, referencesRegistry, /* isCore */ false, /* routeAnalyzer */ null, refEmitter, /* factoryTracker */ null, - NOOP_DEFAULT_IMPORT_RECORDER, /* annotateForClosureCompiler */ false); + NOOP_DEFAULT_IMPORT_RECORDER, /* annotateForClosureCompiler */ false, injectableRegistry); const TestModule = getDeclaration(program, _('/entry.ts'), 'TestModule', isNamedClassDeclaration); const detected = diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index 4fa4890638..e4fbc34866 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -26,6 +26,9 @@ export enum ErrorCode { PARAM_MISSING_TOKEN = 2003, DIRECTIVE_MISSING_SELECTOR = 2004, + /** Raised when an undecorated class is passed in as a provider to a module or a directive. */ + UNDECORATED_PROVIDER = 2005, + SYMBOL_NOT_EXPORTED = 3001, SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002, diff --git a/packages/compiler-cli/src/ngtsc/metadata/index.ts b/packages/compiler-cli/src/ngtsc/metadata/index.ts index 21e32cbe33..0e585c8731 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/index.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/index.ts @@ -8,5 +8,5 @@ export * from './src/api'; export {DtsMetadataReader} from './src/dts'; -export {CompoundMetadataRegistry, LocalMetadataRegistry} from './src/registry'; +export {CompoundMetadataRegistry, LocalMetadataRegistry, InjectableClassRegistry} from './src/registry'; export {extractDirectiveGuards, CompoundMetadataReader} from './src/util'; diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts b/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts index f834218efd..b1bd7a5ffb 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts @@ -7,9 +7,10 @@ */ import {Reference} from '../../imports'; -import {ClassDeclaration} from '../../reflection'; +import {ClassDeclaration, ReflectionHost} from '../../reflection'; import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from './api'; +import {hasInjectableFields} from './util'; /** * A registry of directive, pipe, and module metadata for types defined in the current compilation @@ -59,3 +60,22 @@ 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/src/ngtsc/metadata/src/util.ts b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts index a3982fd37c..48b6a3a4a9 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts @@ -174,3 +174,10 @@ function afterUnderscore(str: string): string { } return str.substr(pos + 1); } + +/** Returns whether a class declaration has the necessary class fields to make it injectable. */ +export function hasInjectableFields(clazz: ClassDeclaration, host: ReflectionHost): boolean { + const members = host.getMembersOfClass(clazz); + return members.some( + ({isStatic, name}) => isStatic && (name === 'ɵprov' || name === 'ɵfac' || name === 'ɵinj')); +} diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 0bc91d2f2a..34e96fd181 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -23,6 +23,7 @@ import {IncrementalDriver} from './incremental'; import {IndexedComponent, IndexingContext} from './indexer'; import {generateAnalysis} from './indexer/src/transform'; import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, LocalMetadataRegistry, MetadataReader} from './metadata'; +import {InjectableClassRegistry} from './metadata/src/registry'; import {ModuleWithProvidersScanner} from './modulewithproviders'; import {PartialEvaluator} from './partial_evaluator'; import {NOOP_PERF_RECORDER, PerfRecorder, PerfTracker} from './perf'; @@ -629,6 +630,7 @@ export class NgtscProgram implements api.Program { localMetaReader, depScopeReader, this.refEmitter, this.aliasingHost); const scopeReader: ComponentScopeReader = this.scopeRegistry; const metaRegistry = new CompoundMetadataRegistry([localMetaRegistry, this.scopeRegistry]); + const injectableRegistry = new InjectableClassRegistry(this.reflector); this.metaReader = new CompoundMetadataReader([localMetaReader, dtsReader]); @@ -658,26 +660,27 @@ export class NgtscProgram implements api.Program { this.options.preserveWhitespaces || false, this.options.i18nUseExternalIds !== false, this.options.enableI18nLegacyMessageIdFormat !== false, this.moduleResolver, this.cycleAnalyzer, this.refEmitter, this.defaultImportTracker, - this.incrementalDriver.depGraph, this.closureCompilerEnabled), + this.incrementalDriver.depGraph, injectableRegistry, this.closureCompilerEnabled), // TODO(alxhub): understand why the cast here is necessary (something to do with `null` not // being assignable to `unknown` when wrapped in `Readonly`). // clang-format off new DirectiveDecoratorHandler( - this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.defaultImportTracker, + this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.defaultImportTracker, injectableRegistry, this.isCore, this.closureCompilerEnabled) as Readonly>, // clang-format on // Pipe handler must be before injectable handler in list so pipe factories are printed // before injectable factories (so injectable factories can delegate to them) new PipeDecoratorHandler( this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.defaultImportTracker, - this.isCore), + injectableRegistry, this.isCore), new InjectableDecoratorHandler( this.reflector, this.defaultImportTracker, this.isCore, - this.options.strictInjectionParameters || false), + this.options.strictInjectionParameters || false, injectableRegistry), new NgModuleDecoratorHandler( this.reflector, evaluator, this.metaReader, metaRegistry, this.scopeRegistry, referencesRegistry, this.isCore, this.routeAnalyzer, this.refEmitter, this.factoryTracker, - this.defaultImportTracker, this.closureCompilerEnabled, this.options.i18nInLocale), + this.defaultImportTracker, this.closureCompilerEnabled, injectableRegistry, + this.options.i18nInLocale), ]; return new TraitCompiler( diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index 66fe6b8e83..84d32fc1d5 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -363,7 +363,7 @@ export class TraitCompiler { } } - if (result.diagnostics !== undefined) { + if (result.diagnostics !== undefined && result.diagnostics.length > 0) { trait = trait.toErrored(result.diagnostics); } else { if (result.data !== undefined) { diff --git a/packages/compiler-cli/test/ngtsc/fake_core/index.ts b/packages/compiler-cli/test/ngtsc/fake_core/index.ts index 5f1a45dc80..d0ed030493 100644 --- a/packages/compiler-cli/test/ngtsc/fake_core/index.ts +++ b/packages/compiler-cli/test/ngtsc/fake_core/index.ts @@ -89,3 +89,5 @@ export class EventEmitter { export interface QueryList/* implements Iterable */ { [Symbol.iterator]: () => Iterator; } export type NgIterable = Array| Iterable; + +export class NgZone {} diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 877b8dc0c7..418feda007 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -5222,6 +5222,334 @@ export const Foo = Foo__PRE_R3__; }); }); + describe('undecorated providers', () => { + it('should error when an undecorated class, with a non-trivial constructor, is provided directly in a module', + () => { + env.write('test.ts', ` + import {NgModule, NgZone} from '@angular/core'; + + class NotAService { + constructor(ngZone: NgZone) {} + } + + @NgModule({ + providers: [NotAService] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('cannot be created via dependency injection'); + }); + + it('should error when an undecorated class is provided via useClass', () => { + env.write('test.ts', ` + import {NgModule, Injectable, NgZone} from '@angular/core'; + + @Injectable({providedIn: 'root'}) + class Service {} + + class NotAService { + constructor(ngZone: NgZone) {} + } + + @NgModule({ + providers: [{provide: Service, useClass: NotAService}] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('cannot be created via dependency injection'); + }); + + it('should not error when an undecorated class is provided via useClass with deps', () => { + env.write('test.ts', ` + import {NgModule, Injectable, NgZone} from '@angular/core'; + + @Injectable({providedIn: 'root'}) + class Service {} + + class NotAService { + constructor(ngZone: NgZone) {} + } + + @NgModule({ + providers: [{provide: Service, useClass: NotAService, deps: [NgZone]}] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should error when an undecorated class is provided via an array', () => { + env.write('test.ts', ` + import {NgModule, Injectable, NgZone} from '@angular/core'; + + @Injectable({providedIn: 'root'}) + class Service {} + + class NotAService { + constructor(ngZone: NgZone) {} + } + + @NgModule({ + providers: [Service, [NotAService]] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('cannot be created via dependency injection'); + }); + + it('should error when an undecorated class is provided to a directive', () => { + env.write('test.ts', ` + import {NgModule, Directive, NgZone} from '@angular/core'; + + class NotAService { + constructor(ngZone: NgZone) {} + } + + @Directive({ + selector: '[some-dir]', + providers: [NotAService] + }) + class SomeDirective {} + + @NgModule({ + declarations: [SomeDirective] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('cannot be created via dependency injection'); + }); + + it('should error when an undecorated class is provided to a component', () => { + env.write('test.ts', ` + import {NgModule, Component, NgZone} from '@angular/core'; + + class NotAService { + constructor(ngZone: NgZone) {} + } + + @Component({ + selector: 'some-comp', + template: '', + providers: [NotAService] + }) + class SomeComponent {} + + @NgModule({ + declarations: [SomeComponent] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('cannot be created via dependency injection'); + }); + + it('should error when an undecorated class is provided to a component via viewProviders', + () => { + env.write('test.ts', ` + import {NgModule, Component, NgZone} from '@angular/core'; + + class NotAService { + constructor(ngZone: NgZone) {} + } + + @Component({ + selector: 'some-comp', + template: '', + viewProviders: [NotAService] + }) + class SomeComponent {} + + @NgModule({ + declarations: [SomeComponent] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('cannot be created via dependency injection'); + }); + + it('should not error when a class with a factory is provided', () => { + env.write('test.ts', ` + import {NgModule, Pipe} from '@angular/core'; + + @Pipe({ + name: 'some-pipe' + }) + class SomePipe {} + + @NgModule({ + declarations: [SomePipe], + providers: [SomePipe] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not error when an NgModule is provided', () => { + env.write('test.ts', ` + import {Injectable, NgModule} from '@angular/core'; + + @Injectable() + export class Service {} + + @NgModule({ + }) + class SomeModule { + constructor(dep: Service) {} + } + + @NgModule({ + providers: [SomeModule], + }) + export class Module {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not error when an undecorated class from a declaration file is provided', () => { + env.write('node_modules/@angular/core/testing/index.d.ts', ` + export declare class Testability { + } + `); + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + import {Testability} from '@angular/core/testing'; + + @NgModule({ + providers: [Testability] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not error when an undecorated class without a constructor from a declaration file is provided via useClass', + () => { + env.write('node_modules/@angular/core/testing/index.d.ts', ` + export declare class Testability { + } + `); + env.write('test.ts', ` + import {NgModule, Injectable} from '@angular/core'; + import {Testability} from '@angular/core/testing'; + + @Injectable() + class TestingService {} + + @NgModule({ + providers: [{provide: TestingService, useClass: Testability}] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not error if the undecorated class does not have a constructor or the constructor is blank', + () => { + env.write('test.ts', ` + import {NgModule, NgZone} from '@angular/core'; + + class NoConstructorService { + } + + class BlankConstructorService { + } + + @NgModule({ + providers: [NoConstructorService, BlankConstructorService] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should error when an undecorated class with a non-trivial constructor in a declaration file is provided via useClass', + () => { + env.write('node_modules/@angular/core/testing/index.d.ts', ` + export declare class NgZone {} + + export declare class Testability { + constructor(ngZone: NgZone) {} + } + `); + env.write('test.ts', ` + import {NgModule, Injectable} from '@angular/core'; + import {Testability} from '@angular/core/testing'; + + @Injectable() + class TestingService {} + + @NgModule({ + providers: [{provide: TestingService, useClass: Testability}] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('cannot be created via dependency injection'); + }); + + it('should not error when an class with a factory definition and a non-trivial constructor in a declaration file is provided via useClass', + () => { + env.write('node_modules/@angular/core/testing/index.d.ts', ` + import * as i0 from '@angular/core'; + + export declare class NgZone {} + + export declare class Testability { + static ɵfac: i0.ɵɵFactoryDef; + constructor(ngZone: NgZone) {} + } + `); + env.write('test.ts', ` + import {NgModule, Injectable} from '@angular/core'; + import {Testability} from '@angular/core/testing'; + + @Injectable() + class TestingService {} + + @NgModule({ + providers: [{provide: TestingService, useClass: Testability}] + }) + export class SomeModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + }); + }); function expectTokenAtPosition(