[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
This revision was automatically updated to reflect the committed changes.
sammccall marked 5 inline comments as done.
Closed by commit rL344620: [clangd] Refactor JSON-over-stdin/stdout code into
Transport abstraction. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53286?vs=169835=169841#toc
Repository:
rL LLVM
https://reviews.llvm.org/D53286
Files:
clang-tools-extra/trunk/clangd/CMakeLists.txt
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
clang-tools-extra/trunk/clangd/JSONTransport.cpp
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
clang-tools-extra/trunk/clangd/Transport.h
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
clang-tools-extra/trunk/test/clangd/completion-snippets.test
clang-tools-extra/trunk/test/clangd/completion.test
clang-tools-extra/trunk/test/clangd/crash-non-added-files.test
clang-tools-extra/trunk/test/clangd/execute-command.test
clang-tools-extra/trunk/test/clangd/input-mirror.test
clang-tools-extra/trunk/test/clangd/signature-help.test
clang-tools-extra/trunk/test/clangd/textdocument-didchange-fail.test
clang-tools-extra/trunk/test/clangd/trace.test
clang-tools-extra/trunk/test/clangd/xrefs.test
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
clang-tools-extra/trunk/unittests/clangd/JSONTransportTests.cpp
Index: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
===
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
@@ -14,6 +14,7 @@
#include "Logger.h"
#include "Protocol.h"
#include "Trace.h"
+#include "Transport.h"
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringMap.h"
@@ -24,37 +25,19 @@
namespace clang {
namespace clangd {
-/// Encapsulates output and logs streams and provides thread-safe access to
-/// them.
+// Logs to an output stream, such as stderr.
+// FIXME: Rename to StreamLogger or such, and move to Logger.h.
class JSONOutput : public Logger {
- // FIXME(ibiryukov): figure out if we can shrink the public interface of
- // JSONOutput now that we pass Context everywhere.
public:
- JSONOutput(llvm::raw_ostream , llvm::raw_ostream ,
- Logger::Level MinLevel, llvm::raw_ostream *InputMirror = nullptr,
- bool Pretty = false)
- : Pretty(Pretty), MinLevel(MinLevel), Outs(Outs), Logs(Logs),
-InputMirror(InputMirror) {}
-
- /// Emit a JSONRPC message.
- void writeMessage(const llvm::json::Value );
+ JSONOutput(llvm::raw_ostream , Logger::Level MinLevel)
+ : MinLevel(MinLevel), Logs(Logs) {}
/// Write a line to the logging stream.
void log(Level, const llvm::formatv_object_base ) override;
- /// Mirror \p Message into InputMirror stream. Does nothing if InputMirror is
- /// null.
- /// Unlike other methods of JSONOutput, mirrorInput is not thread-safe.
- void mirrorInput(const Twine );
-
- // Whether output should be pretty-printed.
- const bool Pretty;
-
private:
Logger::Level MinLevel;
- llvm::raw_ostream
llvm::raw_ostream
- llvm::raw_ostream *InputMirror;
std::mutex StreamMutex;
};
@@ -81,26 +64,39 @@
///
/// The `$/cancelRequest` notification is handled by the dispatcher itself.
/// It marks the matching request as cancelled, if it's still running.
-class JSONRPCDispatcher {
+class JSONRPCDispatcher : private Transport::MessageHandler {
public:
/// A handler responds to requests for a particular method name.
+ /// It returns false if the server should now shut down.
///
/// JSONRPCDispatcher will mark the handler's context as cancelled if a
/// matching cancellation request is received. Handlers are encouraged to
/// check for cancellation and fail quickly in this case.
- using Handler = std::function;
+ using Handler = std::function;
/// Create a new JSONRPCDispatcher. UnknownHandler is called when an unknown
/// method is received.
JSONRPCDispatcher(Handler UnknownHandler);
/// Registers a Handler for the specified Method.
void registerHandler(StringRef Method, Handler H);
- /// Parses a JSONRPC message and calls the Handler for it.
- bool call(const llvm::json::Value , JSONOutput );
+ /// Parses input queries from LSP client (coming from \p In) and runs call
+ /// method for each query.
+ ///
+ /// Input stream(\p In) must be opened in binary mode to avoid
+ /// preliminary replacements of \r\n with \n. We use C-style FILE* for reading
+ /// as std::istream has unclear interaction with signals, which
[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clangd/JSONRPCDispatcher.h:70 /// A handler responds to requests for a particular method name. + /// It returns true if the server should now shut down. /// s/true/false/ Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
sammccall marked 9 inline comments as done.
sammccall added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:156
+void ClangdLSPServer::onExit(ExitParams ) {
+ // XXX handler should return true.
+}
ioeric wrote:
> ?
Fixed this comment.
I'm not totally happy with the shutdown mechanism, it's noisier and more magic
than I'd like. May find something nicer to replace it with.
Comment at: clangd/JSONTransport.cpp:23
+Code = L.Code;
+return make_error(L.Message, inconvertibleErrorCode());
+ }));
ioeric wrote:
> Is `E` guaranteed to be an `LSPError`? Do we need a handler for other errors?
This actually handles generic errors as well, Error just has a really confusing
API.
Rewrote this to be more direct.
Comment at: clangd/Transport.h:50
+virtual ~MessageHandler() = default;
+virtual bool notify(llvm::StringRef Method, llvm::json::Value) = 0;
+virtual bool call(llvm::StringRef Method, llvm::json::Value Params,
ioeric wrote:
> nit: maybe `handleNotify` to make the two directions more distinguishable?
> This can get a bit counter-intuitive in the call sites.
`onNotify` etc
Comment at: unittests/clangd/JSONTransportTests.cpp:106
+ auto Err = T->loop(E);
+ EXPECT_FALSE(bool(Err)) << llvm::toString(std::move(Err));
+
ioeric wrote:
> Would `operator<<` be used when the error is real (i.e. check passes)?
No, EXPECT_* are magic macros that only evaluate the right hand side of << if
they fire.
`EXPECT_FALSE(expr)` expands to something like `if (expr)
CreateErrorStream("expr")`, so the `<< blah` becomes part of the if statement.
(And the temporary stream returned from CreateErrorStream does the actual
logging/mark-the-test-as-failed in its destructor).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53286
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
sammccall updated this revision to Diff 169835.
sammccall marked 9 inline comments as done.
sammccall added a comment.
Addressed review comments.
Also inverted the sense of the boolean return from `onNotify` etc, was "should
exit" and is now "should continue", which I think is slightly clearer.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53286
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/JSONRPCDispatcher.cpp
clangd/JSONRPCDispatcher.h
clangd/JSONTransport.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/Transport.h
clangd/tool/ClangdMain.cpp
test/clangd/compile-commands-path-in-initialize.test
test/clangd/compile-commands-path.test
test/clangd/completion-snippets.test
test/clangd/completion.test
test/clangd/crash-non-added-files.test
test/clangd/execute-command.test
test/clangd/input-mirror.test
test/clangd/signature-help.test
test/clangd/textdocument-didchange-fail.test
test/clangd/trace.test
test/clangd/xrefs.test
unittests/clangd/CMakeLists.txt
unittests/clangd/JSONTransportTests.cpp
Index: unittests/clangd/JSONTransportTests.cpp
===
--- /dev/null
+++ unittests/clangd/JSONTransportTests.cpp
@@ -0,0 +1,199 @@
+//===-- JSONTransportTests.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Protocol.h"
+#include "Transport.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+
+using namespace llvm;
+namespace clang {
+namespace clangd {
+namespace {
+
+// No fmemopen on windows, so we can't easily run this test.
+#ifndef WIN32
+
+// Fixture takes care of managing the input/output buffers for the transport.
+class JSONTransportTest : public ::testing::Test {
+ std::string InBuf, OutBuf, MirrorBuf;
+ llvm::raw_string_ostream Out, Mirror;
+ std::unique_ptr In;
+
+protected:
+ JSONTransportTest() : Out(OutBuf), Mirror(MirrorBuf), In(nullptr, nullptr) {}
+
+ template
+ std::unique_ptr transport(std::string InData, bool Pretty,
+ JSONStreamStyle Style) {
+InBuf = std::move(InData);
+In = {fmemopen([0], InBuf.size(), "r"), };
+return newJSONTransport(In.get(), Out, , Pretty, Style);
+ }
+
+ std::string input() const { return InBuf; }
+ std::string output() { return Out.str(); }
+ std::string input_mirror() { return Mirror.str(); }
+};
+
+// Echo is a simple server running on a transport:
+// - logs each message it gets.
+// - when it gets a call, replies to it
+// - when it gets a notification for method "call", makes a call on Target
+// Hangs up when it gets an exit notification.
+class Echo : public Transport::MessageHandler {
+ Transport
+ std::string LogBuf;
+ raw_string_ostream Log;
+
+public:
+ Echo(Transport ) : Target(Target), Log(LogBuf) {}
+
+ std::string log() { return Log.str(); }
+
+ bool onNotify(StringRef Method, json::Value Params) override {
+Log << "Notification " << Method << ": " << Params << "\n";
+if (Method == "call")
+ Target.call("echo call", std::move(Params), 42);
+return Method != "exit";
+ }
+
+ bool onCall(StringRef Method, json::Value Params, json::Value ID) override {
+Log << "Call " << Method << "(" << ID << "): " << Params << "\n";
+if (Method == "err")
+ Target.reply(ID, make_error("trouble at mill", ErrorCode(88)));
+else
+ Target.reply(ID, std::move(Params));
+return true;
+ }
+
+ bool onReply(json::Value ID, Expected Params) override {
+if (Params)
+ Log << "Reply(" << ID << "): " << *Params << "\n";
+else
+ Log << "Reply(" << ID
+ << "): error = " << llvm::toString(Params.takeError()) << "\n";
+return true;
+ }
+};
+
+std::string trim(StringRef S) { return S.trim().str(); }
+
+// Runs an Echo session using the standard JSON-RPC format we use in production.
+TEST_F(JSONTransportTest, StandardDense) {
+ auto T = transport(
+ "Content-Length: 52\r\n\r\n"
+ R"({"jsonrpc": "2.0", "method": "call", "params": 1234})"
+ "Content-Length: 46\r\n\r\n"
+ R"({"jsonrpc": "2.0", "id": 1234, "result": 5678})"
+ "Content-Length: 67\r\n\r\n"
+ R"({"jsonrpc": "2.0", "method": "foo", "id": "abcd", "params": "efgh"})"
+ "Content-Length: 73\r\n\r\n"
+ R"({"jsonrpc": "2.0", "id": "xyz", "error": {"code": 99, "message": "bad!"}})"
+ "Content-Length: 68\r\n\r\n"
+ R"({"jsonrpc": "2.0", "method": "err", "id": "wxyz", "params": "boom!"})"
+ "Content-Length: 36\r\n\r\n"
+ R"({"jsonrpc": "2.0", "method": "exit"})",
+ /*Pretty=*/false, JSONStreamStyle::Standard);
+ Echo E(*T);
+ auto Err =
[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
ioeric added a comment.
Nice! The code looks much clearer. Just some nits
Comment at: clangd/ClangdLSPServer.cpp:156
+void ClangdLSPServer::onExit(ExitParams ) {
+ // XXX handler should return true.
+}
?
Comment at: clangd/ClangdLSPServer.h:45
/// Run LSP server loop, receiving input for it from \p In. \p In must be
/// opened in binary mode. Output will be written using Out variable passed
to
doc is outdated now
Comment at: clangd/JSONRPCDispatcher.cpp:218
-// Tries to read a line up to and including \n.
-// If failing, feof() or ferror() will be set.
-static bool readLine(std::FILE *In, std::string ) {
- static constexpr int BufSize = 1024;
- size_t Size = 0;
- Out.clear();
- for (;;) {
-Out.resize(Size + BufSize);
-// Handle EINTR which is sent when a debugger attaches on some platforms.
-if (!llvm::sys::RetryAfterSignal(nullptr, ::fgets, [Size], BufSize,
In))
- return false;
-clearerr(In);
-// If the line contained null bytes, anything after it (including \n) will
-// be ignored. Fortunately this is not a legal header or JSON.
-size_t Read = std::strlen([Size]);
-if (Read > 0 && Out[Size + Read - 1] == '\n') {
- Out.resize(Size + Read);
- return true;
-}
-Size += Read;
- }
-}
-
-// Returns None when:
-// - ferror() or feof() are set.
-// - Content-Length is missing or empty (protocol error)
-static llvm::Optional readStandardMessage(std::FILE *In,
- JSONOutput ) {
- // A Language Server Protocol message starts with a set of HTTP headers,
- // delimited by \r\n, and terminated by an empty line (\r\n).
- unsigned long long ContentLength = 0;
- std::string Line;
- while (true) {
-if (feof(In) || ferror(In) || !readLine(In, Line))
- return llvm::None;
-
-Out.mirrorInput(Line);
-llvm::StringRef LineRef(Line);
-
-// We allow comments in headers. Technically this isn't part
-// of the LSP specification, but makes writing tests easier.
-if (LineRef.startswith("#"))
- continue;
-
-// Content-Length is a mandatory header, and the only one we handle.
-if (LineRef.consume_front("Content-Length: ")) {
- if (ContentLength != 0) {
-elog("Warning: Duplicate Content-Length header received. "
- "The previous value for this message ({0}) was ignored.",
- ContentLength);
- }
- llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength);
- continue;
-} else if (!LineRef.trim().empty()) {
- // It's another header, ignore it.
- continue;
-} else {
- // An empty line indicates the end of headers.
- // Go ahead and read the JSON.
- break;
-}
- }
-
- // The fuzzer likes crashing us by sending "Content-Length: "
- if (ContentLength > 1 << 30) { // 1024M
-elog("Refusing to read message with long Content-Length: {0}. "
- "Expect protocol errors",
- ContentLength);
-return llvm::None;
- }
- if (ContentLength == 0) {
-log("Warning: Missing Content-Length header, or zero-length message.");
-return llvm::None;
- }
-
- std::string JSON(ContentLength, '\0');
- for (size_t Pos = 0, Read; Pos < ContentLength; Pos += Read) {
-// Handle EINTR which is sent when a debugger attaches on some platforms.
-Read = llvm::sys::RetryAfterSignal(0u, ::fread, [Pos], 1,
- ContentLength - Pos, In);
-Out.mirrorInput(StringRef([Pos], Read));
-if (Read == 0) {
- elog("Input was aborted. Read only {0} bytes of expected {1}.", Pos,
- ContentLength);
- return llvm::None;
-}
-clearerr(In); // If we're done, the error was transient. If we're not done,
- // either it was transient or we'll see it again on retry.
-Pos += Read;
- }
- return std::move(JSON);
-}
-
-// For lit tests we support a simplified syntax:
-// - messages are delimited by '---' on a line by itself
-// - lines starting with # are ignored.
-// This is a testing path, so favor simplicity over performance here.
-// When returning None, feof() or ferror() will be set.
-static llvm::Optional readDelimitedMessage(std::FILE *In,
-JSONOutput ) {
- std::string JSON;
- std::string Line;
- while (readLine(In, Line)) {
-auto LineRef = llvm::StringRef(Line).trim();
-if (LineRef.startswith("#")) // comment
- continue;
-
-// found a delimiter
-if (LineRef.rtrim() == "---")
- break;
-
-JSON += Line;
- }
-
- if (ferror(In)) {
-elog("Input error while reading message!");
-return llvm::None;
- } else { // Including EOF
-Out.mirrorInput(
-llvm::formatv("Content-Length: {0}\r\n\r\n{1}", JSON.size(), JSON));
-return std::move(JSON);
- }
-}
-
// The use of C-style std::FILE* IO deserves
[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
sammccall updated this revision to Diff 169807.
sammccall added a comment.
Add unit test for JSON transport.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53286
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/JSONRPCDispatcher.cpp
clangd/JSONRPCDispatcher.h
clangd/JSONTransport.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/Transport.h
clangd/tool/ClangdMain.cpp
test/clangd/compile-commands-path-in-initialize.test
test/clangd/compile-commands-path.test
test/clangd/completion-snippets.test
test/clangd/completion.test
test/clangd/crash-non-added-files.test
test/clangd/execute-command.test
test/clangd/input-mirror.test
test/clangd/signature-help.test
test/clangd/textdocument-didchange-fail.test
test/clangd/trace.test
test/clangd/xrefs.test
unittests/clangd/CMakeLists.txt
unittests/clangd/JSONTransportTests.cpp
Index: unittests/clangd/JSONTransportTests.cpp
===
--- /dev/null
+++ unittests/clangd/JSONTransportTests.cpp
@@ -0,0 +1,199 @@
+//===-- JSONTransportTests.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Protocol.h"
+#include "Transport.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+
+using namespace llvm;
+namespace clang {
+namespace clangd {
+namespace {
+
+// No fmemopen on windows, so we can't easily run this test.
+#ifndef WIN32
+
+// Fixture takes care of managing the input/output buffers for the transport.
+class JSONTransportTest : public ::testing::Test {
+ std::string InBuf, OutBuf, MirrorBuf;
+ llvm::raw_string_ostream Out, Mirror;
+ std::unique_ptr In;
+
+protected:
+ JSONTransportTest() : Out(OutBuf), Mirror(MirrorBuf), In(nullptr, nullptr) {}
+
+ template
+ std::unique_ptr transport(std::string InData, bool Pretty,
+ JSONStreamStyle Style) {
+InBuf = std::move(InData);
+In = {fmemopen([0], InBuf.size(), "r"), };
+return newJSONTransport(In.get(), Out, , Pretty, Style);
+ }
+
+ std::string input() const { return InBuf; }
+ std::string output() { return Out.str(); }
+ std::string input_mirror() { return Mirror.str(); }
+};
+
+// Echo is a simple server running on a transport:
+// - logs each message it gets.
+// - when it gets a call, replies to it
+// - when it gets a notification for method "call", makes a call on Target
+// Hangs up when it gets an exit notification.
+class Echo : public Transport::MessageHandler {
+ Transport
+ std::string LogBuf;
+ raw_string_ostream Log;
+
+public:
+ Echo(Transport ) : Target(Target), Log(LogBuf) {}
+
+ std::string log() { return Log.str(); }
+
+ bool notify(StringRef Method, json::Value Params) override {
+Log << "Notification " << Method << ": " << Params << "\n";
+if (Method == "call")
+ Target.call("echo call", std::move(Params), 42);
+return Method == "exit";
+ }
+
+ bool call(StringRef Method, json::Value Params, json::Value ID) override {
+Log << "Call " << Method << "(" << ID << "): " << Params << "\n";
+if (Method == "err")
+ Target.reply(ID, make_error("trouble at mill", ErrorCode(88)));
+else
+ Target.reply(ID, std::move(Params));
+return false;
+ }
+
+ bool reply(json::Value ID, Expected Params) override {
+if (Params)
+ Log << "Reply(" << ID << "): " << *Params << "\n";
+else
+ Log << "Reply(" << ID
+ << "): error = " << llvm::toString(Params.takeError()) << "\n";
+return false;
+ }
+};
+
+std::string trim(StringRef S) { return S.trim().str(); }
+
+// Runs an Echo session using the standard JSON-RPC format we use in production.
+TEST_F(JSONTransportTest, StandardDense) {
+ auto T = transport(
+ "Content-Length: 52\r\n\r\n"
+ R"({"jsonrpc": "2.0", "method": "call", "params": 1234})"
+ "Content-Length: 46\r\n\r\n"
+ R"({"jsonrpc": "2.0", "id": 1234, "result": 5678})"
+ "Content-Length: 67\r\n\r\n"
+ R"({"jsonrpc": "2.0", "method": "foo", "id": "abcd", "params": "efgh"})"
+ "Content-Length: 73\r\n\r\n"
+ R"({"jsonrpc": "2.0", "id": "xyz", "error": {"code": 99, "message": "bad!"}})"
+ "Content-Length: 68\r\n\r\n"
+ R"({"jsonrpc": "2.0", "method": "err", "id": "wxyz", "params": "boom!"})"
+ "Content-Length: 36\r\n\r\n"
+ R"({"jsonrpc": "2.0", "method": "exit"})",
+ /*Pretty=*/false, JSONStreamStyle::Standard);
+ Echo E(*T);
+ auto Err = T->loop(E);
+ EXPECT_FALSE(bool(Err)) << llvm::toString(std::move(Err));
+
+ EXPECT_EQ(trim(E.log()), trim(R"(
+Notification call: 1234
+Reply(1234): 5678
+Call foo("abcd"): "efgh"
[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
sammccall added a comment. For reference, @jkorous has a WIP in https://reviews.llvm.org/D53290. It's scope is a superset, and I think everything in common is basically the same (they were both based on a common prototype). Jan is out at the moment, so I think it makes sense to move ahead with this piece - it's progress, and the smaller scope makes landing the change easier. I just realized though there are no JSONTransport unit tests in this patch, I'll add those. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
sammccall added a comment. This patch is big and hard to navigate. I tried to keep it contained, but JSONRPCDispatcher is pretty tangled. The main parts are: - `Transport.h` is the key new abstraction that should be understood first - `JSONTransport.cpp` is its standard implementation. The raw IO stuff in there is just lifted from the old JSONRPCDispatcher, and isn't very interesting. Everything else should be though - it shows how the new interface works. - The `ClangdLSPServer` and `ClangdMain` changes show how the new interface is used - `JSONRPCDispatcher` has had most of its guts ripped out. There's more that can be done there, that file may go away completely. - The test changes just reflect the shutdown getting a little bit stricter (they also pass before this patch) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
sammccall created this revision.
sammccall added reviewers: jkorous, ioeric, hokein.
Herald added subscribers: cfe-commits, kadircet, arphaman, MaskRay,
ilya-biryukov, mgorny.
This paves the way for alternative transports (mac XPC, maybe messagepack?),
and also generally improves layering: testing ClangdLSPServer becomes less of
a pipe dream, we split up the JSONOutput monolith, etc.
This isn't a final state, much of what remains in JSONRPCDispatcher can go away,
handlers can call reply() on the transport directly, JSONOutput can be renamed
to StreamLogger and removed, etc. But this patch is sprawling already.
The main observable change (see tests) is that hitting EOF on input is now an
error: the client should send the 'exit' notification.
This is defensible: the protocol doesn't spell this case out. Reproducing the
current behavior for all combinations of shutdown/exit/EOF clutters interfaces.
We can iterate on this if desired.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53286
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/JSONRPCDispatcher.cpp
clangd/JSONRPCDispatcher.h
clangd/JSONTransport.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/Transport.h
clangd/tool/ClangdMain.cpp
test/clangd/compile-commands-path-in-initialize.test
test/clangd/compile-commands-path.test
test/clangd/completion-snippets.test
test/clangd/completion.test
test/clangd/crash-non-added-files.test
test/clangd/execute-command.test
test/clangd/input-mirror.test
test/clangd/signature-help.test
test/clangd/textdocument-didchange-fail.test
test/clangd/trace.test
test/clangd/xrefs.test
Index: test/clangd/xrefs.test
===
--- test/clangd/xrefs.test
+++ test/clangd/xrefs.test
@@ -55,3 +55,5 @@
# CHECK-NEXT: ]
---
{"jsonrpc":"2.0","id":1,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/trace.test
===
--- test/clangd/trace.test
+++ test/clangd/trace.test
@@ -21,3 +21,5 @@
# CHECK: },
---
{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/textdocument-didchange-fail.test
===
--- test/clangd/textdocument-didchange-fail.test
+++ test/clangd/textdocument-didchange-fail.test
@@ -35,3 +35,5 @@
# CHECK-NEXT:}
---
{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/signature-help.test
===
--- test/clangd/signature-help.test
+++ test/clangd/signature-help.test
@@ -23,3 +23,5 @@
# CHECK-NEXT: }
---
{"jsonrpc":"2.0","id":10,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/input-mirror.test
===
--- test/clangd/input-mirror.test
+++ test/clangd/input-mirror.test
@@ -12,3 +12,6 @@
Content-Length: 44
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+Content-Length: 33
+
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/execute-command.test
===
--- test/clangd/execute-command.test
+++ test/clangd/execute-command.test
@@ -62,3 +62,5 @@
{"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":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/crash-non-added-files.test
===
--- test/clangd/crash-non-added-files.test
+++ test/clangd/crash-non-added-files.test
@@ -32,3 +32,5 @@
{"jsonrpc":"2.0","id":6,"method":"shutdown"}
---
{"jsonrpc":"2.0","method":"exit"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/completion.test
===
--- test/clangd/completion.test
+++ test/clangd/completion.test
@@ -68,3 +68,5 @@
# CHECK-NEXT: ]
---
{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/completion-snippets.test
===
--- test/clangd/completion-snippets.test
+++ test/clangd/completion-snippets.test
@@ -52,3 +52,5 @@
# CHECK-NEXT: }
---
{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/compile-commands-path.test
===
---
