diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -146,7 +146,27 @@ std::unique_ptr MsgHandler; std::atomic NextCallID = {0}; std::mutex TranspWriter; - void call(StringRef Method, llvm::json::Value Params); + + template + void call(StringRef Method, llvm::json::Value Params, Callback CB) { + // Wrap the callback with LSP conversion and error-handling. + auto Reply = [](decltype(CB) CB, + llvm::Expected RawResponse) { + Response Rsp; + if (!RawResponse) { + CB(RawResponse.takeError()); + } else if (fromJSON(*RawResponse, Rsp)) { + CB(std::move(Rsp)); + } else { + elog("Failed to decode {0} response", *RawResponse); + CB(llvm::make_error("failed to decode reponse", + ErrorCode::InvalidParams)); + } + }; + callImpl(Method, std::move(Params), Bind(std::move(Reply), std::move(CB))); + } + void callImpl(StringRef Method, llvm::json::Value Params, + Callback CB); void notify(StringRef Method, llvm::json::Value Params); const FileSystemProvider &FSProvider; diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -146,11 +146,41 @@ bool onReply(llvm::json::Value ID, llvm::Expected Result) override { WithContext HandlerContext(handlerContext()); - // We ignore replies, just log them. - if (Result) + // A default "log" callback for cases where there is no callback for the + // reply. + Callback CB = [&ID](decltype(Result) Result) { + elog("received a reply with ID {0}, but there was no such call", ID); + if (!Result) + llvm::consumeError(Result.takeError()); + }; + + if (auto IntID = ID.getAsInteger()) { + // Find a corresponding callback for the request ID; + std::lock_guard Mutex(ReplyCallbacksMutex); + int Index = -1; + // We can't use binary search here as ReplyCallbacks is not guaranteed to + // be sorted in the multi-thread environment. + for (size_t I = 0; I < ReplyCallbacks.size() && Index == -1; ++I) { + if (ReplyCallbacks[I].first == *IntID) + Index = I; + } + if (Index != -1) { // found + CB = std::move(ReplyCallbacks[Index].second); + ReplyCallbacks.erase(ReplyCallbacks.begin() + + Index); // remove the entry. + } + } + + // Log and run the callback. + if (Result) { log("<-- reply({0})", ID); - else - log("<-- reply({0}) error: {1}", ID, llvm::toString(Result.takeError())); + CB(std::move(Result)); + } else { + auto Err = Result.takeError(); + log("<-- reply({0}) error: {1}", ID, Err); + CB(std::move(Err)); + } + return true; } @@ -171,6 +201,17 @@ }; } + // Bind a reply to a callback. + // The callback will be invoked when clangd receives the reply from LSP + // client. + void bindReply(int ID, Callback Reply) { + std::lock_guard Mutex(ReplyCallbacksMutex); + ReplyCallbacks.emplace_back(ID, std::move(Reply)); + // Drop the oldest pending callback if we overflow. + if (ReplyCallbacks.size() > MaxCallbacks) + ReplyCallbacks.pop_front(); + } + // Bind an LSP method name to a notification. template void bind(const char *Method, @@ -255,6 +296,16 @@ llvm::StringMap> Notifications; llvm::StringMap> Calls; + // The maximum number of callbacks hold in clangd. + // + // LSP clients should reply for each request, if the clients don't to that, + // clangd will end up memory leakage (the callback will never called and + // removed). To prevent it, we bound the maximum size and drop the oldest + // pending callback if we overflow. + static constexpr int MaxCallbacks = 100; + mutable std::mutex ReplyCallbacksMutex; + std::deque>> + ReplyCallbacks; // Method calls may be cancelled by ID, so keep track of their state. // This needs a mutex: handlers may finish on a different thread, and that's @@ -310,10 +361,11 @@ }; // call(), notify(), and reply() wrap the Transport, adding logging and locking. -void ClangdLSPServer::call(llvm::StringRef Method, llvm::json::Value Params) { +void ClangdLSPServer::callImpl(StringRef Method, llvm::json::Value Params, + Callback CB) { auto ID = NextCallID++; + MsgHandler->bindReply(ID, std::move(CB)); log("--> {0}({1})", Method, ID); - // We currently don't handle responses, so no need to store ID anywhere. std::lock_guard Lock(TranspWriter); Transp.call(Method, std::move(Params), ID); } @@ -496,13 +548,26 @@ void ClangdLSPServer::onCommand(const ExecuteCommandParams &Params, Callback Reply) { - auto ApplyEdit = [this](WorkspaceEdit WE) { + auto ApplyEdit = [this](WorkspaceEdit WE, + Callback Reply) { ApplyWorkspaceEditParams Edit; Edit.edit = std::move(WE); - // Ideally, we would wait for the response and if there is no error, we - // would reply success/failure to the original RPC. - call("workspace/applyEdit", Edit); + call("workspace/applyEdit", std::move(Edit), std::move(Reply)); }; + auto ReplyApplyEdit = + [](decltype(Reply) Reply, + llvm::Expected Response) { + if (!Response) + return Reply(Response.takeError()); + if (!Response->applied) { + std::string Reason = + Response->failureReason ? *(Response->failureReason) : ""; + return Reply(llvm::createStringError( + llvm::inconvertibleErrorCode(), + ("edits fail to applied: " + Reason).c_str())); + } + return Reply("Fix applied."); + }; if (Params.command == ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND && Params.workspaceEdit) { // The flow for "apply-fix" : @@ -511,11 +576,9 @@ // 3. We send code actions, with the fixit embedded as context // 4. The user selects the fixit, the editor asks us to apply it // 5. We unwrap the changes and send them back to the editor - // 6. The editor applies the changes (applyEdit), and sends us a reply (but - // we ignore it) - - Reply("Fix applied."); - ApplyEdit(*Params.workspaceEdit); + // 6. The editor applies the changes (applyEdit), and sends us a reply + // 7. We unwrap the reply and send a reply to the editor. + ApplyEdit(*Params.workspaceEdit, Bind(ReplyApplyEdit, std::move(Reply))); } else if (Params.command == ExecuteCommandParams::CLANGD_APPLY_TWEAK && Params.tweakArgs) { auto Code = DraftMgr.getDraft(Params.tweakArgs->file.file()); @@ -524,9 +587,9 @@ llvm::inconvertibleErrorCode(), "trying to apply a code action for a non-added file")); - auto Action = [this, ApplyEdit](decltype(Reply) Reply, URIForFile File, - std::string Code, - llvm::Expected R) { + auto Action = [this, ApplyEdit, ReplyApplyEdit]( + decltype(Reply) Reply, URIForFile File, std::string Code, + llvm::Expected R) { if (!R) return Reply(R.takeError()); @@ -534,15 +597,15 @@ WorkspaceEdit WE; WE.changes.emplace(); (*WE.changes)[File.uri()] = replacementsToEdits(Code, *R->ApplyEdit); - ApplyEdit(std::move(WE)); + ApplyEdit(std::move(WE), Bind(ReplyApplyEdit, std::move(Reply))); } if (R->ShowMessage) { ShowMessageParams Msg; Msg.message = *R->ShowMessage; Msg.type = MessageType::Info; notify("window/showMessage", Msg); + Reply("Tweak applied."); } - Reply("Tweak applied."); }; Server->applyTweak(Params.tweakArgs->file.file(), Params.tweakArgs->selection, Params.tweakArgs->tweakID, diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -873,6 +873,12 @@ }; llvm::json::Value toJSON(const ApplyWorkspaceEditParams &); +struct ApplyWorkspaceEditResponse { + bool applied; + llvm::Optional failureReason; +}; +bool fromJSON(const llvm::json::Value &, ApplyWorkspaceEditResponse &); + struct TextDocumentPositionParams { /// The text document. TextDocumentIdentifier textDocument; diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -670,6 +670,15 @@ return llvm::json::Object{{"edit", Params.edit}}; } +bool fromJSON(const llvm::json::Value &Response, + ApplyWorkspaceEditResponse &R) { + llvm::json::ObjectMapper O(Response); + if (!O || !O.map("applied", R.applied)) + return false; + O.map("failureReason", R.failureReason); + return true; +} + bool fromJSON(const llvm::json::Value &Params, TextDocumentPositionParams &R) { llvm::json::ObjectMapper O(Params); return O && O.map("textDocument", R.textDocument) && diff --git a/clang-tools-extra/clangd/test/code-action-request.test b/clang-tools-extra/clangd/test/code-action-request.test --- a/clang-tools-extra/clangd/test/code-action-request.test +++ b/clang-tools-extra/clangd/test/code-action-request.test @@ -64,7 +64,9 @@ # CHECK-NEXT: } # CHECK-NEXT: } --- +{"jsonrpc":"2.0","id":0,"result":{"applied":true}} +--- {"jsonrpc":"2.0","id":4,"method":"shutdown"} --- {"jsonrpc":"2.0","method":"exit"} --- \ No newline at end of file diff --git a/clang-tools-extra/clangd/test/execute-command.test b/clang-tools-extra/clangd/test/execute-command.test --- a/clang-tools-extra/clangd/test/execute-command.test +++ b/clang-tools-extra/clangd/test/execute-command.test @@ -33,6 +33,8 @@ --- {"jsonrpc":"2.0","id":5,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","custom":"foo", "arguments":[{"changes":{"test:///foo.c":[{"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":32}},"newText":"("},{"range":{"start":{"line":0,"character":37},"end":{"line":0,"character":37}},"newText":")"}]}}]}} --- +{"jsonrpc":"2.0","id":0,"result":{"applied":false}} +--- # Arguments not a sequence. {"jsonrpc":"2.0","id":6,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":"foo"}} --- @@ -45,6 +47,8 @@ # Custom field in WorkspaceEdit {"jsonrpc":"2.0","id":9,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":[{"custom":"foo", "changes":{"test:///foo.c":[{"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":32}},"newText":"("},{"range":{"start":{"line":0,"character":37},"end":{"line":0,"character":37}},"newText":")"}]}}]}} --- +{"jsonrpc":"2.0","id":1,"result":{"applied":false}} +--- # changes in WorkspaceEdit with no mapping node {"jsonrpc":"2.0","id":10,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":[{"changes":"foo"}]}} --- @@ -61,8 +65,10 @@ {"jsonrpc":"2.0","id":14,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":[{"changes":{"test:///foo.c":[{"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":32}},"newText":"("},{"range":"","newText":")"}]}}]}} --- # Command name after arguments -{"jsonrpc":"2.0","id":9,"method":"workspace/executeCommand","params":{"arguments":[{"custom":"foo", "changes":{"test:///foo.c":[{"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":32}},"newText":"("},{"range":{"start":{"line":0,"character":37},"end":{"line":0,"character":37}},"newText":")"}]}}],"command":"clangd.applyFix"}} +{"jsonrpc":"2.0","id":15,"method":"workspace/executeCommand","params":{"arguments":[{"custom":"foo", "changes":{"test:///foo.c":[{"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":32}},"newText":"("},{"range":{"start":{"line":0,"character":37},"end":{"line":0,"character":37}},"newText":")"}]}}],"command":"clangd.applyFix"}} +--- +{"jsonrpc":"2.0","id":2,"result":{"applied":false}} --- -{"jsonrpc":"2.0","id":3,"method":"shutdown"} +{"jsonrpc":"2.0","id":16,"method":"shutdown"} --- {"jsonrpc":"2.0","method":"exit"} diff --git a/clang-tools-extra/clangd/test/fixits-command.test b/clang-tools-extra/clangd/test/fixits-command.test --- a/clang-tools-extra/clangd/test/fixits-command.test +++ b/clang-tools-extra/clangd/test/fixits-command.test @@ -165,10 +165,6 @@ # CHECK-NEXT: ] --- {"jsonrpc":"2.0","id":4,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":[{"changes":{"test:///foo.c":[{"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":32}},"newText":"("},{"range":{"start":{"line":0,"character":37},"end":{"line":0,"character":37}},"newText":")"}]}}]}} -# CHECK: "id": 4, -# CHECK-NEXT: "jsonrpc": "2.0", -# CHECK-NEXT: "result": "Fix applied." -# # CHECK: "id": 0, # CHECK-NEXT: "jsonrpc": "2.0", # CHECK-NEXT: "method": "workspace/applyEdit", @@ -207,6 +203,11 @@ # CHECK-NEXT: } # CHECK-NEXT: } --- +{"jsonrpc":"2.0","id":0,"result":{"applied":true}} +# CHECK: "id": 4, +# CHECK-NEXT: "jsonrpc": "2.0", +# CHECK-NEXT: "result": "Fix applied." +--- {"jsonrpc":"2.0","id":4,"method":"shutdown"} --- {"jsonrpc":"2.0","method":"exit"} diff --git a/clang-tools-extra/clangd/test/tweaks-format.test b/clang-tools-extra/clangd/test/tweaks-format.test --- a/clang-tools-extra/clangd/test/tweaks-format.test +++ b/clang-tools-extra/clangd/test/tweaks-format.test @@ -6,6 +6,7 @@ {"jsonrpc":"2.0","id":5,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///main.cc"},"range":{"start":{"line":0,"character":11},"end":{"line":0,"character":11}},"context":{"diagnostics":[]}}} --- {"jsonrpc":"2.0","id":6,"method":"workspace/executeCommand","params":{"command":"clangd.applyTweak","arguments":[{"file":"test:///main.cc","selection":{"end":{"character":11,"line":0},"start":{"character":11,"line":0}},"tweakID":"SwapIfBranches"}]}} +# CHECK: "id": 0, # CHECK: "newText": "\n ", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { @@ -45,6 +46,8 @@ # CHECK-NEXT: } # CHECK-NEXT: } --- +{"jsonrpc":"2.0","id":0,"result":{"applied":true}} +--- {"jsonrpc":"2.0","id":3,"method":"shutdown"} --- {"jsonrpc":"2.0","method":"exit"}