[clangd] Store full decl/def range with symbol locations

Apart from fixing the linked issue, this is also necessary
for supporting LSP's LocationLink feature and for finding
proper insertion locations in the DefineOutline tweak.
Memory consumption of the background index grows by about ~2.5%.

Closes clangd/clangd#59

Author: ckandeler

clang-tools-extra/clangd/CodeComplete.cpp

@@ -418,7 +418,7 @@ struct CodeCompletionBuilder {
     auto Inserted = [&](llvm::StringRef Header)
         -> llvm::Expected<std::pair<std::string, bool>> {
       auto ResolvedDeclaring =
-          URI::resolve(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
+          URI::resolve(C.IndexResult->CanonicalDeclaration.fileURI(), FileName);
       if (!ResolvedDeclaring)
         return ResolvedDeclaring.takeError();
       auto ResolvedInserted = toHeaderFile(Header, FileName);
@@ -451,7 +451,7 @@ struct CodeCompletionBuilder {
       } else
         log("Failed to generate include insertion edits for adding header "
             "(FileURI='{0}', IncludeHeader='{1}') into {2}: {3}",
-            C.IndexResult->CanonicalDeclaration.FileURI, Inc.Header, FileName,
+            C.IndexResult->CanonicalDeclaration.fileURI(), Inc.Header, FileName,
             ToInclude.takeError());
     }
     // Prefer includes that do not need edits (i.e. already exist).

clang-tools-extra/clangd/FindSymbols.cpp

@@ -54,27 +54,41 @@ bool approximateScopeMatch(llvm::StringRef Scope, llvm::StringRef Query) {
   return Query.empty();
 }
 
+Range indexToLSPRange(const SymbolPosition &SrcStart,
+                      const SymbolPosition &SrcEnd) {
+  Position Start, End;
+  Start.line = SrcStart.line();
+  Start.character = SrcStart.column();
+  End.line = SrcEnd.line();
+  End.character = SrcEnd.column();
+  return {Start, End};
+}
+
 } // namespace
 
-llvm::Expected<Location> indexToLSPLocation(const SymbolLocation &Loc,
+llvm::Expected<Location> indexToLSPLocation(const SymbolNameLocation &Loc,
                                             llvm::StringRef TUPath) {
   auto Path = URI::resolve(Loc.FileURI, TUPath);
   if (!Path)
     return error("Could not resolve path for file '{0}': {1}", Loc.FileURI,
                  Path.takeError());
   Location L;
   L.uri = URIForFile::canonicalize(*Path, TUPath);
-  Position Start, End;
-  Start.line = Loc.Start.line();
-  Start.character = Loc.Start.column();
-  End.line = Loc.End.line();
-  End.character = Loc.End.column();
-  L.range = {Start, End};
+  L.range = indexToLSPRange(Loc.Start, Loc.End);
   return L;
 }
 
-llvm::Expected<Location> symbolToLocation(const Symbol &Sym,
-                                          llvm::StringRef TUPath) {
+llvm::Expected<std::pair<Location, Range>>
+indexToLSPLocation(const SymbolDeclDefLocation &Loc, StringRef TUPath) {
+  auto L = indexToLSPLocation(Loc.NameLocation, TUPath);
+  if (!L)
+    return L.takeError();
+  return std::make_pair(L.get(),
+                        indexToLSPRange(Loc.DeclDefStart, Loc.DeclDefEnd));
+}
+
+llvm::Expected<std::pair<Location, Range>>
+symbolToLocation(const Symbol &Sym, llvm::StringRef TUPath) {
   // Prefer the definition over e.g. a function declaration in a header
   return indexToLSPLocation(
       Sym.Definition ? Sym.Definition : Sym.CanonicalDeclaration, TUPath);
@@ -152,7 +166,7 @@ getWorkspaceSymbols(llvm::StringRef Query, int Limit,
     SymbolInformation Info;
     Info.name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
     Info.kind = indexSymbolKindToSymbolKind(Sym.SymInfo.Kind);
-    Info.location = *Loc;
+    Info.location = Loc->first;
     Scope.consume_back("::");
     Info.containerName = Scope.str();
 

clang-tools-extra/clangd/FindSymbols.h

@@ -22,12 +22,14 @@ class ParsedAST;
 class SymbolIndex;
 
 /// Helper function for deriving an LSP Location from an index SymbolLocation.
-llvm::Expected<Location> indexToLSPLocation(const SymbolLocation &Loc,
+llvm::Expected<Location> indexToLSPLocation(const SymbolNameLocation &Loc,
                                             llvm::StringRef TUPath);
+llvm::Expected<std::pair<Location, Range>>
+indexToLSPLocation(const SymbolDeclDefLocation &Loc, llvm::StringRef TUPath);
 
 /// Helper function for deriving an LSP Location for a Symbol.
-llvm::Expected<Location> symbolToLocation(const Symbol &Sym,
-                                          llvm::StringRef TUPath);
+llvm::Expected<std::pair<Location, Range>>
+symbolToLocation(const Symbol &Sym, llvm::StringRef TUPath);
 
 /// Searches for the symbols matching \p Query. The syntax of \p Query can be
 /// the non-qualified name or fully qualified of a symbol. For example,

clang-tools-extra/clangd/HeaderSourceSwitch.cpp

@@ -95,9 +95,9 @@ std::optional<Path> getCorrespondingHeaderOrSource(PathRef OriginalFile,
   bool IsHeader = isHeaderFile(OriginalFile, AST.getLangOpts());
   Index->lookup(Request, [&](const Symbol &Sym) {
     if (IsHeader)
-      AwardTarget(Sym.Definition.FileURI);
+      AwardTarget(Sym.Definition.fileURI());
     else
-      AwardTarget(Sym.CanonicalDeclaration.FileURI);
+      AwardTarget(Sym.CanonicalDeclaration.fileURI());
   });
   // FIXME: our index doesn't have any interesting information (this could be
   // that the background-index is not finished), we should use the decl/def

clang-tools-extra/clangd/IncludeFixer.cpp

@@ -289,7 +289,7 @@ std::vector<Fix> IncludeFixer::fixIncompleteType(const Type &T) const {
   if (!Syms.empty()) {
     auto &Matched = *Syms.begin();
     if (!Matched.IncludeHeaders.empty() && Matched.Definition &&
-        Matched.CanonicalDeclaration.FileURI == Matched.Definition.FileURI)
+        Matched.CanonicalDeclaration.fileURI() == Matched.Definition.fileURI())
       Fixes = fixesForSymbols(Syms);
   }
   return Fixes;
@@ -299,7 +299,7 @@ std::vector<Fix> IncludeFixer::fixesForSymbols(const SymbolSlab &Syms) const {
   auto Inserted = [&](const Symbol &Sym, llvm::StringRef Header)
       -> llvm::Expected<std::pair<std::string, bool>> {
     auto ResolvedDeclaring =
-        URI::resolve(Sym.CanonicalDeclaration.FileURI, File);
+        URI::resolve(Sym.CanonicalDeclaration.fileURI(), File);
     if (!ResolvedDeclaring)
       return ResolvedDeclaring.takeError();
     auto ResolvedInserted = toHeaderFile(Header, File);
@@ -616,7 +616,7 @@ IncludeFixer::lookupCached(const SymbolID &ID) const {
   if (!Syms.empty()) {
     auto &Matched = *Syms.begin();
     if (!Matched.IncludeHeaders.empty() && Matched.Definition &&
-        Matched.CanonicalDeclaration.FileURI == Matched.Definition.FileURI)
+        Matched.CanonicalDeclaration.fileURI() == Matched.Definition.fileURI())
       Fixes = fixesForSymbols(Syms);
   }
   auto E = LookupCache.try_emplace(ID, std::move(Syms));

clang-tools-extra/clangd/Quality.cpp

@@ -281,7 +281,7 @@ computeScope(const NamedDecl *D) {
 }
 
 void SymbolRelevanceSignals::merge(const Symbol &IndexResult) {
-  SymbolURI = IndexResult.CanonicalDeclaration.FileURI;
+  SymbolURI = IndexResult.CanonicalDeclaration.fileURI();
   SymbolScope = IndexResult.Scope;
   IsInstanceMember |= isInstanceMember(IndexResult.SymInfo);
   if (!(IndexResult.Flags & Symbol::VisibleOutsideFile)) {

clang-tools-extra/clangd/XRefs.cpp

@@ -114,7 +114,7 @@ const NamedDecl *getDefinition(const NamedDecl *D) {
   return nullptr; // except cases above
 }
 
-void logIfOverflow(const SymbolLocation &Loc) {
+void logIfOverflow(const SymbolNameLocation &Loc) {
   if (Loc.Start.hasOverflow() || Loc.End.hasOverflow())
     log("Possible overflow in symbol location: {0}", Loc);
 }
@@ -123,7 +123,7 @@ void logIfOverflow(const SymbolLocation &Loc) {
 // TUPath is used to resolve the path of URI.
 // FIXME: figure out a good home for it, and share the implementation with
 // FindSymbols.
-std::optional<Location> toLSPLocation(const SymbolLocation &Loc,
+std::optional<Location> toLSPLocation(const SymbolNameLocation &Loc,
                                       llvm::StringRef TUPath) {
   if (!Loc)
     return std::nullopt;
@@ -148,8 +148,9 @@ std::optional<Location> toLSPLocation(const SymbolLocation &Loc,
   return LSPLoc;
 }
 
-SymbolLocation toIndexLocation(const Location &Loc, std::string &URIStorage) {
-  SymbolLocation SymLoc;
+SymbolNameLocation toIndexLocation(const Location &Loc,
+                                   std::string &URIStorage) {
+  SymbolNameLocation SymLoc;
   URIStorage = Loc.uri.uri();
   SymLoc.FileURI = URIStorage.c_str();
   SymLoc.Start.setLine(Loc.range.start.line);
@@ -160,17 +161,17 @@ SymbolLocation toIndexLocation(const Location &Loc, std::string &URIStorage) {
 }
 
 // Returns the preferred location between an AST location and an index location.
-SymbolLocation getPreferredLocation(const Location &ASTLoc,
-                                    const SymbolLocation &IdxLoc,
-                                    std::string &Scratch) {
+SymbolNameLocation getPreferredLocation(const Location &ASTLoc,
+                                        const SymbolNameLocation &IdxLoc,
+                                        std::string &Scratch) {
   // Also use a mock symbol for the index location so that other fields (e.g.
   // definition) are not factored into the preference.
   Symbol ASTSym, IdxSym;
   ASTSym.ID = IdxSym.ID = SymbolID("mock_symbol_id");
-  ASTSym.CanonicalDeclaration = toIndexLocation(ASTLoc, Scratch);
-  IdxSym.CanonicalDeclaration = IdxLoc;
+  ASTSym.CanonicalDeclaration.NameLocation = toIndexLocation(ASTLoc, Scratch);
+  IdxSym.CanonicalDeclaration.NameLocation = IdxLoc;
   auto Merged = mergeSymbol(ASTSym, IdxSym);
-  return Merged.CanonicalDeclaration;
+  return Merged.CanonicalDeclaration.NameLocation;
 }
 
 std::vector<std::pair<const NamedDecl *, DeclRelationSet>>
@@ -323,16 +324,17 @@ std::vector<LocatedSymbol> findImplementors(llvm::DenseSet<SymbolID> IDs,
   Req.Subjects = std::move(IDs);
   std::vector<LocatedSymbol> Results;
   Index->relations(Req, [&](const SymbolID &Subject, const Symbol &Object) {
-    auto DeclLoc =
-        indexToLSPLocation(Object.CanonicalDeclaration, MainFilePath);
+    auto DeclLoc = indexToLSPLocation(Object.CanonicalDeclaration.NameLocation,
+                                      MainFilePath);
     if (!DeclLoc) {
       elog("Find overrides: {0}", DeclLoc.takeError());
       return;
     }
     Results.emplace_back();
     Results.back().Name = Object.Name.str();
     Results.back().PreferredDeclaration = *DeclLoc;
-    auto DefLoc = indexToLSPLocation(Object.Definition, MainFilePath);
+    auto DefLoc =
+        indexToLSPLocation(Object.Definition.NameLocation, MainFilePath);
     if (!DefLoc) {
       elog("Failed to convert location: {0}", DefLoc.takeError());
       return;
@@ -364,23 +366,26 @@ void enhanceLocatedSymbolsFromIndex(llvm::MutableArrayRef<LocatedSymbol> Result,
     if (R.Definition) { // from AST
       // Special case: if the AST yielded a definition, then it may not be
       // the right *declaration*. Prefer the one from the index.
-      if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, MainFilePath))
+      if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration.NameLocation,
+                                   MainFilePath))
         R.PreferredDeclaration = *Loc;
 
       // We might still prefer the definition from the index, e.g. for
       // generated symbols.
       if (auto Loc = toLSPLocation(
-              getPreferredLocation(*R.Definition, Sym.Definition, Scratch),
+              getPreferredLocation(*R.Definition, Sym.Definition.NameLocation,
+                                   Scratch),
               MainFilePath))
         R.Definition = *Loc;
     } else {
-      R.Definition = toLSPLocation(Sym.Definition, MainFilePath);
+      R.Definition = toLSPLocation(Sym.Definition.NameLocation, MainFilePath);
 
       // Use merge logic to choose AST or index declaration.
-      if (auto Loc = toLSPLocation(
-              getPreferredLocation(R.PreferredDeclaration,
-                                   Sym.CanonicalDeclaration, Scratch),
-              MainFilePath))
+      if (auto Loc =
+              toLSPLocation(getPreferredLocation(
+                                R.PreferredDeclaration,
+                                Sym.CanonicalDeclaration.NameLocation, Scratch),
+                            MainFilePath))
         R.PreferredDeclaration = *Loc;
     }
   });
@@ -606,7 +611,7 @@ std::vector<LocatedSymbol> locateSymbolTextually(const SpelledWord &Word,
       return;
 
     auto MaybeDeclLoc =
-        indexToLSPLocation(Sym.CanonicalDeclaration, MainFilePath);
+        indexToLSPLocation(Sym.CanonicalDeclaration.NameLocation, MainFilePath);
     if (!MaybeDeclLoc) {
       log("locateSymbolNamedTextuallyAt: {0}", MaybeDeclLoc.takeError());
       return;
@@ -616,7 +621,8 @@ std::vector<LocatedSymbol> locateSymbolTextually(const SpelledWord &Word,
     Located.Name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
     Located.ID = Sym.ID;
     if (Sym.Definition) {
-      auto MaybeDefLoc = indexToLSPLocation(Sym.Definition, MainFilePath);
+      auto MaybeDefLoc =
+          indexToLSPLocation(Sym.Definition.NameLocation, MainFilePath);
       if (!MaybeDefLoc) {
         log("locateSymbolNamedTextuallyAt: {0}", MaybeDefLoc.takeError());
         return;
@@ -1495,9 +1501,10 @@ ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
           Results.HasMore = true;
           return;
         }
-        const auto LSPLocDecl =
-            toLSPLocation(Object.CanonicalDeclaration, MainFilePath);
-        const auto LSPLocDef = toLSPLocation(Object.Definition, MainFilePath);
+        const auto LSPLocDecl = toLSPLocation(
+            Object.CanonicalDeclaration.NameLocation, MainFilePath);
+        const auto LSPLocDef =
+            toLSPLocation(Object.Definition.NameLocation, MainFilePath);
         if (LSPLocDecl && LSPLocDecl != LSPLocDef) {
           ReferencesResult::Reference Result;
           Result.Loc = {std::move(*LSPLocDecl), std::nullopt};
@@ -1754,11 +1761,9 @@ static std::optional<HierarchyItem> symbolToHierarchyItem(const Symbol &S,
   HierarchyItem HI;
   HI.name = std::string(S.Name);
   HI.kind = indexSymbolKindToSymbolKind(S.SymInfo.Kind);
-  HI.selectionRange = Loc->range;
-  // FIXME: Populate 'range' correctly
-  // (https://github.com/clangd/clangd/issues/59).
-  HI.range = HI.selectionRange;
-  HI.uri = Loc->uri;
+  HI.selectionRange = Loc->first.range;
+  HI.range = Loc->second;
+  HI.uri = Loc->first.uri;
 
   return HI;
 }

clang-tools-extra/clangd/index/FileIndex.cpp

@@ -132,13 +132,13 @@ FileShardedIndex::FileShardedIndex(IndexFileIn Input)
   // Attribute each Symbol to both their declaration and definition locations.
   if (Index.Symbols) {
     for (const auto &S : *Index.Symbols) {
-      auto It = Shards.try_emplace(S.CanonicalDeclaration.FileURI);
+      auto It = Shards.try_emplace(S.CanonicalDeclaration.fileURI());
       It.first->getValue().Symbols.insert(&S);
       SymbolIDToFile[S.ID] = &It.first->getValue();
       // Only bother if definition file is different than declaration file.
       if (S.Definition &&
-          S.Definition.FileURI != S.CanonicalDeclaration.FileURI) {
-        auto It = Shards.try_emplace(S.Definition.FileURI);
+          S.Definition.fileURI() != S.CanonicalDeclaration.fileURI()) {
+        auto It = Shards.try_emplace(S.Definition.fileURI());
         It.first->getValue().Symbols.insert(&S);
       }
     }

clang-tools-extra/clangd/index/Merge.cpp

@@ -26,7 +26,7 @@ bool isIndexAuthoritative(const SymbolIndex::IndexedFiles &Index,
   // We expect the definition to see the canonical declaration, so it seems to
   // be enough to check only the definition if it exists.
   const char *OwningFile =
-      S.Definition ? S.Definition.FileURI : S.CanonicalDeclaration.FileURI;
+      S.Definition ? S.Definition.fileURI() : S.CanonicalDeclaration.fileURI();
   return (Index(OwningFile) & IndexContents::Symbols) != IndexContents::None;
 }
 } // namespace
@@ -189,15 +189,16 @@ void MergedIndex::relations(
 
 // Returns true if \p L is (strictly) preferred to \p R (e.g. by file paths). If
 // neither is preferred, this returns false.
-static bool prefer(const SymbolLocation &L, const SymbolLocation &R) {
+static bool prefer(const SymbolDeclDefLocation &L,
+                   const SymbolDeclDefLocation &R) {
   if (!L)
     return false;
   if (!R)
     return true;
-  auto HasCodeGenSuffix = [](const SymbolLocation &Loc) {
+  auto HasCodeGenSuffix = [](const SymbolDeclDefLocation &Loc) {
     constexpr static const char *CodegenSuffixes[] = {".proto"};
     return llvm::any_of(CodegenSuffixes, [&](llvm::StringRef Suffix) {
-      return llvm::StringRef(Loc.FileURI).ends_with(Suffix);
+      return llvm::StringRef(Loc.fileURI()).ends_with(Suffix);
     });
   };
   return HasCodeGenSuffix(L) && !HasCodeGenSuffix(R);
@@ -211,9 +212,9 @@ Symbol mergeSymbol(const Symbol &L, const Symbol &R) {
   bool PreferR = R.Definition && !L.Definition;
   // Merge include headers only if both have definitions or both have no
   // definition; otherwise, only accumulate references of common includes.
-  assert(L.Definition.FileURI && R.Definition.FileURI);
+  assert(L.Definition.fileURI() && R.Definition.fileURI());
   bool MergeIncludes =
-      bool(*L.Definition.FileURI) == bool(*R.Definition.FileURI);
+      bool(*L.Definition.fileURI()) == bool(*R.Definition.fileURI());
   Symbol S = PreferR ? R : L;        // The target symbol we're merging into.
   const Symbol &O = PreferR ? L : R; // The "other" less-preferred symbol.
 

clang-tools-extra/clangd/index/Ref.h

@@ -18,6 +18,7 @@
 #include <cstdint>
 #include <set>
 #include <utility>
+#include <variant>
 
 namespace clang {
 namespace clangd {
@@ -84,7 +85,7 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &, RefKind);
 /// WARNING: Location does not own the underlying data - Copies are shallow.
 struct Ref {
   /// The source location where the symbol is named.
-  SymbolLocation Location;
+  SymbolNameLocation Location;
   RefKind Kind = RefKind::Unknown;
   /// The ID of the symbol whose definition contains this reference.
   /// For example, for a reference inside a function body, this would
@@ -182,12 +183,8 @@ template <> struct DenseMapInfo<clang::clangd::RefSlab::Builder::Entry> {
         Val.Reference.Location.Start.rep(), Val.Reference.Location.End.rep());
   }
   static bool isEqual(const Entry &LHS, const Entry &RHS) {
-    return std::tie(LHS.Symbol, LHS.Reference.Location.FileURI,
-                    LHS.Reference.Kind) ==
-               std::tie(RHS.Symbol, RHS.Reference.Location.FileURI,
-                        RHS.Reference.Kind) &&
-           LHS.Reference.Location.Start == RHS.Reference.Location.Start &&
-           LHS.Reference.Location.End == RHS.Reference.Location.End;
+    return std::tie(LHS.Symbol, LHS.Reference) ==
+           std::tie(RHS.Symbol, RHS.Reference);
   }
 };
 } // namespace llvm