[DNM] Experiment: Try skipping unavailable/disfavored choices for completion

include/swift/Sema/ConstraintSystem.h

@@ -1067,6 +1067,15 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &out, const Score &score);
 struct Score {
   unsigned Data[NumScoreKinds] = {};
 
+  /// The maximum score that can be represented.
+  static Score max() {
+    Score x;
+    for (unsigned i = 0; i != NumScoreKinds; ++i) {
+      x.Data[i] = ~0U;
+    }
+    return x;
+  }
+
   friend Score &operator+=(Score &x, const Score &y) {
     for (unsigned i = 0; i != NumScoreKinds; ++i) {
       x.Data[i] += y.Data[i];

lib/Sema/CSSolver.cpp

@@ -2361,8 +2361,10 @@ void DisjunctionChoiceProducer::partitionDisjunction(
     return false;
   });
 
-  // Then unavailable constraints if we're skipping them.
-  if (!CS.shouldAttemptFixes()) {
+  // Then unavailable constraints if we're skipping them. Also do this for
+  // completion since we enable fixes there but still want to try unavailable
+  // overloads last.
+  if (!CS.shouldAttemptFixes() || CS.isForCodeCompletion()) {
     forEachChoice(Choices, [&](unsigned index, Constraint *constraint) -> bool {
       if (constraint->getKind() != ConstraintKind::BindOverload)
         return false;

lib/Sema/CSStep.cpp

@@ -615,6 +615,18 @@ bool TypeChecker::isDeclRefinementOf(ValueDecl *declA, ValueDecl *declB) {
                            false);
 }
 
+bool DisjunctionStep::isLastChoiceBetterThan(ScoreKind kind) const {
+  if (!LastSolvedChoice)
+    return false;
+
+  auto &lastScore = LastSolvedChoice->second;
+  for (unsigned i = 0, n = unsigned(kind) + 1; i != n; ++i) {
+    if (lastScore.Data[i] > 0)
+      return false;
+  }
+  return true;
+}
+
 bool DisjunctionStep::shouldSkip(const DisjunctionChoice &choice) const {
   auto &ctx = CS.getASTContext();
 
@@ -635,27 +647,21 @@ bool DisjunctionStep::shouldSkip(const DisjunctionChoice &choice) const {
   if (choice.isDisabled())
     return skip("disabled");
 
-  if (!CS.shouldAttemptFixes()) {
+  if (!CS.shouldAttemptFixes() || CS.isForCodeCompletion()) {
     // Skip unavailable overloads.
-    if (choice.isUnavailable())
-      return skip("unavailable");
+    if (choice.isUnavailable()) {
+      // For code completion, we only want to do this if we've already seen
+      // a better choice.
+      if (!CS.isForCodeCompletion() || isLastChoiceBetterThan(SK_Unavailable))
+        return skip("unavailable");
+    }
 
     // Since the disfavored overloads are always located at the end of
     // the partition they could be skipped if there was at least one
     // valid solution for this partition already, because the solution
     // they produce would always be worse.
-    if (choice.isDisfavored() && LastSolvedChoice) {
-      bool canSkipDisfavored = true;
-      auto &lastScore = LastSolvedChoice->second;
-      for (unsigned i = 0, n = unsigned(SK_DisfavoredOverload) + 1; i != n;
-           ++i) {
-        if (lastScore.Data[i] > 0) {
-          canSkipDisfavored = false;
-          break;
-        }
-      }
-
-      if (canSkipDisfavored)
+    if (choice.isDisfavored()) {
+      if (isLastChoiceBetterThan(SK_DisfavoredOverload))
         return skip("disfavored");
     }
   }
@@ -799,9 +805,24 @@ bool DisjunctionStep::attempt(const DisjunctionChoice &choice) {
 bool ConjunctionStep::attempt(const ConjunctionElement &element) {
   ++CS.solverState->NumConjunctionTerms;
 
-  // Outside or previous element score doesn't affect
-  // subsequent elements.
-  CS.solverState->BestScore.reset();
+  // Outside or previous element score doesn't affect subsequent elements
+  // subsequent elements. However SK_Fix and SK_Hole *are* propagated outside
+  // of the conjunction, so we can use this to inform the best score within
+  // the conjunction.
+  if (BestScore) {
+    auto bestScore = Score::max();
+    bestScore.Data[SK_Fix] = BestScore->Data[SK_Fix];
+    bestScore.Data[SK_Hole] = BestScore->Data[SK_Hole];
+    CS.solverState->BestScore = bestScore;
+  } else {
+    CS.solverState->BestScore.reset();
+  }
+
+  // Make sure that element is solved in isolation
+  // by dropping all non-fatal scoring information.
+  CS.clearScore();
+  CS.increaseScore(SK_Fix, PreviousScore.Data[SK_Fix]);
+  CS.increaseScore(SK_Hole, PreviousScore.Data[SK_Hole]);
 
   // Apply solution inferred for all the previous elements
   // because this element could reference declarations
@@ -811,14 +832,12 @@ bool ConjunctionStep::attempt(const ConjunctionElement &element) {
     // Note that solution is removed here. This is done
     // because we want build a single complete solution
     // incrementally.
-    CS.replaySolution(Solutions.pop_back_val(),
-                      /*shouldIncrementScore=*/false);
+    auto S = Solutions.pop_back_val();
+    CS.replaySolution(S, /*shouldIncrementScore=*/false);
+    CS.increaseScore(SK_Fix, S.getFixedScore().Data[SK_Fix] - PreviousScore.Data[SK_Fix]);
+    CS.increaseScore(SK_Hole, S.getFixedScore().Data[SK_Hole] - PreviousScore.Data[SK_Hole]);
   }
 
-  // Make sure that element is solved in isolation
-  // by dropping all scoring information.
-  CS.clearScore();
-
   // Reset the scope and trail counters to avoid "too complex"
   // failures when closure has a lot of elements in the body.
   CS.NumSolverScopes = 0;
@@ -902,8 +921,10 @@ StepResult ConjunctionStep::resume(bool prevFailed) {
 
       if (Solutions.size() == 1) {
         auto score = Solutions.front().getFixedScore();
-        if (score.Data[SK_Fix] > 0 && !CS.isForCodeCompletion())
+        if (score.Data[SK_Fix] > PreviousScore.Data[SK_Fix] &&
+            !CS.isForCodeCompletion()) {
           Producer.markExhausted();
+        }
       }
     } else if (Solutions.size() != 1) {
       return failConjunction();
@@ -942,6 +963,12 @@ StepResult ConjunctionStep::resume(bool prevFailed) {
           Snapshot &&
           "Isolated conjunction requires a snapshot of the constraint system");
 
+      // Subtract fatal score kinds.
+      for (auto &solution : Solutions) {
+        solution.getFixedScore().Data[SK_Fix] -= PreviousScore.Data[SK_Fix];
+        solution.getFixedScore().Data[SK_Hole] -= PreviousScore.Data[SK_Hole];
+      }
+
       // In diagnostic mode it's valid for an element to have
       // multiple solutions. Ambiguity just needs to be merged
       // into the outer context to be property diagnosed.

lib/Sema/CSStep.h

@@ -662,6 +662,11 @@ class DisjunctionStep final : public BindingStep<DisjunctionChoiceProducer> {
   bool shortCircuitDisjunctionAt(Constraint *currentChoice,
                                  Constraint *lastSuccessfulChoice) const;
 
+  /// Whether the last choice has a better score than a given score kind, i.e
+  /// any non-zero components of its score are strictly less impactful than the
+  /// given kind.
+  bool isLastChoiceBetterThan(ScoreKind kind) const;
+
   bool shouldSkipGenericOperators() const {
     if (!BestNonGenericScore)
       return false;
@@ -858,6 +863,8 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {
     void replaySolution(const Solution &solution);
   };
 
+  Score PreviousScore;
+
   /// Best solution solver reached so far.
   std::optional<Score> BestScore;
 
@@ -901,7 +908,7 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {
                   SmallVectorImpl<Solution> &solutions)
       : BindingStep(cs, {cs, conjunction},
                     conjunction->isIsolated() ? IsolatedSolutions : solutions),
-        BestScore(getBestScore()),
+        PreviousScore(cs.CurrentScore), BestScore(getBestScore()),
         OuterNumSolverScopes(cs.NumSolverScopes, 0),
         OuterNumTrailSteps(cs.NumTrailSteps, 0),
         Conjunction(conjunction),

test/IDE/complete_closure_ambiguity.swift

@@ -24,8 +24,7 @@ foo {
   return x
 }
 
-// CONSTRAINTS: attempting disjunction choice {{.*}}:12:6
-// CONSTRAINTS: increasing 'disfavored overload' score
-// CONSTRAINTS: solution is worse than the best solution
+// CONSTRAINTS: attempting disjunction choice {{.*}}:9:6
+// CONSTRAINTS: skipping disfavored disjunction choice {{.*}}:12:6
 
 // CONSTRAINTS-NOT-NOT: increasing 'hole'

test/IDE/complete_in_closures.swift

@@ -543,8 +543,8 @@ func testCompleteAfterClosureWithIfExprThatContainErrors() {
   }#^AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS^#
 
   // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS: Begin completions, 2 items
-  // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS-DAG: Keyword[self]/CurrNominal:          .self[#() -> _#]; name=self
-  // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS-DAG: Pattern/CurrModule/Flair[ArgLabels]: ()[#_#]; name=()
+  // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS-DAG: Keyword[self]/CurrNominal:          .self[#() -> Void#]; name=self
+  // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS-DAG: Pattern/CurrModule/Flair[ArgLabels]: ()[#Void#]; name=()
   // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS: End completions
 }
 

test/IDE/complete_unavailable_skip.swift

@@ -0,0 +1,33 @@
+// RUN: %empty-directory(%t)
+// RUN: %batch-code-completion -debug-constraints 2> %t/constraints.log
+// RUN: %FileCheck %s -check-prefix CONSTRAINTS < %t/constraints.log
+
+struct S {
+  func bar(_ x: Int) {}
+  func bar(_ x: String) {}
+}
+
+@available(*, unavailable)
+func foo(_ fn: (S) -> Void) {}
+func foo(_ x: () -> Void) {}
+
+// Make sure we can try the unavailable overload if necessary.
+foo {
+  $0.#^COMPLETE1^#
+  // COMPLETE1: Decl[InstanceMethod]/CurrNominal: bar({#(x): Int#})[#Void#]; name=bar(:)
+  // COMPLETE1: Decl[InstanceMethod]/CurrNominal: bar({#(x): String#})[#Void#]; name=bar(:)
+}
+
+func baz(_ fn: (S) -> Void) {}
+
+@available(*, unavailable)
+func baz(_ x: () -> Void) {}
+
+// Here we can prune the unavailable overload of baz.
+baz {
+  $0.#^COMPLETE2^#
+  // COMPLETE2: Decl[InstanceMethod]/CurrNominal: bar({#(x): Int#})[#Void#]; name=bar(:)
+  // COMPLETE2: Decl[InstanceMethod]/CurrNominal: bar({#(x): String#})[#Void#]; name=bar(:)
+}
+// CONSTRAINTS: attempting disjunction choice {{.*}}:21:6
+// CONSTRAINTS: skipping unavailable disjunction choice {{.*}}:24:6

validation-test/compiler_crashers_2/2d5e95ea862946ed.swift → validation-test/compiler_crashers_2_fixed/2d5e95ea862946ed.swift

@@ -1,5 +1,5 @@
 // {"kind":"typecheck","signature":"(anonymous namespace)::ExprWalker::walkToExprPost(swift::Expr*)","signatureAssert":"Assertion failed: (Ptr && \"Cannot dereference a null Type!\"), function operator->"}
-// RUN: not --crash %target-swift-frontend -typecheck %s
+// RUN: not %target-swift-frontend -typecheck %s
 func a {
   {
     \ b() a