[clangd] SymbolLocation only covers symbol name.

[hokein]

Summary

• Change the offset range to half-open, [start, end).
• Fix a few fixmes.

[sammccall]

Nice!

clangd/index/Index.cpp

40

Was this just for debugging? I had this change locally when working on this code, but reverted it.
It's not applicable in lots of places we'd like to debug with a symbol, so I'd lean towards reverting it (and if we really need a verbose version, dumping the YAML)

clangd/index/Index.h

125

This fragment doesn't really make sense. (Semicolon might be better, but really it should be another sentence).

Can we drop "the location of" so this fits on a line?

clangd/index/Index.h

 //===--- Symbol.h -----------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
 
 #include "clang/Index/IndexSymbol.h"
+#include "clang/Lex/Lexer.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include <array>
 #include <string>
 
 namespace clang {
 namespace clangd {
 
 struct SymbolLocation {
   // The URI of the source file where a symbol occurs.
   llvm::StringRef FileURI;
-  // The 0-based offset to the first character of the symbol from the beginning
-  // of the source file.
+  // The 0-based offsets of the symbol from the beginning of the source file,
+  // using half-open range, [StartOffset, EndOffset).
   unsigned StartOffset = 0;
-  // The 0-based offset to the last character of the symbol from the beginning
-  // of the source file.
   unsigned EndOffset = 0;
 
   operator bool() const { return !FileURI.empty(); }
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolLocation &);
 
 // The class identifies a particular C++ symbol (class, function, method, etc).
 //
struct Symbol {
   SymbolID ID;
   // The symbol information, like symbol kind.
   index::SymbolInfo SymInfo;
   // The unqualified name of the symbol, e.g. "bar" (for ns::bar).
   llvm::StringRef Name;
   // The containing namespace. e.g. "" (global), "ns::" (top-level namespace).
   llvm::StringRef Scope;
   // The location of the symbol's definition, if one was found.
-  // This covers the whole definition (e.g. class body).
+  // This just covers the symbol name (e.g. without class/function body).
   SymbolLocation Definition;
-  // The location of the preferred declaration of the symbol.
+  // The location of the preferred declaration of the symbol, just covers the
+  // symbol name.
   // This may be the same as Definition.
   //
   // A C++ symbol may have multiple declarations, and we pick one to prefer.
   //   * For classes, the canonical declaration should be the definition.
   //   * For non-inline functions, the canonical declaration typically appears
   //     in the ".h" file corresponding to the definition.
   SymbolLocation CanonicalDeclaration;
 

clangd/index/Index.cpp

 
 namespace clang {
 namespace clangd {
 using namespace llvm;
 
 raw_ostream &operator<<(raw_ostream &OS, const SymbolLocation &L) {
   if (!L)
     return OS << "(none)";
-  return OS << L.FileURI << "[" << L.StartOffset << "-" << L.EndOffset << "]";
+  return OS << L.FileURI << "[" << L.StartOffset << "-" << L.EndOffset << ")";
 }
 
 SymbolID::SymbolID(StringRef USR)
     : HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {}
 
 raw_ostream &operator<<(raw_ostream &OS, const SymbolID &ID) {
   OS << toHex(toStringRef(ID.HashValue));
   return OS;
 }
 
 void operator>>(StringRef Str, SymbolID &ID) {
   std::string HexString = fromHex(Str);
   assert(HexString.size() == ID.HashValue.size());
   std::copy(HexString.begin(), HexString.end(), ID.HashValue.begin());
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const Symbol &S) {
-  return OS << S.Scope << S.Name;
+  return OS << S.Scope << S.Name << " " << S.CanonicalDeclaration << ", "
+            << S.Definition << "\n";
 }
 
 SymbolSlab::const_iterator SymbolSlab::find(const SymbolID &ID) const {
   auto It = std::lower_bound(Symbols.begin(), Symbols.end(), ID,
                              [](const Symbol &S, const SymbolID &I) {
                                return S.ID < I;
                              });
   if (It != Symbols.end() && It->ID == ID)

clangd/index/SymbolCollector.cpp

bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,
   return false;
 }
 
 // Return the symbol location of the given declaration `D`.
 //
 // For symbols defined inside macros:
 //   * use expansion location, if the symbol is formed via macro concatenation.
 //   * use spelling location, otherwise.
-//
-// FIXME: EndOffset is inclusive (closed range), and should be exclusive.
-// FIXME: Because the underlying ranges are token ranges, this code chops the
-//        last token in half if it contains multiple characters.
-// FIXME: We probably want to get just the location of the symbol name, not
-//        the whole e.g. class.
 llvm::Optional<SymbolLocation>
 getSymbolLocation(const NamedDecl &D, SourceManager &SM,
                   const SymbolCollector::Options &Opts,
+                  const clang::LangOptions& LangOpts,
                   std::string &FileURIStorage) {
-  SourceLocation Loc = D.getLocation();
-  SourceLocation StartLoc = SM.getSpellingLoc(D.getLocStart());
-  SourceLocation EndLoc = SM.getSpellingLoc(D.getLocEnd());
-  if (Loc.isMacroID()) {
-    std::string PrintLoc = SM.getSpellingLoc(Loc).printToString(SM);
+  SourceLocation SpellingLoc = SM.getSpellingLoc(D.getLocation());
+  if (D.getLocation().isMacroID()) {
+    std::string PrintLoc = SpellingLoc.printToString(SM);
     if (llvm::StringRef(PrintLoc).startswith("<scratch") ||
         llvm::StringRef(PrintLoc).startswith("<command line>")) {
       // We use the expansion location for the following symbols, as spelling
       // locations of these symbols are not interesting to us:
       //   * symbols formed via macro concatenation, the spelling location will
       //     be "<scratch space>"
       //   * symbols controlled and defined by a compile command-line option
       //     `-DName=foo`, the spelling location will be "<command line>".
-      StartLoc = SM.getExpansionRange(D.getLocStart()).first;
-      EndLoc = SM.getExpansionRange(D.getLocEnd()).second;
+      SpellingLoc = SM.getExpansionRange(D.getLocation()).first;
     }
   }
 
-  auto U = toURI(SM, SM.getFilename(StartLoc), Opts);
+  auto U = toURI(SM, SM.getFilename(SpellingLoc), Opts);
   if (!U)
     return llvm::None;
   FileURIStorage = std::move(*U);
   SymbolLocation Result;
   Result.FileURI = FileURIStorage;
-  Result.StartOffset = SM.getFileOffset(StartLoc);
-  Result.EndOffset = SM.getFileOffset(EndLoc);
+  Result.StartOffset = SM.getFileOffset(SpellingLoc);
+  Result.EndOffset = Result.StartOffset + clang::Lexer::MeasureTokenLength(
+                                              SpellingLoc, SM, LangOpts);
   return std::move(Result);
 }
 
 } // namespace
 
 SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
 
 void SymbolCollector::initialize(ASTContext &Ctx) {
const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
 
   Symbol S;
   S.ID = std::move(ID);
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
   S.SymInfo = index::getSymbolInfo(&ND);
   std::string FileURI;
   // FIXME: we may want a different "canonical" heuristic than clang chooses.
   // Clang seems to choose the first, which may not have the most information.
-  if (auto DeclLoc = getSymbolLocation(ND, SM, Opts, FileURI))
+  if (auto DeclLoc =
+          getSymbolLocation(ND, SM, Opts, ASTCtx->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 
   // Add completion info.
   // FIXME: we may want to choose a different redecl, or combine from several.
   assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
   CodeCompletionResult SymbolCompletion(&ND, 0);
   const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
       *ASTCtx, *PP, CodeCompletionContext::CCC_Name, *CompletionAllocator,
void SymbolCollector::addDefinition(const NamedDecl &ND,
   if (DeclSym.Definition)
     return;
   // If we saw some forward declaration, we end up copying the symbol.
   // This is not ideal, but avoids duplicating the "is this a definition" check
   // in clang::index. We should only see one definition.
   Symbol S = DeclSym;
   std::string FileURI;
   if (auto DefLoc = getSymbolLocation(ND, ND.getASTContext().getSourceManager(),
-                                      Opts, FileURI))
+                                      Opts, ASTCtx->getLangOpts(), FileURI))
     S.Definition = *DefLoc;
   Symbols.insert(S);
 }
 
 } // namespace clangd
 } // namespace clang

unittests/clangd/SymbolCollectorTests.cpp

 MATCHER_P(Doc, D, "") { return arg.Detail && arg.Detail->Documentation == D; }
 MATCHER_P(Plain, Text, "") { return arg.CompletionPlainInsertText == Text; }
 MATCHER_P(Snippet, S, "") {
   return arg.CompletionSnippetInsertText == S;
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
 MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
 MATCHER_P(DeclRange, Offsets, "") {
-  // Offset range in SymbolLocation is [start, end] while in Clangd is [start,
-  // end).
-  // FIXME: SymbolLocation should be [start, end).
   return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
-      arg.CanonicalDeclaration.EndOffset == Offsets.second - 1;
+      arg.CanonicalDeclaration.EndOffset == Offsets.second;
 }
 MATCHER_P(DefRange, Offsets, "") {
   return arg.Definition.StartOffset == Offsets.first &&
-         arg.Definition.EndOffset == Offsets.second - 1;
+         arg.Definition.EndOffset == Offsets.second;
 }
 
 namespace clang {
 namespace clangd {
 
 namespace {
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
EXPECT_THAT(Symbols,
               UnorderedElementsAreArray(
                   {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
                    QName("kStr"), QName("foo"), QName("foo::bar"),
                    QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
                    QName("foo::bar::v2"), QName("foo::baz")}));
 }
 
 TEST_F(SymbolCollectorTest, Locations) {
-  // FIXME: the behavior here is bad: chopping tokens, including more than the
-  // ident range, using half-open ranges. See fixmes in getSymbolLocation().
   CollectorOpts.IndexMainFiles = true;
   Annotations Header(R"cpp(
     // Declared in header, defined in main.
-    $xdecl[[extern int X]];
-    $clsdecl[[class C]]ls;
-    $printdecl[[void print()]];
+    extern int $xdecl[[X]];
+    class $clsdecl[[Cls]];
+    void $printdecl[[print]]();
 
     // Declared in header, defined nowhere.
-    $zdecl[[extern int Z]];
+    extern int $zdecl[[Z]];
   )cpp");
   Annotations Main(R"cpp(
-    $xdef[[int X = 4]]2;
-    $clsdef[[class Cls {}]];
-    $printdef[[void print() {}]]
+    int $xdef[[X]] = 42;
+    class $clsdef[[Cls]] {};
+    void $printdef[[print]]() {}
 
     // Declared/defined in main only.
-    $y[[int Y]];
+    int $y[[Y]];
   )cpp");
   runSymbolCollector(Header.code(), Main.code());
   EXPECT_THAT(
       Symbols,
       UnorderedElementsAre(
           AllOf(QName("X"), DeclRange(Header.offsetRange("xdecl")),
                 DefRange(Main.offsetRange("xdef"))),
           AllOf(QName("Cls"), DeclRange(Header.offsetRange("clsdecl")),

 
 TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) {
   CollectorOpts.IndexMainFiles = false;
 
   Annotations Header(R"(
     #define FF(name) \
       class name##_Test {};
 
-    $expansion[[FF(abc)]];
+    $expansion[[FF]](abc);
 
     #define FF2() \
-      $spelling[[class Test {}]];
+      class $spelling[[Test]] {};
 
     FF2();
   )");
 
   runSymbolCollector(Header.code(), /*Main=*/"");
   EXPECT_THAT(
       Symbols,
       UnorderedElementsAre(
           AllOf(QName("abc_Test"), DeclRange(Header.offsetRange("expansion")),
                 DeclURI(TestHeaderURI)),
           AllOf(QName("Test"), DeclRange(Header.offsetRange("spelling")),
                 DeclURI(TestHeaderURI))));
 }
 
 TEST_F(SymbolCollectorTest, SymbolFormedFromMacroInMainFile) {
   CollectorOpts.IndexMainFiles = true;
 
   Annotations Main(R"(
     #define FF(name) \
       class name##_Test {};
 
-    $expansion[[FF(abc)]];
+    $expansion[[FF]](abc);
 
     #define FF2() \
-      $spelling[[class Test {}]];
+      class $spelling[[Test]] {};
 
     FF2();
   )");
   runSymbolCollector(/*Header=*/"", Main.code());
   EXPECT_THAT(
       Symbols,
       UnorderedElementsAre(
           AllOf(QName("abc_Test"), DeclRange(Main.offsetRange("expansion")),
                 DeclURI(TestFileURI)),
           AllOf(QName("Test"), DeclRange(Main.offsetRange("spelling")),
                 DeclURI(TestFileURI))));
 }
 
 TEST_F(SymbolCollectorTest, SymbolFormedByCLI) {
   CollectorOpts.IndexMainFiles = false;
 
   Annotations Header(R"(
     #ifdef NAME
-    $expansion[[class NAME {}]];
+    class $expansion[[NAME]] {};
     #endif
   )");
 
   runSymbolCollector(Header.code(), /*Main=*/"",
                      /*ExtraArgs=*/{"-DNAME=name"});
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(AllOf(
                   QName("name"), DeclRange(Header.offsetRange("expansion")),