[Mlir-commits] [mlir] [mlir-lsp] Initialize Reply::method member (PR #89857)
Brian Gesiak
llvmlistbot at llvm.org
Tue Apr 23 18:52:40 PDT 2024
https://github.com/modocache created https://github.com/llvm/llvm-project/pull/89857
Stacked pull requests:
- #89856
- #89855
---
When debug level logging is enabled (by adding a call to
`Logger::setLogLevel(Logger::Level::Debug)`), the
`TransportInputTest.RequestWithInvalidParams` unit test logs:
```
[18:35:00.565] --> reply:(92)
```
The format string for this log statement is `"--> reply:{0}({1})"`,
where `{0}` is the original request's method name (that is, the method
name of the request being replied to), and `{1}` is the request ID.
However, because the `Reply` class never initializes its `method`
member, `{0}` is always empty. Initializing it results in the (nicer)
log error below:
```
I[18:35:00.565] --> reply:invalid-params-request(92)
```
Because this is only ever logged for now, its not possible to add a test
case for this. Future patches will rely on `method` being initialized,
however, and will add test cases for this path.
>From e6c453eea9e63f955e44d11a75dcca477bfc8600 Mon Sep 17 00:00:00 2001
From: Brian Gesiak <brian at modocache.io>
Date: Tue, 23 Apr 2024 13:21:16 -0400
Subject: [PATCH 1/3] [mlir-lsp] Add transport unit tests
Add unit tests for some aspects of the JSON transport and message
handler. These will be expanded in future patches as behavior is
modified.
---
mlir/unittests/CMakeLists.txt | 1 +
mlir/unittests/Tools/CMakeLists.txt | 1 +
.../Tools/lsp-server-support/CMakeLists.txt | 6 ++
.../Tools/lsp-server-support/Transport.cpp | 65 +++++++++++++++++++
4 files changed, 73 insertions(+)
create mode 100644 mlir/unittests/Tools/CMakeLists.txt
create mode 100644 mlir/unittests/Tools/lsp-server-support/CMakeLists.txt
create mode 100644 mlir/unittests/Tools/lsp-server-support/Transport.cpp
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..9877c12c3695be
--- /dev/null
+++ b/mlir/unittests/Tools/lsp-server-support/Transport.cpp
@@ -0,0 +1,65 @@
+//===- 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 "llvm/ADT/ScopeExit.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"));
+}
+
+TEST(TransportTest, MethodNotFound) {
+ auto tempOr = llvm::sys::fs::TempFile::create("lsp-unittest-%%%%%%.json");
+ ASSERT_TRUE((bool)tempOr);
+ auto discardTemp =
+ llvm::make_scope_exit([&]() { ASSERT_FALSE((bool)tempOr->discard()); });
+
+ {
+ std::error_code ec;
+ llvm::raw_fd_ostream os(tempOr->TmpName, ec);
+ ASSERT_FALSE(ec);
+ os << "{\"jsonrpc\":\"2.0\",\"id\":29,\"method\":\"ack\"}\n";
+ os.close();
+ }
+
+ std::string out;
+ llvm::raw_string_ostream os(out);
+ std::FILE *in = std::fopen(tempOr->TmpName.c_str(), "r");
+ auto closeIn = llvm::make_scope_exit([&]() { std::fclose(in); });
+
+ JSONTransport transport(in, os, JSONStreamStyle::Delimited);
+ MessageHandler handler(transport);
+
+ bool gotEOF = false;
+ llvm::Error err = llvm::handleErrors(
+ transport.run(handler), [&](const llvm::ECError &ecErr) {
+ gotEOF = ecErr.convertToErrorCode() == std::errc::io_error;
+ });
+ llvm::consumeError(std::move(err));
+ EXPECT_TRUE(gotEOF);
+ EXPECT_THAT(out, HasSubstr("\"id\":29"));
+ EXPECT_THAT(out, HasSubstr("\"error\""));
+ EXPECT_THAT(out, HasSubstr("\"message\":\"method not found: ack\""));
+}
+} // namespace
>From b8aeba8e8347a47fa081651233a184f5e9046c1e Mon Sep 17 00:00:00 2001
From: Brian Gesiak <brian at modocache.io>
Date: Tue, 23 Apr 2024 18:19:38 -0400
Subject: [PATCH 2/3] [mlir-lsp] Log invalid notification params
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.
---
.../mlir/Tools/lsp-server-support/Transport.h | 11 +-
.../Tools/lsp-server-support/Transport.cpp | 106 +++++++++++++-----
2 files changed, 89 insertions(+), 28 deletions(-)
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/Tools/lsp-server-support/Transport.cpp b/mlir/unittests/Tools/lsp-server-support/Transport.cpp
index 9877c12c3695be..48eae32a0fc3a4 100644
--- a/mlir/unittests/Tools/lsp-server-support/Transport.cpp
+++ b/mlir/unittests/Tools/lsp-server-support/Transport.cpp
@@ -7,7 +7,8 @@
//===----------------------------------------------------------------------===//
#include "mlir/Tools/lsp-server-support/Transport.h"
-#include "llvm/ADT/ScopeExit.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"
@@ -29,37 +30,92 @@ TEST(TransportTest, SendReply) {
EXPECT_THAT(out, HasSubstr("\"result\":null"));
}
-TEST(TransportTest, MethodNotFound) {
- auto tempOr = llvm::sys::fs::TempFile::create("lsp-unittest-%%%%%%.json");
- ASSERT_TRUE((bool)tempOr);
- auto discardTemp =
- llvm::make_scope_exit([&]() { ASSERT_FALSE((bool)tempOr->discard()); });
+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(tempOr->TmpName, ec);
+ llvm::raw_fd_ostream os(inputTempFile->TmpName, ec);
ASSERT_FALSE(ec);
- os << "{\"jsonrpc\":\"2.0\",\"id\":29,\"method\":\"ack\"}\n";
+ os << buffer;
os.close();
}
- std::string out;
- llvm::raw_string_ostream os(out);
- std::FILE *in = std::fopen(tempOr->TmpName.c_str(), "r");
- auto closeIn = llvm::make_scope_exit([&]() { std::fclose(in); });
+ StringRef getOutput() const { return output; }
+ MessageHandler &getMessageHandler() { return *messageHandler; }
- JSONTransport transport(in, os, JSONStreamStyle::Delimited);
- MessageHandler handler(transport);
+ 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 ¶ms,
+ 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 ¶ms) {}
+ } handler;
+ getMessageHandler().notification("invalid-params-notification", &handler,
+ &Handler::onNotification);
+
+ writeInput("{\"jsonrpc\":\"2.0\",\"method\":\"invalid-params-notification\","
+ "\"params\":{}}\n");
+ runTransport();
+}
- bool gotEOF = false;
- llvm::Error err = llvm::handleErrors(
- transport.run(handler), [&](const llvm::ECError &ecErr) {
- gotEOF = ecErr.convertToErrorCode() == std::errc::io_error;
- });
- llvm::consumeError(std::move(err));
- EXPECT_TRUE(gotEOF);
- EXPECT_THAT(out, HasSubstr("\"id\":29"));
- EXPECT_THAT(out, HasSubstr("\"error\""));
- EXPECT_THAT(out, HasSubstr("\"message\":\"method not found: ack\""));
+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
>From c27de89bf5316707078ba5066e36c3188d466af2 Mon Sep 17 00:00:00 2001
From: Brian Gesiak <brian at modocache.io>
Date: Tue, 23 Apr 2024 12:50:56 -0400
Subject: [PATCH 3/3] [mlir-lsp] Initialize `Reply::method` member
When debug level logging is enabled (by adding a call to
`Logger::setLogLevel(Logger::Level::Debug)`), the
`TransportInputTest.RequestWithInvalidParams` unit test logs:
```
[18:35:00.565] --> reply:(92)
```
The format string for this log statement is `"--> reply:{0}({1})"`,
where `{0}` is the original request's method name (that is, the method
name of the request being replied to), and `{1}` is the request ID.
However, because the `Reply` class never initializes its `method`
member, `{0}` is always empty. Initializing it results in the (nicer)
log error below:
```
I[18:35:00.565] --> reply:invalid-params-request(92)
```
Because this is only ever logged for now, its not possible to add a test
case for this. Future patches will rely on `method` being initialized,
however, and will add test cases for this path.
---
mlir/lib/Tools/lsp-server-support/Transport.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/mlir/lib/Tools/lsp-server-support/Transport.cpp b/mlir/lib/Tools/lsp-server-support/Transport.cpp
index 64dea35614c070..339c5f3825165d 100644
--- a/mlir/lib/Tools/lsp-server-support/Transport.cpp
+++ b/mlir/lib/Tools/lsp-server-support/Transport.cpp
@@ -51,12 +51,12 @@ class Reply {
Reply::Reply(const llvm::json::Value &id, llvm::StringRef method,
JSONTransport &transport, std::mutex &transportOutputMutex)
- : id(id), transport(&transport),
+ : method(method), id(id), transport(&transport),
transportOutputMutex(transportOutputMutex) {}
Reply::Reply(Reply &&other)
- : replied(other.replied.load()), id(std::move(other.id)),
- transport(other.transport),
+ : method(other.method), replied(other.replied.load()),
+ id(std::move(other.id)), transport(other.transport),
transportOutputMutex(other.transportOutputMutex) {
other.transport = nullptr;
}
More information about the Mlir-commits
mailing list