Merge pull request swiftlang#36149 from nathawes/try-to-fix-missing-enum-completions-again

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

Author: Nathan Hawes

include/swift/Sema/ConstraintSystem.h

@@ -2800,7 +2800,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)) {
@@ -4827,9 +4827,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