refactor(migrations): support inserting TODOs for skipped input fields (#57898)

This is a helpful option to retrieve some insights on why certain inputs
were not migrated.

PR Close #57898

packages/core/schematics/migrations/signal-migration/src/batch/merge_unit_data.ts

@@ -90,7 +90,5 @@ export function mergeCompilationUnitData(
     }
   }
 
-  console.error(result);
-
   return result;
 }

packages/core/schematics/migrations/signal-migration/src/batch/test_bin.ts

@@ -22,7 +22,7 @@ main().catch((e) => {
 
 async function main() {
   const [mode, ...args] = process.argv.slice(2);
-  const migration = new SignalInputMigration();
+  const migration = new SignalInputMigration({insertTodosForSkippedFields: true});
 
   if (mode === 'extract') {
     const analyzeResult = await executeAnalyzePhase(migration, path.resolve(args[0]));

packages/core/schematics/migrations/signal-migration/src/cli.ts

@@ -12,7 +12,11 @@ import assert from 'assert';
 import {SignalInputMigration} from './migration';
 import {writeMigrationReplacements} from './write_replacements';
 
-main(path.resolve(process.argv[2]), process.argv.includes('--best-effort-mode')).catch((e) => {
+main(
+  path.resolve(process.argv[2]),
+  process.argv.includes('--best-effort-mode'),
+  process.argv.includes('--insert-todos'),
+).catch((e) => {
   console.error(e);
   process.exitCode = 1;
 });
@@ -20,9 +24,14 @@ main(path.resolve(process.argv[2]), process.argv.includes('--best-effort-mode'))
 /**
  * Runs the signal input migration for the given TypeScript project.
  */
-export async function main(absoluteTsconfigPath: string, bestEffortMode: boolean) {
+export async function main(
+  absoluteTsconfigPath: string,
+  bestEffortMode: boolean,
+  insertTodosForSkippedFields: boolean,
+) {
   const migration = new SignalInputMigration({
     bestEffortMode,
+    insertTodosForSkippedFields,
     upgradeAnalysisPhaseToAvoidBatch: true,
   });
   const baseInfo = migration.createProgram(absoluteTsconfigPath);

packages/core/schematics/migrations/signal-migration/src/input_detection/directive_info.ts

@@ -53,6 +53,13 @@ export class DirectiveInfo {
    * then the member is as well.
    */
   isInputMemberIncompatible(input: InputDescriptor): boolean {
-    return this.incompatible !== null || this.memberIncompatibility.has(input.key);
+    return this.getInputMemberIncompatibility(input) !== null;
+  }
+
+  /** Get incompatibility of the given member, if it's incompatible for migration. */
+  getInputMemberIncompatibility(
+    input: InputDescriptor,
+  ): ClassIncompatibilityReason | InputMemberIncompatibility | null {
+    return this.incompatible ?? this.memberIncompatibility.get(input.key) ?? null;
   }
 }

packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts

@@ -101,7 +101,7 @@ export function getMessageForClassIncompatibility(reason: ClassIncompatibilityRe
     case ClassIncompatibilityReason.ClassManuallyInstantiated:
       return {
         short:
-          'Class of this input is manually instantiated (`new Cmp()`). ' +
+          'Class of this input is manually instantiated. ' +
           'This is discouraged and prevents migration',
         extra:
           'Signal inputs require a DI injection context. Manually instantiating ' +

packages/core/schematics/migrations/signal-migration/src/migration_config.ts

@@ -15,6 +15,12 @@ export interface MigrationConfig {
    */
   bestEffortMode?: boolean;
 
+  /**
+   * Whether to insert TODOs for skipped fields, and reasons on why they
+   * were skipped.
+   */
+  insertTodosForSkippedFields?: boolean;
+
   /**
    * Whether the given input should be migrated. With batch execution, this
    * callback fires for foreign inputs from other compilation units too.

packages/core/schematics/migrations/signal-migration/src/passes/6_migrate_input_declarations.ts

@@ -6,19 +6,22 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import ts from 'typescript';
-import {MigrationResult} from '../result';
-import {convertToSignalInput} from '../convert-input/convert_to_signal';
-import assert from 'assert';
-import {KnownInputs} from '../input_detection/known_inputs';
 import {ImportManager} from '@angular/compiler-cli/src/ngtsc/translator';
+import assert from 'assert';
+import ts from 'typescript';
 import {ProgramInfo, projectFile, Replacement, TextUpdate} from '../../../../utils/tsurge';
+import {convertToSignalInput} from '../convert-input/convert_to_signal';
+import {KnownInputs} from '../input_detection/known_inputs';
+import {MigrationHost} from '../migration_host';
+import {MigrationResult} from '../result';
+import {insertTodoForIncompatibility} from '../utils/incompatibility_todos';
 
 /**
  * Phase that migrates `@Input()` declarations to signal inputs and
  * manages imports within the given file.
  */
 export function pass6__migrateInputDeclarations(
+  host: MigrationHost,
   checker: ts.TypeChecker,
   result: MigrationResult,
   knownInputs: KnownInputs,
@@ -30,13 +33,20 @@ export function pass6__migrateInputDeclarations(
 
   for (const [input, metadata] of result.sourceInputs) {
     const sf = input.node.getSourceFile();
+    const inputInfo = knownInputs.get(input)!;
 
     // Do not migrate incompatible inputs.
-    if (knownInputs.get(input)!.isIncompatible() || metadata === null) {
+    if (inputInfo.isIncompatible()) {
+      // Add a TODO for the incompatible input, if desired.
+      if (host.config.insertTodosForSkippedFields) {
+        result.replacements.push(...insertTodoForIncompatibility(input.node, info, inputInfo));
+      }
+
       filesWithIncompatibleInputs.add(sf);
       continue;
     }
 
+    assert(metadata !== null, `Expected metadata to exist for input isn't marked incompatible.`);
     assert(!ts.isAccessor(input.node), 'Accessor inputs are incompatible.');
 
     filesWithMigratedInputs.add(sf);

packages/core/schematics/migrations/signal-migration/src/phase_migrate.ts

@@ -54,7 +54,7 @@ export function executeMigrationPhase(
 
   // Migrate passes.
   pass5__migrateTypeScriptReferences(referenceMigrationHost, result.references, typeChecker, info);
-  pass6__migrateInputDeclarations(typeChecker, result, knownInputs, importManager, info);
+  pass6__migrateInputDeclarations(host, typeChecker, result, knownInputs, importManager, info);
   pass7__migrateTemplateReferences(referenceMigrationHost, result.references);
   pass8__migrateHostBindings(referenceMigrationHost, result.references, info);
   pass9__migrateTypeScriptTypeReferences(

packages/core/schematics/migrations/signal-migration/src/utils/incompatibility_todos.ts

@@ -0,0 +1,71 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {KnownInputInfo} from '../input_detection/known_inputs';
+import {ProgramInfo, Replacement} from '../../../../utils/tsurge';
+import {isInputMemberIncompatibility} from '../input_detection/incompatibility';
+import {
+  getMessageForClassIncompatibility,
+  getMessageForInputIncompatibility,
+} from '../input_detection/incompatibility_human';
+import {insertPrecedingLine} from '../../../../utils/tsurge/helpers/ast/insert_preceding_line';
+import {InputNode} from '../input_detection/input_node';
+
+/**
+ * Inserts a TODO for the incompatibility blocking the given node
+ * from being migrated.
+ */
+export function insertTodoForIncompatibility(
+  node: InputNode,
+  programInfo: ProgramInfo,
+  input: KnownInputInfo,
+): Replacement[] {
+  const incompatibility = input.container.getInputMemberIncompatibility(input.descriptor);
+  if (incompatibility === null) {
+    return [];
+  }
+
+  const message = isInputMemberIncompatibility(incompatibility)
+    ? getMessageForInputIncompatibility(incompatibility.reason).short
+    : getMessageForClassIncompatibility(incompatibility).short;
+  const lines = cutStringToWordLimit(message, 70);
+
+  return [
+    insertPrecedingLine(node, programInfo, `// TODO: Skipped for migration because:`),
+    ...lines.map((line) => insertPrecedingLine(node, programInfo, `//  ${line}`)),
+  ];
+}
+
+/**
+ * Cuts the given string into lines basing around the specified
+ * line length limit. This function breaks the string on a per-word basis.
+ */
+function cutStringToWordLimit(str: string, limit: number): string[] {
+  const words = str.split(' ');
+  const chunks: string[] = [];
+  let chunkIdx = 0;
+
+  while (words.length) {
+    // New line if we exceed limit.
+    if (chunks[chunkIdx] !== undefined && chunks[chunkIdx].length > limit) {
+      chunkIdx++;
+    }
+    // Ensure line is initialized for the given index.
+    if (chunks[chunkIdx] === undefined) {
+      chunks[chunkIdx] = '';
+    }
+
+    const word = words.shift();
+    const needsSpace = chunks[chunkIdx].length > 0;
+
+    // Insert word. Add space before, if the line already contains text.
+    chunks[chunkIdx] += `${needsSpace ? ' ' : ''}${word}`;
+  }
+
+  return chunks;
+}

packages/core/schematics/migrations/signal-migration/test/BUILD.bazel

@@ -38,7 +38,7 @@ integration_test(
         "//packages/core/schematics/migrations/signal-migration/test/golden-test:test_files",
     ],
     commands = [
-        "$(rootpath //packages/core/schematics/migrations/signal-migration/src:bin) ./golden-test/tsconfig.json",
+        "$(rootpath //packages/core/schematics/migrations/signal-migration/src:bin) ./golden-test/tsconfig.json --insert-todos",
         "$(rootpath :golden_test_runner) ./golden-test ./golden.txt",
     ],
     data = [
@@ -60,7 +60,7 @@ integration_test(
             "//packages/core/schematics/migrations/signal-migration/test/ts-versions:loader.mjs",
         ],
         commands = [
-            "$(rootpath //packages/core/schematics/migrations/signal-migration/src:bin) --node_options=--import=./ts-versions/loader.js ./golden-test/tsconfig.json",
+            "$(rootpath //packages/core/schematics/migrations/signal-migration/src:bin) --node_options=--import=./ts-versions/loader.js ./golden-test/tsconfig.json --insert-todos ",
             "$(rootpath :golden_test_runner) ./golden-test ./golden.txt",
         ],
         data = [
@@ -83,7 +83,7 @@ integration_test(
         "//packages/core/schematics/migrations/signal-migration/test/golden-test:test_files",
     ],
     commands = [
-        "$(rootpath //packages/core/schematics/migrations/signal-migration/src:bin) ./golden-test/tsconfig.json --best-effort-mode",
+        "$(rootpath //packages/core/schematics/migrations/signal-migration/src:bin) ./golden-test/tsconfig.json --best-effort-mode --insert-todos",
         "$(rootpath :golden_test_runner) ./golden-test ./golden_best_effort.txt",
     ],
     data = [

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

@@ -36,6 +36,9 @@ class BaseNonAngular {
 @Directive()
 class Sub implements BaseNonAngular {
   // should not be migrated because of the interface.
+  // TODO: Skipped for migration because:
+  //  This input overrides a field from a superclass, while the superclass field
+  //  is not migrated.
   @Input() disabled = '';
 }
 
@@ -55,6 +58,9 @@ interface BaseNonAngularInterface {
 @Directive()
 class Sub3 implements BaseNonAngularInterface {
   // should not be migrated because of the interface.
+  // TODO: Skipped for migration because:
+  //  This input overrides a field from a superclass, while the superclass field
+  //  is not migrated.
   @Input() disabled = '';
 }
 @@@@@@ both_input_imports.ts @@@@@@
@@ -64,6 +70,8 @@ class Sub3 implements BaseNonAngularInterface {
 import {input, Input} from '@angular/core';
 
 class BothInputImported {
+  // TODO: Skipped for migration because:
+  //  Your application code writes to the input. This prevents migration.
   @Input() decoratorInput = true;
   signalInput = input<boolean>();
 
@@ -127,6 +135,8 @@ import {Input, Directive} from '@angular/core';
 
 @Directive()
 export class MyComp {
+  // TODO: Skipped for migration because:
+  //  Your application code writes to the input. This prevents migration.
   @Input() firstName: string;
 
   constructor() {
@@ -170,6 +180,8 @@ import {Input, Directive} from '@angular/core';
 
 @Directive()
 class Base {
+  // TODO: Skipped for migration because:
+  //  The input cannot be migrated because the field is overridden by a subclass.
   @Input() bla = true;
 }
 
@@ -180,12 +192,16 @@ class Derived extends Base {
 // overridden in separate file
 @Directive()
 export class Base2 {
+  // TODO: Skipped for migration because:
+  //  The input cannot be migrated because the field is overridden by a subclass.
   @Input() bla = true;
 }
 
 // overridden in separate file
 @Directive()
 export class Base3 {
+  // TODO: Skipped for migration because:
+  //  Your application code writes to the input. This prevents migration.
   @Input() bla = true;
 
   click() {
@@ -201,6 +217,8 @@ import {Input, Directive} from '@angular/core';
 @Directive()
 class Base {
   // should not be migrated.
+  // TODO: Skipped for migration because:
+  //  The input cannot be migrated because the field is overridden by a subclass.
   @Input() bla = true;
 }
 
@@ -219,6 +237,8 @@ class Derived extends DerivedExternalWithInput {
   // this should be incompatible, because the final superclass
   // within its own batch unit, detected a write that should
   // propagate to this input.
+  // TODO: Skipped for migration because:
+  //  This input is inherited from a superclass, but the parent cannot be migrated.
   @Input() override bla = false;
 }
 @@@@@@ derived_class_separate_file.ts @@@@@@
@@ -233,6 +253,8 @@ class DerivedExternal extends Base2 {
 }
 
 export class DerivedExternalWithInput extends Base3 {
+  // TODO: Skipped for migration because:
+  //  This input is inherited from a superclass, but the parent cannot be migrated.
   @Input() override bla = true;
 }
 @@@@@@ different_instantiations_of_reference.ts @@@@@@
@@ -318,6 +340,8 @@ import {Directive, Input} from '@angular/core';
 
 @Directive({})
 export class WithGetters {
+  // TODO: Skipped for migration because:
+  //  Accessor inputs cannot be migrated as they are too complex.
   @Input()
   get disabled() {
     return this._disabled;
@@ -471,9 +495,13 @@ export class AppComponent {
   readonly bla = input.required<boolean, string | boolean>({ transform: disabledTransform });
   readonly narrowableMultipleTimes = input<Vehicle | null>(null);
   readonly withUndefinedInput = input<string>();
+  // TODO: Skipped for migration because:
+  //  Your application code writes to the input. This prevents migration.
   @Input() incompatible: string | null = null;
 
   private _bla: any;
+  // TODO: Skipped for migration because:
+  //  Accessor inputs cannot be migrated as they are too complex.
   @Input()
   set ngSwitch(newValue: any) {
     this._bla = newValue;
@@ -733,6 +761,9 @@ import {Component, Input} from '@angular/core';
 
 @Component({})
 export class ManualInstantiation {
+  // TODO: Skipped for migration because:
+  //  Class of this input is manually instantiated. This is discouraged and prevents
+  //  migration
   @Input() bla: string = '';
 }
 @@@@@@ modifier_tests.ts @@@@@@
@@ -924,6 +955,8 @@ import {Component, Directive, QueryList, Input} from '@angular/core';
   `,
 })
 class Group {
+  // TODO: Skipped for migration because:
+  //  Class of this input is referenced in the signature of another class.
   @Input() label!: string;
 }
 
@@ -1077,6 +1110,9 @@ export class ScopeMismatchTest {
 import {Input} from '@angular/core';
 
 class MyComp {
+  // TODO: Skipped for migration because:
+  //  Class of this input is manually instantiated. This is discouraged and prevents
+  //  migration
   @Input() myInput = () => {};
 }
 
@@ -1130,8 +1166,17 @@ import {Component, Input, input} from '@angular/core';
   `,
 })
 export class MyComp {
+  // TODO: Skipped for migration because:
+  //  This input is used in a control flow expression (e.g. `@if` or `*ngIf`)
+  //  and migrating would break narrowing currently.
   @Input() first = true;
+  // TODO: Skipped for migration because:
+  //  This input is used in a control flow expression (e.g. `@if` or `*ngIf`)
+  //  and migrating would break narrowing currently.
   @Input() second = false;
+  // TODO: Skipped for migration because:
+  //  This input is used in a control flow expression (e.g. `@if` or `*ngIf`)
+  //  and migrating would break narrowing currently.
   @Input() third = true;
   readonly fourth = input(true);
   readonly fifth = input(true);
@@ -1171,8 +1216,14 @@ import {Component, Input, input} from '@angular/core';
   },
 })
 class TwoWayBinding {
+  // TODO: Skipped for migration because:
+  //  Your application code writes to the input. This prevents migration.
   @Input() inputA = '';
+  // TODO: Skipped for migration because:
+  //  Your application code writes to the input. This prevents migration.
   @Input() inputB = true;
+  // TODO: Skipped for migration because:
+  //  Your application code writes to the input. This prevents migration.
   @Input() inputC = false;
   readonly inputD = input(false);
 }
@@ -1196,6 +1247,9 @@ export class TransformFunctions {
   // This will be a synthetic type because we add `undefined` to `boolean`.
   readonly synthetic1 = input.required<boolean | undefined, string | undefined>({ transform: x });
   // Synthetic as we infer a full type from the initial value. Cannot be checked.
+  // TODO: Skipped for migration because:
+  //  Input is required, but the migration cannot determine a good type for the
+  //  input.
   @Input({required: true, transform: (v: string | undefined) => ''}) synthetic2 = {
     infer: COMPLEX_VAR,
   };
@@ -1220,6 +1274,8 @@ class TransformIncompatibleTypes {
 import {Input, input} from '@angular/core';
 
 export class WithSettersAndGetters {
+  // TODO: Skipped for migration because:
+  //  Accessor inputs cannot be migrated as they are too complex.
   @Input()
   set onlySetter(newValue: any) {
     this._bla = newValue;
@@ -1229,6 +1285,8 @@ export class WithSettersAndGetters {
   }
   private _bla: any;
 
+  // TODO: Skipped for migration because:
+  //  Accessor inputs cannot be migrated as they are too complex.
   @Input()
   get accessor(): string {
     return '';

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

@@ -318,6 +318,8 @@ import {Directive, Input} from '@angular/core';
 
 @Directive({})
 export class WithGetters {
+  // TODO: Skipped for migration because:
+  //  Accessor inputs cannot be migrated as they are too complex.
   @Input()
   get disabled() {
     return this._disabled;
@@ -474,6 +476,8 @@ export class AppComponent {
   readonly incompatible = input<string | null>(null);
 
   private _bla: any;
+  // TODO: Skipped for migration because:
+  //  Accessor inputs cannot be migrated as they are too complex.
   @Input()
   set ngSwitch(newValue: any) {
     this._bla = newValue;
@@ -1196,6 +1200,9 @@ export class TransformFunctions {
   // This will be a synthetic type because we add `undefined` to `boolean`.
   readonly synthetic1 = input.required<boolean | undefined, string | undefined>({ transform: x });
   // Synthetic as we infer a full type from the initial value. Cannot be checked.
+  // TODO: Skipped for migration because:
+  //  Input is required, but the migration cannot determine a good type for the
+  //  input.
   @Input({required: true, transform: (v: string | undefined) => ''}) synthetic2 = {
     infer: COMPLEX_VAR,
   };
@@ -1220,6 +1227,8 @@ class TransformIncompatibleTypes {
 import {Input, input} from '@angular/core';
 
 export class WithSettersAndGetters {
+  // TODO: Skipped for migration because:
+  //  Accessor inputs cannot be migrated as they are too complex.
   @Input()
   set onlySetter(newValue: any) {
     this._bla = newValue;
@@ -1229,6 +1238,8 @@ export class WithSettersAndGetters {
   }
   private _bla: any;
 
+  // TODO: Skipped for migration because:
+  //  Accessor inputs cannot be migrated as they are too complex.
   @Input()
   get accessor(): string {
     return '';

packages/core/schematics/ng-generate/inject-migration/BUILD.bazel

@@ -19,7 +19,7 @@ ts_library(
     tsconfig = "//packages/core/schematics:tsconfig.json",
     deps = [
         "//packages/core/schematics/utils",
-        "//packages/core/schematics/utils/tsurge",
+        "//packages/core/schematics/utils/tsurge/helpers/ast",
         "@npm//@angular-devkit/schematics",
         "@npm//@types/node",
         "@npm//typescript",

packages/core/schematics/ng-generate/signal-input-migration/index.ts

@@ -8,18 +8,18 @@
 
 import {Rule, SchematicsException} from '@angular-devkit/schematics';
 
-import assert from 'assert';
 import {SignalInputMigration} from '../../migrations/signal-migration/src';
 import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths';
 import {DevkitMigrationFilesystem} from '../../utils/tsurge/helpers/angular_devkit/devkit_filesystem';
 import {groupReplacementsByFile} from '../../utils/tsurge/helpers/group_replacements';
 import {setFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system';
 import {CompilationUnitData} from '../../migrations/signal-migration/src/batch/unit_data';
-import {ProjectRootRelativePath, Replacement, TextUpdate} from '../../utils/tsurge';
+import {ProjectRootRelativePath, TextUpdate} from '../../utils/tsurge';
 
 interface Options {
   path: string;
   bestEffortMode?: boolean;
+  insertTodos?: boolean;
   analysisDir: string;
 }
 
@@ -38,6 +38,7 @@ export function migrate(options: Options): Rule {
 
     const migration = new SignalInputMigration({
       bestEffortMode: options.bestEffortMode,
+      insertTodosForSkippedFields: options.insertTodos,
       shouldMigrateInput: (input) => {
         return (
           input.file.rootRelativePath.startsWith(fs.normalize(options.path)) &&

packages/core/schematics/ng-generate/signal-input-migration/schema.json

@@ -20,6 +20,11 @@
       "description": "Whether to eagerly migrate as much as possible, ignoring problematic patterns that would otherwise prevent migration.",
       "x-prompt": "Do you want to migrate as much as possible, even if it may break your build?",
       "default": false
+    },
+    "insertTodos": {
+      "type": "boolean",
+      "description": "Whether the migration should add TODOs for inputs that could not be migrated",
+      "default": false
     }
   }
 }