[clangd] Add xref for macros to FileIndex.

[usaxena95]

Summary

Adds macro references to the dynamic index (FileIndex).
Tests added.

[merge_guards_bot]

Unit tests: pass. 60496 tests passed, 0 failed and 726 were skipped.

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

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

[ilya-biryukov]

clang-tools-extra/clangd/URI.h

115 ↗

(On Diff #233561)

This function does a very specialized form of path-to-uri conversion (fallback dirs, traversing schemes, etc.)
I'm afraid that naming it toURI will mean everyone will find and use it without giving it too much thought.

Another concern is layering. URI.h currently does not depend on SourceManager and, arguably, it should stay that way. We define an interface to extend the URI schemes that clangd supports, there's no reason for it to depend on SourceManager.

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

88

This is trying to emulate existing logic in SymbolCollector::finish. Is there a way we could share this?
Would avoid creating extra copies of reference slabs and allow to keep the code in one place, rather than scattering it between FileIndex.cpp and SymbolCollector.cpp. Would also allow to keep toURI private, meaning we don't have to worry about naming it and the fact it's exposed in the public interface.

One potential way to do this is to have an alternative version of handleMacroOccurence, which would fill SymbolCollector::MacroRefs directly and call this right after indexTopLevelDecls.
Would that work?

[hokein]

@ilya-biryukov thanks for taking it during my OOO, just a drive-by comment :)

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

88

+1 on the concern about performance (this is a hot function, we are paying an extra cost of copying all refs, which should be avoided), and the layering of toURI.

One potential way to do this is to have an alternative version of handleMacroOccurence, which would fill SymbolCollector::MacroRefs directly and call this right after indexTopLevelDecls.
Would that work?

the main problem is that at this point (parsing is finished), preprocessor callback is not available, so we won't see macro references (SymbolCollector::handleMacroOccurence will only receive macro definitions).

I think two options:

• as mentioned above, add alternative version of handleMacroOccurence in SymbolCollector, calling it before indexTopLevelDecls (because finish is called in indexTopLevelDecls which we have no way to customize);
• or we could add a finish callback to the SymbolCollector which is called in SymbolCollector::finish, and put this logic to the callback.

[ilya-biryukov]

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

88

Thanks, good point, in my proposal we should populate macro references before calling indexTopLevelDecls.

I would still suggest going with the first option, it seems simpler to me. But up to you, @usaxena95.

[usaxena95]

Added another version of handleMacroOccurence to handle MacroReferences from main file.

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

88

This looks much better now. Thanks for your inputs Haojian and Ilya.
I have added another version of handleMacroOccurence to add the MainFileMacros to the references.

add alternative version of handleMacroOccurence in SymbolCollector, calling it before indexTopLevelDecls

@hokein
SymbolCollector doesn't build the slabs in the finish so I think it is still fine to call it after indexTopLevelDecls. But doing this before finish makes more sense semantically and reasonable.

[merge_guards_bot]

Unit tests: pass. 60496 tests passed, 0 failed and 726 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

[ilya-biryukov]

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

350

Does this ever happen?
Maybe assert it's not null instead?

362

Won't we get a dangling pointer inside Refs after leaving this function?
MainFileURI gets deallocated at the end, right?

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

108

NIT: could you name it in plural? handleMacros or handleMacroOccurences?

It's different from the other version in this regard, which adds only a single occurrence.
It's also a good practice to avoid adding overloads to virtual/overriden methods, might get confusing at times.

[usaxena95]

Addressed comments.

[usaxena95]

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

362

Yeah I also thought the same when I saw this at other places too. But the Refs.insert(IDToRefs.first, R); actually does a copy of this string and owns the copy.

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

108

Renamed to handleMacros to avoid confusion with the virtual method.

[ilya-biryukov]

LGTM

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

362

Ah, makes sense. Thanks for explaining this.

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

368

Why do we need two tests? To test the file URI?
Setting a custom filename ("test.cc" in our case) should be enough for this.

I suggest removing Test2 completely, it does not seem to test anything new and creates noise. Unless there are things I'm missing that it test.

[merge_guards_bot]

Unit tests: pass. 60496 tests passed, 0 failed and 726 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

[usaxena95]

Removed extra tests.

[merge_guards_bot]

Unit tests: pass. 60496 tests passed, 0 failed and 726 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

[usaxena95]

Added tests.

[merge_guards_bot]

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

clang-tidy: fail. Please fix clang-tidy findings.

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/index/FileIndex.h

 
 using SlabTuple = std::tuple<SymbolSlab, RefSlab, RelationSlab>;
 
 /// Retrieves symbols and refs of local top level decls in \p AST (i.e.
 /// `AST.getLocalTopLevelDecls()`).
 /// Exposed to assist in unit tests.
 SlabTuple indexMainDecls(ParsedAST &AST);
 
-/// Idex declarations from \p AST and macros from \p PP that are declared in
+/// Index declarations from \p AST and macros from \p PP that are declared in
 /// included headers.
 SlabTuple indexHeaderSymbols(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
                              const CanonicalIncludes &Includes);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_FILEINDEX_H

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

 //===--- FileIndex.cpp - Indexes for files. ------------------------ C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "FileIndex.h"
+#include "CollectMacros.h"
 #include "Logger.h"
 #include "ParsedAST.h"
 #include "SymbolCollector.h"
 #include "index/CanonicalIncludes.h"
 #include "index/Index.h"
 #include "index/MemIndex.h"
 #include "index/Merge.h"
 #include "index/SymbolOrigin.h"

 #include "llvm/ADT/StringRef.h"
 #include <memory>
 
 namespace clang {
 namespace clangd {
 
 static SlabTuple indexSymbols(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
                               llvm::ArrayRef<Decl *> DeclsToIndex,
+                              const MainFileMacros *MacroRefsToIndex,
                               const CanonicalIncludes &Includes,
                               bool IsIndexMainAST) {
   SymbolCollector::Options CollectorOpts;
   CollectorOpts.CollectIncludePath = true;
   CollectorOpts.Includes = &Includes;
   CollectorOpts.CountReferences = false;
   CollectorOpts.Origin = SymbolOrigin::Dynamic;
 
static SlabTuple indexSymbols(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
   } else {
     IndexOpts.IndexMacrosInPreprocessor = true;
     CollectorOpts.CollectMacro = true;
     CollectorOpts.StoreAllDocumentation = true;
   }
 
   SymbolCollector Collector(std::move(CollectorOpts));
   Collector.setPreprocessor(PP);
+  if (MacroRefsToIndex)
+    Collector.handleMacros(*MacroRefsToIndex);
   index::indexTopLevelDecls(AST, *PP, DeclsToIndex, Collector, IndexOpts);
 
   const auto &SM = AST.getSourceManager();
   const auto *MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
   std::string FileName = MainFileEntry ? MainFileEntry->getName() : "";
 
   auto Syms = Collector.takeSymbols();
   auto Refs = Collector.takeRefs();
   auto Relations = Collector.takeRelations();
+
   vlog("index AST for {0} (main={1}): \n"
        "  symbol slab: {2} symbols, {3} bytes\n"
        "  ref slab: {4} symbols, {5} refs, {6} bytes\n"
        "  relations slab: {7} relations, {8} bytes",
        FileName, IsIndexMainAST, Syms.size(), Syms.bytes(), Refs.size(),
        Refs.numRefs(), Refs.bytes(), Relations.size(), Relations.bytes());
   return std::make_tuple(std::move(Syms), std::move(Refs),
                          std::move(Relations));
 }
 
 SlabTuple indexMainDecls(ParsedAST &AST) {
   return indexSymbols(AST.getASTContext(), AST.getPreprocessorPtr(),
-                      AST.getLocalTopLevelDecls(), AST.getCanonicalIncludes(),
+                      AST.getLocalTopLevelDecls(), &AST.getMacros(),
+                      AST.getCanonicalIncludes(),
                       /*IsIndexMainAST=*/true);
 }
 
 SlabTuple indexHeaderSymbols(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
                              const CanonicalIncludes &Includes) {
   std::vector<Decl *> DeclsToIndex(
       AST.getTranslationUnitDecl()->decls().begin(),
       AST.getTranslationUnitDecl()->decls().end());
-  return indexSymbols(AST, std::move(PP), DeclsToIndex, Includes,
+  return indexSymbols(AST, std::move(PP), DeclsToIndex,
+                      /*MainFileMacros=*/nullptr, Includes,
                       /*IsIndexMainAST=*/false);
 }
 
 void FileSymbols::update(PathRef Path, std::unique_ptr<SymbolSlab> Symbols,
                          std::unique_ptr<RefSlab> Refs,
                          std::unique_ptr<RelationSlab> Relations,
                          bool CountReferences) {
   std::lock_guard<std::mutex> Lock(Mutex);

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

 //===--- SymbolCollector.h ---------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_COLLECTOR_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_COLLECTOR_H
 
 #include "CanonicalIncludes.h"
+#include "CollectMacros.h"
 #include "Index.h"
 #include "SymbolOrigin.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexSymbol.h"
public:
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
                       ArrayRef<index::SymbolRelation> Relations,
                       SourceLocation Loc,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override;
 
+  void handleMacros(const MainFileMacros &MacroRefsToIndex);
+
   bool handleMacroOccurence(const IdentifierInfo *Name, const MacroInfo *MI,
                             index::SymbolRoleSet Roles,
                             SourceLocation Loc) override;
 
   SymbolSlab takeSymbols() { return std::move(Symbols).build(); }
   RefSlab takeRefs() { return std::move(Refs).build(); }
   RelationSlab takeRelations() { return std::move(Relations).build(); }
 

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

else if (isPreferredDeclaration(*OriginalDecl, Roles))
     BasicSymbol = addDeclaration(*OriginalDecl, std::move(*ID), IsMainFileOnly);
 
   if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
     addDefinition(*OriginalDecl, *BasicSymbol);
 
   return true;
 }
 
+void SymbolCollector::handleMacros(const MainFileMacros &MacroRefsToIndex) {
+  assert(PP.get());
+  const auto &SM = PP->getSourceManager();
+  const auto *MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+  assert(MainFileEntry);
+
+  const auto MainFileURI = toURI(SM, MainFileEntry->getName(), Opts);
+  // Add macro references.
+  for (const auto &IDToRefs : MacroRefsToIndex.MacroRefs) {
+    for (const auto &Range : IDToRefs.second) {
+      Ref R;
+      R.Location.Start.setLine(Range.start.line);
+      R.Location.Start.setColumn(Range.start.character);
+      R.Location.End.setLine(Range.end.line);
+      R.Location.End.setColumn(Range.end.character);
+      R.Location.FileURI = MainFileURI.c_str();
+      // FIXME: Add correct RefKind information to MainFileMacros.
+      R.Kind = RefKind::Reference;
+      Refs.insert(IDToRefs.first, R);
+    }
+  }
+}
+
 bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
                                            const MacroInfo *MI,
                                            index::SymbolRoleSet Roles,
                                            SourceLocation Loc) {
   assert(PP.get());
 
   const auto &SM = PP->getSourceManager();
   auto DefLoc = MI->getDefinitionLoc();

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

TEST(FileIndexTest, Refs) {
 
   EXPECT_THAT(getRefs(Index, Foo.ID),
               RefsAre({AllOf(RefRange(MainCode.range("foo")),
                              FileURI("unittest:///test.cc")),
                        AllOf(RefRange(MainCode.range("foo")),
                              FileURI("unittest:///test2.cc"))}));
 }
 
+TEST(FileIndexTest, MacroRefs) {
+  Annotations HeaderCode(R"cpp(
+    #define $macro[[MACRO]](X) (X+1)
+  )cpp");
+  Annotations MainCode(R"cpp(
+  void f() {
+    int a = $macro[[MACRO]](1);
+  }
+  )cpp");
+
+  auto Macro = findSymbol(
+      TestTU::withHeaderCode(HeaderCode.code()).headerSymbols(), "MACRO");
+  FileIndex Index;
+  // Add test.cc
+  TestTU Test;
+  Test.HeaderCode = HeaderCode.code();
+  Test.Code = MainCode.code();
+  Test.Filename = "test.cc";
+  auto AST = Test.build();
+  Index.updateMain(Test.Filename, AST);
+  EXPECT_THAT(getRefs(Index, Macro.ID),
+              RefsAre({AllOf(RefRange(MainCode.range("macro")),
+                             FileURI("unittest:///test.cc"))}));
+}
+
 TEST(FileIndexTest, CollectMacros) {
   FileIndex M;
   update(M, "f", "#define CLANGD 1");
   EXPECT_THAT(runFuzzyFind(M, ""), Contains(QName("CLANGD")));
 }
 
 TEST(FileIndexTest, Relations) {
   TestTU TU;