[clangd] Better handling symbols defined in macros.

[hokein]

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).

[ioeric]

clangd/index/SymbolCollector.cpp

108 ↗

(On Diff #131578)

PrintLoc? PrintableLoc sounds like a checker for whether a location is printable.

110 ↗

(On Diff #131578)

This works for the use case below (i.e. to get around "<scratch" prefix check) but might cause problem when it's used else where. Please clearly document the behavior, or I'd probably make this a lambda inside GetSymbolLocation.

118 ↗

(On Diff #131578)

Pull the comments in the patch summary here?

127 ↗

(On Diff #131578)

To reduce some code, consider

Loc, Start, End = spelling locs;
if (D->getLocation().isMacroID() && llvm::StringRef(PrintableLoc(Loc, SM)).startswith("<scratch")) {
  Loc, Start, End = expension locs;
}
// Generate SymbolLocation with Loc, Start, End

Also, I think you could use getLocStart in place of getLocation?

unittests/clangd/Annotations.h

63 ↗

(On Diff #131578)

This doesn't seem to be necessary? I think you could use the range method above and convert it to offsets in the test matcher.

unittests/clangd/SymbolCollectorTests.cpp

216–219 ↗

(On Diff #131578)

Consider adding another expansion and spelling in the main file (with IndexMainFiles enabled), so that we could also check file paths are handled correctly?

[hokein]

• address review comments.
• add tests for indexing main files
• use SourceLocation.printToString

[hokein]

Update test.

clangd/index/SymbolCollector.cpp

108 ↗

(On Diff #131578)

SourceLocation has provided printToString API already, we don't need to reinvent the wheel now.

127 ↗

(On Diff #131578)

There is no much duplicated code among the cases here (only one line of generating the SymbolLocation). I prefer to the current way -- it shows the logic a bit clearer, and also saves the cost of getSpellingLoc for the macro case.

unittests/clangd/Annotations.h

63 ↗

(On Diff #131578)

I think this is a useful method (probably be used in the future). Putting it to test matcher, we have to pass Code parameter to the matcher (LocationOffset(range, Header.code())) everywhere. Annotation seems to be the best place.

[ioeric]

lg

clangd/index/SymbolCollector.cpp

127 ↗

(On Diff #131578)

I still think there is value in reducing the duplicates. For example, when the interface of makeAbsolutePath or SymbolLocation changes, we would only need to modify one place rather than two.

I wouldn't worry about the cost of getSpellingLoc for the macro case since it is a cheap operation for a rare case anyway...

unittests/clangd/Annotations.h

63 ↗

(On Diff #131578)

I see. Didn't realize code is needed.

unittests/clangd/SymbolCollectorTests.cpp

240 ↗

(On Diff #132123)

Maybe put this in the header file to make the test more meaningful?

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

if (match(decl(allOf(Opts.IndexMainFiles
                                                      unless(isScoped())))))),
             *ND, *ASTCtx)
           .empty())
     return true;
 
   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)) {}
 
 void SymbolCollector::initialize(ASTContext &Ctx) {
   ASTCtx = &Ctx;
   CompletionAllocator = std::make_shared<GlobalCodeCompletionAllocator>();
   CompletionTUInfo =
if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
     if (index::generateUSRForDecl(ND, USR))
       return true;
 
     auto ID = SymbolID(USR);
     if (Symbols.find(ID) != nullptr)
       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.");
     CodeCompletionResult SymbolCompletion(ND, 0);
     const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
         *ASTCtx, *PP, CodeCompletionContext::CCC_Name, *CompletionAllocator,
         *CompletionTUInfo,
         /*IncludeBriefComments*/ true);

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

public:
   std::vector<Position> points(llvm::StringRef Name = "") const;
 
   // Returns the location of the range marked by [[ ]] (or $name[[ ]]).
   // Crashes if there isn't exactly one.
   Range range(llvm::StringRef Name = "") const;
   // 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;
   llvm::StringMap<llvm::SmallVector<Range, 1>> Ranges;
 };
 
 } // namespace clangd
 } // namespace clang
 #endif

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

require(I != Ranges.end() && I->getValue().size() == 1,
           "expected exactly one range", Code);
   return I->getValue()[0];
 }
 std::vector<Range> Annotations::ranges(llvm::StringRef Name) const {
   auto R = Ranges.lookup(Name);
   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/trunk/unittests/clangd/SymbolCollectorTests.cpp

 //===-- SymbolCollectorTests.cpp  -------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "Annotations.h"
 #include "TestFS.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolYAML.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexingAction.h"

 }
 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(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)
       : COpts(std::move(COpts)) {}
 
   clang::FrontendAction *create() override {
     index::IndexingOptions IndexOpts;
     IndexOpts.SystemSymbolFilter =

 class SymbolCollectorTest : public ::testing::Test {
 public:
   bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode) {
     llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> InMemoryFileSystem(
         new vfs::InMemoryFileSystem);
     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();
     return true;
   }
 
 protected:
   SymbolSlab Symbols;
struct {
       int a;
     } Foo;
   )";
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols,
               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"(
     class Foo {};
     void f1();
     inline void f2() {}
   )";
   const std::string Main = R"(