Merge pull request #59762 from xedin/double-cgfloat-conv-improvements

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

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

@@ -11081,9 +11081,19 @@ bool ConstraintSystem::simplifyAppliedOverloadsImpl(
           return true;
         }
 
-        // If types lined up exactly, let's favor this overload choice.
-        if (Type(argFnType)->isEqual(choiceType))
-          constraint->setFavored();
+        // If types of arguments/parameters and result lined up exactly,
+        // let's favor this overload choice.
+        //
+        // Note this check ignores `ExtInfo` on purpose and only compares
+        // types, if there are overloads that differ only in effects then
+        // all of them are going to be considered and filtered as part of
+        // "favored" group after forming a valid partial solution.
+        if (auto *choiceFnType = choiceType->getAs<FunctionType>()) {
+          if (FunctionType::equalParams(argFnType->getParams(),
+                                        choiceFnType->getParams()) &&
+              argFnType->getResult()->isEqual(choiceFnType->getResult()))
+            constraint->setFavored();
+        }
 
         // Account for any optional unwrapping/binding
         for (unsigned i : range(numOptionalUnwraps)) {
@@ -12442,28 +12452,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?)
 }

test/Constraints/overload_filtering_objc.swift

@@ -22,3 +22,24 @@ func testOptional(obj: P) {
   // CHECK: [disabled] $T2 bound to decl overload_filtering_objc.(file).P.opt(double:)
   _ = obj.opt?(1)
 }
+
+
+func test_double_cgfloat_conversion_filtering(d: Double, cgf: CGFloat) {
+  // CHECK: [favored] $T{{.*}} bound to decl CoreGraphics.(file).CGFloat.init(_:)@{{.*}} : (CGFloat.Type) -> (Double) -> CGFloat {{.*}} -> implicit conversion [Double-to-CGFloat] -> apply function -> constructor member
+  let _: CGFloat = d
+
+  // CHECK: [favored] $T{{.*}} bound to decl CoreGraphics.(file).Double extension.init(_:)@{{.*}} : (Double.Type) -> (CGFloat) -> Double {{.*}} -> implicit conversion [CGFloat-to-Double] -> apply function -> constructor member
+  let _: Double = cgf
+
+  func test_optional_cgf(_: CGFloat??) {
+  }
+
+  func test_optional_double(_: Double??) {
+  }
+
+  // CHECK: [favored] $T{{.*}} bound to decl CoreGraphics.(file).CGFloat.init(_:)@{{.*}} : (CGFloat.Type) -> (Double) -> CGFloat {{.*}} -> implicit conversion [Double-to-CGFloat] -> apply function -> constructor member
+  test_optional_cgf(d)
+
+  // CHECK: [favored] $T{{.*}} bound to decl CoreGraphics.(file).Double extension.init(_:)@{{.*}} : (Double.Type) -> (CGFloat) -> Double {{.*}} -> implicit conversion [CGFloat-to-Double] -> apply function -> constructor member
+  test_optional_double(cgf)
+}