eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
613 stars 145 forks source link

jsonrpc should de-structure byPosition array parameter of requests and notifications #553

Closed testforstephen closed 1 year ago

testforstephen commented 3 years ago

When i installed vscode-languageclient 7.0.0 to adopt LSP 3.16, i found there was a breaking change in languageclient@7.0.0 about adopting Parameter Structures concept of jsonrpc@2.0, see language client 7.0.0 changelog.

  • added support to control the parameter structure when sending requests and notifications in vscode-jsonrpc. The parameter structure can be controlled using the additional parameterStructures argument when creating a request or notification type or when sending an untyped request or notification using the sendRequest or sendNotification function. The default is ParameterStructures.auto which does the following:
    • use byPosition for messages with zero or greater than one parameter
    • for one parameter it used byName for parameters which are object literals. Uses byPosition for all other parameters.

In summary, if the request/notification parameter is not an object parameter, jsonrpc will wrap it into an array. See JSON_RPC implementation of language client.

Here are some samples of how to handle parameters in a language client jsonrpc.

To avoid breaking in language server, jsonrpc in lsp4j should de-structure the outermost array wrapper if it‘s a single array parameter.

The code change is parseParams of MessageTypeAdapter.java. If the message parameter is an array, then flatten the array first and map each child item to the parameter type.

https://github.com/eclipse/lsp4j/blob/00447d0a44d75b03b6cebf3d2720a311411efdca/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/adapters/MessageTypeAdapter.java#L241-L251

pisv commented 3 years ago

For the record, the change in vscode-languageserver-node was introduced via the following commits: https://github.com/microsoft/vscode-languageserver-node/commit/182dfd85183ad42d626422fdb7907dca7fa8c2b0 and https://github.com/microsoft/vscode-languageserver-node/commit/a93e8b5ee26b7fb28245797adbdb0da585c0fb56.

jonahgraham commented 3 years ago

@testforstephen Is this an issue you are going to be able to provide the fix?

testforstephen commented 3 years ago

@jonahgraham sure, i can take a look.

apupier commented 3 years ago

@testforstephen is it still in your plan to provide a fix?

dhuebner commented 1 year ago

@jonahgraham I've created a PR #714 to address this issue.

jonahgraham commented 1 year ago

Thanks @dhuebner - I have added it to my review queue and will get to it soon-ish. BTW there are a few typos of JsonRPC throughout the PR, if that is the only issues I see I can correct on the way to a merge, but if you got to them first I would be grateful.

dhuebner commented 1 year ago

@jonahgraham Thanks for letting me know! I fixed JsonRPC typos

jonahgraham commented 1 year ago

@dhuebner thanks for resolving the unwrap side. IIUC there is a wrap side to do? I see you have some WIP in https://github.com/eclipse-lsp4j/lsp4j/commit/21cb96a1f488ffe86252858912abea03c579f1fc?

dhuebner commented 1 year ago

@jonahgraham Yes, I started working on this one too. I'm not quite sure in some cases and need a bit more time to finish it. Especially not clear to me is how to identify that the argument being passed is a JS primitive or an array. Any comments or re-view upfront in this branch are very welcome. See also here

dhuebner commented 1 year ago

@jonahgraham I've opened a PR #718. I think I have covered all the cases now. One note, the Message#toString() will give a different result in some cases as MessageJsonHandler#serialize(msg) does. The reason is that it is impossible to distinguish between an array parameter and multiple parameters without having the Method description which is only available in MessageJsonHandler context.

jonahgraham commented 1 year ago

Thank @dhuebner - I will have a look at it next week.

jonahgraham commented 1 year ago

I think this is done - but I think needs an entry in the Changelog as this behaviour change could be unexpected.

jonahgraham commented 1 year ago

I think this is done - but I think needs an entry in the Changelog as this behaviour change could be unexpected.

Done in #731