[clangd] Fix updated file detection logic in indexing

Summary:
Files without any symbols were never marked as updated during indexing, which resulted in failure while writing shards for these files.

This patch fixes the logic to mark files that are seen for the first time but don't contain any symbols as updated.

Reviewers: ilya-biryukov

Reviewed By: ilya-biryukov

Subscribers: ioeric, MaskRay, jkorous, arphaman, cfe-commits

Differential Revision: https://reviews.llvm.org/D56592

llvm-svn: 351170

3 files changed, 110 insertions, 33 deletions

diff --git a/clang-tools-extra/clangd/index/Background.cpp b/clang-tools-extra/clangd/index/Background.cpp
index ef97e603433d..b9b945c52bda 100644
--- a/clang-tools-extra/clangd/index/Background.cpp
+++ b/clang-tools-extra/clangd/index/Background.cpp
@@ -91,12 +91,10 @@ IncludeGraph getSubGraph(const URI &U, const IncludeGraph &FullGraph) {
 
 // Creates a filter to not collect index results from files with unchanged
 // digests.
-// \p FileDigests contains file digests for the current indexed files, and all
-// changed files will be added to \p FilesToUpdate.
+// \p FileDigests contains file digests for the current indexed files.
 decltype(SymbolCollector::Options::FileFilter)
-createFileFilter(const llvm::StringMap<FileDigest> &FileDigests,
-                 llvm::StringMap<FileDigest> &FilesToUpdate) {
-  return [&FileDigests, &FilesToUpdate](const SourceManager &SM, FileID FID) {
+createFileFilter(const llvm::StringMap<FileDigest> &FileDigests) {
+  return [&FileDigests](const SourceManager &SM, FileID FID) {
     const auto *F = SM.getFileEntryForID(FID);
     if (!F)
       return false; // Skip invalid files.
@@ -109,8 +107,6 @@ createFileFilter(const llvm::StringMap<FileDigest> &FileDigests,
     auto D = FileDigests.find(*AbsPath);
     if (D != FileDigests.end() && D->second == Digest)
       return false; // Skip files that haven't changed.
-
-    FilesToUpdate[*AbsPath] = *Digest;
     return true;
   };
 }
@@ -264,22 +260,34 @@ void BackgroundIndex::enqueueTask(Task T, ThreadPriority Priority) {
   QueueCV.notify_all();
 }
 
-/// Given index results from a TU, only update files in \p FilesToUpdate.
+/// Given index results from a TU, only update symbols coming from files that
+/// are different or missing from than \p DigestsSnapshot. Also stores new index
+/// information on IndexStorage.
 void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
-                             const llvm::StringMap<FileDigest> &FilesToUpdate,
+                             const llvm::StringMap<FileDigest> &DigestsSnapshot,
                              BackgroundIndexStorage *IndexStorage) {
   // Partition symbols/references into files.
   struct File {
     llvm::DenseSet<const Symbol *> Symbols;
     llvm::DenseSet<const Ref *> Refs;
+    FileDigest Digest;
   };
   llvm::StringMap<File> Files;
   URIToFileCache URICache(MainFile);
+  for (const auto &IndexIt : *Index.Sources) {
+    const auto &IGN = IndexIt.getValue();
+    const auto AbsPath = URICache.resolve(IGN.URI);
+    const auto DigestIt = DigestsSnapshot.find(AbsPath);
+    // File has different contents.
+    if (DigestIt == DigestsSnapshot.end() || DigestIt->getValue() != IGN.Digest)
+      Files.try_emplace(AbsPath).first->getValue().Digest = IGN.Digest;
+  }
   for (const auto &Sym : *Index.Symbols) {
     if (Sym.CanonicalDeclaration) {
       auto DeclPath = URICache.resolve(Sym.CanonicalDeclaration.FileURI);
-      if (FilesToUpdate.count(DeclPath) != 0)
-        Files[DeclPath].Symbols.insert(&Sym);
+      const auto FileIt = Files.find(DeclPath);
+      if (FileIt != Files.end())
+        FileIt->second.Symbols.insert(&Sym);
     }
     // For symbols with different declaration and definition locations, we store
     // the full symbol in both the header file and the implementation file, so
@@ -288,16 +296,18 @@ void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
     if (Sym.Definition &&
         Sym.Definition.FileURI != Sym.CanonicalDeclaration.FileURI) {
       auto DefPath = URICache.resolve(Sym.Definition.FileURI);
-      if (FilesToUpdate.count(DefPath) != 0)
-        Files[DefPath].Symbols.insert(&Sym);
+      const auto FileIt = Files.find(DefPath);
+      if (FileIt != Files.end())
+        FileIt->second.Symbols.insert(&Sym);
     }
   }
   llvm::DenseMap<const Ref *, SymbolID> RefToIDs;
   for (const auto &SymRefs : *Index.Refs) {
     for (const auto &R : SymRefs.second) {
       auto Path = URICache.resolve(R.Location.FileURI);
-      if (FilesToUpdate.count(Path) != 0) {
-        auto &F = Files[Path];
+      const auto FileIt = Files.find(Path);
+      if (FileIt != Files.end()) {
+        auto &F = FileIt->getValue();
         RefToIDs[&R] = SymRefs.first;
         F.Refs.insert(&R);
       }
@@ -305,18 +315,14 @@ void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
   }
 
   // Build and store new slabs for each updated file.
-  for (const auto &I : *Index.Sources) {
-    std::string Path = URICache.resolve(I.first());
+  for (const auto &FileIt : Files) {
+    llvm::StringRef Path = FileIt.getKey();
     SymbolSlab::Builder Syms;
     RefSlab::Builder Refs;
-    auto FileIt = Files.find(Path);
-    if (FileIt != Files.end()) {
-      auto &F = *FileIt;
-      for (const auto *S : F.second.Symbols)
-        Syms.insert(*S);
-      for (const auto *R : F.second.Refs)
-        Refs.insert(RefToIDs[R], *R);
-    }
+    for (const auto *S : FileIt.second.Symbols)
+      Syms.insert(*S);
+    for (const auto *R : FileIt.second.Refs)
+      Refs.insert(RefToIDs[R], *R);
     auto SS = llvm::make_unique<SymbolSlab>(std::move(Syms).build());
     auto RS = llvm::make_unique<RefSlab>(std::move(Refs).build());
     auto IG = llvm::make_unique<IncludeGraph>(
@@ -335,7 +341,7 @@ void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
     }
     {
       std::lock_guard<std::mutex> Lock(DigestsMu);
-      auto Hash = I.second.Digest;
+      auto Hash = FileIt.second.Digest;
       // Skip if file is already up to date.
       auto DigestIt = IndexedFileDigests.try_emplace(Path);
       if (!DigestIt.second && DigestIt.first->second == Hash)
@@ -410,8 +416,7 @@ llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd,
                                    "Couldn't build compiler instance");
 
   SymbolCollector::Options IndexOpts;
-  llvm::StringMap<FileDigest> FilesToUpdate;
-  IndexOpts.FileFilter = createFileFilter(DigestsSnapshot, FilesToUpdate);
+  IndexOpts.FileFilter = createFileFilter(DigestsSnapshot);
   IndexFileIn Index;
   auto Action = createStaticIndexingAction(
       IndexOpts, [&](SymbolSlab S) { Index.Symbols = std::move(S); },
@@ -448,7 +453,7 @@ llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd,
   SPAN_ATTACH(Tracer, "refs", int(Index.Refs->numRefs()));
   SPAN_ATTACH(Tracer, "sources", int(Index.Sources->size()));
 
-  update(AbsolutePath, std::move(Index), FilesToUpdate, IndexStorage);
+  update(AbsolutePath, std::move(Index), DigestsSnapshot, IndexStorage);
 
   if (BuildIndexPeriodMs > 0)
     SymbolsUpdatedSinceLastIndex = true;
diff --git a/clang-tools-extra/clangd/index/Background.h b/clang-tools-extra/clangd/index/Background.h
index c9b290c685ec..1a1fee68a571 100644
--- a/clang-tools-extra/clangd/index/Background.h
+++ b/clang-tools-extra/clangd/index/Background.h
@@ -91,10 +91,11 @@ public:
   blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds = 10);
 
 private:
-  /// Given index results from a TU, only update files in \p FilesToUpdate.
-  /// Also stores new index information on IndexStorage.
+  /// Given index results from a TU, only update symbols coming from files with
+  /// different digests than \p DigestsSnapshot. Also stores new index
+  /// information on IndexStorage.
   void update(llvm::StringRef MainFile, IndexFileIn Index,
-              const llvm::StringMap<FileDigest> &FilesToUpdate,
+              const llvm::StringMap<FileDigest> &DigestsSnapshot,
               BackgroundIndexStorage *IndexStorage);
 
   // configuration
diff --git a/clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp b/clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp
index 70c1e9c0f927..639d35c876ac 100644
--- a/clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp
+++ b/clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp
@@ -351,11 +351,82 @@ TEST_F(BackgroundIndexTest, ShardStorageLoad) {
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
 
   // Check if the new symbol has arrived.
-  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_NE(ShardSource, nullptr);
   EXPECT_THAT(*ShardSource->Symbols,
               Contains(AllOf(Named("f_b"), Declared(), Defined())));
 }
 
+TEST_F(BackgroundIndexTest, ShardStorageEmptyFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+      void common();
+      void f_b();
+      class A_CC {};
+      )cpp";
+  FS.Files[testPath("root/B.h")] = R"cpp(
+      #include "A.h"
+      )cpp";
+  FS.Files[testPath("root/A.cc")] =
+      "#include \"B.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap<std::string> Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check that A.cc, A.h and B.h has been stored.
+  {
+    OverlayCDB CDB(/*Base=*/nullptr);
+    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+                        [&](llvm::StringRef) { return &MSS; });
+    CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+    ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_THAT(Storage.keys(),
+              UnorderedElementsAre(testPath("root/A.cc"), testPath("root/A.h"),
+                                   testPath("root/B.h")));
+  auto ShardHeader = MSS.loadShard(testPath("root/B.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_TRUE(ShardHeader->Symbols->empty());
+
+  // Check that A.cc, A.h and B.h has been loaded.
+  {
+    CacheHits = 0;
+    OverlayCDB CDB(/*Base=*/nullptr);
+    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+                        [&](llvm::StringRef) { return &MSS; });
+    CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+    ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 3U);
+
+  // Update B.h to contain some symbols.
+  FS.Files[testPath("root/B.h")] = R"cpp(
+      #include "A.h"
+      void new_func();
+      )cpp";
+  // Check that B.h has been stored with new contents.
+  {
+    CacheHits = 0;
+    OverlayCDB CDB(/*Base=*/nullptr);
+    BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+                        [&](llvm::StringRef) { return &MSS; });
+    CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+    ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 3U);
+  ShardHeader = MSS.loadShard(testPath("root/B.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols,
+              Contains(AllOf(Named("new_func"), Declared(), Not(Defined()))));
+}
+
 } // namespace clangd
 } // namespace clang