[clangd] Wire up ASTUnit and publish diagnostics with it.

[bkramer]

Summary

This requires an accessible compilation database. The parsing is done
asynchronously on a separate thread.

[cierpuchaw]

clangd/DocumentStore.h

42–49

Shouldn't this be called under a lock_guard?

[cierpuchaw]

clangd/DocumentStore.h

42–49

I should've reworded my comment before submitting it. By 'this' I'm referring to the 'Docs.erase()' part, not the whole function.

[ioeric]

clangd/ASTManager.cpp

22

Any reason not to wrap code in namespaces instead?

[krasimir]

clangd/DocumentStore.h

42–49

Even under a guard, it may potentially still be unsafe since workers may operate on erased/modified StringRef-s.

51–66

StringRef doesn't own the underlying data, right? What if I erase it in the meantime?

78

Same thing, shouldn't these be strings?

[arphaman]

clangd/ASTManager.cpp

70

This is just a suggestion based on personal opinion: RequestIsPending and Done can be merged into one enum value that uses an enum like this:

enum ASTManagerState {
  Waiting, ReceivedRequest, Done
}

Then this condition could be rewritten as if (state == Waiting).

71

I believe std::condition_variable may also be unblocked spuriously when wait is called without a predicate, which would lead to an empty File down below.

clangd/ASTManager.h

68

It looks like Done is always accessed in a scope where RequestLock is locked, so atomic doesn't seem needed here.

[krasimir]

clangd/ASTManager.cpp

149

Do you have an idea about a proper error handling if Error?

163

Then maybe we need a big disclaimer comment at the declaration of createASTUnitForFile?

clangd/ClangDMain.cpp

32

Why not directly Store.addListener(llvm::make_unique<ASTManager>(Out, Store));?

clangd/DocumentStore.h

39

Is only the main thread supposed to addDocument-s? Otherwise the listener notifications might need to be guarded.

[klimek]

clangd/ASTManager.cpp

70

I'm not so sure. Once we have a queue (iff we ever get a queue), the "request available" is an attribute of the queue, while the "we need to exit" is an attribute of the system.

71

Yep, +1, the general pattern is to put while loops around.
It might actually be simpler to write that with a queue from the start (note that this would make the request queueing side somewhat less simple):

... Lock(RequestLock);
while (Requests.empty() && !Done) {
  ClangRequestCV.wait(Lock);
}
if (Done) return;
File = Request.pop_front();
ClangRequestCV.notify_all();
...

the while loop  can be written somewhat simpler with the pred version:
ClangRequestCV.wait(Lock, [&] { return !Request.empty() || Done; });

117

One question is whether we want to couple this to JSON, or have structured output that we serialize.
(I'm happy with doing this now, and refining later - doesn't seem super hard to switch)

clangd/ASTManager.h

35

At a first glance, it's weird that we have a global document store, but get a document store for each notification. This at least needs documentation.

68

Yea, after only having read this header, it looks like we might want to pull out a Request as an abstraction.

clangd/ClangDMain.cpp

32

Why's the ASTManager not on the stack?

clangd/DocumentStore.h

39

It would always be the responsibility of the callee to guard.

42–49

That depends on the guarantees made, and when we create copies :)

[bkramer]

• Address review comments.

clangd/ASTManager.cpp

22

I don't really have an opinion one way or the other, but this seems to be the preferred way of doing this in LLVM.

71

Added the lambda. The current state is pretty weird, but making it a queue just to drop random elements from that queue is even weirder :(

149

For now I'll send it to logs()

clangd/ASTManager.h

35

This is leftover from an earlier design. Removed.

68

Wrapped all the guarded variables in a struct.

clangd/ClangDMain.cpp

32

Fixed. Lifetime is no longer managed by the DocumentStore (that was a bit weird)

clangd/DocumentStore.h

39

Added guard for now.

42–49

Added guard.

51–66

I guess it makes sense to create copies here. Somewhat sad but safe.

78

Done.

[cierpuchaw]

clangd/DocumentStore.h

42–49

'Docs.erase(Uri);' is still outside the critical section. It's easy to miss because of its placement directly after '{'.

[bkramer]

• Do not lock while running DocumentStore callbacks
• Replace fake queue with a real queue (but it still has at most one element.

[klimek]

clangd/ASTManager.h

68

For me that actually makes it worse (sorry): I now read "Request.Done" and think the request is done :)

[bkramer]

• Inline the request struct again.

[klimek]

lg

clangd/ASTManager.cpp

138–139

deque has resize *ducks and runs*

[alekseyshl]

http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/2437 is broken by this revision.

clangd/ASTManager.h

+//===--- ASTManager.h - Clang AST manager -----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_ASTMANAGER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_ASTMANAGER_H
+
+#include "DocumentStore.h"
+#include "JSONRPCDispatcher.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include <condition_variable>
+#include <queue>
+#include <thread>
+
+namespace clang {
+class ASTUnit;
+class DiagnosticsEngine;
+class PCHContainerOperations;
+namespace tooling {
+class CompilationDatabase;
+} // namespace tooling
+
+namespace clangd {
+
+class ASTManager : public DocumentStoreListener {
+public:
+  ASTManager(JSONOutput &Output, DocumentStore &Store);
+  ~ASTManager() override;
+
+  void onDocumentAdd(StringRef Uri) override;
+  // FIXME: Implement onDocumentRemove
+  // FIXME: Implement codeComplete
+
+private:
+  JSONOutput &Output;
+  DocumentStore &Store;
+
+  /// Loads a compilation database for URI. May return nullptr if it fails. The
+  /// database is cached for subsequent accesses.
+  clang::tooling::CompilationDatabase *
+  getOrCreateCompilationDatabaseForFile(StringRef Uri);
+  // Craetes a new ASTUnit for the document at Uri.
+  // FIXME: This calls chdir internally, which is thread unsafe.
+  std::unique_ptr<clang::ASTUnit>
+  createASTUnitForFile(StringRef Uri, const DocumentStore &Docs);
+
+  void runWorker();
+
+  /// Clang objects.
+  llvm::StringMap<std::unique_ptr<clang::ASTUnit>> ASTs;
+  llvm::StringMap<std::unique_ptr<clang::tooling::CompilationDatabase>>
+      CompilationDatabases;
+  std::shared_ptr<clang::PCHContainerOperations> PCHs;
+
+  /// We run parsing on a separate thread. This thread looks into PendingRequest
+  /// as a 'one element work queue' as long as RequestIsPending is true.
+  std::thread ClangWorker;
+  /// Queue of requests.
+  std::queue<std::string> RequestQueue;
+  /// Setting Done to true will make the worker thread terminate.
+  bool Done = false;
+  /// Condition variable to wake up the worker thread.
+  std::condition_variable ClangRequestCV;
+  /// Lock for accesses to RequestQueue and Done.
+  std::mutex RequestLock;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif

clangd/ASTManager.cpp

+//===--- ASTManager.cpp - Clang AST manager -------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ASTManager.h"
+#include "JSONRPCDispatcher.h"
+#include "Protocol.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/Support/Format.h"
+#include "llvm/Support/Path.h"
+#include <mutex>
+#include <thread>
+using namespace clang;
+using namespace clangd;
+
+/// Retrieve a copy of the contents of every file in the store, for feeding into
+/// ASTUnit.
+static std::vector<ASTUnit::RemappedFile>
+getRemappedFiles(const DocumentStore &Docs) {
+  // FIXME: Use VFS instead. This would allow us to get rid of the chdir below.
+  std::vector<ASTUnit::RemappedFile> RemappedFiles;
+  for (const auto &P : Docs.getAllDocuments()) {
+    StringRef FileName = P.first;
+    FileName.consume_front("file://");
+    RemappedFiles.push_back(ASTUnit::RemappedFile(
+        FileName,
+        llvm::MemoryBuffer::getMemBufferCopy(P.second, FileName).release()));
+  }
+  return RemappedFiles;
+}
+
+/// Convert from clang diagnostic level to LSP severity.
+static int getSeverity(DiagnosticsEngine::Level L) {
+  switch (L) {
+  case DiagnosticsEngine::Remark:
+    return 4;
+  case DiagnosticsEngine::Note:
+    return 3;
+  case DiagnosticsEngine::Warning:
+    return 2;
+  case DiagnosticsEngine::Fatal:
+  case DiagnosticsEngine::Error:
+    return 1;
+  case DiagnosticsEngine::Ignored:
+    return 0;
+  }
+}
+
+ASTManager::ASTManager(JSONOutput &Output, DocumentStore &Store)
+    : Output(Output), Store(Store),
+      PCHs(std::make_shared<PCHContainerOperations>()),
+      ClangWorker([this]() { runWorker(); }) {}
+
+void ASTManager::runWorker() {
+  while (true) {
+    std::string File;
+
+    {
+      std::unique_lock<std::mutex> Lock(RequestLock);
+      // Check if there's another request pending. We keep parsing until
+      // our one-element queue is empty.
+      ClangRequestCV.wait(Lock, [this] {
+        return !RequestQueue.empty() || Done;
+      });
+
+      if (RequestQueue.empty() && Done)
+        return;
+
+      File = std::move(RequestQueue.back());
+      RequestQueue.pop();
+    } // unlock.
+
+    auto &Unit = ASTs[File]; // Only one thread can access this at a time.
+
+    if (!Unit) {
+      Unit = createASTUnitForFile(File, this->Store);
+    } else {
+      // Do a reparse if this wasn't the first parse.
+      // FIXME: This might have the wrong working directory if it changed in the
+      // meantime.
+      Unit->Reparse(PCHs, getRemappedFiles(this->Store));
+    }
+
+    if (!Unit)
+      continue;
+
+    // Send the diagnotics to the editor.
+    // FIXME: If the diagnostic comes from a different file, do we want to
+    // show them all? Right now we drop everything not coming from the
+    // main file.
+    // FIXME: Send FixIts to the editor.
+    std::string Diagnostics;
+    for (ASTUnit::stored_diag_iterator D = Unit->stored_diag_begin(),
+                                       DEnd = Unit->stored_diag_end();
+         D != DEnd; ++D) {
+      if (!D->getLocation().isValid() ||
+          !D->getLocation().getManager().isInMainFile(D->getLocation()))
+        continue;
+      Position P;
+      P.line = D->getLocation().getSpellingLineNumber() - 1;
+      P.character = D->getLocation().getSpellingColumnNumber();
+      Range R = {P, P};
+      Diagnostics +=
+          R"({"range":)" + Range::unparse(R) +
+          R"(,"severity":)" + std::to_string(getSeverity(D->getLevel())) +
+          R"(,"message":")" + llvm::yaml::escape(D->getMessage()) +
+          R"("},)";
+    }
+
+    if (!Diagnostics.empty())
+      Diagnostics.pop_back(); // Drop trailing comma.
+    Output.writeMessage(
+        R"({"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":")" +
+        File + R"(","diagnostics":[)" + Diagnostics + R"(]}})");
+  }
+}
+
+ASTManager::~ASTManager() {
+  {
+    std::lock_guard<std::mutex> Guard(RequestLock);
+    // Wake up the clang worker thread, then exit.
+    Done = true;
+    ClangRequestCV.notify_one();
+  }
+  ClangWorker.join();
+}
+
+void ASTManager::onDocumentAdd(StringRef Uri) {
+  std::lock_guard<std::mutex> Guard(RequestLock);
+  // Currently we discard all pending requests and just enqueue the latest one.
+  while (!RequestQueue.empty())
+    RequestQueue.pop();
+  RequestQueue.push(Uri);
+  ClangRequestCV.notify_one();
+}
+
+tooling::CompilationDatabase *
+ASTManager::getOrCreateCompilationDatabaseForFile(StringRef Uri) {
+  auto &I = CompilationDatabases[Uri];
+  if (I)
+    return I.get();
+
+  Uri.consume_front("file://");
+
+  std::string Error;
+  I = tooling::CompilationDatabase::autoDetectFromSource(Uri, Error);
+  Output.logs() << "Failed to load compilation database: " << Error << '\n';
+  return I.get();
+}
+
+std::unique_ptr<clang::ASTUnit>
+ASTManager::createASTUnitForFile(StringRef Uri, const DocumentStore &Docs) {
+  tooling::CompilationDatabase *CDB =
+      getOrCreateCompilationDatabaseForFile(Uri);
+
+  Uri.consume_front("file://");
+  std::vector<tooling::CompileCommand> Commands;
+
+  if (CDB) {
+    Commands = CDB->getCompileCommands(Uri);
+    // chdir. This is thread hostile.
+    if (!Commands.empty())
+      llvm::sys::fs::set_current_path(Commands.front().Directory);
+  }
+  if (Commands.empty()) {
+    // Add a fake command line if we know nothing.
+    Commands.push_back(tooling::CompileCommand(
+        llvm::sys::path::parent_path(Uri), llvm::sys::path::filename(Uri),
+        {"clang", "-fsyntax-only", Uri.str()}, ""));
+  }
+
+  // Inject the resource dir.
+  // FIXME: Don't overwrite it if it's already there.
+  static int Dummy; // Just an address in this process.
+  std::string ResourceDir =
+      CompilerInvocation::GetResourcesPath("clangd", (void *)&Dummy);
+  Commands.front().CommandLine.push_back("-resource-dir=" + ResourceDir);
+
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+      CompilerInstance::createDiagnostics(new DiagnosticOptions);
+
+  std::vector<const char *> ArgStrs;
+  for (const auto &S : Commands.front().CommandLine)
+    ArgStrs.push_back(S.c_str());
+
+  return std::unique_ptr<clang::ASTUnit>(ASTUnit::LoadFromCommandLine(
+      &*ArgStrs.begin(), &*ArgStrs.end(), PCHs, Diags, ResourceDir,
+      /*OnlyLocalDecls=*/false, /*CaptureDiagnostics=*/true,
+      getRemappedFiles(Docs),
+      /*RemappedFilesKeepOriginalName=*/true,
+      /*PrecompilePreambleAfterNParses=*/1, /*TUKind=*/TU_Complete,
+      /*CacheCodeCompletionResults=*/true,
+      /*IncludeBriefCommentsInCodeCompletion=*/true,
+      /*AllowPCHWithCompilerErrors=*/true));
+}

clangd/CMakeLists.txt

 add_clang_executable(clangd
+  ASTManager.cpp
   ClangDMain.cpp
   JSONRPCDispatcher.cpp
   Protocol.cpp
   ProtocolHandlers.cpp
   )
 
 target_link_libraries(clangd
   clangBasic
   clangFormat
+  clangFrontend
+  clangTooling
   LLVMSupport
   )

clangd/ClangDMain.cpp

 //===--- ClangDMain.cpp - clangd server loop ------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "ASTManager.h"
 #include "DocumentStore.h"
 #include "JSONRPCDispatcher.h"
 #include "ProtocolHandlers.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Program.h"
 #include <iostream>
 #include <string>
 using namespace clang::clangd;
 
 int main(int argc, char *argv[]) {
   llvm::raw_ostream &Outs = llvm::outs();
   llvm::raw_ostream &Logs = llvm::errs();
   JSONOutput Out(Outs, Logs);
 
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
 
   // Set up a document store and intialize all the method handlers for JSONRPC
   // dispatching.
   DocumentStore Store;
+  ASTManager AST(Out, Store);
+  Store.addListener(&AST);
   JSONRPCDispatcher Dispatcher(llvm::make_unique<Handler>(Out));
   Dispatcher.registerHandler("initialize",
                              llvm::make_unique<InitializeHandler>(Out));
   auto ShutdownPtr = llvm::make_unique<ShutdownHandler>(Out);
   auto *ShutdownHandler = ShutdownPtr.get();
   Dispatcher.registerHandler("shutdown",std::move(ShutdownPtr));
   Dispatcher.registerHandler(
       "textDocument/didOpen",

clangd/DocumentStore.h

 //===--- DocumentStore.h - File contents container --------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_DOCUMENTSTORE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_DOCUMENTSTORE_H
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringMap.h"
+#include <mutex>
 #include <string>
 
 namespace clang {
 namespace clangd {
+class DocumentStore;
+
+struct DocumentStoreListener {
+  virtual ~DocumentStoreListener() = default;
+  virtual void onDocumentAdd(StringRef Uri) {}
+  virtual void onDocumentRemove(StringRef Uri) {}
+};
 
 /// A container for files opened in a workspace, addressed by URI. The contents
 /// are owned by the DocumentStore.
 class DocumentStore {
 public:
   /// Add a document to the store. Overwrites existing contents.
-  void addDocument(StringRef Uri, StringRef Text) { Docs[Uri] = Text; }
+  void addDocument(StringRef Uri, StringRef Text) {
+    {
+      std::lock_guard<std::mutex> Guard(DocsMutex);
+      Docs[Uri] = Text;
+    }
+    for (const auto &Listener : Listeners)
+      Listener->onDocumentAdd(Uri);
+  }
   /// Delete a document from the store.
-  void removeDocument(StringRef Uri) { Docs.erase(Uri); }
+  void removeDocument(StringRef Uri) {
+    {
+      std::lock_guard<std::mutex> Guard(DocsMutex);
+      Docs.erase(Uri);
+    }
+    for (const auto &Listener : Listeners)
+      Listener->onDocumentRemove(Uri);
+  }
   /// Retrieve a document from the store. Empty string if it's unknown.
-  StringRef getDocument(StringRef Uri) const {
-    auto I = Docs.find(Uri);
-    return I == Docs.end() ? StringRef("") : StringRef(I->second);
+  std::string getDocument(StringRef Uri) const {
+    // FIXME: This could be a reader lock.
+    std::lock_guard<std::mutex> Guard(DocsMutex);
+    return Docs.lookup(Uri);
+  }
+
+  /// Add a listener. Does not take ownership.
+  void addListener(DocumentStoreListener *DSL) { Listeners.push_back(DSL); }
+
+  /// Get name and constents of all documents in this store.
+  std::vector<std::pair<std::string, std::string>> getAllDocuments() const {
+    std::vector<std::pair<std::string, std::string>> AllDocs;
+    std::lock_guard<std::mutex> Guard(DocsMutex);
+    for (const auto &P : Docs)
+      AllDocs.emplace_back(P.first(), P.second);
+    return AllDocs;
   }
 
 private:
   llvm::StringMap<std::string> Docs;
+  std::vector<DocumentStoreListener *> Listeners;
+
+  mutable std::mutex DocsMutex;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

test/clangd/diagnostics.test

+# RUN: clangd < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+#
+Content-Length: 152
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"void main() {}"}}}
+#
+# CHECK: {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///foo.c","diagnostics":[{"range":{"start": {"line": 0, "character": 1}, "end": {"line": 0, "character": 1}},"severity":2,"message":"return type of 'main' is not 'int'"},{"range":{"start": {"line": 0, "character": 1}, "end": {"line": 0, "character": 1}},"severity":3,"message":"change return type to 'int'"}]}}
+#
+#
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}