Merge pull request #63613 from ahoppen/ahoppen/ambiguous-cursor-info-results

[IDE] Report ambiguous cursor info results

Author: ahoppen

include/swift/IDE/CursorInfo.h

@@ -26,7 +26,7 @@ namespace ide {
 class CursorInfoConsumer {
 public:
   virtual ~CursorInfoConsumer() {}
-  virtual void handleResults(ResolvedCursorInfoPtr) = 0;
+  virtual void handleResults(std::vector<ResolvedCursorInfoPtr>) = 0;
 };
 
 /// Create a factory for code completion callbacks.

include/swift/IDETool/IDEInspectionInstance.h

@@ -81,8 +81,8 @@ struct ConformingMethodListResults {
 
 /// The results returned from \c IDEInspectionInstance::cursorInfo.
 struct CursorInfoResults {
-  /// The actual results. If \c nullptr, no results were found.
-  ResolvedCursorInfoPtr Result;
+  /// The actual results.
+  std::vector<ResolvedCursorInfoPtr> ResolvedCursorInfos;
   /// Whether an AST was reused to produce the results.
   bool DidReuseAST;
 };

lib/IDE/CursorInfo.cpp

@@ -121,7 +121,7 @@ class NodeFinderExprResult : public NodeFinderResult {
 /// Walks the AST, looking for a node at \c LocToResolve. While walking the
 /// AST, also gathers information about shorthand shadows.
 class NodeFinder : ASTWalker {
-  SourceFile &SrcFile;
+  DeclContext &DC;
   SourceLoc LocToResolve;
 
   /// As we are walking the tree, this variable is updated to the last seen
@@ -139,11 +139,10 @@ class NodeFinder : ASTWalker {
   llvm::DenseMap<ValueDecl *, ValueDecl *> ShorthandShadowedDecls;
 
 public:
-  NodeFinder(SourceFile &SrcFile, SourceLoc LocToResolve)
-      : SrcFile(SrcFile), LocToResolve(LocToResolve),
-        DeclContextStack({&SrcFile}) {}
+  NodeFinder(DeclContext &DC, SourceLoc LocToResolve)
+      : DC(DC), LocToResolve(LocToResolve), DeclContextStack({&DC}) {}
 
-  void resolve() { SrcFile.walk(*this); }
+  void resolve() { DC.walkContext(*this); }
 
   std::unique_ptr<NodeFinderResult> takeResult() { return std::move(Result); }
 
@@ -161,9 +160,7 @@ class NodeFinder : ASTWalker {
   }
 
 private:
-  SourceManager &getSourceMgr() const {
-    return SrcFile.getASTContext().SourceMgr;
-  }
+  SourceManager &getSourceMgr() const { return DC.getASTContext().SourceMgr; }
 
   /// The decl context that is currently being walked.
   DeclContext *getCurrentDeclContext() { return DeclContextStack.back(); }
@@ -219,7 +216,9 @@ class NodeFinder : ASTWalker {
     if (auto CaptureList = dyn_cast<CaptureListExpr>(E)) {
       for (auto ShorthandShadows :
            getShorthandShadows(CaptureList, getCurrentDeclContext())) {
-        assert(ShorthandShadowedDecls.count(ShorthandShadows.first) == 0);
+        assert(ShorthandShadowedDecls.count(ShorthandShadows.first) == 0 ||
+               ShorthandShadowedDecls[ShorthandShadows.first] ==
+                   ShorthandShadows.second);
         ShorthandShadowedDecls[ShorthandShadows.first] =
             ShorthandShadows.second;
       }
@@ -232,7 +231,8 @@ class NodeFinder : ASTWalker {
     switch (E->getKind()) {
     case ExprKind::DeclRef:
     case ExprKind::UnresolvedDot:
-    case ExprKind::UnresolvedDeclRef: {
+    case ExprKind::UnresolvedDeclRef:
+    case ExprKind::OverloadedDeclRef: {
       assert(Result == nullptr);
       Result =
           std::make_unique<NodeFinderExprResult>(E, getCurrentDeclContext());
@@ -280,13 +280,33 @@ class CursorInfoTypeCheckSolutionCallback : public TypeCheckCompletionCallback {
   };
 
 private:
-  /// The expression for which we want to provide cursor info results.
-  Expr *ResolveExpr;
+  /// The location to resolve and the \c DeclContext to resolve it in.
+  /// Note that we cannot store the expression to resolve directly because an
+  /// \c UnresolvedDeclRefExpr might be replaced by an \c OverloadedDeclRefExpr
+  /// and thus the constraint system solution doesn't know about the
+  /// \c UnresolvedDeclRefExpr. Instead, we find the expression to resolve in
+  /// the source file again after expression pre-check has run.
+  DeclContext &DC;
+  SourceLoc ResolveLoc;
 
   SmallVector<CursorInfoDeclReference, 1> Results;
 
+  Expr *getExprToResolve() {
+    NodeFinder Finder(DC, ResolveLoc);
+    Finder.resolve();
+    auto Result = Finder.takeResult();
+    if (!Result || Result->getKind() != NodeFinderResultKind::Expr) {
+      return nullptr;
+    }
+    return cast<NodeFinderExprResult>(Result.get())->getExpr();
+  }
+
   void sawSolutionImpl(const Solution &S) override {
     auto &CS = S.getConstraintSystem();
+    auto ResolveExpr = getExprToResolve();
+    if (!ResolveExpr) {
+      return;
+    }
 
     auto Locator = CS.getConstraintLocator(ResolveExpr);
     auto CalleeLocator = S.getCalleeLocator(Locator);
@@ -310,8 +330,8 @@ class CursorInfoTypeCheckSolutionCallback : public TypeCheckCompletionCallback {
   }
 
 public:
-  CursorInfoTypeCheckSolutionCallback(Expr *ResolveExpr)
-      : ResolveExpr(ResolveExpr) {}
+  CursorInfoTypeCheckSolutionCallback(DeclContext &DC, SourceLoc ResolveLoc)
+      : DC(DC), ResolveLoc(ResolveLoc) {}
 
   ArrayRef<CursorInfoDeclReference> getResults() const { return Results; }
 };
@@ -327,11 +347,11 @@ class CursorInfoDoneParsingCallback : public DoneParsingCallback {
                                 SourceLoc RequestedLoc)
       : DoneParsingCallback(), Consumer(Consumer), RequestedLoc(RequestedLoc) {}
 
-  ResolvedCursorInfoPtr getDeclResult(NodeFinderDeclResult *DeclResult,
-                                      SourceFile *SrcFile,
-                                      NodeFinder &Finder) const {
+  std::vector<ResolvedCursorInfoPtr>
+  getDeclResult(NodeFinderDeclResult *DeclResult, SourceFile *SrcFile,
+                NodeFinder &Finder) const {
     typeCheckDeclAndParentClosures(DeclResult->getDecl());
-    return new ResolvedValueRefCursorInfo(
+    auto CursorInfo = new ResolvedValueRefCursorInfo(
         SrcFile, RequestedLoc, DeclResult->getDecl(),
         /*CtorTyRef=*/nullptr,
         /*ExtTyRef=*/nullptr, /*IsRef=*/false, /*Ty=*/Type(),
@@ -341,24 +361,27 @@ class CursorInfoDoneParsingCallback : public DoneParsingCallback {
         /*IsDynamic=*/false,
         /*ReceiverTypes=*/{},
         Finder.getShorthandShadowedDecls(DeclResult->getDecl()));
+    return {CursorInfo};
   }
 
-  ResolvedCursorInfoPtr getExprResult(NodeFinderExprResult *ExprResult,
-                                      SourceFile *SrcFile,
-                                      NodeFinder &Finder) const {
+  std::vector<ResolvedCursorInfoPtr>
+  getExprResult(NodeFinderExprResult *ExprResult, SourceFile *SrcFile,
+                NodeFinder &Finder) const {
     Expr *E = ExprResult->getExpr();
     DeclContext *DC = ExprResult->getDeclContext();
 
     // Type check the statemnt containing E and listen for solutions.
-    CursorInfoTypeCheckSolutionCallback Callback(E);
-    llvm::SaveAndRestore<TypeCheckCompletionCallback *> CompletionCollector(
-        DC->getASTContext().SolutionCallback, &Callback);
-    typeCheckASTNodeAtLoc(TypeCheckASTNodeAtLocContext::declContext(DC),
-                          E->getLoc());
+    CursorInfoTypeCheckSolutionCallback Callback(*DC, RequestedLoc);
+    {
+      llvm::SaveAndRestore<TypeCheckCompletionCallback *> CompletionCollector(
+          DC->getASTContext().SolutionCallback, &Callback);
+      typeCheckASTNodeAtLoc(TypeCheckASTNodeAtLocContext::declContext(DC),
+                            E->getLoc());
+    }
 
     if (Callback.getResults().empty()) {
       // No results.
-      return nullptr;
+      return {};
     }
 
     for (auto Info : Callback.getResults()) {
@@ -367,34 +390,38 @@ class CursorInfoDoneParsingCallback : public DoneParsingCallback {
       typeCheckDeclAndParentClosures(Info.ReferencedDecl);
     }
 
-    if (Callback.getResults().size() != 1) {
-      // FIXME: We need to be able to report multiple results.
-      return nullptr;
-    }
-
     // Deliver results
 
-    auto Res = Callback.getResults()[0];
-    SmallVector<NominalTypeDecl *> ReceiverTypes;
-    if (Res.IsDynamicRef && Res.BaseType) {
-      if (auto ReceiverType = Res.BaseType->getAnyNominal()) {
-        ReceiverTypes = {ReceiverType};
-      } else if (auto MT = Res.BaseType->getAs<AnyMetatypeType>()) {
-        // Look through metatypes to get the nominal type decl.
-        if (auto ReceiverType = MT->getInstanceType()->getAnyNominal()) {
+    std::vector<ResolvedCursorInfoPtr> Results;
+    for (auto Res : Callback.getResults()) {
+      SmallVector<NominalTypeDecl *> ReceiverTypes;
+      if (isa<ModuleDecl>(Res.ReferencedDecl)) {
+        // ResolvedModuleRefCursorInfo is not supported by solver-based cursor
+        // info yet.
+        continue;
+      }
+      if (Res.IsDynamicRef && Res.BaseType) {
+        if (auto ReceiverType = Res.BaseType->getAnyNominal()) {
           ReceiverTypes = {ReceiverType};
+        } else if (auto MT = Res.BaseType->getAs<AnyMetatypeType>()) {
+          // Look through metatypes to get the nominal type decl.
+          if (auto ReceiverType = MT->getInstanceType()->getAnyNominal()) {
+            ReceiverTypes = {ReceiverType};
+          }
         }
       }
-    }
 
-    return new ResolvedValueRefCursorInfo(
-        SrcFile, RequestedLoc, Res.ReferencedDecl,
-        /*CtorTyRef=*/nullptr,
-        /*ExtTyRef=*/nullptr, /*IsRef=*/true, /*Ty=*/Type(),
-        /*ContainerType=*/Res.BaseType,
-        /*CustomAttrRef=*/None,
-        /*IsKeywordArgument=*/false, Res.IsDynamicRef, ReceiverTypes,
-        Finder.getShorthandShadowedDecls(Res.ReferencedDecl));
+      auto CursorInfo = new ResolvedValueRefCursorInfo(
+          SrcFile, RequestedLoc, Res.ReferencedDecl,
+          /*CtorTyRef=*/nullptr,
+          /*ExtTyRef=*/nullptr, /*IsRef=*/true, /*Ty=*/Type(),
+          /*ContainerType=*/Res.BaseType,
+          /*CustomAttrRef=*/None,
+          /*IsKeywordArgument=*/false, Res.IsDynamicRef, ReceiverTypes,
+          Finder.getShorthandShadowedDecls(Res.ReferencedDecl));
+      Results.push_back(CursorInfo);
+    }
+    return Results;
   }
 
   void doneParsing(SourceFile *SrcFile) override {
@@ -407,7 +434,7 @@ class CursorInfoDoneParsingCallback : public DoneParsingCallback {
     if (!Result) {
       return;
     }
-    ResolvedCursorInfoPtr CursorInfo;
+    std::vector<ResolvedCursorInfoPtr> CursorInfo;
     switch (Result->getKind()) {
     case NodeFinderResultKind::Decl:
       CursorInfo = getDeclResult(cast<NodeFinderDeclResult>(Result.get()),

lib/IDETool/IDEInspectionInstance.cpp

@@ -827,7 +827,7 @@ void swift::ide::IDEInspectionInstance::cursorInfo(
         : ReusingASTContext(ReusingASTContext),
           CancellationFlag(CancellationFlag), Callback(Callback) {}
 
-    void handleResults(ResolvedCursorInfoPtr result) override {
+    void handleResults(std::vector<ResolvedCursorInfoPtr> result) override {
       HandleResultsCalled = true;
       if (CancellationFlag &&
           CancellationFlag->load(std::memory_order_relaxed)) {
@@ -852,8 +852,8 @@ void swift::ide::IDEInspectionInstance::cursorInfo(
                   ide::makeCursorInfoCallbacksFactory(Consumer, RequestedLoc));
 
               if (!Result.DidFindIDEInspectionTarget) {
-                return DeliverTransformed(ResultType::success(
-                    {/*Results=*/nullptr, Result.DidReuseAST}));
+                return DeliverTransformed(
+                    ResultType::success({/*Results=*/{}, Result.DidReuseAST}));
               }
 
               performIDEInspectionSecondPass(
@@ -863,8 +863,8 @@ void swift::ide::IDEInspectionInstance::cursorInfo(
                 // pass, we didn't receive any results. To make sure Callback
                 // gets called exactly once, call it manually with no results
                 // here.
-                DeliverTransformed(ResultType::success(
-                    {/*Results=*/nullptr, Result.DidReuseAST}));
+                DeliverTransformed(
+                    ResultType::success({/*Results=*/{}, Result.DidReuseAST}));
               }
             },
             Callback);

lib/Sema/CSSolver.cpp

@@ -1430,7 +1430,10 @@ ConstraintSystem::solve(SyntacticElementTarget &target,
     case SolutionResult::Ambiguous:
       // If salvaging produced an ambiguous result, it has already been
       // diagnosed.
-      if (stage == 1) {
+      // If we have found an ambiguous solution in the first stage, salvaging
+      // won't produce more solutions, so we can inform the solution callback
+      // about the current ambiguous solutions straight away.
+      if (stage == 1 || Context.SolutionCallback) {
         reportSolutionsToSolutionCallback(solution);
         solution.markAsDiagnosed();
         return None;
@@ -1449,8 +1452,10 @@ ConstraintSystem::solve(SyntacticElementTarget &target,
       LLVM_FALLTHROUGH;
 
     case SolutionResult::UndiagnosedError:
-      if (shouldSuppressDiagnostics()) {
-        reportSolutionsToSolutionCallback(solution);
+      /// If we have a SolutionCallback, we are inspecting constraint system
+      /// solutions directly and thus also want to receive ambiguous solutions.
+      /// Hence always run the second (salvaging) stage.
+      if (shouldSuppressDiagnostics() && !Context.SolutionCallback) {
         solution.markAsDiagnosed();
         return None;
       }

test/SourceKit/CursorInfo/cursor_ambiguous.swift

@@ -0,0 +1,17 @@
+func testAmbiguousFunctionReference() {
+  func foo(a: Int) {}
+  func foo(a: String) {}
+
+  // RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):7 %s -- %s | %FileCheck %s
+  _ = foo
+
+  // RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):7 %s -- %s | %FileCheck %s
+  _ = foo(a: UInt(1))
+}
+
+// CHECK: source.lang.swift.ref.function.free (2:8-2:19)
+// CHECK: <Declaration>func foo(a: <Type usr="s:Si">Int</Type>)</Declaration>
+// CHECK: SECONDARY SYMBOLS BEGIN
+// CHECK: source.lang.swift.ref.function.free (3:8-3:22)
+// CHECK: <Declaration>func foo(a: <Type usr="s:SS">String</Type>)</Declaration>
+// CHECK: SECONDARY SYMBOLS END

test/SourceKit/CursorInfo/cursor_declref_resolve_triggers_more_typechecking.swift

@@ -0,0 +1,19 @@
+struct GorEach<Data, Content> {
+  public init(_ data: Data, content: (Data) -> Void) {}
+}
+
+struct MavigationLink<Label, Destination> {
+  init(destination: PeopleDetail) {}
+}
+
+struct PeopleDetail {
+  init(peopleId: Int) {}
+}
+
+func test(peoples: [Int]) {
+  GorEach(peoples) { people in
+    // Should not crash
+    // RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):56 %s -- %s
+    MavigationLink(destination: PeopleDetail(peopleId: people))
+  }
+}

test/SourceKit/CursorInfo/cursor_on_invalid_capturing_self.swift

@@ -0,0 +1,8 @@
+class Items {
+  func test() {
+    // Should not crash
+    // RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):9 %s -- %s
+    _ = Invalid.sink { [weak self] items in
+    }
+  }
+}

test/SourceKit/CursorInfo/cursor_on_module.swift

@@ -0,0 +1,7 @@
+func test() {
+  // RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):3 %s -- %s | %FileCheck %s
+  Swift.min
+}
+
+// CHECK: source.lang.swift.ref.module ()
+// CHECK-NEXT: Swift

test/SourceKit/Refactoring/basic.swift

@@ -170,7 +170,7 @@ func hasCallToAsyncAlternative(c: ConvertAsync) {
 // RUN: %sourcekitd-test -req=cursor -pos=117:16  -cursor-action %s -- %s | %FileCheck %s -check-prefix=CHECK-GLOBAL
 // RUN: %sourcekitd-test -req=cursor -pos=117:17  -cursor-action %s -- %s | %FileCheck %s -check-prefix=CHECK-GLOBAL
 
-// RUN: %sourcekitd-test -req=cursor -pos=35:10 -end-pos=35:16 -cursor-action %s -- %s | %FileCheck %s -check-prefix=CHECK-RENAME-EXTRACT
+// RUN: %sourcekitd-test -req=cursor -pos=35:10 -end-pos=35:16 -cursor-action %s -- %s | %FileCheck %s -check-prefix=CHECK-RENAME
 
 // RUN: %sourcekitd-test -req=cursor -pos=54:10 -end-pos=54:22 -cursor-action %s -- %s | %FileCheck %s -check-prefix=CHECK-LOCAL
 // RUN: %sourcekitd-test -req=cursor -pos=54:12 -end-pos=54:22 -cursor-action %s -- %s | %FileCheck %s -check-prefix=CHECK-SELF-RENAME1
@@ -231,6 +231,8 @@ func hasCallToAsyncAlternative(c: ConvertAsync) {
 // CHECK-LOCAL-NOT: Global Rename
 // CHECK-LOCAL: ACTIONS END
 
+// CHECK-RENAME: Global Rename
+
 // CHECK-RENAME-EXTRACT: Global Rename
 // CHECK-RENAME-EXTRACT: Extract Method