[PATCH] D96544: [clangd] Extract binding of typed->untyped LSP handlers to LSPBinder. NFC

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked 7 inline comments as done.
Closed by commit rG5786f64a4ec8: [clangd] Extract binding of typed-untyped 
LSP handlers to LSPBinder. NFC (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D96544?vs=323449=323694#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96544/new/

https://reviews.llvm.org/D96544

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/LSPBinder.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/LSPBinderTests.cpp

Index: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
@@ -0,0 +1,100 @@
+//===-- LSPBinderTests.cpp ===//
+//
+// 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 "LSPBinder.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::HasSubstr;
+using testing::UnorderedElementsAre;
+
+// JSON-serializable type for testing.
+struct Foo {
+  int X;
+};
+bool fromJSON(const llvm::json::Value , Foo , llvm::json::Path P) {
+  return fromJSON(V, F.X, P.field("X"));
+}
+llvm::json::Value toJSON(const Foo ) { return F.X; }
+
+// Creates a Callback that writes its received value into an Optional.
+template 
+llvm::unique_function)>
+capture(llvm::Optional> ) {
+  Out.reset();
+  return [](llvm::Expected V) { Out.emplace(std::move(V)); };
+}
+
+TEST(LSPBinderTest, IncomingCalls) {
+  LSPBinder::RawHandlers RawHandlers;
+  LSPBinder Binder{RawHandlers};
+  struct Handler {
+void plusOne(const Foo , Callback Reply) {
+  Reply(Foo{Params.X + 1});
+}
+void fail(const Foo , Callback Reply) {
+  Reply(error("X={0}", Params.X));
+}
+void notify(const Foo ) {
+  LastNotify = Params.X;
+  ++NotifyCount;
+}
+int LastNotify = -1;
+int NotifyCount = 0;
+  };
+
+  Handler H;
+  Binder.method("plusOne", , ::plusOne);
+  Binder.method("fail", , ::fail);
+  Binder.notification("notify", , ::notify);
+  Binder.command("cmdPlusOne", , ::plusOne);
+  ASSERT_THAT(RawHandlers.MethodHandlers.keys(),
+  UnorderedElementsAre("plusOne", "fail"));
+  ASSERT_THAT(RawHandlers.NotificationHandlers.keys(),
+  UnorderedElementsAre("notify"));
+  ASSERT_THAT(RawHandlers.CommandHandlers.keys(),
+  UnorderedElementsAre("cmdPlusOne"));
+  llvm::Optional> Reply;
+
+  auto  = RawHandlers.MethodHandlers["plusOne"];
+  RawPlusOne(1, capture(Reply));
+  ASSERT_TRUE(Reply.hasValue());
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+  RawPlusOne("foo", capture(Reply));
+  ASSERT_TRUE(Reply.hasValue());
+  EXPECT_THAT_EXPECTED(
+  Reply.getValue(),
+  llvm::FailedWithMessage(
+  HasSubstr("failed to decode plusOne request: expected integer")));
+
+  auto  = RawHandlers.MethodHandlers["fail"];
+  RawFail(2, capture(Reply));
+  ASSERT_TRUE(Reply.hasValue());
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::FailedWithMessage("X=2"));
+
+  auto  = RawHandlers.NotificationHandlers["notify"];
+  RawNotify(42);
+  EXPECT_EQ(H.LastNotify, 42);
+  EXPECT_EQ(H.NotifyCount, 1);
+  RawNotify("hi"); // invalid, will be logged
+  EXPECT_EQ(H.LastNotify, 42);
+  EXPECT_EQ(H.NotifyCount, 1);
+
+  auto  = RawHandlers.CommandHandlers["cmdPlusOne"];
+  RawCmdPlusOne(1, capture(Reply));
+  ASSERT_TRUE(Reply.hasValue());
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -70,6 +70,7 @@
   IndexTests.cpp
   JSONTransportTests.cpp
   LoggerTests.cpp
+  LSPBinderTests.cpp
   LSPClient.cpp
   ModulesTests.cpp
   ParsedASTTests.cpp
Index: clang-tools-extra/clangd/LSPBinder.h
===
--- /dev/null
+++ clang-tools-extra/clangd/LSPBinder.h
@@ -0,0 +1,147 @@
+//===--- LSPBinder.h - Tables of LSP handlers *- C++-*-===//
+//
+// 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: 

[PATCH] D96544: [clangd] Extract binding of typed->untyped LSP handlers to LSPBinder. NFC

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! (I wish outgoing calls were not so different :/)




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:173
 } else {
-  log("unhandled notification {0}", Method);
+  auto Handler = Server.Handlers.NotificationHandlers.find(Method);
+  if (Handler != Server.Handlers.NotificationHandlers.end()) {

why not keep the old lookup style ? since handlers are unique_functions, 
checking for null should still suffice (and be cheap)



Comment at: clang-tools-extra/clangd/LSPBinder.h:67
+  /// e.g. Bind.command("load", this, ::load);
+  /// Handler should be e.g. void peek(const LoadParams&, 
Callback);
+  /// LoadParams must be JSON-parseable and LoadResult must be serializable.

s/peek/load



Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:18
+
+using testing::_;
+using testing::HasSubstr;

please fix



Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:20
+using testing::HasSubstr;
+using testing::Pair;
+using testing::UnorderedElementsAre;

please fix



Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:25
+struct Foo {
+  int x;
+};

please fix (we usually follow camelCase for json-serializable types in clangd, 
but i don't think it is worth doing in tests)



Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:28
+bool fromJSON(const llvm::json::Value , Foo , llvm::json::Path P) {
+  return fromJSON(V, F.x, P);
+}

nit: s/P/P.field("x")



Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:51
+void notify(const Foo ) { lastNotify = Params.x; }
+int lastNotify = -1;
+  };

please fix, and maybe have a counter, in addition to last value to check for 
"invalid type" case?



Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:69
+  RawPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+  RawPlusOne("foo", capture(Reply));

nit: `ASSERT_THAT(Reply.hasValue())`

here and elsewhere


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96544/new/

https://reviews.llvm.org/D96544

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96544: [clangd] Extract binding of typed->untyped LSP handlers to LSPBinder. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 323449.
sammccall added a comment.

Add doc comments to bind methods


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96544/new/

https://reviews.llvm.org/D96544

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/LSPBinder.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/LSPBinderTests.cpp

Index: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
@@ -0,0 +1,92 @@
+//===-- LSPBinderTests.cpp ===//
+//
+// 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 "LSPBinder.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::_;
+using testing::HasSubstr;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// JSON-serializable type for testing.
+struct Foo {
+  int x;
+};
+bool fromJSON(const llvm::json::Value , Foo , llvm::json::Path P) {
+  return fromJSON(V, F.x, P);
+}
+llvm::json::Value toJSON(const Foo ) { return F.x; }
+
+// Creates a Callback that writes its received value into an Optional.
+template 
+llvm::unique_function)>
+capture(llvm::Optional> ) {
+  Out.reset();
+  return [](llvm::Expected V) { Out.emplace(std::move(V)); };
+}
+
+TEST(LSPBinderTest, IncomingCalls) {
+  LSPBinder::RawHandlers RawHandlers;
+  LSPBinder Binder{RawHandlers};
+  struct Handler {
+void plusOne(const Foo , Callback Reply) {
+  Reply(Foo{Params.x + 1});
+}
+void fail(const Foo , Callback Reply) {
+  Reply(error("x={0}", Params.x));
+}
+void notify(const Foo ) { lastNotify = Params.x; }
+int lastNotify = -1;
+  };
+
+  Handler H;
+  Binder.method("plusOne", , ::plusOne);
+  Binder.method("fail", , ::fail);
+  Binder.notification("notify", , ::notify);
+  Binder.command("cmdPlusOne", , ::plusOne);
+  ASSERT_THAT(RawHandlers.MethodHandlers.keys(),
+  UnorderedElementsAre("plusOne", "fail"));
+  ASSERT_THAT(RawHandlers.NotificationHandlers.keys(),
+  UnorderedElementsAre("notify"));
+  ASSERT_THAT(RawHandlers.CommandHandlers.keys(),
+  UnorderedElementsAre("cmdPlusOne"));
+  llvm::Optional> Reply;
+
+  auto  = RawHandlers.MethodHandlers["plusOne"];
+  RawPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+  RawPlusOne("foo", capture(Reply));
+  EXPECT_THAT_EXPECTED(
+  Reply.getValue(),
+  llvm::FailedWithMessage(
+  HasSubstr("failed to decode plusOne request: expected integer")));
+
+  auto  = RawHandlers.MethodHandlers["fail"];
+  RawFail(2, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::FailedWithMessage("x=2"));
+
+  auto  = RawHandlers.NotificationHandlers["notify"];
+  RawNotify(42);
+  EXPECT_EQ(H.lastNotify, 42);
+  RawNotify("hi"); // invalid, will be logged
+  EXPECT_EQ(H.lastNotify, 42);
+
+  auto  = RawHandlers.CommandHandlers["cmdPlusOne"];
+  RawCmdPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -70,6 +70,7 @@
   IndexTests.cpp
   JSONTransportTests.cpp
   LoggerTests.cpp
+  LSPBinderTests.cpp
   LSPClient.cpp
   ModulesTests.cpp
   ParsedASTTests.cpp
Index: clang-tools-extra/clangd/LSPBinder.h
===
--- /dev/null
+++ clang-tools-extra/clangd/LSPBinder.h
@@ -0,0 +1,147 @@
+//===--- LSPBinder.h - Tables of LSP handlers *- C++-*-===//
+//
+// 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_LSPBINDER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_LSPBINDER_H
+
+#include "Protocol.h"
+#include "support/Function.h"
+#include "support/Logger.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/JSON.h"
+
+namespace clang {
+namespace clangd {
+
+/// LSPBinder collects a table of functions that 

[PATCH] D96544: [clangd] Extract binding of typed->untyped LSP handlers to LSPBinder. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 323438.
sammccall added a comment.

Revert to previous version


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96544/new/

https://reviews.llvm.org/D96544

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/LSPBinder.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/LSPBinderTests.cpp

Index: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
@@ -0,0 +1,92 @@
+//===-- LSPBinderTests.cpp ===//
+//
+// 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 "LSPBinder.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::_;
+using testing::HasSubstr;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// JSON-serializable type for testing.
+struct Foo {
+  int x;
+};
+bool fromJSON(const llvm::json::Value , Foo , llvm::json::Path P) {
+  return fromJSON(V, F.x, P);
+}
+llvm::json::Value toJSON(const Foo ) { return F.x; }
+
+// Creates a Callback that writes its received value into an Optional.
+template 
+llvm::unique_function)>
+capture(llvm::Optional> ) {
+  Out.reset();
+  return [](llvm::Expected V) { Out.emplace(std::move(V)); };
+}
+
+TEST(LSPBinderTest, IncomingCalls) {
+  LSPBinder::RawHandlers RawHandlers;
+  LSPBinder Binder{RawHandlers};
+  struct Handler {
+void plusOne(const Foo , Callback Reply) {
+  Reply(Foo{Params.x + 1});
+}
+void fail(const Foo , Callback Reply) {
+  Reply(error("x={0}", Params.x));
+}
+void notify(const Foo ) { lastNotify = Params.x; }
+int lastNotify = -1;
+  };
+
+  Handler H;
+  Binder.method("plusOne", , ::plusOne);
+  Binder.method("fail", , ::fail);
+  Binder.notification("notify", , ::notify);
+  Binder.command("cmdPlusOne", , ::plusOne);
+  ASSERT_THAT(RawHandlers.MethodHandlers.keys(),
+  UnorderedElementsAre("plusOne", "fail"));
+  ASSERT_THAT(RawHandlers.NotificationHandlers.keys(),
+  UnorderedElementsAre("notify"));
+  ASSERT_THAT(RawHandlers.CommandHandlers.keys(),
+  UnorderedElementsAre("cmdPlusOne"));
+  llvm::Optional> Reply;
+
+  auto  = RawHandlers.MethodHandlers["plusOne"];
+  RawPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+  RawPlusOne("foo", capture(Reply));
+  EXPECT_THAT_EXPECTED(
+  Reply.getValue(),
+  llvm::FailedWithMessage(
+  HasSubstr("failed to decode plusOne request: expected integer")));
+
+  auto  = RawHandlers.MethodHandlers["fail"];
+  RawFail(2, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::FailedWithMessage("x=2"));
+
+  auto  = RawHandlers.NotificationHandlers["notify"];
+  RawNotify(42);
+  EXPECT_EQ(H.lastNotify, 42);
+  RawNotify("hi"); // invalid, will be logged
+  EXPECT_EQ(H.lastNotify, 42);
+
+  auto  = RawHandlers.CommandHandlers["cmdPlusOne"];
+  RawCmdPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -70,6 +70,7 @@
   IndexTests.cpp
   JSONTransportTests.cpp
   LoggerTests.cpp
+  LSPBinderTests.cpp
   LSPClient.cpp
   ModulesTests.cpp
   ParsedASTTests.cpp
Index: clang-tools-extra/clangd/LSPBinder.h
===
--- /dev/null
+++ clang-tools-extra/clangd/LSPBinder.h
@@ -0,0 +1,138 @@
+//===--- LSPBinder.h - Tables of LSP handlers *- C++-*-===//
+//
+// 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_LSPBINDER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_LSPBINDER_H
+
+#include "Protocol.h"
+#include "support/Function.h"
+#include "support/Logger.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/JSON.h"
+
+namespace clang {
+namespace clangd {
+
+/// LSPBinder collects a table of functions that handle LSP 

[PATCH] D96544: [clangd] Extract binding of typed->untyped LSP handlers to LSPBinder. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

whoops, meant to create a new review for that followup change, not update this 
one...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96544/new/

https://reviews.llvm.org/D96544

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96544: [clangd] Extract binding of typed->untyped LSP handlers to LSPBinder. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 323436.
sammccall added a comment.

[clangd] Allow modules to bind LSP methods/notifications/commands.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96544/new/

https://reviews.llvm.org/D96544

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/LSPBinder.h
  clang-tools-extra/clangd/Module.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/LSPBinderTests.cpp

Index: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
@@ -0,0 +1,92 @@
+//===-- LSPBinderTests.cpp ===//
+//
+// 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 "LSPBinder.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::_;
+using testing::HasSubstr;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// JSON-serializable type for testing.
+struct Foo {
+  int x;
+};
+bool fromJSON(const llvm::json::Value , Foo , llvm::json::Path P) {
+  return fromJSON(V, F.x, P);
+}
+llvm::json::Value toJSON(const Foo ) { return F.x; }
+
+// Creates a Callback that writes its received value into an Optional.
+template 
+llvm::unique_function)>
+capture(llvm::Optional> ) {
+  Out.reset();
+  return [](llvm::Expected V) { Out.emplace(std::move(V)); };
+}
+
+TEST(LSPBinderTest, IncomingCalls) {
+  LSPBinder::RawHandlers RawHandlers;
+  LSPBinder Binder{RawHandlers};
+  struct Handler {
+void plusOne(const Foo , Callback Reply) {
+  Reply(Foo{Params.x + 1});
+}
+void fail(const Foo , Callback Reply) {
+  Reply(error("x={0}", Params.x));
+}
+void notify(const Foo ) { lastNotify = Params.x; }
+int lastNotify = -1;
+  };
+
+  Handler H;
+  Binder.method("plusOne", , ::plusOne);
+  Binder.method("fail", , ::fail);
+  Binder.notification("notify", , ::notify);
+  Binder.command("cmdPlusOne", , ::plusOne);
+  ASSERT_THAT(RawHandlers.MethodHandlers.keys(),
+  UnorderedElementsAre("plusOne", "fail"));
+  ASSERT_THAT(RawHandlers.NotificationHandlers.keys(),
+  UnorderedElementsAre("notify"));
+  ASSERT_THAT(RawHandlers.CommandHandlers.keys(),
+  UnorderedElementsAre("cmdPlusOne"));
+  llvm::Optional> Reply;
+
+  auto  = RawHandlers.MethodHandlers["plusOne"];
+  RawPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+  RawPlusOne("foo", capture(Reply));
+  EXPECT_THAT_EXPECTED(
+  Reply.getValue(),
+  llvm::FailedWithMessage(
+  HasSubstr("failed to decode plusOne request: expected integer")));
+
+  auto  = RawHandlers.MethodHandlers["fail"];
+  RawFail(2, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::FailedWithMessage("x=2"));
+
+  auto  = RawHandlers.NotificationHandlers["notify"];
+  RawNotify(42);
+  EXPECT_EQ(H.lastNotify, 42);
+  RawNotify("hi"); // invalid, will be logged
+  EXPECT_EQ(H.lastNotify, 42);
+
+  auto  = RawHandlers.CommandHandlers["cmdPlusOne"];
+  RawCmdPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -221,6 +221,29 @@
   DiagMessage("Use of undeclared identifier 'BAR'";
 }
 
+TEST_F(LSPTest, ModulesTest) {
+  class MathModule : public Module {
+void initializeLSP(LSPBinder , const ClientCapabilities ,
+   llvm::json::Object ) override {
+  Bind.notification("add", this, ::add);
+  Bind.method("get", this, ::get);
+}
+
+void add(const int ) { Value += X; }
+void get(const std::nullptr_t &, Callback Reply) { Reply(Value); }
+int Value = 0;
+  };
+  std::vector> Mods;
+  Mods.push_back(std::make_unique());
+  ModuleSet ModSet(std::move(Mods));
+  Opts.Modules = 
+
+  auto  = start();
+  Client.notify("add", 2);
+  Client.notify("add", 8);
+  EXPECT_EQ(10, Client.call("get", nullptr).takeValue());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt

[PATCH] D96544: [clangd] Extract binding of typed->untyped LSP handlers to LSPBinder. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 323321.
sammccall added a comment.

Rebase and include commands after D96507 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96544/new/

https://reviews.llvm.org/D96544

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/LSPBinder.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/LSPBinderTests.cpp

Index: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
@@ -0,0 +1,92 @@
+//===-- LSPBinderTests.cpp ===//
+//
+// 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 "LSPBinder.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::_;
+using testing::HasSubstr;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// JSON-serializable type for testing.
+struct Foo {
+  int x;
+};
+bool fromJSON(const llvm::json::Value , Foo , llvm::json::Path P) {
+  return fromJSON(V, F.x, P);
+}
+llvm::json::Value toJSON(const Foo ) { return F.x; }
+
+// Creates a Callback that writes its received value into an Optional.
+template 
+llvm::unique_function)>
+capture(llvm::Optional> ) {
+  Out.reset();
+  return [](llvm::Expected V) { Out.emplace(std::move(V)); };
+}
+
+TEST(LSPBinderTest, IncomingCalls) {
+  LSPBinder::RawHandlers RawHandlers;
+  LSPBinder Binder{RawHandlers};
+  struct Handler {
+void plusOne(const Foo , Callback Reply) {
+  Reply(Foo{Params.x + 1});
+}
+void fail(const Foo , Callback Reply) {
+  Reply(error("x={0}", Params.x));
+}
+void notify(const Foo ) { lastNotify = Params.x; }
+int lastNotify = -1;
+  };
+
+  Handler H;
+  Binder.method("plusOne", , ::plusOne);
+  Binder.method("fail", , ::fail);
+  Binder.notification("notify", , ::notify);
+  Binder.command("cmdPlusOne", , ::plusOne);
+  ASSERT_THAT(RawHandlers.MethodHandlers.keys(),
+  UnorderedElementsAre("plusOne", "fail"));
+  ASSERT_THAT(RawHandlers.NotificationHandlers.keys(),
+  UnorderedElementsAre("notify"));
+  ASSERT_THAT(RawHandlers.CommandHandlers.keys(),
+  UnorderedElementsAre("cmdPlusOne"));
+  llvm::Optional> Reply;
+
+  auto  = RawHandlers.MethodHandlers["plusOne"];
+  RawPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+  RawPlusOne("foo", capture(Reply));
+  EXPECT_THAT_EXPECTED(
+  Reply.getValue(),
+  llvm::FailedWithMessage(
+  HasSubstr("failed to decode plusOne request: expected integer")));
+
+  auto  = RawHandlers.MethodHandlers["fail"];
+  RawFail(2, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::FailedWithMessage("x=2"));
+
+  auto  = RawHandlers.NotificationHandlers["notify"];
+  RawNotify(42);
+  EXPECT_EQ(H.lastNotify, 42);
+  RawNotify("hi"); // invalid, will be logged
+  EXPECT_EQ(H.lastNotify, 42);
+
+  auto  = RawHandlers.CommandHandlers["cmdPlusOne"];
+  RawCmdPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -70,6 +70,7 @@
   IndexTests.cpp
   JSONTransportTests.cpp
   LoggerTests.cpp
+  LSPBinderTests.cpp
   LSPClient.cpp
   ModulesTests.cpp
   ParsedASTTests.cpp
Index: clang-tools-extra/clangd/LSPBinder.h
===
--- /dev/null
+++ clang-tools-extra/clangd/LSPBinder.h
@@ -0,0 +1,138 @@
+//===--- LSPBinder.h - Tables of LSP handlers *- C++-*-===//
+//
+// 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_LSPBINDER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_LSPBINDER_H
+
+#include "Protocol.h"
+#include "support/Function.h"
+#include "support/Logger.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/JSON.h"
+
+namespace clang {
+namespace clangd {
+
+/// 

[PATCH] D96544: [clangd] Extract binding of typed->untyped LSP handlers to LSPBinder. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman, mgorny.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.

The goal is to allow the LSP bindings of features to be defined outside
the ClangdLSPServer class, turning it into less of a monolith.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96544

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/LSPBinder.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/LSPBinderTests.cpp

Index: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/LSPBinderTests.cpp
@@ -0,0 +1,85 @@
+//===-- LSPBinderTests.cpp ===//
+//
+// 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 "LSPBinder.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::_;
+using testing::HasSubstr;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// JSON-serializable type for testing.
+struct Foo {
+  int x;
+};
+bool fromJSON(const llvm::json::Value , Foo , llvm::json::Path P) {
+  return fromJSON(V, F.x, P);
+}
+llvm::json::Value toJSON(const Foo ) { return F.x; }
+
+// Creates a Callback that writes its received value into an Optional.
+template 
+llvm::unique_function)>
+capture(llvm::Optional> ) {
+  Out.reset();
+  return [](llvm::Expected V) { Out.emplace(std::move(V)); };
+}
+
+TEST(LSPBinderTest, IncomingCalls) {
+  LSPBinder::RawHandlers RawHandlers;
+  LSPBinder Binder{RawHandlers};
+  struct Handler {
+void plusOne(const Foo , Callback Reply) {
+  Reply(Foo{Params.x + 1});
+}
+void fail(const Foo , Callback Reply) {
+  Reply(error("x={0}", Params.x));
+}
+void notify(const Foo ) { lastNotify = Params.x; }
+int lastNotify = -1;
+  };
+
+  Handler H;
+  Binder.method("plusOne", , ::plusOne);
+  Binder.method("fail", , ::fail);
+  Binder.notification("notify", , ::notify);
+  ASSERT_THAT(RawHandlers.MethodHandlers.keys(),
+  UnorderedElementsAre("plusOne", "fail"));
+  ASSERT_THAT(RawHandlers.NotificationHandlers.keys(),
+  UnorderedElementsAre("notify"));
+  llvm::Optional> Reply;
+
+  auto  = RawHandlers.MethodHandlers["plusOne"];
+  RawPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+  RawPlusOne("foo", capture(Reply));
+  EXPECT_THAT_EXPECTED(
+  Reply.getValue(),
+  llvm::FailedWithMessage(
+  HasSubstr("failed to decode plusOne request: expected integer")));
+
+  auto  = RawHandlers.MethodHandlers["fail"];
+  RawFail(2, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::FailedWithMessage("x=2"));
+
+  auto  = RawHandlers.NotificationHandlers["notify"];
+  RawNotify(42);
+  EXPECT_EQ(H.lastNotify, 42);
+  RawNotify("hi"); // invalid, will be logged
+  EXPECT_EQ(H.lastNotify, 42);
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -70,6 +70,7 @@
   IndexTests.cpp
   JSONTransportTests.cpp
   LoggerTests.cpp
+  LSPBinderTests.cpp
   LSPClient.cpp
   ModulesTests.cpp
   ParsedASTTests.cpp
Index: clang-tools-extra/clangd/LSPBinder.h
===
--- /dev/null
+++ clang-tools-extra/clangd/LSPBinder.h
@@ -0,0 +1,119 @@
+//===--- LSPBinder.h - Tables of LSP handlers *- C++-*-===//
+//
+// 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_LSPBINDER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_LSPBINDER_H
+
+#include "Protocol.h"
+#include "support/Function.h"
+#include "support/Logger.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/JSON.h"
+
+namespace clang {
+namespace clangd {
+
+/// LSPBinder collects a table of functions that handle LSP calls.
+///
+/// It translates a handler method's signature, e.g.
+///