-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Dispatch textDocument/didChange after rename (2) #4049
Dispatch textDocument/didChange after rename (2) #4049
Conversation
Previously whenever we renamed a symbol that was referenced from other files we'd just edit those files in the background, and the LSP wouldn't know about these changes. If we tried to rename the same symbol again, the renaming would fail. In some scenarios, the operation would just be wrong. Here is an attempt to fix this issue. I also noticed another bug when using Go with `gopls` LSP and the `gofmt` fixer. Whenever the file was saved, the `gofmt` would run and reformat the file. But it seems there was some kind of a race condition so I disabled saving for now, and all of the modified files will be unsaved, so the user should call `:wa` to save them. I personally like this even better because I can inspect exactly what changes happened, and I instantly see them in the other opened buffers, which was previously not the case. Fixes dense-analysis#3343, dense-analysis#3642, dense-analysis#3781.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't recall what changes I wanted to make to the tests, and reading the code again now it looks all right. So I just added some final nitpicks that I spotted now and didn't notice before, but overall it's probably good.
(Oh and I just noticed that python-lsp-server can do cross-file renames, so I could probably switch to this branch and try it. Can't promise today, though.)
@@ -35,19 +35,16 @@ Before: | |||
set fileformats=unix | |||
|
|||
" two files, one accessed through a buffer, the other using write/readfile only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outdated comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, removed!
test/test_code_action.vader
Outdated
AssertEqual [2, 2], getpos('.')[1:2] | ||
|
||
# ====C==== | ||
Execute(Cursor column will move to the change end when cursor between start/end): | ||
let g:test.changes = g:test.create_change(2, 3, 2, 8, 'value2') | ||
|
||
for r in range(3, 8) | ||
" make tests at the end work | ||
let g:notified_changes = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to assert the contents of g:notified_changes
explicitly/deliberately in some tests (where we test for the notification) rather than to pollute most tests with workarounds for an assertion in the test teardown, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, added more and better assertions and removed these resets.
The issue still persists for me: Exact steps to reproduce:
My ALEInfo:
|
Thanks @bogdan! Would you be able to paste the ALE commit you have checked out just make we're testing on the same codebase? |
I checked out last master commit (a81512c) and merged this branch. |
Got it. Do you have any other plugins installed that might interfere with this functionality? I haven't gotten the chance to check out your repo yet, but I've definitely tested I think its failing at the |
I tried to disable all plugins except ALE and the issue persisted. |
Thanks, just one more question: which version of Neovim are you running? |
I tried to update to 0.6.1 but nothing changed. |
@bogdan there must be something different in our configurations. I've cloned your repo and it behaves the same as any other TS project: 2022_02_02_22_45_47_recording.mp4I've also tried disabling all of my configs and plugins except |
@bogdan from my limited testing this PR does not break anything on my environment and would like to merge it. Is the issue you are seeing caused by this PR? that is, without it the issue does not occur? I cannot merge it if it breaks stuff for others. |
@bogdan could you try disabling all fixers from I don't think these changes are synchronized, perhaps they should be. |
Well in my case, it made
Didn't help. While debugging all that I was under the expression that the plugin tries to apply changes from all files to a currently open file: here is how Any idea what may cause that? I've put some debug info to make sure it is on the same page. Inside
Inside
Inside
|
@bogdan thanks for digging into it! Can you try making sure You can check the curent setting using |
I have |
Maybe we should force save when |
It makes perfect sense that we can't edit files in the background when
|
Makes me wonder if perhaps going back to #3782 (with the additional
If we stick with your approach here, @bogdan with On the other hand your proposed code is way simpler… |
The feature that you may undo a change in multiple files is quite superior. I am not sure if any IDE supports that. I would expect the change to multiple files (especially if some files also got renamed) to be automatically saved and irreversible so in case I need to rollback I call Is it possible to set It is also possible to introduce different behaviour if multiple or single files are changed: allow |
I just remembered that I had do disable saving of files to get around a bug when modifying open files in the background: whenever such a file was modified and saved ALE would run fixers if they were enabled and then the files would sometimes be modified again, but the LSP wouldn't get notified about this change, which introduced a bug when the same rename operation was performed twice because the LSP response would contain old positions, before the fixer was run. I wrote about it in the commit message of the first commit:
|
Is it possible to disable fixers temporarily after when rename operation is performed instead? |
I'm not sure at this point. But this PR has been working perfectly for me with |
@jeremija I really want to merge this PR as it does fixes the three issues mentioned in the description and from my testing using ruby solargraph it works very well. But considering that "nohidden" is the default in vim/neovim and that if this option is set then the renaming fails I thinks is hard to justify the merge. After some thinking I believe the best option here is to tell ALE to save the files by setting "should_save" to true when calling the code action handler. I left comments in the PR showing were to make the changes. I tested this and works find with hidden and nohidden set and also is the way filerename works so I think this is the proper way to handle this. |
@@ -116,7 +116,7 @@ function! ale#rename#HandleLSPResponse(conn_id, response) abort | |||
\ 'changes': l:changes, | |||
\ }, | |||
\ { | |||
\ 'should_save': 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually seems this was the behavior before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it was, the reason why I changed the default is in the description of the first commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, also added an option to disable this behavior.
Where did you get that info from? According to the excerpt from Neovim help I posted above, it seems like That said, I understand why you would want to make these changes, but I'd like to at least make the saving optional, and we can warn the users in help that disabling saving won't work with |
bfca341
to
780971d
Compare
Also provide options to disable automatic saving, as well as instructions to enable `set hidden` before doing that.
780971d
to
026de06
Compare
Making it configurable should be straightforward but honestly I think allowing users to set this variable with the "hidden" option off will result in many shooting themselves in the foot. If "hidden" is off then ALERename will throw cryptic errors and all files won't be properly changed as expected. This most likely will be seen as a bug by many even if documented in the help. I rather set the behavior based on if hidden is on or off like:
This would tell ALE to save the buffers if "hidden" is off and don't save them if hidden is on. If you prefer to allow users to change this behavior via a config (e.g. g:ale_save_on_rename) at least make the default use the same logic based on hidden configuration. This way ALE will just work, in the sense that no errors will be thrown, for both hidden on and off. |
@jeremija great!, thanks for the changes. Tested with hidden and nohidden and works as expected. Just one little detail that is annoying at best. When renaming a class that is used in several files neovim asks me to press enter many times. Three times per file when nohidden is set and twice per file when hidden is set. This is because the "Execute() function calls. I left some comments in the PR adding silent to the problematic places so this does not happen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one last request and I think this is ready to merge.
autoload/ale/code_action.vim
Outdated
else | ||
let l:lines = readfile(a:filename, 'b') | ||
if l:buffer != l:orig_buffer | ||
call ale#util#Execute('edit ' . a:filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add silent so neovim stops asking to press Enter to continue:
call ale#util#Execute('silent edit ' . a:filename)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
autoload/ale/code_action.vim
Outdated
call ale#lsp#NotifyForChanges(l:conn_id, l:buffer) | ||
|
||
if l:should_save | ||
call ale#util#Execute(':w!') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here:
call ale#util#Execute(':silent w!')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
autoload/ale/code_action.vim
Outdated
call setpos('.', [0, l:pos[0], l:pos[1], 0]) | ||
|
||
if l:orig_buffer != l:buffer && bufexists(l:orig_buffer) | ||
call ale#util#Execute('buf ' . string(l:orig_buffer)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here:
call ale#util#Execute('silent buf ' . string(l:orig_buffer))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work and patience.
Opening a new PR because #3783 was closed and we cannot reopen it.
Fixes #3343, #3642, #3781.
Video example: