[clangd] Better handling symbols defined in macros.

Summary:
For symbols defined inside macros:
 * use expansion location, if the symbol is formed via macro concatenation.
 * use spelling location, otherwise.

This will fix some symbols that have ill-format location (especial invalid filepath).

Reviewers: ioeric

Reviewed By: ioeric

Subscribers: klimek, ilya-biryukov, jkorous-apple, cfe-commits

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

llvm-svn: 323867

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

@@ -112,6 +112,37 @@ 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.
+SymbolLocation GetSymbolLocation(const NamedDecl *D, SourceManager &SM,
+                                 StringRef FallbackDir,
+                                 std::string &FilePathStorage) {
+  SymbolLocation Location;
+
+  SourceLocation Loc = SM.getSpellingLoc(D->getLocation());
+  if (D->getLocation().isMacroID()) {
+    // The symbol is formed via macro concatenation, the spelling location will
+    // be "<scratch space>", which is not interesting to us, use the expansion
+    // location instead.
+    if (llvm::StringRef(Loc.printToString(SM)).startswith("<scratch")) {
+      FilePathStorage = makeAbsolutePath(
+          SM, SM.getFilename(SM.getExpansionLoc(D->getLocation())),
+          FallbackDir);
+      return {FilePathStorage,
+              SM.getFileOffset(SM.getExpansionRange(D->getLocStart()).first),
+              SM.getFileOffset(SM.getExpansionRange(D->getLocEnd()).second)};
+    }
+  }
+
+  FilePathStorage = makeAbsolutePath(SM, SM.getFilename(Loc), FallbackDir);
+  return {FilePathStorage,
+          SM.getFileOffset(SM.getSpellingLoc(D->getLocStart())),
+          SM.getFileOffset(SM.getSpellingLoc(D->getLocEnd()))};
+}
+
 } // namespace
 
 SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
@@ -149,17 +180,14 @@ bool SymbolCollector::handleDeclOccurence(
       return true;
 
     auto &SM = ND->getASTContext().getSourceManager();
-    std::string FilePath = makeAbsolutePath(
-        SM, SM.getFilename(D->getLocation()), Opts.FallbackDir);
-    SymbolLocation Location = {FilePath, SM.getFileOffset(D->getLocStart()),
-                               SM.getFileOffset(D->getLocEnd())};
     std::string QName = ND->getQualifiedNameAsString();
 
     Symbol S;
     S.ID = std::move(ID);
     std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
     S.SymInfo = index::getSymbolInfo(D);
-    S.CanonicalDeclaration = Location;
+    std::string FilePath;
+    S.CanonicalDeclaration = GetSymbolLocation(ND, SM, Opts.FallbackDir, FilePath);
 
     // Add completion info.
     assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");

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

@@ -83,5 +83,11 @@ std::vector<Range> Annotations::ranges(llvm::StringRef Name) const {
   return {R.begin(), R.end()};
 }
 
+std::pair<std::size_t, std::size_t>
+Annotations::offsetRange(llvm::StringRef Name) const {
+  auto R = range(Name);
+  return {positionToOffset(Code, R.start), positionToOffset(Code, R.end)};
+}
+
 } // namespace clangd
 } // namespace clang

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

@@ -58,6 +58,10 @@ public:
   // Returns the location of all ranges marked by [[ ]] (or $name[[ ]]).
   std::vector<Range> ranges(llvm::StringRef Name = "") const;
 
+  // The same to `range` method, but returns range in offsets [start, end).
+  std::pair<std::size_t, std::size_t>
+  offsetRange(llvm::StringRef Name = "") const;
+
 private:
   std::string Code;
   llvm::StringMap<llvm::SmallVector<Position, 1>> Points;

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

@@ -7,6 +7,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "Annotations.h"
 #include "TestFS.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolYAML.h"
@@ -46,11 +47,22 @@ MATCHER_P(Snippet, S, "") {
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
 MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(LocationOffsets, Offsets, "") {
+  // Offset range in SymbolLocation is [start, end] while in Clangd is [start,
+  // end).
+  return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
+      arg.CanonicalDeclaration.EndOffset == Offsets.second - 1;
+}
+//MATCHER_P(FilePath, P, "") {
+  //return arg.CanonicalDeclaration.FilePath.contains(P);
+//}
 
 namespace clang {
 namespace clangd {
 
 namespace {
+const char TestHeaderName[] = "symbols.h";
+const char TestFileName[] = "symbol.cc";
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
   SymbolIndexActionFactory(SymbolCollector::Options COpts)
@@ -79,21 +91,20 @@ public:
     llvm::IntrusiveRefCntPtr<FileManager> Files(
         new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
-    const std::string FileName = "symbol.cc";
-    const std::string HeaderName = "symbols.h";
     auto Factory = llvm::make_unique<SymbolIndexActionFactory>(CollectorOpts);
 
     tooling::ToolInvocation Invocation(
-        {"symbol_collector", "-fsyntax-only", "-std=c++11", FileName},
+        {"symbol_collector", "-fsyntax-only", "-std=c++11", TestFileName},
         Factory->create(), Files.get(),
         std::make_shared<PCHContainerOperations>());
 
-    InMemoryFileSystem->addFile(HeaderName, 0,
+    InMemoryFileSystem->addFile(TestHeaderName, 0,
                                 llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
-    std::string Content = "#include\"" + std::string(HeaderName) + "\"";
-    Content += "\n" + MainCode.str();
-    InMemoryFileSystem->addFile(FileName, 0,
+    std::string Content = MainCode;
+    if (!HeaderCode.empty())
+      Content = "#include\"" + std::string(TestHeaderName) + "\"\n" + Content;
+    InMemoryFileSystem->addFile(TestFileName, 0,
                                 llvm::MemoryBuffer::getMemBuffer(Content));
     Invocation.run();
     Symbols = Factory->Collector->takeSymbols();
@@ -206,6 +217,57 @@ TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) {
               UnorderedElementsAre(QName("Foo")));
 }
 
+TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) {
+  CollectorOpts.IndexMainFiles = false;
+
+  Annotations Header(R"(
+    #define FF(name) \
+      class name##_Test {};
+
+    $expansion[[FF(abc)]];
+
+    #define FF2() \
+      $spelling[[class Test {}]];
+
+    FF2();
+  )");
+
+  runSymbolCollector(Header.code(), /*Main=*/"");
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(
+                  AllOf(QName("abc_Test"),
+                        LocationOffsets(Header.offsetRange("expansion")),
+                        CPath(TestHeaderName)),
+                  AllOf(QName("Test"),
+                        LocationOffsets(Header.offsetRange("spelling")),
+                        CPath(TestHeaderName))));
+}
+
+TEST_F(SymbolCollectorTest, SymbolFormedFromMacroInMainFile) {
+  CollectorOpts.IndexMainFiles = true;
+
+  Annotations Main(R"(
+    #define FF(name) \
+      class name##_Test {};
+
+    $expansion[[FF(abc)]];
+
+    #define FF2() \
+      $spelling[[class Test {}]];
+
+    FF2();
+  )");
+  runSymbolCollector(/*Header=*/"", Main.code());
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(
+                  AllOf(QName("abc_Test"),
+                        LocationOffsets(Main.offsetRange("expansion")),
+                        CPath(TestFileName)),
+                  AllOf(QName("Test"),
+                        LocationOffsets(Main.offsetRange("spelling")),
+                        CPath(TestFileName))));
+}
+
 TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) {
   CollectorOpts.IndexMainFiles = false;
   const std::string Header = R"(