[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Nice! The code looks much clearer. Just some nits



================
Comment at: clangd/ClangdLSPServer.cpp:156
+void ClangdLSPServer::onExit(ExitParams &Params) {
+  // 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 &Out) {
-  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, &Out[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(&Out[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<std::string> readStandardMessage(std::FILE *In,
-                                                       JSONOutput &Out) {
-  // 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: 9999999999999999"
-  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, &JSON[Pos], 1,
-                                       ContentLength - Pos, In);
-    Out.mirrorInput(StringRef(&JSON[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<std::string> readDelimitedMessage(std::FILE *In,
-                                                        JSONOutput &Out) {
-  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 some explanation.
 // Previously, std::istream was used. When a debugger attached on MacOS, the
----------------
The comment should also be moved?


================
Comment at: clangd/JSONRPCDispatcher.h:29
+// Logs to an output stream, such as stderr.
+// TODO: rename and move.
 class JSONOutput : public Logger {
----------------
nit: make the FIXME more concrete?


================
Comment at: clangd/JSONTransport.cpp:23
+    Code = L.Code;
+    return make_error<StringError>(L.Message, inconvertibleErrorCode());
+  }));
----------------
Is `E` guaranteed to be an `LSPError`?  Do we need a handler for other errors?


================
Comment at: clangd/JSONTransport.cpp:83
+          vlog(Pretty ? "<<< {0:2}\n" : "<<< {0}\n", *Doc);
+          if (handleMessage(std::move(*Doc), Handler))
+            return Error::success();
----------------
Maybe add a comment here that the return value of `handleMessage` indicates whether to shutdown. It' not clear without context.


================
Comment at: clangd/JSONTransport.cpp:129
+  if (!Object || Object->getString("jsonrpc") != Optional<StringRef>("2.0")) {
+    elog("Not a JSON-RPC message: {0:2}", Message);
+    return false;
----------------
nit: add `2.0` to the log?


================
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,
----------------
nit: maybe `handleNotify` to make the two directions more distinguishable? This can get a bit counter-intuitive in the call sites.


================
Comment at: unittests/clangd/JSONTransportTests.cpp:106
+  auto Err = T->loop(E);
+  EXPECT_FALSE(bool(Err)) << llvm::toString(std::move(Err));
+
----------------
Would `operator<<` be used when the error is real (i.e. check passes)? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53286