angular · crisbeto · Nov 20, 2024
diff --git 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<ts.ParameterDeclaration>,
+  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<string>();
+  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<ts.Declaration>).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
@@ -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
@@ -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});