[clangd] Respect shouldIndexFile when collecting symbols.

[ioeric]

clangd/index/SymbolCollector.cpp

576

Should we use getTokenLocation like what we do below?

634

IIUC, if D is declared in d.h that is filtered out and defined in d.cc that is not filtered, this would only create a basic symbol with definition?

I think what we want is a full symbol with declaration, definition, includes etc. This is one assumption we make in order for the merging in auto-index to work.

[hokein]

Address comments and simplify the code.

[hokein]

clang-format and fix typos

[hokein]

clangd/index/SymbolCollector.cpp

576

getTokenLocation doesn't return a SourceLocation, it returns a SymbolLocation.

[ioeric]

clangd/index/SymbolCollector.cpp

217

nit: this is very easily confused with the callback with the same name on the call sites.

220

This is following the implementation detail of how location is generated in addDeclaration/addDefinition. I think we could put the filtering into addDeclaration/addDeclaration to avoid the duplicate here (i.e. return nullptr if decl is filtered like what you did in the previous revision?).

391

OriginalDecl.getCanonicalDecl() might not be the one we prefer. I think we should check all redecls and pick the preferred one?

unittests/clangd/SymbolCollectorTests.cpp

533

It's not trivial to tell what are the cases we are testing here. Maybe add some comments?

There are a few cases that are interesting. For examples:

• Symbol is declared in a filtered header and defined in a index file.
• Symbol is declared in a indexed file and defined in a filter file.
• Symbol has forward declaration and canonical declaration in filtered header and definition in indexed file.
• ...

It might be clearer to split them into multiple cases.

[hokein]

Update the patch based on offline discussion.

[hokein]

clangd/index/SymbolCollector.cpp

217

removed it.

220

as discussed, we decide to move the filter logic out of addDeclaration/addDefinition, and also move out the findNameLoc logic.

391

As discussed, canonicalDecl is always the decl we preferred here. If the definition decl is preferred, it will be handled in above isPreferredDeclaration code.

unittests/clangd/SymbolCollectorTests.cpp

533

Added comments.

[ioeric]

clangd/index/SymbolCollector.cpp

386

shouldIndexLoc seems more accurate?

392

DeclSLoc is a bit confusing (SLoc is used somewhere else). Maybe DeclSymLoc or simply DeclLoc?

397

We would be doing redundant work if OriginalDecl == ND?

409

Could you document here why we are also checking !BasicSymbol here?

571

nit: s/DeclSLoc/DeclLoc? and maybe add documentation about why we need this?

unittests/clangd/SymbolCollectorTests.cpp

598

I think making RefsInFile take a single file and using it in combination with anyOf/allOf would be more straightforward.

clangd/index/SymbolCollector.h

struct Options {
     SymbolOrigin Origin = SymbolOrigin::Unknown;
     /// Collect macros.
     /// Note that SymbolCollector must be run with preprocessor in order to
     /// collect macros. For example, `indexTopLevelDecls` will not index any
     /// macro even if this is true.
     bool CollectMacro = false;
     /// If this is set, only collect symbols/references from a file if
     /// `FileFilter(SM, FID)` is true. If not set, all files are indexed.
+    /// Note that a symbol can be redeclared in multiple files, a complete
+    /// symbol will be constructed if this symbol is declared in one of indexed
+    /// files.
     std::function<bool(const SourceManager &, FileID)> FileFilter = nullptr;
   };
 
   SymbolCollector(Options Opts);
 
   /// Returns true is \p ND should be collected.
   /// AST matchers require non-const ASTContext.
   static bool shouldCollectSymbol(const NamedDecl &ND, ASTContext &ASTCtx,
bool handleMacroOccurence(const IdentifierInfo *Name, const MacroInfo *MI,
                             SourceLocation Loc) override;
 
   SymbolSlab takeSymbols() { return std::move(Symbols).build(); }
   RefSlab takeRefs() { return std::move(Refs).build(); }
 
   void finish() override;
 
 private:
-  const Symbol *addDeclaration(const NamedDecl &, SymbolID);
-  void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
+  const Symbol *addDeclaration(const NamedDecl &, SourceLocation, SymbolID);
+  void addDefinition(SourceLocation, const Symbol &DeclSymbol);
 
   // All Symbols collected from the AST.
   SymbolSlab::Builder Symbols;
   // All refs collected from the AST.
   // Only symbols declared in preamble (from #include) and referenced from the
   // main file will be included.
   RefSlab::Builder Refs;
   ASTContext *ASTCtx;

clangd/index/SymbolCollector.cpp

bool shouldIndexFile(const SourceManager &SM, FileID FID,
   if (!Opts.FileFilter)
     return true;
   auto I = FilesToIndexCache->try_emplace(FID);
   if (I.second)
     I.first->second = Opts.FileFilter(SM, FID);
   return I.first->second;
 }
 
 // Return the symbol location of the token at \p TokLoc.
 Optional<SymbolLocation> getTokenLocation(SourceLocation TokLoc,
                                           const SourceManager &SM,
                                           const SymbolCollector::Options &Opts,
                                           const clang::LangOptions &LangOpts,
                                           std::string &FileURIStorage) {
   auto U = toURI(SM, SM.getFilename(TokLoc), Opts);
   if (!U)
     return None;
   FileURIStorage = std::move(*U);
   SymbolLocation Result;
   Result.FileURI = FileURIStorage;
bool SymbolCollector::handleDeclOccurence(
   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();
   auto SpellingLoc = SM.getSpellingLoc(Loc);
+  auto FID = SM.getFileID(SpellingLoc);
   if (Opts.CountReferences &&
       (Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
-      SM.getFileID(SpellingLoc) == SM.getMainFileID())
+      FID == SM.getMainFileID())
     ReferencedDecls.insert(ND);
 
   bool CollectRef = static_cast<unsigned>(Opts.RefFilter) & Roles;
   bool IsOnlyRef =
       !(Roles & (static_cast<unsigned>(index::SymbolRole::Declaration) |
                  static_cast<unsigned>(index::SymbolRole::Definition)));
 
   if (IsOnlyRef && !CollectRef)
     return true;
   if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
     return true;
   if (CollectRef && !isa<NamespaceDecl>(ND) &&
-      (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
+      (Opts.RefsInHeaders || FID == SM.getMainFileID()) &&
+      shouldIndexFile(SM, FID, Opts, &FilesToIndexCache)) {
     DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+  }
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
     return true;
 
   auto ID = getSymbolID(ND);
   if (!ID)
     return true;
 
   const NamedDecl &OriginalDecl = *cast<NamedDecl>(ASTNode.OrigD);
   const Symbol *BasicSymbol = Symbols.find(*ID);
-  if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
-    BasicSymbol = addDeclaration(*ND, std::move(*ID));
-  else if (isPreferredDeclaration(OriginalDecl, Roles))
+  auto shouldIndexSymbol = [&](SourceLocation TargetLoc) {
+    return shouldIndexFile(SM, SM.getFileID(TargetLoc), Opts,
+                           &FilesToIndexCache);
+  };
+
+  if (!BasicSymbol) {
+    auto DeclSLoc = findNameLoc(ND);
+    if (shouldIndexSymbol(DeclSLoc))
+      // Regardless of role, ND is the canonical declaration.
+      BasicSymbol = addDeclaration(*ND, DeclSLoc, *ID);
+  }
+  if (isPreferredDeclaration(OriginalDecl, Roles)) {
+    auto DeclSLoc = findNameLoc(&OriginalDecl);
+    if (shouldIndexSymbol(DeclSLoc))
       // 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));
+      BasicSymbol = addDeclaration(OriginalDecl, DeclSLoc, *ID);
+  }
+
+  if (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) {
+    auto DefLoc = findNameLoc(&OriginalDecl);
+    if (!shouldIndexSymbol(DefLoc) && !BasicSymbol)
+      return true;
 
-  if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
-    addDefinition(OriginalDecl, *BasicSymbol);
+    if (!BasicSymbol) {
+      // The symbol is defined in indexed file, but we haven't indexed the
+      // symbolyet because files where the symbol is declared are filtered out,
+      // now we construct a complete symbols from canononical decl (even the
+      // decl is in a filtered file).
+      const auto *CanonicalDecl =
+          cast<NamedDecl>(OriginalDecl.getCanonicalDecl());
+      BasicSymbol =
+          addDeclaration(*CanonicalDecl, findNameLoc(CanonicalDecl), *ID);
+    }
+    addDefinition(DefLoc, *BasicSymbol);
+  }
   return true;
 }
 
 bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
                                            const MacroInfo *MI,
                                            index::SymbolRoleSet Roles,
                                            SourceLocation Loc) {
   if (!Opts.CollectMacro)
if (Symbols.find(*ID) != nullptr)
     return true;
 
   Symbol S;
   S.ID = std::move(*ID);
   S.Name = Name->getName();
   S.Flags |= Symbol::IndexedForCodeCompletion;
   S.SymInfo = index::getSymbolInfoForMacro(*MI);
   std::string FileURI;
-  // FIXME: use the result to filter out symbols.
-  shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
+  if (!shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache))
+    return true;
   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) -> Optional<std::string> {
     return Found->second;
   };
 
   if (auto MainFileURI = GetURI(SM.getMainFileID())) {
     for (const auto &It : DeclRefs) {
       if (auto ID = getSymbolID(It.first)) {
         for (const auto &LocAndRole : It.second) {
           auto FileID = SM.getFileID(LocAndRole.first);
-          // FIXME: use the result to filter out references.
-          shouldIndexFile(SM, FileID, Opts, &FilesToIndexCache);
           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;
             R.Kind = toRefKind(LocAndRole.second);
             Refs.insert(*ID, R);
           }
         }
       }
     }
   }
 
   ReferencedDecls.clear();
   ReferencedMacros.clear();
   DeclRefs.clear();
   FilesToIndexCache.clear();
 }
 
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
+                                              SourceLocation DeclSLoc,
                                               SymbolID ID) {
   auto &Ctx = ND.getASTContext();
   auto &SM = Ctx.getSourceManager();
 
   Symbol S;
   S.ID = std::move(ID);
   std::string QName = printQualifiedName(ND);
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
   // FIXME: this returns foo:bar: for objective-C methods, we prefer only foo:
   // for consistency with CodeCompletionString and a clean name/signature split.
 
   if (isIndexedForCodeCompletion(ND, Ctx))
     S.Flags |= Symbol::IndexedForCodeCompletion;
   if (isImplementationDetail(&ND))
     S.Flags |= Symbol::ImplementationDetail;
   S.SymInfo = index::getSymbolInfo(&ND);
   std::string FileURI;
-  auto Loc = findNameLoc(&ND);
-  // FIXME: use the result to filter out symbols.
-  shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
   if (auto DeclLoc =
-          getTokenLocation(Loc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
+          getTokenLocation(DeclSLoc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 
   // Add completion info.
   // FIXME: we may want to choose a different redecl, or combine from several.
   assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
   // We use the primary template, as clang does during code completion.
   CodeCompletionResult SymbolCompletion(&getTemplateOrThis(ND), 0);
   const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
 
   S.Origin = Opts.Origin;
   if (ND.getAvailability() == AR_Deprecated)
     S.Flags |= Symbol::Deprecated;
   Symbols.insert(S);
   return Symbols.find(S.ID);
 }
 
-void SymbolCollector::addDefinition(const NamedDecl &ND,
+void SymbolCollector::addDefinition(SourceLocation DefSLoc,
                                     const Symbol &DeclSym) {
   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;
-  auto Loc = findNameLoc(&ND);
-  const auto &SM = ND.getASTContext().getSourceManager();
-  // FIXME: use the result to filter out symbols.
-  shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
-  if (auto DefLoc =
-          getTokenLocation(Loc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
+  if (auto DefLoc = getTokenLocation(DefSLoc, ASTCtx->getSourceManager(), Opts,
+                                     ASTCtx->getLangOpts(), FileURI))
     S.Definition = *DefLoc;
   Symbols.insert(S);
 }
 
 } // namespace clangd
 } // namespace clang

unittests/clangd/SymbolCollectorTests.cpp

return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
                          Pos.Location.End.line(), Pos.Location.End.column()) ==
          std::make_tuple(Range.start.line, Range.start.character,
                          Range.end.line, Range.end.character);
 }
 testing::Matcher<const std::vector<Ref> &>
 HaveRanges(const std::vector<Range> Ranges) {
   return testing::UnorderedPointwise(RefRange(), Ranges);
 }
+MATCHER_P(RefsInFiles, Files, "") {
+  for (const Ref &R : arg) {
+    if (!Files.count(R.Location.FileURI))
+      return false;
+  }
+  return true;
+}
 
 class ShouldCollectSymbolTest : public ::testing::Test {
 public:
   void build(StringRef HeaderCode, StringRef Code = "") {
     File.HeaderFilename = HeaderName;
     File.Filename = FileName;
     File.HeaderCode = HeaderCode;
     File.Code = Code;
TEST_F(SymbolCollectorTest, References) {
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(AllOf(QName("W"), RefCount(1)),
                                    AllOf(QName("X"), RefCount(1)),
                                    AllOf(QName("Y"), RefCount(0)),
                                    AllOf(QName("Z"), RefCount(0)), QName("y")));
 }
 
+TEST_F(SymbolCollectorTest, FileFilter) {
+  const std::string NonIndexedHeader = R"(
+    // non-indexed symbols, didn't see any decl/def in indexed files.
+    class Foo1 {};
+    void k();
+    inline void k() {}
+    #define GLOBAL_Z(name) Z name;
+
+    // indexed symbols
+    class Foo2;    // def in indexed .h file
+    class Foo3;    // def in indexed .cc file
+    void fun1();   // def in indexed .cc file
+  )";
+
+  Annotations IndexedHeader(R"(
+    // indexed symbols
+    class Bar1 {};
+    class Bar2;
+    class Foo2 {};
+  )");
+  Annotations Main(R"(
+    #include "nonindexed_header.h"
+    #include "indexed_header.h"
+
+    class Foo3 {};
+    class Bar2 {};
+    void fun1() {}
+  )");
+  const std::string NonIndexedHeaderPath = testPath("nonindexed_header.h");
+  const std::string IndexedHeaderPath = testPath("indexed_header.h");
+  const std::string NonIndexedHeaderURI =
+      URI::createFile(NonIndexedHeaderPath).toString();
+  const std::string IndexedHeaderURI =
+      URI::createFile(IndexedHeaderPath).toString();
+
+  InMemoryFileSystem->addFile(NonIndexedHeaderPath, 0,
+                              MemoryBuffer::getMemBuffer(NonIndexedHeader));
+  InMemoryFileSystem->addFile(IndexedHeaderPath, 0,
+                              MemoryBuffer::getMemBuffer(IndexedHeader.code()));
+  CollectorOpts.FileFilter = [](const SourceManager &SM, FileID FID) {
+    if (const auto *F = SM.getFileEntryForID(FID))
+      return !F->getName().contains("nonindexed_header.h");
+    return true;
+  };
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  runSymbolCollector("", Main.code());
+
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
+                           AllOf(QName("Foo2"), DeclURI(IndexedHeaderURI),
+                                 DefURI(IndexedHeaderURI)),
+                           AllOf(QName("Foo3"), DeclURI(NonIndexedHeaderURI),
+                                 DefURI(TestFileURI)),
+                           AllOf(QName("fun1"), DeclURI(NonIndexedHeaderURI),
+                                 DefURI(TestFileURI)),
+                           AllOf(QName("Bar1"), DeclURI(IndexedHeaderURI),
+                                 DefURI(IndexedHeaderURI)),
+                           AllOf(QName("Bar2"), DeclURI(IndexedHeaderURI),
+                                 DefURI(TestFileURI))));
+  auto Files = [](const std::initializer_list<llvm::StringRef> &Files) {
+    return llvm::StringSet<>(Files);
+  };
+  EXPECT_THAT(Refs,
+              UnorderedElementsAre(
+                  Pair(findSymbol(Symbols, "Foo2").ID,
+                       RefsInFiles(Files({IndexedHeaderURI}))),
+                  Pair(findSymbol(Symbols, "Foo3").ID,
+                       RefsInFiles(Files({TestFileURI}))),
+                  Pair(findSymbol(Symbols, "fun1").ID,
+                       RefsInFiles(Files({TestFileURI}))),
+                  Pair(findSymbol(Symbols, "Bar1").ID,
+                       RefsInFiles(Files({IndexedHeaderURI}))),
+                  Pair(findSymbol(Symbols, "Bar2").ID,
+                       RefsInFiles(Files({IndexedHeaderURI, TestFileURI})))));
+}
+
 TEST_F(SymbolCollectorTest, SymbolRelativeNoFallback) {
   runSymbolCollector("class Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            AllOf(QName("Foo"), DeclURI(TestHeaderURI))));
 }
 
 TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
   TestHeaderName = "x.h";