lsp: Do not set error/result fields if they're missing. (#10610)

Piotr Osiewicz created 2 years ago

We were previously not conforming to LSP spec, as we were setting
**both** result field and error field on response fields, which could
confuse servers. Excerpt from the spec:
> * The result of a request. This member is REQUIRED on success.
> * This member MUST NOT exist if there was an error invoking the
method.

Fixes #10595

Release Notes:

- N/A

Change summary

crates/lsp/src/lsp.rs | 43 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 36 insertions(+), 7 deletions(-)

Detailed changes

crates/lsp/src/lsp.rs

@@ -138,8 +138,16 @@ struct AnyResponse<'a> {
 struct Response<T> {
     jsonrpc: &'static str,
     id: RequestId,
-    result: Option<T>,
-    error: Option<Error>,
+    #[serde(flatten)]
+    value: LspResult<T>,
+}
+
+#[derive(Serialize)]
+#[serde(rename_all = "snake_case")]
+enum LspResult<T> {
+    #[serde(rename = "result")]
+    Ok(Option<T>),
+    Error(Option<Error>),
 }
 
 /// Language server protocol RPC notification message.
@@ -867,16 +875,14 @@ impl LanguageServer {
                                             Ok(result) => Response {
                                                 jsonrpc: JSON_RPC_VERSION,
                                                 id,
-                                                result: Some(result),
-                                                error: None,
+                                                value: LspResult::Ok(Some(result)),
                                             },
                                             Err(error) => Response {
                                                 jsonrpc: JSON_RPC_VERSION,
                                                 id,
-                                                result: None,
-                                                error: Some(Error {
+                                                value: LspResult::Error(Some(Error {
                                                     message: error.to_string(),
-                                                }),
+                                                })),
                                             },
                                         };
                                         if let Some(response) =
@@ -1503,4 +1509,27 @@ mod tests {
         let expected_id = RequestId::Int(2);
         assert_eq!(notification.id, Some(expected_id));
     }
+
+    #[test]
+    fn test_serialize_has_no_nulls() {
+        // Ensure we're not setting both result and error variants. (ticket #10595)
+        let no_tag = Response::<u32> {
+            jsonrpc: "",
+            id: RequestId::Int(0),
+            value: LspResult::Ok(None),
+        };
+        assert_eq!(
+            serde_json::to_string(&no_tag).unwrap(),
+            "{\"jsonrpc\":\"\",\"id\":0,\"result\":null}"
+        );
+        let no_tag = Response::<u32> {
+            jsonrpc: "",
+            id: RequestId::Int(0),
+            value: LspResult::Error(None),
+        };
+        assert_eq!(
+            serde_json::to_string(&no_tag).unwrap(),
+            "{\"jsonrpc\":\"\",\"id\":0,\"error\":null}"
+        );
+    }
 }