From 13c2fad240cb87be58df299764adc8ba47fdb8c8 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Mon, 11 Nov 2019 00:18:43 -0800 Subject: [PATCH] fix(ivy): throw a better error when DI can't inject a ctor param (#33739) Occasionally a factory function needs to be generated for an "invalid" constructor (one with parameters types which aren't injectable). Typically this happens in JIT mode where understanding of parameters cannot be done in the same "up-front" way that the AOT compiler can. This commit changes the JIT compiler to generate a new `invalidFactoryDep` call for each invalid parameter. This instruction will error at runtime if called, indicating both the index of the invalid parameter as well as (via the stack trace) the factory function which was generated for the type being constructed. Fixes #33637 PR Close #33739 --- .../compiler/src/compiler_facade_interface.ts | 1 + packages/compiler/src/render3/r3_factory.ts | 12 ++++- .../compiler/src/render3/r3_identifiers.ts | 1 + .../src/compiler/compiler_facade_interface.ts | 1 + packages/core/src/di/index.ts | 2 +- .../core/src/di/injector_compatibility.ts | 24 ++++++++++ packages/core/src/di/jit/environment.ts | 3 +- packages/core/src/di/jit/util.ts | 10 ++-- packages/core/src/render3/jit/environment.ts | 3 +- packages/core/test/render3/ivy/jit_spec.ts | 48 +++++++++++++++++++ tools/public_api_guard/core/core.d.ts | 2 + 11 files changed, 97 insertions(+), 10 deletions(-) diff --git a/packages/compiler/src/compiler_facade_interface.ts b/packages/compiler/src/compiler_facade_interface.ts index 05da363184..79d06d41c1 100644 --- a/packages/compiler/src/compiler_facade_interface.ts +++ b/packages/compiler/src/compiler_facade_interface.ts @@ -67,6 +67,7 @@ export enum R3ResolvedDependencyType { Token = 0, Attribute = 1, ChangeDetectorRef = 2, + Invalid = 3, } export enum R3FactoryTarget { diff --git a/packages/compiler/src/render3/r3_factory.ts b/packages/compiler/src/render3/r3_factory.ts index 10d1edc7e7..7fe5eefc48 100644 --- a/packages/compiler/src/render3/r3_factory.ts +++ b/packages/compiler/src/render3/r3_factory.ts @@ -125,6 +125,11 @@ export enum R3ResolvedDependencyType { * Injecting the `ChangeDetectorRef` token. Needs special handling when injected into a pipe. */ ChangeDetectorRef = 2, + + /** + * An invalid dependency (no token could be determined). An error should be thrown at runtime. + */ + Invalid = 3, } /** @@ -271,11 +276,12 @@ export function compileFactoryFunction(meta: R3FactoryMetadata): R3FactoryFn { function injectDependencies( deps: R3DependencyMetadata[], injectFn: o.ExternalReference, isPipe: boolean): o.Expression[] { - return deps.map(dep => compileInjectDependency(dep, injectFn, isPipe)); + return deps.map((dep, index) => compileInjectDependency(dep, injectFn, isPipe, index)); } function compileInjectDependency( - dep: R3DependencyMetadata, injectFn: o.ExternalReference, isPipe: boolean): o.Expression { + dep: R3DependencyMetadata, injectFn: o.ExternalReference, isPipe: boolean, + index: number): o.Expression { // Interpret the dependency according to its resolved type. switch (dep.resolved) { case R3ResolvedDependencyType.Token: @@ -305,6 +311,8 @@ function compileInjectDependency( case R3ResolvedDependencyType.Attribute: // In the case of attributes, the attribute name in question is given as the token. return o.importExpr(R3.injectAttribute).callFn([dep.token]); + case R3ResolvedDependencyType.Invalid: + return o.importExpr(R3.invalidFactoryDep).callFn([o.literal(index)]); default: return unsupported( `Unknown R3ResolvedDependencyType: ${R3ResolvedDependencyType[dep.resolved]}`); diff --git a/packages/compiler/src/render3/r3_identifiers.ts b/packages/compiler/src/render3/r3_identifiers.ts index 63f22321a5..66d0b0a5b9 100644 --- a/packages/compiler/src/render3/r3_identifiers.ts +++ b/packages/compiler/src/render3/r3_identifiers.ts @@ -212,6 +212,7 @@ export class Identifiers { static directiveInject: o.ExternalReference = {name: 'ɵɵdirectiveInject', moduleName: CORE}; static invalidFactory: o.ExternalReference = {name: 'ɵɵinvalidFactory', moduleName: CORE}; + static invalidFactoryDep: o.ExternalReference = {name: 'ɵɵinvalidFactoryDep', moduleName: CORE}; static templateRefExtractor: o.ExternalReference = {name: 'ɵɵtemplateRefExtractor', moduleName: CORE}; diff --git a/packages/core/src/compiler/compiler_facade_interface.ts b/packages/core/src/compiler/compiler_facade_interface.ts index 05da363184..79d06d41c1 100644 --- a/packages/core/src/compiler/compiler_facade_interface.ts +++ b/packages/core/src/compiler/compiler_facade_interface.ts @@ -67,6 +67,7 @@ export enum R3ResolvedDependencyType { Token = 0, Attribute = 1, ChangeDetectorRef = 2, + Invalid = 3, } export enum R3FactoryTarget { diff --git a/packages/core/src/di/index.ts b/packages/core/src/di/index.ts index 4281c5dc81..e941091a9a 100644 --- a/packages/core/src/di/index.ts +++ b/packages/core/src/di/index.ts @@ -18,7 +18,7 @@ export {ɵɵdefineInjectable, defineInjectable, ɵɵdefineInjector, InjectableTy export {forwardRef, resolveForwardRef, ForwardRefFn} from './forward_ref'; export {Injectable, InjectableDecorator, InjectableProvider} from './injectable'; export {Injector} from './injector'; -export {ɵɵinject, inject, INJECTOR} from './injector_compatibility'; +export {ɵɵinject, inject, INJECTOR, ɵɵinvalidFactoryDep} from './injector_compatibility'; export {ReflectiveInjector} from './reflective_injector'; export {ClassProvider, ClassSansProvider, ConstructorProvider, ConstructorSansProvider, ExistingProvider, ExistingSansProvider, FactoryProvider, FactorySansProvider, Provider, StaticClassProvider, StaticClassSansProvider, StaticProvider, TypeProvider, ValueProvider, ValueSansProvider} from './interface/provider'; export {ResolvedReflectiveFactory, ResolvedReflectiveProvider} from './reflective_provider'; diff --git a/packages/core/src/di/injector_compatibility.ts b/packages/core/src/di/injector_compatibility.ts index 04abc56919..bb420a7b23 100644 --- a/packages/core/src/di/injector_compatibility.ts +++ b/packages/core/src/di/injector_compatibility.ts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ +import '../util/ng_dev_mode'; + import {Type} from '../interface/type'; import {getClosureSafeProperty} from '../util/property'; import {stringify} from '../util/stringify'; @@ -114,6 +116,28 @@ export function ɵɵinject(token: Type| InjectionToken, flags = InjectF return (_injectImplementation || injectInjectorOnly)(resolveForwardRef(token), flags); } +/** + * Throws an error indicating that a factory function could not be generated by the compiler for a + * particular class. + * + * This instruction allows the actual error message to be optimized away when ngDevMode is turned + * off, saving bytes of generated code while still providing a good experience in dev mode. + * + * The name of the class is not mentioned here, but will be in the generated factory function name + * and thus in the stack trace. + * + * @codeGenApi + */ +export function ɵɵinvalidFactoryDep(index: number): never { + const msg = ngDevMode ? + `This constructor is not compatible with Angular Dependency Injection because its dependency at index ${index} of the parameter list is invalid. +This can happen if the dependency type is a primitive like a string or if an ancestor of this class is missing an Angular decorator. + +Please check that 1) the type for the parameter at index ${index} is correct and 2) the correct Angular decorators are defined for this class and its ancestors.` : + 'invalid'; + throw new Error(msg); +} + /** * Injects a token from the currently active injector. * diff --git a/packages/core/src/di/jit/environment.ts b/packages/core/src/di/jit/environment.ts index 3d2fc5a497..d544fcc446 100644 --- a/packages/core/src/di/jit/environment.ts +++ b/packages/core/src/di/jit/environment.ts @@ -8,7 +8,7 @@ import {Type} from '../../interface/type'; import {isForwardRef, resolveForwardRef} from '../forward_ref'; -import {ɵɵinject} from '../injector_compatibility'; +import {ɵɵinject, ɵɵinvalidFactoryDep} from '../injector_compatibility'; import {getInjectableDef, getInjectorDef, ɵɵdefineInjectable, ɵɵdefineInjector} from '../interface/defs'; @@ -23,6 +23,7 @@ export const angularCoreDiEnv: {[name: string]: Function} = { 'ɵɵdefineInjector': ɵɵdefineInjector, 'ɵɵinject': ɵɵinject, 'ɵɵgetFactoryOf': getFactoryOf, + 'ɵɵinvalidFactoryDep': ɵɵinvalidFactoryDep, }; function getFactoryOf(type: Type): ((type?: Type) => T)|null { diff --git a/packages/core/src/di/jit/util.ts b/packages/core/src/di/jit/util.ts index 51270cc21c..6ba1920d17 100644 --- a/packages/core/src/di/jit/util.ts +++ b/packages/core/src/di/jit/util.ts @@ -7,7 +7,7 @@ */ import {ChangeDetectorRef} from '../../change_detection/change_detector_ref'; -import {CompilerFacade, R3DependencyMetadataFacade, getCompilerFacade} from '../../compiler/compiler_facade'; +import {CompilerFacade, R3DependencyMetadataFacade, R3ResolvedDependencyType, getCompilerFacade} from '../../compiler/compiler_facade'; import {Type} from '../../interface/type'; import {ReflectionCapabilities} from '../../reflection/reflection_capabilities'; import {Attribute, Host, Inject, Optional, Self, SkipSelf} from '../metadata'; @@ -42,10 +42,7 @@ function reflectDependency(compiler: CompilerFacade, dep: any | any[]): R3Depend meta.token = token; } - if (Array.isArray(dep)) { - if (dep.length === 0) { - throw new Error('Dependency array must have arguments.'); - } + if (Array.isArray(dep) && dep.length > 0) { for (let j = 0; j < dep.length; j++) { const param = dep[j]; if (param === undefined) { @@ -74,6 +71,9 @@ function reflectDependency(compiler: CompilerFacade, dep: any | any[]): R3Depend setTokenAndResolvedType(param); } } + } else if (dep === undefined || (Array.isArray(dep) && dep.length === 0)) { + meta.token = undefined; + meta.resolved = R3ResolvedDependencyType.Invalid; } else { setTokenAndResolvedType(dep); } diff --git a/packages/core/src/render3/jit/environment.ts b/packages/core/src/render3/jit/environment.ts index bab9d77657..5e4612edb2 100644 --- a/packages/core/src/render3/jit/environment.ts +++ b/packages/core/src/render3/jit/environment.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ɵɵinject} from '../../di/injector_compatibility'; +import {ɵɵinject, ɵɵinvalidFactoryDep} from '../../di/injector_compatibility'; import {ɵɵdefineInjectable, ɵɵdefineInjector} from '../../di/interface/defs'; import * as sanitization from '../../sanitization/sanitization'; import * as r3 from '../index'; @@ -42,6 +42,7 @@ export const angularCoreEnv: {[name: string]: Function} = 'ɵɵinject': ɵɵinject, 'ɵɵinjectAttribute': r3.ɵɵinjectAttribute, 'ɵɵinvalidFactory': r3.ɵɵinvalidFactory, + 'ɵɵinvalidFactoryDep': ɵɵinvalidFactoryDep, 'ɵɵinjectPipeChangeDetectorRef': r3.ɵɵinjectPipeChangeDetectorRef, 'ɵɵtemplateRefExtractor': r3.ɵɵtemplateRefExtractor, 'ɵɵNgOnChangesFeature': r3.ɵɵNgOnChangesFeature, diff --git a/packages/core/test/render3/ivy/jit_spec.ts b/packages/core/test/render3/ivy/jit_spec.ts index 2f033346b3..37d9572772 100644 --- a/packages/core/test/render3/ivy/jit_spec.ts +++ b/packages/core/test/render3/ivy/jit_spec.ts @@ -342,6 +342,54 @@ ivyEnabled && describe('render3 jit', () => { expect((TestComponent as any).ɵcmp.viewQuery).not.toBeNull(); }); + + describe('invalid parameters', () => { + it('should error when creating an @Injectable that extends a class with a faulty parameter', () => { + @Injectable({providedIn: 'root'}) + class Legit { + } + + + @Injectable() + class Base { + constructor(first: Legit, second: any) {} + } + + @Injectable({providedIn: 'root'}) + class Service extends Base { + } + + const ServiceAny = Service as any; + + expect(ServiceAny.ɵprov).toBeDefined(); + expect(ServiceAny.ɵprov.providedIn).toBe('root'); + expect(() => ɵɵinject(Service)) + .toThrowError( + /constructor is not compatible with Angular Dependency Injection because its dependency at index 1 of the parameter list is invalid/); + }); + + it('should error when creating an @Directive that extends an undecorated class with parameters', + () => { + @Injectable({providedIn: 'root'}) + class Legit { + } + + class BaseDir { + constructor(first: Legit) {} + } + + @Directive({selector: 'test'}) + class TestDir extends BaseDir { + } + + const TestDirAny = TestDir as any; + + expect(TestDirAny.ɵfac).toBeDefined(); + expect(() => TestDirAny.ɵfac()) + .toThrowError( + /constructor is not compatible with Angular Dependency Injection because its dependency at index 0 of the parameter list is invalid/); + }); + }); }); it('ensure at least one spec exists', () => {}); diff --git a/tools/public_api_guard/core/core.d.ts b/tools/public_api_guard/core/core.d.ts index b31ed025c7..7d0e7e2417 100644 --- a/tools/public_api_guard/core/core.d.ts +++ b/tools/public_api_guard/core/core.d.ts @@ -897,6 +897,8 @@ export declare function ɵɵinjectPipeChangeDetectorRef(flags?: InjectFlags): Ch export declare function ɵɵinvalidFactory(): never; +export declare function ɵɵinvalidFactoryDep(index: number): never; + export declare function ɵɵlistener(eventName: string, listenerFn: (e?: any) => any, useCapture?: boolean, eventTargetResolver?: GlobalTargetResolver): typeof ɵɵlistener; export declare function ɵɵloadQuery(): QueryList;