[clangd] Make HadErrors part of background index's internal state

[sammccall]

Logic looks great, just naming/comment nits

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

274–275

, or we indexed successfully this time.

444–463

I'm happy with this inline or out-of-line, but if it's inline here it shouldn't have a function-style comment with \p :-)

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

98

I wonder whether a more "flavorful" name like "ShardVersion" might help explain the purpose here?

124–125

It's not obvious to me why we're apologising for this not being the include graph. The data happens to come from there (kind of) but I wouldn't really mention it.

125–126

rename mutex to match guarded variable

127

Again, I think "Versions" would make it more obvious what this is for.

[kadircet]

• Address comments

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

 //===--- Background.h - Build an index in a background thread ----*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
 
 #include "Context.h"
 #include "FSProvider.h"
 #include "GlobalCompilationDatabase.h"
+#include "SourceCode.h"
 #include "Threading.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/Threading.h"
public:
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds = 10);
 
   // Disables thread priority lowering in background index to make sure it can
   // progress on loaded systems. Only affects tasks that run after the call.
   static void preventThreadStarvationInTests();
 
 private:
+  /// Represents the state of a single file when indexing was performed.
+  struct ShardVersion {
+    FileDigest Digest{0};
+    bool HadErrors = false;
+  };
+
   /// Given index results from a TU, only update symbols coming from files with
-  /// different digests than \p DigestsSnapshot. Also stores new index
+  /// different digests than \p ShardVersionsSnapshot. Also stores new index
   /// information on IndexStorage.
   void update(llvm::StringRef MainFile, IndexFileIn Index,
-              const llvm::StringMap<FileDigest> &DigestsSnapshot,
+              const llvm::StringMap<ShardVersion> &ShardVersionsSnapshot,
               BackgroundIndexStorage *IndexStorage, bool HadErrors);
 
   // configuration
   const FileSystemProvider &FSProvider;
   const GlobalCompilationDatabase &CDB;
   Context BackgroundContext;
 
   // index state
   llvm::Error index(tooling::CompileCommand,
                     BackgroundIndexStorage *IndexStorage);
   void buildIndex(); // Rebuild index periodically every BuildIndexPeriodMs.
   const size_t BuildIndexPeriodMs;
   std::atomic<bool> SymbolsUpdatedSinceLastIndex;
   std::mutex IndexMu;
   std::condition_variable IndexCV;
 
   FileSymbols IndexedSymbols;
-  llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.
-  std::mutex DigestsMu;
+  llvm::StringMap<ShardVersion> ShardVersions; // Key is absolute file path.
+  std::mutex ShardVersionsMu;
 
   BackgroundIndexStorage::Factory IndexStorageFactory;
   struct Source {
     std::string Path;
     bool NeedsReIndexing;
     Source(llvm::StringRef Path, bool NeedsReIndexing)
         : Path(Path), NeedsReIndexing(NeedsReIndexing) {}
   };
   // Loads the shards for a single TU and all of its dependencies. Returns the

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

for (auto &Include : Node.DirectIncludes) {
     auto I = IG.try_emplace(Include).first;
     I->getValue().URI = I->getKey();
     Include = I->getKey();
   }
 
   return IG;
 }
 
-// Creates a filter to not collect index results from files with unchanged
-// digests.
-// \p FileDigests contains file digests for the current indexed files.
-decltype(SymbolCollector::Options::FileFilter)
-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.
-    auto AbsPath = getCanonicalPath(F, SM);
-    if (!AbsPath)
-      return false; // Skip files without absolute path.
-    auto Digest = digestFile(SM, FID);
-    if (!Digest)
-      return false;
-    auto D = FileDigests.find(*AbsPath);
-    if (D != FileDigests.end() && D->second == Digest)
-      return false; // Skip files that haven't changed.
-    return true;
-  };
-}
-
 // We cannot use vfs->makeAbsolute because Cmd.FileName is either absolute or
 // relative to Cmd.Directory, which might not be the same as current working
 // directory.
 llvm::SmallString<128> getAbsolutePath(const tooling::CompileCommand &Cmd) {
   llvm::SmallString<128> AbsolutePath;
   if (llvm::sys::path::is_absolute(Cmd.Filename)) {
     AbsolutePath = Cmd.Filename;
   } else {
if (Priority == llvm::ThreadPriority::Default) {
           });
     }
     Queue.insert(I, {std::move(T), Priority});
   }
   QueueCV.notify_all();
 }
 
 /// 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> &DigestsSnapshot,
-                             BackgroundIndexStorage *IndexStorage,
-                             bool HadErrors) {
+/// 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,
+    BackgroundIndexStorage *IndexStorage, bool HadErrors) {
   // Partition symbols/references into files.
   struct File {
     llvm::DenseSet<const Symbol *> Symbols;
     llvm::DenseSet<const Ref *> Refs;
     llvm::DenseSet<const Relation *> Relations;
     FileDigest Digest;
   };
   llvm::StringMap<File> Files;
   URIToFileCache URICache(MainFile);
   for (const auto &IndexIt : *Index.Sources) {
     const auto &IGN = IndexIt.getValue();
-    // In case of failures, we only store main file of TU. That way we can still
-    // get symbols from headers if some other TU can compile them. Note that
-    // sources do not contain any information regarding missing headers, since
-    // we don't even know what absolute path they should fall in.
-    // FIXME: Also store contents from other files whenever the current contents
-    // for those files are missing or if they had errors before.
-    if (HadErrors && !(IGN.Flags & IncludeGraphNode::SourceFlag::IsTU))
-      continue;
+    // Note that sources do not contain any information regarding missing
+    // headers, since we don't even know what absolute path they should fall in.
     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)
+    const auto DigestIt = ShardVersionsSnapshot.find(AbsPath);
+    // File has different contents, or indexing was successfull this time.
+    if (DigestIt == ShardVersionsSnapshot.end() ||
+        DigestIt->getValue().Digest != IGN.Digest ||
+        (DigestIt->getValue().HadErrors && !HadErrors))
       Files.try_emplace(AbsPath).first->getValue().Digest = IGN.Digest;
   }
   // This map is used to figure out where to store relations.
   llvm::DenseMap<SymbolID, File *> SymbolIDToFile;
   for (const auto &Sym : *Index.Symbols) {
     if (Sym.CanonicalDeclaration) {
       auto DeclPath = URICache.resolve(Sym.CanonicalDeclaration.FileURI);
       const auto FileIt = Files.find(DeclPath);
if (IndexStorage) {
         Shard.Cmd = Index.Cmd.getPointer();
 
       if (auto Error = IndexStorage->storeShard(Path, Shard))
         elog("Failed to write background-index shard for file {0}: {1}", Path,
              std::move(Error));
     }
 
     {
-      std::lock_guard<std::mutex> Lock(DigestsMu);
+      std::lock_guard<std::mutex> Lock(ShardVersionsMu);
       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)
+      auto DigestIt = ShardVersions.try_emplace(Path);
+      ShardVersion &SV = DigestIt.first->second;
+      // Skip if file is already up to date, unless previous index was broken
+      // and this one is not.
+      if (!DigestIt.second && SV.Digest == Hash && SV.HadErrors && !HadErrors)
         continue;
-      DigestIt.first->second = Hash;
+      SV.Digest = Hash;
+      SV.HadErrors = HadErrors;
+
       // This can override a newer version that is added in another thread, if
       // this thread sees the older version but finishes later. This should be
       // rare in practice.
       IndexedSymbols.update(Path, std::move(SS), std::move(RS), std::move(RelS),
                             Path == MainFile);
     }
   }
 }
llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd,
   auto AbsolutePath = getAbsolutePath(Cmd);
 
   auto FS = FSProvider.getFileSystem();
   auto Buf = FS->getBufferForFile(AbsolutePath);
   if (!Buf)
     return llvm::errorCodeToError(Buf.getError());
   auto Hash = digest(Buf->get()->getBuffer());
 
-  // Take a snapshot of the digests to avoid locking for each file in the TU.
-  llvm::StringMap<FileDigest> DigestsSnapshot;
+  // 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(DigestsMu);
-    DigestsSnapshot = IndexedFileDigests;
+    std::lock_guard<std::mutex> Lock(ShardVersionsMu);
+    ShardVersionsSnapshot = ShardVersions;
   }
 
   vlog("Indexing {0} (digest:={1})", Cmd.Filename, llvm::toHex(Hash));
   ParseInputs Inputs;
   Inputs.FS = std::move(FS);
   Inputs.FS->setCurrentWorkingDirectory(Cmd.Directory);
   Inputs.CompileCommand = std::move(Cmd);
   auto CI = buildCompilerInvocation(Inputs);
   if (!CI)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Couldn't build compiler invocation");
   IgnoreDiagnostics IgnoreDiags;
   auto Clang = prepareCompilerInstance(std::move(CI), /*Preamble=*/nullptr,
                                        std::move(*Buf), Inputs.FS, IgnoreDiags);
   if (!Clang)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Couldn't build compiler instance");
 
   SymbolCollector::Options IndexOpts;
-  IndexOpts.FileFilter = createFileFilter(DigestsSnapshot);
+  // 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.getFileEntryForID(FID);
+    if (!F)
+      return false; // Skip invalid files.
+    auto AbsPath = getCanonicalPath(F, SM);
+    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;
+  };
+
   IndexFileIn Index;
   auto Action = createStaticIndexingAction(
       IndexOpts, [&](SymbolSlab S) { Index.Symbols = std::move(S); },
       [&](RefSlab R) { Index.Refs = std::move(R); },
       [&](RelationSlab R) { Index.Relations = std::move(R); },
       [&](IncludeGraph IG) { Index.Sources = std::move(IG); });
 
   // We're going to run clang here, and it could potentially crash.
llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd,
 
   bool HadErrors = Clang->hasDiagnostics() &&
                    Clang->getDiagnostics().hasUncompilableErrorOccurred();
   if (HadErrors) {
     log("Failed to compile {0}, index may be incomplete", AbsolutePath);
     for (auto &It : *Index.Sources)
       It.second.Flags |= IncludeGraphNode::SourceFlag::HadErrors;
   }
-  update(AbsolutePath, std::move(Index), DigestsSnapshot, IndexStorage,
+  update(AbsolutePath, std::move(Index), ShardVersionsSnapshot, IndexStorage,
          HadErrors);
 
   if (BuildIndexPeriodMs > 0)
     SymbolsUpdatedSinceLastIndex = true;
   else
     reset(
         IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
 
   return llvm::Error::success();
 }
 
 std::vector<BackgroundIndex::Source>
 BackgroundIndex::loadShard(const tooling::CompileCommand &Cmd,
                            BackgroundIndexStorage *IndexStorage,
                            llvm::StringSet<> &LoadedShards) {
   struct ShardInfo {
     std::string AbsolutePath;
     std::unique_ptr<IndexFileIn> Shard;
     FileDigest Digest = {};
     bool CountReferences = false;
+    bool HadErrors = false;
   };
   std::vector<ShardInfo> IntermediateSymbols;
   // Make sure we don't have duplicate elements in the queue. Keys are absolute
   // paths.
   llvm::StringSet<> InQueue;
   auto FS = FSProvider.getFileSystem();
   // Dependencies of this TU, paired with the information about whether they
   // need to be re-indexed or not.
for (const auto &I : *Shard->Sources) {
       // We found source file info for current dependency.
       assert(I.getValue().Digest != FileDigest{{0}} && "Digest is empty?");
       ShardInfo SI;
       SI.AbsolutePath = CurDependency.Path;
       SI.Shard = std::move(Shard);
       SI.Digest = I.getValue().Digest;
       SI.CountReferences =
           I.getValue().Flags & IncludeGraphNode::SourceFlag::IsTU;
+      SI.HadErrors =
+          I.getValue().Flags & IncludeGraphNode::SourceFlag::HadErrors;
       IntermediateSymbols.push_back(std::move(SI));
       // Check if the source needs re-indexing.
       // Get the digest, skip it if file doesn't exist.
       auto Buf = FS->getBufferForFile(CurDependency.Path);
       if (!Buf) {
         elog("Couldn't get buffer for file: {0}: {1}", CurDependency.Path,
              Buf.getError().message());
         continue;
       }
       // If digests match then dependency doesn't need re-indexing.
       // FIXME: Also check for dependencies(sources) of this shard and compile
       // commands for cache invalidation.
       CurDependency.NeedsReIndexing =
           digest(Buf->get()->getBuffer()) != I.getValue().Digest;
     }
   }
   // Load shard information into background-index.
   {
-    std::lock_guard<std::mutex> Lock(DigestsMu);
+    std::lock_guard<std::mutex> Lock(ShardVersionsMu);
     // This can override a newer version that is added in another thread,
     // if this thread sees the older version but finishes later. This
     // should be rare in practice.
     for (const ShardInfo &SI : IntermediateSymbols) {
       auto SS =
           SI.Shard->Symbols
               ? llvm::make_unique<SymbolSlab>(std::move(*SI.Shard->Symbols))
               : nullptr;
       auto RS = SI.Shard->Refs
                     ? llvm::make_unique<RefSlab>(std::move(*SI.Shard->Refs))
                     : nullptr;
       auto RelS =
           SI.Shard->Relations
               ? llvm::make_unique<RelationSlab>(std::move(*SI.Shard->Relations))
               : nullptr;
-      IndexedFileDigests[SI.AbsolutePath] = SI.Digest;
+      ShardVersion &SV = ShardVersions[SI.AbsolutePath];
+      SV.Digest = SI.Digest;
+      SV.HadErrors = SI.HadErrors;
+
       IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS),
                             std::move(RelS), SI.CountReferences);
     }
   }
 
   return Dependencies;
 }
 

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

TEST_F(BackgroundIndexTest, UncompilableFiles) {
   void foo() {}
   )cpp";
   Cmd.Filename = "../A.cc";
   Cmd.Directory = testPath("build");
   Cmd.CommandLine = {"clang++", "../A.cc"};
   CDB.setCompileCommand(testPath("build/../A.cc"), Cmd);
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
 
-  // Make sure we only store the shard for main file.
-  EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc")));
+  EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc"), testPath("A.h"),
+                                          testPath("B.h"), testPath("C.h")));
+
+  {
     auto Shard = MSS.loadShard(testPath("A.cc"));
     EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));
     EXPECT_THAT(Shard->Sources->keys(),
                 UnorderedElementsAre("unittest:///A.cc", "unittest:///A.h",
                                      "unittest:///B.h"));
-
     EXPECT_THAT(Shard->Sources->lookup("unittest:///A.cc"), HadErrors());
-  // FIXME: We should also persist headers while marking them with errors.
-  EXPECT_THAT(Shard->Sources->lookup("unittest:///A.h"), Not(HadErrors()));
-  EXPECT_THAT(Shard->Sources->lookup("unittest:///B.h"), Not(HadErrors()));
+  }
+
+  {
+    auto Shard = MSS.loadShard(testPath("A.h"));
+    EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));
+    EXPECT_THAT(Shard->Sources->keys(),
+                UnorderedElementsAre("unittest:///A.h"));
+    EXPECT_THAT(Shard->Sources->lookup("unittest:///A.h"), HadErrors());
+  }
+
+  {
+    auto Shard = MSS.loadShard(testPath("B.h"));
+    EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("asdf")));
+    EXPECT_THAT(Shard->Sources->keys(),
+                UnorderedElementsAre("unittest:///B.h", "unittest:///C.h"));
+    EXPECT_THAT(Shard->Sources->lookup("unittest:///B.h"), HadErrors());
+  }
+
+  {
+    auto Shard = MSS.loadShard(testPath("C.h"));
+    EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre());
+    EXPECT_THAT(Shard->Sources->keys(),
+                UnorderedElementsAre("unittest:///C.h"));
+    EXPECT_THAT(Shard->Sources->lookup("unittest:///C.h"), HadErrors());
+  }
 }
 
 TEST_F(BackgroundIndexTest, CmdLineHash) {
   MockFSProvider FS;
   llvm::StringMap<std::string> Storage;
   size_t CacheHits = 0;
   MemoryShardStorage MSS(Storage, CacheHits);
   OverlayCDB CDB(/*Base=*/nullptr, /*FallbackFlags=*/{},