[clangd] Fix Origin and MainFileOnly-ness for macros

[kadircet]

Summary

This was resulting in macros coming from preambles vanishing when user
have opened the source header. For example:

// test.h:

and

// test.cc
#include "test.h"
^

If user only opens test.cc, we'll get X as a completion candidate,
since it is indexed as part of the preamble. But if the user opens
test.h afterwards we would index it as part of the main file and lose
the symbol (as new index shard for test.h will override the existing one
in dynamic index).

Also we were not setting origins for macros correctly, this patch also
fixes it.

Fixes https://github.com/clangd/clangd/issues/461

[hokein]

thanks!

the test.h in the patch description is missing a #define X.

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

383

nit: move this var to Line 398, in some cases (builtin macros), it is not used, so would save some cost.

this is duplicated with the one in handleDeclOccurrence, creating a new function seems not worthy...

[kadircet]

• Move declarations closer to use.

[kadircet]

the test.h in the patch description is missing a #define X.

ah right, git descriptions omitting lines starting with # fixed it for include, but missed the define :face_palm:

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

383

yeah and these are slightly different, the one in declOccurence makes sure the decl is written inside the main file, whereas this one also accepts remappings through #line directives.

not sure if it is necessary, but it was the old behavior, so I am just keeping it, no tests seem to break and we already do not index builtin macros or the ones spelled in a builtin-file. I don't think line directives are widely used within the rest, but again such a change would deserve a separate patch.

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

bool SymbolCollector::handleMacroOccurrence(const IdentifierInfo *Name,
                                             const MacroInfo *MI,
                                             index::SymbolRoleSet Roles,
                                             SourceLocation Loc) {
   assert(PP.get());
 
   const auto &SM = PP->getSourceManager();
   auto DefLoc = MI->getDefinitionLoc();
   auto SpellingLoc = SM.getSpellingLoc(Loc);
-  bool IsMainFileSymbol = SM.isInMainFile(SM.getExpansionLoc(DefLoc));
+  bool IsMainFileOnly =
+      SM.isInMainFile(SM.getExpansionLoc(DefLoc)) &&
+      !isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
+                    ASTCtx->getLangOpts());
 
   // Builtin macros don't have useful locations and aren't needed in completion.
   if (MI->isBuiltinMacro())
     return true;
 
   // Also avoid storing predefined macros like __DBL_MIN__.
   if (SM.isWrittenInBuiltinFile(DefLoc))
     return true;
 
   auto ID = getSymbolID(Name->getName(), MI, SM);
   if (!ID)
     return true;
 
   // Do not store references to main-file macros.
-  if ((static_cast<unsigned>(Opts.RefFilter) & Roles) && !IsMainFileSymbol &&
+  if ((static_cast<unsigned>(Opts.RefFilter) & Roles) && !IsMainFileOnly &&
       (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
     MacroRefs[*ID].push_back({Loc, Roles});
 
   // Collect symbols.
   if (!Opts.CollectMacro)
     return true;
 
   // Skip main-file macros if we are not collecting them.
-  if (IsMainFileSymbol && !Opts.CollectMainFileSymbols)
+  if (IsMainFileOnly && !Opts.CollectMainFileSymbols)
     return false;
 
   // 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(SpellingLoc) == 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;
 
   // 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) {
+  if (!IsMainFileOnly) {
     S.Flags |= Symbol::IndexedForCodeCompletion;
     S.Flags |= Symbol::VisibleOutsideFile;
   }
   S.SymInfo = index::getSymbolInfoForMacro(*MI);
+  S.Origin = Opts.Origin;
   std::string FileURI;
   // FIXME: use the result to filter out symbols.
   shouldIndexFile(SM.getFileID(Loc));
   if (auto DeclLoc =
           getTokenLocation(DefLoc, SM, Opts, PP->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 
   CodeCompletionResult SymbolCompletion(Name);

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

EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), RefCount(1)),
                                             AllOf(QName("C"), RefCount(0))));
 }
 
 TEST_F(SymbolCollectorTest, Origin) {
   CollectorOpts.Origin = SymbolOrigin::Static;
   runSymbolCollector("class Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            Field(&Symbol::Origin, SymbolOrigin::Static)));
+  runSymbolCollector("#define FOO", /*Main=*/"");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
+                           Field(&Symbol::Origin, SymbolOrigin::Static)));
 }
 
 TEST_F(SymbolCollectorTest, CollectMacros) {
   CollectorOpts.CollectIncludePath = true;
   Annotations Header(R"(
     #pragma once
     #define X 1
     #define $mac[[MAC]](x) int x
TEST_F(SymbolCollectorTest, BadUTF8) {
   CollectorOpts.RefsInHeaders = true;
   runSymbolCollector(Header, "");
   EXPECT_THAT(Symbols, Contains(QName("types")));
   EXPECT_THAT(Symbols, Contains(QName("PUNCT")));
   // Reference is stored, although offset within line is not reliable.
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "PUNCT").ID, _)));
 }
 
+TEST_F(SymbolCollectorTest, MacrosInHeaders) {
+  CollectorOpts.CollectMacro = true;
+  TestFileName = testPath("test.h");
+  runSymbolCollector("", "#define X");
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(AllOf(QName("X"), ForCodeCompletion(true))));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang