Various improvements for the signal input migration

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

@@ -74,6 +74,7 @@ export function extract(absoluteTsconfigPath: string) {
           from: {
             fileId: r.from.fileId,
             node: {positionEndInFile: r.from.node.getEnd()},
+            isWrite: r.from.isWrite,
           },
         };
       } else if (isHostBindingInputReference(r)) {

packages/core/schematics/migrations/signal-migration/src/convert-input/convert_to_signal.ts

@@ -12,6 +12,7 @@ import ts from 'typescript';
 import {MigrationHost} from '../migration_host';
 import {ConvertInputPreparation} from './prepare_and_check';
 import {DecoratorInputTransform} from '../../../../../../compiler-cli/src/ngtsc/metadata';
+import {ImportManager} from '../../../../../../compiler-cli/src/ngtsc/translator';
 
 const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed});
 
@@ -26,6 +27,7 @@ export function convertToSignalInput(
   node: ts.PropertyDeclaration,
   {resolvedMetadata: metadata, resolvedType, isResolvedTypeCheckable}: ConvertInputPreparation,
   checker: ts.TypeChecker,
+  importManager: ImportManager,
 ): string {
   let initialValue = node.initializer;
   let optionsLiteral: ts.ObjectLiteralExpression | null = null;
@@ -89,11 +91,11 @@ export function convertToSignalInput(
     inputArgs.push(optionsLiteral);
   }
 
-  const inputFnRef =
-    metadata.inputDecoratorRef !== null && ts.isPropertyAccessExpression(metadata.inputDecoratorRef)
-      ? ts.factory.createPropertyAccessExpression(metadata.inputDecoratorRef.expression, 'input')
-      : ts.factory.createIdentifier('input');
-
+  const inputFnRef = importManager.addImport({
+    exportModuleSpecifier: '@angular/core',
+    exportSymbolName: 'input',
+    requestedFile: node.getSourceFile(),
+  });
   const inputInitializerFn = metadata.required
     ? ts.factory.createPropertyAccessExpression(inputFnRef, 'required')
     : inputFnRef;

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

@@ -16,13 +16,14 @@ import {executeAnalysisPhase} from './phase_analysis';
 import {executeMigrationPhase} from './phase_migrate';
 import {MigrationResult} from './result';
 import {writeMigrationReplacements} from './write_replacements';
+import {nonIgnorableIncompatibilities} from './input_detection/incompatibility';
 
-main(path.resolve(process.argv[2]));
+main(path.resolve(process.argv[2]), process.argv.includes('--best-effort-mode'));
 
 /**
  * Runs the signal input migration for the given TypeScript project.
  */
-export function main(absoluteTsconfigPath: string) {
+export function main(absoluteTsconfigPath: string, bestEffortMode: boolean) {
   const analysisDeps = createAndPrepareAnalysisProgram(absoluteTsconfigPath);
   const {tsHost, tsconfig, basePath, sourceFiles, metaRegistry} = analysisDeps;
   const knownInputs = new KnownInputs();
@@ -35,9 +36,22 @@ export function main(absoluteTsconfigPath: string) {
   );
 
   const {inheritanceGraph} = executeAnalysisPhase(host, knownInputs, result, analysisDeps);
-
   pass4__checkInheritanceOfInputs(host, inheritanceGraph, metaRegistry, knownInputs);
 
+  // Remove all "ignorable" incompatibilities of inputs, if best effort mode is requested.
+  if (bestEffortMode) {
+    knownInputs.knownInputIds.forEach(({container: c}) => {
+      if (c.incompatible !== null && !nonIgnorableIncompatibilities.includes(c.incompatible)) {
+        c.incompatible = null;
+      }
+      for (const [key, i] of c.memberIncompatibility.entries()) {
+        if (!nonIgnorableIncompatibilities.includes(i.reason)) {
+          c.memberIncompatibility.delete(key);
+        }
+      }
+    });
+  }
+
   executeMigrationPhase(host, knownInputs, result, analysisDeps);
 
   // Apply replacements

packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.md

@@ -0,0 +1,85 @@
+# Possible reasons why an `@Input` is not migrated
+
+The signal input migration may skip migration of `@Input()` declarations if the automated refactoring isn't possible or safe.
+This document explains some of the reasons, or better known "migration incompatibilities".
+
+### `REASON:Accessor`
+The input is declared using a getter/setter.
+This is non-trivial to migrate without knowing the intent.
+
+### `REASON:WriteAssignment`
+Parts of your application write to this input class field.
+This blocks the automated migration because signal inputs cannot be modified programmatically.
+
+### `REASON:OverriddenByDerivedClass`
+The input is part of a class that is extended by another class which overrides the field.
+Migrating the input would then cause type conflicts and break the application build.
+
+```ts
+@Component()
+class MyComp {
+  @Input() myInput = true;
+}
+
+class Derived extends MyComp {
+  override myInput = false; // <-- this wouldn't be a signal input and break!
+}
+```
+
+### `REASON:RedeclaredViaDerivedClassInputsArray`
+The input is part of a class that is extended by another class which overrides the field via the `inputs` array of `@Component` or `@Directive`.
+Migrating the input would cause a mismatch because fields declared via `inputs` cannot be signal inputs.
+
+```ts
+@Component()
+class MyComp {
+  @Input() myInput = true;
+}
+
+@Component({
+  inputs: ['myInput: aliasedName']
+})
+class Derived extends MyComp {}
+```
+
+### `REASON:TypeConflictWithBaseClass`
+The input is overriding a field from a superclass, but the superclass field is not an Angular `@Input` and is not migrated.
+This results in a type conflict and would break the build.
+
+```ts
+interface Parent {
+  disabled: boolean;
+}
+
+@Component()
+class MyComp implements Parent {
+  @Input() disabled = true;
+}
+```
+
+### `REASON:ParentIsIncompatible`
+The input is overriding a field from a superclass, but the superclass field could not be migrated.
+This means that migrating the input would break the build.
+
+### `REASON:SpyOnThatOverwritesField`
+The input can be migrated, but a Jasmine `spyOn` call for the input field was discovered.
+`spyOn` calls are incompatible with signal inputs because they attempt to override the value of the field.
+Signal inputs cannot be changed programmatically though— so this breaks.
+
+### `REASON:PotentiallyNarrowedInTemplateButNoSupportYet`
+The input is part of an `@if` or `*ngIf`, or template input in general.
+This indicates that the input value type may be narrowed.
+
+The migration skips migrating such inputs because support for narrowed signals is not available yet.
+
+### `REASON:RequiredInputButNoGoodExplicitTypeExtractable`
+The input is required, but cannot be safely migrated because no good type could be detected.
+
+A required input with initial value doesn't make sense.
+The type is inferred with `@Input` via the initial value, but this isn't possible with `input.required<T>`.
+
+```ts
+class MyComp {
+  @Input({required: true}) bla = someComplexInitialValue();
+}
+```

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

@@ -17,15 +17,15 @@ export enum InputIncompatibilityReason {
   TypeConflictWithBaseClass,
   ParentIsIncompatible,
   SpyOnThatOverwritesField,
-  NarrowedInTemplateButNotSupportedYetTODO,
+  PotentiallyNarrowedInTemplateButNoSupportYet,
   IgnoredBecauseOfLanguageServiceRefactoringRange,
   RequiredInputButNoGoodExplicitTypeExtractable,
 }
 
 /** Reasons why a whole class and its inputs cannot be migrated. */
 export enum ClassIncompatibilityReason {
   ClassManuallyInstantiated,
-  ClassReferencedInPotentiallyBadLocation,
+  InputOwningClassReferencedInClassProperty,
 }
 
 /** Description of an input incompatibility and some helpful debug context. */
@@ -34,6 +34,16 @@ export interface InputMemberIncompatibility {
   context: ts.Node | null;
 }
 
+/** Reasons that cannot be ignored. */
+export const nonIgnorableIncompatibilities: Array<
+  InputIncompatibilityReason | ClassIncompatibilityReason
+> = [
+  // There is no good output for accessor inputs.
+  InputIncompatibilityReason.Accessor,
+  // There is no good output for such inputs. We can't perform "conversion".
+  InputIncompatibilityReason.RequiredInputButNoGoodExplicitTypeExtractable,
+];
+
 /** Whether the given value refers to an input member incompatibility. */
 export function isInputMemberIncompatibility(value: unknown): value is InputMemberIncompatibility {
   return (

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

@@ -34,7 +34,6 @@ import {InputNode, isInputContainerNode} from '../input_detection/input_node';
 /** Metadata extracted of an input declaration (in `.ts` or `.d.ts` files). */
 export interface ExtractedInput extends InputMapping {
   inSourceFile: boolean;
-  inputDecoratorRef: DecoratorIdentifier | null;
 }
 
 /** Attempts to extract metadata of a potential TypeScript `@Input()` declaration. */
@@ -87,7 +86,6 @@ function extractDtsInput(node: ts.Node, metadataReader: DtsMetadataReader): Extr
     ? null
     : {
         ...inputMapping,
-        inputDecoratorRef: null,
         inSourceFile: false,
       };
 }
@@ -152,7 +150,6 @@ function extractSourceCodeInput(
     isSignal: false,
     inSourceFile: true,
     transform: transformResult,
-    inputDecoratorRef: inputDecorator.identifier,
   };
 }
 

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

@@ -17,6 +17,7 @@ import {
   PropertyRead,
   PropertyWrite,
   RecursiveAstVisitor,
+  SafePropertyRead,
   ThisReceiver,
   TmplAstBoundAttribute,
   TmplAstBoundEvent,
@@ -274,12 +275,15 @@ export class TemplateExpressionReferenceVisitor<ExprContext> extends RecursiveAs
     super.visitLiteralMap(ast, context);
   }
 
-  // TODO: Consider safe property reads/writes.
-
   override visitPropertyRead(ast: PropertyRead) {
     this._inspectPropertyAccess(ast);
     super.visitPropertyRead(ast, null);
   }
+  override visitSafePropertyRead(ast: SafePropertyRead) {
+    this._inspectPropertyAccess(ast);
+    super.visitPropertyRead(ast, null);
+  }
+
   override visitPropertyWrite(ast: PropertyWrite) {
     this._inspectPropertyAccess(ast);
     super.visitPropertyWrite(ast, null);

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

@@ -0,0 +1,59 @@
+/**
+ * @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 ts from 'typescript';
+import {ImportManager} from '../../../../../../compiler-cli/src/ngtsc/translator';
+import {Replacement} from '../replacement';
+import {MigrationResult} from '../result';
+
+/**
+ * Phase that applies all changes recorded by the import manager in
+ * previous migrate phases.
+ */
+export function pass10_applyImportManager(
+  importManager: ImportManager,
+  result: MigrationResult,
+  sourceFiles: readonly ts.SourceFile[],
+) {
+  const {newImports, updatedImports, deletedImports} = importManager.finalize();
+  const printer = ts.createPrinter({});
+  const pathToFile = new Map<string, ts.SourceFile>(sourceFiles.map((s) => [s.fileName, s]));
+
+  // Capture new imports
+  newImports.forEach((newImports, fileName) => {
+    newImports.forEach((newImport) => {
+      const printedImport = printer.printNode(
+        ts.EmitHint.Unspecified,
+        newImport,
+        pathToFile.get(fileName)!,
+      );
+      result.addReplacement(fileName, new Replacement(0, 0, `${printedImport}\n`));
+    });
+  });
+
+  // Capture updated imports
+  for (const [oldBindings, newBindings] of updatedImports.entries()) {
+    const printedBindings = printer.printNode(
+      ts.EmitHint.Unspecified,
+      newBindings,
+      oldBindings.getSourceFile(),
+    );
+    result.addReplacement(
+      oldBindings.getSourceFile().fileName,
+      new Replacement(oldBindings.getStart(), oldBindings.getEnd(), printedBindings),
+    );
+  }
+
+  // Update removed imports
+  for (const removedImport of deletedImports) {
+    result.addReplacement(
+      removedImport.getSourceFile().fileName,
+      new Replacement(removedImport.getStart(), removedImport.getEnd(), ''),
+    );
+  }
+}

packages/core/schematics/migrations/signal-migration/src/passes/2_find_source_file_references.ts

@@ -19,6 +19,7 @@ import {identifyTemplateReferences} from './references/identify_template_referen
 import {identifyPotentialTypeScriptReference} from './references/identify_ts_references';
 import {PartialDirectiveTypeInCatalystTests} from '../pattern_advisors/partial_directive_type';
 import {InputReferenceKind} from '../utils/input_reference';
+import {GroupedTsAstVisitor} from '../utils/grouped_ts_ast_visitor';
 
 /**
  * Phase where we iterate through all source file references and
@@ -34,11 +35,11 @@ import {InputReferenceKind} from '../utils/input_reference';
  *    - Host binding expressions.
  */
 export function pass2_IdentifySourceFileReferences(
-  sf: ts.SourceFile,
   host: MigrationHost,
   checker: ts.TypeChecker,
   reflector: ReflectionHost,
   templateTypeChecker: TemplateTypeChecker,
+  groupedTsAstVisitor: GroupedTsAstVisitor,
   knownInputs: KnownInputs,
   result: MigrationResult,
 ) {
@@ -86,8 +87,7 @@ export function pass2_IdentifySourceFileReferences(
         target: partialDirectiveInCatalyst.targetClass,
       });
     }
-
-    ts.forEachChild(node, visitor);
   };
-  ts.forEachChild(sf, visitor);
+
+  groupedTsAstVisitor.register(visitor);
 }