[clangd] Fix crash when CompilerInvocation can't be created.

[ilya-biryukov]

Summary

This can happen if the CompileCommand provided by compilation database
is malformed.

[sammccall]

just nits

clangd/ClangdUnit.cpp

417

remove?

unittests/clangd/ClangdTests.cpp

47

doc that this consumes Expected<T>

48

This helper should go in Matchers.h I think

52

More context:

<< " from " << #expectedValue << " but got " << ::testing::PrintToString(*ComputedValue)

(please check the error message works!)

[ilya-biryukov]

• Removed assert that is now redundant
• Moved EXPECT_ERROR to Matchers.h
• Added more context into EXPECT_ERROR's error message

clangd/ClangdUnit.cpp

std::unique_ptr<CompilerInvocation> CI;
     // FIXME(ibiryukov): store diagnostics from CommandLine when we start
     // reporting them.
     IgnoreDiagnostics IgnoreDiagnostics;
     IntrusiveRefCntPtr<DiagnosticsEngine> CommandLineDiagsEngine =
         CompilerInstance::createDiagnostics(new DiagnosticOptions,
                                             &IgnoreDiagnostics, false);
     CI = createInvocationFromCommandLine(ArgStrs, CommandLineDiagsEngine,
                                          Inputs.FS);
+    if (!CI) {
+      log("Could not build CompilerInvocation for file " + FileName);
+      AST = llvm::None;
+      Preamble = nullptr;
+      return llvm::None;
+    }
     // createInvocationFromCommandLine sets DisableFree.
     CI->getFrontendOpts().DisableFree = false;
   }
   assert(CI && "Couldn't create CompilerInvocation");
 
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, FileName);
 
   // Compute updated Preamble.
   std::shared_ptr<const PreambleData> NewPreamble =
       rebuildPreamble(*CI, Inputs.FS, *ContentsBuffer);
 

clangd/CodeComplete.cpp

bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
   Input.VFS->setCurrentWorkingDirectory(Input.Command.Directory);
 
   IgnoreDiagnostics DummyDiagsConsumer;
   auto CI = createInvocationFromCommandLine(
       ArgStrs,
       CompilerInstance::createDiagnostics(new DiagnosticOptions,
                                           &DummyDiagsConsumer, false),
       Input.VFS);
-  assert(CI && "Couldn't create CompilerInvocation");
+  if (!CI) {
+    log("Couldn't create CompilerInvocation");;
+    return false;
+  }
   CI->getFrontendOpts().DisableFree = false;
 
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Input.Contents, Input.FileName);
 
   // We reuse the preamble whether it's valid or not. This is a
   // correctness/performance tradeoff: building without a preamble is slow, and
   // completion is latency-sensitive.

unittests/clangd/ClangdTests.cpp

 using ::testing::Eq;
 using ::testing::Gt;
 using ::testing::IsEmpty;
 using ::testing::Pair;
 using ::testing::UnorderedElementsAre;
 
 namespace {
 
+// EXPECT_ERROR seems like a pretty generic name, make sure it's not defined
+// already.
+#ifdef EXPECT_ERROR
+#error "Refusing to redefine EXPECT_ERROR"
+#endif
+
+// Checks there is an error and marks it as handled.
+#define EXPECT_ERROR(expectedValue)                                            \
+  do {                                                                         \
+    auto &&ComputedValue = (expectedValue);                                    \
+    if (ComputedValue) {                                                       \
+      ADD_FAILURE() << "expected an error";                                    \
+      break;                                                                   \
+    }                                                                          \
+    handleAllErrors(ComputedValue.takeError(),                                 \
+                    [](const llvm::ErrorInfoBase &) {});                       \
+                                                                               \
+  } while (false)
+
 // Don't wait for async ops in clangd test more than that to avoid blocking
 // indefinitely in case of bugs.
 static const std::chrono::seconds DefaultFutureTimeout =
     std::chrono::seconds(10);
 
 static bool diagsContainErrors(ArrayRef<DiagWithFixIts> Diagnostics) {
   for (const auto &DiagAndFixIts : Diagnostics) {
     // FIXME: severities returned by clangd should have a descriptive
)cpp";
 
   Server.removeDocument(FooCpp);
   EXPECT_THAT(Server.getUsedBytesPerFile(), ElementsAre(Pair(BarCpp, Gt(0u))));
 
   Server.removeDocument(BarCpp);
   EXPECT_THAT(Server.getUsedBytesPerFile(), IsEmpty());
 }
 
+TEST_F(ClangdVFSTest, InvalidCompileCommand) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, DiagConsumer, FS,
+                      /*AsyncThreadsCount=*/0,
+                      /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  // clang cannot create CompilerInvocation if we pass two files in the
+  // CompileCommand. We pass the file in ExtraFlags once and CDB adds another
+  // one in getCompileCommand().
+  CDB.ExtraClangFlags.push_back(FooCpp.str());
+
+  // Clang can't parse command args in that case, but we shouldn't crash.
+  Server.addDocument(FooCpp, "int main() {}");
+
+  EXPECT_EQ(Server.dumpAST(FooCpp), "<no-ast>");
+  EXPECT_ERROR(Server.findDefinitions(FooCpp, Position{0, 0}));
+  EXPECT_ERROR(Server.findDocumentHighlights(FooCpp, Position{0, 0}));
+  EXPECT_ERROR(Server.rename(FooCpp, Position{0, 0}, "new_name"));
+  // FIXME: codeComplete and signatureHelp should also return errors when they
+  // can't parse the file.
+  EXPECT_THAT(
+      Server.codeComplete(FooCpp, Position{0, 0}, clangd::CodeCompleteOptions())
+          .get()
+          .Value.items,
+      IsEmpty());
+  EXPECT_THAT(
+      Server.signatureHelp(FooCpp, Position{0, 0}).get().Value.signatures,
+      IsEmpty());
+}
+
 class ClangdThreadingTest : public ClangdVFSTest {};
 
 TEST_F(ClangdThreadingTest, StressTest) {
   // Without 'static' clang gives an error for a usage inside TestDiagConsumer.
   static const unsigned FilesCount = 5;
   const unsigned RequestsCount = 500;
   // Blocking requests wait for the parsing to complete, they slow down the test
   // dramatically, so they are issued rarely. Each