Index: clangd/index/SymbolCollector.h =================================================================== --- clangd/index/SymbolCollector.h +++ clangd/index/SymbolCollector.h @@ -78,6 +78,9 @@ 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 FileFilter = nullptr; }; @@ -110,8 +113,8 @@ 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; Index: clangd/index/SymbolCollector.cpp =================================================================== --- clangd/index/SymbolCollector.cpp +++ clangd/index/SymbolCollector.cpp @@ -353,9 +353,10 @@ // 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(index::SymbolRole::Reference)) && - SM.getFileID(SpellingLoc) == SM.getMainFileID()) + FID == SM.getMainFileID()) ReferencedDecls.insert(ND); bool CollectRef = static_cast(Opts.RefFilter) & Roles; @@ -368,8 +369,10 @@ if (!shouldCollectSymbol(*ND, *ASTCtx, Opts)) return true; if (CollectRef && !isa(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; @@ -380,17 +383,44 @@ const NamedDecl &OriginalDecl = *cast(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)) - // 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)); - - if (Roles & static_cast(index::SymbolRole::Definition)) - addDefinition(OriginalDecl, *BasicSymbol); + 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, DeclSLoc, *ID); + } + + if (Roles & static_cast(index::SymbolRole::Definition)) { + auto DefLoc = findNameLoc(&OriginalDecl); + if (!shouldIndexSymbol(DefLoc) && !BasicSymbol) + return true; + + 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(OriginalDecl.getCanonicalDecl()); + BasicSymbol = + addDeclaration(*CanonicalDecl, findNameLoc(CanonicalDecl), *ID); + } + addDefinition(DefLoc, *BasicSymbol); + } return true; } @@ -438,8 +468,8 @@ 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; @@ -516,8 +546,6 @@ 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()); @@ -540,6 +568,7 @@ } const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, + SourceLocation DeclSLoc, SymbolID ID) { auto &Ctx = ND.getASTContext(); auto &SM = Ctx.getSourceManager(); @@ -557,11 +586,8 @@ 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. @@ -603,7 +629,7 @@ return Symbols.find(S.ID); } -void SymbolCollector::addDefinition(const NamedDecl &ND, +void SymbolCollector::addDefinition(SourceLocation DefSLoc, const Symbol &DeclSym) { if (DeclSym.Definition) return; @@ -612,12 +638,8 @@ // 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); } Index: unittests/clangd/SymbolCollectorTests.cpp =================================================================== --- unittests/clangd/SymbolCollectorTests.cpp +++ unittests/clangd/SymbolCollectorTests.cpp @@ -98,6 +98,13 @@ HaveRanges(const std::vector 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: @@ -523,6 +530,82 @@ 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 &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(