Do not use u64 for numbers that could be negative

[mawww]

Hello,

It seems various fields specified as "number" in the lsp spec are set as u64 in the library, for example SignatureHelp::activeParameter. While it seems to make sense, there is no guarantees language servers wont send a negative value there, and in particular eclipse.jdt.ls seems to send -1 in some cases for this, leading to deserialization failures.

I think u64 should only be used for fields that are guaranteed by the lsp standard to be always >= 0, such as positions.

[Marwes]

That seems like a mistake in this crate. Do you know of any other fields where this could be an issue? PRs appreciated!

[mawww]

This is the only field I have seen failing in my case, but it could be nice to do a general review of all the uses of unsigned types for what the spec specifies as number. I think some cases do make sense (positions in the buffer is one of them, I see no cases where that could be negative, and it make sense not to use the full range number accepts, as that would require f32...), but it should not be the default...

"Be tolerant in what you receive and conservative in what you send" as they say.

[johanneshoehn]

The typescript type number actually corresponds to f64 and not f32.

This library generally uses u64 where LSP uses number.
LSP uses the type number at the following locations:

• ❗️id in varying locations
• code of ResponseError: Is not part of this crate, already specified by json-rpc.
• line and character of Position: Here only the used u64 values make sense.
• Some enums like severity in Diagnostic
• ❗️code in Diagnostic: Here there is a discrepancy. Float error codes don't make any sense to me, but are allowed by the spec, while negative error codes even make sense to me
• ❗️version in TextDocumentItem and TextDocumentIdentifier. Nothing prevents usage of negative or floating point numbers here.
• processId in InitializeParams. I don't know any operating system which has negative or floating point process ids
• rangeLimit in foldingRange in TextDocumentClientCapabilities: Here only positive integers make sense
• rangeLength in TextDocumentContentChangeEvent: The same
• activeSignature and activeParameter of SignatureHelp: are indices, only positive integers make sense
• color is correctly represented with f64 in this library
• tabSize in FormattingOptions: here only positive integers make sense
• further properties of FormattingOptions are correctly represented as f64 in this library
• Everything in FoldingRange is zero-based index

[Marwes]

@hannni Thanks for looking into this!

id in varying locations

These refer to JSON-RPC request ids so they will always be an integer at least but it does seem like they can technically be negative. As each request should have a unique id there probably every implementation will start at 0 and increment the counter from there. Wouldn't hurt to use i64 but it is unlikely to cause issues.

code in Diagnostic: Here there is a discrepancy. Float error codes don't make any sense to me, but are allowed by the spec, while negative error codes even make sense to me

In this case, code refers to a number that can be easily searched for to find further information about the error. I don't expect anyone to use a negative number for that but technically someone could use it for something else. Wouldn't hurt to use i64 I suppose since it won't exactly limit anything.

version in TextDocumentItem and TextDocumentIdentifier. Nothing prevents usage of negative or floating point numbers here.

Same deal as with id except here it is also specified to strictly increase. An implementation would need to start on a negative version number, which (again) is really strange.

I'd be happy to accept a PR for any issues but they do seem rather implausible. activeParameter is the only one that has caused issues so we should probably fix that (though rather than sending -1 it would make more sense for eclipse to omit the field 🤷‍♂️ )

"Be tolerant in what you receive and conservative in what you send" as they say.

Well, that can also cause incompatibilities between different implementations as these ambigous cases get treated differently.

[Marwes]

I may sound a bit hesitant against changes in the above comment. That is just because the crate is meant to give users of the crate as many assurances about the values they get as possible. Expanding a field to allow both positive and negative numbers drops one such assurance so even while it can be technically correct, it may not be necessary in practice and thus be a bit of a pessimisation.

[vnagarnaik]

I ran into this issue using "kak-lsp" with an internal LSP server that uses -1 an invalid value for SignatureHelp.activeSignature.

While I appreciate that the goal of this crate is to provide type guarantees, the LSP spec documentation says this about the SignatureHelp.activeSignature and SignatureHelp.activeParameter values:

If omitted or the value lies outside range of signatures the value defaults to zero or is ignored signatures.length === 0.

(https://microsoft.github.io/language-server-protocol/specification#textDocument_signatureHelp)

That means an implementor is free to return -1. However, since lsp-types uses u64, kak-lsp panics while parsing even though the value should be parsed as valid. Which means kak-lsp cannot even access SignatureHelp.signatures array and notice that it's empty and then ignore the SignatureHelp.activeSignature value.

Adding @ul to provide a better solution for kak-lsp if I am missing something here.

[Marwes]

@vnagarnaik I'd be happy to accept a PR for changing activeParameter since that causes issues (or for any of the other instances).

[vnagarnaik]

Created pull request for updating SignatureHelp types: #100

[Vincent-P]

The plugin LSP Support for Jetbrains IDE's uses -1 as the first version for text document :(

[kjeremy]

microsoft/language-server-protocol#1037