[Mlir-commits] [mlir] [mlir-lsp] Initialize Reply::method member (PR #89857)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Tue Apr 23 18:53:08 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir
Author: Brian Gesiak (modocache)
<details>
<summary>Changes</summary>
Stacked pull requests:
- -> #<!-- -->89857
- #<!-- -->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.
---
Full diff: https://github.com/llvm/llvm-project/pull/89857.diff
6 Files Affected:
- (modified) mlir/include/mlir/Tools/lsp-server-support/Transport.h (+8-3)
- (modified) mlir/lib/Tools/lsp-server-support/Transport.cpp (+3-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
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/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;
}
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 ¶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();
+}
+
+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
``````````
</details>
https://github.com/llvm/llvm-project/pull/89857
More information about the Mlir-commits
mailing list