Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LSP Client: id handling requires certain number serialization #12367

Closed
HolonProduction opened this issue Dec 30, 2024 · 4 comments · Fixed by #12376
Closed

LSP Client: id handling requires certain number serialization #12367

HolonProduction opened this issue Dec 30, 2024 · 4 comments · Fixed by #12376
Labels
C-bug Category: This is a bug E-good-first-issue Call for participation: Issues suitable for new contributors E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR

Comments

@HolonProduction
Copy link

Summary

Original issue at godotengine/godot#100914
tl;dr

  • Godot changed its serialization for floats to always include trailing .0
  • json numbers are stored as floats in Godot, thus they are effected by the new serialization
  • the language server thus sends messages containing ids of the form "id": 0.0
  • Helix discards those messages as invalid

Reproduction Steps

I tried this:

  1. Start Godot 4.4-dev7 and open a project
  2. Configure the Godot language server
  3. hx -v ~/godot_project/some_script.gd

I expected this to happen:

  • Helix connects to the language server

Instead, this happened:

  • Helix doesn't connect to the language server

Helix log

Log for the error:

helix_lsp::transport [INFO] godot -> {"jsonrpc":"2.0","method":"initialize","params":{"capabilities":{"general":{"positionEncodings":["utf-8","utf-32","utf-16"]},"textDocument":{"codeAction":{"codeActionLiteralSupport":{"codeActionKind":{"valueSet":["","quickfix","refactor","refactor.extract","refactor.inline","refactor.rewrite","source","source.organizeImports"]}},"dataSupport":true,"disabledSupport":true,"isPreferredSupport":true,"resolveSupport":{"properties":["edit","command"]}},"completion":{"completionItem":{"deprecatedSupport":true,"insertReplaceSupport":true,"resolveSupport":{"properties":["documentation","detail","additionalTextEdits"]},"snippetSupport":true,"tagSupport":{"valueSet":[1]}},"completionItemKind":{}},"formatting":{"dynamicRegistration":false},"hover":{"contentFormat":["markdown"]},"inlayHint":{"dynamicRegistration":false},"publishDiagnostics":{"tagSupport":{"valueSet":[1,2]},"versionSupport":true},"rename":{"dynamicRegistration":false,"honorsChangeAnnotations":false,"prepareSupport":true},"signatureHelp":{"signatureInformation":{"activeParameterSupport":true,"documentationFormat":["markdown"],"parameterInformation":{"labelOffsetSupport":true}}}},"window":{"workDoneProgress":true},"workspace":{"applyEdit":true,"configuration":true,"didChangeConfiguration":{"dynamicRegistration":false},"didChangeWatchedFiles":{"dynamicRegistration":true,"relativePatternSupport":false},"executeCommand":{"dynamicRegistration":false},"fileOperations":{"didRename":true,"willRename":true},"inlayHint":{"refreshSupport":false},"symbol":{"dynamicRegistration":false},"workspaceEdit":{"documentChanges":true,"failureHandling":"abort","normalizesLineEndings":false,"resourceOperations":["create","rename","delete"]},"workspaceFolders":true}},"clientInfo":{"name":"helix","version":"24.7 (079f5442)"},"processId":redacted,"rootPath":"redacted","rootUri":"file://redacted","workspaceFolders":[{"name":"redacted","uri":"file://redacted"}]},"id":0}
helix_lsp::transport [INFO] godot <- {"id":0.0,"jsonrpc":"2.0","result":{"capabilities":{"codeActionProvider":false,"colorProvider":false,"completionProvider":{"resolveProvider":true,"triggerCharacters":[".","$","'","\""]},"declarationProvider":true,"definitionProvider":true,"documentFormattingProvider":false,"documentHighlightProvider":false,"documentLinkProvider":{"resolveProvider":false},"documentOnTypeFormattingProvider":{"firstTriggerCharacter":"","moreTriggerCharacter":[]},"documentRangeFormattingProvider":false,"documentSymbolProvider":true,"executeCommandProvider":{"commands":[]},"foldingRangeProvider":false,"hoverProvider":true,"implementationProvider":false,"referencesProvider":true,"renameProvider":{"prepareProvider":true},"signatureHelpProvider":{"triggerCharacters":[",","("]},"textDocumentSync":{"change":1,"openClose":true,"save":{"includeText":true},"willSave":false,"willSaveWaitUntil":true},"typeDefinitionProvider":false,"workspace":{"fileOperations":{"didDelete":{"filters":[{"pattern":{"glob":"**/*.gd","matches":"file"}}]}}},"workspaceSymbolProvider":true}}}
helix_lsp::transport [ERROR] Exiting godot after unexpected error: Parse(Error("data did not match any variant of untagged enum ServerMessage", line: 0, column: 0))

The old Godot serialization behavior in comparison:

helix_lsp::transport [INFO] godot -> {"jsonrpc":"2.0","method":"initialize","params":{"capabilities":{"general":{"positionEncodings":["utf-8","utf-32","utf-16"]},"textDocument":{"codeAction":{"codeActionLiteralSupport":{"codeActionKind":{"valueSet":["","quickfix","refactor","refactor.extract","refactor.inline","refactor.rewrite","source","source.organizeImports"]}},"dataSupport":true,"disabledSupport":true,"isPreferredSupport":true,"resolveSupport":{"properties":["edit","command"]}},"completion":{"completionItem":{"deprecatedSupport":true,"insertReplaceSupport":true,"resolveSupport":{"properties":["documentation","detail","additionalTextEdits"]},"snippetSupport":true,"tagSupport":{"valueSet":[1]}},"completionItemKind":{}},"formatting":{"dynamicRegistration":false},"hover":{"contentFormat":["markdown"]},"inlayHint":{"dynamicRegistration":false},"publishDiagnostics":{"tagSupport":{"valueSet":[1,2]},"versionSupport":true},"rename":{"dynamicRegistration":false,"honorsChangeAnnotations":false,"prepareSupport":true},"signatureHelp":{"signatureInformation":{"activeParameterSupport":true,"documentationFormat":["markdown"],"parameterInformation":{"labelOffsetSupport":true}}}},"window":{"workDoneProgress":true},"workspace":{"applyEdit":true,"configuration":true,"didChangeConfiguration":{"dynamicRegistration":false},"didChangeWatchedFiles":{"dynamicRegistration":true,"relativePatternSupport":false},"executeCommand":{"dynamicRegistration":false},"fileOperations":{"didRename":true,"willRename":true},"inlayHint":{"refreshSupport":false},"symbol":{"dynamicRegistration":false},"workspaceEdit":{"documentChanges":true,"failureHandling":"abort","normalizesLineEndings":false,"resourceOperations":["create","rename","delete"]},"workspaceFolders":true}},"clientInfo":{"name":"helix","version":"24.7 (079f5442)"},"processId":redacted,"rootPath":"redacted","rootUri":"file://redacted","workspaceFolders":[{"name":"redacted","uri":"file://redacted"}]},"id":0}
helix_lsp::transport [INFO] godot <- {"id":0,"jsonrpc":"2.0","result":{"capabilities":{"codeActionProvider":false,"colorProvider":false,"completionProvider":{"resolveProvider":true,"triggerCharacters":[".","$","'","\""]},"declarationProvider":true,"definitionProvider":true,"documentFormattingProvider":false,"documentHighlightProvider":false,"documentLinkProvider":{"resolveProvider":false},"documentOnTypeFormattingProvider":{"firstTriggerCharacter":"","moreTriggerCharacter":[]},"documentRangeFormattingProvider":false,"documentSymbolProvider":true,"executeCommandProvider":{"commands":[]},"foldingRangeProvider":false,"hoverProvider":true,"implementationProvider":false,"referencesProvider":true,"renameProvider":{"prepareProvider":true},"signatureHelpProvider":{"triggerCharacters":[",","("]},"textDocumentSync":{"change":1,"openClose":true,"save":{"includeText":true},"willSave":false,"willSaveWaitUntil":true},"typeDefinitionProvider":false,"workspace":{"fileOperations":{"didDelete":{"filters":[{"pattern":{"glob":"**/*.gd","matches":"file"}}]}}},"workspaceSymbolProvider":true}}}

Platform

Fedora 41

Terminal Emulator

GNOME Terminal 😔

Installation Method

dnf

Helix Version

helix 24.7 (079f544)

@HolonProduction HolonProduction added the C-bug Category: This is a bug label Dec 30, 2024
@WolfEYc
Copy link

WolfEYc commented Dec 30, 2024

Thank you for quickly doing the more thorough debugging that I did not perform.
If "id" is the only offender, could that be made a special case?
If not, but the set of fields is fixed that used to send integers, could that set be made an exception as well?

Not super familiar with the internals of LSPs (I use them but don't work on em)
If there is an arbitrary number of fields behaving this way, if the old JSON Serializer could be used just for the LSP at least then would this also fix the issue?

PS:
Use Alacritty!

@pascalkuthe
Copy link
Member

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#requestMessage

id is only allowed to be an integer according to spec so I would argue Godot is jot pliant here. They are referencing the wrong spec. The jsonrpc spec allows any number for the client ID but the LSP spec only allows integers

@HolonProduction
Copy link
Author

The link is indeed wrong, thanks for the hint! The LSP spec links to the JSONRPC spec, I guess that confused me a bit.

Still integer in LSP is only a typedef for number (in the json/js type system) so from my understanding when serializing we should be able to serialize it with fraction as long as the fraction is zero. (Did I miss some json extensions in either LSP or JSONRPC?)

That being said, as stated on the Godot issue I understand that this might be quite hard to solve when not using js, and we should fix it on our side in case other non js implementations also do the integer differentiation on json level. Just thought I'd report it here in case there are other rogue language servers out there.

@the-mikedavis
Copy link
Member

the-mikedavis commented Dec 30, 2024

Yeah this is a bit of a gray area in the LSP spec because of the number type. The JSONRPC spec says that "Numbers SHOULD NOT contain fractional parts" (in RFC2119-speak) implying that we should try to handle numbers with fractional parts. I think it's a good middleground here to accept fractional parts as long as they are exactly .0 (by stripping a suffix of ".0" from the input). To do this we should switch the #[derive(Deserialize)] in helix-lsp/src/jsonrpc.rs to a manual implementation that tries that stripping if parsing as a u64 fails.

@the-mikedavis the-mikedavis added E-good-first-issue Call for participation: Issues suitable for new contributors E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR labels Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug E-good-first-issue Call for participation: Issues suitable for new contributors E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants