Gleam LSP fails to format because the end range is out of bounds

[the-mikedavis]

Summary

The textDocument/formatting response from the gleam LSP sends a line number that is way past the end of the document:

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": [
    {
      "newText": "fn main() {\n  let x = 5\n}\n",
      "range": {
        "end": {
          "character": 0,
          "line": 4294967295
        },
        "start": {
          "character": 0,
          "line": 0
        }
      }
    }
  ]
}

That causes lsp_pos_to_pos to return None which causes an early return here:

helix/helix-lsp/src/lib.rs

Lines 323 to 327 in ce0837d

	let end = if let Some(end) = lsp_pos_to_pos(doc, edit.range.end, offset_encoding) {
	end
	} else {
	return (0, 0, None);
	};

making the Transaction a no-op.

Reproduction Steps

I tried this:

1. hx file.gleam
2. add some contents like

   fn main() {
       let x = 5
   }

3. :format

I expected this to happen: the formatter should turn the 4 column indent into a 2 column indent.

Instead, this happened: no change to the document.

Helix log

~/.cache/helix/helix.log

2023-02-16T13:51:01.704 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"initialize","params":{"capabilities":{"general":{"positionEncodings":["utf-32","utf-8","utf-16"]},"textDocument":{"codeAction":{"codeActionLiteralSupport":{"codeActionKind":{"valueSet":["","quickfix","refactor","refactor.extract","refactor.inline","refactor.rewrite","source","source.organizeImports"]}}},"completion":{"completionItem":{"deprecatedSupport":true,"insertReplaceSupport":true,"resolveSupport":{"properties":["documentation","detail","additionalTextEdits"]},"snippetSupport":false,"tagSupport":{"valueSet":[1]}},"completionItemKind":{}},"hover":{"contentFormat":["markdown"]},"publishDiagnostics":{},"rename":{"dynamicRegistration":false,"honorsChangeAnnotations":false,"prepareSupport":false},"signatureHelp":{"signatureInformation":{"activeParameterSupport":true,"documentationFormat":["markdown"],"parameterInformation":{"labelOffsetSupport":true}}}},"window":{"workDoneProgress":true},"workspace":{"applyEdit":true,"configuration":true,"didChangeConfiguration":{"dynamicRegistration":false},"executeCommand":{"dynamicRegistration":false},"symbol":{"dynamicRegistration":false},"workspaceFolders":true}},"clientInfo":{"name":"helix","version":"22.12 (78a1e2db)"},"processId":192552,"rootPath":"/home/michael/tmp","rootUri":"file:///home/michael/tmp","workspaceFolders":[{"name":"tmp","uri":"file:///home/michael/tmp"}]},"id":0}
2023-02-16T13:51:01.705 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","id":0,"result":{"capabilities":{"completionProvider":{"triggerCharacters":["."," "]},"definitionProvider":true,"documentFormattingProvider":true,"hoverProvider":true,"textDocumentSync":{"change":1,"save":{"includeText":false}}}}}
2023-02-16T13:51:01.706 helix_lsp::transport [INFO] <- {"capabilities":{"completionProvider":{"triggerCharacters":["."," "]},"definitionProvider":true,"documentFormattingProvider":true,"hoverProvider":true,"textDocumentSync":{"change":1,"save":{"includeText":false}}}}
2023-02-16T13:51:01.706 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"initialized","params":{}}
2023-02-16T13:51:01.706 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","id":"create-compiling-progress-token","method":"window/workDoneProgress/create","params":{"token":"compiling-gleam"}}
2023-02-16T13:51:01.706 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"$/progress","params":{"token":"compiling-gleam","value":{"cancellable":false,"kind":"begin","title":"Compiling Gleam"}}}
2023-02-16T13:51:01.706 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","method":"$/progress","params":{"token":"compiling-gleam","value":{"kind":"end"}}}
2023-02-16T13:51:01.712 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"languageId":"gleam","text":"fn main() {\n    let x = 5\n}\n","uri":"file:///home/michael/tmp/file.gleam","version":0}}}
2023-02-16T13:51:01.712 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","result":null,"id":"create-compiling-progress-token"}
2023-02-16T13:51:02.665 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"textDocument/formatting","params":{"options":{"insertSpaces":true,"tabSize":2},"textDocument":{"uri":"file:///home/michael/tmp/file.gleam"}},"id":1}
2023-02-16T13:51:02.665 helix_lsp::transport [INFO] <- {"jsonrpc":"2.0","id":1,"result":[{"newText":"fn main() {\n  let x = 5\n}\n","range":{"end":{"character":0,"line":4294967295},"start":{"character":0,"line":0}}}]}
2023-02-16T13:51:02.666 helix_lsp::transport [INFO] <- [{"newText":"fn main() {\n  let x = 5\n}\n","range":{"end":{"character":0,"line":4294967295},"start":{"character":0,"line":0}}}]

Platform

Linux

Terminal Emulator

Kitty 0.26.2

Helix Version

78a1e2d

[the-mikedavis]

We discussed this on the Matrix chat a little: it's unclear in the LSP spec whether ranges that point outside of the document are allowed, so I'm not sure whether this should be fixed in Helix or in Gleam.

To fix it in Helix, lsp_pos_to_pos could return the document end when the given position is past the end of the document.

[pascalkuthe]

Gleam does this on purpose: https://github.com/gleam-lang/gleam/blob/94caeb35b284dff0b4b8c3b0b800e3b3021b45a4/compiler-cli/src/lsp.rs#L860

In my opinion this is not really LSP compliant but I guess it doesn't hurt to still accept such positions. It should be quite easy. We should log a warning in that case tough in-case it's a genuine bug

[the-mikedavis]

To fix it in Helix, lsp_pos_to_pos could return the document end when the given position is past the end of the document.

Actually, instead we should probably just handle the None return of lsp_pos_to_pos in this case to set end to the document end char.

[pascalkuthe]

Hmm not quite I think the

To fix it in Helix, lsp_pos_to_pos could return the document end when the given position is past the end of the document.

Actually, instead we should probably just handle the None return of lsp_pos_to_pos in this case to set end to the document end char.

Hmm not sure I think we should return Some(document.len_chars()) here

helix/helix-lsp/src/lib.rs

Line 129 in ce0837d

return None;

I guess since other LSP client's don't have problems with such positions other servers might do this for other things than formatting too.
And lsp_pos_to_pos can also return None if an invalid (between chars) utf-16 /utf-8 character offset is returned. I still we should abort in that case as we do currently

[VlkrS]

I patched my helix 22.12 to return Some(doc.len_chars()) in place of None and that seems to work nicely for the gleam lsp. Haven't noticed any fallout with other language servers so far.

[archseer]

This isn't spec compliant. Could Gleam fix this upstream instead?

[pascalkuthe]

This isn't spec compliant. Could Gleam fix this upstream instead?

The spec isn't really clear how out of bounds lines should be handled and this is not an accident by the gleam LSP. They specifically barcode a line range of 0..u32::MAX as a cheap way to format the whole file. I assume this works in vscoe and sadly when the spec is not clear servers just roll with what works in vscode.

This shouldn't affect servers that don't sent out of bounds ranges and should meet the expectation of those that do. It's also only a 1 line change so while I would usually not spend effort to work around a misbehaving LS in this case it's probably ok.

[VlkrS]

A comment in the vscode language server explicitly mentions this behavior here

[ghost]

The spec isn't really clear how out of bounds lines should be handled and this is not an accident by the gleam LSP. They specifically barcode a line range of 0..u32::MAX as a cheap way to format the whole file. I assume this works in vscoe and sadly when the spec is not clear servers just roll with what works in vscode.

Not that this would be likely to happen, but in theory this would fail on a file with u32::MAX + 1 lines. I agree with @archseer that the language server in question is relying on undefined behavior in the spec. Just because VS Code accepts it doesn't make it any more compliant.

IMO the Gleam LSP needs to only send valid ranges. 😄

[pascalkuthe]

The spec isn't really clear how out of bounds lines should be handled and this is not an accident by the gleam LSP. They specifically barcode a line range of 0..u32::MAX as a cheap way to format the whole file. I assume this works in vscoe and sadly when the spec is not clear servers just roll with what works in vscode.

Not that this would be likely to happen, but in theory this would fail on a file with u32::MAX + 1 lines. I agree with @archseer that the language server in question is relying on undefined behavior in the spec. Just because VS Code accepts it doesn't make it any more compliant.

IMO the Gleam LSP needs to only send valid ranges. smile

While in an ideal world server would only follow the standard in practice almost all servers target VSCode and therefore those parts which are underdefined are practically implementation defined by vscode. Especially as the same people that develop VSCode handle the standard that means that "undefined behaviour" like this usually just gets retrospectively definied to whatever VSCode was doing anyway to not break backwards compatability.

The point about u32::MAX + 1 is pretty moot because the lsp spec defines the line number as an uinteger and an uinteger is definied as:

/**
 * Defines an unsigned integer number in the range of 0 to 2^31 - 1.
 */
export type [uinteger](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#uinteger) = number;

(yes it says 2^31 - 1 instead of 2^32 - 1 here, that is on purpose microsoft/language-server-protocol@b795b3b#r54624532 so it does technically violate the standard but I see no reason to be pedantic about the maximum here as gleam could easilychange the maximum to 2^31-1 and be compliant)

I think the link @VlkrS provided above is a really strong indication that this is intentional (considering that vscode language server is written by the main author of the spec) and just another detail forgotten in the spec so I think it's a reasonable change

[VlkrS]

I clarified with vscode-language-server-node and this behaviour is specific to that particular implementation, however it also does not contradict the specification either.

[ghost]

If it is not specifically outlined in the spec, it is out of spec by definition. The spec makes no mention of this behavior, Gleam is out of spec here.

[lpil]

Thanks for digging into this folks! Happy to make the change in Gleam.

It would be fab if the spec could be made more clear here, my reading of it was that this was supported (I can see nothing that suggests otherwise) and VSCode's accepting of it solidified that in my mind.

[lpil]

If someone could give it a test with the nightly build tomorrow that would be fab! Thank you

[the-mikedavis]

I just tried it out building from source and it works great! Thanks @VlkrS and @lpil!

[lpil]

Thank you everyone! 💜