diff --git a/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts index 84d747fc98..03149452bc 100644 --- a/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts @@ -77,7 +77,7 @@ export class DecorationAnalyzer { this.moduleResolver, this.cycleAnalyzer), new DirectiveDecoratorHandler( this.reflectionHost, this.evaluator, this.scopeRegistry, this.isCore), - new InjectableDecoratorHandler(this.reflectionHost, this.isCore), + new InjectableDecoratorHandler(this.reflectionHost, this.isCore, /* strictCtorDeps */ false), new NgModuleDecoratorHandler( this.reflectionHost, this.evaluator, this.scopeRegistry, this.referencesRegistry, this.isCore, /* routeAnalyzer */ null), diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 8c3c5c66d2..61f2a72e2d 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -17,7 +17,7 @@ import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; import {generateSetClassMetadataCall} from './metadata'; import {SelectorScopeRegistry} from './selector_scope'; -import {extractDirectiveGuards, getConstructorDependencies, isAngularCore, unwrapExpression, unwrapForwardRef} from './util'; +import {extractDirectiveGuards, getValidConstructorDependencies, isAngularCore, unwrapExpression, unwrapForwardRef} from './util'; const EMPTY_OBJECT: {[key: string]: string} = {}; @@ -196,7 +196,7 @@ export function extractDirectiveMetadata( clazz.heritageClauses.some(hc => hc.token === ts.SyntaxKind.ExtendsKeyword); const metadata: R3DirectiveMetadata = { name: clazz.name !.text, - deps: getConstructorDependencies(clazz, reflector, isCore), host, + deps: getValidConstructorDependencies(clazz, reflector, isCore), host, lifecycle: { usesOnChanges, }, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts index 9f72891cb8..c3434fadce 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts @@ -14,7 +14,7 @@ import {Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection' import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; import {generateSetClassMetadataCall} from './metadata'; -import {getConstructorDependencies, isAngularCore} from './util'; +import {getConstructorDependencies, getValidConstructorDependencies, isAngularCore, validateConstructorDependencies} from './util'; export interface InjectableHandlerData { meta: R3InjectableMetadata; @@ -26,7 +26,9 @@ export interface InjectableHandlerData { */ export class InjectableDecoratorHandler implements DecoratorHandler { - constructor(private reflector: ReflectionHost, private isCore: boolean) {} + constructor( + private reflector: ReflectionHost, private isCore: boolean, private strictCtorDeps: boolean) { + } detect(node: ts.Declaration, decorators: Decorator[]|null): Decorator|undefined { if (!decorators) { @@ -39,7 +41,8 @@ export class InjectableDecoratorHandler implements analyze(node: ts.ClassDeclaration, decorator: Decorator): AnalysisOutput { return { analysis: { - meta: extractInjectableMetadata(node, decorator, this.reflector, this.isCore), + meta: extractInjectableMetadata( + node, decorator, this.reflector, this.isCore, this.strictCtorDeps), metadataStmt: generateSetClassMetadataCall(node, this.reflector, this.isCore), }, }; @@ -62,23 +65,49 @@ export class InjectableDecoratorHandler implements /** * Read metadata from the `@Injectable` decorator and produce the `IvyInjectableMetadata`, the input * metadata needed to run `compileIvyInjectable`. + * + * A `null` return value indicates this is @Injectable has invalid data. */ function extractInjectableMetadata( - clazz: ts.ClassDeclaration, decorator: Decorator, reflector: ReflectionHost, - isCore: boolean): R3InjectableMetadata { + clazz: ts.ClassDeclaration, decorator: Decorator, reflector: ReflectionHost, isCore: boolean, + strictCtorDeps: boolean): R3InjectableMetadata { if (clazz.name === undefined) { throw new FatalDiagnosticError( ErrorCode.DECORATOR_ON_ANONYMOUS_CLASS, decorator.node, `@Injectable on anonymous class`); } const name = clazz.name.text; const type = new WrappedNodeExpr(clazz.name); - const ctorDeps = getConstructorDependencies(clazz, reflector, isCore); const typeArgumentCount = reflector.getGenericArityOfClass(clazz) || 0; if (decorator.args === null) { throw new FatalDiagnosticError( ErrorCode.DECORATOR_NOT_CALLED, decorator.node, '@Injectable must be called'); } if (decorator.args.length === 0) { + // Ideally, using @Injectable() would have the same effect as using @Injectable({...}), and be + // subject to the same validation. However, existing Angular code abuses @Injectable, applying + // it to things like abstract classes with constructors that were never meant for use with + // Angular's DI. + // + // To deal with this, @Injectable() without an argument is more lenient, and if the constructor + // signature does not work for DI then an ngInjectableDef that throws. + let ctorDeps: R3DependencyMetadata[]|'invalid'|null = null; + if (strictCtorDeps) { + ctorDeps = getValidConstructorDependencies(clazz, reflector, isCore); + } else { + const possibleCtorDeps = getConstructorDependencies(clazz, reflector, isCore); + if (possibleCtorDeps !== null) { + if (possibleCtorDeps.deps !== null) { + // This use of @Injectable has valid constructor dependencies. + ctorDeps = possibleCtorDeps.deps; + } else { + // This use of @Injectable is technically invalid. Generate a factory function which + // throws + // an error. + // TODO(alxhub): log warnings for the bad use of @Injectable. + ctorDeps = 'invalid'; + } + } + } return { name, type, @@ -86,6 +115,20 @@ function extractInjectableMetadata( providedIn: new LiteralExpr(null), ctorDeps, }; } else if (decorator.args.length === 1) { + const rawCtorDeps = getConstructorDependencies(clazz, reflector, isCore); + let ctorDeps: R3DependencyMetadata[]|'invalid'|null = null; + + // rawCtorDeps will be null if the class has no constructor. + if (rawCtorDeps !== null) { + if (rawCtorDeps.deps !== null) { + // A constructor existed and had valid dependencies. + ctorDeps = rawCtorDeps.deps; + } else { + // A constructor existed but had invalid dependencies. + ctorDeps = 'invalid'; + } + } + const metaNode = decorator.args[0]; // Firstly make sure the decorator argument is an inline literal - if not, it's illegal to // transport references from one location to another. This is the problem that lowering @@ -119,7 +162,7 @@ function extractInjectableMetadata( typeArgumentCount, ctorDeps, providedIn, - useValue: new WrappedNodeExpr(meta.get('useValue') !) + useValue: new WrappedNodeExpr(meta.get('useValue') !), }; } else if (meta.has('useExisting')) { return { @@ -128,7 +171,7 @@ function extractInjectableMetadata( typeArgumentCount, ctorDeps, providedIn, - useExisting: new WrappedNodeExpr(meta.get('useExisting') !) + useExisting: new WrappedNodeExpr(meta.get('useExisting') !), }; } else if (meta.has('useClass')) { return { @@ -137,13 +180,23 @@ function extractInjectableMetadata( typeArgumentCount, ctorDeps, providedIn, - useClass: new WrappedNodeExpr(meta.get('useClass') !), userDeps + useClass: new WrappedNodeExpr(meta.get('useClass') !), userDeps, }; } else if (meta.has('useFactory')) { // useFactory is special - the 'deps' property must be analyzed. const factory = new WrappedNodeExpr(meta.get('useFactory') !); - return {name, type, typeArgumentCount, providedIn, useFactory: factory, ctorDeps, userDeps}; + return { + name, + type, + typeArgumentCount, + providedIn, + useFactory: factory, ctorDeps, userDeps, + }; } else { + if (strictCtorDeps) { + // Since use* was not provided, validate the deps according to strictCtorDeps. + validateConstructorDependencies(clazz, rawCtorDeps); + } return {name, type, typeArgumentCount, providedIn, ctorDeps}; } } else { 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 bb15665356..99c2863a76 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -19,7 +19,7 @@ import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; import {generateSetClassMetadataCall} from './metadata'; import {ReferencesRegistry} from './references_registry'; import {SelectorScopeRegistry} from './selector_scope'; -import {getConstructorDependencies, isAngularCore, toR3Reference, unwrapExpression} from './util'; +import {getValidConstructorDependencies, isAngularCore, toR3Reference, unwrapExpression} from './util'; export interface NgModuleAnalysis { ngModuleDef: R3NgModuleMetadata; @@ -145,7 +145,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler { 'i0.ɵNgModuleDefWithMeta'); }); + describe('compiling invalid @Injectables', () => { + describe('with strictInjectionParameters = true', () => { + it('should give a compile-time error if an invalid @Injectable is used with no arguments', + () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable() + export class Test { + constructor(private notInjectable: string) {} + } + `); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(1); + expect(errors[0].messageText).toContain('No suitable injection token for parameter'); + }); + + it('should give a compile-time error if an invalid @Injectable is used with an argument', + () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable() + export class Test { + constructor(private notInjectable: string) {} + } + `); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(1); + expect(errors[0].messageText).toContain('No suitable injection token for parameter'); + }); + + it('should not give a compile-time error if an invalid @Injectable is used with useValue', + () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable({ + providedIn: 'root', + useValue: '42', + }) + export class Test { + constructor(private notInjectable: string) {} + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toMatch(/if \(t\).*throw new Error.* else .* '42'/ms); + }); + }); + + describe('with strictInjectionParameters = false', () => { + it('should compile an @Injectable on a class with a non-injectable constructor', () => { + env.tsconfig({strictInjectionParameters: false}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable() + export class Test { + constructor(private notInjectable: string) {} + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('factory: function Test_Factory(t) { throw new Error('); + }); + + it('should compile an @Injectable provided in the root on a class with a non-injectable constructor', + () => { + env.tsconfig({strictInjectionParameters: false}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + @Injectable({providedIn: 'root'}) + export class Test { + constructor(private notInjectable: string) {} + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('factory: function Test_Factory(t) { throw new Error('); + }); + + }); + }); + describe('former View Engine AST transform bugs', () => { it('should compile array literals behind conditionals', () => { env.tsconfig(); diff --git a/packages/compiler/src/injectable_compiler_2.ts b/packages/compiler/src/injectable_compiler_2.ts index c2484b3ebc..e1c7b869de 100644 --- a/packages/compiler/src/injectable_compiler_2.ts +++ b/packages/compiler/src/injectable_compiler_2.ts @@ -22,7 +22,7 @@ export interface R3InjectableMetadata { name: string; type: o.Expression; typeArgumentCount: number; - ctorDeps: R3DependencyMetadata[]|null; + ctorDeps: R3DependencyMetadata[]|'invalid'|null; providedIn: o.Expression; useClass?: o.Expression; useFactory?: o.Expression; @@ -46,11 +46,14 @@ export function compileInjectable(meta: R3InjectableMetadata): InjectableDef { // used to instantiate the class with dependencies injected, or deps are not specified and // the factory of the class is used to instantiate it. // - // A special case exists for useClass: Type where Type is the injectable type itself, in which - // case omitting deps just uses the constructor dependencies instead. + // A special case exists for useClass: Type where Type is the injectable type itself and no + // deps are specified, in which case 'useClass' is effectively ignored. const useClassOnSelf = meta.useClass.isEquivalent(meta.type); - const deps = meta.userDeps || (useClassOnSelf && meta.ctorDeps) || undefined; + let deps: R3DependencyMetadata[]|undefined = undefined; + if (meta.userDeps !== undefined) { + deps = meta.userDeps; + } if (deps !== undefined) { // factory: () => new meta.useClass(...deps) @@ -60,6 +63,8 @@ export function compileInjectable(meta: R3InjectableMetadata): InjectableDef { delegateDeps: deps, delegateType: R3FactoryDelegateType.Class, }); + } else if (useClassOnSelf) { + result = compileFactoryFunction(factoryMeta); } else { result = compileFactoryFunction({ ...factoryMeta, diff --git a/packages/compiler/src/render3/r3_factory.ts b/packages/compiler/src/render3/r3_factory.ts index d1f0a8a5e4..4808322ef4 100644 --- a/packages/compiler/src/render3/r3_factory.ts +++ b/packages/compiler/src/render3/r3_factory.ts @@ -40,9 +40,11 @@ export interface R3ConstructorFactoryMetadata { * Regardless of whether `fnOrClass` is a constructor function or a user-defined factory, it * may have 0 or more parameters, which will be injected according to the `R3DependencyMetadata` * for those parameters. If this is `null`, then the type's constructor is nonexistent and will - * be inherited from `fnOrClass` which is interpreted as the current type. + * be inherited from `fnOrClass` which is interpreted as the current type. If this is `'invalid'`, + * then one or more of the parameters wasn't resolvable and any attempt to use these deps will + * result in a runtime error. */ - deps: R3DependencyMetadata[]|null; + deps: R3DependencyMetadata[]|'invalid'|null; /** * An expression for the function which will be used to inject dependencies. The API of this @@ -152,7 +154,9 @@ export function compileFactoryFunction(meta: R3FactoryMetadata): let ctorExpr: o.Expression|null = null; if (meta.deps !== null) { // There is a constructor (either explicitly or implicitly defined). - ctorExpr = new o.InstantiateExpr(typeForCtor, injectDependencies(meta.deps, meta.injectFn)); + if (meta.deps !== 'invalid') { + ctorExpr = new o.InstantiateExpr(typeForCtor, injectDependencies(meta.deps, meta.injectFn)); + } } else { const baseFactory = o.variable(`ɵ${meta.name}_BaseFactory`); const getInheritedFactory = o.importExpr(R3.getInheritedFactory); @@ -173,7 +177,13 @@ export function compileFactoryFunction(meta: R3FactoryMetadata): function makeConditionalFactory(nonCtorExpr: o.Expression): o.ReadVarExpr { const r = o.variable('r'); body.push(r.set(o.NULL_EXPR).toDeclStmt()); - body.push(o.ifStmt(t, [r.set(ctorExprFinal).toStmt()], [r.set(nonCtorExpr).toStmt()])); + let ctorStmt: o.Statement|null = null; + if (ctorExprFinal !== null) { + ctorStmt = r.set(ctorExprFinal).toStmt(); + } else { + ctorStmt = makeErrorStmt(meta.name); + } + body.push(o.ifStmt(t, [ctorStmt], [r.set(nonCtorExpr).toStmt()])); return r; } @@ -207,10 +217,16 @@ export function compileFactoryFunction(meta: R3FactoryMetadata): retExpr = ctorExpr; } + if (retExpr !== null) { + body.push(new o.ReturnStatement(retExpr)); + } else { + body.push(makeErrorStmt(meta.name)); + } + return { factory: o.fn( - [new o.FnParam('t', o.DYNAMIC_TYPE)], [...body, new o.ReturnStatement(retExpr)], - o.INFERRED_TYPE, undefined, `${meta.name}_Factory`), + [new o.FnParam('t', o.DYNAMIC_TYPE)], body, o.INFERRED_TYPE, undefined, + `${meta.name}_Factory`), statements, }; } @@ -292,6 +308,13 @@ export function dependenciesFromGlobalMetadata( return deps; } +function makeErrorStmt(name: string): o.Statement { + return new o.ThrowStmt(new o.InstantiateExpr(new o.ReadVarExpr('Error'), [ + o.literal( + `${name} has a constructor which is not compatible with Dependency Injection. It should probably not be @Injectable().`) + ])); +} + function isDelegatedMetadata(meta: R3FactoryMetadata): meta is R3DelegatedFactoryMetadata| R3DelegatedFnOrClassMetadata { return (meta as any).delegateType !== undefined; diff --git a/packages/core/src/di/jit/injectable.ts b/packages/core/src/di/jit/injectable.ts index 45a2e592f7..128985b578 100644 --- a/packages/core/src/di/jit/injectable.ts +++ b/packages/core/src/di/jit/injectable.ts @@ -43,7 +43,7 @@ export function compileInjectable(type: Type, srcMeta?: Injectable): void { typeArgumentCount: 0, providedIn: meta.providedIn, ctorDeps: reflectDependencies(type), - userDeps: undefined + userDeps: undefined, }; if ((isUseClassProvider(meta) || isUseFactoryProvider(meta)) && meta.deps !== undefined) { compilerMeta.userDeps = convertDependencies(meta.deps);