From b144126612e2cd14cbccc8d3cf4e2136a2e540ff Mon Sep 17 00:00:00 2001 From: Ryan Russell Date: Wed, 2 Apr 2025 19:09:28 +0000 Subject: [PATCH] fix(core): inject migration: replace param with this. (#60713) The inject tool inserts `const foo = this.foo` if code in the constructor referenced the constructor parameter `foo`. If `foo` is a readonly property, we can instead replace `foo` with `this.foo`. This allows more properties to be moved out of the constructor with combineMemberInitializers. For now, it only touches initializers, not all of the code in the constructor. PR Close #60713 --- .../ng-generate/inject-migration/analysis.ts | 21 ++ .../ng-generate/inject-migration/internal.ts | 97 ++++++++- .../ng-generate/inject-migration/migration.ts | 98 ++++++--- .../schematics/test/inject_migration_spec.ts | 190 +++++++++++++++--- 4 files changed, 355 insertions(+), 51 deletions(-) diff --git a/packages/core/schematics/ng-generate/inject-migration/analysis.ts b/packages/core/schematics/ng-generate/inject-migration/analysis.ts index ec3ced79b18..4706a1b7431 100644 --- a/packages/core/schematics/ng-generate/inject-migration/analysis.ts +++ b/packages/core/schematics/ng-generate/inject-migration/analysis.ts @@ -40,6 +40,27 @@ export interface MigrationOptions { * ``` */ _internalCombineMemberInitializers?: boolean; + + /** + * Internal-only option that determines whether the migration should + * replace constructor parameter references with `this.param` property + * references. Only applies to references to readonly properties in + * initializers. + * + * ``` + * // Before + * private foo; + * + * constructor(readonly service: Service) { + * this.foo = service.getFoo(); + * } + * + * // After + * readonly service = inject(Service); + * private foo = this.service.getFoo(); + * ``` + */ + _internalReplaceParameterReferencesInInitializers?: boolean; } /** Names of decorators that enable DI on a class declaration. */ diff --git a/packages/core/schematics/ng-generate/inject-migration/internal.ts b/packages/core/schematics/ng-generate/inject-migration/internal.ts index 095b733e4f2..47ff64f06df 100644 --- a/packages/core/schematics/ng-generate/inject-migration/internal.ts +++ b/packages/core/schematics/ng-generate/inject-migration/internal.ts @@ -7,7 +7,12 @@ */ import ts from 'typescript'; -import {isAccessedViaThis, isInlineFunction, parameterDeclaresProperty} from './analysis'; +import { + isAccessedViaThis, + isInlineFunction, + MigrationOptions, + parameterDeclaresProperty, +} from './analysis'; /** Property that is a candidate to be combined. */ interface CombineCandidate { @@ -40,6 +45,7 @@ export function findUninitializedPropertiesToCombine( node: ts.ClassDeclaration, constructor: ts.ConstructorDeclaration, localTypeChecker: ts.TypeChecker, + options: MigrationOptions, ): { toCombine: CombineCandidate[]; toHoist: ts.PropertyDeclaration[]; @@ -67,11 +73,15 @@ export function findUninitializedPropertiesToCombine( return null; } + const inlinableParameters = options._internalReplaceParameterReferencesInInitializers + ? findInlinableParameterReferences(constructor, localTypeChecker) + : new Set(); + for (const [name, decl] of membersToDeclarations.entries()) { if (memberInitializers.has(name)) { const initializer = memberInitializers.get(name)!; - if (!hasLocalReferences(initializer, constructor, localTypeChecker)) { + if (!hasLocalReferences(initializer, constructor, inlinableParameters, localTypeChecker)) { toCombine ??= []; toCombine.push({declaration: membersToDeclarations.get(name)!, initializer}); } @@ -230,6 +240,87 @@ function getMemberInitializers(constructor: ts.ConstructorDeclaration) { return memberInitializers; } +/** + * Checks if the node is an identifier that references a property from the given + * list. Returns the property if it is. + */ +function getIdentifierReferencingProperty( + node: ts.Node, + localTypeChecker: ts.TypeChecker, + propertyNames: Set, + properties: Set, +): ts.ParameterDeclaration | undefined { + if (!ts.isIdentifier(node) || !propertyNames.has(node.text)) { + return undefined; + } + const declarations = localTypeChecker.getSymbolAtLocation(node)?.declarations; + if (!declarations) { + return undefined; + } + + for (const decl of declarations) { + if (properties.has(decl)) { + return decl as ts.ParameterDeclaration; + } + } + return undefined; +} + +/** + * Returns true if the node introduces a new `this` scope (so we can't + * reference the outer this). + */ +function introducesNewThisScope(node: ts.Node): boolean { + return ( + ts.isFunctionDeclaration(node) || + ts.isFunctionExpression(node) || + ts.isMethodDeclaration(node) || + ts.isClassDeclaration(node) || + ts.isClassExpression(node) + ); +} + +/** + * Finds constructor parameter references which can be inlined as `this.prop`. + * - prop must be a readonly property + * - the reference can't be in a nested function where `this` might refer + * to something else + */ +function findInlinableParameterReferences( + constructorDeclaration: ts.ConstructorDeclaration, + localTypeChecker: ts.TypeChecker, +): Set { + const eligibleProperties = constructorDeclaration.parameters.filter( + (p) => + ts.isIdentifier(p.name) && p.modifiers?.some((s) => s.kind === ts.SyntaxKind.ReadonlyKeyword), + ); + const eligibleNames = new Set(eligibleProperties.map((p) => (p.name as ts.Identifier).text)); + const eligiblePropertiesSet: Set = new Set(eligibleProperties); + + function walk(node: ts.Node, canReferenceThis: boolean) { + const property = getIdentifierReferencingProperty( + node, + localTypeChecker, + eligibleNames, + eligiblePropertiesSet, + ); + if (property && !canReferenceThis) { + // The property is referenced in a nested context where + // we can't use `this`, so we can't inline it. + eligiblePropertiesSet.delete(property); + } else if (introducesNewThisScope(node)) { + canReferenceThis = false; + } + + ts.forEachChild(node, (child) => { + walk(child, canReferenceThis); + }); + } + + walk(constructorDeclaration, true); + return eligiblePropertiesSet; +} + /** * Determines if a node has references to local symbols defined in the constructor. * @param root Expression to check for local references. @@ -239,6 +330,7 @@ function getMemberInitializers(constructor: ts.ConstructorDeclaration) { function hasLocalReferences( root: ts.Expression, constructor: ts.ConstructorDeclaration, + allowedParameters: Set, localTypeChecker: ts.TypeChecker, ): boolean { const sourceFile = root.getSourceFile(); @@ -265,6 +357,7 @@ function hasLocalReferences( // 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. + !allowedParameters.has(decl) && decl.getSourceFile() === sourceFile && decl.getStart() >= constructor.getStart() && decl.getEnd() <= constructor.getEnd() && diff --git a/packages/core/schematics/ng-generate/inject-migration/migration.ts b/packages/core/schematics/ng-generate/inject-migration/migration.ts index e1cf82f570f..73332574066 100644 --- a/packages/core/schematics/ng-generate/inject-migration/migration.ts +++ b/packages/core/schematics/ng-generate/inject-migration/migration.ts @@ -71,6 +71,7 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions prependToClass, afterInjectCalls, memberIndentation, + options, ); } @@ -755,8 +756,9 @@ function applyInternalOnlyChanges( prependToClass: string[], afterInjectCalls: string[], memberIndentation: string, + options: MigrationOptions, ) { - const result = findUninitializedPropertiesToCombine(node, constructor, localTypeChecker); + const result = findUninitializedPropertiesToCombine(node, constructor, localTypeChecker, options); if (result === null) { return; @@ -774,36 +776,46 @@ function applyInternalOnlyChanges( result.toCombine.forEach(({declaration, initializer}) => { const initializerStatement = closestNode(initializer, ts.isStatement); + // Strip comments if we are just going modify the node in-place. + const modifiers = preserveInitOrder + ? declaration.modifiers + : cloneModifiers(declaration.modifiers); + const name = preserveInitOrder ? declaration.name : cloneName(declaration.name); + + const newProperty = ts.factory.createPropertyDeclaration( + modifiers, + name, + declaration.questionToken, + declaration.type, + undefined, + ); + + const propText = printer.printNode( + ts.EmitHint.Unspecified, + newProperty, + declaration.getSourceFile(), + ); + const initializerText = replaceParameterReferencesInInitializer( + initializer, + constructor, + localTypeChecker, + ); + const withInitializer = `${propText.slice(0, -1)} = ${initializerText};`; + // If the initialization order is being preserved, we have to remove the original // declaration and re-declare it. Otherwise we can do the replacement in-place. if (preserveInitOrder) { - // Preserve comment in the new property since we are removing the entire node. - const newProperty = ts.factory.createPropertyDeclaration( - declaration.modifiers, - declaration.name, - declaration.questionToken, - declaration.type, - initializer, - ); - tracker.removeNode(declaration, true); removedMembers.add(declaration); - afterInjectCalls.push( - memberIndentation + - printer.printNode(ts.EmitHint.Unspecified, newProperty, declaration.getSourceFile()), - ); + afterInjectCalls.push(memberIndentation + withInitializer); } else { - // Strip comments from the declaration since we are replacing just - // the node, not the leading comment. - const newProperty = ts.factory.createPropertyDeclaration( - cloneModifiers(declaration.modifiers), - cloneName(declaration.name), - declaration.questionToken, - declaration.type, - initializer, + const sourceFile = declaration.getSourceFile(); + tracker.replaceText( + sourceFile, + declaration.getStart(), + declaration.getWidth(), + withInitializer, ); - - tracker.replaceNode(declaration, newProperty); } // This should always be defined, but null check it just in case. @@ -826,3 +838,41 @@ function applyInternalOnlyChanges( prependToClass.push(''); } } + +function replaceParameterReferencesInInitializer( + initializer: ts.Expression, + constructor: ts.ConstructorDeclaration, + localTypeChecker: ts.TypeChecker, +): string { + // 1. Collect the locations of identifier nodes that reference constructor parameters. + // 2. Add `this.` to those locations. + const insertLocations = [0]; + + function walk(node: ts.Node) { + if ( + ts.isIdentifier(node) && + !(ts.isPropertyAccessExpression(node.parent) && node === node.parent.name) && + localTypeChecker + .getSymbolAtLocation(node) + ?.declarations?.some((decl) => + constructor.parameters.includes(decl as ts.ParameterDeclaration), + ) + ) { + insertLocations.push(node.getStart() - initializer.getStart()); + } + ts.forEachChild(node, walk); + } + walk(initializer); + + const initializerText = initializer.getText(); + insertLocations.push(initializerText.length); + + insertLocations.sort((a, b) => a - b); + + const result: string[] = []; + for (let i = 0; i < insertLocations.length - 1; i++) { + result.push(initializerText.slice(insertLocations[i], insertLocations[i + 1])); + } + + return result.join('this.'); +} diff --git a/packages/core/schematics/test/inject_migration_spec.ts b/packages/core/schematics/test/inject_migration_spec.ts index 010a4a0c1e6..c80f8e2041c 100644 --- a/packages/core/schematics/test/inject_migration_spec.ts +++ b/packages/core/schematics/test/inject_migration_spec.ts @@ -30,6 +30,7 @@ describe('inject migration', () => { migrateAbstractClasses?: boolean; nonNullableOptional?: boolean; _internalCombineMemberInitializers?: boolean; + _internalReplaceParameterReferencesInInitializers?: boolean; }) { return runner.runSchematic('inject-migration', options, tree); } @@ -1875,8 +1876,13 @@ describe('inject migration', () => { }); describe('internal-only behavior', () => { - function runInternalMigration() { - return runMigration({_internalCombineMemberInitializers: true}); + function runInternalMigration( + {replaceParameterReferences} = {replaceParameterReferences: true}, + ) { + return runMigration({ + _internalCombineMemberInitializers: true, + _internalReplaceParameterReferencesInInitializers: replaceParameterReferences, + }); } it('should inline initializers that depend on DI', async () => { @@ -1918,7 +1924,7 @@ describe('inject migration', () => { ]); }); - it('should not inline initializers that access injected parameters without `this`', async () => { + it('should inline initializers that access injected parameters without `this` if possible', async () => { writeFile( '/dir.ts', [ @@ -1952,13 +1958,7 @@ describe('inject migration', () => { ` readonly bar = inject(BAR_TOKEN);`, ``, ` private value: number = this.foo.getValue();`, - ` private otherValue: string;`, - ``, - ` constructor() {`, - ` const bar = this.bar;`, - ``, - ` this.otherValue = bar.getOtherValue();`, - ` }`, + ` private otherValue: string = this.bar.getOtherValue();`, `}`, ]); }); @@ -2198,7 +2198,7 @@ describe('inject migration', () => { ` 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; });`, + ` private names: string[] = this.foo.getValue().map(function(current) { return current.name; });`, `}`, ]); }); @@ -2242,13 +2242,13 @@ describe('inject migration', () => { // 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;`, - `});`, + ` const id = val.id;`, + ` return id;`, + ` });`, + ` private names: string[] = this.foo.getValue().map(function(current) {`, + ` const name = current.name;`, + ` return name;`, + ` });`, `}`, ]); }); @@ -2506,13 +2506,7 @@ describe('inject migration', () => { `export class SomeService {`, ` readonly differentName = inject(OtherService);`, ``, - ` readonly otherService: OtherService;`, - ``, - ` constructor() {`, - ` const differentName = this.differentName;`, - ``, - ` this.otherService = differentName;`, - ` }`, + ` readonly otherService: OtherService = this.differentName;`, `}`, ]); }); @@ -2739,5 +2733,151 @@ describe('inject migration', () => { `}`, ]); }); + + it('should replace parameter references with property references when possible.', async () => { + writeFile( + '/dir.ts', + [ + `import { Directive } from '@angular/core';`, + `import { Foo } from 'foo';`, + ``, + `@Directive()`, + `class MyDir {`, + ` uninit: string;`, + ` constructor(readonly foo: Foo) {`, + ` this.uninit = foo.get();`, + // Replacing the param in other statements is out of scope for now, + // only change initializers. + ` console.log(foo);`, + ` }`, + `}`, + ].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 {`, + ` readonly foo = inject(Foo);`, + ``, + ` uninit: string = this.foo.get();`, + ` constructor() {`, + ` const foo = this.foo;`, + ``, + ` console.log(foo);`, + ` }`, + `}`, + ]); + }); + + it('should respect the replace parameter references flag', async () => { + writeFile( + '/dir.ts', + [ + `import { Directive } from '@angular/core';`, + `import { Foo } from 'foo';`, + ``, + `@Directive()`, + `class MyDir {`, + ` uninit: string;`, + ` constructor(readonly foo: Foo) {`, + ` this.uninit = foo.get();`, + // Replacing the param in other statements is out of scope for now, + // only change initializers. + ` console.log(foo);`, + ` }`, + `}`, + ].join('\n'), + ); + + await runInternalMigration({replaceParameterReferences: false}); + + expect(tree.readContent('/dir.ts').split('\n')).toEqual([ + `import { Directive, inject } from '@angular/core';`, + `import { Foo } from 'foo';`, + ``, + `@Directive()`, + `class MyDir {`, + ` readonly foo = inject(Foo);`, + ``, + ` uninit: string;`, + ` constructor() {`, + ` const foo = this.foo;`, + ``, + ` this.uninit = foo.get();`, + // Replacing the param in other statements is out of scope for now, + // only change initializers. + ` console.log(foo);`, + ` }`, + `}`, + ]); + }); + + it('should not replace parameter references with property references in nested contexts where `this` is different', async () => { + writeFile( + '/dir.ts', + [ + `import { Directive } from '@angular/core';`, + `import { Foo } from 'foo';`, + ``, + `@Directive()`, + `class MyDir {`, + ` uninit1: {};`, + ` uninit2: {};`, + ` uninit3: {};`, + ``, + ` constructor(readonly foo: Foo, readonly bar: Bar, readonly baz: Baz) {`, + ` this.uninit1 = function() {`, + ` foo;`, + ` };`, + ` this.uninit2 = class {`, + ` static a = bar;`, + ` };`, + ` this.uninit3 = {`, + ` method() { baz; }`, + ` };`, + ` }`, + `}`, + ].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 {`, + ` readonly foo = inject(Foo);`, + ` readonly bar = inject(Bar);`, + ` readonly baz = inject(Baz);`, + ``, + ` uninit1: {};`, + ` uninit2: {};`, + ` uninit3: {};`, + ``, + ` constructor() {`, + ` const foo = this.foo;`, + ` const bar = this.bar;`, + ` const baz = this.baz;`, + ``, + ` this.uninit1 = function() {`, + ` foo;`, + ` };`, + ` this.uninit2 = class {`, + ` static a = bar;`, + ` };`, + ` this.uninit3 = {`, + ` method() { baz; }`, + ` };`, + ` }`, + `}`, + ]); + }); }); });