[Clangd] Index main-file macros (bug 39761)

[nridge]

Rebase

[nridge]

@hokein, @sammccall, this is a follow-up to https://reviews.llvm.org/D55185. Would one (or both) of you be intereested in reviewing it?

[sammccall]

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

383

why do we check IsMainFileSymbol here?

[hokein]

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

416

I think we need to set VisibleOutsideFile flag.

[nridge]

Address review comments

[nridge]

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

383

Good point, it doesn't seem to be necessary.

416

Good catch! Fixed and amended tests.

[hokein]

Just realize one missing point: we should respect the CollectMainFileSymbols option, otherwise looks good.

[nridge]

Address more review comments

[nridge]

Don't include unrelated file

[nridge]

In D55739#1369188, @hokein wrote:

Just realize one missing point: we should respect the CollectMainFileSymbols option, otherwise looks good.

Good catch once again -- fixed.

[hokein]

Measured this patch on LLVM, the increasing number of symbols is reasonable (from 422 K to 425 K, with CollectMacro enabled).

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

bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
                                            index::SymbolRoleSet Roles,
                                            SourceLocation Loc) {
   if (!Opts.CollectMacro)
     return true;
   assert(PP.get());
 
   const auto &SM = PP->getSourceManager();
   auto DefLoc = MI->getDefinitionLoc();
-  if (SM.isInMainFile(SM.getExpansionLoc(DefLoc)))
-    return true;
+  
   // Header guards are not interesting in index. Builtin macros don't have
   // useful locations and are not needed for code completions.
   if (MI->isUsedForHeaderGuard() || MI->isBuiltinMacro())
     return true;
 
+  // Skip main-file symbols if we are not collecting them.
+  bool IsMainFileSymbol = SM.isInMainFile(SM.getExpansionLoc(DefLoc));
+  if (IsMainFileSymbol && !Opts.CollectMainFileSymbols)
+    return false;
+
+  // Also avoid storing predefined macros like __DBL_MIN__.
+  if (SM.isWrittenInBuiltinFile(DefLoc))
+    return true;
+
   // Mark the macro as referenced if this is a reference coming from the main
   // file. The macro may not be an interesting symbol, but it's cheaper to check
   // at the end.
   if (Opts.CountReferences &&
       (Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
       SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
     ReferencedMacros.insert(Name);
   // Don't continue indexing if this is a mere reference.
   // FIXME: remove macro with ID if it is undefined.
   if (!(Roles & static_cast<unsigned>(index::SymbolRole::Declaration) ||
         Roles & static_cast<unsigned>(index::SymbolRole::Definition)))
     return true;
 
   auto ID = getSymbolID(*Name, MI, SM);
   if (!ID)
     return true;
 
   // Only collect one instance in case there are multiple.
   if (Symbols.find(*ID) != nullptr)
     return true;
 
   Symbol S;
   S.ID = std::move(*ID);
   S.Name = Name->getName();
+  if (!IsMainFileSymbol) {
     S.Flags |= Symbol::IndexedForCodeCompletion;
+    S.Flags |= Symbol::VisibleOutsideFile;
+  }
   S.SymInfo = index::getSymbolInfoForMacro(*MI);
   std::string FileURI;
   // FIXME: use the result to filter out symbols.
   shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
   if (auto DeclLoc =
           getTokenLocation(DefLoc, SM, Opts, PP->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 

clang-tools-extra/unittests/clangd/CMakeLists.txt

target_link_libraries(ClangdTests
   clangBasic
   clangDaemon
   clangFormat
   clangFrontend
   clangIndex
   clangLex
   clangSema
   clangSerialization
+  clangTidy
   clangTooling
   clangToolingCore
   clangToolingInclusions
   LLVMSupport
   LLVMTestingSupport
   )
 
 if (CLANGD_BUILD_XPC)
   add_subdirectory(xpc)
 endif ()

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

void addFile(llvm::StringRef FileName, llvm::StringRef Contents) {
     auto Path = testPath(FileName);
     FSProvider.Files[Path] = Contents;
     Server.addDocument(Path, Contents);
   }
 };
 
 } // namespace
 
-TEST_F(WorkspaceSymbolsTest, NoMacro) {
+TEST_F(WorkspaceSymbolsTest, Macros) {
   addFile("foo.cpp", R"cpp(
       #define MACRO X
       )cpp");
 
-  // Macros are not in the index.
-  EXPECT_THAT(getSymbols("macro"), IsEmpty());
+  // LSP's SymbolKind doesn't have a "Macro" kind, and
+  // indexSymbolKindToSymbolKind() currently maps macros
+  // to SymbolKind::String.
+  EXPECT_THAT(getSymbols("macro"),
+              ElementsAre(AllOf(QName("MACRO"), WithKind(SymbolKind::String))));
 }
 
 TEST_F(WorkspaceSymbolsTest, NoLocals) {
   addFile("foo.cpp", R"cpp(
       void test(int FirstParam, int SecondParam) {
         struct LocalClass {};
         int local_var;
       })cpp");

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

TEST_F(SymbolCollectorTest, CollectMacros) {
   CollectorOpts.CollectIncludePath = true;
   Annotations Header(R"(
     #define X 1
     #define $mac[[MAC]](x) int x
     #define $used[[USED]](y) float y;
 
     MAC(p);
   )");
-  const std::string Main = R"(
-    #define MAIN 1  // not indexed
+  Annotations Main(R"(
+    #define $main[[MAIN]] 1
     USED(t);
-  )";
+  )");
   CollectorOpts.CountReferences = true;
   CollectorOpts.CollectMacro = true;
-  runSymbolCollector(Header.code(), Main);
+  runSymbolCollector(Header.code(), Main.code());
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(QName("p"), QName("t"),
                                    AllOf(QName("X"), DeclURI(TestHeaderURI),
                                          IncludeHeader(TestHeaderURI)),
                                    AllOf(Labeled("MAC(x)"), RefCount(0),
-                                         DeclRange(Header.range("mac"))),
+                                         DeclRange(Header.range("mac")),
+                                         VisibleOutsideFile()),
                                    AllOf(Labeled("USED(y)"), RefCount(1),
-                                         DeclRange(Header.range("used")))));
+                                         DeclRange(Header.range("used")),
+                                         VisibleOutsideFile()),
+                                   AllOf(Labeled("MAIN"), RefCount(0),
+                                         DeclRange(Main.range("main")),
+                                         Not(VisibleOutsideFile()))));
 }
 
 TEST_F(SymbolCollectorTest, DeprecatedSymbols) {
   const std::string Header = R"(
     void TestClangc() __attribute__((deprecated("", "")));
     void TestClangd();
   )";
   runSymbolCollector(Header, /**/ "");