[Mlir-commits] [mlir] 37e13d4 - [mlir-lsp] Log invalid notification params (#89856)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Wed Apr 24 12:14:42 PDT 2024
Author: Brian Gesiak
Date: 2024-04-24T15:14:38-04:00
New Revision: 37e13d4924841bd84edb8c67c667d6d2a6c2bc63
URL: https://github.com/llvm/llvm-project/commit/37e13d4924841bd84edb8c67c667d6d2a6c2bc63
DIFF: https://github.com/llvm/llvm-project/commit/37e13d4924841bd84edb8c67c667d6d2a6c2bc63.diff
LOG: [mlir-lsp] Log invalid notification params (#89856)
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.
Added:
Modified:
mlir/include/mlir/Tools/lsp-server-support/Transport.h
mlir/unittests/Tools/lsp-server-support/Transport.cpp
Removed:
################################################################################
diff --git a/mlir/include/mlir/Tools/lsp-server-support/Transport.h b/mlir/include/mlir/Tools/lsp-server-support/Transport.h
index ce742be7a941c9..44c71058cf717c 100644
--- a/mlir/include/mlir/Tools/lsp-server-support/Transport.h
+++ b/mlir/include/mlir/Tools/lsp-server-support/Transport.h
@@ -147,9 +147,15 @@ 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("JSON parsing error: {0}",
+ 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
More information about the Mlir-commits
mailing list