search

MopeSWTP-SS21 / MopeSWTP

MIT License
1 stars 0 forks source link

Major bug: JSON RPC calls with a single primitive argument are broken in LSP4J #85

Open CSchoel opened 3 years ago

CSchoel commented 3 years ago

Today, I wanted to implement support for the custom JSON RPC calls understood by the Mo|E server so that you can test your implementation with the vscode client. However, regardless of how I sent the loadModel request from the vscode-plugin, the server would just not accept the request, reporting an error in the parsing stage of the JSON RPC implementation.

After a whopping 4 hours of debugging, I found out that the error can be traced to an LSP4J issue regarding requests with a single parameter. According to the JSON RPC spec, parameters MUST always be wrapped either in an array or an object. LSP4J does not respect this and instead expects that params is a raw JSON string literal. With this behavior, it is virtually impossible to generate a request with the LSP implementation in vscode that would be accepted by the LSP4J server.

Long story short, since the issue is known since May and there is no fix on the horizon, we have to completely restructure our custom JSON RPC calls. I have implemented a quick example in MopeSWTP-SS21/LSP4J-test-CS, which does work with the vscode client. I think the best way to circumvent this problem is to simply always use parameter objects, which also would make the Mo|E calls more consistent with the standard LSP calls in LSP4J.

CSchoel commented 3 years ago

One alternative that may also be worth considering is to go back to workspace/executeCommand. With the way Diagnostics are handled, it seems that we might not have to transmit that much structured result data anyway.

manuEbg commented 3 years ago

I don't really like the Idea of going back to workspace/executeCommand because I just think this wouldn't be correct:) However I am not sure what the actual purpose of workspace/executeCommand is. Maybe it is exactly to provide Commands like checkModel... I don't know:) Since @CSchoel has a little more experience in developing a LspClient i am wondering what you would consider the easiest way to implement the commands to the Client?

CSchoel commented 3 years ago

What makes you think that workspace/executeCommand would be "incorrect"? I can understand the confusion about its purpose, because the LSP spec is extremely vague on this, but I see no concrete evidence that it would be wrong to use it in our case. I would suspect that the spec is so vague precisely because this method is designed to be a one-size-fits-all solution to any kind of command that a client might want to execute on the server. The fact that it is bound to the workspace makes sense in the context that some commands might want to edit one or multiple files, but there is no requirement that they do.

To get a little closer to what the "standard" way of using workspace/executeCommand might be supposed to do, I searched through some of the larger projects implementing LSP and found command lists for metals and for clojure-lsp (again: I don't have more experience, I just read stuff until I do :nerd_face: ). This, too, seems like this is a "everything goes" kind of thing. People seem to use workspace/executeCommand for anything that is convenient to implement in that way. This would also be my main argument: It is much simpler to use existing features than to extend the LSP protocol, so why don't we go the simple route? Finding out how to send workspace/executeCommand in the vscode client took me maybe half an hour, for the custom JSON RPC call I needed four hours.

When we last discussed this, you brought up the argument that custom JSON RPC commands would allow us to better specify the return type for code completions. This was a valid argument at the time, but as it currently is, I suspect that most of our commands would just publish diagnostics and not need to return much at all.

manuEbg commented 3 years ago

I'm not thinking it is incorrect I just feel like it is not correct:D Probably because using JsonRpc seems much cleaner. However I re-implemented the ExecuteCommand which is now calling methods from our ModelicaService. (see https://github.com/MopeSWTP-SS21/MopeSWTP/commit/2890b9e2c2bfb65df528d473183a60045154dd55 currently only in feature/Capabilities) If we stick to this Way we should change the ReturnValues of ModelicaService because the CompletableFutures make things unnecessary complicated.