[CSOptimizer] Reset the overall score of operator disjunctions that is based speculation

New ranking + selection algorithm suffered from over-eagerly selecting
operator disjunctions vs. unsupported non-operator ones even if the
ranking was based purely on literal candidates.

This change introduces a notion of a speculative candidate - one which
has a type inferred from a literal or an initializer call that has
failable overloads and/or implicit conversions (i.e. Double/CGFloat).

`determineBestChoicesInContext` would reset the score of an operator
disjunction which was computed based on speculative candidates alone
but would preserve favoring information. This way selection algorithm
would not be skewed towards operators and at the same time if there
is no no choice by to select one we'd still have favoring information
available which is important for operator chains that consist purely
of literals.

Author: xedin

lib/Sema/CSOptimizer.cpp

@@ -108,9 +108,12 @@ static bool isSupportedSpecialConstructor(ConstructorDecl *ctor) {
   return false;
 }
 
-static bool isStandardComparisonOperator(ValueDecl *decl) {
-  return decl->isOperator() &&
-         decl->getBaseIdentifier().isStandardComparisonOperator();
+static bool isStandardComparisonOperator(Constraint *disjunction) {
+  auto *choice = disjunction->getNestedConstraints()[0];
+  if (auto *decl = getOverloadChoiceDecl(choice))
+    return decl->isOperator() &&
+           decl->getBaseIdentifier().isStandardComparisonOperator();
+  return false;
 }
 
 static bool isOperatorNamed(Constraint *disjunction, StringRef name) {
@@ -694,6 +697,38 @@ static void determineBestChoicesInContext(
             fromInitializerCall(fromInitializerCall) {}
     };
 
+    // Determine whether there are any non-speculative choices
+    // in the given set of candidates. Speculative choices are
+    // literals or types inferred from initializer calls.
+    auto anyNonSpeculativeCandidates =
+        [&](ArrayRef<ArgumentCandidate> candidates) {
+          // If there is only one (non-CGFloat) candidate inferred from
+          // an initializer call we don't consider this a speculation.
+          //
+          // CGFloat inference is always speculative because of the
+          // implicit conversion between Double and CGFloat.
+          if (llvm::count_if(candidates, [&](const auto &candidate) {
+                return candidate.fromInitializerCall &&
+                       !candidate.type->isCGFloat();
+              }) == 1)
+            return true;
+
+          // If there are no non-literal and non-initializer-inferred types
+          // in the list, consider this is a speculation.
+          return llvm::any_of(candidates, [&](const auto &candidate) {
+            return !candidate.fromLiteral && !candidate.fromInitializerCall;
+          });
+        };
+
+    auto anyNonSpeculativeResultTypes = [](ArrayRef<Type> results) {
+      return llvm::any_of(results, [](Type resultTy) {
+        // Double and CGFloat are considered speculative because
+        // there exists an implicit conversion between them and
+        // preference is based on score impact in the overall solution.
+        return !(resultTy->isDouble() || resultTy->isCGFloat());
+      });
+    };
+
     SmallVector<SmallVector<ArgumentCandidate, 2>, 2>
         argumentCandidates;
     argumentCandidates.resize(argFuncType->getNumParams());
@@ -818,19 +853,19 @@ static void determineBestChoicesInContext(
       resultTypes.push_back(resultType);
     }
 
-    // Determine whether all of the argument candidates are inferred from literals.
-    // This information is going to be used later on when we need to decide how to
-    // score a matching choice.
-    bool onlyLiteralCandidates =
+    // Determine whether all of the argument candidates are speculative (i.e.
+    // literals). This information is going to be used later on when we need to
+    // decide how to score a matching choice.
+    bool onlySpeculativeArgumentCandidates =
         hasArgumentCandidates &&
         llvm::none_of(
             indices(argFuncType->getParams()), [&](const unsigned argIdx) {
-              auto &candidates = argumentCandidates[argIdx];
-              return llvm::any_of(candidates, [&](const auto &candidate) {
-                return !candidate.fromLiteral;
-              });
+              return anyNonSpeculativeCandidates(argumentCandidates[argIdx]);
             });
 
+    bool canUseContextualResultTypes =
+        isOperator && !isStandardComparisonOperator(disjunction);
+
     // Match arguments to the given overload choice.
     auto matchArguments = [&](OverloadChoice choice, FunctionType *overloadType)
 			-> std::optional<MatchCallArgumentResult> {
@@ -1229,16 +1264,12 @@ static void determineBestChoicesInContext(
           if (!matchings)
             return;
 
-          auto canUseContextualResultTypes = [&decl]() {
-            return decl->isOperator() && !isStandardComparisonOperator(decl);
-          };
-
           // Require exact matches only if all of the arguments
           // are literals and there are no usable contextual result
           // types that could help narrow favored choices.
           bool favorExactMatchesOnly =
-              onlyLiteralCandidates &&
-              (!canUseContextualResultTypes() || resultTypes.empty());
+              onlySpeculativeArgumentCandidates &&
+              (!canUseContextualResultTypes || resultTypes.empty());
 
           // This is important for SIMD operators in particular because
           // a lot of their overloads have same-type requires to a concrete
@@ -1384,7 +1415,7 @@ static void determineBestChoicesInContext(
           // because regular functions/methods/subscripts and
           // especially initializers could end up with a lot of
           // favored overloads because on the result type alone.
-          if (canUseContextualResultTypes() &&
+          if (canUseContextualResultTypes &&
               (score > 0 || !hasArgumentCandidates)) {
             if (llvm::any_of(resultTypes, [&](const Type candidateResultTy) {
                   return scoreCandidateMatch(genericSig, decl,
@@ -1439,6 +1470,34 @@ static void determineBestChoicesInContext(
         info.FavoredChoices.push_back(choice.first);
     }
 
+    // Reset operator disjunction score but keep favoring
+    // choices only available candidates where speculative
+    // with no contextual information available, standard
+    // comparison operators are a special cases because
+    // their return type is fixed to `Bool` unlike that of
+    // bitwise, arithmetic, and other operators.
+    //
+    // This helps to prevent over-eager selection of the
+    // operators over unsupported non-operator declarations.
+    if (isOperator && onlySpeculativeArgumentCandidates &&
+        (!canUseContextualResultTypes ||
+         !anyNonSpeculativeResultTypes(resultTypes))) {
+      if (cs.isDebugMode()) {
+        PrintOptions PO;
+        PO.PrintTypesForDebugging = true;
+
+        llvm::errs().indent(cs.solverState->getCurrentIndent())
+            << "<<< Disjunction "
+            << disjunction->getNestedConstraints()[0]
+                   ->getFirstType()
+                   ->getString(PO)
+            << " score " << bestScore
+            << " was reset due to having only speculative candidates\n";
+      }
+
+      info.Score = 0;
+    }
+
     recordResult(disjunction, std::move(info));
   }
 
@@ -1610,8 +1669,13 @@ ConstraintSystem::selectDisjunction() {
             return *firstScore > *secondScore;
         }
 
-        unsigned numFirstFavored = firstFavoredChoices.size();
-        unsigned numSecondFavored = secondFavoredChoices.size();
+        // Use favored choices only if disjunction score is higher
+        // than zero. This means that we can maintain favoring
+        // choices without impacting selection decisions.
+        unsigned numFirstFavored =
+            firstScore.value_or(0) ? firstFavoredChoices.size() : 0;
+        unsigned numSecondFavored =
+            secondScore.value_or(0) ? secondFavoredChoices.size() : 0;
 
         if (numFirstFavored == numSecondFavored) {
           if (firstActive != secondActive)

validation-test/Sema/type_checker_perf/fast/operators_inside_closure.swift

@@ -0,0 +1,9 @@
+// RUN: %target-swift-frontend -typecheck %s -solver-scope-threshold=10000
+// REQUIRES: tools-release,no_asan
+
+// Selecting operators from the closure before arguments to `zip` makes this "too complex"
+func compute(_ ids: [UInt64]) {
+  let _ = zip(ids[ids.indices.dropLast()], ids[ids.indices.dropFirst()]).map { pair in
+    ((pair.0 % 2 == 0) && (pair.1 % 2 == 1)) ? UInt64(pair.1 - pair.0) : 42
+  }
+}

validation-test/Sema/type_checker_perf/fast/rdar47492691.swift

@@ -7,3 +7,7 @@ import simd
 func test(foo: CGFloat, bar: CGFloat) {
   _ = CGRect(x: 0.0 + 1.0, y: 0.0 + foo, width: 3.0 - 1 - 1 - 1.0, height: bar)
 }
+
+func test_with_generic_func_and_literals(bounds: CGRect) {
+  _ = CGRect(x: 0, y: 0, width: 1, height: bounds.height - 2 + bounds.height / 2 + max(bounds.height / 2, bounds.height / 2))
+}

validation-test/Sema/type_checker_perf/fast/should_skip_bitwise_2.swift

@@ -1,3 +1,3 @@
-// RUN: %target-swift-frontend -typecheck %s -solver-scope-threshold=2000
+// RUN: %target-swift-frontend -typecheck %s -solver-scope-threshold=4000
 
 func f896() { let _ = ((0 >> ((0 >> 0) + ((0 / 0) & 0))) >> (0 << ((0 << 0) >> (0 << (0 << 0))))) }

validation-test/Sema/type_checker_perf/fast/swift_package_index_1.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift -solver-scope-threshold=1000
+// RUN: %target-swift-frontend -typecheck %s -solver-scope-threshold=700
 // REQUIRES: tools-release,no_asan
 
 public class Cookie {

validation-test/Sema/type_checker_perf/slow/array_count_property_vs_method.swift

@@ -0,0 +1,11 @@
+// RUN: %target-typecheck-verify-swift -solver-scope-threshold=5000
+// REQUIRES: tools-release,no_asan
+// REQUIRES: OS=macosx
+
+// FIXME: Array literals are considered "speculative" bindings at the moment but they cannot actually
+// assume different types unlike integer and floating-pointer literals.
+func f(n: Int, a: [Int]) {
+  // expected-error@+1 {{the compiler is unable to type-check this expression in reasonable time}}
+  let _ = [(0 ..< n + a.count).map { Int8($0) }] +
+          [(0 ..< n + a.count).map { Int8($0) }.reversed()] // Ok
+}

validation-test/Sema/type_checker_perf/fast/rdar23682605.swift renamed to validation-test/Sema/type_checker_perf/slow/rdar23682605.swift

@@ -14,6 +14,7 @@ func memoize<T: Hashable, U>( body: @escaping ((T)->U, T)->U ) -> (T)->U {
 }
 
 let fibonacci = memoize {
+  // expected-error@-1 {{reasonable time}}
   fibonacci, n in
   n < 2 ? Double(n) : fibonacci(n - 1) + fibonacci(n - 2)
 }

validation-test/Sema/type_checker_perf/fast/swift_package_index_3.swift renamed to validation-test/Sema/type_checker_perf/slow/swift_package_index_3.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift -solver-scope-threshold=50000
+// RUN: %target-typecheck-verify-swift %s -solver-scope-threshold=50000
 // REQUIRES: tools-release,no_asan
 
 extension String {
@@ -9,7 +9,7 @@ extension String {
 func getProperties(
     from ics: String
 ) -> [(name: String, value: String)] {
-    return ics
+    return ics // expected-error {{the compiler is unable to type-check this expression in reasonable time}}
         .replacingOccurrences(of: "\r\n ", with: "")
         .components(separatedBy: "\r\n")
         .map { $0.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: true) }