Merge pull request #84907 from hamishknight/fixed-point

[CS] Penalize implicit pointer conversions to generic parameter types

Author: hamishknight

include/swift/Sema/ConstraintSystem.h

@@ -1014,6 +1014,11 @@ enum ScoreKind: unsigned int {
   SK_EmptyExistentialConversion,
   /// A key path application subscript.
   SK_KeyPathSubscript,
+  /// A pointer conversion where the destination type is a generic parameter.
+  /// This should eventually be removed in favor of outright banning pointer
+  /// conversions for generic parameters. As such we consider it more impactful
+  /// than \c SK_ValueToPointerConversion.
+  SK_GenericParamPointerConversion,
   /// A conversion from a string, array, or inout to a pointer.
   SK_ValueToPointerConversion,
   /// A closure/function conversion to an autoclosure parameter.
@@ -1191,6 +1196,9 @@ struct Score {
     case SK_KeyPathSubscript:
       return "key path subscript";
 
+    case SK_GenericParamPointerConversion:
+      return "pointer conversion to generic parameter";
+
     case SK_ValueToPointerConversion:
       return "value-to-pointer conversion";
 

lib/Sema/CSSimplify.cpp

@@ -15403,6 +15403,50 @@ ConstraintSystem::simplifyRestrictedConstraintImpl(
   llvm_unreachable("bad conversion restriction");
 }
 
+static void increaseScoreForGenericParamPointerConversion(
+    ConstraintSystem &cs, ConversionRestrictionKind kind,
+    ConstraintLocatorBuilder locator) {
+  switch (kind) {
+  case ConversionRestrictionKind::InoutToPointer:
+  case ConversionRestrictionKind::ArrayToPointer:
+  case ConversionRestrictionKind::StringToPointer:
+  case ConversionRestrictionKind::PointerToPointer:
+    break;
+  default:
+    return;
+  }
+
+  auto *loc = cs.getConstraintLocator(locator);
+  auto argInfo = loc->findLast<LocatorPathElt::ApplyArgToParam>();
+  if (!argInfo)
+    return;
+
+  auto overload = cs.findSelectedOverloadFor(cs.getCalleeLocator(loc));
+  if (!overload)
+    return;
+
+  auto *D = overload->choice.getDeclOrNull();
+  if (!D)
+    return;
+
+  auto *param = getParameterAt(D, argInfo->getParamIdx());
+  if (!param)
+    return;
+
+  // Check to see if the parameter is a generic parameter, or dependent member.
+  auto paramTy = param->getInterfaceType()->lookThroughAllOptionalTypes();
+  if (!paramTy->isTypeParameter())
+    return;
+
+  // Don't increase the score if the type parameter is rooted on the protocol
+  // 'Self' type, e.g extensions on `_Pointer` shouldn't be penalized.
+  if (auto *PD = D->getDeclContext()->getSelfProtocolDecl()) {
+    if (PD->getSelfInterfaceType()->isEqual(paramTy->getRootGenericParam()))
+      return;
+  }
+  cs.increaseScore(SK_GenericParamPointerConversion, locator);
+}
+
 ConstraintSystem::SolutionKind
 ConstraintSystem::simplifyRestrictedConstraint(
                                        ConversionRestrictionKind restriction,
@@ -15430,6 +15474,13 @@ ConstraintSystem::simplifyRestrictedConstraint(
       addFixConstraint(fix, matchKind, type1, type2, locator);
     }
 
+    // Increase the score if needed for a pointer conversion to a generic
+    // parameter type.
+    // FIXME: We ought to consider outright banning pointer conversions to
+    // generic parameter types, in which case this logic could be adjusted to
+    // record a fix instead.
+    increaseScoreForGenericParamPointerConversion(*this, restriction, locator);
+
     addConversionRestriction(type1, type2, restriction);
     return SolutionKind::Solved;
   }

test/Constraints/valid_pointer_conversions.swift

@@ -88,3 +88,31 @@ do {
   let _: UInt8 = result1 // Ok
   let _: [UInt8] = result2 // Ok
 }
+
+protocol PointerProtocol {}
+extension UnsafePointer: PointerProtocol {}
+
+extension PointerProtocol {
+  func foo(_ x: Self) {} // expected-note {{found this candidate}}
+  func foo(_ x: UnsafePointer<CChar>) {} // expected-note {{found this candidate}}
+}
+
+func testGenericPointerConversions(
+  chars: [CChar], mutablePtr: UnsafeMutablePointer<CChar>, ptr: UnsafePointer<CChar>
+) {
+  func id<T>(_ x: T) -> T { x }
+  func optID<T>(_ x: T?) -> T { x! }
+  func takesCharPtrs(_: UnsafePointer<CChar>, _: UnsafePointer<CChar>?) {}
+
+  // Make sure we don't end up with an ambiguity here, we should prefer to
+  // do the pointer conversion for `takesPtrs` not `id`.
+  takesCharPtrs(chars, "a")
+  takesCharPtrs(id(chars), id("a"))
+  takesCharPtrs(id("a"), optID(chars))
+  takesCharPtrs(mutablePtr, mutablePtr)
+  takesCharPtrs(id(mutablePtr), id(mutablePtr))
+  takesCharPtrs(id(mutablePtr), optID(mutablePtr))
+
+  // Make sure this is ambiguous.
+  ptr.foo(chars) // expected-error {{ambiguous use of 'foo'}}
+}

unittests/Sema/ConstraintSystemDumpTests.cpp

@@ -33,7 +33,7 @@ TEST_F(SemaTest, DumpConstraintSystemBasic) {
       TupleType::get({Type(t0), Type(t1)}, Context), emptyLoc));
 
   std::string expectedOutput =
-      R"(Score: <default 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0>
+      R"(Score: <default 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0>
 Type Variables:
   $T0 [can bind to: lvalue] [adjacent to: $T1, $T2] [potential bindings: <none>] @ locator@ []
   $T1 [adjacent to: $T0, $T2] [potential bindings: <none>] @ locator@ []