Better error reporting in ResponseMessage. Fix #317

src/command_line.cc

@@ -238,33 +238,30 @@ void LaunchStdinLoop(Config* config,
   WorkThread::StartThread("stdin", [request_times]() {
     auto* queue = QueueManager::instance();
     while (true) {
-      std::variant<std::string, std::unique_ptr<BaseIpcMessage>> err_or_message =
+      std::unique_ptr<BaseIpcMessage> message;
+      optional<std::string> err =
           MessageRegistry::instance()->ReadMessageFromStdin(
-              g_log_stdin_stdout_to_stderr);
+              g_log_stdin_stdout_to_stderr, &message);
 
       // Message parsing can fail if we don't recognize the method.
-      auto* err = std::get_if<std::string>(&err_or_message);
       if (err) {
-        // FIXME LanguageClient-neovim will error without the check, probably
-        // because we do not support didChangeConfiguration and do not fill in
-        // the |id| field.
-        if (err->find("workspace/didChangeConfiguration") ==
-            std::string::npos) {
+        // The message may be partially deserialized.
+        // Emit an error ResponseMessage if |id| is available.
+        if (message) {
           Out_Error out;
-          // TODO We cannot fill in out.id because RequestMessage.id is not a base
-          // field.
-          out.error.code = lsErrorCodes::InvalidParams;
-          out.error.message = std::move(*err);
-          queue->WriteStdout(IpcId::Unknown, out);
+          out.id = message->GetRequestId();
+          if (!std::holds_alternative<std::monostate>(out.id)) {
+            out.error.code = lsErrorCodes::InvalidParams;
+            out.error.message = std::move(*err);
+            queue->WriteStdout(IpcId::Unknown, out);
+          }
         }
         continue;
       }
 
       // Cache |method_id| so we can access it after moving |message|.
-      auto& message = std::get<std::unique_ptr<BaseIpcMessage>>(err_or_message);
       IpcId method_id = message->method_id;
-
-      (*request_times)[message->method_id] = Timer();
+      (*request_times)[method_id] = Timer();
 
       switch (method_id) {
         case IpcId::Initialized: {

src/language_server_api.cc

@@ -113,8 +113,9 @@ optional<char> ReadCharFromStdinBlocking() {
   return nullopt;
 }
 
-std::variant<std::string, std::unique_ptr<BaseIpcMessage>>
-MessageRegistry::ReadMessageFromStdin(bool log_stdin_to_stderr) {
+optional<std::string> MessageRegistry::ReadMessageFromStdin(
+    bool log_stdin_to_stderr,
+    std::unique_ptr<BaseIpcMessage>* message) {
   optional<std::string> content =
       ReadJsonRpcContentFrom(&ReadCharFromStdinBlocking);
   if (!content) {
@@ -135,11 +136,12 @@ MessageRegistry::ReadMessageFromStdin(bool log_stdin_to_stderr) {
   assert(!document.HasParseError());
 
   JsonReader json_reader{&document};
-  return Parse(json_reader);
+  return Parse(json_reader, message);
 }
 
-std::variant<std::string, std::unique_ptr<BaseIpcMessage>>
-    MessageRegistry::Parse(Reader& visitor) {
+optional<std::string> MessageRegistry::Parse(
+    Reader& visitor,
+    std::unique_ptr<BaseIpcMessage>* message) {
   if (!visitor.HasMember("jsonrpc") ||
       std::string(visitor["jsonrpc"]->GetString()) != "2.0") {
     LOG_S(FATAL) << "Bad or missing jsonrpc version";
@@ -155,8 +157,11 @@ std::variant<std::string, std::unique_ptr<BaseIpcMessage>>
 
   Allocator& allocator = allocators[method];
   try {
-    return allocator(visitor);
+    allocator(visitor, message);
+    return nullopt;
   } catch (std::invalid_argument& e) {
+    // *message is partially deserialized but some field (e.g. |id|) are likely
+    // available.
     return std::string("Unable to deserialize request '") + method + "' " +
            static_cast<JsonReader&>(visitor).GetPath() + " " + e.what();
   }

src/language_server_api.h

@@ -40,24 +40,26 @@ struct MessageRegistry {
   static MessageRegistry* instance();
 
   using Allocator =
-      std::function<std::unique_ptr<BaseIpcMessage>(Reader& visitor)>;
+      std::function<void(Reader& visitor, std::unique_ptr<BaseIpcMessage>*)>;
   std::unordered_map<std::string, Allocator> allocators;
 
-  std::variant<std::string, std::unique_ptr<BaseIpcMessage>>
-      ReadMessageFromStdin(bool log_stdin_to_stderr);
-  std::variant<std::string, std::unique_ptr<BaseIpcMessage>> Parse(
-      Reader& visitor);
+  optional<std::string> ReadMessageFromStdin(
+      bool log_stdin_to_stderr,
+      std::unique_ptr<BaseIpcMessage>* message);
+  optional<std::string> Parse(Reader& visitor,
+                              std::unique_ptr<BaseIpcMessage>* message);
 };
 
 template <typename T>
 struct MessageRegistryRegister {
   MessageRegistryRegister() {
     std::string method_name = IpcIdToString(T::kIpcId);
-    MessageRegistry::instance()->allocators[method_name] = [](Reader& visitor) {
-      auto result = MakeUnique<T>();
-      Reflect(visitor, *result);
-      return result;
-    };
+    MessageRegistry::instance()->allocators[method_name] =
+        [](Reader& visitor, std::unique_ptr<BaseIpcMessage>* message) {
+          *message = MakeUnique<T>();
+          // Reflect may throw and *message will be partially deserialized.
+          Reflect(visitor, static_cast<T&>(**message));
+        };
   }
 };