Merge pull request #17855 from rudkx/rdar19748710

[ConstraintSystem] Fix the ordering of functions with optional parame…

Author: rudkx

lib/Sema/CSRanking.cpp

@@ -67,7 +67,11 @@ void ConstraintSystem::increaseScore(ScoreKind kind, unsigned value) {
     case SK_CollectionUpcastConversion:
       log << "collection upcast conversion";
       break;
-        
+
+    case SK_BindOptionalToArchetype:
+      log << "bind optional to archetype";
+      break;
+
     case SK_ValueToOptional:
       log << "value to optional";
       break;
@@ -634,6 +638,28 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
           Type paramType1 = getAdjustedParamType(param1);
           Type paramType2 = getAdjustedParamType(param2);
 
+          // If we have:
+          //   param1Type = $T0?
+          //   param2Type = $T1
+          // the subtype constraint check will always return true
+          // since we'll attempt to bind $T1 as $T0?.
+          //
+          // What we're comparing here is foo<T>(_: T?) vs. foo<T>(_: T) and
+          // we don't want to consider the optional-taking function to be the
+          // the more specialized one since throughout the type system we
+          // consider T to be a subtype of T?.
+          SmallVector<Type, 2> optionals1;
+          paramType1->lookThroughAllOptionalTypes(optionals1);
+          auto numOptionals1 = optionals1.size();
+
+          SmallVector<Type, 2> optionals2;
+          Type objType2 = paramType2->lookThroughAllOptionalTypes(optionals2);
+          auto numOptionals2 = optionals2.size();
+
+          if (numOptionals1 > numOptionals2 &&
+              (objType2->is<TypeVariableType>() || objType2->isAny()))
+            return false;
+
           // Check whether the first parameter is a subtype of the second.
           cs.addConstraint(ConstraintKind::Subtype,
                            paramType1, paramType2, locator);
@@ -682,10 +708,7 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
 
       if (!knownNonSubtype) {
         // Solve the system.
-        auto solution = cs.solveSingle(FreeTypeVariableBinding::Allow);
-
-        // Ban value-to-optional conversions.
-        if (solution && solution->getFixedScore().Data[SK_ValueToOptional] == 0)
+        if (cs.solveSingle(FreeTypeVariableBinding::Allow))
           return true;
       }
 
@@ -752,9 +775,6 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
   auto foundRefinement1 = false;
   auto foundRefinement2 = false;
 
-  bool isStdlibOptionalMPlusOperator1 = false;
-  bool isStdlibOptionalMPlusOperator2 = false;
-
   auto getWeight = [&](ConstraintLocator *locator) -> unsigned {
     if (auto *anchor = locator->getAnchor()) {
       auto weight = weights.find(anchor);
@@ -960,40 +980,6 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
         foundRefinement2 = decl2InSubprotocol;
       }
     }
-
-    // FIXME: Lousy hack for ?? to prefer the catamorphism (flattening)
-    // over the mplus (non-flattening) overload if all else is equal.
-    if (decl1->getBaseName() == "??") {
-      assert(decl2->getBaseName() == "??");
-
-      auto check = [](const ValueDecl *VD) -> bool {
-        if (!VD->getModuleContext()->isStdlibModule())
-          return false;
-        auto fnTy = VD->getInterfaceType()->castTo<AnyFunctionType>();
-        if (!fnTy->getResult()->getOptionalObjectType())
-          return false;
-
-        // Check that the standard library hasn't added another overload of
-        // the ?? operator.
-        auto params = fnTy->getParams();
-        assert(params.size() == 2);
-
-        auto param1 = params[0].getType();
-        auto param2 = params[1].getType()->castTo<AnyFunctionType>();
-
-        assert(param1->getOptionalObjectType());
-        assert(param2->isAutoClosure());
-        assert(param2->getResult()->getOptionalObjectType());
-
-        (void) param1;
-        (void) param2;
-
-        return true;
-      };
-
-      isStdlibOptionalMPlusOperator1 = check(decl1);
-      isStdlibOptionalMPlusOperator2 = check(decl2);
-    }
   }
 
   // Compare the type variable bindings.
@@ -1109,14 +1095,6 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
     }
   }
 
-  // FIXME: All other things being equal, prefer the catamorphism (flattening)
-  // overload of ?? over the mplus (non-flattening) overload.
-  if (score1 == score2) {
-    // This is correct: we want to /disprefer/ the mplus.
-    score2 += isStdlibOptionalMPlusOperator1;
-    score1 += isStdlibOptionalMPlusOperator2;
-  }
-
   // FIXME: There are type variables and overloads not common to both solutions
   // that haven't been considered. They make the systems different, but don't
   // affect ranking. We need to handle this.

lib/Sema/CSSimplify.cpp

@@ -683,6 +683,9 @@ matchCallArguments(ConstraintSystem &cs, ConstraintKind kind,
       }
     }
 
+    if (!argType->isAny())
+      cs.increaseScore(ScoreKind::SK_EmptyExistentialConversion);
+
     return cs.getTypeMatchSuccess();
   }
 
@@ -1657,6 +1660,16 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
       auto *typeVar = typeVar1 ? typeVar1 : typeVar2;
       auto type = typeVar1 ? type2 : type1;
 
+      if (type->getOptionalObjectType()) {
+        if (auto *typeVarLocator = typeVar->getImpl().getLocator()) {
+          auto path = typeVarLocator->getPath();
+          if (!path.empty() &&
+              path.back().getKind() == ConstraintLocator::Archetype) {
+            increaseScore(SK_BindOptionalToArchetype);
+          }
+        }
+      }
+
       return matchTypesBindTypeVar(typeVar, type, kind, flags, locator,
                                    formUnsolvedResult);
     }

lib/Sema/ConstraintSystem.h

@@ -456,6 +456,8 @@ enum ScoreKind {
   SK_CollectionUpcastConversion,
   /// A value-to-optional conversion.
   SK_ValueToOptional,
+  /// Instantiating a function with archetype T as an Optional value.
+  SK_BindOptionalToArchetype,
   /// A conversion to an empty existential type ('Any' or '{}').
   SK_EmptyExistentialConversion,
   /// A key path application subscript.

test/Constraints/diag_ambiguities.swift

@@ -29,11 +29,19 @@ C(g) // expected-error{{ambiguous use of 'g'}}
 func h<T>(_ x: T) -> () {}
 C(h) // expected-error{{ambiguous use of 'init'}}
 
-func rdar29691909_callee(_ o: AnyObject?) -> Any? { return o } // expected-note {{found this candidate}}
-func rdar29691909_callee(_ o: AnyObject) -> Any { return o } // expected-note {{found this candidate}}
+func rdar29691909_callee(_ o: AnyObject?) -> Any? { return o }
+func rdar29691909_callee(_ o: AnyObject) -> Any { return o }
 
 func rdar29691909(o: AnyObject) -> Any? {
-  return rdar29691909_callee(o) // expected-error{{ambiguous use of 'rdar29691909_callee'}}
+  return rdar29691909_callee(o)
+}
+
+func rdar29691909_callee_alt(_ o: AnyObject?) -> Int? { fatalError() }
+func rdar29691909_callee_alt(_ o: AnyObject) -> Int { fatalError() }
+
+func rdar29691909_1(o: AnyObject) -> Int {
+  let r = rdar29691909_callee_alt(o)
+  return r
 }
 
 func rdar29907555(_ value: Any!) -> String {

test/Constraints/overload.swift

@@ -236,3 +236,53 @@ func autoclosure1<T>(_: [T], _: X) { }
 func test_autoclosure1(ia: [Int]) {
   autoclosure1(ia, X()) // okay: resolves to the second function
 }
+
+// Ensure that we select the (T?, T) -> T overload of '??' in coalesce() below.
+func apply<T>(_ fn: () -> T) -> T { return fn() }
+func opt(x: String) -> String { return x }
+func opt() -> String? { return "hi" }
+
+func coalesce() {
+  let _ = apply{
+    _ = opt()
+    return opt()
+  } ?? ""
+}
+
+// Ensure that we do not select the (T?, T) -> T version of ??, which
+// would result in a warning about RHS never being selected.
+func rdar19748710_0(_ value: Int?) -> Int? {
+  return value ?? value
+}
+
+// Again, ensure we do not select (T?, T) -> T, despite forcing the result.
+func rdar19748710_1(_ value: Int?) -> Int? {
+  return (value ?? value)!
+}
+
+// FIXME: We choose to inject the LHS, which really doesn't seem like
+// the reasonable choice here. It seems more predictable and safe to
+// either inject the result of ??, or treat this as an error since the
+// current behavior will result in .some(nil) being force-unwrapped
+// rather than the RHS value. The diagnostic is problematic, too,
+// since it claims 'Int?' is a non-optional type.
+func rdar19748710_2(_ value: Int?) -> Int? {
+  return (value ?? value)!! // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'Int?', so the right side is never used}}
+}
+
+// Ensure that we select the more closely matching function between
+// the two _g overloads.
+func giveAny() -> Any { fatalError() }
+func giveAnyOptional() -> Any? { fatalError() }
+func takeAny(_ a: Any?) -> Any? { fatalError() }
+func takeAny(_ a: Any) -> Any { fatalError() }
+
+func testAnyOptional() {
+  let r = takeAny(giveAnyOptional())
+  let _ = r!
+}
+
+func testAny() {
+  let r = takeAny(giveAny())
+  let _ = r! // expected-error {{cannot force unwrap value of non-optional type 'Any'}}
+}

test/Constraints/overload_silgen.swift

@@ -0,0 +1,10 @@
+// RUN: %target-swift-emit-silgen %s | %FileCheck %s
+
+// CHECK: sil hidden @$S15overload_silgen8anyToIntySiypF
+// CHECK: end sil function '$S15overload_silgen8anyToIntySiypF'
+func anyToInt(_: Any) -> Int { fatalError() }
+
+// CHECK: sil hidden @$S15overload_silgen8anyToIntySiSgypSgF
+// CHECK: function_ref @$S15overload_silgen8anyToIntySiypF
+// CHECK: end sil function '$S15overload_silgen8anyToIntySiSgypSgF'
+func anyToInt(_ value: Any?) -> Int? { return anyToInt(value!) }

validation-test/Sema/OverridesAndOverloads.swift

@@ -328,7 +328,8 @@ Overloads.test("generic methods are worse than non-generic") {
   }
 
   Base().foo(C1());     expectEqual("foo(C1)", which)
-  Base().foo(Token1()); expectEqual("foo(Any)", which)
+  Base().foo(Token1()); expectEqual("foo(T)", which)
+  Base().foo(Token1() as Any); expectEqual("foo(Any)", which)
 
   Base().bar(C1());     expectEqual("bar(C1)", which)
   Base().bar(Token1()); expectEqual("bar(T)", which)