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

lib/Sema/CSRanking.cpp

@@ -67,11 +67,7 @@ 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;
@@ -638,28 +634,6 @@ 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);
@@ -708,7 +682,10 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
 
       if (!knownNonSubtype) {
         // Solve the system.
-        if (cs.solveSingle(FreeTypeVariableBinding::Allow))
+        auto solution = cs.solveSingle(FreeTypeVariableBinding::Allow);
+
+        // Ban value-to-optional conversions.
+        if (solution && solution->getFixedScore().Data[SK_ValueToOptional] == 0)
           return true;
       }
 
@@ -775,6 +752,9 @@ 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);
@@ -980,6 +960,40 @@ 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.
@@ -1095,6 +1109,14 @@ 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,9 +683,6 @@ matchCallArguments(ConstraintSystem &cs, ConstraintKind kind,
       }
     }
 
-    if (!argType->isAny())
-      cs.increaseScore(ScoreKind::SK_EmptyExistentialConversion);
-
     return cs.getTypeMatchSuccess();
   }
 
@@ -1660,16 +1657,6 @@ 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,8 +456,6 @@ 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,19 +29,11 @@ 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 }
-func rdar29691909_callee(_ o: AnyObject) -> Any { return o }
+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(o: AnyObject) -> Any? {
-  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
+  return rdar29691909_callee(o) // expected-error{{ambiguous use of 'rdar29691909_callee'}}
 }
 
 func rdar29907555(_ value: Any!) -> String {

test/Constraints/overload.swift

@@ -236,53 +236,3 @@ 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'}}
-}

validation-test/Sema/OverridesAndOverloads.swift

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