[CodeCompletion] Only filter solutions based on number of fixes

rdar://73282163

Author: ahoppen

include/swift/Sema/ConstraintSystem.h

@@ -365,6 +365,10 @@ class TypeVariableType::Implementation {
 
   bool isTypeSequence() const;
 
+  /// Determine whether this type variable represents a code completion
+  /// expression.
+  bool isCodeCompletionToken() const;
+
   /// Retrieve the representative of the equivalence class to which this
   /// type variable belongs.
   ///
@@ -3032,10 +3036,25 @@ class ConstraintSystem {
     if (solutions.size() < 2)
       return;
 
-    if (auto best = findBestSolution(solutions, minimize)) {
-      if (*best != 0)
-        solutions[0] = std::move(solutions[*best]);
-      solutions.erase(solutions.begin() + 1, solutions.end());
+    if (isForCodeCompletion()) {
+      // For code completion remove solutions that have more fixes than the
+      // minimum but don't filter based on the lower-priority scores because we
+      // want to show all options even if they need to go through more
+      // conversions.
+      Score minScore = std::min_element(solutions.begin(), solutions.end(),
+                                        [](const Solution &a, const Solution &b) {
+        return a.getFixedScore() < b.getFixedScore();
+      })->getFixedScore();
+
+      llvm::erase_if(solutions, [&](const Solution &S) {
+        return S.getFixedScore().Data[SK_Fix] > minScore.Data[SK_Fix];
+      });
+    } else {
+      if (auto best = findBestSolution(solutions, minimize)) {
+        if (*best != 0)
+          solutions[0] = std::move(solutions[*best]);
+        solutions.erase(solutions.begin() + 1, solutions.end());
+      }
     }
   }
 

lib/Sema/CSBindings.cpp

@@ -1788,7 +1788,12 @@ bool TypeVarBindingProducer::computeNext() {
     // Allow solving for T even for a binding kind where that's invalid
     // if fixes are allowed, because that gives us the opportunity to
     // match T? values to the T binding by adding an unwrap fix.
-    if (binding.Kind == BindingKind::Subtypes || CS.shouldAttemptFixes()) {
+    // But don't allow it when solving for code completion because otherwise
+    // we might also receive T as a contextual type and we can't differentiate
+    // that completion results should only be considered to have a convertible,
+    // not identical type relation.
+    if ((binding.Kind == BindingKind::Subtypes || CS.shouldAttemptFixes()) &&
+        !TypeVar->getImpl().isCodeCompletionToken()) {
       // If we were unsuccessful solving for T?, try solving for T.
       if (auto objTy = type->getOptionalObjectType()) {
         // If T is a type variable, only attempt this if both the

lib/Sema/CSRanking.cpp

@@ -130,10 +130,21 @@ bool ConstraintSystem::worseThanBestSolution() const {
   if (getASTContext().TypeCheckerOpts.DisableConstraintSolverPerformanceHacks)
     return false;
 
-  if (!solverState || !solverState->BestScore ||
-      CurrentScore <= *solverState->BestScore)
+  if (!solverState || !solverState->BestScore)
     return false;
 
+  if (isForCodeCompletion()) {
+    // For code completion, we only filter based on SK_Fix, not the entire
+    // score. See ConstraintSystem::filterSolutions.
+    if (CurrentScore.Data[SK_Fix] <= solverState->BestScore->Data[SK_Fix]) {
+      return false;
+    }
+  } else {
+    if (CurrentScore <= *solverState->BestScore) {
+      return false;
+    }
+  }
+
   if (isDebugMode()) {
     llvm::errs().indent(solverState->depth * 2)
       << "(solution is worse than the best solution)\n";

lib/Sema/CSSolver.cpp

@@ -73,8 +73,14 @@ Solution ConstraintSystem::finalize() {
 
   // Update the best score we've seen so far.
   auto &ctx = getASTContext();
-  assert(ctx.TypeCheckerOpts.DisableConstraintSolverPerformanceHacks ||
-         !solverState->BestScore || CurrentScore <= *solverState->BestScore);
+  if (isForCodeCompletion()) {
+    assert(ctx.TypeCheckerOpts.DisableConstraintSolverPerformanceHacks ||
+           !solverState->BestScore ||
+           CurrentScore.Data[SK_Fix] <= solverState->BestScore->Data[SK_Fix]);
+  } else {
+    assert(ctx.TypeCheckerOpts.DisableConstraintSolverPerformanceHacks ||
+           !solverState->BestScore || CurrentScore <= *solverState->BestScore);
+  }
 
   if (!solverState->BestScore || CurrentScore <= *solverState->BestScore) {
     solverState->BestScore = CurrentScore;

lib/Sema/TypeCheckConstraints.cpp

@@ -130,6 +130,10 @@ bool TypeVariableType::Implementation::isTypeSequence() const {
       && locator->getGenericParameter()->isTypeSequence();
 }
 
+bool TypeVariableType::Implementation::isCodeCompletionToken() const {
+  return locator && locator->directlyAt<CodeCompletionExpr>();
+}
+
 void *operator new(size_t bytes, ConstraintSystem& cs,
                    size_t alignment) {
   return cs.getAllocator().Allocate(bytes, alignment);

test/IDE/complete_ambiguous.swift

@@ -448,8 +448,8 @@ struct Struct123: Equatable {
 }
 func testBestSolutionFilter() {
   let a = Struct123();
-  let b = [Struct123]().first(where: { $0 == a && 1 + 90 * 5 / 8 == 45 * -10 })?.structMem != .#^BEST_SOLUTION_FILTER?xfail=rdar73282163^#
-  let c = min(10.3, 10 / 10.4) < 6 / 7 ? true : Optional(a)?.structMem != .#^BEST_SOLUTION_FILTER2?check=BEST_SOLUTION_FILTER;xfail=rdar73282163^#
+  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?check=BEST_SOLUTION_FILTER^#
 }
 
 // BEST_SOLUTION_FILTER: Begin completions
@@ -465,7 +465,7 @@ func testBestSolutionGeneric() {
   func genAndInt(_ x: Int) -> Int { return 1 }
   func genAndInt<T>(_ x: T) -> Test1 { return Test1() }
 
-  genAndInt(2).#^BEST_SOLUTION_FILTER_GEN?xfail=rdar73282163^#
+  genAndInt(2).#^BEST_SOLUTION_FILTER_GEN^#
 }
 
 // BEST_SOLUTION_FILTER_GEN: Begin completions

test/IDE/complete_unresolved_members.swift

@@ -259,7 +259,18 @@ class C6 {
 
 class C6 {
   func f1(e: SomeEnum1) {
-    if let x = Optional(e) where x == .#^UNRESOLVED_25?check=UNRESOLVED_3^#
+    if let x = Optional(e) where x == .#^UNRESOLVED_25^#
+// UNRESOLVED_25: Begin completions, 9 items
+// UNRESOLVED_25-DAG: Decl[EnumElement]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Identical]: North[#SomeEnum1#];
+// UNRESOLVED_25-DAG: Decl[EnumElement]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Identical]: South[#SomeEnum1#];
+// UNRESOLVED_25-DAG: Decl[InstanceMethod]/CurrNominal/TypeRelation[Invalid]: hash({#(self): SomeEnum1#})[#(into: inout Hasher) -> Void#];
+// UNRESOLVED_25-DAG: Keyword[nil]/None/Erase[1]/TypeRelation[Identical]: nil[#SomeEnum1?#]; name=nil
+// UNRESOLVED_25-DAG: Decl[EnumElement]/CurrNominal/IsSystem/TypeRelation[Identical]: none[#Optional<SomeEnum1>#]; name=none
+// UNRESOLVED_25-DAG: Decl[EnumElement]/CurrNominal/IsSystem/TypeRelation[Identical]: some({#SomeEnum1#})[#Optional<SomeEnum1>#];
+// UNRESOLVED_25-DAG: Decl[InstanceMethod]/CurrNominal/IsSystem: map({#(self): Optional<SomeEnum1>#})[#((SomeEnum1) throws -> U) -> U?#];
+// UNRESOLVED_25-DAG: Decl[InstanceMethod]/CurrNominal/IsSystem: flatMap({#(self): Optional<SomeEnum1>#})[#((SomeEnum1) throws -> U?) -> U?#];
+// UNRESOLVED_25-DAG: Decl[InstanceMethod]/CurrNominal/IsSystem/TypeRelation[Invalid]: hash({#(self): Optional<SomeEnum1>#})[#(into: inout Hasher) -> Void#];
+// UNRESOLVED_25: End completions
   }
 }
 class C7 {}
@@ -721,3 +732,56 @@ func testSameType() {
 // SUGAR_TYPE: End completions
 }
 
+func testOverloadWithConvertibleArg() {
+  // This test case used to cause problems because the second overload needs a value to optional conversion, which increases the solution’s score in one of the later compontens.
+  // We were prematurely filtering the solution because its score is worse than the 0-score of the first overload (which only ignores the second argument, which doesn't increase the score)
+  struct S {
+    func xxx(_ foo: Foo) { }
+    func xxx(_ bar: Bar, _ other: Int?) { }
+  }
+
+  struct Foo {
+    init(x: String) {}
+  }
+
+  struct Bar {
+    static let top: Bar = Bar()
+  }
+
+  S().xxx(.#^CONVERTIBLE_ARG_OVERLOAD^#, 32)
+// CONVERTIBLE_ARG_OVERLOAD:     Begin completions, 2 items
+// CONVERTIBLE_ARG_OVERLOAD-DAG: Decl[StaticVar]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Identical]: top[#Bar#];
+// CONVERTIBLE_ARG_OVERLOAD-DAG: Decl[Constructor]/CurrNominal/TypeRelation[Identical]: init()[#Bar#];
+// CONVERTIBLE_ARG_OVERLOAD:     End completions
+}
+
+func testOverloadWithConvertibleArgWithFixesInScore() {
+  // same as testOverloadWithConvertibleArg but with solutions that contain a fix-score > 0 because the result builder has no body for the initialization of FStack.
+
+  @resultBuilder struct ResBuilder {
+    static func buildBlock<Content>(_ content: Content) -> Content { fatalError() }
+  }
+
+  struct S<Content> {
+    init(@ResBuilder content: () -> Content) {}
+    
+    func xxx(_ foo: Foo) -> Self { return self }
+    func xxx(_ bar: Bar = .top, _ other: Int?) -> Self { return self }
+  }
+
+  struct Foo {
+    init(x: String) {}
+  }
+
+  struct Bar {
+    static let top: Bar = Bar()
+  }
+
+  S() {}
+  .xxx(.#^CONVERTIBLE_ARG_OVERLOAD_WITH_FIX_IN_SCORE^#top, 32)
+// CONVERTIBLE_ARG_OVERLOAD_WITH_FIX_IN_SCORE:     Begin completions, 2 items
+// CONVERTIBLE_ARG_OVERLOAD_WITH_FIX_IN_SCORE-DAG: Decl[StaticVar]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Identical]: top[#Bar#]; name=top
+// CONVERTIBLE_ARG_OVERLOAD_WITH_FIX_IN_SCORE-DAG: Decl[Constructor]/CurrNominal/TypeRelation[Identical]: init()[#Bar#]; name=init()
+// CONVERTIBLE_ARG_OVERLOAD_WITH_FIX_IN_SCORE:     End completions
+
+}