[ConstraintSystem] Fix the ordering of functions with optional parameters.

Treat non-optional generic parameters as being more specialized than
optional generic parameters, and penalize any solutions that involve
generic arguments that are themselves Optional.

By doing these things, we can remove the special-cased code for the
two overloads of '??' in the stdlib, instead treating the (T?, T)
overload as better than the (T?, T?) overload except where a user
actually passes an optionally-typed value as the second parameter.

Fixes: rdar://problem/19748710

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,27 @@ 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>())
+            return false;
+
           // Check whether the first parameter is a subtype of the second.
           cs.addConstraint(ConstraintKind::Subtype,
                            paramType1, paramType2, locator);
@@ -682,10 +707,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 +774,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 +979,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 +1094,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

@@ -1657,6 +1657,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,21 @@ 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(_ value: Int?) -> Int? {
+  return value ?? value
+}