[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

643

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

643

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

643

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

if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
     BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
   else if (isPreferredDeclaration(*OriginalDecl, Roles))
     // If OriginalDecl is preferred, replace the existing canonical
     // declaration (e.g. a class forward declaration). There should be at most
     // one duplicate as we expect to see only one preferred declaration per
     // TU, because in practice they are definitions.
     BasicSymbol = addDeclaration(*OriginalDecl, std::move(*ID), IsMainFileOnly);
 
+  if (BasicSymbol != nullptr)
     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());
bool SymbolCollector::handleMacroOccurrence(const IdentifierInfo *Name,
   S.Name = Name->getName();
   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 (!shouldIndexFile(SM.getFileID(Loc)))
+    return false;
   if (auto DeclLoc =
           getTokenLocation(DefLoc, SM, Opts, PP->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 
   CodeCompletionResult SymbolCompletion(Name);
   const auto *CCS = SymbolCompletion.CreateCodeCompletionStringForMacro(
       *PP, *CompletionAllocator, *CompletionTUInfo);
   std::string Signature;
auto GetURI = [&](FileID FID) -> llvm::Optional<std::string> {
     }
     return Found->second;
   };
   auto CollectRef =
       [&](SymbolID ID,
           const std::pair<SourceLocation, index::SymbolRoleSet> &LocAndRole,
           bool Spelled = false) {
         auto FileID = SM.getFileID(LocAndRole.first);
-        // FIXME: use the result to filter out references.
-        shouldIndexFile(FileID);
+        if (!shouldIndexFile(FileID))
+          return;
         if (auto FileURI = GetURI(FileID)) {
           auto Range =
               getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
           Ref R;
           R.Location.Start = Range.first;
           R.Location.End = Range.second;
           R.Location.FileURI = FileURI->c_str();
           R.Kind = toRefKind(LocAndRole.second, Spelled);
const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
   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 (!shouldIndexFile(SM.getFileID(Loc)))
+    return nullptr;
   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;
 
if (DeclSym.Definition)
     return;
   // If we saw some forward declaration, we end up copying the symbol.
   // This is not ideal, but avoids duplicating the "is this a definition" check
   // in clang::index. We should only see one definition.
   Symbol S = DeclSym;
   std::string FileURI;
   const auto &SM = ND.getASTContext().getSourceManager();
   auto Loc = nameLocation(ND, SM);
-  // FIXME: use the result to filter out symbols.
-  shouldIndexFile(SM.getFileID(Loc));
+  if (!shouldIndexFile(SM.getFileID(Loc)))
+    return;
   if (auto DefLoc =
           getTokenLocation(Loc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
     S.Definition = *DefLoc;
   Symbols.insert(S);
 }
 
 /// Gets a canonical include (URI of the header or <header> or "header") for
 /// header of \p FID (which should usually be the *expansion* file).