Merge pull request #74180 from hamishknight/complete-conversion

hamishknight · web-flow · commit 1c58d8b7f300 · 2024-06-07T13:18:51.000+01:00
[Completion] Better handle merging of lookup base types
diff --git a/include/swift/IDE/PostfixCompletion.h b/include/swift/IDE/PostfixCompletion.h
@@ -66,13 +66,9 @@ class PostfixCompletionCallback : public TypeCheckCompletionCallback {
     llvm::DenseMap<AbstractClosureExpr *, ActorIsolation>
         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);
+    /// Merge this result with \p Other, returning \c true if
+    /// successful, else \c false.
+    bool tryMerge(const Result &Other, DeclContext *DC);
   };
 
   CodeCompletionExpr *CompletionExpr;
diff --git a/include/swift/IDE/UnresolvedMemberCompletion.h b/include/swift/IDE/UnresolvedMemberCompletion.h
@@ -33,13 +33,9 @@ class UnresolvedMemberTypeCheckCompletionCallback
     /// 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);
+    /// Attempts to merge this result with \p Other, returning \c true if
+    /// successful, else \c false.
+    bool tryMerge(const Result &Other, DeclContext *DC);
   };
 
   CodeCompletionExpr *CompletionExpr;
diff --git a/include/swift/Sema/IDETypeChecking.h b/include/swift/Sema/IDETypeChecking.h
@@ -60,11 +60,20 @@ namespace swift {
   /// Typecheck binding initializer at \p bindingIndex.
   void typeCheckPatternBinding(PatternBindingDecl *PBD, unsigned bindingIndex);
 
+  /// Attempt to merge two types for the purposes of completion lookup. In
+  /// general this means preferring a subtype over a supertype, but can also e.g
+  /// prefer an optional over a non-optional. If the two types are incompatible,
+  /// null is returned.
+  Type tryMergeBaseTypeForCompletionLookup(Type ty1, Type ty2, DeclContext *dc);
+
   /// Check if T1 is convertible to T2.
   ///
   /// \returns true on convertible, false on not.
   bool isConvertibleTo(Type T1, Type T2, bool openArchetypes, DeclContext &DC);
 
+  /// Check whether \p T1 is a subtype of \p T2.
+  bool isSubtypeOf(Type T1, Type T2, DeclContext *DC);
+
   void collectDefaultImplementationForProtocolMembers(ProtocolDecl *PD,
                         llvm::SmallDenseMap<ValueDecl*, ValueDecl*> &DefaultMap);
 
diff --git a/include/swift/Sema/IDETypeCheckingRequests.h b/include/swift/Sema/IDETypeCheckingRequests.h
@@ -86,6 +86,7 @@ class IsDeclApplicableRequest:
 //----------------------------------------------------------------------------//
 enum class TypeRelation: uint8_t {
   ConvertTo,
+  SubtypeOf
 };
 
 struct TypePair {
@@ -153,6 +154,7 @@ struct TypeRelationCheckInput {
     switch(owner.Relation) {
 #define CASE(NAME) case TypeRelation::NAME: out << #NAME << " "; break;
     CASE(ConvertTo)
+    CASE(SubtypeOf)
 #undef CASE
     }
   }
diff --git a/lib/IDE/IDETypeChecking.cpp b/lib/IDE/IDETypeChecking.cpp
@@ -941,13 +941,58 @@ bool swift::isMemberDeclApplied(const DeclContext *DC, Type BaseTy,
     IsDeclApplicableRequest(DeclApplicabilityOwner(DC, BaseTy, VD)), false);
 }
 
+Type swift::tryMergeBaseTypeForCompletionLookup(Type ty1, Type ty2,
+                                                DeclContext *dc) {
+  // Easy case, equivalent so just pick one.
+  if (ty1->isEqual(ty2))
+    return ty1;
+
+  // Check to see if one is an optional of another. In that case, prefer the
+  // optional since we can unwrap a single level when doing a lookup.
+  {
+    SmallVector<Type, 4> ty1Optionals;
+    SmallVector<Type, 4> ty2Optionals;
+    auto ty1Unwrapped = ty1->lookThroughAllOptionalTypes(ty1Optionals);
+    auto ty2Unwrapped = ty2->lookThroughAllOptionalTypes(ty2Optionals);
+
+    if (ty1Unwrapped->isEqual(ty2Unwrapped)) {
+      // We currently only unwrap a single level of optional, so if the
+      // difference is greater, don't merge.
+      if (ty1Optionals.size() == 1 && ty2Optionals.empty())
+        return ty1;
+      if (ty2Optionals.size() == 1 && ty1Optionals.empty())
+        return ty2;
+    }
+    // We don't want to consider subtyping for optional mismatches since
+    // optional promotion is modelled as a subtype, which isn't useful for us
+    // (i.e if we have T? and U, preferring U would miss members on T?).
+    if (ty1Optionals.size() != ty2Optionals.size())
+      return Type();
+  }
+
+  // In general we want to prefer a subtype over a supertype.
+  if (isSubtypeOf(ty1, ty2, dc))
+    return ty1;
+  if (isSubtypeOf(ty2, ty1, dc))
+    return ty2;
+
+  // Incomparable, return null.
+  return Type();
+}
+
 bool swift::isConvertibleTo(Type T1, Type T2, bool openArchetypes,
                             DeclContext &DC) {
   return evaluateOrDefault(DC.getASTContext().evaluator,
     TypeRelationCheckRequest(TypeRelationCheckInput(&DC, T1, T2,
       TypeRelation::ConvertTo, openArchetypes)), false);
 }
 
+bool swift::isSubtypeOf(Type T1, Type T2, DeclContext *DC) {
+  return evaluateOrDefault(DC->getASTContext().evaluator,
+    TypeRelationCheckRequest(TypeRelationCheckInput(DC, T1, T2,
+      TypeRelation::SubtypeOf, /*openArchetypes*/ false)), false);
+}
+
 Type swift::getRootTypeOfKeypathDynamicMember(SubscriptDecl *SD) {
   return evaluateOrDefault(SD->getASTContext().evaluator,
     RootTypeOfKeypathDynamicMemberRequest{SD}, Type());
diff --git a/lib/IDE/PostfixCompletion.cpp b/lib/IDE/PostfixCompletion.cpp
@@ -21,31 +21,20 @@ 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) {
+bool PostfixCompletionCallback::Result::tryMerge(const Result &Other,
+                                                 DeclContext *DC) {
+  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(IsBaseDeclUnapplied == Other.IsBaseDeclUnapplied);
   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;
-  }
+  auto baseTy = tryMergeBaseTypeForCompletionLookup(BaseTy, Other.BaseTy, DC);
+  if (!baseTy)
+    return false;
+
+  BaseTy = 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
@@ -66,18 +55,15 @@ void PostfixCompletionCallback::Result::merge(const Result &Other,
   ExpectsNonVoid &= Other.ExpectsNonVoid;
   IsImpliedResult |= Other.IsImpliedResult;
   IsInAsyncContext |= Other.IsInAsyncContext;
+  return true;
 }
 
 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);
+  for (auto idx : indices(Results)) {
+    if (Results[idx].tryMerge(Res, DC))
+      return;
   }
+  Results.push_back(Res);
 }
 
 void PostfixCompletionCallback::fallbackTypeCheck(DeclContext *DC) {
diff --git a/lib/IDE/UnresolvedMemberCompletion.cpp b/lib/IDE/UnresolvedMemberCompletion.cpp
@@ -21,43 +21,27 @@ 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)) {
+bool UnresolvedMemberTypeCheckCompletionCallback::Result::tryMerge(
+    const Result &Other, DeclContext *DC) {
+  auto expectedTy = tryMergeBaseTypeForCompletionLookup(ExpectedTy,
+                                                        Other.ExpectedTy, DC);
+  if (!expectedTy)
     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;
-  }
+  ExpectedTy = expectedTy;
 
   IsImpliedResult |= Other.IsImpliedResult;
   IsInAsyncContext |= Other.IsInAsyncContext;
+  return true;
 }
 
 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);
+  for (auto idx : indices(ExprResults)) {
+    if (ExprResults[idx].tryMerge(Res, DC))
+      return;
   }
+  ExprResults.push_back(Res);
 }
 
 void UnresolvedMemberTypeCheckCompletionCallback::sawSolutionImpl(
diff --git a/lib/Sema/IDETypeCheckingRequests.cpp b/lib/Sema/IDETypeCheckingRequests.cpp
@@ -246,10 +246,14 @@ IsDeclApplicableRequest::evaluate(Evaluator &evaluator,
 bool
 TypeRelationCheckRequest::evaluate(Evaluator &evaluator,
                                    TypeRelationCheckInput Owner) const {
-  std::optional<constraints::ConstraintKind> CKind;
+  using namespace constraints;
+  std::optional<ConstraintKind> CKind;
   switch (Owner.Relation) {
   case TypeRelation::ConvertTo:
-    CKind = constraints::ConstraintKind::Conversion;
+    CKind = ConstraintKind::Conversion;
+    break;
+  case TypeRelation::SubtypeOf:
+    CKind = ConstraintKind::Subtype;
     break;
   }
   assert(CKind.has_value());
diff --git a/test/IDE/complete_cgfloat_double.swift b/test/IDE/complete_cgfloat_double.swift
@@ -0,0 +1,14 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-ide-test(mock-sdk: %clang-importer-sdk) -batch-code-completion -source-filename %s -filecheck %raw-FileCheck -completion-output-dir %t
+
+// REQUIRES: objc_interop
+
+import Foundation
+
+func foo(_ x: CGFloat) {}
+func foo(_ x: Double) {}
+
+// Make sure we suggest completions for both CGFloat and Double.
+foo(.#^FOO^#)
+// FOO-DAG: Decl[Constructor]/CurrNominal/TypeRelation[Convertible]: init()[#CGFloat#]; name=init()
+// FOO-DAG: Decl[Constructor]/CurrNominal/IsSystem/TypeRelation[Convertible]: init()[#Double#]; name=init()
diff --git a/test/IDE/complete_rdar126168123.swift b/test/IDE/complete_rdar126168123.swift
@@ -0,0 +1,22 @@
+// RUN: %empty-directory(%t)
+// RUN: %swift-ide-test -batch-code-completion -source-filename %s -filecheck %raw-FileCheck -completion-output-dir %t
+
+// rdar://126168123
+
+protocol MyProto {}
+protocol MyProto2 {}
+
+struct MyStruct : MyProto {}
+
+extension MyProto where Self == MyStruct {
+  static var automatic: MyStruct { fatalError() }
+}
+
+func use<T: MyProto>(_ someT: T) {}
+func use<T: MyProto2>(_ someT: T) {}
+
+func test() {
+  use(.#^COMPLETE^#)
+}
+
+// COMPLETE: Decl[StaticVar]/CurrNominal/TypeRelation[Convertible]: automatic[#MyStruct#]; name=automatic
diff --git a/test/IDE/complete_subtype_overload.swift b/test/IDE/complete_subtype_overload.swift
@@ -0,0 +1,61 @@
+// RUN: %empty-directory(%t)
+// RUN: %swift-ide-test -batch-code-completion -source-filename %s -filecheck %raw-FileCheck -completion-output-dir %t
+
+class C {
+  static func cMethod() -> C {}
+}
+class D : C {
+  static func dMethod() -> D {}
+}
+
+func test1(_ x: C) {}
+func test1(_ x: D) {}
+
+// We prefer the subtype here, so we show completions for D.
+test1(.#^TEST1^#)
+// TEST1-DAG: Decl[StaticMethod]/Super:           cMethod()[#C#]; name=cMethod()
+// TEST1-DAG: Decl[StaticMethod]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Convertible]: dMethod()[#D#]; name=dMethod()
+// TEST1-DAG: Decl[Constructor]/CurrNominal/TypeRelation[Convertible]: init()[#D#]; name=init()
+
+func test2(_ x: C?) {}
+func test2(_ x: D?) {}
+
+test2(.#^TEST2^#)
+// TEST2-DAG: Decl[StaticMethod]/Super:           cMethod()[#C#]; name=cMethod()
+// TEST2-DAG: Decl[StaticMethod]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Convertible]: dMethod()[#D#]; name=dMethod()
+// TEST2-DAG: Decl[Constructor]/CurrNominal/TypeRelation[Convertible]: init()[#D#]; name=init()
+// TEST2-DAG: Decl[EnumElement]/CurrNominal/IsSystem/TypeRelation[Convertible]: none[#Optional<D>#]; name=none
+// TEST2-DAG: Decl[EnumElement]/CurrNominal/IsSystem/TypeRelation[Convertible]: some({#D#})[#Optional<D>#]; name=some()
+
+func test3(_ x: C?) {}
+func test3(_ x: D) {}
+
+// We can still provide both C and D completions here.
+test3(.#^TEST3^#)
+// TEST3-DAG: Decl[StaticMethod]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Convertible]: cMethod()[#C#]; name=cMethod()
+// TEST3-DAG: Decl[StaticMethod]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Convertible]: dMethod()[#D#]; name=dMethod()
+// TEST3-DAG: Decl[Constructor]/CurrNominal/TypeRelation[Convertible]: init()[#D#]; name=init()
+// TEST3-DAG: Decl[EnumElement]/CurrNominal/IsSystem/TypeRelation[Convertible]: none[#Optional<C>#]; name=none
+// TEST3-DAG: Decl[EnumElement]/CurrNominal/IsSystem/TypeRelation[Convertible]: some({#C#})[#Optional<C>#]; name=some()
+
+func test4(_ x: Int) {}
+func test4(_ x: AnyHashable) {}
+
+// Make sure we show Int completions.
+test4(.#^TEST4^#)
+// TEST4: Decl[StaticVar]/Super/Flair[ExprSpecific]/IsSystem/TypeRelation[Convertible]: zero[#Int#]; name=zero
+
+protocol P {}
+extension P {
+  func pMethod() {}
+}
+struct S : P {
+  func sMethod() {}
+}
+
+func test5() -> any P {}
+func test5() -> S {}
+
+test5().#^TEST5^#
+// TEST5-DAG: Decl[InstanceMethod]/CurrNominal:   pMethod()[#Void#]; name=pMethod()
+// TEST5-DAG: Decl[InstanceMethod]/CurrNominal:   sMethod()[#Void#]; name=sMethod()

Original file line number	Diff line number	Diff line change
`@@ -86,6 +86,7 @@ class IsDeclApplicableRequest:`
`86`	`86`	`//----------------------------------------------------------------------------//`
`87`	`87`	`enum class TypeRelation: uint8_t {`
`88`	`88`	`ConvertTo,`
	`89`	`+ SubtypeOf`
`89`	`90`	`};`
`90`	`91`
`91`	`92`	`struct TypePair {`
`@@ -153,6 +154,7 @@ struct TypeRelationCheckInput {`
`153`	`154`	`switch(owner.Relation) {`
`154`	`155`	`#define CASE(NAME) case TypeRelation::NAME: out << #NAME << " "; break;`
`155`	`156`	`CASE(ConvertTo)`
	`157`	`+ CASE(SubtypeOf)`
`156`	`158`	`#undef CASE`
`157`	`159`	`}`
`158`	`160`	`}`