Skip to content
Permalink
Browse files

feat(core): missing-injectable migration should migrate empty object …

…literal providers (#33709)

In View Engine, providers which neither used `useValue`, `useClass`,
`useFactory` or `useExisting`, were interpreted differently.

e.g.

```
{provide: X} -> {provide: X, useValue: undefined}, // this is how it works in View Engine
{provide: X} -> {provide: X, useClass: X}, // this is how it works in Ivy
```

The missing-injectable migration should migrate such providers to the
explicit `useValue` provider. This ensures that there is no unexpected
behavioral change when updating to v9.

PR Close #33709
  • Loading branch information
devversion authored and alxhub committed Nov 14, 2019
1 parent 1b7aa05 commit b7c012f91b896e9aa63a8abe6d9e54ac168aa99e
@@ -34,9 +34,9 @@ class BaseProviderCase {
constructor(zone: NgZone) {}
}

export class ProvideCase extends BaseProviderCase {}
export class EmptyProviderLiteralCase {}

export class ProviderClass {}
export class ProviderClass extends BaseProviderCase {}

export class DontNeedCase {}

@@ -46,7 +46,7 @@ export class DirectiveCase {}
@NgModule({
providers: [
TypeCase,
{provide: ProvideCase},
{provide: EmptyProviderLiteralCase},
{provide: DontNeedCase, useValue: 0},
{provide: DontNeedCase, useFactory: () => null},
{provide: DontNeedCase, useExisting: TypeCase},
@@ -56,4 +56,3 @@ export class DirectiveCase {}
declarations: [ProvidersTestDirective, ProvidersTestComponent],
})
export class ProvidersTestModule {}

@@ -41,11 +41,10 @@ class BaseProviderCase {
constructor(zone: NgZone) {}
}

@Injectable()
export class ProvideCase extends BaseProviderCase {}
export class EmptyProviderLiteralCase {}

@Injectable()
export class ProviderClass {}
export class ProviderClass extends BaseProviderCase {}

export class DontNeedCase {}

@@ -55,7 +54,7 @@ export class DirectiveCase {}
@NgModule({
providers: [
TypeCase,
{provide: ProvideCase},
{ provide: EmptyProviderLiteralCase, useValue: undefined },
{provide: DontNeedCase, useValue: 0},
{provide: DontNeedCase, useFactory: () => null},
{provide: DontNeedCase, useExisting: TypeCase},
@@ -65,4 +64,3 @@ export class DirectiveCase {}
declarations: [ProvidersTestDirective, ProvidersTestComponent],
})
export class ProvidersTestModule {}

@@ -160,7 +160,7 @@ export class StaticInterpreter {
return array;
}

private visitObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: Context):
protected visitObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: Context):
ResolvedValue {
const map: ResolvedValueMap = new Map<string, ResolvedValue>();
for (let i = 0; i < node.properties.length; i++) {
@@ -54,5 +54,13 @@ export class TslintUpdateRecorder implements UpdateRecorder {
this.ruleName, fix));
}


updateObjectLiteral(node: ts.ObjectLiteralExpression, newText: string): void {
this.failures.push(new RuleFailure(
this.sourceFile, node.getStart(), node.getEnd(),
`Object literal needs to be updated to: ${newText}`, this.ruleName,
Replacement.replaceFromTo(node.getStart(), node.getEnd(), newText)));
}

commitUpdate() {}
}
@@ -107,6 +107,10 @@ function runMissingInjectableMigration(
treeRecorder.remove(namedBindings.getStart(), namedBindings.getWidth());
treeRecorder.insertRight(namedBindings.getStart(), newNamedBindings);
},
updateObjectLiteral(node: ts.ObjectLiteralExpression, newText: string) {
treeRecorder.remove(node.getStart(), node.getWidth());
treeRecorder.insertRight(node.getStart(), newText);
},
commitUpdate() { tree.commitUpdate(treeRecorder); }
};
updateRecorders.set(sourceFile, recorder);
@@ -0,0 +1,58 @@
/**
* @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 {forwardRefResolver} from '@angular/compiler-cli/src/ngtsc/annotations';
import {ResolvedValue} from '@angular/compiler-cli/src/ngtsc/partial_evaluator';
import {StaticInterpreter} from '@angular/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter';
import * as ts from 'typescript';

export interface ProviderLiteral {
node: ts.ObjectLiteralExpression;
resolvedValue: ResolvedValue;
}

/**
* Providers evaluator that extends the ngtsc static interpreter. This is necessary because
* the static interpreter by default only exposes the resolved value, but we are also interested
* in the TypeScript nodes that declare providers. It would be possible to manually traverse the
* AST to collect these nodes, but that would mean that we need to re-implement the static
* interpreter in order to handle all possible scenarios. (e.g. spread operator, function calls,
* callee scope). This can be avoided by simply extending the static interpreter and intercepting
* the "visitObjectLiteralExpression" method.
*/
export class ProvidersEvaluator extends StaticInterpreter {
private _providerLiterals: ProviderLiteral[] = [];

visitObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: any) {
const resolvedValue =
super.visitObjectLiteralExpression(node, {...context, insideProviderDef: true});
// do not collect nested object literals. e.g. a provider could use a
// spread assignment (which resolves to another object literal). In that
// case the referenced object literal is not a provider object literal.
if (!context.insideProviderDef) {
this._providerLiterals.push({node, resolvedValue});
}
return resolvedValue;
}

/**
* Evaluates the given expression and returns its statically resolved value
* and a list of object literals which define Angular providers.
*/
evaluate(expr: ts.Expression) {
this._providerLiterals = [];
const resolvedValue = this.visit(expr, {
originatingFile: expr.getSourceFile(),
absoluteModuleName: null,
resolutionContext: expr.getSourceFile().fileName,
scope: new Map(),
foreignFunctionResolver: forwardRefResolver
});
return {resolvedValue, literals: this._providerLiterals};
}
}
@@ -6,16 +6,16 @@
* found in the LICENSE file at https://angular.io/license
*/

import {forwardRefResolver} from '@angular/compiler-cli/src/ngtsc/annotations/src/util';
import {Reference} from '@angular/compiler-cli/src/ngtsc/imports';
import {DynamicValue, PartialEvaluator, ResolvedValue} from '@angular/compiler-cli/src/ngtsc/partial_evaluator';
import {DynamicValue, ResolvedValue} from '@angular/compiler-cli/src/ngtsc/partial_evaluator';
import {TypeScriptReflectionHost} from '@angular/compiler-cli/src/ngtsc/reflection';
import * as ts from 'typescript';

import {getAngularDecorators} from '../../utils/ng_decorators';

import {ResolvedDirective, ResolvedNgModule} from './definition_collector';
import {ImportManager} from './import_manager';
import {ProviderLiteral, ProvidersEvaluator} from './providers_evaluator';
import {UpdateRecorder} from './update_recorder';


@@ -30,16 +30,19 @@ export interface AnalysisFailure {
export class MissingInjectableTransform {
private printer = ts.createPrinter();
private importManager = new ImportManager(this.getUpdateRecorder, this.printer);
private partialEvaluator: PartialEvaluator;
private providersEvaluator: ProvidersEvaluator;

/** Set of provider class declarations which were already checked or migrated. */
private visitedProviderClasses = new Set<ts.ClassDeclaration>();

/** Set of provider object literals which were already checked or migrated. */
private visitedProviderLiterals = new Set<ts.ObjectLiteralExpression>();

constructor(
private typeChecker: ts.TypeChecker,
private getUpdateRecorder: (sf: ts.SourceFile) => UpdateRecorder) {
this.partialEvaluator =
new PartialEvaluator(new TypeScriptReflectionHost(typeChecker), typeChecker);
this.providersEvaluator =
new ProvidersEvaluator(new TypeScriptReflectionHost(typeChecker), typeChecker);
}

recordChanges() { this.importManager.recordChanges(); }
@@ -68,16 +71,17 @@ export class MissingInjectableTransform {
return [];
}

const evaluatedExpr = this._evaluateExpression(module.providersExpr);
const {resolvedValue, literals} = this.providersEvaluator.evaluate(module.providersExpr);
this._migrateLiteralProviders(literals);

if (!Array.isArray(evaluatedExpr)) {
if (!Array.isArray(resolvedValue)) {
return [{
node: module.providersExpr,
message: 'Providers of module are not statically analyzable.'
}];
}

return this._visitProviderResolvedValue(evaluatedExpr, module);
return this._visitProviderResolvedValue(resolvedValue, module);
}


@@ -90,24 +94,27 @@ export class MissingInjectableTransform {

// Migrate "providers" on directives and components if defined.
if (directive.providersExpr) {
const evaluatedExpr = this._evaluateExpression(directive.providersExpr);
if (!Array.isArray(evaluatedExpr)) {
const {resolvedValue, literals} = this.providersEvaluator.evaluate(directive.providersExpr);
this._migrateLiteralProviders(literals);
if (!Array.isArray(resolvedValue)) {
return [
{node: directive.providersExpr, message: `Providers are not statically analyzable.`}
];
}
failures.push(...this._visitProviderResolvedValue(evaluatedExpr, directive));
failures.push(...this._visitProviderResolvedValue(resolvedValue, directive));
}

// Migrate "viewProviders" on components if defined.
if (directive.viewProvidersExpr) {
const evaluatedExpr = this._evaluateExpression(directive.viewProvidersExpr);
if (!Array.isArray(evaluatedExpr)) {
const {resolvedValue, literals} =
this.providersEvaluator.evaluate(directive.viewProvidersExpr);
this._migrateLiteralProviders(literals);
if (!Array.isArray(resolvedValue)) {
return [
{node: directive.viewProvidersExpr, message: `Providers are not statically analyzable.`}
];
}
failures.push(...this._visitProviderResolvedValue(evaluatedExpr, directive));
failures.push(...this._visitProviderResolvedValue(resolvedValue, directive));
}
return failures;
}
@@ -160,11 +167,39 @@ export class MissingInjectableTransform {
}

/**
* Evaluates the given TypeScript expression using the partial evaluator with
* the foreign function resolver for handling "forwardRef" calls.
* Migrates object literal providers which do not use "useValue", "useClass",
* "useExisting" or "useFactory". These providers behave differently in Ivy. e.g.
*
* ```ts
* {provide: X} -> {provide: X, useValue: undefined} // this is how it behaves in VE
* {provide: X} -> {provide: X, useClass: X} // this is how it behaves in Ivy
* ```
*
* To ensure forward compatibility, we migrate these empty object literal providers
* to explicitly use `useValue: undefined`.
*/
private _evaluateExpression(expr: ts.Expression): ResolvedValue {
return this.partialEvaluator.evaluate(expr, forwardRefResolver);
private _migrateLiteralProviders(literals: ProviderLiteral[]) {
for (let {node, resolvedValue} of literals) {
if (this.visitedProviderLiterals.has(node)) {
continue;
}
this.visitedProviderLiterals.add(node);

if (!resolvedValue || !(resolvedValue instanceof Map) || !resolvedValue.has('provide') ||
resolvedValue.has('useClass') || resolvedValue.has('useValue') ||
resolvedValue.has('useExisting') || resolvedValue.has('useFactory')) {
continue;
}

const sourceFile = node.getSourceFile();
const newObjectLiteral = ts.updateObjectLiteral(
node, node.properties.concat(
ts.createPropertyAssignment('useValue', ts.createIdentifier('undefined'))));

this.getUpdateRecorder(sourceFile)
.updateObjectLiteral(
node, this.printer.printNode(ts.EmitHint.Unspecified, newObjectLiteral, sourceFile));
}
}

/**
@@ -177,14 +212,11 @@ export class MissingInjectableTransform {
if (value instanceof Reference && ts.isClassDeclaration(value.node)) {
this.migrateProviderClass(value.node, module);
} else if (value instanceof Map) {
if (!value.has('provide') || value.has('useValue') || value.has('useFactory') ||
value.has('useExisting')) {
return [];
}
if (value.has('useClass')) {
// If a "ClassProvider" has the "deps" property set, then we do not need to
// decorate the class. This is because the class is instantiated through the
// specified "deps" and the class does not need a factory definition.
if (value.has('provide') && value.has('useClass') && value.get('deps') == null) {
return this._visitProviderResolvedValue(value.get('useClass') !, module);
} else {
return this._visitProviderResolvedValue(value.get('provide') !, module);
}
} else if (Array.isArray(value)) {
return value.reduce((res, v) => res.concat(this._visitProviderResolvedValue(v, module)), [
@@ -18,5 +18,6 @@ export interface UpdateRecorder {
updateExistingImport(namedBindings: ts.NamedImports, newNamedBindings: string): void;
addClassDecorator(node: ts.ClassDeclaration, text: string, className: string): void;
replaceDecorator(node: ts.Decorator, newText: string, className: string): void;
updateObjectLiteral(node: ts.ObjectLiteralExpression, newText: string): void;
commitUpdate(): void;
}
@@ -121,6 +121,7 @@ describe('Google3 missing injectable tslint rule', () => {
export class MyServiceE {}
export class MyServiceF {}
export class MyServiceG {}
export class MyServiceH {}
@${type}({${propName}: [
WithPipe,
@@ -135,6 +136,7 @@ describe('Google3 missing injectable tslint rule', () => {
{provide: null, useExisting: MyServiceE},
{provide: MyServiceF, useFactory: () => null},
{provide: MyServiceG, useValue: null},
{provide: MyServiceH, deps: []},
]})
export class TestClass {}
`);
@@ -149,11 +151,13 @@ describe('Google3 missing injectable tslint rule', () => {
.toMatch(/WithDirective {}\s+@Component\(\)\s+export class WithComponent/);
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceA/);
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceB/);
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceC/);
expect(getFile('/index.ts')).toMatch(/MyServiceB {}\s+export class MyServiceC/);
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceD/);
expect(getFile('/index.ts')).toMatch(/MyServiceD {}\s+export class MyServiceE/);
expect(getFile('/index.ts')).toMatch(/MyServiceE {}\s+export class MyServiceF/);
expect(getFile('/index.ts')).toMatch(/MyServiceF {}\s+export class MyServiceG/);
expect(getFile('/index.ts')).toMatch(/MyServiceG {}\s+export class MyServiceH/);
expect(getFile('/index.ts')).toContain(`{ provide: MyServiceC, useValue: undefined },`);
});

it(`should migrate provider once if referenced in multiple ${type} definitions`, () => {

0 comments on commit b7c012f

Please sign in to comment.
You can’t perform that action at this time.