[clangd] Allow embedders some control over when diagnostics are gener…

…ated.

Summary:
Through the C++ API, we support for a given snapshot version:
 - Yes: make sure we generate diagnostics for exactly this version
 - Auto: generate eventually-consistent diagnostics for at least this version
 - No: don't generate diagnostics for this version
Eventually auto should be debounced for better UX.

Through LSP, we force diagnostics for initial load (bypassing future debouncing)
and all updates follow the "auto" policy.

This is complicated to implement under the CancellationFlag design, so
rewrote that part to just inspect the queue instead.

It turns out we never pass None to the diagnostics callback, so remove Optional
from the signature. The questionable behavior of not invoking the callback at
all if CppFile::rebuild fails is not changed.

Reviewers: ilya-biryukov

Subscribers: klimek, jkorous-apple, ioeric, cfe-commits

Differential Revision: https://reviews.llvm.org/D43518

llvm-svn: 325774

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -141,7 +141,8 @@ void ClangdLSPServer::onDocumentDidOpen(DidOpenTextDocumentParams &Params) {
   if (Params.metadata && !Params.metadata->extraFlags.empty())
     CDB.setExtraFlagsForFile(Params.textDocument.uri.file(),
                              std::move(Params.metadata->extraFlags));
-  Server.addDocument(Params.textDocument.uri.file(), Params.textDocument.text);
+  Server.addDocument(Params.textDocument.uri.file(), Params.textDocument.text,
+                     WantDiagnostics::Yes);
 }
 
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
@@ -150,7 +151,7 @@ void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
                       "can only apply one change at a time");
   // We only support full syncing right now.
   Server.addDocument(Params.textDocument.uri.file(),
-                     Params.contentChanges[0].text);
+                     Params.contentChanges[0].text, WantDiagnostics::Auto);
 }
 
 void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams &Params) {

clang-tools-extra/clangd/ClangdServer.cpp

@@ -110,11 +110,12 @@ void ClangdServer::setRootPath(PathRef RootPath) {
     this->RootPath = NewRootPath;
 }
 
-void ClangdServer::addDocument(PathRef File, StringRef Contents) {
+void ClangdServer::addDocument(PathRef File, StringRef Contents,
+                               WantDiagnostics WantDiags) {
   DocVersion Version = DraftMgr.updateDraft(File, Contents);
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
   scheduleReparseAndDiags(File, VersionedDraft{Version, Contents.str()},
-                          std::move(TaggedFS));
+                          WantDiags, std::move(TaggedFS));
 }
 
 void ClangdServer::removeDocument(PathRef File) {
@@ -133,7 +134,8 @@ void ClangdServer::forceReparse(PathRef File) {
   CompileArgs.invalidate(File);
 
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
-  scheduleReparseAndDiags(File, std::move(FileContents), std::move(TaggedFS));
+  scheduleReparseAndDiags(File, std::move(FileContents), WantDiagnostics::Yes,
+                          std::move(TaggedFS));
 }
 
 void ClangdServer::codeComplete(
@@ -519,20 +521,16 @@ void ClangdServer::findHover(
 }
 
 void ClangdServer::scheduleReparseAndDiags(
-    PathRef File, VersionedDraft Contents,
+    PathRef File, VersionedDraft Contents, WantDiagnostics WantDiags,
     Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS) {
   tooling::CompileCommand Command = CompileArgs.getCompileCommand(File);
 
-  using OptDiags = llvm::Optional<std::vector<DiagWithFixIts>>;
-
   DocVersion Version = Contents.Version;
   Path FileStr = File.str();
   VFSTag Tag = std::move(TaggedFS.Tag);
 
-  auto Callback = [this, Version, FileStr, Tag](OptDiags Diags) {
-    if (!Diags)
-      return; // A new reparse was requested before this one completed.
-
+  auto Callback = [this, Version, FileStr,
+                   Tag](std::vector<DiagWithFixIts> Diags) {
     // We need to serialize access to resulting diagnostics to avoid calling
     // `onDiagnosticsReady` in the wrong order.
     std::lock_guard<std::mutex> DiagsLock(DiagnosticsMutex);
@@ -546,14 +544,14 @@ void ClangdServer::scheduleReparseAndDiags(
     LastReportedDiagsVersion = Version;
 
     DiagConsumer.onDiagnosticsReady(
-        FileStr, make_tagged(std::move(*Diags), std::move(Tag)));
+        FileStr, make_tagged(std::move(Diags), std::move(Tag)));
   };
 
   WorkScheduler.update(File,
                        ParseInputs{std::move(Command),
                                    std::move(TaggedFS.Value),
                                    std::move(*Contents.Draft)},
-                       std::move(Callback));
+                       WantDiags, std::move(Callback));
 }
 
 void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {

clang-tools-extra/clangd/ClangdServer.h

@@ -150,7 +150,8 @@ class ClangdServer {
   /// \p File is already tracked. Also schedules parsing of the AST for it on a
   /// separate thread. When the parsing is complete, DiagConsumer passed in
   /// constructor will receive onDiagnosticsReady callback.
-  void addDocument(PathRef File, StringRef Contents);
+  void addDocument(PathRef File, StringRef Contents,
+                   WantDiagnostics WD = WantDiagnostics::Auto);
 
   /// Remove \p File from list of tracked files, schedule a request to free
   /// resources associated with it.
@@ -278,6 +279,7 @@ class ClangdServer {
 
   void
   scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
+                          WantDiagnostics WD,
                           Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS);
 
   CompileArgsCache CompileArgs;

clang-tools-extra/clangd/TUScheduler.cpp

@@ -82,9 +82,8 @@ class ASTWorker {
                                 Semaphore &Barrier, CppFile AST);
   ~ASTWorker();
 
-  void update(ParseInputs Inputs,
-              UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
-                  OnUpdated);
+  void update(ParseInputs Inputs, WantDiagnostics,
+              UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
   void runWithAST(llvm::StringRef Name,
                   UniqueFunction<void(llvm::Expected<InputsAndAST>)> Action);
   bool blockUntilIdle(Deadline Timeout) const;
@@ -101,12 +100,15 @@ class ASTWorker {
   void stop();
   /// Adds a new task to the end of the request queue.
   void startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
-                 bool isUpdate, llvm::Optional<CancellationFlag> CF);
+                 llvm::Optional<WantDiagnostics> UpdateType);
+  /// Should the first task in the queue be skipped instead of run?
+  bool shouldSkipHeadLocked() const;
 
   struct Request {
     UniqueFunction<void()> Action;
     std::string Name;
     Context Ctx;
+    llvm::Optional<WantDiagnostics> UpdateType;
   };
 
   std::string File;
@@ -123,10 +125,7 @@ class ASTWorker {
   std::size_t LastASTSize; /* GUARDED_BY(Mutex) */
   // Set to true to signal run() to finish processing.
   bool Done;                           /* GUARDED_BY(Mutex) */
-  std::queue<Request> Requests;        /* GUARDED_BY(Mutex) */
-  // Only set when last request is an update. This allows us to cancel an update
-  // that was never read, if a subsequent update comes in.
-  llvm::Optional<CancellationFlag> LastUpdateCF; /* GUARDED_BY(Mutex) */
+  std::deque<Request> Requests;        /* GUARDED_BY(Mutex) */
   mutable std::condition_variable RequestsCV;
 };
 
@@ -200,14 +199,9 @@ ASTWorker::~ASTWorker() {
 }
 
 void ASTWorker::update(
-    ParseInputs Inputs,
-    UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
-        OnUpdated) {
-  auto Task = [=](CancellationFlag CF, decltype(OnUpdated) OnUpdated) mutable {
-    if (CF.isCancelled()) {
-      OnUpdated(llvm::None);
-      return;
-    }
+    ParseInputs Inputs, WantDiagnostics WantDiags,
+    UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
+  auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
     FileInputs = Inputs;
     auto Diags = AST.rebuild(std::move(Inputs));
 
@@ -220,12 +214,11 @@ void ASTWorker::update(
     // We want to report the diagnostics even if this update was cancelled.
     // It seems more useful than making the clients wait indefinitely if they
     // spam us with updates.
-    OnUpdated(std::move(Diags));
+    if (Diags && WantDiags != WantDiagnostics::No)
+      OnUpdated(std::move(*Diags));
   };
 
-  CancellationFlag UpdateCF;
-  startTask("Update", BindWithForward(Task, UpdateCF, std::move(OnUpdated)),
-            /*isUpdate=*/true, UpdateCF);
+  startTask("Update", BindWithForward(Task, std::move(OnUpdated)), WantDiags);
 }
 
 void ASTWorker::runWithAST(
@@ -247,7 +240,7 @@ void ASTWorker::runWithAST(
   };
 
   startTask(Name, BindWithForward(Task, std::move(Action)),
-            /*isUpdate=*/false, llvm::None);
+            /*UpdateType=*/llvm::None);
 }
 
 std::shared_ptr<const PreambleData>
@@ -271,10 +264,7 @@ void ASTWorker::stop() {
 }
 
 void ASTWorker::startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
-                          bool isUpdate, llvm::Optional<CancellationFlag> CF) {
-  assert(isUpdate == CF.hasValue() &&
-         "Only updates are expected to pass CancellationFlag");
-
+                          llvm::Optional<WantDiagnostics> UpdateType) {
   if (RunSync) {
     assert(!Done && "running a task after stop()");
     trace::Span Tracer(Name + ":" + llvm::sys::path::filename(File));
@@ -285,18 +275,9 @@ void ASTWorker::startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     assert(!Done && "running a task after stop()");
-    if (isUpdate) {
-      if (!Requests.empty() && LastUpdateCF) {
-        // There were no reads for the last unprocessed update, let's cancel it
-        // to not waste time on it.
-        LastUpdateCF->cancel();
-      }
-      LastUpdateCF = std::move(*CF);
-    } else {
-      LastUpdateCF = llvm::None;
-    }
-    Requests.push({std::move(Task), Name, Context::current().clone()});
-  } // unlock Mutex.
+    Requests.push_back(
+        {std::move(Task), Name, Context::current().clone(), UpdateType});
+  }
   RequestsCV.notify_all();
 }
 
@@ -313,6 +294,9 @@ void ASTWorker::run() {
       // Even when Done is true, we finish processing all pending requests
       // before exiting the processing loop.
 
+      while (shouldSkipHeadLocked())
+        Requests.pop_front();
+      assert(!Requests.empty() && "skipped the whole queue");
       Req = std::move(Requests.front());
       // Leave it on the queue for now, so waiters don't see an empty queue.
     } // unlock Mutex
@@ -326,12 +310,40 @@ void ASTWorker::run() {
 
     {
       std::lock_guard<std::mutex> Lock(Mutex);
-      Requests.pop();
+      Requests.pop_front();
     }
     RequestsCV.notify_all();
   }
 }
 
+// Returns true if Requests.front() is a dead update that can be skipped.
+bool ASTWorker::shouldSkipHeadLocked() const {
+  assert(!Requests.empty());
+  auto Next = Requests.begin();
+  auto UpdateType = Next->UpdateType;
+  if (!UpdateType) // Only skip updates.
+    return false;
+  ++Next;
+  // An update is live if its AST might still be read.
+  // That is, if it's not immediately followed by another update.
+  if (Next == Requests.end() || !Next->UpdateType)
+    return false;
+  // The other way an update can be live is if its diagnostics might be used.
+  switch (*UpdateType) {
+  case WantDiagnostics::Yes:
+    return false; // Always used.
+  case WantDiagnostics::No:
+    return true; // Always dead.
+  case WantDiagnostics::Auto:
+    // Used unless followed by an update that generates diagnostics.
+    for (; Next != Requests.end(); ++Next)
+      if (Next->UpdateType == WantDiagnostics::Yes ||
+          Next->UpdateType == WantDiagnostics::Auto)
+        return true; // Prefer later diagnostics.
+    return false;
+  }
+}
+
 bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
   std::unique_lock<std::mutex> Lock(Mutex);
   return wait(Lock, RequestsCV, Timeout, [&] { return Requests.empty(); });
@@ -389,9 +401,8 @@ bool TUScheduler::blockUntilIdle(Deadline D) const {
 }
 
 void TUScheduler::update(
-    PathRef File, ParseInputs Inputs,
-    UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
-        OnUpdated) {
+    PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags,
+    UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
   std::unique_ptr<FileData> &FD = Files[File];
   if (!FD) {
     // Create a new worker to process the AST-related tasks.
@@ -402,7 +413,7 @@ void TUScheduler::update(
   } else {
     FD->Inputs = Inputs;
   }
-  FD->Worker->update(std::move(Inputs), std::move(OnUpdated));
+  FD->Worker->update(std::move(Inputs), WantDiags, std::move(OnUpdated));
 }
 
 void TUScheduler::remove(PathRef File) {

clang-tools-extra/clangd/TUScheduler.h

@@ -32,6 +32,14 @@ struct InputsAndPreamble {
   const PreambleData *Preamble;
 };
 
+/// Determines whether diagnostics should be generated for a file snapshot.
+enum class WantDiagnostics {
+  Yes,  /// Diagnostics must be generated for this snapshot.
+  No,   /// Diagnostics must not be generated for this snapshot.
+  Auto, /// Diagnostics must be generated for this snapshot or a subsequent one,
+        /// within a bounded amount of time.
+};
+
 /// Handles running tasks for ClangdServer and managing the resources (e.g.,
 /// preambles and ASTs) for opened files.
 /// TUScheduler is not thread-safe, only one thread should be providing updates
@@ -51,9 +59,8 @@ class TUScheduler {
   /// Schedule an update for \p File. Adds \p File to a list of tracked files if
   /// \p File was not part of it before.
   /// FIXME(ibiryukov): remove the callback from this function.
-  void update(PathRef File, ParseInputs Inputs,
-              UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
-                  OnUpdated);
+  void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD,
+              UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
 
   /// Remove \p File from the list of tracked files and schedule removal of its
   /// resources.

clang-tools-extra/clangd/Threading.cpp

@@ -7,8 +7,18 @@
 namespace clang {
 namespace clangd {
 
-CancellationFlag::CancellationFlag()
-    : WasCancelled(std::make_shared<std::atomic<bool>>(false)) {}
+void Notification::notify() {
+  {
+    std::lock_guard<std::mutex> Lock(Mu);
+    Notified = true;
+  }
+  CV.notify_all();
+}
+
+void Notification::wait() const {
+  std::unique_lock<std::mutex> Lock(Mu);
+  CV.wait(Lock, [this] { return Notified; });
+}
 
 Semaphore::Semaphore(std::size_t MaxLocks) : FreeSlots(MaxLocks) {}
 

clang-tools-extra/clangd/Threading.h

@@ -13,7 +13,6 @@
 #include "Context.h"
 #include "Function.h"
 #include "llvm/ADT/Twine.h"
-#include <atomic>
 #include <cassert>
 #include <condition_variable>
 #include <memory>
@@ -23,24 +22,18 @@
 namespace clang {
 namespace clangd {
 
-/// A shared boolean flag indicating if the computation was cancelled.
-/// Once cancelled, cannot be returned to the previous state.
-class CancellationFlag {
+/// A threadsafe flag that is initially clear.
+class Notification {
 public:
-  CancellationFlag();
-
-  void cancel() {
-    assert(WasCancelled && "the object was moved");
-    WasCancelled->store(true);
-  }
-
-  bool isCancelled() const {
-    assert(WasCancelled && "the object was moved");
-    return WasCancelled->load();
-  }
+  // Sets the flag. No-op if already set.
+  void notify();
+  // Blocks until flag is set.
+  void wait() const;
 
 private:
-  std::shared_ptr<std::atomic<bool>> WasCancelled;
+  bool Notified = false;
+  mutable std::condition_variable CV;
+  mutable std::mutex Mu;
 };
 
 /// Limits the number of threads that can acquire the lock at the same time.