[clangd] Add USR to textDocument/definition response

[jkorous]

Summary

We need a way for given code position to get the best definition/declaration location in given translation unit and it's USR.

Since the first element in list returned by textDocument/definition is meeting the def/decl location requirement we'd just need to add the USR. Other option would be to split this to a separate method. I thought this patch to be a better fit from protocol perspective but I am happy to go the other way if that's preferred.

I'd like to ask if someone more familiar with gtest matchers could take a look at changes I made in unit-tests. I am not sure it's exactly the idiomatic way. I was trying to use Property() matchers first but gave up since I wasn't able to debug it (got lost in template error messages).

[sammccall]

Hi Jan,

Clangd uses SymbolIDs rather than USRs, to identify symbols.

However these are used only internally, and for extension point APIs (SymbolIndex), and not actually exposed over the wire.
Can you explain more about the need to identify the symbol in go-to-definition?

Conceptually the idea of providing at least an opaque identifier when naming symbols makes sense - the cursor position can often be ambiguous.
However LSP decided not to go this route, and we risk diverging (e.g. should "hover" be able to accept such an identifier too? etc).

Giving particular semantics to the identifiers seems much more risky.

[jkorous]

One more thing - shall I create new client capability flag for this?

[jkorous]

Hi Sam!

I am aware of clangd using SymbolID. We basically need USR for integration with our external indexing service. We don't plan to systematically use it in the protocol and I am not aware of any other requirement for using USR in LSP - will double check that to be sure.

[jkorous]

We are also discussing creating separate method as we'll likely want to add other data to the response in the future. Would you prefer USR not to be in the standard part of LSP but only in our extension?

[jkorous]

Correction - I asked offline about our intended use of USRs in LSP and it seems we might want to receive it as part of other responses too. Nothing specific for now but it's probable that it won't be just this singular case.

[benlangmuir]

@sammccall RE using USRs: we want to integrate clangd into a larger source tooling system that is already using USRs extensively to identify symbols. The most obvious case is that we have an index outside of clangd that uses USRs from clang-the-compiler, so exposing USRs in clangd lets us integrate information from both places. The most critical piece of this is being able to ask "what symbol is at this location" and correlate that with index queries. Beyond this one case, my experience is that any time a language service API is providing information about a symbol or list of symbols it may be useful to add the USR (e.g. hover, definition or workspace symbols).

You mentioned a concern that this would make us diverge from LSP - could you expand on that? I think for any existing requests the USR is purely additive. If the concern is about affecting the return values of existing queries, would it help to put this behind a configuration option?

There is also an extension that might be interesting to look at here: https://github.com/sourcegraph/language-server-protocol/blob/master/extension-symbol-descriptor.md In this case the symbol descriptor is opaque, but in practice our use case would require knowing how to dig out a USR string.

For the specific use case of getting cursor info, as @jkorous mentioned we might want to move this out of "definition". Our experience with sourcekitd for Swift is that a cursor info request is a useful place to put an assortment of information that may grow over time - e.g. name, type name, USR, type USR, symbol kind, local declaration/definition location, etc.

[sammccall]

If we're extending the API, we should be careful to do it in an orthogonal and reasonable general way. Apple may not need USR in other places now, but there are users other than Apple and times other than today :-)

One of the reasons this looks odd is the method is "find definitions" rather than "get symbol info". So e.g. it's odd to return the symbol USR here but not in "find references", and doing a lookup in the index is unnecessary if what you want want is the USR. So I do think a new "get symbol info" method is better, if that's what you want.

This is doable as an LSP extension, though it does give me pause to reconsider the design:

• • the C++ API is for this much easier to design: get a SymbolID & Decl* while the AST is alive, with the option of looking up in the index. With LSP, it's not obvious what to include/exclude.
  • an LSP call can't return any persistent handle to the symbol (i.e Decl*) as the AST has no lifetime guarantees. So any follow-up operation on the symbol needs to repeat work and handle errors (symbol gone?). This puts further pressure on orthogonality.
• it's early days, are there more? The more custom actions there are that don't fit LSP, the less coherent ClangdLSPServer becomes (it's meant to model LSP quite closely, where ClangdServer matches "reality")

So are we still sure "mostly unmodified clangd over a different transport with a few extensions" is a better fit for the xcode XPC service than "AST management from ClangdServer, some features from ClangdServer, some custom features built on TUScheduler, exposed over an XPC protocol inspired by LSP"?

[sammccall]

@benlangmuir: I hadn't seen your comment while replying, sorry for any confusion...

The most critical piece of this is being able to ask "what symbol is at this location" and correlate that with index queries.

This makes sense (at least doing follow-up index queries, I'm not sure exactly what "correlate" would entail).
clangd does have an internal concept of an index that is designed as an integration point for out-of-tree implementations.
Of course you're free not to use that and to query a separate index based on results returned from the LSP layer, but this is likely to be lossy (e.g. find refs will give you an empty list if the symbol under the cursor has no refs visible to clangd, leaving nothing to query the external index for).
The SymbolIndex API is not USR based, but in practice SymbolIDs are truncated hashes of USRs and we could probably add some guarantees there if it would help - integration may be an option.

Beyond this one case, my experience is that any time a language service API is providing information about a symbol or list of symbols it may be useful to add the USR (e.g. hover, definition or workspace symbols).

So in general I'm afraid this is unlikely.

• As mentioned, clangd doesn't use USRs to identify symbols, but rather SymbolID. This representation was chosen specifically to avoid coupling to USR, and to be more compact. Index results do not have USRs available.
• in principle embedding SymbolID in results seems ok. I do think in practice there's not much you can *do* with one outside clangd, without guarantees about SymbolID format. At that point (with guarantees) we're closer to the C++-embedding level of stability/coupling rather than the LSP-protocol level.

Our experience with sourcekitd for Swift is that a cursor info request is a useful place to put an assortment of information that may grow over time

Good to know, this does seem like a reasonable direction, and exposing USR there seems totally fine. The only high-level question I can see here is should such a request hit the index or rely on AST only? The index is needed for e.g. definitions not visible in the AST, but may spend some CPU or even a network RPC.

[arphaman]

I don't think we should be using textDocument/definition here, and I agree with Sam that a new method would be better. We don't actually need the semantic guarantees/constrains imposed by LSP's description of textDocument/definition, as we want to find any declaration that corresponds to the reference at the location, so we don't need to guarantee that we return some definition. This would also avoid any confusion about USRs returned in textDocument/definition/textDocument/references.

@sammccall

Good to know, this does seem like a reasonable direction, and exposing USR there seems totally fine. The only high-level question I can see here is should such a request hit the index or rely on AST only

It's up to all of us to come up with the semantics of this new request. For our use-case we need the AST information only, and we don't want to pay the index hit here. I would propose to define the semantics similar to returning information about the symbol at the location from the view point of just the textDocument in which the query is located. This might not be useful to high level clients that want all queries to be backed by the Index, but it could be useful to other clients that are interested in the symbol as its viewed by that text document. We don't have to specify whether it's a declaration or a definition as well.

it's early days, are there more? The more custom actions there are that don't fit LSP, the less coherent ClangdLSPServer becomes (it's meant to model LSP quite closely, where ClangdServer matches "reality")

So are we still sure "mostly unmodified clangd over a different transport with a few extensions" is a better fit for the xcode XPC service than "AST management from ClangdServer, some features from ClangdServer, some custom features built on TUScheduler, exposed over an XPC protocol inspired by LSP"?

We are committed to relying on LSP with necessary extension / capability points. We don't really have any other option at this point due to scheduling concerns. As far as the need for different customizations, this "cursor-info" request is probably the most low level extension we need, and the majority of our future work will be relevant to high level, pure LSP clients (e.g. semantic highlighting, refactoring).

@jkorous

We don't need to rely on finding the definition or using the index for the new method. What we really need is to map from a location to a Decl * which we can represent as a symbol (USR, etc.) in the response. A declaration in this case is perfectly fine. You can leverage existing code like clang::tooling::getNamedDeclAt that will return the declaration for a location, and that's used by things like clang-rename.

[sammccall]

OK, sounds like we have something to move forward with. I'd suggest we start with an operation returning {SymbolID, scope qualifiers, unqualified name, USR} and ignoring location for now, unless you have an immediate need. Reason being this sidesteps the index question, what the semantics of the location are, which redecl to use etc. Hmm, this looks a lot like SymbolInformation as used by textDocument/documentSymbol with a USR extension. We could consider reusing that type...

A declaration in this case is perfectly fine. You can leverage existing code like clang::tooling::getNamedDeclAt that will return the declaration for a location, and that's used by things like clang-rename.

I'd suggest putting this operation in XRefs.cpp and reusing getSymbolAtPosition, because that's consistent with how we handle other "symbol at cursor" operations, and is aware of our preamble handling. If the implementation can be improved or reuse other libraries that's a good idea, but it should be uniform.
(getNamedDeclAt looks simpler/cleaner, but I assume you want this to work when the cursor is on a reference rather than a decl? If I'm reading getNamedDeclAt correctly, it doesn't seem to handle that).

[jkorous]

Morphed into https://reviews.llvm.org/D54799

ClangdLSPServer.h

private:
   void onDocumentSymbol(const DocumentSymbolParams &,
                         Callback<std::vector<SymbolInformation>>);
   void onCodeAction(const CodeActionParams &, Callback<llvm::json::Value>);
   void onCompletion(const TextDocumentPositionParams &,
                     Callback<CompletionList>);
   void onSignatureHelp(const TextDocumentPositionParams &,
                        Callback<SignatureHelp>);
   void onGoToDefinition(const TextDocumentPositionParams &,
-                        Callback<std::vector<Location>>);
+                        Callback<std::vector<DefinitionData>>);
   void onReference(const ReferenceParams &, Callback<std::vector<Location>>);
   void onSwitchSourceHeader(const TextDocumentIdentifier &,
                             Callback<std::string>);
   void onDocumentHighlight(const TextDocumentPositionParams &,
                            Callback<std::vector<DocumentHighlight>>);
   void onFileEvent(const DidChangeWatchedFilesParams &);
   void onCommand(const ExecuteCommandParams &, Callback<llvm::json::Value>);
   void onWorkspaceSymbol(const WorkspaceSymbolParams &,

ClangdLSPServer.cpp

 }
 
 void ClangdLSPServer::onSignatureHelp(const TextDocumentPositionParams &Params,
                                       Callback<SignatureHelp> Reply) {
   Server->signatureHelp(Params.textDocument.uri.file(), Params.position,
                         std::move(Reply));
 }
 
-void ClangdLSPServer::onGoToDefinition(const TextDocumentPositionParams &Params,
-                                       Callback<std::vector<Location>> Reply) {
+void ClangdLSPServer::onGoToDefinition(
+    const TextDocumentPositionParams &Params,
+    Callback<std::vector<DefinitionData>> Reply) {
   Server->findDefinitions(Params.textDocument.uri.file(), Params.position,
                           std::move(Reply));
 }
 
 void ClangdLSPServer::onSwitchSourceHeader(const TextDocumentIdentifier &Params,
                                            Callback<std::string> Reply) {
   Optional<Path> Result = Server->switchSourceHeader(Params.uri.file());
   Reply(Result ? URI::createFile(*Result).toString() : "");

ClangdServer.h

void codeComplete(PathRef File, Position Pos,
                     Callback<CodeCompleteResult> CB);
 
   /// Provide signature help for \p File at \p Pos.  This method should only be
   /// 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.
   void findDefinitions(PathRef File, Position Pos,
-                       Callback<std::vector<Location>> CB);
+                       Callback<std::vector<DefinitionData>> CB);
 
   /// Helper function that returns a path to the corresponding source file when
   /// given a header file and vice versa.
   llvm::Optional<Path> switchSourceHeader(PathRef Path);
 
   /// Get document highlights for a given position.
   void findDocumentHighlights(PathRef File, Position Pos,
                               Callback<std::vector<DocumentHighlight>> CB);

ClangdServer.cpp

auto Action = [](decltype(Callback) Callback, Expected<InputsAndAST> InpAST) {
 
     Callback(Result);
   };
 
   WorkScheduler.runWithAST("DumpAST", File, Bind(Action, std::move(Callback)));
 }
 
 void ClangdServer::findDefinitions(PathRef File, Position Pos,
-                                   Callback<std::vector<Location>> CB) {
-  auto Action = [Pos, this](Callback<std::vector<Location>> CB,
+                                   Callback<std::vector<DefinitionData>> CB) {
+  auto Action = [Pos, this](Callback<std::vector<DefinitionData>> CB,
                             Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     CB(clangd::findDefinitions(InpAST->AST, Pos, Index));
   };
 
   WorkScheduler.runWithAST("Definitions", File, Bind(Action, std::move(CB)));
 }

Protocol.h

struct Location {
 
   friend bool operator<(const Location &LHS, const Location &RHS) {
     return std::tie(LHS.uri, LHS.range) < std::tie(RHS.uri, RHS.range);
   }
 };
 llvm::json::Value toJSON(const Location &);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Location &);
 
+struct DefinitionData {
+  Location DefOrDeclLocation;
+  llvm::SmallString<128> USR;
+
+  friend bool operator==(const DefinitionData &LHS, const DefinitionData &RHS) {
+    return LHS.DefOrDeclLocation == RHS.DefOrDeclLocation && LHS.USR == RHS.USR;
+  }
+
+  friend bool operator!=(const DefinitionData &LHS, const DefinitionData &RHS) {
+    return !(LHS == RHS);
+  }
+
+  friend bool operator<(const DefinitionData &LHS, const DefinitionData &RHS) {
+    return std::tie(LHS.DefOrDeclLocation, LHS.USR) <
+           std::tie(RHS.DefOrDeclLocation, RHS.USR);
+  }
+};
+llvm::json::Value toJSON(const DefinitionData &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const DefinitionData &);
+
 struct TextEdit {
   /// The range of the text document to be manipulated. To insert
   /// text into a document create a range where start === end.
   Range range;
 
   /// The string to be inserted. For delete operations use an
   /// empty string.
   std::string newText;

Protocol.cpp

return json::Object{
       {"range", P.range},
   };
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const Location &L) {
   return OS << L.range << '@' << L.uri;
 }
 
+json::Value toJSON(const DefinitionData &P) {
+  return json::Object{
+      {"uri", P.DefOrDeclLocation.uri},
+      {"range", P.DefOrDeclLocation.range},
+      {"usr", P.USR},
+  };
+}
+
+raw_ostream &operator<<(raw_ostream &OS, const DefinitionData &L) {
+  return OS << L.USR << '@' << L.DefOrDeclLocation.uri << ":"
+            << L.DefOrDeclLocation.range;
+}
+
 bool fromJSON(const json::Value &Params, TextDocumentItem &R) {
   json::ObjectMapper O(Params);
   return O && O.map("uri", R.uri) && O.map("languageId", R.languageId) &&
          O.map("version", R.version) && O.map("text", R.text);
 }
 
 bool fromJSON(const json::Value &Params, TextEdit &R) {
   json::ObjectMapper O(Params);

XRefs.h

 #include "index/Index.h"
 #include "llvm/ADT/Optional.h"
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 /// Get definition of symbol at a specified \p Pos.
-std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+std::vector<DefinitionData> findDefinitions(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,
                                                       Position Pos);
 
 /// Get the hover information when hovering at \p Pos.
 llvm::Optional<Hover> getHover(ParsedAST &AST, Position Pos);
 
 /// Returns reference locations of the symbol at a specified \p Pos.
 std::vector<Location> findReferences(ParsedAST &AST, Position Pos,
                                      const SymbolIndex *Index = nullptr);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_XREFS_H

XRefs.cpp

Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc) {
   Location L;
   L.uri = URIForFile(*FilePath);
   L.range = getTokenRange(AST, TokLoc);
   return L;
 }
 
 } // namespace
 
-std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+std::vector<DefinitionData> findDefinitions(ParsedAST &AST, Position Pos,
                                             const SymbolIndex *Index) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
 
-  std::vector<Location> Result;
+  std::vector<DefinitionData> Result;
   // Handle goto definition for #include.
   for (auto &Inc : AST.getIncludeStructure().MainFileIncludes) {
     if (!Inc.Resolved.empty() && Inc.R.start.line == Pos.line)
-      Result.push_back(Location{URIForFile{Inc.Resolved}, {}});
+      Result.push_back(DefinitionData{Location{URIForFile{Inc.Resolved}, {}},
+                                      SmallString<128>{}});
   }
   if (!Result.empty())
     return Result;
 
   // Identified symbols at a specific position.
   SourceLocation SourceLocationBeg =
       getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
   auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
 
   for (auto Item : Symbols.Macros) {
     auto Loc = Item.Info->getDefinitionLoc();
     auto L = makeLocation(AST, Loc);
     if (L)
-      Result.push_back(*L);
+      Result.push_back(DefinitionData{*L, SmallString<128>{}});
   }
 
   // Declaration and definition are different terms in C-family languages, and
   // LSP only defines the "GoToDefinition" specification, so we try to perform
   // the "most sensible" GoTo operation:
   //
   //  - We use the location from AST and index (if available) to provide the
   //    final results. When there are duplicate results, we prefer AST over
   //    index because AST is more up-to-date.
   //
   //  - 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 {
+  struct CandidateInfo {
     Optional<Location> Def;
     Optional<Location> Decl;
+    SmallString<128> USR;
   };
   // We respect the order in Symbols.Decls.
-  SmallVector<CandidateLocation, 8> ResultCandidates;
+  SmallVector<CandidateInfo, 8> ResultCandidates;
   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).
     // Ideally, there should be a USR for each identified symbols. Symbols
     // without USR are rare and unimportant cases, we use the a fake holder to
     // minimize the invasiveness of these cases.
+    SmallString<128> USR;
     SymbolID Key("");
-    if (auto ID = getSymbolID(D))
-      Key = *ID;
+    if (!index::generateUSRForDecl(D, USR))
+      Key = SymbolID(USR);
 
     auto R = CandidatesIndex.try_emplace(Key, ResultCandidates.size());
-    if (R.second) // new entry
+    if (R.second) { // new entry
       ResultCandidates.emplace_back();
+      ResultCandidates[R.first->second].USR = USR;
+    }
+
     auto &Candidate = ResultCandidates[R.first->second];
 
     auto Loc = findNameLoc(D);
     auto L = makeLocation(AST, Loc);
     // 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.
Index->lookup(QueryRequest, [&HintPath, &ResultCandidates,
       if (!Value.Decl)
         Value.Decl = toLSPLocation(Sym.CanonicalDeclaration, HintPath);
     });
   }
 
   // Populate the results, definition first.
   for (const auto &Candidate : ResultCandidates) {
     if (Candidate.Def)
-      Result.push_back(*Candidate.Def);
+      Result.push_back(DefinitionData{*Candidate.Def, Candidate.USR});
     if (Candidate.Decl &&
         Candidate.Decl != Candidate.Def) // Decl and Def might be the same
-      Result.push_back(*Candidate.Decl);
+      Result.push_back(DefinitionData{*Candidate.Decl, Candidate.USR});
   }
 
   return Result;
 }
 
 namespace {
 
 /// Collects references to symbols within the main file.

clangd/ClangdTests.cpp

 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
 using ::testing::Pair;
 using ::testing::UnorderedElementsAre;
 
 namespace {
 
+MATCHER_P(LocationIs, R, "") { return arg.DefOrDeclLocation == R; }
+
 bool diagsContainErrors(const std::vector<Diag> &Diagnostics) {
   for (auto D : Diagnostics) {
     if (D.Severity == DiagnosticsEngine::Error ||
         D.Severity == DiagnosticsEngine::Fatal)
       return true;
   }
   return false;
 }
)cpp");
   Server.addDocument(BarCpp, BarSource.code());
   Server.addDocument(BazCpp, BazSource.code());
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 
   EXPECT_THAT(DiagConsumer.filesWithDiags(),
               UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true),
                                    Pair(BazCpp, false)));
 
-  auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
-  EXPECT_TRUE(bool(Locations));
-  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
-                                               FooSource.range("one")}));
+  auto DefData = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(DefData));
+  EXPECT_THAT(*DefData, ElementsAre(LocationIs(Location{
+                            URIForFile{FooCpp}, FooSource.range("one")})));
 
   // Undefine MACRO, close baz.cpp.
   CDB.ExtraClangFlags.clear();
   DiagConsumer.clear();
   Server.removeDocument(BazCpp);
   Server.addDocument(FooCpp, FooSource.code(), WantDiagnostics::Auto);
   Server.addDocument(BarCpp, BarSource.code(), WantDiagnostics::Auto);
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 
   EXPECT_THAT(DiagConsumer.filesWithDiags(),
               UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false)));
 
-  Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
-  EXPECT_TRUE(bool(Locations));
-  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
-                                               FooSource.range("two")}));
+  DefData = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(DefData));
+  EXPECT_THAT(*DefData, ElementsAre(LocationIs(Location{
+                            URIForFile{FooCpp}, FooSource.range("two")})));
 }
 
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 

clangd/SyncAPI.h

 
 llvm::Expected<CodeCompleteResult>
 runCodeComplete(ClangdServer &Server, PathRef File, Position Pos,
                 clangd::CodeCompleteOptions Opts);
 
 llvm::Expected<SignatureHelp> runSignatureHelp(ClangdServer &Server,
                                                PathRef File, Position Pos);
 
-llvm::Expected<std::vector<Location>>
+llvm::Expected<std::vector<DefinitionData>>
 runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos);
 
 llvm::Expected<std::vector<DocumentHighlight>>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
 llvm::Expected<std::vector<tooling::Replacement>>
 runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
 

clangd/SyncAPI.cpp

 
 Expected<SignatureHelp> runSignatureHelp(ClangdServer &Server, PathRef File,
                                          Position Pos) {
   Optional<Expected<SignatureHelp>> Result;
   Server.signatureHelp(File, Pos, capture(Result));
   return std::move(*Result);
 }
 
-Expected<std::vector<Location>> runFindDefinitions(ClangdServer &Server,
-                                                   PathRef File, Position Pos) {
-  Optional<Expected<std::vector<Location>>> Result;
+Expected<std::vector<DefinitionData>>
+runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos) {
+  Optional<Expected<std::vector<DefinitionData>>> Result;
   Server.findDefinitions(File, Pos, capture(Result));
   return std::move(*Result);
 }
 
 Expected<std::vector<DocumentHighlight>>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos) {
   Optional<Expected<std::vector<DocumentHighlight>>> Result;
   Server.findDocumentHighlights(File, Pos, capture(Result));

clangd/XRefsTests.cpp

for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = TestTU::withCode(T.code()).build();
     EXPECT_THAT(findDocumentHighlights(AST, T.point()), HighlightsFrom(T))
         << Test;
   }
 }
 
 MATCHER_P(RangeIs, R, "") { return arg.range == R; }
+MATCHER_P(RangeInLocationIs, R, "") { return arg.DefOrDeclLocation.range == R; }
+MATCHER_P(RangeInLocationIsRH, R, "") {
+  return arg == R.DefOrDeclLocation.range;
+}
+MATCHER_P(LocationIs, R, "") { return arg.DefOrDeclLocation == R; }
 
 TEST(GoToDefinition, WithIndex) {
   Annotations SymbolHeader(R"cpp(
         class $forward[[Forward]];
         class $foo[[Foo]] {};
 
         void $f1[[f1]]();
 
TEST(GoToDefinition, WithIndex) {
   };
 
   Annotations Test(R"cpp(// only declaration in AST.
         void [[f1]]();
         int main() {
           ^f1();
         }
       )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
-              testing::ElementsAreArray(
-                  {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())}));
+  EXPECT_THAT(
+      runFindDefinitionsWithIndex(Test),
+      testing::ElementsAreArray({RangeInLocationIs(SymbolCpp.range("f1")),
+                                 RangeInLocationIs(Test.range())}));
 
   Test = Annotations(R"cpp(// definition in AST.
         void [[f1]]() {}
         int main() {
           ^f1();
         }
       )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
-              testing::ElementsAreArray(
-                  {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))}));
+  EXPECT_THAT(
+      runFindDefinitionsWithIndex(Test),
+      testing::ElementsAreArray({RangeInLocationIs(Test.range()),
+                                 RangeInLocationIs(SymbolHeader.range("f1"))}));
 
   Test = Annotations(R"cpp(// forward declaration in AST.
         class [[Foo]];
         F^oo* create();
       )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
-              testing::ElementsAreArray(
-                  {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())}));
+  EXPECT_THAT(
+      runFindDefinitionsWithIndex(Test),
+      testing::ElementsAreArray({RangeInLocationIs(SymbolHeader.range("foo")),
+                                 RangeInLocationIs(Test.range())}));
 
   Test = Annotations(R"cpp(// defintion in AST.
         class [[Forward]] {};
         F^orward create();
       )cpp");
   EXPECT_THAT(runFindDefinitionsWithIndex(Test),
               testing::ElementsAreArray({
-                  RangeIs(Test.range()), RangeIs(SymbolHeader.range("forward")),
+                  RangeInLocationIs(Test.range()),
+                  RangeInLocationIs(SymbolHeader.range("forward")),
               }));
 }
 
 TEST(GoToDefinition, All) {
   const char *Tests[] = {
       R"cpp(// Local variable
         int main() {
           int [[bonjour]];
const char *Tests[] = {
          #define FF() class [[Test]] {};
          FF();
          void f() { T^est a; }
       )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = TestTU::withCode(T.code()).build();
-    std::vector<Matcher<Location>> ExpectedLocations;
-    for (const auto &R : T.ranges())
-      ExpectedLocations.push_back(RangeIs(R));
-    EXPECT_THAT(findDefinitions(AST, T.point()),
-                ElementsAreArray(ExpectedLocations))
-        << Test;
+
+    auto definitions = findDefinitions(AST, T.point());
+    std::vector<Matcher<Range>> Assertions;
+    for (const auto &D : definitions)
+      Assertions.push_back(RangeInLocationIsRH(D));
+
+    EXPECT_THAT(T.ranges(), ElementsAreArray(Assertions)) << Test;
   }
 }
 
 TEST(GoToDefinition, Rank) {
   auto T = Annotations(R"cpp(
     struct $foo1[[Foo]] {
       $foo2[[Foo]]();
       $foo3[[Foo]](Foo&&);
void call() {
       Foo b = Foo($2^str);
       Foo c = $3^f();
       $4^g($5^f());
       g($6^str);
     }
   )cpp");
   auto AST = TestTU::withCode(T.code()).build();
   EXPECT_THAT(findDefinitions(AST, T.point("1")),
-              ElementsAre(RangeIs(T.range("str")), RangeIs(T.range("foo4"))));
+              ElementsAre(RangeInLocationIs(T.range("str")),
+                          RangeInLocationIs(T.range("foo4"))));
   EXPECT_THAT(findDefinitions(AST, T.point("2")),
-              ElementsAre(RangeIs(T.range("str"))));
+              ElementsAre(RangeInLocationIs(T.range("str"))));
   EXPECT_THAT(findDefinitions(AST, T.point("3")),
-              ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3"))));
+              ElementsAre(RangeInLocationIs(T.range("f")),
+                          RangeInLocationIs(T.range("foo3"))));
   EXPECT_THAT(findDefinitions(AST, T.point("4")),
-              ElementsAre(RangeIs(T.range("g"))));
+              ElementsAre(RangeInLocationIs(T.range("g"))));
   EXPECT_THAT(findDefinitions(AST, T.point("5")),
-              ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3"))));
+              ElementsAre(RangeInLocationIs(T.range("f")),
+                          RangeInLocationIs(T.range("foo3"))));
 
   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"))));
+  EXPECT_THAT(DefinitionAtPoint6,
+              HasSubsequence(RangeInLocationIs(T.range("str")),
+                             RangeInLocationIs(T.range("foo4"))));
+  EXPECT_THAT(DefinitionAtPoint6,
+              HasSubsequence(RangeInLocationIs(T.range("str")),
+                             RangeInLocationIs(T.range("foo3"))));
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
   // The source is in "/clangd-test/src".
   // We build in "/clangd-test/build".
 
   Annotations SourceAnnotations(R"cpp(
 #include "header_in_preamble.h"
)cpp");
   auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
   FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
   auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
   FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
 
   // Go to a definition in main source file.
-  auto Locations =
+  auto DefData =
       runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
-  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
-                                               SourceAnnotations.range()}));
+  EXPECT_TRUE(bool(DefData)) << "findDefinitions returned an error";
+  EXPECT_THAT(*DefData, ElementsAre(LocationIs(Location{
+                            URIForFile{FooCpp}, SourceAnnotations.range()})));
 
   // Go to a definition in header_in_preamble.h.
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
-  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
-              ElementsAre(Location{URIForFile{HeaderInPreambleH},
-                                   HeaderInPreambleAnnotations.range()}));
+  DefData = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(DefData)) << "findDefinitions returned an error";
+  EXPECT_THAT(*DefData, ElementsAre(LocationIs(
+                            Location{URIForFile{HeaderInPreambleH},
+                                     HeaderInPreambleAnnotations.range()})));
 
   // Go to a definition in header_not_in_preamble.h.
-  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
-  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
-              ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
-                                   HeaderNotInPreambleAnnotations.range()}));
+  DefData = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(DefData)) << "findDefinitions returned an error";
+  EXPECT_THAT(*DefData, ElementsAre(LocationIs(
+                            Location{URIForFile{HeaderNotInPreambleH},
+                                     HeaderNotInPreambleAnnotations.range()})));
 }
 
 TEST(Hover, All) {
   struct OneTest {
     StringRef Input;
     StringRef ExpectedHover;
   };
 
TEST(GoToInclude, All) {
 
   Server.addDocument(FooH, HeaderAnnotations.code());
   Server.addDocument(FooCpp, SourceAnnotations.code());
 
   // Test include in preamble.
   auto Locations =
       runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+  EXPECT_THAT(*Locations, ElementsAre(LocationIs(
+                              Location{FooHUri, HeaderAnnotations.range()})));
 
   // Test include in preamble, last char.
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("2"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+  EXPECT_THAT(*Locations, ElementsAre(LocationIs(
+                              Location{FooHUri, HeaderAnnotations.range()})));
 
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+  EXPECT_THAT(*Locations, ElementsAre(LocationIs(
+                              Location{FooHUri, HeaderAnnotations.range()})));
 
   // Test include outside of preamble.
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("6"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+  EXPECT_THAT(*Locations, ElementsAre(LocationIs(
+                              Location{FooHUri, HeaderAnnotations.range()})));
 
   // Test a few positions that do not result in Locations.
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations, IsEmpty());
 
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+  EXPECT_THAT(*Locations, ElementsAre(LocationIs(
+                              Location{FooHUri, HeaderAnnotations.range()})));
 
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+  EXPECT_THAT(*Locations, ElementsAre(LocationIs(
+                              Location{FooHUri, HeaderAnnotations.range()})));
 }
 
 TEST(GoToDefinition, WithPreamble) {
   // Test stragety: AST should always use the latest preamble instead of last
   // good preamble.
   MockFSProvider FS;
   IgnoreDiagnostics DiagConsumer;
   MockCompilationDatabase CDB;
TEST(GoToDefinition, WithPreamble) {
   auto FooHUri = URIForFile{FooH};
   Annotations FooHeader(R"cpp([[]])cpp");
   FS.Files[FooH] = FooHeader.code();
 
   runAddDocument(Server, FooCpp, FooWithHeader.code());
   // GoToDefinition goes to a #include file: the result comes from the preamble.
   EXPECT_THAT(
       cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
-      ElementsAre(Location{FooHUri, FooHeader.range()}));
+      ElementsAre(LocationIs(Location{FooHUri, 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())),
-      ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+      ElementsAre(LocationIs(Location{FooCppUri, FooWithoutHeader.range()})));
 
   // Reset test environment.
   runAddDocument(Server, FooCpp, FooWithHeader.code());
   // Both preamble and AST are built in this request.
   Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
   // Use the AST being built in above request.
   EXPECT_THAT(
       cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
-      ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+      ElementsAre(LocationIs(Location{FooCppUri, FooWithoutHeader.range()})));
 }
 
 TEST(FindReferences, WithinAST) {
   const char *Tests[] = {
       R"cpp(// Local variable
         int main() {
           int [[foo]];
           [[^foo]] = 2;

clangd/textdocument-didchange-fail.test

 # CHECK-NEXT:          "line": 0
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "start": {
 # CHECK-NEXT:          "character": 4,
 # CHECK-NEXT:          "line": 0
 # CHECK-NEXT:        }
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "uri": "file://{{.*}}/clangd-test/main.cpp"
+# CHECK-NEXT:      "usr": "c:@F@main#"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
 {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///main.cpp"},"contentChanges":[{"range":{"start":{"line":100,"character":0},"end":{"line":100,"character":0}},"text": "foo"}]}}
 ---
 {"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":6}}}
 #      CHECK:  "error": {
 # CHECK-NEXT:    "code": -32602,

clangd/xrefs.test

 # CHECK-NEXT:          "character": 5,
 # CHECK-NEXT:          "line": 0
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "start": {
 # CHECK-NEXT:          "character": 4,
 # CHECK-NEXT:          "line": 0
 # CHECK-NEXT:        }
 # CHECK-NEXT:      },
-# CHECK-NEXT:      "uri": "file://{{.*}}/{{([A-Z]:/)?}}main.cpp"
+# CHECK-NEXT:      "uri": "file://{{.*}}/{{([A-Z]:/)?}}main.cpp",
+# CHECK-NEXT:      "usr": "c:@x"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
 {"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":1,"character":8}}}
 #      CHECK: "id": 1
 # CHECK-NEXT: "jsonrpc": "2.0",
 # CHECK-NEXT: "result": [
 # CHECK-NEXT:   {