bash-lsp / bash-language-server

A language server for Bash
MIT License
2.08k stars 121 forks source link

Send valid document range for textDocument/format #1177

Open Waqar144 opened 1 month ago

Waqar144 commented 1 month ago

Code editor

Kate

Platform

Linux

Version

No response

What steps will reproduce the bug?

For the textDocument/format request, this server sends an invalid range as reply with the end set to MAX_VALUE instead of the actual end. This breaks some clients such as Kate.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

Valid document range

What do you see instead?

Invalid document range

Additional information

https://github.com/bash-lsp/bash-language-server/blob/23fdaa9cc136338e5073a28fdd33879b2edf2561/server/src/shfmt/index.ts#L55

IIUC, such special values are against the spec https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#position.

Position in a text document expressed as zero-based line and zero-based character offset. A position is between two characters like an ‘insert’ cursor in an editor. Special values like for example -1 to denote the end of a line are not supported.

chris-reeves commented 1 month ago

Thanks for the report. I'll aim to look into this next week.

chris-reeves commented 1 month ago

I've started to look into this. First off, I get that specifying the end of the current/original document as the end of the range (rather than just picking the largest possible number) would likely fix Kate. Before doing that, I'm keen to understand why this is breaking Kate (for my own edification, if nothing else).

Regarding the spec, I don't think Number.MAX_VALUE would be considered a "special value" in my interpretation of the spec. My reading is simply that there are no magic numbers which mean "end of line" or "end of document". Therefore Number.MAX_VALUE is a valid value (certainly for a sufficiently large document!).

Digging into Kate a little, I can see that the LSPTextEdit returned by the language server should be parsed and validated correctly by parseTextEdit (following this down the call stack I can see that ultimately LSPPosition is mapped to KTextEditor::Cursor and would be considered a valid position according to the docs). So my best guess is that KTextEditor::Document::transformRange or KTextEditor::Document::replaceText is unable to handle a range end that is outwith the current document. Does that sound about right?

Actually, having dug yet further, it looks like KTextEditor::DocumentPrivate::removeText (at least) should be able to handle the end of the range being past the end of the document. Any ideas? What behaviour are you seeing in Kate?

chris-reeves commented 1 month ago

Getting the correct line number to use in the range end should be fairly straightforward (document.lineCount()), but getting the correct character number will be tricker and would need careful thinking and reading of the spec to make sure multi-byte characters and line endings are accounted for correctly.

It would be good to better understand the issues that Kate is having before going down this route.

Waqar144 commented 1 month ago

(Of the top of my head) A range containing 0 -> MAX_VALUE, if invalid, would most likely be converted to an empty range by the MovingRange (think of it as a smart range that automatically adapts to text changes in the document) I think. Thus remove text fails i.e., removes nothing and we end up duplicating the document.

getting the correct character number will be tricker and would need careful thinking and reading of the spec

It would be utf-16 based IIUC second para. Or I would do whatever vscode does to get column number correctly.


Regardless, this issue can actually be solved on the Kate side by doing what VSCode (and nvim?) does which is snap the range to the end of document if the range is bigger and I am actually considering this since there are other servers having this same problem.


Also see https://github.com/microsoft/language-server-protocol/issues/257 for a similar issue.

Waqar144 commented 1 month ago

Done for Kate https://invent.kde.org/utilities/kate/-/merge_requests/1549