diff --git a/clang-tools-extra/clangd/Cancellation.cpp b/clang-tools-extra/clangd/Cancellation.cpp index 6b0e21d34e779..ccd27a1452d17 100644 --- a/clang-tools-extra/clangd/Cancellation.cpp +++ b/clang-tools-extra/clangd/Cancellation.cpp @@ -13,20 +13,30 @@ namespace clang { namespace clangd { char CancelledError::ID = 0; -static Key>> FlagKey; + +// We don't want a cancelable scope to "shadow" an enclosing one. +struct CancelState { + std::shared_ptr> Cancelled; + const CancelState *Parent; +}; +static Key StateKey; std::pair cancelableTask() { - auto Flag = std::make_shared>(); + CancelState State; + State.Cancelled = std::make_shared>(); + State.Parent = Context::current().get(StateKey); return { - Context::current().derive(FlagKey, Flag), - [Flag] { *Flag = true; }, + Context::current().derive(StateKey, State), + [Flag(State.Cancelled)] { *Flag = true; }, }; } bool isCancelled(const Context &Ctx) { - if (auto *Flag = Ctx.get(FlagKey)) - return **Flag; - return false; // Not in scope of a task. + for (const CancelState *State = Ctx.get(StateKey); State != nullptr; + State = State->Parent) + if (State->Cancelled->load()) + return true; + return false; } } // namespace clangd diff --git a/clang-tools-extra/clangd/Cancellation.h b/clang-tools-extra/clangd/Cancellation.h index 6872c448de86b..bdb0bd4ef09fd 100644 --- a/clang-tools-extra/clangd/Cancellation.h +++ b/clang-tools-extra/clangd/Cancellation.h @@ -76,6 +76,7 @@ using Canceler = std::function; std::pair cancelableTask(); /// True if the current context is within a cancelable task which was cancelled. +/// (If the context is within multiple nested tasks, true if any are cancelled). /// Always false if there is no active cancelable task. /// This isn't free (context lookup) - don't call it in a tight loop. bool isCancelled(const Context &Ctx = Context::current()); diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 5dd00322a5abf..cd833af13e49e 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -437,7 +437,8 @@ void ClangdServer::enumerateTweaks(PathRef File, Range Sel, CB(std::move(Res)); }; - WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action)); + WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action), + TUScheduler::InvalidateOnUpdate); } void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID, @@ -556,7 +557,8 @@ void ClangdServer::findDocumentHighlights( CB(clangd::findDocumentHighlights(InpAST->AST, Pos)); }; - WorkScheduler.runWithAST("Highlights", File, std::move(Action)); + WorkScheduler.runWithAST("Highlights", File, std::move(Action), + TUScheduler::InvalidateOnUpdate); } void ClangdServer::findHover(PathRef File, Position Pos, @@ -570,7 +572,8 @@ void ClangdServer::findHover(PathRef File, Position Pos, CB(clangd::getHover(InpAST->AST, Pos, std::move(Style), Index)); }; - WorkScheduler.runWithAST("Hover", File, std::move(Action)); + WorkScheduler.runWithAST("Hover", File, std::move(Action), + TUScheduler::InvalidateOnUpdate); } void ClangdServer::typeHierarchy(PathRef File, Position Pos, int Resolve, @@ -618,7 +621,8 @@ void ClangdServer::documentSymbols(llvm::StringRef File, return CB(InpAST.takeError()); CB(clangd::getDocumentSymbols(InpAST->AST)); }; - WorkScheduler.runWithAST("documentSymbols", File, std::move(Action)); + WorkScheduler.runWithAST("documentSymbols", File, std::move(Action), + TUScheduler::InvalidateOnUpdate); } void ClangdServer::findReferences(PathRef File, Position Pos, uint32_t Limit, @@ -664,7 +668,8 @@ void ClangdServer::documentLinks(PathRef File, return CB(InpAST.takeError()); CB(clangd::getDocumentLinks(InpAST->AST)); }; - WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action)); + WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action), + TUScheduler::InvalidateOnUpdate); } std::vector> diff --git a/clang-tools-extra/clangd/JSONTransport.cpp b/clang-tools-extra/clangd/JSONTransport.cpp index 43be9f71aaf84..d82dca167f3b1 100644 --- a/clang-tools-extra/clangd/JSONTransport.cpp +++ b/clang-tools-extra/clangd/JSONTransport.cpp @@ -19,6 +19,8 @@ namespace { llvm::json::Object encodeError(llvm::Error E) { std::string Message; ErrorCode Code = ErrorCode::UnknownErrorCode; + // FIXME: encode cancellation errors using RequestCancelled or ContentModified + // as appropriate. if (llvm::Error Unhandled = llvm::handleErrors( std::move(E), [&](const LSPError &L) -> llvm::Error { Message = L.Message; diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp index 3f3162a33c303..714bc7217b8b9 100644 --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -184,7 +184,8 @@ class ASTWorker { void update(ParseInputs Inputs, WantDiagnostics); void runWithAST(llvm::StringRef Name, - llvm::unique_function)> Action); + llvm::unique_function)> Action, + TUScheduler::ASTActionInvalidation); bool blockUntilIdle(Deadline Timeout) const; std::shared_ptr getPossiblyStalePreamble() const; @@ -214,7 +215,8 @@ class ASTWorker { void stop(); /// Adds a new task to the end of the request queue. void startTask(llvm::StringRef Name, llvm::unique_function Task, - llvm::Optional UpdateType); + llvm::Optional UpdateType, + TUScheduler::ASTActionInvalidation); /// Updates the TUStatus and emits it. Only called in the worker thread. void emitTUStatus(TUAction FAction, const TUStatus::BuildDetails *Detail = nullptr); @@ -237,6 +239,8 @@ class ASTWorker { steady_clock::time_point AddTime; Context Ctx; llvm::Optional UpdateType; + TUScheduler::ASTActionInvalidation InvalidationPolicy; + Canceler Invalidate; }; /// Handles retention of ASTs. @@ -542,12 +546,13 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) { // Stash the AST in the cache for further use. IdleASTs.put(this, std::move(*AST)); }; - startTask(TaskName, std::move(Task), WantDiags); + startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation); } void ASTWorker::runWithAST( llvm::StringRef Name, - llvm::unique_function)> Action) { + llvm::unique_function)> Action, + TUScheduler::ASTActionInvalidation Invalidation) { auto Task = [=, Action = std::move(Action)]() mutable { if (isCancelled()) return Action(llvm::make_error()); @@ -576,7 +581,7 @@ void ASTWorker::runWithAST( "invalid AST", llvm::errc::invalid_argument)); Action(InputsAndAST{*CurrentInputs, **AST}); }; - startTask(Name, std::move(Task), /*UpdateType=*/None); + startTask(Name, std::move(Task), /*UpdateType=*/None, Invalidation); } std::shared_ptr @@ -606,7 +611,9 @@ void ASTWorker::getCurrentPreamble( }, "GetPreamble", steady_clock::now(), Context::current().clone(), - /*UpdateType=*/None}); + /*UpdateType=*/None, + /*InvalidationPolicy=*/TUScheduler::NoInvalidation, + /*Invalidate=*/nullptr}); Lock.unlock(); RequestsCV.notify_all(); } @@ -650,7 +657,8 @@ void ASTWorker::stop() { void ASTWorker::startTask(llvm::StringRef Name, llvm::unique_function Task, - llvm::Optional UpdateType) { + llvm::Optional UpdateType, + TUScheduler::ASTActionInvalidation Invalidation) { if (RunSync) { assert(!Done && "running a task after stop()"); trace::Span Tracer(Name + ":" + llvm::sys::path::filename(FileName)); @@ -661,9 +669,26 @@ void ASTWorker::startTask(llvm::StringRef Name, { std::lock_guard Lock(Mutex); assert(!Done && "running a task after stop()"); - Requests.push_back( - {std::move(Task), std::string(Name), steady_clock::now(), - Context::current().derive(kFileBeingProcessed, FileName), UpdateType}); + // Cancel any requests invalidated by this request. + if (UpdateType) { + for (auto &R : llvm::reverse(Requests)) { + if (R.InvalidationPolicy == TUScheduler::InvalidateOnUpdate) + R.Invalidate(); + if (R.UpdateType) + break; // Older requests were already invalidated by the older update. + } + } + + // Allow this request to be cancelled if invalidated. + Context Ctx = Context::current().derive(kFileBeingProcessed, FileName); + Canceler Invalidate = nullptr; + if (Invalidation) { + WithContext WC(std::move(Ctx)); + std::tie(Ctx, Invalidate) = cancelableTask(); + } + Requests.push_back({std::move(Task), std::string(Name), steady_clock::now(), + std::move(Ctx), UpdateType, Invalidation, + std::move(Invalidate)}); } RequestsCV.notify_all(); } @@ -942,7 +967,8 @@ void TUScheduler::run(llvm::StringRef Name, void TUScheduler::runWithAST( llvm::StringRef Name, PathRef File, - llvm::unique_function)> Action) { + llvm::unique_function)> Action, + TUScheduler::ASTActionInvalidation Invalidation) { auto It = Files.find(File); if (It == Files.end()) { Action(llvm::make_error( @@ -950,7 +976,7 @@ void TUScheduler::runWithAST( return; } - It->second->Worker->runWithAST(Name, std::move(Action)); + It->second->Worker->runWithAST(Name, std::move(Action), Invalidation); } void TUScheduler::runWithPreamble(llvm::StringRef Name, PathRef File, @@ -1067,5 +1093,15 @@ DebouncePolicy DebouncePolicy::fixed(clock::duration T) { return P; } +void TUScheduler::InvalidatedError::log(llvm::raw_ostream &OS) const { + switch (Policy) { + case InvalidateOnUpdate: + OS << "Task was cancelled due to a subsequent change to the file."; + break; + case NoInvalidation: + llvm_unreachable("Invalidated for no reason?"); + } +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h index 948fde7ed1099..074404c23c116 100644 --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -219,6 +219,29 @@ class TUScheduler { /// Schedule an async task with no dependencies. void run(llvm::StringRef Name, llvm::unique_function Action); + /// Defines how a runWithAST action is implicitly cancelled by other actions. + enum ASTActionInvalidation { + /// The request will run unless explicitly cancelled. + NoInvalidation, + /// The request will be implicitly cancelled by a subsequent update(). + /// (Only if the request was not yet cancelled). + /// Useful for requests that are generated by clients, without any explicit + /// user action. These can otherwise e.g. force every version to be built. + InvalidateOnUpdate, + }; + + /// Error to return when an ASTActionInvalidation policy fires. + class InvalidatedError : public llvm::ErrorInfo { + public: + static char ID; + ASTActionInvalidation Policy; + + void log(llvm::raw_ostream &OS) const override; + std::error_code convertToErrorCode() const override { + return std::make_error_code(std::errc::interrupted); + } + }; + /// Schedule an async read of the AST. \p Action will be called when AST is /// ready. The AST passed to \p Action refers to the version of \p File /// tracked at the time of the call, even if new updates are received before @@ -226,9 +249,11 @@ class TUScheduler { /// If an error occurs during processing, it is forwarded to the \p Action /// callback. /// If the context is cancelled before the AST is ready, the callback will - /// receive a CancelledError. + /// receive a CancelledError. If the invalidation policy is triggered, the + /// callback will receive an InvalidatedError. void runWithAST(llvm::StringRef Name, PathRef File, - Callback Action); + Callback Action, + ASTActionInvalidation = NoInvalidation); /// Controls whether preamble reads wait for the preamble to be up-to-date. enum PreambleConsistency { diff --git a/clang-tools-extra/clangd/unittests/CancellationTests.cpp b/clang-tools-extra/clangd/unittests/CancellationTests.cpp index 611ce07dd8ee3..3fe71901a2067 100644 --- a/clang-tools-extra/clangd/unittests/CancellationTests.cpp +++ b/clang-tools-extra/clangd/unittests/CancellationTests.cpp @@ -44,6 +44,30 @@ TEST(CancellationTest, TaskContextDiesHandleLives) { Task.second(); } +struct NestedTasks { + std::pair Outer, Inner; + NestedTasks() { + Outer = cancelableTask(); + { + WithContext WithOuter(Outer.first.clone()); + Inner = cancelableTask(); + } + } +}; + +TEST(CancellationTest, Nested) { + // Cancelling inner task works but leaves outer task unaffected. + NestedTasks CancelInner; + CancelInner.Inner.second(); + EXPECT_TRUE(isCancelled(CancelInner.Inner.first)); + EXPECT_FALSE(isCancelled(CancelInner.Outer.first)); + // Cancellation of outer task is inherited by inner task. + NestedTasks CancelOuter; + CancelOuter.Outer.second(); + EXPECT_TRUE(isCancelled(CancelOuter.Inner.first)); + EXPECT_TRUE(isCancelled(CancelOuter.Outer.first)); +} + TEST(CancellationTest, AsynCancellationTest) { std::atomic HasCancelled(false); Notification Cancelled; diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp index 9e5952fe2cb53..365e534fd66c2 100644 --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "Annotations.h" +#include "Cancellation.h" #include "Context.h" #include "Diagnostics.h" #include "Matchers.h" @@ -346,6 +347,63 @@ TEST_F(TUSchedulerTests, Cancellation) { << "All reads other than R2B were cancelled"; } +TEST_F(TUSchedulerTests, Invalidation) { + auto Path = testPath("foo.cpp"); + TUScheduler S(CDB, optsForTest(), captureDiags()); + std::atomic Builds(0), Actions(0); + + Notification Start; + updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector) { + ++Builds; + Start.wait(); + }); + S.runWithAST( + "invalidatable", Path, + [&](llvm::Expected AST) { + ++Actions; + EXPECT_FALSE(bool(AST)); + llvm::Error E = AST.takeError(); + EXPECT_TRUE(E.isA()); + consumeError(std::move(E)); + }, + TUScheduler::InvalidateOnUpdate); + S.runWithAST( + "not-invalidatable", Path, + [&](llvm::Expected AST) { + ++Actions; + EXPECT_TRUE(bool(AST)); + }, + TUScheduler::NoInvalidation); + updateWithDiags(S, Path, "b", WantDiagnostics::Auto, [&](std::vector) { + ++Builds; + ADD_FAILURE() << "Shouldn't build, all dependents invalidated"; + }); + S.runWithAST( + "invalidatable", Path, + [&](llvm::Expected AST) { + ++Actions; + EXPECT_FALSE(bool(AST)); + llvm::Error E = AST.takeError(); + EXPECT_TRUE(E.isA()); + consumeError(std::move(E)); + }, + TUScheduler::InvalidateOnUpdate); + updateWithDiags(S, Path, "c", WantDiagnostics::Auto, + [&](std::vector) { ++Builds; }); + S.runWithAST( + "invalidatable", Path, + [&](llvm::Expected AST) { + ++Actions; + EXPECT_TRUE(bool(AST)) << "Shouldn't be invalidated, no update follows"; + }, + TUScheduler::InvalidateOnUpdate); + Start.notify(); + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); + + EXPECT_EQ(2, Builds.load()) << "Middle build should be skipped"; + EXPECT_EQ(4, Actions.load()) << "All actions should run (some with error)"; +} + TEST_F(TUSchedulerTests, ManyUpdates) { const int FilesCount = 3; const int UpdatesPerFile = 10;