Skip to content

[mlir-lsp] Rename OutgoingNotification #90076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 25, 2024
Merged

[mlir-lsp] Rename OutgoingNotification #90076

merged 1 commit into from
Apr 25, 2024

Conversation

modocache
Copy link
Member

Rename OutgoingNotification to OutgoingMessage, since the same
callback function type will be used in a future commit to represent
outgoing requests, in addition to outgoing notifications.

No functional change to behavior here, but an additional test is added
for outgoing notifications.

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Apr 25, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 25, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Brian Gesiak (modocache)

Changes

Rename OutgoingNotification to OutgoingMessage, since the same
callback function type will be used in a future commit to represent
outgoing requests, in addition to outgoing notifications.

No functional change to behavior here, but an additional test is added
for outgoing notifications.


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

5 Files Affected:

  • (modified) mlir/include/mlir/Tools/lsp-server-support/Transport.h (+6-5)
  • (modified) mlir/lib/Tools/mlir-lsp-server/LSPServer.cpp (+1-1)
  • (modified) mlir/lib/Tools/mlir-pdll-lsp-server/LSPServer.cpp (+1-1)
  • (modified) mlir/lib/Tools/tblgen-lsp-server/LSPServer.cpp (+1-1)
  • (modified) mlir/unittests/Tools/lsp-server-support/Transport.cpp (+7)
diff --git a/mlir/include/mlir/Tools/lsp-server-support/Transport.h b/mlir/include/mlir/Tools/lsp-server-support/Transport.h
index 44c71058cf717c..c56e7219fff940 100644
--- a/mlir/include/mlir/Tools/lsp-server-support/Transport.h
+++ b/mlir/include/mlir/Tools/lsp-server-support/Transport.h
@@ -95,10 +95,10 @@ class JSONTransport {
 template <typename T>
 using Callback = llvm::unique_function<void(llvm::Expected<T>)>;
 
-/// An OutgoingNotification<T> is a function used for outgoing notifications
-/// send to the client.
+/// An OutgoingMessage<T> is a function used for outgoing requests o
+/// notifications to send to the client.
 template <typename T>
-using OutgoingNotification = llvm::unique_function<void(const T &)>;
+using OutgoingMessage = llvm::unique_function<void(const T &)>;
 
 /// A handler used to process the incoming transport messages.
 class MessageHandler {
@@ -160,9 +160,10 @@ class MessageHandler {
     };
   }
 
-  /// Create an OutgoingNotification object used for the given method.
+  /// Create an OutgoingMessage function that, when called, sends a notification
+  /// with the given method via the transport.
   template <typename T>
-  OutgoingNotification<T> outgoingNotification(llvm::StringLiteral method) {
+  OutgoingMessage<T> outgoingNotification(llvm::StringLiteral method) {
     return [&, method](const T &params) {
       std::lock_guard<std::mutex> transportLock(transportOutputMutex);
       Logger::info("--> {0}", method);
diff --git a/mlir/lib/Tools/mlir-lsp-server/LSPServer.cpp b/mlir/lib/Tools/mlir-lsp-server/LSPServer.cpp
index 0f23366f6fe80a..bd7f2a5dedc257 100644
--- a/mlir/lib/Tools/mlir-lsp-server/LSPServer.cpp
+++ b/mlir/lib/Tools/mlir-lsp-server/LSPServer.cpp
@@ -91,7 +91,7 @@ struct LSPServer {
 
   /// An outgoing notification used to send diagnostics to the client when they
   /// are ready to be processed.
-  OutgoingNotification<PublishDiagnosticsParams> publishDiagnostics;
+  OutgoingMessage<PublishDiagnosticsParams> publishDiagnostics;
 
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.
diff --git a/mlir/lib/Tools/mlir-pdll-lsp-server/LSPServer.cpp b/mlir/lib/Tools/mlir-pdll-lsp-server/LSPServer.cpp
index f02372367e38c8..ffaa1c8d4de46f 100644
--- a/mlir/lib/Tools/mlir-pdll-lsp-server/LSPServer.cpp
+++ b/mlir/lib/Tools/mlir-pdll-lsp-server/LSPServer.cpp
@@ -104,7 +104,7 @@ struct LSPServer {
 
   /// An outgoing notification used to send diagnostics to the client when they
   /// are ready to be processed.
-  OutgoingNotification<PublishDiagnosticsParams> publishDiagnostics;
+  OutgoingMessage<PublishDiagnosticsParams> publishDiagnostics;
 
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.
diff --git a/mlir/lib/Tools/tblgen-lsp-server/LSPServer.cpp b/mlir/lib/Tools/tblgen-lsp-server/LSPServer.cpp
index b62f68db9d60fa..bc312d18ea4037 100644
--- a/mlir/lib/Tools/tblgen-lsp-server/LSPServer.cpp
+++ b/mlir/lib/Tools/tblgen-lsp-server/LSPServer.cpp
@@ -72,7 +72,7 @@ struct LSPServer {
 
   /// An outgoing notification used to send diagnostics to the client when they
   /// are ready to be processed.
-  OutgoingNotification<PublishDiagnosticsParams> publishDiagnostics;
+  OutgoingMessage<PublishDiagnosticsParams> publishDiagnostics;
 
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.
diff --git a/mlir/unittests/Tools/lsp-server-support/Transport.cpp b/mlir/unittests/Tools/lsp-server-support/Transport.cpp
index 48eae32a0fc3a4..b46f02bc4b197b 100644
--- a/mlir/unittests/Tools/lsp-server-support/Transport.cpp
+++ b/mlir/unittests/Tools/lsp-server-support/Transport.cpp
@@ -118,4 +118,11 @@ TEST_F(TransportInputTest, MethodNotFound) {
   EXPECT_THAT(getOutput(), HasSubstr("\"error\""));
   EXPECT_THAT(getOutput(), HasSubstr("\"message\":\"method not found: ack\""));
 }
+
+TEST_F(TransportInputTest, OutgoingNotification) {
+  auto notifyFn = getMessageHandler().outgoingNotification<CompletionList>(
+      "outgoing-notification");
+  notifyFn(CompletionList{});
+  EXPECT_THAT(getOutput(), HasSubstr("\"method\":\"outgoing-notification\""));
+}
 } // namespace

Copy link
Contributor

@River707 River707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typedefs are fairly cheap, so it isn't really that necessary to share here, but the code still reads fine after this change, so LGTM. Could you split out the new test from the rename? Can be done when landing.

modocache added a commit that referenced this pull request Apr 25, 2024
Add a unit test exercising `lsp::MessageHanlder::outgoingNotification`.
Split off from pull request #90076, as requested.
Rename `OutgoingNotification` to `OutgoingMessage`, since the same
callback function type will be used in a future commit to represent
outgoing requests, in addition to outgoing notifications.
@modocache modocache merged commit b77416e into llvm:main Apr 25, 2024
3 of 4 checks passed
@modocache modocache deleted the lsp-0 branch April 25, 2024 19:39
modocache added a commit that referenced this pull request Apr 26, 2024
This reverts commit b77416e. While that seemed like an
appropriate rename at the time, further review on pull request #90078
makes it unnecessary. This revert changes the name back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants