Navigation Menu

Skip to content

Commit

Permalink
fix(ivy): throw a better error when DI can't inject a ctor param
Browse files Browse the repository at this point in the history
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
  • Loading branch information
alxhub committed Nov 19, 2019
1 parent bb290ce commit 2f8a0ec
Show file tree
Hide file tree
Showing 11 changed files with 73 additions and 10 deletions.
1 change: 1 addition & 0 deletions packages/compiler/src/compiler_facade_interface.ts
Expand Up @@ -67,6 +67,7 @@ export enum R3ResolvedDependencyType {
Token = 0,
Attribute = 1,
ChangeDetectorRef = 2,
Invalid = 3,
}

export enum R3FactoryTarget {
Expand Down
12 changes: 10 additions & 2 deletions packages/compiler/src/render3/r3_factory.ts
Expand Up @@ -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,
}

/**
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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]}`);
Expand Down
1 change: 1 addition & 0 deletions packages/compiler/src/render3/r3_identifiers.ts
Expand Up @@ -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};
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/compiler/compiler_facade_interface.ts
Expand Up @@ -67,6 +67,7 @@ export enum R3ResolvedDependencyType {
Token = 0,
Attribute = 1,
ChangeDetectorRef = 2,
Invalid = 3,
}

export enum R3FactoryTarget {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/di/index.ts
Expand Up @@ -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';
Expand Down
22 changes: 22 additions & 0 deletions packages/core/src/di/injector_compatibility.ts
Expand Up @@ -114,6 +114,28 @@ export function ɵɵinject<T>(token: Type<T>| InjectionToken<T>, 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.
*
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/di/jit/environment.ts
Expand Up @@ -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';


Expand All @@ -23,6 +23,7 @@ export const angularCoreDiEnv: {[name: string]: Function} = {
'ɵɵdefineInjector': ɵɵdefineInjector,
'ɵɵinject': ɵɵinject,
'ɵɵgetFactoryOf': getFactoryOf,
'ɵɵinvalidFactoryDep': ɵɵinvalidFactoryDep,
};

function getFactoryOf<T>(type: Type<any>): ((type?: Type<T>) => T)|null {
Expand Down
10 changes: 5 additions & 5 deletions packages/core/src/di/jit/util.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/render3/jit/environment.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down
26 changes: 26 additions & 0 deletions packages/core/test/render3/ivy/jit_spec.ts
Expand Up @@ -342,6 +342,32 @@ 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('ensure at least one spec exists', () => {});
2 changes: 2 additions & 0 deletions tools/public_api_guard/core/core.d.ts
Expand Up @@ -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): TsickleIssue1009;

export declare function ɵɵloadQuery<T>(): QueryList<T>;
Expand Down

0 comments on commit 2f8a0ec

Please sign in to comment.