[clangd] Publish xref for macros from Index and AST.

[usaxena95]

Summary

With this patch the findReferences API will return Xref for macros.
If the symbol under the cursor is a macro then we collect the references to it from:

1. Main file by looking at the ParsedAST. (These were added to the ParsedAST in https://reviews.llvm.org/D70008)
2. Files other than the mainfile by looking at the:
   • static index (Added in https://reviews.llvm.org/D70489)
   • file index (Added in https://reviews.llvm.org/D71406)

This patch collects all the xref from the above places and outputs it in findReferences API.

[merge_guards_bot]

Unit tests: pass. 61306 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

462

could we merge this and the code for decls by only populating Req.IDs with MacroSID here and with the symbol id below and by finally making the query in the end ? i.e.

RefsRequest Req;
Req.Limit = Limit;
if (macro) {
 handleMainFileMacros;
 Req.IDs.insert(*MacroID);
} else {
 handleMainFileDecls;
 populateReqIDs(Req);
}
handleIndexResults(Req);

[kadircet]

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

1021–1022

nit: i don't think there's much benefit in combining refs for macros and symbols in a single test. their handling in the code is disjoint, whereas this test is not. so it makes the test a little bit harder to read and also failures would be harder to reason about. but the test is currently small, so up to you whether you want to separate it or not.

1023

nit: redundant parentheses around R"cpp

[usaxena95]

Addressed comments.

[usaxena95]

clang-tools-extra/clangd/XRefs.cpp

462

Tried to do something on those lines.

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

1021–1022

Added the test for macros as a separate test.

[merge_guards_bot]

Unit tests: pass. 61307 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[kadircet]

thanks, LGTM!

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

1092

i know the comment is copied over, but it doesn't reflect what's going on.

as this check makes sure we prever results from AST rather than index for main file, not the duplicate handling(which actually de-dups references on the *same* location).

since this is handled in the common code path, it is OK to check only in one of the tests(up to you though). but please update the comments.

[usaxena95]

Removed repeated check from the test.

[usaxena95]

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

1092

Updated the old comment to show the correct intent.
Also removed the check from the macros test since this was on the common code path.

[merge_guards_bot]

Unit tests: pass. 61641 tests passed, 0 failed and 777 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

clang-tools-extra/clangd/XRefs.cpp

ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
   ReferencesResult Results;
   const SourceManager &SM = AST.getSourceManager();
   auto MainFilePath =
       getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
   if (!MainFilePath) {
     elog("Failed to get a path for the main file, so no references");
     return Results;
   }
+  auto URIMainFile = URIForFile::canonicalize(*MainFilePath, *MainFilePath);
   auto Loc = SM.getMacroArgExpandedLocation(
       getBeginningOfIdentifier(Pos, SM, AST.getLangOpts()));
-  // TODO: should we handle macros, too?
+  RefsRequest Req;
+
+  if (auto Macro = locateMacroAt(Loc, AST.getPreprocessor())) {
+    // Handle references to macro.
+    if (auto MacroSID = getSymbolID(Macro->Name, Macro->Info, SM)) {
+      // Collect macro references from main file.
+      const auto &IDToRefs = AST.getMacros().MacroRefs;
+      auto Refs = IDToRefs.find(*MacroSID);
+      if (Refs != IDToRefs.end()) {
+        for (const auto Ref : Refs->second) {
+          Location Result;
+          Result.range = Ref;
+          Result.uri = URIMainFile;
+          Results.References.push_back(std::move(Result));
+        }
+      }
+      Req.IDs.insert(*MacroSID);
+    }
+  } else {
+    // Handle references to Decls.
+
     // We also show references to the targets of using-decls, so we include
     // DeclRelation::Underlying.
     DeclRelationSet Relations = DeclRelation::TemplatePattern |
                                 DeclRelation::Alias | DeclRelation::Underlying;
     auto Decls = getDeclAtPosition(AST, Loc, Relations);
 
     // We traverse the AST to find references in the main file.
     auto MainFileRefs = findRefs(Decls, AST);
     // We may get multiple refs with the same location and different Roles, as
     // cross-reference is only interested in locations, we deduplicate them
     // by the location to avoid emitting duplicated locations.
     MainFileRefs.erase(std::unique(MainFileRefs.begin(), MainFileRefs.end(),
                                    [](const ReferenceFinder::Reference &L,
                                       const ReferenceFinder::Reference &R) {
                                      return L.Loc == R.Loc;
                                    }),
                        MainFileRefs.end());
     for (const auto &Ref : MainFileRefs) {
       if (auto Range = getTokenRange(SM, AST.getLangOpts(), Ref.Loc)) {
         Location Result;
         Result.range = *Range;
-      Result.uri = URIForFile::canonicalize(*MainFilePath, *MainFilePath);
+        Result.uri = URIMainFile;
         Results.References.push_back(std::move(Result));
       }
     }
-  // Now query the index for references from other files.
     if (Index && Results.References.size() <= Limit) {
-    RefsRequest Req;
-    Req.Limit = Limit;
-
       for (const Decl *D : Decls) {
-      // Not all symbols can be referenced from outside (e.g. function-locals).
+        // Not all symbols can be referenced from outside (e.g.
+        // function-locals).
         // TODO: we could skip TU-scoped symbols here (e.g. static functions) if
         // we know this file isn't a header. The details might be tricky.
         if (D->getParentFunctionOrMethod())
           continue;
         if (auto ID = getSymbolID(D))
           Req.IDs.insert(*ID);
       }
-    if (Req.IDs.empty())
-      return Results;
+    }
+  }
+  // Now query the index for references from other files.
+  if (!Req.IDs.empty() && Index && Results.References.size() <= Limit) {
+    Req.Limit = Limit;
     Results.HasMore |= Index->refs(Req, [&](const Ref &R) {
-      // no need to continue process if we reach the limit.
+      // No need to continue process if we reach the limit.
       if (Results.References.size() > Limit)
         return;
       auto LSPLoc = toLSPLocation(R.Location, *MainFilePath);
       // Avoid indexed results for the main file - the AST is authoritative.
       if (!LSPLoc || LSPLoc->uri.file() == *MainFilePath)
         return;
 
       Results.References.push_back(std::move(*LSPLoc));

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

const char *Tests[] = {
         class [[Fo^o]] {};
         void test() {
           TYPE([[Foo]]) foo;
           [[FOO]] foo2;
           TYPE(TYPE([[Foo]])) foo3;
           [[CAT]](Fo, o) foo4;
         }
       )cpp",
+
+      R"cpp(// Macros
+        #define [[MA^CRO]](X) (X+1)
+        void test() {
+          int x = [[MACRO]]([[MACRO]](1));
+        }
+      )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = TestTU::withCode(T.code()).build();
     std::vector<Matcher<Location>> ExpectedLocations;
     for (const auto &R : T.ranges())
       ExpectedLocations.push_back(RangeIs(R));
     EXPECT_THAT(findReferences(AST, T.point(), 0).References,
for (const auto &R : T.ranges())
       ExpectedLocations.push_back(RangeIs(R));
     ASSERT_THAT(ExpectedLocations, Not(IsEmpty()));
     EXPECT_THAT(findReferences(AST, T.point(), 0).References,
                 ElementsAreArray(ExpectedLocations))
         << Test;
   }
 }
 
-TEST(FindReferences, NeedsIndex) {
+TEST(FindReferences, NeedsIndexForSymbols) {
   const char *Header = "int foo();";
   Annotations Main("int main() { [[f^oo]](); }");
   TestTU TU;
   TU.Code = Main.code();
   TU.HeaderCode = Header;
   auto AST = TU.build();
 
   // References in main file are returned without index.
   EXPECT_THAT(
       findReferences(AST, Main.point(), 0, /*Index=*/nullptr).References,
TEST(FindReferences, NeedsIndexForSymbols) {
   EXPECT_THAT(
       findReferences(AST, Main.point(), 0, IndexedTU.index().get()).References,
       ElementsAre(RangeIs(Main.range()), RangeIs(IndexedMain.range())));
   auto LimitRefs =
       findReferences(AST, Main.point(), /*Limit*/ 1, IndexedTU.index().get());
   EXPECT_EQ(1u, LimitRefs.References.size());
   EXPECT_TRUE(LimitRefs.HasMore);
 
-  // If the main file is in the index, we don't return duplicates.
-  // (even if the references are in a different location)
+  // Avoid indexed results for the main file. Use AST for the mainfile.
   TU.Code = ("\n\n" + Main.code()).str();
   EXPECT_THAT(findReferences(AST, Main.point(), 0, TU.index().get()).References,
               ElementsAre(RangeIs(Main.range())));
 }
 
+TEST(FindReferences, NeedsIndexForMacro) {
+  const char *Header = "#define MACRO(X) (X+1)";
+  Annotations Main(R"cpp(
+    int main() {
+      int a = [[MA^CRO]](1);
+    }
+  )cpp");
+  TestTU TU;
+  TU.Code = Main.code();
+  TU.HeaderCode = Header;
+  auto AST = TU.build();
+
+  // References in main file are returned without index.
+  EXPECT_THAT(
+      findReferences(AST, Main.point(), 0, /*Index=*/nullptr).References,
+      ElementsAre(RangeIs(Main.range())));
+
+  Annotations IndexedMain(R"cpp(
+    int indexed_main() {
+      int a = [[MACRO]](1);
+    }
+  )cpp");
+
+  // References from indexed files are included.
+  TestTU IndexedTU;
+  IndexedTU.Code = IndexedMain.code();
+  IndexedTU.Filename = "Indexed.cpp";
+  IndexedTU.HeaderCode = Header;
+  EXPECT_THAT(
+      findReferences(AST, Main.point(), 0, IndexedTU.index().get()).References,
+      ElementsAre(RangeIs(Main.range()), RangeIs(IndexedMain.range())));
+  auto LimitRefs =
+      findReferences(AST, Main.point(), /*Limit*/ 1, IndexedTU.index().get());
+  EXPECT_EQ(1u, LimitRefs.References.size());
+  EXPECT_TRUE(LimitRefs.HasMore);
+}
+
 TEST(FindReferences, NoQueryForLocalSymbols) {
   struct RecordingIndex : public MemIndex {
     mutable Optional<llvm::DenseSet<SymbolID>> RefIDs;
     bool refs(const RefsRequest &Req,
               llvm::function_ref<void(const Ref &)>) const override {
       RefIDs = Req.IDs;
       return false;
     }