[CodeCompletion][CSRanking] Still allow some filtering of soltions when solving for code completion to improve performance

We were previously completely skipping the "best" solution filtering the solver
does to make sure we didn't miss any non-best but still viable solutions, as
the completions generated from them can make them become the best solution. E.g:

struct Foo { let onFoo = 10 }

func foo(_ x: Int) -> Int { return 1 }
func foo<T>(_ x: T) -> Foo { return Foo() }

foo(3).<here> // the "best" solution is the one with the more-specialized foo(x: Int) overload

In the example above we shouldn't remove the solution for foo(x: T) even though
there is a single "best" solution (`foo(x: Int)`) as picking a completion
result generated from it (`onFoo`) would make the foo(x: T) overload the best
and only viable solution.

Completely skipping this filtering as we were previously doing is overkill
though and adversely affects performance. E.g. it makes sense to filter out
and stop exploring solutions with overload choices for foo that required fixes
for missing arguments if there is another solution with an overload choice that
didn't require any fixes.

This patch restores best solution filtering during code completion and instead updates
the compareSolutions function it compare solutions based purely on their fixed score.

Resolves rdar://problem/73282163

Author: Nathan Hawes

include/swift/Sema/ConstraintSystem.h

@@ -2787,7 +2787,7 @@ class ConstraintSystem {
   void
   filterSolutions(SmallVectorImpl<Solution> &solutions,
                   bool minimize = false) {
-    if (solutions.size() < 2 || isForCodeCompletion())
+    if (solutions.size() < 2)
       return;
 
     if (auto best = findBestSolution(solutions, minimize)) {
@@ -4803,9 +4803,11 @@ class ConstraintSystem {
   /// \param diff The differences among the solutions.
   /// \param idx1 The index of the first solution.
   /// \param idx2 The index of the second solution.
+  /// \param isForCodeCompletion Whether solving for code completion.
   static SolutionCompareResult
   compareSolutions(ConstraintSystem &cs, ArrayRef<Solution> solutions,
-                   const SolutionDiff &diff, unsigned idx1, unsigned idx2);
+                   const SolutionDiff &diff, unsigned idx1, unsigned idx2,
+                   bool isForCodeCompletion);
 
 public:
   /// Increase the score of the given kind for the current (partial) solution

lib/Sema/CSRanking.cpp

@@ -757,14 +757,48 @@ static void addKeyPathDynamicMemberOverloads(
   }
 }
 
+SolutionCompareResult compareSolutionsForCodeCompletion(
+    ConstraintSystem &cs, ArrayRef<Solution> solutions, unsigned idx1,
+    unsigned idx2) {
+
+  // When solving for code completion we can't consider one solution worse than
+  // another according to the same rules as regular compilation. For example,
+  // with the code below:
+  //
+  //  func foo(_ x: Int) -> Int {}
+  //  func foo<T>(_ x: T) -> String {}
+  //  foo(3).<complete here> // Still want solutions with for both foo
+  //                         // overloads - String and Int members are both
+  //                         // valid here.
+  //
+  // the comparison for regular compilation considers the solution with the more
+  // specialized `foo` overload `foo(_: Int)` to be better than the solution
+  // with the generic overload `foo(_: T)` even though both are otherwise
+  // viable. For code completion purposes offering members of 'String' based
+  // on the solution with the generic overload is equally as import as offering
+  // members of 'Int' as choosing one of those completions will then result in
+  // regular compilation resolving the call to the generic overload instead.
+
+  if (solutions[idx1].getFixedScore() == solutions[idx2].getFixedScore())
+    return SolutionCompareResult::Incomparable;
+  return solutions[idx1].getFixedScore() < solutions[idx2].getFixedScore()
+             ? SolutionCompareResult::Better
+             : SolutionCompareResult::Worse;
+}
+
+
 SolutionCompareResult ConstraintSystem::compareSolutions(
     ConstraintSystem &cs, ArrayRef<Solution> solutions,
-    const SolutionDiff &diff, unsigned idx1, unsigned idx2) {
+    const SolutionDiff &diff, unsigned idx1, unsigned idx2,
+    bool isForCodeCompletion) {
   if (cs.isDebugMode()) {
     llvm::errs().indent(cs.solverState->depth * 2)
       << "comparing solutions " << idx1 << " and " << idx2 <<"\n";
   }
 
+  if (isForCodeCompletion)
+    return compareSolutionsForCodeCompletion(cs, solutions, idx1, idx2);
+
   // Whether the solutions are identical.
   bool identical = true;
 
@@ -800,7 +834,7 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
 
   SmallVector<SolutionDiff::OverloadDiff, 4> overloadDiff(diff.overloads);
   // Single type of keypath dynamic member lookup could refer to different
-  // member overlaods, we have to do a pair-wise comparison in such cases
+  // member overloads, we have to do a pair-wise comparison in such cases
   // otherwise ranking would miss some viable information e.g.
   // `_ = arr[0..<3]` could refer to subscript through writable or read-only
   // key path and each of them could also pick overload which returns `Slice<T>`
@@ -1300,7 +1334,8 @@ ConstraintSystem::findBestSolution(SmallVectorImpl<Solution> &viable,
   SmallVector<bool, 16> losers(viable.size(), false);
   unsigned bestIdx = 0;
   for (unsigned i = 1, n = viable.size(); i != n; ++i) {
-    switch (compareSolutions(*this, viable, diff, i, bestIdx)) {
+    switch (compareSolutions(*this, viable, diff, i, bestIdx,
+                             isForCodeCompletion())) {
     case SolutionCompareResult::Identical:
       // FIXME: Might want to warn about this in debug builds, so we can
       // find a way to eliminate the redundancy in the search space.
@@ -1324,7 +1359,8 @@ ConstraintSystem::findBestSolution(SmallVectorImpl<Solution> &viable,
     if (i == bestIdx)
       continue;
 
-    switch (compareSolutions(*this, viable, diff, bestIdx, i)) {
+    switch (compareSolutions(*this, viable, diff, bestIdx, i,
+                             isForCodeCompletion())) {
     case SolutionCompareResult::Identical:
       // FIXME: Might want to warn about this in debug builds, so we can
       // find a way to eliminate the redundancy in the search space.
@@ -1376,7 +1412,8 @@ ConstraintSystem::findBestSolution(SmallVectorImpl<Solution> &viable,
       if (losers[j])
         continue;
 
-      switch (compareSolutions(*this, viable, diff, i, j)) {
+      switch (compareSolutions(*this, viable, diff, i, j,
+                               isForCodeCompletion())) {
       case SolutionCompareResult::Identical:
         // FIXME: Dub one of these the loser arbitrarily?
         break;

test/IDE/complete_ambiguous.swift

@@ -48,6 +48,7 @@
 // RUN: %swift-ide-test -code-completion  -source-filename %s -code-completion-token=REGULAR_MULTICLOSURE_APPLIED | %FileCheck %s --check-prefix=POINT_MEMBER
 // RUN: %swift-ide-test -code-completion  -source-filename %s -code-completion-token=BEST_SOLUTION_FILTER | %FileCheck %s --check-prefix=BEST_SOLUTION_FILTER
 // RUN: %swift-ide-test -code-completion  -source-filename %s -code-completion-token=BEST_SOLUTION_FILTER2 | %FileCheck %s --check-prefix=BEST_SOLUTION_FILTER
+// RUN: %swift-ide-test -code-completion  -source-filename %s -code-completion-token=BEST_SOLUTION_FILTER_GEN | %FileCheck %s --check-prefix=BEST_SOLUTION_FILTER_GEN
 // RUN: %swift-ide-test -code-completion  -source-filename %s -code-completion-token=MISSINGARG_INLINE | %FileCheck %s --check-prefix=MISSINGARG_INLINE
 // RUN: %swift-ide-test -code-completion  -source-filename %s -code-completion-token=MISSINGARG_TRAILING | %FileCheck %s --check-prefix=MISSINGARG_TRAILING
 
@@ -495,7 +496,7 @@ enum Enum123 {
 struct Struct123: Equatable {
     var structMem = Enum123.enumElem
 }
-func testNoBestSolutionFilter() {
+func testBestSolutionFilter() {
   let a = Struct123();
   let b = [Struct123]().first(where: { $0 == a && 1 + 90 * 5 / 8 == 45 * -10 })?.structMem != .#^BEST_SOLUTION_FILTER^#
   let c = min(10.3, 10 / 10.4) < 6 / 7 ? true : Optional(a)?.structMem != .#^BEST_SOLUTION_FILTER2^#
@@ -508,3 +509,16 @@ func testNoBestSolutionFilter() {
 // BEST_SOLUTION_FILTER-DAG: Decl[EnumElement]/CurrNominal/IsSystem/TypeRelation[Identical]: none[#Optional<Enum123>#]{{; name=.+$}}
 // BEST_SOLUTION_FILTER-DAG: Decl[EnumElement]/CurrNominal/IsSystem/TypeRelation[Identical]: some({#Enum123#})[#Optional<Enum123>#]{{; name=.+$}}
 // BEST_SOLUTION_FILTER: End completions
+
+func testBestSolutionGeneric() {
+  struct Test1 {}
+  func genAndInt(_ x: Int) -> Int { return 1 }
+  func genAndInt<T>(_ x: T) -> Test1 { return Test1() }
+
+  genAndInt(2).#^BEST_SOLUTION_FILTER_GEN^#
+}
+
+// BEST_SOLUTION_FILTER_GEN: Begin completions
+// BEST_SOLUTION_FILTER_GEN-DAG: Keyword[self]/CurrNominal:     self[#Int#]; name=self
+// BEST_SOLUTION_FILTER_GEN-DAG: Keyword[self]/CurrNominal:     self[#Test1#]; name=self
+// BEST_SOLUTION_FILTER_GEN: End completions