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: Autoformat on save in neovim #575

Open
mna opened this issue Aug 17, 2023 · 9 comments
Open

LSP: Autoformat on save in neovim #575

mna opened this issue Aug 17, 2023 · 9 comments

Comments

@mna
Copy link

mna commented Aug 17, 2023

Hello,

I'm using neovim with the standardrb LSP configured:

$ nvim --version
NVIM v0.10.0-dev+852-g54be7d6b4
Build type: RelWithDebInfo
LuaJIT 2.1.0-beta3

It does report the documentFormattingProvider capability, but it doesn't seem to work when I try to format a buffer, I get the following error in the LSP log:

[ERROR][2023-08-17 10:54:09] .../vim/lsp/rpc.lua:675	"rpc"	"<path>/standardrb"	"stderr"	"[server] Format request arrived before text synchonized; skipping: `file:///<path>/a.rb'\n"

Is there some configuration I'm missing? I'm using the default LSP configuration as documented in the nvim-lspconfig link above (require'lspconfig'.standardrb.setup{}). I noticed the stardard repo's Wiki on neovim doesn't mention formatting as part of its page, so I'm not sure if that's something that's supported or not (even though it does report the capability).

Thanks,
Martin

@searls
Copy link
Contributor

searls commented Aug 17, 2023

hey @will would you be so kind as to look at this?

@mna
Copy link
Author

mna commented Aug 17, 2023

I've taken a closer look at this, it's not just autoformat on save, it's the formatting of the buffer that has this bug (that is, it doesn't have to be called in a BufWritePre to reproduce).

If that helps (I'm not a ruby expert, nor an LSP one!), when I tweak that line https://github.com/standardrb/standard/blob/main/lib/standard/lsp/routes.rb#L163 for this:

@text_cache[file_uri] = text unless text.nil?

formatting does work. It seems like for some reason, it receives a nil text before formatting (even though the vim buffer is never empty) and that clears the @text_cache thing so no formatting happens.

@fernandes
Copy link

@mna after testing this unless, right, it formats, but it does not show the diagnostics anymore

@mna
Copy link
Author

mna commented Aug 17, 2023

@fernandes oh no, for sure, I didn't mean that this was a workaround (and even less a fix)! Just that it illustrates that for some reason, the diagnostic call receives a nil text before formatting, storing it in the @text_cache and that's why it doesn't format anything.

@will
Copy link
Contributor

will commented Aug 18, 2023

@mna it formats for me on nvim 0.9.1, which is the current released version of nivm. Formatting breaking might be due to an upcoming change in 10?

I'm not in a good situation at the moment to build and try neovim nightly. Would you be able to try formatting with 0.9.1 to test this theory?

@fernandes
Copy link

@will 💥 you nailed it!

on 0.10 (nighly compiled / macos) happens this error, using nvim 0.9.1 it works perfectly, so yes, some breaking on 10 👍

@waferbaby
Copy link

Just wanted to add that I'm seeing this too!

@RomainMorlevat
Copy link

I also have the error with SublimeText 4169.

muxcmux added a commit to muxcmux/standard that referenced this issue Jan 30, 2024
The parameters for this request only include a textDocument, which is a
TextDocumentIdentifier, an identifier? and a previousResultId?. The
TextDocumentIdentifer only has a uri property, which is of type
DocumentUri.

According to the LSP spec:

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

this request does not include the contents of the file (which is what
the handler was expecting).

I have simply made the handling of this request a no-op, since
diagnostics are already sent to the client in textDocument/didChange.

Note: This also fixes formatting problems related to Neovim 0.10
described in standardrb#575
@muxcmux
Copy link
Contributor

muxcmux commented Jan 30, 2024

I think this is because currently textDocument/diagnostic is handled incorrectly. It expects the client to send the text in the request, but according to the spec, this request only includes the document uri. I've opened a PR with a fix.

muxcmux added a commit to muxcmux/rubocop that referenced this issue Jan 31, 2024
The `textDocument` parameter for the `textDocument/diagnostic` request
is of type `TextDocumentIdentifier`, which has a single property `uri`
of type `DocumentUri`.

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

According to the spec the `DocumentUri` type does not hold the contents
of the document, which the handler currently relies on in order to
publish diagnostics to the client.

This breaks the LSP mode in Neovim 0.10, which now supports pull
diagnostics, and possibly other editors too. The same problem exists in
`standardrb` (standardrb/standard#575)

I have made the `textDocument/diagnostic` hander a no-op, since
diagnostics are already being pushed to clients on
`textDocument/didChange`.
muxcmux added a commit to muxcmux/rubocop that referenced this issue Feb 2, 2024
The `textDocument` parameter for the `textDocument/diagnostic` request
is of type `TextDocumentIdentifier`, which has a single property `uri`
of type `DocumentUri`.

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

According to the spec the `DocumentUri` type does not hold the contents
of the document, which the handler currently relies on in order to
publish diagnostics to the client.

This breaks the LSP mode in Neovim 0.10, which now supports pull
diagnostics, and possibly other editors too. The same problem exists in
`standardrb` (standardrb/standard#575)

I have made the `textDocument/diagnostic` hander a no-op, since
diagnostics are already being pushed to clients on
`textDocument/didChange`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants