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

JSON-RPC request without params violates specification with null #655

Open sim642 opened 2 years ago

sim642 commented 2 years ago

When a JSON-RPC request without params (i.e. them being null in Java) is emitted, that gets written as "params": null instead of just omitting the params field from the output. The latter should be done instead.

Specifications

JSON-RPC specification says:

params A Structured value that holds the parameter values to be used during the invocation of the method. This member MAY be omitted.

where "two structured types (Objects and Arrays)".

LSP specification says (in TypeScript syntax):

/**
 * The method's params.
 */
params?: array | object;

which exactly matches the JSON-RPC specification with the ? meaning omission, but not null (which would have to be explicitly given as a | null variant).

Implementation

The writing of params here: https://github.com/eclipse/lsp4j/blob/22f8edce55379bd9d4ae1b96bc614e74549bc0f9/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/adapters/MessageTypeAdapter.java#L404-L409 uses the writeNullValue function here: https://github.com/eclipse/lsp4j/blob/22f8edce55379bd9d4ae1b96bc614e74549bc0f9/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/adapters/MessageTypeAdapter.java#L449-L457 which goes out of its way to output a null instead of omitting missing params.

This clearly violates the specifications and trips up other JSON-RPC/LSP implementations that require incoming requests to follow the specification to the letter.

Background

According to git blame, this behavior was introduced in #137, the goal of which was to fix the behavior for response result that indeed may not be omitted but may be null. Looks like the same logic was erroneously applied to request/notification params as well.

jonahgraham commented 2 years ago

I suspect to change this back (i.e. revert #137) we need to get xtext involved so to not regress all the xtext generated language servers. See https://github.com/eclipse/xtext-core/issues/558 for the discussion that led to #137's behaviour.

cdietrich commented 2 years ago

Is

Received message which is neither a response nor a notification message

fixed in vscode meanwhile?

jonahgraham commented 8 months ago

If this is still relevant a failing unit test or similar would be helpful so we can decide how to proceed