[mlir-lsp] Log invalid notification params

[modocache]

Stacked pull request:

• [mlir-lsp] Initialize Reply::method member  #89857
• -> [mlir-lsp] Log invalid notification params #89856
• [mlir-lsp] Add transport unit tests #89855

---

When the lsp::MessageHandler processes a request with invalid params
(that is, the "params" JSON sent along with the request does not match
the shape expected by the message handler for the given method), it
replies by sending an error response to the client.

On the other hand, the language server protocol specifies that
notifications must not result in responses. As a result, when the
JSON params accompanying a notification cannot be parsed, no error is
sent back; there is no indication that an error has occurred at all.

This patch adds an error log for that case. Although clients cannot
parse error logs, this at least provides an indication that something
went wrong on the language server side.

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Brian Gesiak (modocache)

Changes

Stacked pull request:

• #89855

---

When the lsp::MessageHandler processes a request with invalid params
(that is, the "params" JSON sent along with the request does not match
the shape expected by the message handler for the given method), it
replies by sending an error response to the client.

On the other hand, the language server protocol specifies that
notifications must not result in responses. As a result, when the
JSON params accompanying a notification cannot be parsed, no error is
sent back; there is no indication that an error has occurred at all.

This patch adds an error log for that case. Although clients cannot
parse error logs, this at least provides an indication that something
went wrong on the language server side.

---

Full diff: https://github.com/llvm/llvm-project/pull/89856.diff

5 Files Affected:

• (modified) mlir/include/mlir/Tools/lsp-server-support/Transport.h (+8-3)
• (modified) mlir/unittests/CMakeLists.txt (+1)
• (added) mlir/unittests/Tools/CMakeLists.txt (+1)
• (added) mlir/unittests/Tools/lsp-server-support/CMakeLists.txt (+6)
• (added) mlir/unittests/Tools/lsp-server-support/Transport.cpp (+121)

diff --git a/mlir/include/mlir/Tools/lsp-server-support/Transport.h b/mlir/include/mlir/Tools/lsp-server-support/Transport.h
index ce742be7a941c9..ee9b3184528984 100644
--- a/mlir/include/mlir/Tools/lsp-server-support/Transport.h
+++ b/mlir/include/mlir/Tools/lsp-server-support/Transport.h
@@ -147,9 +147,14 @@ class MessageHandler {
                     void (ThisT::*handler)(const Param &)) {
     notificationHandlers[method] = [method, handler,
                                     thisPtr](llvm::json::Value rawParams) {
-      llvm::Expected<Param> param = parse<Param>(rawParams, method, "request");
-      if (!param)
-        return llvm::consumeError(param.takeError());
+      llvm::Expected<Param> param =
+          parse<Param>(rawParams, method, "notification");
+      if (!param) {
+        return llvm::consumeError(
+            llvm::handleErrors(param.takeError(), [](const LSPError &lspError) {
+              Logger::error(lspError.message.c_str());
+            }));
+      }
       (thisPtr->*handler)(*param);
     };
   }
diff --git a/mlir/unittests/CMakeLists.txt b/mlir/unittests/CMakeLists.txt
index 6fad249a0b2fba..6d8aa290e82f25 100644
--- a/mlir/unittests/CMakeLists.txt
+++ b/mlir/unittests/CMakeLists.txt
@@ -20,6 +20,7 @@ add_subdirectory(Support)
 add_subdirectory(Rewrite)
 add_subdirectory(TableGen)
 add_subdirectory(Target)
+add_subdirectory(Tools)
 add_subdirectory(Transforms)
 
 if(MLIR_ENABLE_EXECUTION_ENGINE)
diff --git a/mlir/unittests/Tools/CMakeLists.txt b/mlir/unittests/Tools/CMakeLists.txt
new file mode 100644
index 00000000000000..a97588d9286685
--- /dev/null
+++ b/mlir/unittests/Tools/CMakeLists.txt
@@ -0,0 +1 @@
+add_subdirectory(lsp-server-support)
diff --git a/mlir/unittests/Tools/lsp-server-support/CMakeLists.txt b/mlir/unittests/Tools/lsp-server-support/CMakeLists.txt
new file mode 100644
index 00000000000000..3aa8b9c4bc7735
--- /dev/null
+++ b/mlir/unittests/Tools/lsp-server-support/CMakeLists.txt
@@ -0,0 +1,6 @@
+add_mlir_unittest(MLIRLspServerSupportTests
+  Transport.cpp
+)
+target_link_libraries(MLIRLspServerSupportTests
+  PRIVATE
+  MLIRLspServerSupportLib)
diff --git a/mlir/unittests/Tools/lsp-server-support/Transport.cpp b/mlir/unittests/Tools/lsp-server-support/Transport.cpp
new file mode 100644
index 00000000000000..48eae32a0fc3a4
--- /dev/null
+++ b/mlir/unittests/Tools/lsp-server-support/Transport.cpp
@@ -0,0 +1,121 @@
+//===- Transport.cpp - LSP JSON transport unit tests ----------------------===//
+//
+// 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 "mlir/Tools/lsp-server-support/Transport.h"
+#include "mlir/Tools/lsp-server-support/Logging.h"
+#include "mlir/Tools/lsp-server-support/Protocol.h"
+#include "llvm/Support/FileSystem.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace mlir;
+using namespace mlir::lsp;
+using namespace testing;
+
+namespace {
+
+TEST(TransportTest, SendReply) {
+  std::string out;
+  llvm::raw_string_ostream os(out);
+  JSONTransport transport(nullptr, os);
+  MessageHandler handler(transport);
+
+  transport.reply(1989, nullptr);
+  EXPECT_THAT(out, HasSubstr("\"id\":1989"));
+  EXPECT_THAT(out, HasSubstr("\"result\":null"));
+}
+
+class TransportInputTest : public Test {
+  std::optional<llvm::sys::fs::TempFile> inputTempFile;
+  std::FILE *in = nullptr;
+  std::string output = "";
+  llvm::raw_string_ostream os;
+  std::optional<JSONTransport> transport = std::nullopt;
+  std::optional<MessageHandler> messageHandler = std::nullopt;
+
+protected:
+  TransportInputTest() : os(output) {}
+
+  void SetUp() override {
+    auto tempOr = llvm::sys::fs::TempFile::create("lsp-unittest-%%%%%%.json");
+    ASSERT_TRUE((bool)tempOr);
+    llvm::sys::fs::TempFile t = std::move(*tempOr);
+    inputTempFile = std::move(t);
+
+    in = std::fopen(inputTempFile->TmpName.c_str(), "r");
+    transport.emplace(in, os, JSONStreamStyle::Delimited);
+    messageHandler.emplace(*transport);
+  }
+
+  void TearDown() override {
+    EXPECT_FALSE(inputTempFile->discard());
+    EXPECT_EQ(std::fclose(in), 0);
+  }
+
+  void writeInput(StringRef buffer) {
+    std::error_code ec;
+    llvm::raw_fd_ostream os(inputTempFile->TmpName, ec);
+    ASSERT_FALSE(ec);
+    os << buffer;
+    os.close();
+  }
+
+  StringRef getOutput() const { return output; }
+  MessageHandler &getMessageHandler() { return *messageHandler; }
+
+  void runTransport() {
+    bool gotEOF = false;
+    llvm::Error err = llvm::handleErrors(
+        transport->run(*messageHandler), [&](const llvm::ECError &ecErr) {
+          gotEOF = ecErr.convertToErrorCode() == std::errc::io_error;
+        });
+    llvm::consumeError(std::move(err));
+    EXPECT_TRUE(gotEOF);
+  }
+};
+
+TEST_F(TransportInputTest, RequestWithInvalidParams) {
+  struct Handler {
+    void onMethod(const TextDocumentItem &params,
+                  mlir::lsp::Callback<TextDocumentIdentifier> callback) {}
+  } handler;
+  getMessageHandler().method("invalid-params-request", &handler,
+                             &Handler::onMethod);
+
+  writeInput("{\"jsonrpc\":\"2.0\",\"id\":92,"
+             "\"method\":\"invalid-params-request\",\"params\":{}}\n");
+  runTransport();
+  EXPECT_THAT(getOutput(), HasSubstr("error"));
+  EXPECT_THAT(getOutput(), HasSubstr("missing value at (root).uri"));
+}
+
+TEST_F(TransportInputTest, NotificationWithInvalidParams) {
+  // JSON parsing errors are only reported via error logging. As a result, this
+  // test can't make any expectations -- but it prints the output anyway, by way
+  // of demonstration.
+  Logger::setLogLevel(Logger::Level::Error);
+
+  struct Handler {
+    void onNotification(const TextDocumentItem &params) {}
+  } handler;
+  getMessageHandler().notification("invalid-params-notification", &handler,
+                                   &Handler::onNotification);
+
+  writeInput("{\"jsonrpc\":\"2.0\",\"method\":\"invalid-params-notification\","
+             "\"params\":{}}\n");
+  runTransport();
+}
+
+TEST_F(TransportInputTest, MethodNotFound) {
+  writeInput("{\"jsonrpc\":\"2.0\",\"id\":29,\"method\":\"ack\"}\n");
+  runTransport();
+  EXPECT_THAT(getOutput(), HasSubstr("\"id\":29"));
+  EXPECT_THAT(getOutput(), HasSubstr("\"error\""));
+  EXPECT_THAT(getOutput(), HasSubstr("\"message\":\"method not found: ack\""));
+}
+} // namespace

[walter-erquinigo]

should this has been "notification" instead of "request" all along?

[modocache]

I assume it was a typo, yeah. But the string would never be printed or logged anywhere prior to this change, so it didn't matter in practice :)

[walter-erquinigo]

you could also be more specific, e.g.

Error when parsing the notification JSON packet: ...

[modocache]

So, just to be clear, that would result in:

E[12:57:59.961] Error when parsing the notification JSON packet: failed to decode invalid-params-notification notification: missing value at (root).uri

Which mentions "notification" twice. Personally I think that's equally informative as the current patch:

E[12:57:59.961] JSON parsing error: failed to decode invalid-params-notification notification: missing value at (root).uri

[walter-erquinigo]

Ahh, then nvm. Your code seems right already