Tighten parse_message error handling logic

src/cmd.rs

@@ -217,7 +217,7 @@ impl ChannelMsgReader {
 }
 
 impl server::MessageReader for ChannelMsgReader {
-    fn parsed_message(&self) -> Option<Result<ServerMessage, jsonrpc_core::Failure>> {
+    fn parsed_message(&self) -> Option<Result<ServerMessage, Option<jsonrpc_core::Failure>>> {
         let channel = self.channel.lock().unwrap();
         let msg = channel.recv().expect("Error reading from channel");
         Some(Ok(msg))

src/server.rs

@@ -117,24 +117,36 @@ macro_rules! messages {
             $($method_name$(($method_arg))*,)*
         }
 
-        // TODO: Change return type to Result<ServerMessage, Option<jsonrpc::Failure>>,
-        // since Notifications don't have id and only Requests can return a response
-        fn parse_message(input: &str) -> Result<ServerMessage, jsonrpc::Failure>  {
+        // Notifications can't return a response, hence why Err is an Option
+        fn parse_message(input: &str) -> Result<ServerMessage, Option<jsonrpc::Failure>>  {
             trace!("parse_message `{}`", input);
-            let ls_command: serde_json::Value = serde_json::from_str(input).expect("Can't make value");
+            let ls_command: serde_json::Value = match serde_json::from_str(input) {
+                Ok(value) => value,
+                Err(_) => return Err(Some(server_failure(Id::Null, jsonrpc::Error::parse_error()))),
+            };
 
             let params = ls_command.get("params");
 
             macro_rules! params_as {
                 ($ty: ty) => ({
-                    let method: $ty =
-                        serde_json::from_value(params.expect("no params").to_owned()).expect("Can't extract params");
+                    let params = match params {
+                        Some(value) => value,
+                        None => return Err(Some(server_failure(Id::Null, jsonrpc::Error::invalid_request()))),
+                    };
+
+                    let method: $ty = match serde_json::from_value(params.to_owned()) {
+                        Ok(value) => value,
+                        Err(_) => return Err(Some(server_failure(Id::Null,
+                            jsonrpc::Error::invalid_params(format!("Expected {}", stringify!($ty)))))),
+                    };
                     method
                 });
             }
 
             // Per JSON-RPC/LSP spec, Requests must have id, whereas Notifications can't
             let id = ls_command.get("id").map(|id| serde_json::from_value(id.to_owned()).unwrap());
+            // TODO: We only support numeric responses, ideally we should switch from using parsed usize
+            // to using jsonrpc_core::Id
             let parsed_numeric_id = match &id {
                 &Some(Id::Num(n)) => Some(n as usize),
                 &Some(Id::Str(ref s)) => usize::from_str_radix(s, 10).ok(),
@@ -148,7 +160,12 @@ macro_rules! messages {
                             $method_str => {
                                 match parsed_numeric_id {
                                     Some(id) => Ok(ServerMessage::Request(Request{id, method: Method::$method_name$((params_as!($method_arg)))* })),
-                                    None => Err(server_failure(id.unwrap_or(Id::Null), jsonrpc::Error::invalid_params("Id is not a number or numeric string"))),
+                                    None => match id {
+                                        None => Err(Some(server_failure(Id::Null, jsonrpc::Error::invalid_request()))),
+                                        // FIXME: This behaviour is custom and non conformant to the protocol
+                                        Some(id) => Err(Some(server_failure(id,
+                                            jsonrpc::Error::invalid_params("Id is not a number or numeric string")))),
+                                    }
                                 }
                             }
                         )*
@@ -159,14 +176,17 @@ macro_rules! messages {
                         )*
                         $(
                             // If $other_expr is Err, then we need to pass id of actual message we're handling
-                            $other_str => { ($other_expr).map_err(|err| server_failure(id.unwrap_or(Id::Null), err)) }
+                            $other_str => { ($other_expr).map_err(|err| id.map(|id| server_failure(id, err))) }
                         )*
                     }
                 } else {
-                    Err(server_failure(id.unwrap_or(Id::Null), jsonrpc::Error::invalid_params("Method is not a string")))
+                    // Message has a "method" field, so it can be a Notification/Request - if it doesn't have id then we 
+                    // assume it's a Notification for which we can't return a response, so return Err(None)
+                    Err(id.map(|id| server_failure(id, jsonrpc::Error::invalid_request())))
                 }
             } else {
-                Err(server_failure(id.unwrap_or(Id::Null), jsonrpc::Error::method_not_found()))
+                // FIXME: Handle possible client responses to server->client requests (which don't have "method" field)
+                Err(Some(server_failure(id.unwrap_or(Id::Null), jsonrpc::Error::invalid_request())))
             }
         }
 
@@ -277,13 +297,7 @@ messages! {
         "$/cancelRequest" => Cancel(CancelParams);
         "workspace/didChangeConfiguration" => WorkspaceChangeConfiguration(DidChangeConfigurationParams);
     }
-    // TODO handle me
-    "$/setTraceNotification" => Err(jsonrpc::Error {
-                                       code: jsonrpc::ErrorCode::MethodNotFound,
-                                       message: String::from("setTraceNotification"),
-                                       data: None
-                                    });
-    _ => Err(jsonrpc::Error::method_not_found());
+    _ => Err(jsonrpc::Error::method_not_found()); // TODO: Handle more possible messages
 }
 
 pub struct LsService<O: Output> {
@@ -414,8 +428,8 @@ impl<O: Output> LsService<O> {
                     },
                     Err(e) => {
                         trace!("parsing invalid message: {:?}", e);
-                        if e.id != Id::Null {
-                            this.output.failure(e.id, e.error);
+                        if let Some(failure) = e {
+                            this.output.failure(failure.id, failure.error);
                         }
                     },
                 }
@@ -492,7 +506,7 @@ pub trait MessageReader {
         None
     }
 
-    fn parsed_message(&self) -> Option<Result<ServerMessage, jsonrpc::Failure>> {
+    fn parsed_message(&self) -> Option<Result<ServerMessage, Option<jsonrpc::Failure>>> {
         self.read_message().map(|m| parse_message(&m))
     }
 }