lsp-types
lsp-types copied to clipboard
Do not use u64 for numbers that could be negative
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.
That seems like a mistake in this crate. Do you know of any other fields where this could be an issue? PRs appreciated!
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.
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
ofResponseError
: Is not part of this crate, already specified by json-rpc. -
line
andcharacter
ofPosition
: Here only the usedu64
values make sense. - Some enums like
severity
inDiagnostic
- ❗️
code
inDiagnostic
: 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
inTextDocumentItem
andTextDocumentIdentifier
. Nothing prevents usage of negative or floating point numbers here. -
processId
inInitializeParams
. I don't know any operating system which has negative or floating point process ids -
rangeLimit
infoldingRange
inTextDocumentClientCapabilities
: Here only positive integers make sense -
rangeLength
inTextDocumentContentChangeEvent
: The same -
activeSignature
andactiveParameter
ofSignatureHelp
: are indices, only positive integers make sense -
color
is correctly represented withf64
in this library -
tabSize
inFormattingOptions
: here only positive integers make sense - further properties of
FormattingOptions
are correctly represented asf64
in this library - Everything in
FoldingRange
is zero-based index
@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.
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.
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 ignoredsignatures.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.
@vnagarnaik I'd be happy to accept a PR for changing activeParameter
since that causes issues (or for any of the other instances).
Created pull request for updating SignatureHelp types: https://github.com/gluon-lang/lsp-types/pull/100
The plugin LSP Support for Jetbrains IDE's uses -1 as the first version for text document :(
https://github.com/microsoft/language-server-protocol/issues/1037