refactor(migrations): properly migrate inputs marked as optional via question mark (angular#58031)

Currently if inputs are marked as optional via the question mark syntax,
we add `undefined` only if there is an explicit type. This is wrong as
we should do the same if there is just an initializer.

This commit fixes this.

PR Close angular#58031

Author: devversion

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

@@ -91,15 +91,33 @@ export function prepareAndCheckForConversion(
   // If the input is using `@Input() bla?: string;` with the "optional question mark",
   // then we try to explicitly add `undefined` as type, if it's not part of the type already.
   // This is ensuring correctness, as `bla?` automatically includes `undefined` currently.
-  if (
-    node.type !== undefined &&
-    node.questionToken !== undefined &&
-    !checker.isTypeAssignableTo(checker.getUndefinedType(), checker.getTypeFromTypeNode(node.type))
-  ) {
-    typeToAdd = ts.factory.createUnionTypeNode([
-      node.type,
-      ts.factory.createKeywordTypeNode(ts.SyntaxKind.UndefinedKeyword),
-    ]);
+  if (node.questionToken !== undefined) {
+    // If there is no type, but we have an initial value, try inferring
+    // it from the initializer.
+    if (typeToAdd === undefined && initialValue !== undefined) {
+      const inferredType = inferImportableTypeForInput(checker, node, initialValue);
+      if (inferredType !== null) {
+        typeToAdd = inferredType;
+      }
+    }
+    if (typeToAdd === undefined) {
+      return {
+        context: node,
+        reason: InputIncompatibilityReason.InputWithQuestionMarkButNoGoodExplicitTypeExtractable,
+      };
+    }
+
+    if (
+      !checker.isTypeAssignableTo(
+        checker.getUndefinedType(),
+        checker.getTypeFromTypeNode(typeToAdd),
+      )
+    ) {
+      typeToAdd = ts.factory.createUnionTypeNode([
+        typeToAdd,
+        ts.factory.createKeywordTypeNode(ts.SyntaxKind.UndefinedKeyword),
+      ]);
+    }
   }
 
   let leadingTodoText: string | null = null;
@@ -128,25 +146,9 @@ export function prepareAndCheckForConversion(
   // Attempt to extract type from input initial value. No explicit type, but input is required.
   // Hence we need an explicit type, or fall back to `typeof`.
   if (typeToAdd === undefined && initialValue !== undefined && metadata.required) {
-    const propertyType = checker.getTypeAtLocation(node);
-    if (propertyType.flags & ts.TypeFlags.Boolean) {
-      typeToAdd = ts.factory.createKeywordTypeNode(ts.SyntaxKind.BooleanKeyword);
-    } else if (propertyType.flags & ts.TypeFlags.String) {
-      typeToAdd = ts.factory.createKeywordTypeNode(ts.SyntaxKind.StringKeyword);
-    } else if (propertyType.flags & ts.TypeFlags.Number) {
-      typeToAdd = ts.factory.createKeywordTypeNode(ts.SyntaxKind.NumberKeyword);
-    } else if (ts.isIdentifier(initialValue)) {
-      // @Input({required: true}) bla = SOME_DEFAULT;
-      typeToAdd = ts.factory.createTypeQueryNode(initialValue);
-    } else if (
-      ts.isPropertyAccessExpression(initialValue) &&
-      ts.isIdentifier(initialValue.name) &&
-      ts.isIdentifier(initialValue.expression)
-    ) {
-      // @Input({required: true}) bla = prop.SOME_DEFAULT;
-      typeToAdd = ts.factory.createTypeQueryNode(
-        ts.factory.createQualifiedName(initialValue.name, initialValue.expression),
-      );
+    const inferredType = inferImportableTypeForInput(checker, node, initialValue);
+    if (inferredType !== null) {
+      typeToAdd = inferredType;
     } else {
       // Note that we could use `typeToTypeNode` here but it's likely breaking because
       // the generated type might depend on imports that we cannot add here (nor want).
@@ -167,3 +169,49 @@ export function prepareAndCheckForConversion(
     leadingTodoText,
   };
 }
+
+function inferImportableTypeForInput(
+  checker: ts.TypeChecker,
+  node: InputNode,
+  initialValue: ts.Node,
+): ts.TypeNode | null {
+  const propertyType = checker.getTypeAtLocation(node);
+
+  // If the resolved type is a primitive, or union of primitive types,
+  // return a type node fully derived from the resolved type.
+  if (
+    isPrimitiveImportableTypeNode(propertyType) ||
+    (propertyType.isUnion() && propertyType.types.every(isPrimitiveImportableTypeNode))
+  ) {
+    return checker.typeToTypeNode(propertyType, node, ts.NodeBuilderFlags.NoTypeReduction) ?? null;
+  }
+
+  // Alternatively, try to infer a simple importable type from\
+  // the initializer.
+
+  if (ts.isIdentifier(initialValue)) {
+    // @Input({required: true}) bla = SOME_DEFAULT;
+    return ts.factory.createTypeQueryNode(initialValue);
+  } else if (
+    ts.isPropertyAccessExpression(initialValue) &&
+    ts.isIdentifier(initialValue.name) &&
+    ts.isIdentifier(initialValue.expression)
+  ) {
+    // @Input({required: true}) bla = prop.SOME_DEFAULT;
+    return ts.factory.createTypeQueryNode(
+      ts.factory.createQualifiedName(initialValue.name, initialValue.expression),
+    );
+  }
+
+  return null;
+}
+
+function isPrimitiveImportableTypeNode(type: ts.Type): boolean {
+  return !!(
+    type.flags & ts.TypeFlags.BooleanLike ||
+    type.flags & ts.TypeFlags.StringLike ||
+    type.flags & ts.TypeFlags.NumberLike ||
+    type.flags & ts.TypeFlags.Undefined ||
+    type.flags & ts.TypeFlags.Null
+  );
+}

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

@@ -22,10 +22,11 @@ export enum InputIncompatibilityReason {
   SpyOnThatOverwritesField = 5,
   PotentiallyNarrowedInTemplateButNoSupportYet = 6,
   RequiredInputButNoGoodExplicitTypeExtractable = 7,
-  WriteAssignment = 8,
-  Accessor = 9,
-  OutsideOfMigrationScope = 10,
-  SkippedViaConfigFilter = 11,
+  InputWithQuestionMarkButNoGoodExplicitTypeExtractable = 8,
+  WriteAssignment = 9,
+  Accessor = 10,
+  OutsideOfMigrationScope = 11,
+  SkippedViaConfigFilter = 12,
 }
 
 /** Reasons why a whole class and its inputs cannot be migrated. */

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

@@ -53,6 +53,13 @@ export function getMessageForInputIncompatibility(reason: InputIncompatibilityRe
         short: `Input is required, but the migration cannot determine a good type for the input.`,
         extra: 'Consider adding an explicit type to make the migration possible.',
       };
+    case InputIncompatibilityReason.InputWithQuestionMarkButNoGoodExplicitTypeExtractable:
+      return {
+        short: `Input is marked with a question mark. Migration could not determine a good type for the input.`,
+        extra:
+          'The migration needs to be able to resolve a type, so that it can include `undefined` in your type. ' +
+          'Consider adding an explicit type to make the migration possible.',
+      };
     case InputIncompatibilityReason.SkippedViaConfigFilter:
       return {
         short: `This input is not part of the current migration scope.`,

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

@@ -5,4 +5,5 @@ import {Directive, Input} from '@angular/core';
 @Directive()
 class OptionalInput {
   @Input() bla?: string;
+  @Input() isLegacyHttpOnly? = false;
 }

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

@@ -942,6 +942,7 @@ import {Directive, input} from '@angular/core';
 @Directive()
 class OptionalInput {
   readonly bla = input<string>();
+  readonly isLegacyHttpOnly = input<boolean | undefined>(false);
 }
 @@@@@@ problematic_type_reference.ts @@@@@@
 

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

@@ -915,6 +915,7 @@ import {Directive, input} from '@angular/core';
 @Directive()
 class OptionalInput {
   readonly bla = input<string>();
+  readonly isLegacyHttpOnly = input<boolean | undefined>(false);
 }
 @@@@@@ problematic_type_reference.ts @@@@@@