refactor(migrations): improve control flow analysis for loops in signal migration (#58581)

Currently whenever we would come across a code snippet like this, where
`maxCellsPerRow` is an input, the control flow analysis would fall apart
because the first occurence of the node points to a control flow node
that is offset-wise "after" the first occurence. This commit makes the
logic more robust.

PR Close #58581

packages/core/schematics/migrations/signal-migration/src/flow_analysis/index.ts

@@ -87,10 +87,18 @@ export function analyzeControlFlow(
 
   // Prepare easy lookups for reference nodes to flow info.
   for (const [idx, entry] of entries.entries()) {
+    const flowContainer = getControlFlowContainer(entry);
     referenceToMetadata.set(entry, {
-      flowContainer: getControlFlowContainer(entry),
+      flowContainer,
       resultIndex: idx,
     });
+
+    result.push({
+      flowContainer,
+      id: idx,
+      originalNode: entry,
+      recommendedNode: 'preserve',
+    });
   }
 
   for (const entry of entries) {
@@ -110,13 +118,6 @@ export function analyzeControlFlow(
       checker,
     );
 
-    result.push({
-      id: resultIndex,
-      originalNode: entry,
-      flowContainer,
-      recommendedNode: 'preserve',
-    });
-
     if (narrowPartners.length !== 0) {
       connectSharedReferences(result, narrowPartners, resultIndex);
     }
@@ -154,13 +155,19 @@ function connectSharedReferences(
   assert(earliestPartner !== null, 'Expected an earliest partner to be found.');
   assert(earliestPartnerId !== null, 'Expected an earliest partner to be found.');
 
+  // Earliest partner ID could be higher than `refId` in cyclic
+  // situations like `loop` flow nodes. We need to find the minimum
+  // and maximum to iterate through partners in between.
+  const min = Math.min(earliestPartnerId, refId);
+  const max = Math.max(earliestPartnerId, refId);
+
   // Then, incorporate all similar references (or flow nodes) in between
   // the reference and the earliest partner. References in between can also
   // use the shared flow node and not preserve their original reference— as
   // this would be rather unreadable and inefficient.
   const seenBlocks = new Set<ts.ArrowFunction | ts.Block | ts.SourceFile>();
   let highestBlock: ts.Block | ts.SourceFile | ts.ArrowFunction | null = null;
-  for (let i = earliestPartnerId; i <= refId; i++) {
+  for (let i = min; i <= max; i++) {
     // Different flow container captured sequentially in result. Ignore.
     if (result[i].flowContainer !== refFlowContainer) {
       continue;

packages/core/schematics/migrations/signal-migration/src/passes/10_apply_import_manager.ts

@@ -10,7 +10,6 @@ import {ImportManager} from '@angular/compiler-cli/src/ngtsc/translator';
 import ts from 'typescript';
 import {applyImportManagerChanges} from '../../../../utils/tsurge/helpers/apply_import_manager';
 import {MigrationResult} from '../result';
-import {AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system';
 import {ProgramInfo} from '../../../../utils/tsurge';
 
 /**

packages/core/schematics/migrations/signal-migration/src/passes/reference_migration/helpers/standard_reference.ts

@@ -7,11 +7,12 @@
  */
 
 import ts from 'typescript';
-import {analyzeControlFlow} from '../../../flow_analysis';
+import {analyzeControlFlow, ControlFlowAnalysisNode} from '../../../flow_analysis';
 import {ProgramInfo, projectFile, Replacement, TextUpdate} from '../../../../../../utils/tsurge';
 import {traverseAccess} from '../../../utils/traverse_access';
 import {UniqueNamesGenerator} from '../../../utils/unique_names';
 import {createNewBlockToInsertVariable} from '../helpers/create_block_arrow_function';
+import assert from 'assert';
 
 export interface NarrowableTsReferences {
   accesses: ts.Identifier[];
@@ -30,6 +31,19 @@ export function migrateStandardTsReference(
     const controlFlowResult = analyzeControlFlow(reference.accesses, checker);
     const idToSharedField = new Map<number, string>();
 
+    const isSharePartnerRef = (val: ControlFlowAnalysisNode['recommendedNode']) => {
+      return val !== 'preserve' && typeof val !== 'number';
+    };
+
+    // Ensure we generate shared fields before reference entries.
+    // This allows us to safely make use of `idToSharedField` whenever we come
+    // across a referenced pointing to a share partner.
+    controlFlowResult.sort((a, b) => {
+      const aPriority = isSharePartnerRef(a.recommendedNode) ? 1 : 0;
+      const bPriority = isSharePartnerRef(b.recommendedNode) ? 1 : 0;
+      return bPriority - aPriority;
+    });
+
     for (const {id, originalNode, recommendedNode} of controlFlowResult) {
       const sf = originalNode.getSourceFile();
 
@@ -53,15 +67,18 @@ export function migrateStandardTsReference(
       // This reference is shared with a previous reference. Replace the access
       // with the temporary variable.
       if (typeof recommendedNode === 'number') {
+        // Extract the shared field name.
+        const toInsert = idToSharedField.get(recommendedNode);
         const replaceNode = traverseAccess(originalNode);
+
+        assert(toInsert, 'no shared variable yet available');
         replacements.push(
           new Replacement(
             projectFile(sf, info),
             new TextUpdate({
               position: replaceNode.getStart(),
               end: replaceNode.getEnd(),
-              // Extract the shared field name.
-              toInsert: idToSharedField.get(recommendedNode)!,
+              toInsert,
             }),
           ),
         );

packages/core/schematics/migrations/signal-migration/test/golden-test/flow_cases.ts

@@ -0,0 +1,13 @@
+// tslint:disable
+
+import {Input} from '@angular/core';
+
+class Test {
+  @Input() maxCellsPerRow = 5;
+
+  private test(arr: string[]) {
+    for (let i = 0; i < arr.length; i += this.maxCellsPerRow) {
+      console.log(this.maxCellsPerRow);
+    }
+  }
+}

packages/core/schematics/migrations/signal-migration/test/golden.txt

@@ -368,6 +368,22 @@ import {Component, input} from '@angular/core';
 export class WithTemplate {
   readonly test = input(true);
 }
+@@@@@@ flow_cases.ts @@@@@@
+
+// tslint:disable
+
+import {input} from '@angular/core';
+
+class Test {
+  readonly maxCellsPerRow = input(5);
+
+  private test(arr: string[]) {
+    const maxCellsPerRow = this.maxCellsPerRow();
+    for (let i = 0; i < arr.length; i += maxCellsPerRow) {
+      console.log(maxCellsPerRow);
+    }
+  }
+}
 @@@@@@ getters.ts @@@@@@
 
 // tslint:disable

packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt

@@ -344,6 +344,22 @@ import {Component, input} from '@angular/core';
 export class WithTemplate {
   readonly test = input(true);
 }
+@@@@@@ flow_cases.ts @@@@@@
+
+// tslint:disable
+
+import {input} from '@angular/core';
+
+class Test {
+  readonly maxCellsPerRow = input(5);
+
+  private test(arr: string[]) {
+    const maxCellsPerRow = this.maxCellsPerRow();
+    for (let i = 0; i < arr.length; i += maxCellsPerRow) {
+      console.log(maxCellsPerRow);
+    }
+  }
+}
 @@@@@@ getters.ts @@@@@@
 
 // tslint:disable