refactor(migrations): add internal cleanup logic (#57315)

Expands the `inject` migration to add some cleanups that are only relevant internally. Externally this isn't exposed to users.

PR Close #57315
This commit is contained in:
Kristiyan Kostadinov 2024-08-09 12:51:54 +02:00 committed by Andrew Kushnir
parent 296216cbe1
commit cab6c23602
4 changed files with 676 additions and 16 deletions

View file

@ -9,7 +9,6 @@
import ts from 'typescript';
import {getAngularDecorators} from '../../utils/ng_decorators';
import {getNamedImports} from '../../utils/typescript/imports';
import {isReferenceToImport} from '../../utils/typescript/symbol';
/** Names of decorators that enable DI on a class declaration. */
const DECORATORS_SUPPORTING_DI = new Set([
@ -124,10 +123,12 @@ export function analyzeFile(sourceFile: ts.SourceFile, localTypeChecker: ts.Type
* Returns the parameters of a function that aren't used within its body.
* @param declaration Function in which to search for unused parameters.
* @param localTypeChecker Type checker scoped to the file in which the function was declared.
* @param removedStatements Statements that were already removed from the constructor.
*/
export function getConstructorUnusedParameters(
declaration: ts.ConstructorDeclaration,
localTypeChecker: ts.TypeChecker,
removedStatements: Set<ts.Statement> | null,
): Set<ts.Declaration> {
const accessedTopLevelParameters = new Set<ts.Declaration>();
const topLevelParameters = new Set<ts.Declaration>();
@ -147,6 +148,11 @@ export function getConstructorUnusedParameters(
}
declaration.body.forEachChild(function walk(node) {
// Don't descend into statements that were removed already.
if (removedStatements && ts.isStatement(node) && removedStatements.has(node)) {
return;
}
if (!ts.isIdentifier(node) || !topLevelParameterNames.has(node.text)) {
node.forEachChild(walk);
return;
@ -154,11 +160,7 @@ export function getConstructorUnusedParameters(
// Don't consider `this.<name>` accesses as being references to
// parameters since they'll be moved to property declarations.
if (
ts.isPropertyAccessExpression(node.parent) &&
node.parent.expression.kind === ts.SyntaxKind.ThisKeyword &&
node.parent.name === node
) {
if (isAccessedViaThis(node)) {
return;
}
@ -287,6 +289,15 @@ export function hasGenerics(node: ts.TypeNode): boolean {
return false;
}
/** Checks whether an identifier is accessed through `this`, e.g. `this.<some identifier>`. */
export function isAccessedViaThis(node: ts.Identifier): boolean {
return (
ts.isPropertyAccessExpression(node.parent) &&
node.parent.expression.kind === ts.SyntaxKind.ThisKeyword &&
node.parent.name === node
);
}
/** Finds a `super` call inside of a specific node. */
function findSuperCall(root: ts.Node): ts.CallExpression | null {
let result: ts.CallExpression | null = null;

View file

@ -0,0 +1,204 @@
/*!
* @license
* Copyright Google LLC 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 ts from 'typescript';
import {isAccessedViaThis} from './analysis';
/**
* Finds class property declarations without initializers whose constructor-based initialization
* can be inlined into the declaration spot after migrating to `inject`. For example:
*
* ```
* private foo: number;
*
* constructor(private service: MyService) {
* this.foo = this.service.getFoo();
* }
* ```
*
* The initializer of `foo` can be inlined, because `service` will be initialized
* before it after the `inject` migration has finished running.
*
* @param node Class declaration that is being migrated.
* @param constructor Constructor declaration of the class being migrated.
* @param localTypeChecker Type checker scoped to the current file.
*/
export function findUninitializedPropertiesToCombine(
node: ts.ClassDeclaration,
constructor: ts.ConstructorDeclaration,
localTypeChecker: ts.TypeChecker,
): Map<ts.PropertyDeclaration, ts.Expression> | null {
let result: Map<ts.PropertyDeclaration, ts.Expression> | null = null;
const membersToDeclarations = new Map<string, ts.PropertyDeclaration>();
for (const member of node.members) {
if (
ts.isPropertyDeclaration(member) &&
!member.initializer &&
!ts.isComputedPropertyName(member.name)
) {
membersToDeclarations.set(member.name.text, member);
}
}
if (membersToDeclarations.size === 0) {
return result;
}
const memberInitializers = getMemberInitializers(constructor);
if (memberInitializers === null) {
return result;
}
for (const [name, initializer] of memberInitializers.entries()) {
if (
membersToDeclarations.has(name) &&
!hasLocalReferences(initializer, constructor, localTypeChecker)
) {
result = result || new Map();
result.set(membersToDeclarations.get(name)!, initializer);
}
}
return result;
}
/**
* Finds the expressions from the constructor that initialize class members, for example:
*
* ```
* private foo: number;
*
* constructor() {
* this.foo = 123;
* }
* ```
*
* @param constructor Constructor declaration being analyzed.
*/
function getMemberInitializers(constructor: ts.ConstructorDeclaration) {
let memberInitializers: Map<string, ts.Expression> | null = null;
if (!constructor.body) {
return memberInitializers;
}
// Only look at top-level constructor statements.
for (const node of constructor.body.statements) {
// Only look for statements in the form of `this.<name> = <expr>;` or `this[<name>] = <expr>;`.
if (
!ts.isExpressionStatement(node) ||
!ts.isBinaryExpression(node.expression) ||
node.expression.operatorToken.kind !== ts.SyntaxKind.EqualsToken ||
(!ts.isPropertyAccessExpression(node.expression.left) &&
!ts.isElementAccessExpression(node.expression.left)) ||
node.expression.left.expression.kind !== ts.SyntaxKind.ThisKeyword
) {
continue;
}
let name: string | undefined;
if (ts.isPropertyAccessExpression(node.expression.left)) {
name = node.expression.left.name.text;
} else if (ts.isElementAccessExpression(node.expression.left)) {
name = ts.isStringLiteralLike(node.expression.left.argumentExpression)
? node.expression.left.argumentExpression.text
: undefined;
}
// If the member is initialized multiple times, take the first one.
if (name && (!memberInitializers || !memberInitializers.has(name))) {
memberInitializers = memberInitializers || new Map();
memberInitializers.set(name, node.expression.right);
}
}
return memberInitializers;
}
/**
* Determines if a node has references to local symbols defined in the constructor.
* @param root Expression to check for local references.
* @param constructor Constructor within which the expression is used.
* @param localTypeChecker Type checker scoped to the current file.
*/
function hasLocalReferences(
root: ts.Expression,
constructor: ts.ConstructorDeclaration,
localTypeChecker: ts.TypeChecker,
): boolean {
const sourceFile = root.getSourceFile();
let hasLocalRefs = false;
root.forEachChild(function walk(node) {
// Stop searching if we know that it has local references.
if (hasLocalRefs) {
return;
}
// Skip identifiers that are accessed via `this` since they're accessing class members
// that aren't local to the constructor. This is here primarily to catch cases like this
// where `foo` is defined inside the constructor, but is a class member:
// ```
// constructor(private foo: Foo) {
// this.bar = this.foo.getFoo();
// }
// ```
if (ts.isIdentifier(node) && !isAccessedViaThis(node)) {
const declarations = localTypeChecker.getSymbolAtLocation(node)?.declarations;
const isReferencingLocalSymbol = declarations?.some(
(decl) =>
// The source file check is a bit redundant since the type checker
// is local to the file, but it's inexpensive and it can prevent
// bugs in the future if we decide to use a full type checker.
decl.getSourceFile() === sourceFile &&
decl.getStart() >= constructor.getStart() &&
decl.getEnd() <= constructor.getEnd() &&
!isInsideInlineFunction(decl, constructor),
);
if (isReferencingLocalSymbol) {
hasLocalRefs = true;
}
}
if (!hasLocalRefs) {
node.forEachChild(walk);
}
});
return hasLocalRefs;
}
/**
* Determines if a node is defined inside of an inline function.
* @param startNode Node from which to start checking for inline functions.
* @param boundary Node at which to stop searching.
*/
function isInsideInlineFunction(startNode: ts.Node, boundary: ts.Node): boolean {
let current = startNode;
while (current) {
if (current === boundary) {
return false;
}
if (
ts.isFunctionDeclaration(current) ||
ts.isFunctionExpression(current) ||
ts.isArrowFunction(current)
) {
return true;
}
current = current.parent;
}
return false;
}

View file

@ -20,6 +20,8 @@ import {
} from './analysis';
import {getAngularDecorators} from '../../utils/ng_decorators';
import {getImportOfIdentifier} from '../../utils/typescript/imports';
import {closestNode} from '../../utils/typescript/nodes';
import {findUninitializedPropertiesToCombine} from './internal';
/**
* Placeholder used to represent expressions inside the AST.
@ -30,13 +32,32 @@ const PLACEHOLDER = 'ɵɵngGeneratePlaceholderɵɵ';
/** Options that can be used to configure the migration. */
export interface MigrationOptions {
/** Whether to generate code that keeps injectors backwards compatible. */
backwardsCompatibleConstructors: boolean;
backwardsCompatibleConstructors?: boolean;
/** Whether to migrate abstract classes. */
migrateAbstractClasses: boolean;
migrateAbstractClasses?: boolean;
/** Whether to make the return type of `@Optinal()` parameters to be non-nullable. */
nonNullableOptional: boolean;
nonNullableOptional?: boolean;
/**
* Internal-only option that determines whether the migration should try to move the
* initializers of class members from the constructor back into the member itself. E.g.
*
* ```
* // Before
* private foo;
*
* constructor(@Inject(BAR) private bar: Bar) {
* this.foo = this.bar.getValue();
* }
*
* // After
* private bar = inject(BAR);
* private foo = this.bar.getValue();
* ```
*/
_internalCombineMemberInitializers?: boolean;
}
/**
@ -60,12 +81,45 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
const printer = ts.createPrinter();
const tracker = new ChangeTracker(printer);
analysis.classes.forEach((result) => {
analysis.classes.forEach(({node, constructor, superCall}) => {
let removedStatements: Set<ts.Statement> | null = null;
if (options._internalCombineMemberInitializers) {
findUninitializedPropertiesToCombine(node, constructor, localTypeChecker)?.forEach(
(initializer, property) => {
const statement = closestNode(initializer, ts.isStatement);
if (!statement) {
return;
}
const newProperty = ts.factory.updatePropertyDeclaration(
property,
property.modifiers,
property.name,
property.questionToken,
property.type,
initializer,
);
tracker.replaceText(
statement.getSourceFile(),
statement.getFullStart(),
statement.getFullWidth(),
'',
);
tracker.replaceNode(property, newProperty);
removedStatements = removedStatements || new Set();
removedStatements.add(statement);
},
);
}
migrateClass(
result.node,
result.constructor,
result.superCall,
node,
constructor,
superCall,
options,
removedStatements,
localTypeChecker,
printer,
tracker,
@ -88,6 +142,7 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
* @param constructor Reference to the class' constructor node.
* @param superCall Reference to the constructor's `super()` call, if any.
* @param options Options used to configure the migration.
* @param removedStatements Statements that have been removed from the constructor already.
* @param localTypeChecker Type checker set up for the specific file.
* @param printer Printer used to output AST nodes as strings.
* @param tracker Object keeping track of the changes made to the file.
@ -97,6 +152,7 @@ function migrateClass(
constructor: ts.ConstructorDeclaration,
superCall: ts.CallExpression | null,
options: MigrationOptions,
removedStatements: Set<ts.Statement> | null,
localTypeChecker: ts.TypeChecker,
printer: ts.Printer,
tracker: ChangeTracker,
@ -110,12 +166,20 @@ function migrateClass(
}
const sourceFile = node.getSourceFile();
const unusedParameters = getConstructorUnusedParameters(constructor, localTypeChecker);
const unusedParameters = getConstructorUnusedParameters(
constructor,
localTypeChecker,
removedStatements,
);
const superParameters = superCall
? getSuperParameters(constructor, superCall, localTypeChecker)
: null;
const memberIndentation = getNodeIndentation(node.members[0]);
const innerReference = superCall || constructor.body?.statements[0] || constructor;
const removedStatementCount = removedStatements?.size || 0;
const innerReference =
superCall ||
constructor.body?.statements.find((statement) => !removedStatements?.has(statement)) ||
constructor;
const innerIndentation = getNodeIndentation(innerReference);
const propsToAdd: string[] = [];
const prependToConstructor: string[] = [];
@ -154,7 +218,7 @@ function migrateClass(
if (
!options.backwardsCompatibleConstructors &&
(!constructor.body || constructor.body.statements.length === 0)
(!constructor.body || constructor.body.statements.length - removedStatementCount === 0)
) {
// Drop the constructor if it was empty.
removedMembers.add(constructor);

View file

@ -29,6 +29,7 @@ describe('inject migration', () => {
backwardsCompatibleConstructors?: boolean;
migrateAbstractClasses?: boolean;
nonNullableOptional?: boolean;
_internalCombineMemberInitializers?: boolean;
}) {
return runner.runSchematic('inject-migration', options, tree);
}
@ -1339,4 +1340,384 @@ describe('inject migration', () => {
`}`,
]);
});
describe('internal-only behavior', () => {
function runInternalMigration() {
return runMigration({_internalCombineMemberInitializers: true});
}
it('should inline initializers that depend on DI', async () => {
writeFile(
'/dir.ts',
[
`import { Directive, Inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
`import { BAR_TOKEN, Bar } from './bar';`,
``,
`@Directive()`,
`class MyDir {`,
` private value: number;`,
` private otherValue: string;`,
``,
` constructor(private foo: Foo, @Inject(BAR_TOKEN) readonly bar: Bar) {`,
` this.value = this.foo.getValue();`,
` this.otherValue = this.bar.getOtherValue();`,
` }`,
`}`,
].join('\n'),
);
await runInternalMigration();
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
`import { BAR_TOKEN, Bar } from './bar';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo);`,
` readonly bar = inject(BAR_TOKEN);`,
``,
` private value: number = this.foo.getValue();`,
` private otherValue: string = this.bar.getOtherValue();`,
`}`,
]);
});
it('should not inline initializers that access injected parameters without `this`', async () => {
writeFile(
'/dir.ts',
[
`import { Directive, Inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
`import { BAR_TOKEN, Bar } from './bar';`,
``,
`@Directive()`,
`class MyDir {`,
` private value: number;`,
` private otherValue: string;`,
``,
` constructor(private foo: Foo, @Inject(BAR_TOKEN) readonly bar: Bar) {`,
` this.value = this.foo.getValue();`,
` this.otherValue = bar.getOtherValue();`,
` }`,
`}`,
].join('\n'),
);
await runInternalMigration();
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
`import { BAR_TOKEN, Bar } from './bar';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo);`,
` readonly bar = inject(BAR_TOKEN);`,
``,
` private value: number = this.foo.getValue();`,
` private otherValue: string;`,
``,
` constructor() {`,
` const bar = this.bar;`,
``,
` this.otherValue = bar.getOtherValue();`,
` }`,
`}`,
]);
});
it('should not inline initializers that depend on local symbols from the constructor', async () => {
writeFile(
'/dir.ts',
[
`import { Directive } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private value: number;`,
``,
` constructor(private foo: Foo) {`,
` const val = 456;`,
` this.value = this.foo.getValue([123, [val]]);`,
` }`,
`}`,
].join('\n'),
);
await runInternalMigration();
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo);`,
``,
` private value: number;`,
``,
` constructor() {`,
` const val = 456;`,
` this.value = this.foo.getValue([123, [val]]);`,
` }`,
`}`,
]);
});
it('should inline initializers that depend on local symbols defined outside of the constructor', async () => {
writeFile(
'/dir.ts',
[
`import { Directive } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`const val = 456;`,
``,
`@Directive()`,
`class MyDir {`,
` private value: number;`,
``,
` constructor(private foo: Foo) {`,
` this.value = this.getValue(this.foo, extra);`,
` }`,
``,
` private getValue(foo: Foo, extra: number) {`,
` return foo.getValue([123, [extra]]);`,
` }`,
`}`,
].join('\n'),
);
await runInternalMigration();
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`const val = 456;`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo);`,
``,
` private value: number = this.getValue(this.foo, extra);`,
``,
` private getValue(foo: Foo, extra: number) {`,
` return foo.getValue([123, [extra]]);`,
` }`,
`}`,
]);
});
it('should inline initializers defined through an element access', async () => {
writeFile(
'/dir.ts',
[
`import { Directive, Inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
`import { BAR_TOKEN, Bar } from './bar';`,
``,
`@Directive()`,
`class MyDir {`,
` private 'my-value': number;`,
` private 'my-other-value': string;`,
``,
` constructor(private foo: Foo, @Inject(BAR_TOKEN) readonly bar: Bar) {`,
` this['my-value'] = this.foo.getValue();`,
` this['my-other-value'] = this.bar.getOtherValue();`,
` }`,
`}`,
].join('\n'),
);
await runInternalMigration();
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
`import { BAR_TOKEN, Bar } from './bar';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo);`,
` readonly bar = inject(BAR_TOKEN);`,
``,
` private 'my-value': number = this.foo.getValue();`,
` private 'my-other-value': string = this.bar.getOtherValue();`,
`}`,
]);
});
it('should take the first initializer for properties initialized multiple times', async () => {
writeFile(
'/dir.ts',
[
`import { Directive } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private value: number;`,
``,
` constructor(private foo: Foo) {`,
` this.value = this.foo.getValue();`,
``,
` this.value = this.foo.getOtherValue();`,
` }`,
`}`,
].join('\n'),
);
await runInternalMigration();
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo);`,
``,
` private value: number = this.foo.getValue();`,
``,
` constructor() {`,
``,
` this.value = this.foo.getOtherValue();`,
` }`,
`}`,
]);
});
it('should not inline initializers that are not at the top level', async () => {
writeFile(
'/dir.ts',
[
`import { Directive, Optional } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private value: number;`,
``,
` constructor(@Optional() private foo: Foo | null) {`,
` if (this.foo) {`,
` this.value = this.foo.getValue();`,
` }`,
` }`,
`}`,
].join('\n'),
);
await runInternalMigration();
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo, { optional: true });`,
``,
` private value: number;`,
``,
` constructor() {`,
` if (this.foo) {`,
` this.value = this.foo.getValue();`,
` }`,
` }`,
`}`,
]);
});
it('should inline initializers that have expressions using local parameters', async () => {
writeFile(
'/dir.ts',
[
`import { Directive } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private ids: number[];`,
` private names: string[];`,
``,
` constructor(private foo: Foo) {`,
` this.ids = this.foo.getValue().map(val => val.id);`,
` this.names = this.foo.getValue().map(function(current) { return current.name; });`,
` }`,
`}`,
].join('\n'),
);
await runInternalMigration();
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo);`,
``,
` private ids: number[] = this.foo.getValue().map(val => val.id);`,
` private names: string[] = this.foo.getValue().map(function (current) { return current.name; });`,
`}`,
]);
});
it('should inline initializers that have expressions using local variables', async () => {
writeFile(
'/dir.ts',
[
`import { Directive } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private ids: number[];`,
` private names: string[];`,
``,
` constructor(private foo: Foo) {`,
` this.ids = this.foo.getValue().map(val => {`,
` const id = val.id;`,
` return id;`,
` });`,
` this.names = this.foo.getValue().map(function(current) {`,
` const name = current.name;`,
` return name;`,
` });`,
` }`,
`}`,
].join('\n'),
);
await runInternalMigration();
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo);`,
``,
// The indentation of the closing braces here is slightly off,
// but it's not a problem because this code is internal-only.
` private ids: number[] = this.foo.getValue().map(val => {`,
` const id = val.id;`,
` return id;`,
`});`,
` private names: string[] = this.foo.getValue().map(function (current) {`,
` const name = current.name;`,
` return name;`,
`});`,
`}`,
]);
});
});
});