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

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. */

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<ts.Declaration>();
+
   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<string>,
+  properties: Set<ts.Declaration>,
+): 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<ts.Declaration> {
+  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<ts.Declaration> = 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<ts.Declaration>,
   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() &&

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.');
+}

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>(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; }`,
+        `    };`,
+        `  }`,
+        `}`,
+      ]);
+    });
   });
 });