[clangd] Implement textDocument/declaration from LSP 3.14

Summary:
LSP now reflects the declaration/definition distinction.

Language server changes:
 - textDocument/definition now returns a definition if one is found, otherwise
   the declaration. It no longer returns declaration + definition if they are
   distinct.
 - textDocument/declaration returns the best declaration we can find.
 - For macros, the active macro definition is returned for both methods.
 - For include directive, the top of the target file is returned for both.
There doesn't appear to be a discovery mechanism (we can't return everything to
clients that only know about definition), so this changes existing behavior.
In practice, it should greatly reduce the fraction of the time we need to show
the user a menu of options.

C++ API changes:
 - findDefinitions is replaced by locateSymbolAt, which returns a
   vector<LocatedSymbol> - one for each symbol under the cursor.
 - this contains the preferred declaration, the definition (if found), and
   the symbol name
This API enables some potentially-neat extensions, like swapping between decl
and def, and exposing the symbol name to the UI in the case of multiple symbols.

Reviewers: hokein

Subscribers: ilya-biryukov, javed.absar, ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Differential Revision: https://reviews.llvm.org/D57388

llvm-svn: 352864

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -354,6 +354,7 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params,
              llvm::json::Object{
                  {"triggerCharacters", {"(", ","}},
              }},
+            {"declarationProvider", true},
             {"definitionProvider", true},
             {"documentHighlightProvider", true},
             {"hoverProvider", true},
@@ -727,8 +728,37 @@ void ClangdLSPServer::onSignatureHelp(const TextDocumentPositionParams &Params,
 
 void ClangdLSPServer::onGoToDefinition(const TextDocumentPositionParams &Params,
                                        Callback<std::vector<Location>> Reply) {
-  Server->findDefinitions(Params.textDocument.uri.file(), Params.position,
+  Server->locateSymbolAt(
-                          std::move(Reply));
+      Params.textDocument.uri.file(), Params.position,
+      Bind(
+          [&](decltype(Reply) Reply,
+              llvm::Expected<std::vector<LocatedSymbol>> Symbols) {
+            if (!Symbols)
+              return Reply(Symbols.takeError());
+            std::vector<Location> Defs;
+            for (const auto &S : *Symbols)
+              Defs.push_back(S.Definition.getValueOr(S.PreferredDeclaration));
+            Reply(std::move(Defs));
+          },
+          std::move(Reply)));
+}
+
+void ClangdLSPServer::onGoToDeclaration(
+    const TextDocumentPositionParams &Params,
+    Callback<std::vector<Location>> Reply) {
+  Server->locateSymbolAt(
+      Params.textDocument.uri.file(), Params.position,
+      Bind(
+          [&](decltype(Reply) Reply,
+              llvm::Expected<std::vector<LocatedSymbol>> Symbols) {
+            if (!Symbols)
+              return Reply(Symbols.takeError());
+            std::vector<Location> Decls;
+            for (const auto &S : *Symbols)
+              Decls.push_back(S.PreferredDeclaration);
+            Reply(std::move(Decls));
+          },
+          std::move(Reply)));
 }
 
 void ClangdLSPServer::onSwitchSourceHeader(const TextDocumentIdentifier &Params,
@@ -814,6 +844,7 @@ ClangdLSPServer::ClangdLSPServer(class Transport &Transp,
   MsgHandler->bind("textDocument/completion", &ClangdLSPServer::onCompletion);
   MsgHandler->bind("textDocument/signatureHelp", &ClangdLSPServer::onSignatureHelp);
   MsgHandler->bind("textDocument/definition", &ClangdLSPServer::onGoToDefinition);
+  MsgHandler->bind("textDocument/declaration", &ClangdLSPServer::onGoToDeclaration);
   MsgHandler->bind("textDocument/references", &ClangdLSPServer::onReference);
   MsgHandler->bind("textDocument/switchSourceHeader", &ClangdLSPServer::onSwitchSourceHeader);
   MsgHandler->bind("textDocument/rename", &ClangdLSPServer::onRename);

clang-tools-extra/clangd/ClangdLSPServer.h

@@ -77,6 +77,8 @@ private:
   void onCompletion(const CompletionParams &, Callback<CompletionList>);
   void onSignatureHelp(const TextDocumentPositionParams &,
                        Callback<SignatureHelp>);
+  void onGoToDeclaration(const TextDocumentPositionParams &,
+                         Callback<std::vector<Location>>);
   void onGoToDefinition(const TextDocumentPositionParams &,
                         Callback<std::vector<Location>>);
   void onReference(const ReferenceParams &, Callback<std::vector<Location>>);

clang-tools-extra/clangd/ClangdServer.cpp

@@ -13,7 +13,6 @@
 #include "Headers.h"
 #include "SourceCode.h"
 #include "Trace.h"
-#include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/Merge.h"
 #include "refactor/Tweak.h"
@@ -400,13 +399,13 @@ void ClangdServer::dumpAST(PathRef File,
   WorkScheduler.runWithAST("DumpAST", File, Bind(Action, std::move(Callback)));
 }
 
-void ClangdServer::findDefinitions(PathRef File, Position Pos,
+void ClangdServer::locateSymbolAt(PathRef File, Position Pos,
-                                   Callback<std::vector<Location>> CB) {
+                                  Callback<std::vector<LocatedSymbol>> CB) {
-  auto Action = [Pos, this](Callback<std::vector<Location>> CB,
+  auto Action = [Pos, this](decltype(CB) CB,
                             llvm::Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
-    CB(clangd::findDefinitions(InpAST->AST, Pos, Index));
+    CB(clangd::locateSymbolAt(InpAST->AST, Pos, Index));
   };
 
   WorkScheduler.runWithAST("Definitions", File, Bind(Action, std::move(CB)));

clang-tools-extra/clangd/ClangdServer.h

@@ -18,6 +18,7 @@
 #include "GlobalCompilationDatabase.h"
 #include "Protocol.h"
 #include "TUScheduler.h"
+#include "XRefs.h"
 #include "index/Background.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
@@ -167,9 +168,9 @@ public:
   /// called for tracked files.
   void signatureHelp(PathRef File, Position Pos, Callback<SignatureHelp> CB);
 
-  /// Get definition of symbol at a specified \p Line and \p Column in \p File.
+  /// Find declaration/definition locations of symbol at a specified position.
-  void findDefinitions(PathRef File, Position Pos,
+  void locateSymbolAt(PathRef File, Position Pos,
-                       Callback<std::vector<Location>> CB);
+                      Callback<std::vector<LocatedSymbol>> CB);
 
   /// Helper function that returns a path to the corresponding source file when
   /// given a header file and vice versa.

clang-tools-extra/clangd/XRefs.cpp

@@ -21,18 +21,26 @@ namespace clang {
 namespace clangd {
 namespace {
 
-// Get the definition from a given declaration `D`.
+// Returns the single definition of the entity declared by D, if visible.
-// Return nullptr if no definition is found, or the declaration type of `D` is
+// In particular:
-// not supported.
+// - for non-redeclarable kinds (e.g. local vars), return D
+// - for kinds that allow multiple definitions (e.g. namespaces), return nullptr
+// Kinds of nodes that always return nullptr here will not have definitions
+// reported by locateSymbolAt().
 const Decl *getDefinition(const Decl *D) {
   assert(D);
+  // Decl has one definition that we can find.
   if (const auto *TD = dyn_cast<TagDecl>(D))
     return TD->getDefinition();
-  else if (const auto *VD = dyn_cast<VarDecl>(D))
+  if (const auto *VD = dyn_cast<VarDecl>(D))
     return VD->getDefinition();
-  else if (const auto *FD = dyn_cast<FunctionDecl>(D))
+  if (const auto *FD = dyn_cast<FunctionDecl>(D))
     return FD->getDefinition();
-  return nullptr;
+  // Only a single declaration is allowed.
+  if (isa<ValueDecl>(D)) // except cases above
+    return D;
+  // Multiple definitions are allowed.
+  return nullptr; // except cases above
 }
 
 void logIfOverflow(const SymbolLocation &Loc) {
@@ -240,8 +248,8 @@ llvm::Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc,
 
 } // namespace
 
-std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
-                                      const SymbolIndex *Index) {
+                                          const SymbolIndex *Index) {
   const auto &SM = AST.getASTContext().getSourceManager();
   auto MainFilePath =
       getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
@@ -250,116 +258,85 @@ std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
     return {};
   }
 
-  std::vector<Location> Result;
+  // Treat #included files as symbols, to enable go-to-definition on them.
-  // Handle goto definition for #include.
   for (auto &Inc : AST.getIncludeStructure().MainFileIncludes) {
-    if (!Inc.Resolved.empty() && Inc.R.start.line == Pos.line)
+    if (!Inc.Resolved.empty() && Inc.R.start.line == Pos.line) {
-      Result.push_back(
+      LocatedSymbol File;
-          Location{URIForFile::canonicalize(Inc.Resolved, *MainFilePath), {}});
+      File.Name = llvm::sys::path::filename(Inc.Resolved);
+      File.PreferredDeclaration = {
+          URIForFile::canonicalize(Inc.Resolved, *MainFilePath), Range{}};
+      File.Definition = File.PreferredDeclaration;
+      // We're not going to find any further symbols on #include lines.
+      return {std::move(File)};
+    }
   }
-  if (!Result.empty())
-    return Result;
 
-  // Identified symbols at a specific position.
   SourceLocation SourceLocationBeg =
       getBeginningOfIdentifier(AST, Pos, SM.getMainFileID());
   auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
 
-  for (auto Item : Symbols.Macros) {
+  // Macros are simple: there's no declaration/definition distinction.
-    auto Loc = Item.Info->getDefinitionLoc();
+  // As a consequence, there's no need to look them up in the index either.
-    auto L = makeLocation(AST, Loc, *MainFilePath);
+  std::vector<LocatedSymbol> Result;
-    if (L)
+  for (auto M : Symbols.Macros) {
-      Result.push_back(*L);
+    if (auto Loc =
+            makeLocation(AST, M.Info->getDefinitionLoc(), *MainFilePath)) {
+      LocatedSymbol Macro;
+      Macro.Name = M.Name;
+      Macro.PreferredDeclaration = *Loc;
+      Macro.Definition = Loc;
+      Result.push_back(std::move(Macro));
+    }
   }
 
-  // Declaration and definition are different terms in C-family languages, and
+  // Decls are more complicated.
-  // LSP only defines the "GoToDefinition" specification, so we try to perform
+  // The AST contains at least a declaration, maybe a definition.
-  // the "most sensible" GoTo operation:
+  // These are up-to-date, and so generally preferred over index results.
-  //
+  // We perform a single batch index lookup to find additional definitions.
-  //  - We use the location from AST and index (if available) to provide the
+
-  //    final results. When there are duplicate results, we prefer AST over
+  // Results follow the order of Symbols.Decls.
-  //    index because AST is more up-to-date.
+  // Keep track of SymbolID -> index mapping, to fill in index data later.
-  //
+  llvm::DenseMap<SymbolID, size_t> ResultIndex;
-  //  - For each symbol, we will return a location of the canonical declaration
-  //    (e.g. function declaration in header), and a location of definition if
-  //    they are available.
-  //
-  // So the work flow:
-  //
-  //   1. Identify the symbols being search for by traversing the AST.
-  //   2. Populate one of the locations with the AST location.
-  //   3. Use the AST information to query the index, and populate the index
-  //      location (if available).
-  //   4. Return all populated locations for all symbols, definition first (
-  //      which  we think is the users wants most often).
-  struct CandidateLocation {
-    llvm::Optional<Location> Def;
-    llvm::Optional<Location> Decl;
-  };
-  // We respect the order in Symbols.Decls.
-  llvm::SmallVector<CandidateLocation, 8> ResultCandidates;
-  llvm::DenseMap<SymbolID, size_t> CandidatesIndex;
 
   // Emit all symbol locations (declaration or definition) from AST.
   for (const DeclInfo &DI : Symbols.Decls) {
     const Decl *D = DI.D;
-    // Fake key for symbols don't have USR (no SymbolID).
+    auto Loc = makeLocation(AST, findNameLoc(D), *MainFilePath);
-    // Ideally, there should be a USR for each identified symbols. Symbols
+    if (!Loc)
-    // without USR are rare and unimportant cases, we use the a fake holder to
+      continue;
-    // minimize the invasiveness of these cases.
+
-    SymbolID Key("");
+    Result.emplace_back();
+    if (auto *ND = dyn_cast<NamedDecl>(D))
+      Result.back().Name = printName(AST.getASTContext(), *ND);
+    Result.back().PreferredDeclaration = *Loc;
+    // DeclInfo.D is always a definition if possible, so this check works.
+    if (getDefinition(D) == D)
+      Result.back().Definition = *Loc;
+
+    // Record SymbolID for index lookup later.
     if (auto ID = getSymbolID(D))
-      Key = *ID;
+      ResultIndex[*ID] = Result.size() - 1;
-
-    auto R = CandidatesIndex.try_emplace(Key, ResultCandidates.size());
-    if (R.second) // new entry
-      ResultCandidates.emplace_back();
-    auto &Candidate = ResultCandidates[R.first->second];
-
-    auto Loc = findNameLoc(D);
-    auto L = makeLocation(AST, Loc, *MainFilePath);
-    // The declaration in the identified symbols is a definition if possible
-    // otherwise it is declaration.
-    bool IsDef = getDefinition(D) == D;
-    // Populate one of the slots with location for the AST.
-    if (!IsDef)
-      Candidate.Decl = L;
-    else
-      Candidate.Def = L;
   }
 
-  if (Index) {
+  // Now query the index for all Symbol IDs we found in the AST.
+  if (Index && !ResultIndex.empty()) {
     LookupRequest QueryRequest;
-    // Build request for index query, using SymbolID.
+    for (auto It : ResultIndex)
-    for (auto It : CandidatesIndex)
       QueryRequest.IDs.insert(It.first);
-    std::string TUPath;
+    Index->lookup(QueryRequest, [&](const Symbol &Sym) {
-    const FileEntry *FE = SM.getFileEntryForID(SM.getMainFileID());
+      auto &R = Result[ResultIndex.lookup(Sym.ID)];
-    if (auto Path = getCanonicalPath(FE, SM))
-      TUPath = *Path;
-    // Query the index and populate the empty slot.
-    Index->lookup(QueryRequest, [&TUPath, &ResultCandidates,
-                                 &CandidatesIndex](const Symbol &Sym) {
-      auto It = CandidatesIndex.find(Sym.ID);
-      assert(It != CandidatesIndex.end());
-      auto &Value = ResultCandidates[It->second];
 
-      if (!Value.Def)
+      // Special case: if the AST yielded a definition, then it may not be
-        Value.Def = toLSPLocation(Sym.Definition, TUPath);
+      // the right *declaration*. Prefer the one from the index.
-      if (!Value.Decl)
+      if (R.Definition) // from AST
-        Value.Decl = toLSPLocation(Sym.CanonicalDeclaration, TUPath);
+        if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, *MainFilePath))
+          R.PreferredDeclaration = *Loc;
+
+      if (!R.Definition)
+        R.Definition = toLSPLocation(Sym.Definition, *MainFilePath);
     });
   }
 
-  // Populate the results, definition first.
-  for (const auto &Candidate : ResultCandidates) {
-    if (Candidate.Def)
-      Result.push_back(*Candidate.Def);
-    if (Candidate.Decl &&
-        Candidate.Decl != Candidate.Def) // Decl and Def might be the same
-      Result.push_back(*Candidate.Decl);
-  }
-
   return Result;
 }
 
@@ -805,5 +782,12 @@ std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos) {
   return Results;
 }
 
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const LocatedSymbol &S) {
+  OS << S.Name << ": " << S.PreferredDeclaration;
+  if (S.Definition)
+    OS << " def=" << *S.Definition;
+  return OS;
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/XRefs.h

@@ -22,9 +22,25 @@
 namespace clang {
 namespace clangd {
 
+// Describes where a symbol is declared and defined (as far as clangd knows).
+// There are three cases:
+//  - a declaration only, no definition is known (e.g. only header seen)
+//  - a declaration and a distinct definition (e.g. function declared in header)
+//  - a declaration and an equal definition (e.g. inline function, or class)
+// For some types of symbol, e.g. macros, definition == declaration always.
+struct LocatedSymbol {
+  // The (unqualified) name of the symbol.
+  std::string Name;
+  // The canonical or best declaration: where most users find its interface.
+  Location PreferredDeclaration;
+  // Where the symbol is defined, if known. May equal PreferredDeclaration.
+  llvm::Optional<Location> Definition;
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const LocatedSymbol &);
 /// Get definition of symbol at a specified \p Pos.
-std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+/// Multiple locations may be returned, corresponding to distinct symbols.
-                                      const SymbolIndex *Index = nullptr);
+std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
+                                          const SymbolIndex *Index = nullptr);
 
 /// Returns highlights for all usages of a symbol at \p Pos.
 std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,

clang-tools-extra/test/clangd/initialize-params.test

@@ -14,6 +14,7 @@
 # CHECK-NEXT:          ":"
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
+# CHECK-NEXT:      "declarationProvider": true,
 # CHECK-NEXT:      "definitionProvider": true,
 # CHECK-NEXT:      "documentFormattingProvider": true,
 # CHECK-NEXT:      "documentHighlightProvider": true,

clang-tools-extra/unittests/clangd/ClangdTests.cpp

@@ -43,8 +43,9 @@ using ::testing::IsEmpty;
 using ::testing::Pair;
 using ::testing::UnorderedElementsAre;
 
-MATCHER_P2(FileRange, File, Range, "") {
+MATCHER_P2(DeclAt, File, Range, "") {
-  return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
+  return arg.PreferredDeclaration ==
+         Location{URIForFile::canonicalize(File, testRoot()), Range};
 }
 
 bool diagsContainErrors(const std::vector<Diag> &Diagnostics) {
@@ -458,10 +459,9 @@ int hello;
               UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true),
                                    Pair(BazCpp, false)));
 
-  auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  auto Locations = runLocateSymbolAt(Server, FooCpp, FooSource.point());
   EXPECT_TRUE(bool(Locations));
-  EXPECT_THAT(*Locations,
+  EXPECT_THAT(*Locations, ElementsAre(DeclAt(FooCpp, FooSource.range("one"))));
-              ElementsAre(FileRange(FooCpp, FooSource.range("one"))));
 
   // Undefine MACRO, close baz.cpp.
   CDB.ExtraClangFlags.clear();
@@ -474,10 +474,9 @@ int hello;
   EXPECT_THAT(DiagConsumer.filesWithDiags(),
               UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false)));
 
-  Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  Locations = runLocateSymbolAt(Server, FooCpp, FooSource.point());
   EXPECT_TRUE(bool(Locations));
-  EXPECT_THAT(*Locations,
+  EXPECT_THAT(*Locations, ElementsAre(DeclAt(FooCpp, FooSource.range("two"))));
-              ElementsAre(FileRange(FooCpp, FooSource.range("two"))));
 }
 
 TEST_F(ClangdVFSTest, MemoryUsage) {
@@ -532,7 +531,7 @@ TEST_F(ClangdVFSTest, InvalidCompileCommand) {
   runAddDocument(Server, FooCpp, "int main() {}");
 
   EXPECT_EQ(runDumpAST(Server, FooCpp), "<no-ast>");
-  EXPECT_ERROR(runFindDefinitions(Server, FooCpp, Position()));
+  EXPECT_ERROR(runLocateSymbolAt(Server, FooCpp, Position()));
   EXPECT_ERROR(runFindDocumentHighlights(Server, FooCpp, Position()));
   EXPECT_ERROR(runRename(Server, FooCpp, Position(), "new_name"));
   // FIXME: codeComplete and signatureHelp should also return errors when they
@@ -717,7 +716,7 @@ int d;
                                clangd::CodeCompleteOptions()));
     };
 
-    auto FindDefinitionsRequest = [&]() {
+    auto LocateSymbolRequest = [&]() {
       unsigned FileIndex = FileIndexDist(RandGen);
       // Make sure we don't violate the ClangdServer's contract.
       if (ReqStats[FileIndex].FileIsRemoved)
@@ -727,13 +726,13 @@ int d;
       Pos.line = LineDist(RandGen);
       Pos.character = ColumnDist(RandGen);
 
-      ASSERT_TRUE(!!runFindDefinitions(Server, FilePaths[FileIndex], Pos));
+      ASSERT_TRUE(!!runLocateSymbolAt(Server, FilePaths[FileIndex], Pos));
     };
 
     std::vector<std::function<void()>> AsyncRequests = {
         AddDocumentRequest, ForceReparseRequest, RemoveDocumentRequest};
     std::vector<std::function<void()>> BlockingRequests = {
-        CodeCompletionRequest, FindDefinitionsRequest};
+        CodeCompletionRequest, LocateSymbolRequest};
 
     // Bash requests to ClangdServer in a loop.
     std::uniform_int_distribution<int> AsyncRequestIndexDist(

clang-tools-extra/unittests/clangd/SyncAPI.cpp

@@ -84,10 +84,10 @@ llvm::Expected<SignatureHelp> runSignatureHelp(ClangdServer &Server,
   return std::move(*Result);
 }
 
-llvm::Expected<std::vector<Location>>
+llvm::Expected<std::vector<LocatedSymbol>>
-runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos) {
+runLocateSymbolAt(ClangdServer &Server, PathRef File, Position Pos) {
-  llvm::Optional<llvm::Expected<std::vector<Location>>> Result;
+  llvm::Optional<llvm::Expected<std::vector<LocatedSymbol>>> Result;
-  Server.findDefinitions(File, Pos, capture(Result));
+  Server.locateSymbolAt(File, Pos, capture(Result));
   return std::move(*Result);
 }
 

clang-tools-extra/unittests/clangd/SyncAPI.h

@@ -32,8 +32,8 @@ runCodeComplete(ClangdServer &Server, PathRef File, Position Pos,
 llvm::Expected<SignatureHelp> runSignatureHelp(ClangdServer &Server,
                                                PathRef File, Position Pos);
 
-llvm::Expected<std::vector<Location>>
+llvm::Expected<std::vector<LocatedSymbol>>
-runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos);
+runLocateSymbolAt(ClangdServer &Server, PathRef File, Position Pos);
 
 llvm::Expected<std::vector<DocumentHighlight>>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);

clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp

@@ -687,10 +687,10 @@ TEST_F(TUSchedulerTests, TUStatus) {
   // We schedule the following tasks in the queue:
   //   [Update] [GoToDefinition]
   Server.addDocument(testPath("foo.cpp"), Code.code(), WantDiagnostics::Yes);
-  Server.findDefinitions(testPath("foo.cpp"), Code.point(),
+  Server.locateSymbolAt(testPath("foo.cpp"), Code.point(),
-                         [](Expected<std::vector<Location>> Result) {
+                        [](Expected<std::vector<LocatedSymbol>> Result) {
-                           ASSERT_TRUE((bool)Result);
+                          ASSERT_TRUE((bool)Result);
-                         });
+                        });
 
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 

clang-tools-extra/unittests/clangd/XRefsTests.cpp

@@ -17,6 +17,7 @@
 #include "index/SymbolCollector.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -25,7 +26,6 @@ namespace clangd {
 namespace {
 
 using testing::ElementsAre;
-using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
 using testing::UnorderedElementsAreArray;
@@ -95,9 +95,35 @@ TEST(HighlightsTest, All) {
   }
 }
 
+MATCHER_P3(Sym, Name, Decl, DefOrNone, "") {
+  llvm::Optional<Range> Def = DefOrNone;
+  if (Name != arg.Name) {
+    *result_listener << "Name is " << arg.Name;
+    return false;
+  }
+  if (Decl != arg.PreferredDeclaration.range) {
+    *result_listener << "Declaration is "
+                     << llvm::to_string(arg.PreferredDeclaration);
+    return false;
+  }
+  if (Def && !arg.Definition) {
+    *result_listener << "Has no definition";
+    return false;
+  }
+  if (Def && arg.Definition->range != *Def) {
+    *result_listener << "Definition is " << llvm::to_string(arg.Definition);
+    return false;
+  }
+  return true;
+}
+testing::Matcher<LocatedSymbol> Sym(std::string Name, Range Decl) {
+  return Sym(Name, Decl, llvm::None);
+}
+MATCHER_P(Sym, Name, "") { return arg.Name == Name; }
+
 MATCHER_P(RangeIs, R, "") { return arg.range == R; }
 
-TEST(GoToDefinition, WithIndex) {
+TEST(LocateSymbol, WithIndex) {
   Annotations SymbolHeader(R"cpp(
         class $forward[[Forward]];
         class $foo[[Foo]] {};
@@ -115,9 +141,9 @@ TEST(GoToDefinition, WithIndex) {
   TU.Code = SymbolCpp.code();
   TU.HeaderCode = SymbolHeader.code();
   auto Index = TU.index();
-  auto runFindDefinitionsWithIndex = [&Index](const Annotations &Main) {
+  auto LocateWithIndex = [&Index](const Annotations &Main) {
     auto AST = TestTU::withCode(Main.code()).build();
-    return clangd::findDefinitions(AST, Main.point(), Index.get());
+    return clangd::locateSymbolAt(AST, Main.point(), Index.get());
   };
 
   Annotations Test(R"cpp(// only declaration in AST.
@@ -126,9 +152,8 @@ TEST(GoToDefinition, WithIndex) {
           ^f1();
         }
       )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  EXPECT_THAT(LocateWithIndex(Test),
-              testing::ElementsAreArray(
+              ElementsAre(Sym("f1", Test.range(), SymbolCpp.range("f1"))));
-                  {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())}));
 
   Test = Annotations(R"cpp(// definition in AST.
         void [[f1]]() {}
@@ -136,30 +161,30 @@ TEST(GoToDefinition, WithIndex) {
           ^f1();
         }
       )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  EXPECT_THAT(LocateWithIndex(Test),
-              testing::ElementsAreArray(
+              ElementsAre(Sym("f1", SymbolHeader.range("f1"), Test.range())));
-                  {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))}));
 
   Test = Annotations(R"cpp(// forward declaration in AST.
         class [[Foo]];
         F^oo* create();
       )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  EXPECT_THAT(LocateWithIndex(Test),
-              testing::ElementsAreArray(
+              ElementsAre(Sym("Foo", Test.range(), SymbolHeader.range("foo"))));
-                  {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())}));
 
   Test = Annotations(R"cpp(// defintion in AST.
         class [[Forward]] {};
         F^orward create();
       )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+  EXPECT_THAT(
-              testing::ElementsAreArray({
+      LocateWithIndex(Test),
-                  RangeIs(Test.range()),
+      ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range())));
-                  RangeIs(SymbolHeader.range("forward")),
-              }));
 }
 
-TEST(GoToDefinition, All) {
+TEST(LocateSymbol, All) {
+  // Ranges in tests:
+  //   $decl is the declaration location (if absent, no symbol is located)
+  //   $def is the definition location (if absent, symbol has no definition)
+  //   unnamed range becomes both $decl and $def.
   const char *Tests[] = {
       R"cpp(// Local variable
         int main() {
@@ -186,7 +211,7 @@ TEST(GoToDefinition, All) {
       )cpp",
 
       R"cpp(// Function declaration via call
-        int [[foo]](int);
+        int $decl[[foo]](int);
         int main() {
           return ^foo(42);
         }
@@ -222,7 +247,7 @@ TEST(GoToDefinition, All) {
       )cpp",
 
       R"cpp(// Method call
-        struct Foo { int [[x]](); };
+        struct Foo { int $decl[[x]](); };
         int main() {
           Foo bar;
           bar.^x();
@@ -230,7 +255,7 @@ TEST(GoToDefinition, All) {
       )cpp",
 
       R"cpp(// Typedef
-        typedef int [[Foo]];
+        typedef int $decl[[Foo]];
         int main() {
           ^Foo bar;
         }
@@ -243,7 +268,7 @@ TEST(GoToDefinition, All) {
       )cpp", */
 
       R"cpp(// Namespace
-        namespace [[ns]] {
+        namespace $decl[[ns]] {
         struct Foo { static void bar(); }
         } // namespace ns
         int main() { ^ns::Foo::bar(); }
@@ -304,30 +329,45 @@ TEST(GoToDefinition, All) {
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
+    llvm::Optional<Range> WantDecl;
+    llvm::Optional<Range> WantDef;
+    if (!T.ranges().empty())
+      WantDecl = WantDef = T.range();
+    if (!T.ranges("decl").empty())
+      WantDecl = T.range("decl");
+    if (!T.ranges("def").empty())
+      WantDef = T.range("def");
+
     auto AST = TestTU::withCode(T.code()).build();
-    std::vector<Matcher<Location>> ExpectedLocations;
+    auto Results = locateSymbolAt(AST, T.point());
-    for (const auto &R : T.ranges())
+
-      ExpectedLocations.push_back(RangeIs(R));
+    if (!WantDecl) {
-    EXPECT_THAT(findDefinitions(AST, T.point()),
+      EXPECT_THAT(Results, IsEmpty()) << Test;
-                ElementsAreArray(ExpectedLocations))
+    } else {
-        << Test;
+      ASSERT_THAT(Results, ::testing::SizeIs(1)) << Test;
+      EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
+      llvm::Optional<Range> GotDef;
+      if (Results[0].Definition)
+        GotDef = Results[0].Definition->range;
+      EXPECT_EQ(WantDef, GotDef) << Test;
+    }
   }
 }
 
-TEST(GoToDefinition, Rank) {
+TEST(LocateSymbol, Ambiguous) {
   auto T = Annotations(R"cpp(
-    struct $foo1[[Foo]] {
+    struct Foo {
-      $foo2[[Foo]]();
+      Foo();
-      $foo3[[Foo]](Foo&&);
+      Foo(Foo&&);
-      $foo4[[Foo]](const char*);
+      Foo(const char*);
     };
 
-    Foo $f[[f]]();
+    Foo f();
 
-    void $g[[g]](Foo foo);
+    void g(Foo foo);
 
     void call() {
-      const char* $str[[str]] = "123";
+      const char* str = "123";
       Foo a = $1^str;
       Foo b = Foo($2^str);
       Foo c = $3^f();
@@ -336,26 +376,20 @@ TEST(GoToDefinition, Rank) {
     }
   )cpp");
   auto AST = TestTU::withCode(T.code()).build();
-  EXPECT_THAT(findDefinitions(AST, T.point("1")),
+  // Ordered assertions are deliberate: we expect a predictable order.
-              ElementsAre(RangeIs(T.range("str")), RangeIs(T.range("foo4"))));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("1")),
-  EXPECT_THAT(findDefinitions(AST, T.point("2")),
+              ElementsAre(Sym("str"), Sym("Foo")));
-              ElementsAre(RangeIs(T.range("str"))));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("2")), ElementsAre(Sym("str")));
-  EXPECT_THAT(findDefinitions(AST, T.point("3")),
+  EXPECT_THAT(locateSymbolAt(AST, T.point("3")),
-              ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3"))));
+              ElementsAre(Sym("f"), Sym("Foo")));
-  EXPECT_THAT(findDefinitions(AST, T.point("4")),
+  EXPECT_THAT(locateSymbolAt(AST, T.point("4")), ElementsAre(Sym("g")));
-              ElementsAre(RangeIs(T.range("g"))));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("5")),
-  EXPECT_THAT(findDefinitions(AST, T.point("5")),
+              ElementsAre(Sym("f"), Sym("Foo")));
-              ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3"))));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("6")),
-
+              ElementsAre(Sym("str"), Sym("Foo"), Sym("Foo")));
-  auto DefinitionAtPoint6 = findDefinitions(AST, T.point("6"));
-  EXPECT_EQ(3ul, DefinitionAtPoint6.size());
-  EXPECT_THAT(DefinitionAtPoint6, HasSubsequence(RangeIs(T.range("str")),
-                                                 RangeIs(T.range("foo4"))));
-  EXPECT_THAT(DefinitionAtPoint6, HasSubsequence(RangeIs(T.range("str")),
-                                                 RangeIs(T.range("foo3"))));
 }
 
-TEST(GoToDefinition, RelPathsInCompileCommand) {
+TEST(LocateSymbol, RelPathsInCompileCommand) {
   // The source is in "/clangd-test/src".
   // We build in "/clangd-test/build".
 
@@ -397,24 +431,23 @@ int [[bar_not_preamble]];
 
   // Go to a definition in main source file.
   auto Locations =
-      runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
+      runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
+  EXPECT_THAT(*Locations, ElementsAre(Sym("foo", SourceAnnotations.range())));
-              ElementsAre(FileRange(FooCpp, SourceAnnotations.range())));
 
   // Go to a definition in header_in_preamble.h.
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p2"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
+  EXPECT_THAT(
-              ElementsAre(FileRange(HeaderInPreambleH,
+      *Locations,
-                                    HeaderInPreambleAnnotations.range())));
+      ElementsAre(Sym("bar_preamble", HeaderInPreambleAnnotations.range())));
 
   // Go to a definition in header_not_in_preamble.h.
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p3"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations,
-              ElementsAre(FileRange(HeaderNotInPreambleH,
+              ElementsAre(Sym("bar_not_preamble",
-                                    HeaderNotInPreambleAnnotations.range())));
+                              HeaderNotInPreambleAnnotations.range())));
 }
 
 TEST(Hover, All) {
@@ -1061,46 +1094,39 @@ TEST(GoToInclude, All) {
   Server.addDocument(FooCpp, SourceAnnotations.code());
 
   // Test include in preamble.
-  auto Locations =
+  auto Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point());
-      runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
-  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
-  EXPECT_THAT(*Locations,
-              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
   // Test include in preamble, last char.
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("2"));
+  Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("2"));
-  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
-  EXPECT_THAT(*Locations,
+  EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
-              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3"));
+  Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("3"));
-  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
-  EXPECT_THAT(*Locations,
+  EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
-              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
   // Test include outside of preamble.
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("6"));
+  Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("6"));
-  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
-  EXPECT_THAT(*Locations,
+  EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
-              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
   // Test a few positions that do not result in Locations.
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4"));
+  Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("4"));
-  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
   EXPECT_THAT(*Locations, IsEmpty());
 
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5"));
+  Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("5"));
-  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
-  EXPECT_THAT(*Locations,
+  EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
-              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
+  Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("7"));
-  ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
-  EXPECT_THAT(*Locations,
+  EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
-              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 }
 
-TEST(GoToDefinition, WithPreamble) {
+TEST(LocateSymbol, WithPreamble) {
   // Test stragety: AST should always use the latest preamble instead of last
   // good preamble.
   MockFSProvider FS;
@@ -1120,18 +1146,18 @@ TEST(GoToDefinition, WithPreamble) {
   FS.Files[FooH] = FooHeader.code();
 
   runAddDocument(Server, FooCpp, FooWithHeader.code());
-  // GoToDefinition goes to a #include file: the result comes from the preamble.
+  // LocateSymbol goes to a #include file: the result comes from the preamble.
   EXPECT_THAT(
-      cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
+      cantFail(runLocateSymbolAt(Server, FooCpp, FooWithHeader.point())),
-      ElementsAre(FileRange(FooH, FooHeader.range())));
+      ElementsAre(Sym("foo.h", FooHeader.range())));
 
   // Only preamble is built, and no AST is built in this request.
   Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
   // We build AST here, and it should use the latest preamble rather than the
   // stale one.
   EXPECT_THAT(
-      cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+      cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
-      ElementsAre(FileRange(FooCpp, FooWithoutHeader.range())));
+      ElementsAre(Sym("foo", FooWithoutHeader.range())));
 
   // Reset test environment.
   runAddDocument(Server, FooCpp, FooWithHeader.code());
@@ -1139,8 +1165,8 @@ TEST(GoToDefinition, WithPreamble) {
   Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
   // Use the AST being built in above request.
   EXPECT_THAT(
-      cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+      cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
-      ElementsAre(FileRange(FooCpp, FooWithoutHeader.range())));
+      ElementsAre(Sym("foo", FooWithoutHeader.range())));
 }
 
 TEST(FindReferences, WithinAST) {