iSharkFly-Docs
/
angular-docs-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

feat(ivy): compile @Injectable on classes not meant for DI (#28523) 

In the past, @Injectable had no side effects and existing Angular code is
therefore littered with @Injectable usage on classes which are not intended
to be injected.

A common example is:

@Injectable()
class Foo {
  constructor(private notInjectable: string) {}
}

and somewhere else:

providers: [{provide: Foo, useFactory: ...})

Here, there is no need for Foo to be injectable - indeed, it's impossible
for the DI system to create an instance of it, as it has a non-injectable
constructor. The provider configures a factory for the DI system to be
able to create instances of Foo.

Adding @Injectable in Ivy signifies that the class's own constructor, and
not a provider, determines how the class will be created.

This commit adds logic to compile classes which are marked with @Injectable
but are otherwise not injectable, and create an ngInjectableDef field with
a factory function that throws an error. This way, existing code in the wild
continues to compile, but if someone attempts to use the injectable it will
fail with a useful error message.

In the case where strictInjectionParameters is set to true, a compile-time
error is thrown instead of the runtime error, as ngtsc has enough
information to determine when injection couldn't possibly be valid.

PR Close #28523
This commit is contained in:
Alex Rickabaugh 2019-01-31 14:23:54 -08:00 committed by Misko Hevery
parent f8b67712bc
commit d2742cf473
12 changed files with 264 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts
View File

@ -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),

4
packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
View File

@ -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,
},

73
packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts
View File

@ -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<InjectableHandlerData, Decorator> {
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<InjectableHandlerData> {
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 {

4
packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
View File

@ -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<NgModuleAnalys
const ngInjectorDef: R3InjectorMetadata = {
name: node.name !.text,
type: new WrappedNodeExpr(node.name !),
deps: getConstructorDependencies(node, this.reflector, this.isCore), providers,
deps: getValidConstructorDependencies(node, this.reflector, this.isCore), providers,
imports: new LiteralArrayExpr(injectorImports),
};

4
packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts
View File

@ -16,7 +16,7 @@ import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform';
import {generateSetClassMetadataCall} from './metadata';
import {SelectorScopeRegistry} from './selector_scope';
import {getConstructorDependencies, isAngularCore, unwrapExpression} from './util';
import {getValidConstructorDependencies, isAngularCore, unwrapExpression} from './util';
export interface PipeHandlerData {
meta: R3PipeMetadata;
@ -87,7 +87,7 @@ export class PipeDecoratorHandler implements DecoratorHandler<PipeHandlerData, D
name,
type,
pipeName,
deps: getConstructorDependencies(clazz, this.reflector, this.isCore), pure,
deps: getValidConstructorDependencies(clazz, this.reflector, this.isCore), pure,
},
metadataStmt: generateSetClassMetadataCall(clazz, this.reflector, this.isCore),
},

67
packages/compiler-cli/src/ngtsc/annotations/src/util.ts
View File

@ -11,12 +11,30 @@ import * as ts from 'typescript';
import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
import {AbsoluteReference, ImportMode, Reference} from '../../imports';
import {ClassMemberKind, Decorator, ReflectionHost} from '../../reflection';
import {ClassMemberKind, CtorParameter, Decorator, ReflectionHost} from '../../reflection';
export enum ConstructorDepErrorKind {
NO_SUITABLE_TOKEN,
}
export type ConstructorDeps = {
deps: R3DependencyMetadata[];
} |
{
deps: null;
errors: ConstructorDepError[];
};
export interface ConstructorDepError {
index: number;
param: CtorParameter;
kind: ConstructorDepErrorKind;
}
export function getConstructorDependencies(
clazz: ts.ClassDeclaration, reflector: ReflectionHost, isCore: boolean): R3DependencyMetadata[]|
null {
const useType: R3DependencyMetadata[] = [];
clazz: ts.ClassDeclaration, reflector: ReflectionHost, isCore: boolean): ConstructorDeps|null {
const deps: R3DependencyMetadata[] = [];
const errors: ConstructorDepError[] = [];
let ctorParams = reflector.getConstructorParameters(clazz);
if (ctorParams === null) {
if (reflector.hasBaseClass(clazz)) {
@ -60,14 +78,43 @@ export function getConstructorDependencies(
}
});
if (tokenExpr === null) {
throw new FatalDiagnosticError(
ErrorCode.PARAM_MISSING_TOKEN, param.nameNode,
`No suitable injection token for parameter '${param.name || idx}' of class '${clazz.name!.text}'. Found: ${param.typeNode!.getText()}`);
errors.push({
index: idx,
kind: ConstructorDepErrorKind.NO_SUITABLE_TOKEN, param,
});
} else {
const token = new WrappedNodeExpr(tokenExpr);
deps.push({token, optional, self, skipSelf, host, resolved});
}
const token = new WrappedNodeExpr(tokenExpr);
useType.push({token, optional, self, skipSelf, host, resolved});
});
return useType;
if (errors.length === 0) {
return {deps};
} else {
return {deps: null, errors};
}
}
export function getValidConstructorDependencies(
clazz: ts.ClassDeclaration, reflector: ReflectionHost, isCore: boolean): R3DependencyMetadata[]|
null {
return validateConstructorDependencies(
clazz, getConstructorDependencies(clazz, reflector, isCore));
}
export function validateConstructorDependencies(
clazz: ts.ClassDeclaration, deps: ConstructorDeps | null): R3DependencyMetadata[]|null {
if (deps === null) {
return null;
} else if (deps.deps !== null) {
return deps.deps;
} else {
// TODO(alxhub): this cast is necessary because the g3 typescript version doesn't narrow here.
const {param, index} = (deps as{errors: ConstructorDepError[]}).errors[0];
// There is at least one error.
throw new FatalDiagnosticError(
ErrorCode.PARAM_MISSING_TOKEN, param.nameNode,
`No suitable injection token for parameter '${param.name || index}' of class '${clazz.name!.text}'. Found: ${param.typeNode!.getText()}`);
}
}
export function toR3Reference(

3
packages/compiler-cli/src/ngtsc/program.ts
View File

@ -340,7 +340,8 @@ export class NgtscProgram implements api.Program {
this.rootDirs, this.options.preserveWhitespaces || false,
this.options.i18nUseExternalIds !== false, this.moduleResolver, this.cycleAnalyzer),
new DirectiveDecoratorHandler(this.reflector, evaluator, scopeRegistry, this.isCore),
new InjectableDecoratorHandler(this.reflector, this.isCore),
new InjectableDecoratorHandler(
this.reflector, this.isCore, this.options.strictInjectionParameters || false),
new NgModuleDecoratorHandler(
this.reflector, evaluator, scopeRegistry, referencesRegistry, this.isCore,
this.routeAnalyzer),

4
packages/compiler-cli/src/ngtsc/translator/src/translator.ts
View File

@ -139,7 +139,9 @@ class ExpressionTranslatorVisitor implements ExpressionVisitor, StatementVisitor
throw new Error('Method not implemented.');
}
visitThrowStmt(stmt: ThrowStmt, context: Context) { throw new Error('Method not implemented.'); }
visitThrowStmt(stmt: ThrowStmt, context: Context): ts.ThrowStatement {
return ts.createThrow(stmt.error.visitExpression(this, context.withExpressionMode));
}
visitCommentStmt(stmt: CommentStmt, context: Context): never {
throw new Error('Method not implemented.');

93
packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
View File

@ -624,6 +624,99 @@ describe('ngtsc behavioral tests', () => {
'i0.ɵNgModuleDefWithMeta<TestModule, [typeof TestPipe, typeof TestCmp], never, never>');
});
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();

13
packages/compiler/src/injectable_compiler_2.ts
View File

@ -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,

35
packages/compiler/src/render3/r3_factory.ts
View File

@ -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;

2
packages/core/src/di/jit/injectable.ts
View File

@ -43,7 +43,7 @@ export function compileInjectable(type: Type<any>, 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);