[IDE] Ignore score kinds that represent implicit conversions when solving for code completion

Ignore conversion score increases during code completion to make sure we don't filter solutions that might start receiving the best score based on a choice of the code completion token.

Author: ahoppen

include/swift/IDE/PostfixCompletion.h

@@ -25,11 +25,32 @@ namespace ide {
 /// type-checking.
 class PostfixCompletionCallback : public TypeCheckCompletionCallback {
   struct Result {
+    /// The type that we are completing on. Will never be null.
     Type BaseTy;
+
+    /// The decl that we are completing on. Is \c nullptr if we are completing
+    /// on an expression.
     ValueDecl *BaseDecl;
+
+    /// If the expression we are completing on statically refers to a metatype,
+    /// that is if it's something like 'MyType'. In such cases we want to offer
+    /// constructor call pattern completions and don't want to suggeste
+    /// operators that work on metatypes.
+    bool BaseIsStaticMetaType;
+
+    /// The types that the completion is expected to produce.
     SmallVector<Type, 4> ExpectedTypes;
+
+    /// Whether results that produce 'Void' should be disfavored. This happens
+    /// if the context is requiring a value. Once a completion produces 'Void',
+    /// we know that we can't retrieve a value from it anymore.
     bool ExpectsNonVoid;
-    bool BaseIsStaticMetaType;
+
+    /// If the code completion expression occurs as a single statement in a
+    /// single-expression closure. In such cases we don't want to disfavor
+    /// results that produce 'Void' because the user might intend to make the
+    /// closure a multi-statment closure, in which case this expression is no
+    /// longer implicitly returned.
     bool IsImplicitSingleExpressionReturn;
 
     /// Whether the surrounding context is async and thus calling async
@@ -40,13 +61,24 @@ class PostfixCompletionCallback : public TypeCheckCompletionCallback {
     /// haven't been saved to the AST.
     llvm::DenseMap<AbstractClosureExpr *, ClosureActorIsolation>
         ClosureActorIsolations;
+
+    /// Checks whether this result has the same \c BaseTy and \c BaseDecl as
+    /// \p Other and if the two can thus be merged to be one value lookup in
+    /// \c deliverResults.
+    bool canBeMergedWith(const Result &Other, DeclContext &DC) const;
+
+    /// Merge this result with \p Other. Assumes that they can be merged.
+    void merge(const Result &Other, DeclContext &DC);
   };
 
   CodeCompletionExpr *CompletionExpr;
   DeclContext *DC;
 
   SmallVector<Result, 4> Results;
-  llvm::DenseMap<std::pair<Type, Decl *>, size_t> BaseToSolutionIdx;
+
+  /// Add a result to \c Results, merging it with an existing result, if
+  /// possible.
+  void addResult(const Result &Res);
 
   void sawSolutionImpl(const constraints::Solution &solution) override;
 

include/swift/IDE/UnresolvedMemberCompletion.h

@@ -32,6 +32,14 @@ class UnresolvedMemberTypeCheckCompletionCallback
     /// Whether the surrounding context is async and thus calling async
     /// functions is supported.
     bool IsInAsyncContext;
+
+    /// Checks whether this result has the same \c BaseTy and \c BaseDecl as
+    /// \p Other and if the two can thus be merged to be one value lookup in
+    /// \c deliverResults.
+    bool canBeMergedWith(const Result &Other, DeclContext &DC) const;
+
+    /// Merge this result with \p Other. Assumes that they can be merged.
+    void merge(const Result &Other, DeclContext &DC);
   };
 
   CodeCompletionExpr *CompletionExpr;
@@ -40,6 +48,10 @@ class UnresolvedMemberTypeCheckCompletionCallback
   SmallVector<Result, 4> ExprResults;
   SmallVector<Result, 1> EnumPatternTypes;
 
+  /// Add a result to \c Results, merging it with an existing result, if
+  /// possible.
+  void addExprResult(const Result &Res);
+
   void sawSolutionImpl(const constraints::Solution &solution) override;
 
 public:

include/swift/Sema/ConstraintSystem.h

@@ -938,6 +938,9 @@ enum ScoreKind: unsigned int {
   /// A reference to an @unavailable declaration.
   SK_Unavailable,
   /// A reference to an async function in a synchronous context.
+  ///
+  /// \note Any score kind after this is considered a conversion that doesn't
+  /// require fixing the source and will be ignored during code completion.
   SK_AsyncInSyncMismatch,
   /// Synchronous function in an asynchronous context or a conversion of
   /// a synchronous function to an asynchronous one.
@@ -5228,7 +5231,8 @@ class ConstraintSystem {
 public:
   /// Increase the score of the given kind for the current (partial) solution
   /// along the.
-  void increaseScore(ScoreKind kind, unsigned value = 1);
+  void increaseScore(ScoreKind kind, ConstraintLocatorBuilder Locator,
+                     unsigned value = 1);
 
   /// Determine whether this solution is guaranteed to be worse than the best
   /// solution found so far.

lib/IDE/PostfixCompletion.cpp

@@ -21,6 +21,64 @@ using namespace swift;
 using namespace swift::constraints;
 using namespace swift::ide;
 
+bool PostfixCompletionCallback::Result::canBeMergedWith(const Result &Other,
+                                                        DeclContext &DC) const {
+  if (BaseDecl != Other.BaseDecl) {
+    return false;
+  }
+  if (!BaseTy->isEqual(Other.BaseTy) &&
+      !isConvertibleTo(BaseTy, Other.BaseTy, /*openArchetypes=*/true, DC) &&
+      !isConvertibleTo(Other.BaseTy, BaseTy, /*openArchetypes=*/true, DC)) {
+    return false;
+  }
+  return true;
+}
+
+void PostfixCompletionCallback::Result::merge(const Result &Other,
+                                              DeclContext &DC) {
+  assert(canBeMergedWith(Other, DC));
+  // These properties should match if we are talking about the same BaseDecl.
+  assert(BaseIsStaticMetaType == Other.BaseIsStaticMetaType);
+
+  if (!BaseTy->isEqual(Other.BaseTy) &&
+      isConvertibleTo(Other.BaseTy, BaseTy, /*openArchetypes=*/true, DC)) {
+    // Pick the more specific base type as it will produce more solutions.
+    BaseTy = Other.BaseTy;
+  }
+
+  // There could be multiple results that have different actor isolations if the
+  // closure is an argument to a function that has multiple overloads with
+  // different isolations for the closure. Producing multiple results for these
+  // is usually not very enlightning. For now, we just pick the first actor
+  // isolation that we find. This is good enough in practice.
+  // What we should really do is probably merge these two actor isolations and
+  // pick the weakest isolation for each closure.
+
+  for (auto &OtherExpectedTy : Other.ExpectedTypes) {
+    auto IsEqual = [&](Type Ty) { return Ty->isEqual(OtherExpectedTy); };
+    if (llvm::any_of(ExpectedTypes, IsEqual)) {
+      // We already know if this expected type
+      continue;
+    }
+    ExpectedTypes.push_back(OtherExpectedTy);
+  }
+  ExpectsNonVoid &= Other.ExpectsNonVoid;
+  IsImplicitSingleExpressionReturn |= Other.IsImplicitSingleExpressionReturn;
+  IsInAsyncContext |= Other.IsInAsyncContext;
+}
+
+void PostfixCompletionCallback::addResult(const Result &Res) {
+  auto ExistingRes =
+      llvm::find_if(Results, [&Res, DC = DC](const Result &ExistingResult) {
+        return ExistingResult.canBeMergedWith(Res, *DC);
+      });
+  if (ExistingRes != Results.end()) {
+    ExistingRes->merge(Res, *DC);
+  } else {
+    Results.push_back(Res);
+  }
+}
+
 void PostfixCompletionCallback::fallbackTypeCheck(DeclContext *DC) {
   assert(!gotCallback());
 
@@ -101,39 +159,43 @@ void PostfixCompletionCallback::sawSolutionImpl(
     }
   }
 
-  auto Key = std::make_pair(BaseTy, ReferencedDecl);
-  auto Ret = BaseToSolutionIdx.insert({Key, Results.size()});
-  if (Ret.second) {
-    bool ISDMT = S.isStaticallyDerivedMetatype(ParsedExpr);
-    bool ImplicitReturn = isImplicitSingleExpressionReturn(CS, CompletionExpr);
-    bool DisallowVoid = false;
-    DisallowVoid |= ExpectedTy && !ExpectedTy->isVoid();
-    DisallowVoid |= !ParentExpr &&
-                    CS.getContextualTypePurpose(CompletionExpr) != CTP_Unused;
-    for (auto SAT : S.targets) {
-      if (DisallowVoid) {
-        // DisallowVoid is already set. No need to iterate further.
-        break;
-      }
-      if (SAT.second.getAsExpr() == CompletionExpr) {
-        DisallowVoid |= SAT.second.getExprContextualTypePurpose() != CTP_Unused;
-      }
-    }
+  bool BaseIsStaticMetaType = S.isStaticallyDerivedMetatype(ParsedExpr);
+
+  SmallVector<Type, 4> ExpectedTypes;
+  if (ExpectedTy) {
+    ExpectedTypes.push_back(ExpectedTy);
+  }
 
-    Results.push_back({BaseTy, ReferencedDecl,
-                       /*ExpectedTypes=*/{}, DisallowVoid, ISDMT,
-                       ImplicitReturn, IsAsync, ClosureActorIsolations});
-    if (ExpectedTy) {
-      Results.back().ExpectedTypes.push_back(ExpectedTy);
+  bool ExpectsNonVoid = false;
+  ExpectsNonVoid |= ExpectedTy && !ExpectedTy->isVoid();
+  ExpectsNonVoid |=
+      !ParentExpr && CS.getContextualTypePurpose(CompletionExpr) != CTP_Unused;
+
+  for (auto SAT : S.targets) {
+    if (ExpectsNonVoid) {
+      // ExpectsNonVoid is already set. No need to iterate further.
+      break;
     }
-  } else if (ExpectedTy) {
-    auto &ExistingResult = Results[Ret.first->getSecond()];
-    ExistingResult.IsInAsyncContext |= IsAsync;
-    auto IsEqual = [&](Type Ty) { return ExpectedTy->isEqual(Ty); };
-    if (!llvm::any_of(ExistingResult.ExpectedTypes, IsEqual)) {
-      ExistingResult.ExpectedTypes.push_back(ExpectedTy);
+    if (SAT.second.getAsExpr() == CompletionExpr) {
+      ExpectsNonVoid |= SAT.second.getExprContextualTypePurpose() != CTP_Unused;
     }
   }
+
+  bool IsImplicitSingleExpressionReturn =
+      isImplicitSingleExpressionReturn(CS, CompletionExpr);
+
+  Result Res = {
+      BaseTy,
+      ReferencedDecl,
+      BaseIsStaticMetaType,
+      ExpectedTypes,
+      ExpectsNonVoid,
+      IsImplicitSingleExpressionReturn,
+      IsAsync,
+      ClosureActorIsolations
+  };
+
+  addResult(Res);
 }
 
 void PostfixCompletionCallback::deliverResults(

lib/IDE/UnresolvedMemberCompletion.cpp

@@ -21,6 +21,45 @@ using namespace swift;
 using namespace swift::constraints;
 using namespace swift::ide;
 
+bool UnresolvedMemberTypeCheckCompletionCallback::Result::canBeMergedWith(
+    const Result &Other, DeclContext &DC) const {
+  if (!isConvertibleTo(ExpectedTy, Other.ExpectedTy, /*openArchetypes=*/true,
+                       DC) &&
+      !isConvertibleTo(Other.ExpectedTy, ExpectedTy, /*openArchetypes=*/true,
+                       DC)) {
+    return false;
+  }
+  return true;
+}
+
+void UnresolvedMemberTypeCheckCompletionCallback::Result::merge(
+    const Result &Other, DeclContext &DC) {
+  assert(canBeMergedWith(Other, DC));
+  if (!ExpectedTy->isEqual(Other.ExpectedTy) &&
+      isConvertibleTo(ExpectedTy, Other.ExpectedTy, /*openArchetypes=*/true,
+                      DC)) {
+    // ExpectedTy is more general than Other.ExpectedTy. Complete based on the
+    // more general type because it offers more completion options.
+    ExpectedTy = Other.ExpectedTy;
+  }
+
+  IsImplicitSingleExpressionReturn |= Other.IsImplicitSingleExpressionReturn;
+  IsInAsyncContext |= Other.IsInAsyncContext;
+}
+
+void UnresolvedMemberTypeCheckCompletionCallback::addExprResult(
+    const Result &Res) {
+  auto ExistingRes =
+      llvm::find_if(ExprResults, [&Res, DC = DC](const Result &ExistingResult) {
+        return ExistingResult.canBeMergedWith(Res, *DC);
+      });
+  if (ExistingRes != ExprResults.end()) {
+    ExistingRes->merge(Res, *DC);
+  } else {
+    ExprResults.push_back(Res);
+  }
+}
+
 void UnresolvedMemberTypeCheckCompletionCallback::sawSolutionImpl(
     const constraints::Solution &S) {
   auto &CS = S.getConstraintSystem();
@@ -32,15 +71,9 @@ void UnresolvedMemberTypeCheckCompletionCallback::sawSolutionImpl(
   // to derive it from), let's not attempt to do a lookup since it wouldn't
   // produce any useful results anyway.
   if (ExpectedTy) {
-    // If ExpectedTy is a duplicate of any other result, ignore this solution.
-    auto IsEqual = [&](const Result &R) {
-      return R.ExpectedTy->isEqual(ExpectedTy);
-    };
-    if (!llvm::any_of(ExprResults, IsEqual)) {
-      bool SingleExprBody =
-          isImplicitSingleExpressionReturn(CS, CompletionExpr);
-      ExprResults.push_back({ExpectedTy, SingleExprBody, IsAsync});
-    }
+    bool SingleExprBody = isImplicitSingleExpressionReturn(CS, CompletionExpr);
+    Result Res = {ExpectedTy, SingleExprBody, IsAsync};
+    addExprResult(Res);
   }
 
   if (auto PatternType = getPatternMatchType(S, CompletionExpr)) {

lib/Sema/CSBindings.cpp

@@ -2315,7 +2315,7 @@ bool TypeVariableBinding::attempt(ConstraintSystem &cs) const {
     }
     // Reflect in the score that this type variable couldn't be
     // resolved and had to be bound to a placeholder "hole" type.
-    cs.increaseScore(SK_Hole);
+    cs.increaseScore(SK_Hole, srcLocator);
 
     if (auto fix = fixForHole(cs)) {
       if (cs.recordFix(/*fix=*/fix->first, /*impact=*/fix->second))