[clangd] Add an option to exclude symbols outside of project root from index

[kbobyrev]

Summary

This is useful for the remote index: system libraries and external dependencies
would still have a different location on local and remote machine, so they
should be simply excluded.

[hokein]

clang-tools-extra/clangd/indexer/IndexerMain.cpp

110

I think we could reuse the FileShardedIndex to do the filtering job, rather than implementing a new one.

[kbobyrev]

clang-tools-extra/clangd/indexer/IndexerMain.cpp

110

Ah, this is interesting, I haven't seen FileShardedIndex before, let me take a look at its API, it looks like a convenient piece of infrastructure for implementing this!

[kbobyrev]

Switch to SymbolCollector::Options::FileFilter that is even more
straightforward.

[hokein]

clang-tools-extra/clangd/indexer/IndexerMain.cpp

70

FileFilter seems more promising, but there are some FIXMEs ("use the result to filter out symbols.") in the SymbolCollector.cpp` -- looks like the SymbolCollector doesn't support this yet, sigh...

[kbobyrev]

clang-tools-extra/clangd/indexer/IndexerMain.cpp

70

Hmm, you are right. However, I tried running the indxer with and without new option and the number of symbols & references was different. Hmm, this is interesting, I wonder why this happened.

Yeah, I think I should update the code to make sure everything works. Thanks for noticing it!

[kbobyrev]

Need tot resolve FIXMEs around shouldIndexFile().

[kbobyrev]

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

[hokein]

In D84811#2199510, @kbobyrev wrote:

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

[kbobyrev]

In D84811#2199820, @hokein wrote:

In D84811#2199510, @kbobyrev wrote:

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

True, it doesn't filter _all_ of them, but it partially filters some symbols which is still a win I guess :) My rationale is probably that having this blocked on more implementation details would be more reasonable if there was no implementation at all but since it still filters out some symbols this should probably be fine.

[hokein]

In D84811#2199829, @kbobyrev wrote:

In D84811#2199820, @hokein wrote:

In D84811#2199510, @kbobyrev wrote:

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

True, it doesn't filter _all_ of them, but it partially filters some symbols which is still a win I guess :) My rationale is probably that having this blocked on more implementation details would be more reasonable if there was no implementation at all but since it still filters out some symbols this should probably be fine.

It just reduces symptoms, not solving the problem. I think remote-index is also blocked the FileFiltering stuff (we can't launch remote-index without fixing this issue entirely).

As you may have noticed implementing FileFiltering is tricky, I think the indexer here is a good opportunity to test/verify your implementation (comparing the index data before vs after), so my preference would be to implement the FileFilter, test it with this patch together, then finally land this patch after making sure everything works.

[kbobyrev]

In D84811#2199891, @hokein wrote:

In D84811#2199829, @kbobyrev wrote:

In D84811#2199820, @hokein wrote:

In D84811#2199510, @kbobyrev wrote:

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

True, it doesn't filter _all_ of them, but it partially filters some symbols which is still a win I guess :) My rationale is probably that having this blocked on more implementation details would be more reasonable if there was no implementation at all but since it still filters out some symbols this should probably be fine.

It just reduces symptoms, not solving the problem. I think remote-index is also blocked the FileFiltering stuff (we can't launch remote-index without fixing this issue entirely).

As you may have noticed implementing FileFiltering is tricky, I think the indexer here is a good opportunity to test/verify your implementation (comparing the index data before vs after), so my preference would be to implement the FileFilter, test it with this patch together, then finally land this patch after making sure everything works.

Fair enough, this is a viable strategy.

clang-tools-extra/clangd/indexer/IndexerMain.cpp

 
 #include "index/IndexAction.h"
 #include "index/Merge.h"
 #include "index/Ref.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/SymbolCollector.h"
 #include "support/Logger.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Execution.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 static llvm::cl::opt<IndexFileFormat>
     Format("format", llvm::cl::desc("Format of the index to be written"),
            llvm::cl::values(clEnumValN(IndexFileFormat::YAML, "yaml",
                                        "human-readable YAML format"),
                             clEnumValN(IndexFileFormat::RIFF, "binary",
                                        "binary RIFF format")),
            llvm::cl::init(IndexFileFormat::RIFF));
 
+static llvm::cl::opt<std::string>
+    ProjectRoot("project-root",
+                llvm::cl::desc("Filter out all symbols outside of this "
+                               "directory. This is useful for remote index."),
+                llvm::cl::Hidden);
+
 class IndexActionFactory : public tooling::FrontendActionFactory {
 public:
-  IndexActionFactory(IndexFileIn &Result) : Result(Result) {}
+  IndexActionFactory(IndexFileIn &Result, llvm::StringRef IndexRoot)
+      : Result(Result) {
+    if (!IndexRoot.empty()) {
+      // Returns true if file should be indexed.
+      FileFilter = [&](const SourceManager &SM, FileID ID) {
+        const FileEntry *Entry = SM.getFileEntryForID(ID);
+        if (!Entry)
+          return false;
+        llvm::SmallString<256> FilePath = Entry->tryGetRealPathName();
+        // When replacing prefix it checks whether Path starts with OldPrefix.
+        // NewPrefix doesn't matter in this case.
+        return llvm::sys::path::replace_path_prefix(
+            /*Path=*/FilePath, /*OldPrefix=*/IndexRoot, /*NewPrefix=*/"");
+      };
+    }
+  }
 
   std::unique_ptr<FrontendAction> create() override {
     SymbolCollector::Options Opts;
+    Opts.FileFilter = FileFilter;
     Opts.CountReferences = true;
     return createStaticIndexingAction(
         Opts,
         [&](SymbolSlab S) {
           // Merge as we go.
           std::lock_guard<std::mutex> Lock(SymbolsMu);
           for (const auto &Sym : S) {
             if (const auto *Existing = Symbols.find(Sym.ID))
               Symbols.insert(mergeSymbol(*Existing, Sym));
             else
               Symbols.insert(Sym);
           }
         },
         [&](RefSlab S) {
           std::lock_guard<std::mutex> Lock(SymbolsMu);
-          for (const auto &Sym : S) {
+          for (const auto &SymbolAndRefs : S) {
             // Deduplication happens during insertion.
-            for (const auto &Ref : Sym.second)
-              Refs.insert(Sym.first, Ref);
+            for (const auto &Ref : SymbolAndRefs.second)
+              Refs.insert(SymbolAndRefs.first, Ref);
           }
         },
         [&](RelationSlab S) {
           std::lock_guard<std::mutex> Lock(SymbolsMu);
           for (const auto &R : S) {
             Relations.insert(R);
           }
         },
         /*IncludeGraphCallback=*/nullptr);
   }
 
   // Awkward: we write the result in the destructor, because the executor
   // takes ownership so it's the easiest way to get our data back out.
   ~IndexActionFactory() {
     Result.Symbols = std::move(Symbols).build();
     Result.Refs = std::move(Refs).build();
     Result.Relations = std::move(Relations).build();
   }
 
 private:
   IndexFileIn &Result;
   std::mutex SymbolsMu;
   SymbolSlab::Builder Symbols;
   RefSlab::Builder Refs;
   RelationSlab::Builder Relations;
+  std::function<bool(const SourceManager &, FileID)> FileFilter = nullptr;
 };
 
 } // namespace
 } // namespace clangd
 } // namespace clang
 
 int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
int main(int argc, const char **argv) {
   if (!Executor) {
     llvm::errs() << llvm::toString(Executor.takeError()) << "\n";
     return 1;
   }
 
   // Collect symbols found in each translation unit, merging as we go.
   clang::clangd::IndexFileIn Data;
   auto Err = Executor->get()->execute(
-      std::make_unique<clang::clangd::IndexActionFactory>(Data),
+      std::make_unique<clang::clangd::IndexActionFactory>(
+          Data, llvm::sys::path::convert_to_slash(
+                    clang::clangd::ProjectRoot, llvm::sys::path::Style::posix)),
       clang::tooling::getStripPluginsAdjuster());
   if (Err) {
     clang::clangd::elog("{0}", std::move(Err));
   }
 
   // Emit collected data.
   clang::clangd::IndexFileOut Out(Data);
   Out.Format = clang::clangd::Format;
   llvm::outs() << Out;
   return 0;
 }