fix: crash when renaming files that have sync-formatting autocmd #591
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When renaming or moving files/directories, neo-tree used to change the
name of renamed buffers while retaining the same buffer numbers. It did
not delete the old buffers and open new ones for editing.
This ended up crashing Neovim in the following scenario:
* a `BufWritePre` autocmd was created to call
`vim.lsp.buf.format({ async = false })`
* there was an LSP client registered for the buffer
* the buffer was opened and then put in the background (e.g. by
`:Neotree position=current`)
* the file was renamed/moved in neo-tree
The operation that caused the Neovim crash was the `silent! write!` that
neo-tree executes on the buffer right after it is renamed. During that
write, synchronous formatting is attempted in the `BufWritePre` autocmd.
My assumption is that the LSP server does not get a notification about
the file rename and sends some invalid formatting information back to
Neovim. Neovim cannot interpret it and crashes.
The crash does not happen if the renamed buffer is currently visible,
rather than being hidden (e.g. when the renamed buffer is in one window
and neo-tree is opened in a vertical split).
The approach to handling file renamed used in this commit is the same as
used in the `vim.lsp.util.rename` [0]. Instead of reusing the buffers in
Neovim before and after rename, neo-tree deletes old buffers for the
file paths before the rename, and opens new buffers for files after the
rename. If old buffers were shown in some Neovim window, the code opens
the new buffers in these windows.
The code that handles modified buffers during rename calls `silent!
write!` on the new buffers. This no longer crashes Neovim. The downside
is that LSP clients are not attached at the moment `write!` is called,
which means the files after the rename will not be formatted after
neo-tree saves the modified content. Overall, this seems like a lesser
evil compared to Neovim crashing on each rename.
Related to #308
[0]: https://github.com/neovim/neovim/blob/234b8c5f3d57294dda06dbc6c1760e5983bd2c19/runtime/lua/vim/lsp/util.lua#L751-L775
Gelio
changed the title
Codecov Report
@@ Coverage Diff @@
## main #591 +/- ##
==========================================
- Coverage 50.56% 50.56% -0.01%
==========================================
Files 47 47
Lines 6053 6061 +8
==========================================
+ Hits 3061 3065 +4
- Misses 2992 2996 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Contributor
Author
|
Thanks for merging my PRs! What is the schedule of merging |
Contributor
|
I just released. My normal rule is to let new changes sit in main for at least a day to see if there are any bugs reported. |
Contributor
Author
|
I see, thanks for quick action! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When renaming or moving files/directories, neo-tree used to change the name of renamed buffers while retaining the same buffer numbers. It did not delete the old buffers and open new ones for editing.
This ended up crashing Neovim in the following scenario:
BufWritePreautocmd was created to callvim.lsp.buf.format({ async = false }):Neotree position=current)The operation that caused the Neovim crash was the
silent! write!that neo-tree executes on the buffer right after it is renamed. During that write, synchronous formatting is attempted in theBufWritePreautocmd.My assumption is that the LSP server does not get a notification about the file rename and sends some invalid formatting information back to Neovim. Neovim cannot interpret it and crashes.
Crash backtrace: bt.txt
Peek.2022-11-05.14-07.rename.crash.mp4
The crash does not happen if the renamed buffer is currently visible, rather than being hidden (e.g. when the renamed buffer is in one window and neo-tree is opened in a vertical split). The crash also does not happen if I don't run
vim.lsp.buf.formatonBufWritePre. Neither of these is a great workaround, as I don't want to remember to keep files open or remove my formatting autocmd.The approach to handling file renamed used in this commit is the same as used in the
vim.lsp.util.rename. Instead of reusing the buffers in Neovim before and after rename, neo-tree deletes old buffers for the file paths before the rename, and opens new buffers for files after the rename. If old buffers were shown in some Neovim window, the code opens the new buffers in these windows.Peek.2022-11-05.14-08.rename.after.fix.mp4
The code that handles modified buffers during rename calls
silent! write!on the new buffers. This no longer crashes Neovim. The downside is that LSP clients are not attached at the momentwrite!is called, which means the files after the rename will not be formatted after neo-tree saves the modified content. Overall, this seems like a lesser evil compared to Neovim crashing on each rename.All in all, the functionality seems to be the same before and after my fix, with the difference that I no longer get Neovim crashes when renaming files that have a formatting autocmd.
Related to #308