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

[testforstephen]

trafficstars

When i installed vscode-languageclient 7.0.0 to adopt LSP 3.16, i found there was a breaking change in [email protected] about adopting Parameter Structures concept of [email protected], 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.

• client.sendRequest(type, true)

  jsonrpc: { ... Params: [ true ] }

• client.sendRequest(type, [true])

  jsonrpc: { ... Params: [ [ true ] ] }

• client.sendRequest(type, { value: true })

  jsonrpc: { ... Params: { value: true } }

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]

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]

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

[testforstephen]

@jonahgraham sure, i can take a look.

[apupier]

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

[dhuebner]

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

[jonahgraham]

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]

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

[jonahgraham]

@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]

@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]

@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]

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

[jonahgraham]

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

[jonahgraham]

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

Done in #731