[clangd] Send the correct error code when cancelling requests.

[sammccall]

Summary

I couldn't quite bring myself to make Cancellation depend on LSP ErrorCode.
Magic numbers instead...

[kadircet]

Thanks for remembering this :D LGTM!

clang-tools-extra/clangd/JSONTransport.cpp

28

Maybe static_cast<ErrorCode>(C.Reason) instead of casting cases.

I know the reason is not necessarily LSP specific, but we seem to be defaulting to ErrorCode::RequestCancelled anyways.

29

please fix

[sammccall]

clang-tools-extra/clangd/JSONTransport.cpp

28

Sadly this is UB if the value isn't an element of the enum (or in its range, or something). Whereas casting the cases leaves the code valid for arbitrary integers.

clang-tools-extra/clangd/Cancellation.h

 namespace clangd {
 
 /// A canceller requests cancellation of a task, when called.
 /// Calling it again has no effect.
 using Canceler = std::function<void()>;
 
 /// Defines a new task whose cancellation may be requested.
 /// The returned Context defines the scope of the task.
-/// When the context is active, isCancelled() is false until the Canceler is
-/// invoked, and true afterwards.
-std::pair<Context, Canceler> cancelableTask();
+/// When the context is active, isCancelled() is 0 until the Canceler is
+/// invoked, and equal to Reason afterwards.
+/// Conventionally, Reason may be the LSP error code to return.
+std::pair<Context, Canceler> cancelableTask(int Reason = 1);
 
-/// True if the current context is within a cancelable task which was cancelled.
+/// If the current context is within a cancelled task, returns the reason.
 /// (If the context is within multiple nested tasks, true if any are cancelled).
-/// Always false if there is no active cancelable task.
+/// Always zero 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());
+int isCancelled(const Context &Ctx = Context::current());
 
 /// Conventional error when no result is returned due to cancellation.
 class CancelledError : public llvm::ErrorInfo<CancelledError> {
 public:
   static char ID;
+  const int Reason;
+
+  CancelledError(int Reason) : Reason(Reason) {}
 
   void log(llvm::raw_ostream &OS) const override {
     OS << "Task was cancelled.";
   }
   std::error_code convertToErrorCode() const override {
     return std::make_error_code(std::errc::operation_canceled);
   }
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/Cancellation.cpp

 
 namespace clang {
 namespace clangd {
 
 char CancelledError::ID = 0;
 
 // We don't want a cancelable scope to "shadow" an enclosing one.
 struct CancelState {
-  std::shared_ptr<std::atomic<bool>> Cancelled;
+  std::shared_ptr<std::atomic<int>> Cancelled;
   const CancelState *Parent;
 };
 static Key<CancelState> StateKey;
 
-std::pair<Context, Canceler> cancelableTask() {
+std::pair<Context, Canceler> cancelableTask(int Reason) {
+  assert(Reason != 0 && "Can't detect cancellation if Reason is zero");
   CancelState State;
-  State.Cancelled = std::make_shared<std::atomic<bool>>();
+  State.Cancelled = std::make_shared<std::atomic<int>>();
   State.Parent = Context::current().get(StateKey);
   return {
       Context::current().derive(StateKey, State),
-      [Flag(State.Cancelled)] { *Flag = true; },
+      [Reason, Flag(State.Cancelled)] { *Flag = Reason; },
   };
 }
 
-bool isCancelled(const Context &Ctx) {
+int isCancelled(const Context &Ctx) {
   for (const CancelState *State = Ctx.get(StateKey); State != nullptr;
        State = State->Parent)
-    if (State->Cancelled->load())
-      return true;
-  return false;
+    if (int Reason = State->Cancelled->load())
+      return Reason;
+  return 0;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/ClangdLSPServer.cpp

return Context::current().derive(
         Server.NegotiatedOffsetEncoding.getValueOr(OffsetEncoding::UTF16));
   }
 
   // We run cancelable requests in a context that does two things:
   //  - allows cancellation using RequestCancelers[ID]
   //  - cleans up the entry in RequestCancelers when it's no longer needed
   // If a client reuses an ID, the last wins and the first cannot be canceled.
   Context cancelableRequestContext(const llvm::json::Value &ID) {
-    auto Task = cancelableTask();
+    auto Task = cancelableTask(
+        /*Reason=*/static_cast<int>(ErrorCode::RequestCancelled));
     auto StrID = llvm::to_string(ID);  // JSON-serialize ID for map key.
     auto Cookie = NextRequestCookie++; // No lock, only called on main thread.
     {
       std::lock_guard<std::mutex> Lock(RequestCancelersMutex);
       RequestCancelers[StrID] = {std::move(Task.second), Cookie};
     }
     // When the request ends, we can clean up the entry we just added.
     // The cookie lets us check that it hasn't been overwritten due to ID

clang-tools-extra/clangd/ClangdServer.cpp

void ClangdServer::codeComplete(PathRef File, Position Pos,
   if (!CodeCompleteOpts.Index) // Respect overridden index.
     CodeCompleteOpts.Index = Index;
 
   auto Task = [Pos, FS = FSProvider.getFileSystem(), CodeCompleteOpts,
                File = File.str(), CB = std::move(CB),
                this](llvm::Expected<InputsAndPreamble> IP) mutable {
     if (!IP)
       return CB(IP.takeError());
-    if (isCancelled())
-      return CB(llvm::make_error<CancelledError>());
+    if (auto Reason = isCancelled())
+      return CB(llvm::make_error<CancelledError>(Reason));
 
     llvm::Optional<SpeculativeFuzzyFind> SpecFuzzyFind;
     if (!IP->Preamble) {
       // No speculation in Fallback mode, as it's supposed to be much faster
       // without compiling.
       vlog("Build for file {0} is not ready. Enter fallback mode.", File);
     } else {
       if (CodeCompleteOpts.Index && CodeCompleteOpts.SpeculativeIndexRequest) {

clang-tools-extra/clangd/JSONTransport.cpp

 //===--- JSONTransport.cpp - sending and receiving LSP messages over JSON -===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
+#include "Cancellation.h"
 #include "Logger.h"
 #include "Protocol.h" // For LSPError
 #include "Shutdown.h"
 #include "Transport.h"
 #include "llvm/Support/Errno.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
 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 {
+          std::move(E),
+          [&](const CancelledError &C) -> llvm::Error {
+            switch (C.Reason) {
+            case static_cast<int>(ErrorCode::ContentModified):
+              Code = ErrorCode::ContentModified;
+              Message = "Request cancelled because the document was modified";
+              break;
+            default:
+              Code = ErrorCode::RequestCancelled;
+              Message = "Request cancelled";
+              break;
+            }
+            return llvm::Error::success();
+          },
+          [&](const LSPError &L) -> llvm::Error {
             Message = L.Message;
             Code = L.Code;
             return llvm::Error::success();
           }))
     Message = llvm::toString(std::move(Unhandled));
 
   return llvm::json::Object{
       {"message", std::move(Message)},

clang-tools-extra/clangd/Protocol.h

enum class ErrorCode {
   InvalidParams = -32602,
   InternalError = -32603,
 
   ServerNotInitialized = -32002,
   UnknownErrorCode = -32001,
 
   // Defined by the protocol.
   RequestCancelled = -32800,
+  ContentModified = -32801,
 };
 // Models an LSP error as an llvm::Error.
 class LSPError : public llvm::ErrorInfo<LSPError> {
 public:
   std::string Message;
   ErrorCode Code;
   static char ID;
 

clang-tools-extra/clangd/TUScheduler.cpp

void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
   startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation);
 }
 
 void ASTWorker::runWithAST(
     llvm::StringRef Name,
     llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
     TUScheduler::ASTActionInvalidation Invalidation) {
   auto Task = [=, Action = std::move(Action)]() mutable {
-    if (isCancelled())
-      return Action(llvm::make_error<CancelledError>());
+    if (auto Reason = isCancelled())
+      return Action(llvm::make_error<CancelledError>(Reason));
     llvm::Optional<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
     if (!AST) {
       StoreDiags CompilerInvocationDiagConsumer;
       std::unique_ptr<CompilerInvocation> Invocation =
           buildCompilerInvocation(FileInputs, CompilerInvocationDiagConsumer);
       // Try rebuilding the AST.
       vlog("ASTWorker rebuilding evicted AST to run {0}: {1} version {2}", Name,
            FileName, FileInputs.Version);
if (UpdateType) {
       }
     }
 
     // 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();
+      std::tie(Ctx, Invalidate) = cancelableTask(
+          /*Reason=*/static_cast<int>(ErrorCode::ContentModified));
     }
     Requests.push_back({std::move(Task), std::string(Name), steady_clock::now(),
                         std::move(Ctx), UpdateType, Invalidation,
                         std::move(Invalidate)});
   }
   RequestsCV.notify_all();
 }
 

clang-tools-extra/clangd/unittests/CancellationTests.cpp

auto Task = cancelableTask();
     Task.second();
     EXPECT_TRUE(isCancelled());
   }
   // Still should be able to cancel without any problems.
   Task.second();
 }
 
 struct NestedTasks {
+  enum { OuterReason = 1, InnerReason = 2 };
   std::pair<Context, Canceler> Outer, Inner;
   NestedTasks() {
-    Outer = cancelableTask();
+    Outer = cancelableTask(OuterReason);
     {
       WithContext WithOuter(Outer.first.clone());
-      Inner = cancelableTask();
+      Inner = cancelableTask(InnerReason);
     }
   }
 };
 
 TEST(CancellationTest, Nested) {
   // Cancelling inner task works but leaves outer task unaffected.
   NestedTasks CancelInner;
   CancelInner.Inner.second();
-  EXPECT_TRUE(isCancelled(CancelInner.Inner.first));
+  EXPECT_EQ(NestedTasks::InnerReason, 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));
+  EXPECT_EQ(NestedTasks::OuterReason, isCancelled(CancelOuter.Inner.first));
+  EXPECT_EQ(NestedTasks::OuterReason, isCancelled(CancelOuter.Outer.first));
 }
 
 TEST(CancellationTest, AsynCancellationTest) {
   std::atomic<bool> HasCancelled(false);
   Notification Cancelled;
   auto TaskToBeCancelled = [&](Context Ctx) {
     WithContext ContextGuard(std::move(Ctx));
     Cancelled.wait();

clang-tools-extra/clangd/unittests/JSONTransportTests.cpp

 //===-- JSONTransportTests.cpp  -------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
+#include "Cancellation.h"
 #include "Protocol.h"
 #include "Transport.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cstdio>
 
 namespace clang {
 namespace clangd {
public:
   }
 
   bool onCall(llvm::StringRef Method, llvm::json::Value Params,
               llvm::json::Value ID) override {
     Log << "Call " << Method << "(" << ID << "): " << Params << "\n";
     if (Method == "err")
       Target.reply(
           ID, llvm::make_error<LSPError>("trouble at mill", ErrorCode(88)));
+    else if (Method == "invalidated") // gone out skew on treadle
+      Target.reply(ID, llvm::make_error<CancelledError>(
+                           static_cast<int>(ErrorCode::ContentModified)));
     else
       Target.reply(ID, std::move(Params));
     return true;
   }
 
   bool onReply(llvm::json::Value ID,
                llvm::Expected<llvm::json::Value> Params) override {
     if (Params)

 {"jsonrpc": "2.0", "method": "call", "params": 1234}
 ---
 {"jsonrpc": "2.0", "id": 1234, "result": 5678}
 ---
 {"jsonrpc": "2.0", "method": "foo", "id": "abcd", "params": "efgh"}
 ---
 {"jsonrpc": "2.0", "id": "xyz", "error": {"code": 99, "message": "bad!"}}
 ---
-{"jsonrpc": "2.0", "method": "err", "id": "wxyz", "params": "boom!"}
+{"jsonrpc": "2.0", "method": "invalidated", "id": "wxyz", "params": "boom!"}
 ---
 {"jsonrpc": "2.0", "method": "exit"}
   )jsonrpc",
                      /*Pretty=*/true, JSONStreamStyle::Delimited);
   Echo E(*T);
   auto Err = T->loop(E);
   EXPECT_FALSE(bool(Err)) << toString(std::move(Err));
 
   const char *WantLog = R"(
 Notification call: 1234
 Reply(1234): 5678
 Call foo("abcd"): "efgh"
 Reply("xyz"): error = 99: bad!
-Call err("wxyz"): "boom!"
+Call invalidated("wxyz"): "boom!"
 Notification exit: null
   )";
   EXPECT_EQ(trim(E.log()), trim(WantLog));
   const char *WantOutput = "Content-Length: 77\r\n\r\n"
                            R"({
   "id": 42,
   "jsonrpc": "2.0",
   "method": "echo call",
   "params": 1234
 })"
                            "Content-Length: 58\r\n\r\n"
                            R"({
   "id": "abcd",
   "jsonrpc": "2.0",
   "result": "efgh"
 })"
-                           "Content-Length: 105\r\n\r\n"
+                           "Content-Length: 145\r\n\r\n"
                            R"({
   "error": {
-    "code": 88,
-    "message": "trouble at mill"
+    "code": -32801,
+    "message": "Request cancelled because the document was modified"
   },
   "id": "wxyz",
   "jsonrpc": "2.0"
 })";
   EXPECT_EQ(output(), WantOutput);
   EXPECT_EQ(trim(input_mirror()), trim(input()));
 }
 

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

TEST_F(TUSchedulerTests, Invalidation) {
   });
   S.runWithAST(
       "invalidatable", Path,
       [&](llvm::Expected<InputsAndAST> AST) {
         ++Actions;
         EXPECT_FALSE(bool(AST));
         llvm::Error E = AST.takeError();
         EXPECT_TRUE(E.isA<CancelledError>());
-        consumeError(std::move(E));
+        handleAllErrors(std::move(E), [&](const CancelledError &E) {
+          EXPECT_EQ(E.Reason, static_cast<int>(ErrorCode::ContentModified));
+        });
       },
       TUScheduler::InvalidateOnUpdate);
   S.runWithAST(
       "not-invalidatable", Path,
       [&](llvm::Expected<InputsAndAST> AST) {
         ++Actions;
         EXPECT_TRUE(bool(AST));
       },