[clangd] Implement FileFilter for the indexer

[kbobyrev]

Summary

Tests failing so far: BackgroundIndexTests.IndexTwoFiles,
BackgroundIndexTests.ShardStorageLoad.

[kbobyrev]

@sammccall This is the change I was talking about during the standup: the code looks legit, but BackgroundIndexTests.IndexTwoFiles fails because with the following code path in Background.cpp's FileFilter:

const auto *F = SM.getFileEntryForID(FID);
if (!F)
  return false; // Skip invalid files.

[hokein]

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

676

A drive-by comment from D84811: the file granularity vs symbol granularity is tricky here.

Note that a *full* symbol (with declaration, definition, etc) may be formed from different files (.h, .cc), thinking of a following case:

// foo.h
void func();

// user.cc
#include "foo.h"

// foo.cc
#include "foo.h"
void func() {}

if our indexer indexes user.cc first, then foo.h is considered indexed, later when indexing foo.cc, we will skip the func symbol. so the symbol foo will not have definition.

[kbobyrev]

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

676

Oh, you're right, good catch! That's why the post-filtering would probably work but maybe won't be as fancy :(

Thank you for mentioning it!

[sammccall]

In D85426#2199501, @kbobyrev wrote:

@sammccall This is the change I was talking about during the standup: the code looks legit, but BackgroundIndexTests.IndexTwoFiles fails because with the following code path in Background.cpp's FileFilter:

What location is invalid, and why?

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

676

This is indeed happening in the wrong place, I think, but postfiltering isn't the only solution.

The FileFilter refers to what files you want to index, not which symbols. So I think you want to make the decision in handle*Occurrence, based on the location of the occurrence, not the location of the declaration it refers to.

By the time you get to addDefinition/addDeclaration it really is too late, the caller has already decided that we want to index this symbol and which declaration should be used. (BTW, weakening addDeclaration to return nullptr sometimes seems wrong for the same reason).

This is all very confusing, and we should rewrite the indexer to make it more explicit how data is combined, and less stateful. (More like a mapreduce)

[kbobyrev]

"Fail" early.

[kbobyrev]

Also check if deinition comes from unwwanted file (e.g. omit forward declarations).

[kbobyrev]

WIP. Need to get around background indexing tricks to make it work.

Failed Tests (3):
  Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.Config
  Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.IndexTwoFiles
  Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.ShardStorageLoad

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

 // Always return true to continue indexing.
 bool SymbolCollector::handleDeclOccurrence(
     const Decl *D, index::SymbolRoleSet Roles,
     llvm::ArrayRef<index::SymbolRelation> Relations, SourceLocation Loc,
     index::IndexDataConsumer::ASTNodeInfo ASTNode) {
   assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
   assert(CompletionAllocator && CompletionTUInfo);
   assert(ASTNode.OrigD);
+  auto &SM = ASTCtx->getSourceManager();
+  if (!SM.isWrittenInBuiltinFile(Loc) &&
+      (!shouldIndexFile(SM.getFileID(Loc)) ||
+       !shouldIndexFile(SM.getFileID(D->getLocation()))))
+    return true;
   // Indexing API puts canonical decl into D, which might not have a valid
   // source location for implicit/built-in decls. Fallback to original decl in
   // such cases.
   if (D->getLocation().isInvalid())
     D = ASTNode.OrigD;
   // If OrigD is an declaration associated with a friend declaration and it's
   // not a definition, skip it. Note that OrigD is the occurrence that the
   // collector is currently visiting.
if (const auto *CD = CID->getCategoryDecl())
       D = CD;
   }
   const NamedDecl *ND = dyn_cast<NamedDecl>(D);
   if (!ND)
     return true;
 
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
-  auto &SM = ASTCtx->getSourceManager();
   if (Opts.CountReferences &&
       (Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
       SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
     ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
   if (!ID)
     return true;
bool SymbolCollector::handleMacroOccurrence(const IdentifierInfo *Name,
                                             index::SymbolRoleSet Roles,
                                             SourceLocation Loc) {
   assert(PP.get());
   // Builtin macros don't have useful locations and aren't needed in completion.
   if (MI->isBuiltinMacro())
     return true;
 
   const auto &SM = PP->getSourceManager();
+  if (!SM.isWrittenInBuiltinFile(Loc) && !shouldIndexFile(SM.getFileID(Loc)))
+    return true;
+
   auto DefLoc = MI->getDefinitionLoc();
   // 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;
if (isImplementationDetail(&ND))
     S.Flags |= Symbol::ImplementationDetail;
   if (!IsMainFileOnly)
     S.Flags |= Symbol::VisibleOutsideFile;
   S.SymInfo = index::getSymbolInfo(&ND);
   std::string FileURI;
   auto Loc = nameLocation(ND, SM);
   assert(Loc.isValid() && "Invalid source location for NamedDecl");
   // FIXME: use the result to filter out symbols.
   shouldIndexFile(SM.getFileID(Loc));
   if (auto DeclLoc =
           getTokenLocation(Loc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 
   S.Origin = Opts.Origin;
   if (ND.getAvailability() == AR_Deprecated)
     S.Flags |= Symbol::Deprecated;