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: vim.lsp.buf.formatting_sync() messes up marks #14307

Closed
MagicDuck opened this issue Apr 6, 2021 · 6 comments · Fixed by #14630
Closed

LSP: vim.lsp.buf.formatting_sync() messes up marks #14307

MagicDuck opened this issue Apr 6, 2021 · 6 comments · Fixed by #14630
Labels
bug issues reporting wrong behavior lsp

Comments

@MagicDuck
Copy link

MagicDuck commented Apr 6, 2021

  • nvim --version:
NVIM v0.5.0-dev+1157-g0ab88c2ea
Build type: Release
LuaJIT 2.1.0-beta3
Compilation: /Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 -O2 -DNDEBUG -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wshadow -Wconversion -Wmissing-prototypes -Wimplicit-fallthrough -Wvla -fstack-protector-strong -fno-common -fdiagnostics-color=always -DINCLUDE_GENERATED_DECLARATIONS -D_GNU_SOURCE -DNVIM_MSGPACK_HAS_FLOAT32 -DNVIM_UNIBI_HAS_VAR_FROM -DMIN_LOG_LEVEL=3 -I/Users/runner/work/neovim/neovim/build/config -I/Users/runner/work/neovim/neovim/src -I/Users/runner/work/neovim/neovim/.deps/usr/include -I/Applications/Xcode_12.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk/usr/include -I/Library/Frameworks/Mono.framework/Headers -I/Users/runner/work/neovim/neovim/build/src/nvim/auto -I/Users/runner/work/neovim/neovim/build/include
Compiled by runner@Mac-1616045056542.local

Features: +acl +iconv +tui
  • language server name/version: N/A
  • Operating system/version: macos

Steps to reproduce using nvim -u minimal_init.lua

  1. Set up some language server that does formatting. For instance diagnosticls:
    (You will need prettier installed in the example below, but I don't think the particular formatter matters)
lspconfig.diagnosticls.setup {
  root_dir = function(filepath)
    return vim.fn.expand("~")
  end,
  filetypes = {
    "javascript",
  },
  init_options = {
    formatFiletypes = {
      javascript = {"prettier"},
    },
    formatters = {
      prettier = {
        command = "prettier",
        args = {"--stdin", "--stdin-filepath", "%filepath"},
        isStdout = true,
        rootPatterns = {".prettierrc", ".prettierrc.json"}
      }
    }
  },
  on_init = function(client)
    client.resolved_capabilities.document_formatting = true
  end,
  on_attach = function()
      vim.api.nvim_command [[autocmd BufWritePre <buffer> lua vim.lsp.buf.formatting_sync({}, 5000) ]]
  end,
}
  1. Open a js file with a bunch of code in it
  2. add mark A on line 3 for example
  3. move to line 7, add a bunch of whitespace lines, then save to trigger formatter

Actual behaviour

Mark is removed (or moved to the top of the file)

Observation:
If :wshada! is executed before saving and :rshada! after, the marks are restored to proper position

Expected behaviour

Marks stay in place.

A hacky workaround:

Wasn't sure how expensive :wshada/rshada are so wrote the following instead to use on BufWritePre

LspFormatBuffer = function(bufnr) 
  local marks = {}
  local letters = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'
  for i = 1, #letters do
    local letter = letters:sub(i, i)
    local markLocation = vim.api.nvim_buf_get_mark(bufnr, letter)
    marks[letter] = markLocation;
  end

  vim.lsp.buf.formatting_sync({}, 5000) 

  for i = 1, #letters do
    local letter = letters:sub(i, i)
    local markLocation = marks[letter];
    local linenr = markLocation[1]
    if (linenr ~= 0) then
      vim.api.nvim_command("" .. linenr .. "mark " .. letter)
    end
  end
end
@mfussenegger
Copy link
Member

I think the root cause of this is that formatting_sync applies the changes using nvim_buf_set_lines, and nvim_buf_set_lines doesn't preserve extmarks when only part of a line is modified.

There's a PR pending (#13703) which would change it to use nvim_buf_set_text which preserves on the extmarks on non-modified parts of changed lines.

@chentoast
Copy link
Contributor

I think the author might be talking about regular vim marks instead of extmarks; nvim_buf_set_text and nvim_buf_set_lines will both invalidate any marks that lie within the changed range.

@MagicDuck
Copy link
Author

MagicDuck commented Apr 10, 2021

Yes, that's correct, talking about regular marks! Wasn't sure what extmarks were 😄
Unfortunately, with most formatters they operate on the whole file instead of a range, so all marks are lost. I think it's ok to at least keep the same number as before as typically re-formats don't produce world-shaking movements of code.

@folke
Copy link
Member

folke commented May 22, 2021

I noticed that as well, but never thought I was due to LSP :) Will see if I can make a PR for this, since it's pretty annoying

@mjlbach mjlbach added this to Triage in LSP integration via automation Nov 1, 2021
@mjlbach mjlbach moved this from Triage to 0.8 to-dos in LSP integration Nov 1, 2021
@mjlbach mjlbach moved this from 0.8 to-dos to 0.9 to-dos in LSP integration Nov 1, 2021
@JoaquimEsteves
Copy link

JoaquimEsteves commented May 31, 2023

Any chance the maintainers could revisit this?

I noticed that @folke was kind enough to provide a pr that has been closed.

In the comments to the PR a contributer mentioned that this is due to the fact that after formatting some marks point to garbage; and as such the user should "assume" that after formatting a file all marks will be removed.

As a user I don't agree, and would appreciate some config option that would incorporate this pr.

Something like:

nvim_lsp.my_lang_server.setup({ keep_marks_even_if_garbage = True })

In practise, formatting the text would in all likelihood keep most of my marks exactly the same.

Let's take this example:

a| 0|         some_obj = {
 | 1|             "integer": {
b| 2|                 "numpy": np.int32,
 | 3|                 "pandas_str": "int32",
c| 4|                 "sql": sql.types.Integer,      "python": int,
 | 5|             },

On the first column we have the marks, second we have the line number.

Assume that mark C points to the 'p' character in "python" on line 4.

After running my formatting program I'd end up with:

a| 0|         some_obj = {
 | 1|             "integer": {
b| 2|                 "numpy": np.int32,
 | 3|                 "pandas_str": "int32",
c| 4|                 "sql": sql.types.Integer,
 | 5|                 "python": int,
 | 6|             },

Marks 'a' and 'b' remain exactly the same, and using 'c I'd still end up on the correct line - typing `c would point me to garbage, but ideally that would just result in an error message.

Edit: Another point in favour of adopting the PR is that marks aren't deleted when calling an external formatter on a buffer. If I invoke !formatter % then vim will do it's best to make it so the marks remain in place.

@justinmk
Copy link
Member

justinmk commented Jun 4, 2023

I noticed that @folke was kind enough to provide a pr that has been closed.

In favor of reopening that, if @folke feels motivated.

justinmk pushed a commit that referenced this issue Jun 4, 2023
PROBLEM:
Whenever any text edits are applied to the buffer, the `marks` part of those
lines will be lost. This is mostly problematic for code formatters that format
the whole buffer like `prettier`, `luafmt`, ...

When doing atomic changes inside a vim doc, vim keeps track of those changes and
can update the positions of marks accordingly, but in this case we have a whole
doc that changed. There's no simple way to update the positions of all marks
from the previous document state to the new document state.

SOLUTION:
* save marks right before `nvim_buf_set_lines` is called inside `apply_text_edits`
* check if any marks were lost after doing `nvim_buf_set_lines`
* restore those marks to the previous positions

TEST CASE:
* have a formatter enabled
* open any file
* create a couple of marks
* indent the whole file to the right
* save the file
Before this change: all marks will be removed.
After this change: they will be preserved.

Fixes #14307
vimpostor added a commit to vimpostor/ale that referenced this issue Jul 8, 2023
The signs flickered because nvim_buf_set_lines() removes all signs from
lines that it touches, which will immediately be readded by Ale (causing
the brief flicker). This is intended behaviour in neovim [0].

Neovim itself faced this problem in their own LSP formatting sync,
although they had the problem with marks instead of signs [1].
Similar to how neovim fixed it by storing and restoring the marks [2],
we can do the same thing with signs.

In fact it is easier with signs, because sign_placelist() will just
ignore and skip invalid line numbers, so we don't need to filter signs
that are not valid anymore.

[0] neovim/neovim#10880 (comment)
[1] neovim/neovim#14307
[2] neovim/neovim#14630
vimpostor added a commit to vimpostor/ale that referenced this issue Aug 20, 2023
The signs flickered because nvim_buf_set_lines() removes all signs from
lines that it touches, which will immediately be readded by Ale (causing
the brief flicker). This is intended behaviour in neovim [0].

Neovim itself faced this problem in their own LSP formatting sync,
although they had the problem with marks instead of signs [1].
Similar to how neovim fixed it by storing and restoring the marks [2],
we can do the same thing with signs.

In fact it is easier with signs, because sign_placelist() will just
ignore and skip invalid line numbers, so we don't need to filter signs
that are not valid anymore.

[0] neovim/neovim#10880 (comment)
[1] neovim/neovim#14307
[2] neovim/neovim#14630
vimpostor added a commit to vimpostor/ale that referenced this issue Aug 20, 2023
The signs flickered because nvim_buf_set_lines() removes all signs from
lines that it touches, which will immediately be readded by Ale (causing
the brief flicker). This is intended behaviour in neovim [0].

Neovim itself faced this problem in their own LSP formatting sync,
although they had the problem with marks instead of signs [1].
Similar to how neovim fixed it by storing and restoring the marks [2],
we can do the same thing with signs.

In fact it is easier with signs, because sign_placelist() will just
ignore and skip invalid line numbers, so we don't need to filter signs
that are not valid anymore.

[0] neovim/neovim#10880 (comment)
[1] neovim/neovim#14307
[2] neovim/neovim#14630
w0rp pushed a commit to dense-analysis/ale that referenced this issue Sep 5, 2023
* Avoid performance problems with setbufline() and Treesitter

Call nvim_buf_set_lines() instead.

Since this is a performance problem only in Neovim (Treesitter is only
available there), it doesn't matter that this API is unavailable in Vim.

Note: nvim_buf_set_lines() returns E5555, when set nomodifiable is on.

Fixes #3669

* Avoid sign flickering

The signs flickered because nvim_buf_set_lines() removes all signs from
lines that it touches, which will immediately be readded by Ale (causing
the brief flicker). This is intended behaviour in neovim [0].

Neovim itself faced this problem in their own LSP formatting sync,
although they had the problem with marks instead of signs [1].
Similar to how neovim fixed it by storing and restoring the marks [2],
we can do the same thing with signs.

In fact it is easier with signs, because sign_placelist() will just
ignore and skip invalid line numbers, so we don't need to filter signs
that are not valid anymore.

[0] neovim/neovim#10880 (comment)
[1] neovim/neovim#14307
[2] neovim/neovim#14630
mnikulin pushed a commit to mnikulin/ale that referenced this issue Nov 12, 2023
* Avoid performance problems with setbufline() and Treesitter

Call nvim_buf_set_lines() instead.

Since this is a performance problem only in Neovim (Treesitter is only
available there), it doesn't matter that this API is unavailable in Vim.

Note: nvim_buf_set_lines() returns E5555, when set nomodifiable is on.

Fixes dense-analysis#3669

* Avoid sign flickering

The signs flickered because nvim_buf_set_lines() removes all signs from
lines that it touches, which will immediately be readded by Ale (causing
the brief flicker). This is intended behaviour in neovim [0].

Neovim itself faced this problem in their own LSP formatting sync,
although they had the problem with marks instead of signs [1].
Similar to how neovim fixed it by storing and restoring the marks [2],
we can do the same thing with signs.

In fact it is easier with signs, because sign_placelist() will just
ignore and skip invalid line numbers, so we don't need to filter signs
that are not valid anymore.

[0] neovim/neovim#10880 (comment)
[1] neovim/neovim#14307
[2] neovim/neovim#14630
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issues reporting wrong behavior lsp
Projects
LSP integration
0.9 to-dos (Speed (round two), rpc ho...
Development

Successfully merging a pull request may close this issue.

6 participants