[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 ↗

(On Diff #88171)

Shouldn't this be called under a lock_guard?

[cierpuchaw]

clangd/DocumentStore.h

42 ↗

(On Diff #88171)

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

21 ↗

(On Diff #88171)

Any reason not to wrap code in namespaces instead?

[krasimir]

clangd/DocumentStore.h

42 ↗

(On Diff #88171)

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

51 ↗

(On Diff #88171)

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

63 ↗

(On Diff #88171)

Same thing, shouldn't these be strings?

[arphaman]

clangd/ASTManager.cpp

69 ↗

(On Diff #88171)

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).

70 ↗

(On Diff #88171)

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

67 ↗

(On Diff #88171)

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

148 ↗

(On Diff #88171)

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

162 ↗

(On Diff #88171)

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

clangd/ClangDMain.cpp

32 ↗

(On Diff #88171)

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

clangd/DocumentStore.h

39 ↗

(On Diff #88171)

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

[klimek]

clangd/ASTManager.cpp

69 ↗

(On Diff #88171)

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.

70 ↗

(On Diff #88171)

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; });

116 ↗

(On Diff #88171)

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

34 ↗

(On Diff #88171)

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.

67 ↗

(On Diff #88171)

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 ↗

(On Diff #88171)

Why's the ASTManager not on the stack?

clangd/DocumentStore.h

39 ↗

(On Diff #88171)

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

42 ↗

(On Diff #88171)

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

[bkramer]

• Address review comments.

clangd/ASTManager.cpp

21 ↗

(On Diff #88171)

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.

70 ↗

(On Diff #88171)

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 :(

148 ↗

(On Diff #88171)

For now I'll send it to logs()

clangd/ASTManager.h

34 ↗

(On Diff #88171)

This is leftover from an earlier design. Removed.

67 ↗

(On Diff #88171)

Wrapped all the guarded variables in a struct.

clangd/ClangDMain.cpp

32 ↗

(On Diff #88171)

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

clangd/DocumentStore.h

39 ↗

(On Diff #88171)

Added guard for now.

42 ↗

(On Diff #88171)

Added guard.

51 ↗

(On Diff #88171)

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

63 ↗

(On Diff #88171)

Done.

[cierpuchaw]

clangd/DocumentStore.h

42 ↗

(On Diff #88171)

'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

67 ↗

(On Diff #88171)

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 ↗

(On Diff #88523)

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.

clang-tools-extra/trunk/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 <deque>
+#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::deque<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

clang-tools-extra/trunk/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_back();
+    } // 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.
+  RequestQueue.clear();
+  RequestQueue.push_back(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));
+}

clang-tools-extra/trunk/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
   )

clang-tools-extra/trunk/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",

clang-tools-extra/trunk/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

clang-tools-extra/trunk/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"}