[clang-tools-extra] r325774 - [clangd] Allow embedders some control over when diagnostics are generated.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 22 05:11:13 PST 2018
Author: sammccall
Date: Thu Feb 22 05:11:12 2018
New Revision: 325774
URL: http://llvm.org/viewvc/llvm-project?rev=325774&view=rev
Log:
[clangd] Allow embedders some control over when diagnostics are generated.
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
Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/clangd/Threading.cpp
clang-tools-extra/trunk/clangd/Threading.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=325774&r1=325773&r2=325774&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Thu Feb 22 05:11:12 2018
@@ -141,7 +141,8 @@ void ClangdLSPServer::onDocumentDidOpen(
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::onDocumentDidChang
"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) {
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=325774&r1=325773&r2=325774&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Thu Feb 22 05:11:12 2018
@@ -110,11 +110,12 @@ void ClangdServer::setRootPath(PathRef R
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
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::scheduleReparseAndDia
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) {
Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=325774&r1=325773&r2=325774&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.h Thu Feb 22 05:11:12 2018
@@ -150,7 +150,8 @@ public:
/// \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 @@ private:
void
scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
+ WantDiagnostics WD,
Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS);
CompileArgsCache CompileArgs;
Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=325774&r1=325773&r2=325774&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Thu Feb 22 05:11:12 2018
@@ -82,9 +82,8 @@ public:
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 @@ private:
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 @@ private:
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::StringRe
{
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(Deadlin
}
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) {
Modified: clang-tools-extra/trunk/clangd/TUScheduler.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.h?rev=325774&r1=325773&r2=325774&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.h (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.h Thu Feb 22 05:11:12 2018
@@ -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 @@ public:
/// 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.
Modified: clang-tools-extra/trunk/clangd/Threading.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Threading.cpp?rev=325774&r1=325773&r2=325774&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Threading.cpp (original)
+++ clang-tools-extra/trunk/clangd/Threading.cpp Thu Feb 22 05:11:12 2018
@@ -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) {}
Modified: clang-tools-extra/trunk/clangd/Threading.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Threading.h?rev=325774&r1=325773&r2=325774&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Threading.h (original)
+++ clang-tools-extra/trunk/clangd/Threading.h Thu Feb 22 05:11:12 2018
@@ -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.
Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=325774&r1=325773&r2=325774&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu Feb 22 05:11:12 2018
@@ -50,7 +50,7 @@ TEST_F(TUSchedulerTests, MissingFiles) {
auto Missing = testPath("missing.cpp");
Files[Missing] = "";
- S.update(Added, getInputs(Added, ""), ignoreUpdate);
+ S.update(Added, getInputs(Added, ""), WantDiagnostics::No, ignoreUpdate);
// Assert each operation for missing file is an error (even if it's available
// in VFS).
@@ -88,6 +88,37 @@ TEST_F(TUSchedulerTests, MissingFiles) {
S.remove(Added);
}
+TEST_F(TUSchedulerTests, WantDiagnostics) {
+ std::atomic<int> CallbackCount(0);
+ {
+ TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr);
+ auto Path = testPath("foo.cpp");
+
+ // To avoid a racy test, don't allow tasks to actualy run on the worker
+ // thread until we've scheduled them all.
+ Notification Ready;
+ S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
+ [&](std::vector<DiagWithFixIts>) { Ready.wait(); });
+
+ S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
+ [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
+ [&](std::vector<DiagWithFixIts> Diags) {
+ ADD_FAILURE() << "auto should have been cancelled by auto";
+ });
+ S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
+ [&](std::vector<DiagWithFixIts> Diags) {
+ ADD_FAILURE() << "no diags should not be called back";
+ });
+ S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
+ [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ Ready.notify();
+ }
+ EXPECT_EQ(2, CallbackCount);
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
@@ -132,7 +163,7 @@ TEST_F(TUSchedulerTests, ManyUpdates) {
{
WithContextValue WithNonce(NonceKey, ++Nonce);
- S.update(File, Inputs,
+ S.update(File, Inputs, WantDiagnostics::Auto,
[Nonce, &Mut, &TotalUpdates](
llvm::Optional<std::vector<DiagWithFixIts>> Diags) {
EXPECT_THAT(Context::current().get(NonceKey),
More information about the cfe-commits
mailing list