[clangd] Drop the optimization where only shards for modified files are updated in the background index

The optimization is not valid, given that the target of a symbol
reference can change without the spelling of the reference (or
anything else in the file containing the reference) changing,
as illustrated in the added test case.

Partially fixes clangd/clangd#1104

Author: HighCommander4

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

@@ -180,13 +180,10 @@ void BackgroundIndex::boostRelated(llvm::StringRef Path) {
     Queue.boost(filenameWithoutExtension(Path), IndexBoostedFile);
 }
 
-/// Given index results from a TU, only update symbols coming from files that
-/// are different or missing from than \p ShardVersionsSnapshot. Also stores new
-/// index information on IndexStorage.
-void BackgroundIndex::update(
-    llvm::StringRef MainFile, IndexFileIn Index,
-    const llvm::StringMap<ShardVersion> &ShardVersionsSnapshot,
-    bool HadErrors) {
+/// Given index results from a TU, update symbols coming from all files. Also
+/// stores new index information on IndexStorage.
+void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
+                             bool HadErrors) {
   // Keys are URIs.
   llvm::StringMap<std::pair<Path, FileDigest>> FilesToUpdate;
   // Note that sources do not contain any information regarding missing headers,
@@ -198,12 +195,7 @@ void BackgroundIndex::update(
       elog("Failed to resolve URI: {0}", AbsPath.takeError());
       continue;
     }
-    const auto DigestIt = ShardVersionsSnapshot.find(*AbsPath);
-    // File has different contents, or indexing was successful this time.
-    if (DigestIt == ShardVersionsSnapshot.end() ||
-        DigestIt->getValue().Digest != IGN.Digest ||
-        (DigestIt->getValue().HadErrors && !HadErrors))
-      FilesToUpdate[IGN.URI] = {std::move(*AbsPath), IGN.Digest};
+    FilesToUpdate[IGN.URI] = {std::move(*AbsPath), IGN.Digest};
   }
 
   // Shard slabs into files.
@@ -263,13 +255,6 @@ llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd) {
     return llvm::errorCodeToError(Buf.getError());
   auto Hash = digest(Buf->get()->getBuffer());
 
-  // Take a snapshot of the versions to avoid locking for each file in the TU.
-  llvm::StringMap<ShardVersion> ShardVersionsSnapshot;
-  {
-    std::lock_guard<std::mutex> Lock(ShardVersionsMu);
-    ShardVersionsSnapshot = ShardVersions;
-  }
-
   vlog("Indexing {0} (digest:={1})", Cmd.Filename, llvm::toHex(Hash));
   ParseInputs Inputs;
   Inputs.TFS = &TFS;
@@ -286,25 +271,6 @@ llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd) {
     return error("Couldn't build compiler instance");
 
   SymbolCollector::Options IndexOpts;
-  // Creates a filter to not collect index results from files with unchanged
-  // digests.
-  IndexOpts.FileFilter = [&ShardVersionsSnapshot](const SourceManager &SM,
-                                                  FileID FID) {
-    const auto F = SM.getFileEntryRefForID(FID);
-    if (!F)
-      return false; // Skip invalid files.
-    auto AbsPath = getCanonicalPath(*F, SM.getFileManager());
-    if (!AbsPath)
-      return false; // Skip files without absolute path.
-    auto Digest = digestFile(SM, FID);
-    if (!Digest)
-      return false;
-    auto D = ShardVersionsSnapshot.find(*AbsPath);
-    if (D != ShardVersionsSnapshot.end() && D->second.Digest == Digest &&
-        !D->second.HadErrors)
-      return false; // Skip files that haven't changed, without errors.
-    return true;
-  };
   IndexOpts.CollectMainFileRefs = true;
 
   IndexFileIn Index;
@@ -345,7 +311,7 @@ llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd) {
     for (auto &It : *Index.Sources)
       It.second.Flags |= IncludeGraphNode::SourceFlag::HadErrors;
   }
-  update(AbsolutePath, std::move(Index), ShardVersionsSnapshot, HadErrors);
+  update(AbsolutePath, std::move(Index), HadErrors);
 
   Rebuilder.indexedTU();
   return llvm::Error::success();

clang-tools-extra/clangd/index/Background.h

@@ -190,12 +190,9 @@ class BackgroundIndex : public SwapIndex {
     bool HadErrors = false;
   };
 
-  /// Given index results from a TU, only update symbols coming from files with
-  /// different digests than \p ShardVersionsSnapshot. Also stores new index
-  /// information on IndexStorage.
-  void update(llvm::StringRef MainFile, IndexFileIn Index,
-              const llvm::StringMap<ShardVersion> &ShardVersionsSnapshot,
-              bool HadErrors);
+  /// Given index results from a TU, update symbols coming from all files. Also
+  /// stores new index information on IndexStorage.
+  void update(llvm::StringRef MainFile, IndexFileIn Index, bool HadErrors);
 
   // configuration
   const ThreadsafeFS &TFS;

clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp

@@ -14,6 +14,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <deque>
+#include <optional>
 #include <thread>
 
 using ::testing::_;
@@ -213,10 +214,11 @@ TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   CDB.setCompileCommand(testPath("root/B.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
-  // B_CC is dropped as we don't collect symbols from A.h in this compilation.
+  // A_CC is dropped as A.h was now most recently indexed in the context of
+  // B.cc.
   EXPECT_THAT(runFuzzyFind(Idx, ""),
               UnorderedElementsAre(AllOf(named("common"), numReferences(5U)),
-                                   AllOf(named("A_CC"), numReferences(0U)),
+                                   AllOf(named("B_CC"), numReferences(0U)),
                                    AllOf(named("g"), numReferences(1U)),
                                    AllOf(named("f_b"), declared(), defined(),
                                          numReferences(1U))));
@@ -682,6 +684,76 @@ TEST_F(BackgroundIndexTest, Reindex) {
   EXPECT_EQ(OldShard, Storage.lookup(testPath("A.cc")));
 }
 
+// Test that restarting clangd properly updates the index with changes
+// to files since the last time the index was built.
+TEST_F(BackgroundIndexTest, UpdateAfterRestart) {
+  MockFS FS;
+  llvm::StringMap<std::string> Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+  auto CDB = std::make_unique<OverlayCDB>(/*Base=*/nullptr);
+  auto Idx = std::make_unique<BackgroundIndex>(
+      FS, *CDB, [&](llvm::StringRef) { return &MSS; },
+      BackgroundIndex::Options{});
+
+  // Create a header file containing a function declaration, and a source file
+  // containing a call to the function.
+  FS.Files[testPath("test.h")] = R"cpp(
+    #ifndef TEST_H
+    #define TEST_H
+    void waldo(int);
+    #endif
+  )cpp";
+  FS.Files[testPath("test.cc")] = R"cpp(
+    #include "test.h"
+    int main() {
+      waldo(42);
+    }
+  )cpp";
+
+  // Index the files in this state.
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = "../test.cc";
+  Cmd.Directory = testPath("build");
+  Cmd.CommandLine = {"clang++", "../test.cc", "-fsyntax-only"};
+  CDB->setCompileCommand(testPath("test.cc"), Cmd);
+  ASSERT_TRUE(Idx->blockUntilIdleForTest());
+
+  // Verify that the function 'waldo' has two references in the index
+  // (the declaration, and the call site).
+  auto CheckRefCount = [&](std::string SymbolName) {
+    auto Syms = runFuzzyFind(*Idx, SymbolName);
+    EXPECT_THAT(Syms, UnorderedElementsAre(named(SymbolName)));
+    auto Sym = *Syms.begin();
+    return getRefs(*Idx, Sym.ID).numRefs();
+  };
+  EXPECT_EQ(CheckRefCount("waldo"), 2u);
+
+  // Modify the declaration of 'waldo' in a way that changes its SymbolID
+  // without changing how existing call sites are written. Here, we add
+  // a new parameter with a default argument.
+  FS.Files[testPath("test.h")] = R"cpp(
+    #ifndef TEST_H
+    #define TEST_H
+    void waldo(int, int = 0);
+    #endif
+  )cpp";
+
+  // Simulate clangd shutting down and restarting, and the background index
+  // being rebuilt after restart.
+  Idx = nullptr;
+  CDB = std::make_unique<OverlayCDB>(/*Base=*/nullptr);
+  Idx = std::make_unique<BackgroundIndex>(
+      FS, *CDB, [&](llvm::StringRef) { return &MSS; },
+      BackgroundIndex::Options{});
+  CDB->setCompileCommand(testPath("test.cc"), Cmd);
+  ASSERT_TRUE(Idx->blockUntilIdleForTest());
+
+  // The rebuild should have updated things so that 'waldo' now again has
+  // two references in the index.
+  EXPECT_EQ(CheckRefCount("waldo"), 2u);
+}
+
 class BackgroundIndexRebuilderTest : public testing::Test {
 protected:
   BackgroundIndexRebuilderTest()