diff --git a/packages/core/schematics/ng-generate/inject-migration/analysis.ts b/packages/core/schematics/ng-generate/inject-migration/analysis.ts index ca197ed561b3..526a05f59232 100644 --- a/packages/core/schematics/ng-generate/inject-migration/analysis.ts +++ b/packages/core/schematics/ng-generate/inject-migration/analysis.ts @@ -185,14 +185,14 @@ export function getConstructorUnusedParameters( return topLevelParameters; } - declaration.body.forEachChild(function walk(node) { + const analyze = (node: ts.Node) => { // Don't descend into statements that were removed already. if (ts.isStatement(node) && removedStatements.has(node)) { return; } if (!ts.isIdentifier(node) || !topLevelParameterNames.has(node.text)) { - node.forEachChild(walk); + node.forEachChild(analyze); return; } @@ -213,8 +213,16 @@ export function getConstructorUnusedParameters( } } }); + }; + + declaration.parameters.forEach((param) => { + if (param.initializer) { + analyze(param.initializer); + } }); + declaration.body.forEachChild(analyze); + for (const param of topLevelParameters) { if (!accessedTopLevelParameters.has(param)) { unusedParams.add(param); @@ -261,6 +269,51 @@ export function getSuperParameters( return usedParams; } +/** + * Determines if a specific parameter has references to other parameters. + * @param param Parameter to check. + * @param allParameters All parameters of the containing function. + * @param localTypeChecker Type checker scoped to the current file. + */ +export function parameterReferencesOtherParameters( + param: ts.ParameterDeclaration, + allParameters: ts.NodeArray, + localTypeChecker: ts.TypeChecker, +): boolean { + // A parameter can only reference other parameters through its initializer. + if (!param.initializer || allParameters.length < 2) { + return false; + } + + const paramNames = new Set(); + for (const current of allParameters) { + if (current !== param && ts.isIdentifier(current.name)) { + paramNames.add(current.name.text); + } + } + + let result = false; + const analyze = (node: ts.Node) => { + if (ts.isIdentifier(node) && paramNames.has(node.text) && !isAccessedViaThis(node)) { + const symbol = localTypeChecker.getSymbolAtLocation(node); + const referencesOtherParam = symbol?.declarations?.some((decl) => { + return (allParameters as ts.NodeArray).includes(decl); + }); + + if (referencesOtherParam) { + result = true; + } + } + + if (!result) { + node.forEachChild(analyze); + } + }; + + analyze(param.initializer); + return result; +} + /** Checks whether a parameter node declares a property on its class. */ export function parameterDeclaresProperty(node: ts.ParameterDeclaration): boolean { return !!node.modifiers?.some( diff --git a/packages/core/schematics/ng-generate/inject-migration/migration.ts b/packages/core/schematics/ng-generate/inject-migration/migration.ts index 426d3113fc68..3d657fef3ae1 100644 --- a/packages/core/schematics/ng-generate/inject-migration/migration.ts +++ b/packages/core/schematics/ng-generate/inject-migration/migration.ts @@ -17,6 +17,7 @@ import { parameterDeclaresProperty, DI_PARAM_SYMBOLS, MigrationOptions, + parameterReferencesOtherParameters, } from './analysis'; import {getAngularDecorators} from '../../utils/ng_decorators'; import {getImportOfIdentifier} from '../../utils/typescript/imports'; @@ -149,6 +150,11 @@ function migrateClass( for (const param of constructor.parameters) { const usedInSuper = superParameters !== null && superParameters.has(param); const usedInConstructor = !unusedParameters.has(param); + const usesOtherParams = parameterReferencesOtherParameters( + param, + constructor.parameters, + localTypeChecker, + ); migrateParameter( param, @@ -159,6 +165,7 @@ function migrateClass( superCall, usedInSuper, usedInConstructor, + usesOtherParams, memberIndentation, innerIndentation, prependToConstructor, @@ -176,7 +183,15 @@ function migrateClass( } } - if (canRemoveConstructor(options, constructor, removedStatementCount, superCall)) { + if ( + canRemoveConstructor( + options, + constructor, + removedStatementCount, + prependToConstructor, + superCall, + ) + ) { // Drop the constructor if it was empty. removedMembers.add(constructor); tracker.removeNode(constructor, true); @@ -186,11 +201,24 @@ function migrateClass( stripConstructorParameters(constructor, tracker); if (prependToConstructor.length > 0) { - tracker.insertText( - sourceFile, - (firstConstructorStatement || innerReference).getFullStart(), - `\n${prependToConstructor.join('\n')}\n`, - ); + if ( + firstConstructorStatement || + (innerReference !== constructor && + innerReference.getStart() >= constructor.getStart() && + innerReference.getEnd() <= constructor.getEnd()) + ) { + tracker.insertText( + sourceFile, + (firstConstructorStatement || innerReference).getFullStart(), + `\n${prependToConstructor.join('\n')}\n`, + ); + } else { + tracker.insertText( + sourceFile, + constructor.body!.getStart() + 1, + `\n${prependToConstructor.map((p) => innerIndentation + p).join('\n')}\n${innerIndentation}`, + ); + } } } @@ -268,6 +296,7 @@ function migrateParameter( superCall: ts.CallExpression | null, usedInSuper: boolean, usedInConstructor: boolean, + usesOtherParams: boolean, memberIndentation: string, innerIndentation: string, prependToConstructor: string[], @@ -290,6 +319,9 @@ function migrateParameter( // If the parameter declares a property, we need to declare it (e.g. `private foo: Foo`). if (declaresProp) { + // We can't initialize the property if it's referenced within a `super` call or it references + // other parameters. See the logic further below for the initialization. + const canInitialize = !usedInSuper && !usesOtherParams; const prop = ts.factory.createPropertyDeclaration( cloneModifiers( node.modifiers?.filter((modifier) => { @@ -302,10 +334,8 @@ function migrateParameter( node.modifiers?.some((modifier) => modifier.kind === ts.SyntaxKind.PrivateKeyword) ? undefined : node.questionToken, - // We can't initialize the property if it's referenced within a `super` call. - // See the logic further below for the initialization. - usedInSuper ? node.type : undefined, - usedInSuper ? undefined : ts.factory.createIdentifier(PLACEHOLDER), + canInitialize ? undefined : node.type, + canInitialize ? ts.factory.createIdentifier(PLACEHOLDER) : undefined, ); propsToAdd.push( @@ -341,6 +371,14 @@ function migrateParameter( // don't need to declare any new properties. prependToConstructor.push(`${innerIndentation}const ${name} = ${replacementCall};`); } + } else if (usesOtherParams && declaresProp) { + const toAdd = `${innerIndentation}this.${name} = ${replacementCall};`; + + if (superCall === null) { + prependToConstructor.push(toAdd); + } else { + afterSuper.push(toAdd); + } } } @@ -449,6 +487,15 @@ function createInjectReplacementCall( } } + // If the parameter is initialized, add the initializer as a fallback. + if (param.initializer) { + expression = ts.factory.createBinaryExpression( + expression, + ts.SyntaxKind.QuestionQuestionToken, + param.initializer, + ); + } + return replaceNodePlaceholder(param.getSourceFile(), expression, injectedType, printer); } @@ -638,15 +685,17 @@ function cloneName(node: ts.PropertyName): ts.PropertyName { * @param options Options used to configure the migration. * @param constructor Node representing the constructor. * @param removedStatementCount Number of statements that were removed by the migration. + * @param prependToConstructor Statements that should be prepended to the constructor. * @param superCall Node representing the `super()` call within the constructor. */ function canRemoveConstructor( options: MigrationOptions, constructor: ts.ConstructorDeclaration, removedStatementCount: number, + prependToConstructor: string[], superCall: ts.CallExpression | null, ): boolean { - if (options.backwardsCompatibleConstructors) { + if (options.backwardsCompatibleConstructors || prependToConstructor.length > 0) { return false; } diff --git a/packages/core/schematics/test/inject_migration_spec.ts b/packages/core/schematics/test/inject_migration_spec.ts index 8435733ad969..94ce0046a13a 100644 --- a/packages/core/schematics/test/inject_migration_spec.ts +++ b/packages/core/schematics/test/inject_migration_spec.ts @@ -1501,6 +1501,149 @@ describe('inject migration', () => { ]); }); + it('should preserve initializers', async () => { + writeFile( + '/dir.ts', + [ + `import { Directive, Optional } from '@angular/core';`, + `import { Foo } from './foo';`, + ``, + `function createFoo() { return new Foo(); }`, + ``, + `@Directive()`, + `class MyDir {`, + ` constructor(@Optional() private foo: Foo = createFoo()) {}`, + `}`, + ].join('\n'), + ); + + await runMigration(); + + expect(tree.readContent('/dir.ts').split('\n')).toEqual([ + `import { Directive, inject } from '@angular/core';`, + `import { Foo } from './foo';`, + ``, + `function createFoo() { return new Foo(); }`, + ``, + `@Directive()`, + `class MyDir {`, + ` private foo = inject(Foo, { optional: true }) ?? createFoo();`, + `}`, + ]); + }); + + it('should handle initializers referencing other parameters', async () => { + writeFile( + '/dir.ts', + [ + `import { Directive, Optional } from '@angular/core';`, + `import { Foo, Bar } from './providers';`, + ``, + `function createFoo(bar: Bar) { return new Foo(bar); }`, + ``, + `@Directive()`, + `class MyDir {`, + ` constructor(bar: Bar, @Optional() private foo: Foo = createFoo(bar)) {}`, + `}`, + ].join('\n'), + ); + + await runMigration(); + + expect(tree.readContent('/dir.ts').split('\n')).toEqual([ + `import { Directive, inject } from '@angular/core';`, + `import { Foo, Bar } from './providers';`, + ``, + `function createFoo(bar: Bar) { return new Foo(bar); }`, + ``, + `@Directive()`, + `class MyDir {`, + ` private foo: Foo;`, + ``, + ` constructor() {`, + ` const bar = inject(Bar);`, + ` this.foo = inject(Foo, { optional: true }) ?? createFoo(bar);`, + ` }`, + `}`, + ]); + }); + + it('should handle initializers referencing other parameters through "this"', async () => { + writeFile( + '/dir.ts', + [ + `import { Directive, Optional } from '@angular/core';`, + `import { Foo, Bar } from './providers';`, + ``, + `function createFoo(bar: Bar) { return new Foo(bar); }`, + ``, + `@Directive()`, + `class MyDir {`, + ` constructor(private bar: Bar, @Optional() private foo: Foo = createFoo(this.bar)) {}`, + `}`, + ].join('\n'), + ); + + await runMigration(); + + expect(tree.readContent('/dir.ts').split('\n')).toEqual([ + `import { Directive, inject } from '@angular/core';`, + `import { Foo, Bar } from './providers';`, + ``, + `function createFoo(bar: Bar) { return new Foo(bar); }`, + ``, + `@Directive()`, + `class MyDir {`, + ` private bar = inject(Bar);`, + ` private foo = inject(Foo, { optional: true }) ?? createFoo(this.bar);`, + `}`, + ]); + }); + + it('should handle parameters with initializers referenced inside super()', async () => { + writeFile( + '/dir.ts', + [ + `import { Directive, Optional } from '@angular/core';`, + `import { Foo, Bar } from './providers';`, + `import { Parent } from './parent';`, + ``, + `function createFoo(bar: Bar) { return new Foo(bar); }`, + ``, + `@Directive()`, + `class MyDir extends Parent {`, + ` constructor(bar: Bar, @Optional() private foo: Foo = createFoo(bar)) {`, + ` super(foo);`, + ` }`, + `}`, + ].join('\n'), + ); + + await runMigration(); + + expect(tree.readContent('/dir.ts').split('\n')).toEqual([ + `import { Directive, inject } from '@angular/core';`, + `import { Foo, Bar } from './providers';`, + `import { Parent } from './parent';`, + ``, + `function createFoo(bar: Bar) { return new Foo(bar); }`, + ``, + `@Directive()`, + `class MyDir extends Parent {`, + ` private foo: Foo;`, + ``, + ` constructor() {`, + ` const bar = inject(Bar);`, + ` const foo = inject(Foo, { optional: true }) ?? createFoo(bar);`, + ``, + ` super(foo);`, + ` this.foo = foo;`, + ``, + ` }`, + `}`, + ]); + }); + describe('internal-only behavior', () => { function runInternalMigration() { return runMigration({_internalCombineMemberInitializers: true});