refactor(migrations): hoist uninitialized members to top of class if they are not combined in the internal migration (#58393)

crisbeto · alxhub · commit a2a9ac76f591 · 2024-10-28T12:39:03.000-07:00
When the internal mode for the `inject` migration is enabled, we find properties without initializers, we add their initializers and we prepend the `inject` calls before them. The remaining properties that couldn't be combined are left in place. This appears to break some internal cases. These changes work around the issue by hoisting all the non-combined members above the `inject()` calls. This should be safe, because they don't have initializers and as such can't have dependencies. PR Close #58393
diff --git a/packages/core/schematics/ng-generate/inject-migration/analysis.ts b/packages/core/schematics/ng-generate/inject-migration/analysis.ts
@@ -128,7 +128,7 @@ export function analyzeFile(sourceFile: ts.SourceFile, localTypeChecker: ts.Type
 export function getConstructorUnusedParameters(
   declaration: ts.ConstructorDeclaration,
   localTypeChecker: ts.TypeChecker,
-  removedStatements: Set<ts.Statement> | null,
+  removedStatements: Set<ts.Statement>,
 ): Set<ts.Declaration> {
   const accessedTopLevelParameters = new Set<ts.Declaration>();
   const topLevelParameters = new Set<ts.Declaration>();
@@ -149,7 +149,7 @@ 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)) {
+    if (ts.isStatement(node) && removedStatements.has(node)) {
       return;
     }
 
diff --git a/packages/core/schematics/ng-generate/inject-migration/internal.ts b/packages/core/schematics/ng-generate/inject-migration/internal.ts
@@ -32,8 +32,12 @@ 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;
+): {
+  toCombine: Map<ts.PropertyDeclaration, ts.Expression>;
+  toHoist: ts.PropertyDeclaration[];
+} | null {
+  let toCombine: Map<ts.PropertyDeclaration, ts.Expression> | null = null;
+  let toHoist: ts.PropertyDeclaration[] = [];
 
   const membersToDeclarations = new Map<string, ts.PropertyDeclaration>();
   for (const member of node.members) {
@@ -47,25 +51,43 @@ export function findUninitializedPropertiesToCombine(
   }
 
   if (membersToDeclarations.size === 0) {
-    return result;
+    return null;
   }
 
   const memberInitializers = getMemberInitializers(constructor);
   if (memberInitializers === null) {
-    return result;
+    return null;
   }
 
-  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);
+  for (const [name, decl] of membersToDeclarations.entries()) {
+    if (memberInitializers.has(name)) {
+      const initializer = memberInitializers.get(name)!;
+
+      if (!hasLocalReferences(initializer, constructor, localTypeChecker)) {
+        toCombine = toCombine || new Map();
+        toCombine.set(membersToDeclarations.get(name)!, initializer);
+      }
+    } else {
+      // Mark members that have no initializers and can't be combined to be hoisted above the
+      // injected members. This is either a no-op or it allows us to avoid some patterns internally
+      // like the following:
+      // ```
+      // class Foo {
+      //   publicFoo: Foo;
+      //   private privateFoo: Foo;
+      //
+      //   constructor() {
+      //     this.initializePrivateFooSomehow();
+      //     this.publicFoo = this.privateFoo;
+      //   }
+      // }
+      // ```
+      toHoist.push(decl);
     }
   }
 
-  return result;
+  // If no members need to be combined, none need to be hoisted either.
+  return toCombine === null ? null : {toCombine, toHoist};
 }
 
 /**
diff --git a/packages/core/schematics/ng-generate/inject-migration/migration.ts b/packages/core/schematics/ng-generate/inject-migration/migration.ts
@@ -82,34 +82,20 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
   const tracker = new ChangeTracker(printer);
 
   analysis.classes.forEach(({node, constructor, superCall}) => {
-    let removedStatements: Set<ts.Statement> | null = null;
+    const memberIndentation = getLeadingLineWhitespaceOfNode(node.members[0]);
+    const prependToClass: string[] = [];
+    const removedStatements = new Set<ts.Statement>();
 
     if (options._internalCombineMemberInitializers) {
-      findUninitializedPropertiesToCombine(node, constructor, localTypeChecker)?.forEach(
-        (initializer, property) => {
-          const statement = closestNode(initializer, ts.isStatement);
-
-          if (!statement) {
-            return;
-          }
-
-          const newProperty = ts.factory.createPropertyDeclaration(
-            cloneModifiers(property.modifiers),
-            cloneName(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);
-        },
+      applyInternalOnlyChanges(
+        node,
+        constructor,
+        localTypeChecker,
+        tracker,
+        printer,
+        removedStatements,
+        prependToClass,
+        memberIndentation,
       );
     }
 
@@ -118,6 +104,8 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
       constructor,
       superCall,
       options,
+      memberIndentation,
+      prependToClass,
       removedStatements,
       localTypeChecker,
       printer,
@@ -141,6 +129,8 @@ 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 memberIndentation Indentation string of the members of the class.
+ * @param prependToClass Text that should be prepended to the class.
  * @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.
@@ -151,7 +141,9 @@ function migrateClass(
   constructor: ts.ConstructorDeclaration,
   superCall: ts.CallExpression | null,
   options: MigrationOptions,
-  removedStatements: Set<ts.Statement> | null,
+  memberIndentation: string,
+  prependToClass: string[],
+  removedStatements: Set<ts.Statement>,
   localTypeChecker: ts.TypeChecker,
   printer: ts.Printer,
   tracker: ChangeTracker,
@@ -173,14 +165,12 @@ function migrateClass(
   const superParameters = superCall
     ? getSuperParameters(constructor, superCall, localTypeChecker)
     : null;
-  const memberIndentation = getLeadingLineWhitespaceOfNode(node.members[0]);
-  const removedStatementCount = removedStatements?.size || 0;
+  const removedStatementCount = removedStatements.size;
   const innerReference =
     superCall ||
-    constructor.body?.statements.find((statement) => !removedStatements?.has(statement)) ||
+    constructor.body?.statements.find((statement) => !removedStatements.has(statement)) ||
     constructor;
   const innerIndentation = getLeadingLineWhitespaceOfNode(innerReference);
-  const propsToAdd: string[] = [];
   const prependToConstructor: string[] = [];
   const afterSuper: string[] = [];
   const removedMembers = new Set<ts.ClassElement>();
@@ -201,7 +191,7 @@ function migrateClass(
       memberIndentation,
       innerIndentation,
       prependToConstructor,
-      propsToAdd,
+      prependToClass,
       afterSuper,
     );
   }
@@ -249,21 +239,21 @@ function migrateClass(
 
     // The new signature always has to be right before the constructor implementation.
     if (memberReference === constructor) {
-      propsToAdd.push(extraSignature);
+      prependToClass.push(extraSignature);
     } else {
       tracker.insertText(sourceFile, constructor.getFullStart(), '\n' + extraSignature);
     }
   }
 
-  if (propsToAdd.length > 0) {
+  if (prependToClass.length > 0) {
     if (removedMembers.size === node.members.length) {
-      tracker.insertText(sourceFile, constructor.getEnd() + 1, `${propsToAdd.join('\n')}\n`);
+      tracker.insertText(sourceFile, constructor.getEnd() + 1, `${prependToClass.join('\n')}\n`);
     } else {
       // Insert the new properties after the first member that hasn't been deleted.
       tracker.insertText(
         sourceFile,
         memberReference.getFullStart(),
-        `\n${propsToAdd.join('\n')}\n`,
+        `\n${prependToClass.join('\n')}\n`,
       );
     }
   }
@@ -685,3 +675,63 @@ function canRemoveConstructor(
     (statementCount === 1 && superCall !== null && superCall.arguments.length === 0)
   );
 }
+
+/**
+ * Applies the internal-specific migrations to a class.
+ * @param node Class being migrated.
+ * @param constructor The migrated class' constructor.
+ * @param localTypeChecker File-specific type checker.
+ * @param tracker Object keeping track of the changes.
+ * @param printer Printer used to output AST nodes as text.
+ * @param removedStatements Statements that have been removed by the migration.
+ * @param prependToClass Text that will be prepended to a class.
+ * @param memberIndentation Indentation string of the class' members.
+ */
+function applyInternalOnlyChanges(
+  node: ts.ClassDeclaration,
+  constructor: ts.ConstructorDeclaration,
+  localTypeChecker: ts.TypeChecker,
+  tracker: ChangeTracker,
+  printer: ts.Printer,
+  removedStatements: Set<ts.Statement>,
+  prependToClass: string[],
+  memberIndentation: string,
+) {
+  const result = findUninitializedPropertiesToCombine(node, constructor, localTypeChecker);
+
+  result?.toCombine.forEach((initializer, property) => {
+    const statement = closestNode(initializer, ts.isStatement);
+
+    if (!statement) {
+      return;
+    }
+
+    const newProperty = ts.factory.createPropertyDeclaration(
+      cloneModifiers(property.modifiers),
+      cloneName(property.name),
+      property.questionToken,
+      property.type,
+      initializer,
+    );
+    tracker.replaceText(
+      statement.getSourceFile(),
+      statement.getFullStart(),
+      statement.getFullWidth(),
+      '',
+    );
+    tracker.replaceNode(property, newProperty);
+    removedStatements.add(statement);
+  });
+
+  result?.toHoist.forEach((decl) => {
+    prependToClass.push(
+      memberIndentation + printer.printNode(ts.EmitHint.Unspecified, decl, decl.getSourceFile()),
+    );
+    tracker.replaceText(decl.getSourceFile(), decl.getFullStart(), decl.getFullWidth(), '');
+  });
+
+  // If we added any hoisted properties, separate them visually with a new line.
+  if (prependToClass.length > 0) {
+    prependToClass.push('');
+  }
+}
diff --git a/packages/core/schematics/test/inject_migration_spec.ts b/packages/core/schematics/test/inject_migration_spec.ts
@@ -1884,5 +1884,62 @@ describe('inject migration', () => {
         `}`,
       ]);
     });
+
+    it('should hoist property declarations that were not combined above the inject() calls', async () => {
+      writeFile(
+        '/dir.ts',
+        [
+          `import { Injectable } from '@angular/core';`,
+          `import { Observable } from 'rxjs';`,
+          `import { StateService, State } from './state';`,
+          ``,
+          `@Injectable()`,
+          `export class SomeService {`,
+          `  /** Public state */`,
+          `  readonly state: Observable<State>;`,
+          ``,
+          `  /** Private state */`,
+          `  private internalState?: State;`,
+          ``,
+          `  constructor(readonly stateService: StateService) {`,
+          `    this.initializeInternalState();`,
+          `    this.state = this.internalState;`,
+          `  }`,
+          ``,
+          `  private initializeInternalState() {`,
+          `    this.internalState = new State();`,
+          `  }`,
+          `}`,
+        ].join('\n'),
+      );
+
+      await runInternalMigration();
+
+      expect(tree.readContent('/dir.ts').split('\n')).toEqual([
+        `import { Injectable, inject } from '@angular/core';`,
+        `import { Observable } from 'rxjs';`,
+        `import { StateService, State } from './state';`,
+        ``,
+        `@Injectable()`,
+        `export class SomeService {`,
+        `  /** Private state */`,
+        // The indentation here is slightly off, but it's not a problem because this code is internal-only.
+        `private internalState?: State;`,
+        ``,
+        `  readonly stateService = inject(StateService);`,
+        ``,
+        `  /** Public state */`,
+        `  readonly state: Observable<State> = this.internalState;`,
+        ``,
+        `  constructor() {`,
+        `    this.initializeInternalState();`,
+        `  }`,
+        ``,
+        `  private initializeInternalState() {`,
+        `    this.internalState = new State();`,
+        `  }`,
+        `}`,
+      ]);
+    });
   });
 });
