[TypeChecker] Adjust Double<->CGFloat conversion to always preserve its location

Unfortunately current approach of making a conversion independent of location
doesn't work when conversion is required for multiple arguments to the
same call because solver expects that either there are no Double<->CGFloat
conversions, or one of them has already been applied which is not the case.

The reason why locations weren't preserved in the first place is due to
how a solution is applied to AST - AST is mutated first and then, if there
are any conversions, they are applied to the already mutated version of
original AST. This creates a problem for Double<->CGFloat which depends
on an overload choice of injected call and it's impossible to find it based
on the mutated AST. But it turns out that this is only an issue in two
specific cases - conversions against contextual type and after optional injection.
This situations could be mitigated by dropping parts of the locator which are
unimportant for the Double<->CGFloat conversion - anchor in case of contextual
and `OptionalPayload` element(s) in case of optional injection.

Resolves: #59374

Author: xedin

include/swift/Sema/ConstraintSystem.h

@@ -3702,6 +3702,12 @@ class ConstraintSystem {
   ConstraintLocator *
   getConstraintLocator(const ConstraintLocatorBuilder &builder);
 
+  /// Compute a constraint locator for an implicit value-to-value
+  /// conversion rooted at the given location.
+  ConstraintLocator *
+  getImplicitValueConversionLocator(ConstraintLocatorBuilder root,
+                                    ConversionRestrictionKind restriction);
+
   /// Lookup and return parent associated with given expression.
   Expr *getParentExpr(Expr *expr) {
     if (auto result = getExprDepthAndParent(expr))

lib/Sema/CSApply.cpp

@@ -6779,10 +6779,13 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
             callLocator, {ConstraintLocator::ApplyFunction,
                           ConstraintLocator::ConstructorMember});
 
-        auto overload = solution.getOverloadChoice(cs.getConstraintLocator(
-            ASTNode(), {LocatorPathElt::ImplicitConversion(conversionKind),
-                        ConstraintLocator::ApplyFunction,
-                        ConstraintLocator::ConstructorMember}));
+        ConstraintLocator *baseLoc =
+            cs.getImplicitValueConversionLocator(locator, conversionKind);
+
+        auto overload =
+            solution.getOverloadChoice(solution.getConstraintLocator(
+                baseLoc, {ConstraintLocator::ApplyFunction,
+                          ConstraintLocator::ConstructorMember}));
 
         solution.overloadChoices.insert({memberLoc, overload});
       }
@@ -8992,15 +8995,19 @@ ExprWalker::rewriteTarget(SolutionApplicationTarget target) {
     // If we're supposed to convert the expression to some particular type,
     // do so now.
     if (shouldCoerceToContextualType()) {
-      resultExpr =
-          Rewriter.coerceToType(resultExpr, solution.simplifyType(convertType),
-                                cs.getConstraintLocator(resultExpr));
+      resultExpr = Rewriter.coerceToType(
+          resultExpr, solution.simplifyType(convertType),
+          cs.getConstraintLocator(resultExpr,
+                                  LocatorPathElt::ContextualType(
+                                      target.getExprContextualTypePurpose())));
     } else if (cs.getType(resultExpr)->hasLValueType() &&
                !target.isDiscardedExpr()) {
       // We referenced an lvalue. Load it.
       resultExpr = Rewriter.coerceToType(
           resultExpr, cs.getType(resultExpr)->getRValueType(),
-          cs.getConstraintLocator(resultExpr));
+          cs.getConstraintLocator(resultExpr,
+                                  LocatorPathElt::ContextualType(
+                                      target.getExprContextualTypePurpose())));
     }
 
     if (!resultExpr)

lib/Sema/CSSimplify.cpp

@@ -12400,28 +12400,26 @@ ConstraintSystem::simplifyRestrictedConstraintImpl(
     if (worseThanBestSolution())
       return SolutionKind::Error;
 
-    auto *conversionLoc = getConstraintLocator(
-        /*anchor=*/ASTNode(), LocatorPathElt::ImplicitConversion(restriction));
+    auto *conversionLoc =
+        getImplicitValueConversionLocator(locator, restriction);
 
     auto *applicationLoc =
         getConstraintLocator(conversionLoc, ConstraintLocator::ApplyFunction);
 
     auto *memberLoc = getConstraintLocator(
         applicationLoc, ConstraintLocator::ConstructorMember);
 
-    // Conversion has been already attempted for this direction
-    // and constructor choice has been recorded.
-    if (findSelectedOverloadFor(memberLoc))
-      return SolutionKind::Solved;
-
     // Allocate a single argument info to cover all possible
     // Double <-> CGFloat conversion locations.
-    if (!ArgumentLists.count(memberLoc)) {
+    auto *argumentsLoc =
+        getConstraintLocator(conversionLoc, ConstraintLocator::ApplyArgument);
+
+    if (!ArgumentLists.count(argumentsLoc)) {
       auto *argList = ArgumentList::createImplicit(
           getASTContext(), {Argument(SourceLoc(), Identifier(), nullptr)},
           /*firstTrailingClosureIndex=*/None,
           AllocationArena::ConstraintSolver);
-      ArgumentLists.insert({memberLoc, argList});
+      ArgumentLists.insert({argumentsLoc, argList});
     }
 
     auto *memberTypeLoc = getConstraintLocator(

lib/Sema/ConstraintSystem.cpp

@@ -424,21 +424,43 @@ ConstraintLocator *ConstraintSystem::getConstraintLocator(
   return getConstraintLocator(anchor, newPath);
 }
 
+ConstraintLocator *ConstraintSystem::getImplicitValueConversionLocator(
+    ConstraintLocatorBuilder root, ConversionRestrictionKind restriction) {
+  SmallVector<LocatorPathElt, 4> path;
+  auto anchor = root.getLocatorParts(path);
+  {
+    // Drop any value-to-optional conversions that were applied along the
+    // way to reach this one.
+    while (!path.empty()) {
+      if (path.back().is<LocatorPathElt::OptionalPayload>()) {
+        path.pop_back();
+        continue;
+      }
+      break;
+    }
+
+    // If the conversion is associated with a contextual type e.g.
+    // `_: Double = CGFloat(1)` then drop `ContextualType` so that
+    // it's easy to find when the underlying expression has been
+    // rewritten.
+    if (!path.empty() && path.back().is<LocatorPathElt::ContextualType>()) {
+      anchor = ASTNode();
+      path.clear();
+    }
+  }
+
+  return getConstraintLocator(/*base=*/getConstraintLocator(anchor, path),
+                              LocatorPathElt::ImplicitConversion(restriction));
+}
+
 ConstraintLocator *ConstraintSystem::getCalleeLocator(
     ConstraintLocator *locator, bool lookThroughApply,
     llvm::function_ref<Type(Expr *)> getType,
     llvm::function_ref<Type(Type)> simplifyType,
     llvm::function_ref<Optional<SelectedOverload>(ConstraintLocator *)>
         getOverloadFor) {
-  if (auto conversion =
-          locator->findLast<LocatorPathElt::ImplicitConversion>()) {
-    if (conversion->is(ConversionRestrictionKind::DoubleToCGFloat) ||
-        conversion->is(ConversionRestrictionKind::CGFloatToDouble)) {
-      return getConstraintLocator(
-          ASTNode(), {*conversion, ConstraintLocator::ApplyFunction,
-                      ConstraintLocator::ConstructorMember});
-    }
-  }
+  if (locator->findLast<LocatorPathElt::ImplicitConversion>())
+    return locator;
 
   auto anchor = locator->getAnchor();
   auto path = locator->getPath();
@@ -5323,6 +5345,9 @@ ConstraintSystem::getArgumentInfoLocator(ConstraintLocator *locator) {
   if (anchor.isNull() && locator->getPath().empty())
     return nullptr;
 
+  if (locator->findLast<LocatorPathElt::ImplicitConversion>())
+    return locator;
+
   // Applies and unresolved member exprs can have callee locators that are
   // dependent on the type of their function, which may not have been resolved
   // yet. Therefore we need to handle them specially.

test/Constraints/implicit_double_cgfloat_conversion.swift

@@ -194,18 +194,25 @@ func test_no_ambiguity_with_unary_operators(width: CGFloat, height: CGFloat) {
 }
 
 func test_conversions_with_optional_promotion(d: Double, cgf: CGFloat) {
-  func test_double(_: Double??) {}
-  func test_cgfloat(_: CGFloat??) {}
+  func test_double(_: Double??, _: Double???) {}
+  func test_cgfloat(_: CGFloat??, _: CGFloat???) {}
 
   // CHECK: function_ref @$sSd12CoreGraphicsEySdAA7CGFloatVcfC
   // CHECK-NEXT: apply
   // CHECK-NEXT: enum $Optional<Double>, #Optional.some!enumelt
   // CHECK-NEXT: enum $Optional<Optional<Double>>, #Optional.some!enumelt
-  test_double(cgf)
+  test_double(cgf, cgf)
 
   // CHECK: function_ref @$s12CoreGraphics7CGFloatVyACSdcfC
   // CHECK-NEXT: apply
   // CHECK-NEXT: enum $Optional<CGFloat>, #Optional.some!enumelt
   // CHECK-NEXT: enum $Optional<Optional<CGFloat>>, #Optional.some!enumelt
-  test_cgfloat(d)
+  test_cgfloat(d, d)
+}
+
+// https://github.com/apple/swift/issues/59374
+func test_multi_argument_conversion_with_optional(d: Double, cgf: CGFloat) {
+  func test(_: Double, _: CGFloat?) {}
+
+  test(cgf, d) // Ok (CGFloat -> Double and Double? -> CGFloat?)
 }