[clangd] Fix an assertion failure in background index.

[hokein]

Summary

When indexing a file which contains an uncompilable error, we will
trigger an assertion failure -- the IndexFileIn data is not set, but we
access them in the backgound index.

[kadircet]

I think the description is misleading, you are saying that we were triggering an assertion and you are fixing that. But in reality, we were not triggering any assertions, this patch is adding the assertions right?

clangd/index/Background.cpp

385 ↗

(On Diff #178036)

I don't think it is a good idea to assert and die just for one file, since we are actually trying to index all the files. We should rather check these exists and propagate the error in case of failure. You can have a look at https://reviews.llvm.org/D55224, same lines.

Also with your changes in the endsourcefileaction this assertion is not possible to be triggered, since you will fill them with "empty" versions.

clangd/index/IndexAction.cpp

144 ↗

(On Diff #178036)

I think this is same as if (!HasUncompilableErrors) so you can keep the old one just by negating it.

150 ↗

(On Diff #178036)

I don't think it is a good idea to respond with an "empty" symbol slab rather than not replying. They have different semantics. The former implies everything went well, just couldn't find any symbols in the file, whereas the latter implies there was an error.

So I believe we should keep this code as it was.

[hokein]

Keep the old behavior.

[hokein]

In D55650#1329692, @kadircet wrote:

I think the description is misleading, you are saying that we were triggering an assertion and you are fixing that. But in reality, we were not triggering any assertions, this patch is adding the assertions right?

Ah, sorry for the confusion caused by the assertion I added. The description is correct, and this patch is trying to fix an assertion (not the assertion I added, it is the assertion in llvm::Optional, we are accessing an empty Optional).

clangd/index/Background.cpp

385 ↗

(On Diff #178036)

check these exists seems a wired way to me, We should explicitly check the error here.

The assertion I added here is to guarantee that we have all the data when indexing a file successfully.

clangd/index/IndexAction.cpp

150 ↗

(On Diff #178036)

Sounds fair, I kept the old behavior.
but we have different behavior in clangd-indexer -- clangd-indexer will emit an empty index file when processing a problematic cpp file... a tradeoff is that we will reindex the problematic cpp file since we don't emit any index file, and the backgroundindex thinks this file is not being indexed.

[kadircet]

Ah you are right, the Optional.. Thanks!

clangd/index/Background.cpp

384 ↗

(On Diff #178203)

Then don't need to keep these checks anymore in the endsourcefileaction of indexingaction right?

[hokein]

clangd/index/Background.cpp

384 ↗

(On Diff #178203)

We need indeed. We have other clients (clangd-indexer, and our internal indexer) using the IndexingAction -- we don't want symbols from broken TUs because they causes many troublesomes.

[kadircet]

LGTM, thanks!

clangd/index/Background.cpp

384 ↗

(On Diff #178203)

Ah you are right.

[hokein]

Thanks for the review.

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

Error BackgroundIndex::index(tooling::CompileCommand Cmd,
 
   const FrontendInputFile &Input = Clang->getFrontendOpts().Inputs.front();
   if (!Action->BeginSourceFile(*Clang, Input))
     return createStringError(inconvertibleErrorCode(),
                              "BeginSourceFile() failed");
   if (!Action->Execute())
     return createStringError(inconvertibleErrorCode(), "Execute() failed");
   Action->EndSourceFile();
+  if (Clang->hasDiagnostics() &&
+      Clang->getDiagnostics().hasUncompilableErrorOccurred()) {
+    return createStringError(inconvertibleErrorCode(),
+                             "IndexingAction failed: has uncompilable errors");
+  }
+
+  assert(Index.Symbols && Index.Refs && Index.Sources
+     && "Symbols, Refs and Sources must be set.");
 
   log("Indexed {0} ({1} symbols, {2} refs, {3} files)",
       Inputs.CompileCommand.Filename, Index.Symbols->size(),
       Index.Refs->numRefs(), Index.Sources->size());
   SPAN_ATTACH(Tracer, "symbols", int(Index.Symbols->size()));
   SPAN_ATTACH(Tracer, "refs", int(Index.Refs->numRefs()));
   SPAN_ATTACH(Tracer, "sources", int(Index.Sources->size()));
 

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

public:
   }
 };
 
 class BackgroundIndexTest : public ::testing::Test {
 protected:
   BackgroundIndexTest() { preventThreadStarvationInTests(); }
 };
 
+TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.cc")] = "error file";
+  llvm::StringMap<std::string> Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+  OverlayCDB CDB(/*Base=*/nullptr);
+  BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+                      [&](llvm::StringRef) { return &MSS; });
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+
+  ASSERT_TRUE(Idx.blockUntilIdleForTest());
+}
+
 TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   MockFSProvider FS;
   // a.h yields different symbols when included by A.cc vs B.cc.
   FS.Files[testPath("root/A.h")] = R"cpp(
       void common();
       void f_b();
       #if A
         class A_CC {};