dense-analysis / ale

Check syntax in Vim/Neovim asynchronously and fix files, with Language Server Protocol (LSP) support
BSD 2-Clause "Simplified" License
13.55k stars 1.44k forks source link

Running ALEFix via g:ale_fix_on_save causes an error in Neovim on :wq #1960

Closed Utagai closed 5 years ago

Utagai commented 6 years ago

Information

VIM version

NVIM v0.3.1
Build type: Release

Operating System:

Distributor ID: Ubuntu
Description:    Ubuntu 16.04.5 LTS
Release:        16.04
Codename:       xenial

:ALEInfo

Current Filetype: go
Available Linters: ['gobuild', 'gofmt', 'golangci-lint', 'golint', 'gometalinter', 'gosimple', 'gotype', 'govet', 'gola
ngserver', 'staticcheck']
   Linter Aliases:
'gobuild' -> ['go build']
'govet' -> ['go vet']
  Enabled Linters: ['gofmt', 'golint', 'govet']
 Suggested Fixers:
  'gofmt' - Fix Go files with go fmt.
  'goimports' - Fix Go files imports with goimports.
  'remove_trailing_lines' - Remove all blank lines at the end of a file.
  'trim_whitespace' - Remove all trailing whitespace characters at the end of every line.
 Linter Variables:
let g:ale_go_govet_options = ''
 Global Variables:
let g:ale_cache_executable_check_failures = v:null
let g:ale_change_sign_column_color = 0
let g:ale_command_wrapper = ''
let g:ale_completion_delay = v:null
let g:ale_completion_enabled = 0
let g:ale_completion_max_suggestions = v:null
let g:ale_echo_cursor = 1
let g:ale_echo_msg_error_str = 'Error'
let g:ale_echo_msg_format = '%linter% says %s'
let g:ale_echo_msg_info_str = 'Info'
let g:ale_echo_msg_warning_str = 'Warning'
let g:ale_enabled = 1
let g:ale_fix_on_save = 1
let g:ale_fixers = {'go': ['goimports'], 'cpp': ['clang-format']}
let g:ale_history_enabled = 1
let g:ale_history_log_output = 1
let g:ale_keep_list_window_open = 0
let g:ale_lint_delay = 200
let g:ale_lint_on_enter = 1
let g:ale_lint_on_filetype_changed = 1
let g:ale_lint_on_save = 1
let g:ale_lint_on_text_changed = 'always'
let g:ale_lint_on_insert_leave = 0
let g:ale_linter_aliases = {}
let g:ale_linters = {}
let g:ale_linters_explicit = 0
let g:ale_list_window_size = 10
let g:ale_list_vertical = 0
let g:ale_loclist_msg_format = '%linter% says %s'
let g:ale_max_buffer_history_size = 20
let g:ale_max_signs = -1
let g:ale_maximum_file_size = v:null
let g:ale_open_list = 0
let g:ale_pattern_options = v:null
let g:ale_pattern_options_enabled = v:null
let g:ale_set_balloons = 0
let g:ale_set_highlights = 1
let g:ale_set_loclist = 1
let g:ale_set_quickfix = 0
let g:ale_set_signs = 1
let g:ale_sign_column_always = 0
let g:ale_sign_error = '•'
let g:ale_sign_info = '•'
let g:ale_sign_offset = 1000000
let g:ale_sign_style_error = '•'
let g:ale_sign_style_warning = '•'
let g:ale_sign_warning = '•'
let g:ale_statusline_format = v:null
let g:ale_type_map = {}
let g:ale_use_global_executables = v:null
let g:ale_warn_about_trailing_blank_lines = 1
let g:ale_warn_about_trailing_whitespace = 1
  Command History:
(executable check - success) gofmt
(finished - exit code 0) ['/bin/bash', '-c', 'gofmt -e ''/tmp/nvimvOq7Mv/1/test.go''']
<<<NO OUTPUT RETURNED>>>
(executable check - success) golint
(finished - exit code 0) ['/bin/bash', '-c', 'golint ''/tmp/nvimvOq7Mv/2/test.go''']
<<<NO OUTPUT RETURNED>>>
(executable check - success) go
(finished - exit code 0) ['/bin/bash', '-c', 'cd ''/home/may/quick/go_test'' &&  go vet  .']
<<<NO OUTPUT RETURNED>>>
(finished - exit code 0) ['/bin/bash', '-c', 'gofmt -e ''/tmp/nvimvOq7Mv/4/test.go''']
<<<NO OUTPUT RETURNED>>>
(finished - exit code 0) ['/bin/bash', '-c', 'golint ''/tmp/nvimvOq7Mv/5/test.go''']
<<<NO OUTPUT RETURNED>>>

What went wrong

This error does not occur to me when using vim, whose version is:

VIM - Vi IMproved 8.1 (2018 May 18, compiled Jul  6 2018 20:28:46)
Included patches: 1-155

This only occurs on neovim. In particular, it only happens when ale_fix_on_save is turned on, and a fixer is ran. Furthermore, it only occurs when running :wq. Just writing the buffer via :w works perfectly, and just quitting via :q also works. Also, running :ALEFix works on its own. It is the combination of :wq that causes this issue. An error is printed by neovim upon closing:

go_test • vim test.go

Error detected while processing function <SNR>101_NeoVimCallback[29]..<SNR>120_HandleExit:
line   13:
E484: Can't open file /tmp/nvimvOq7Mv/6/test.go⏎

After looking into this a bit more myself, I realized that it seems like the temporary file/buffer/whatever, (in this situation, at /tmp/nvimvOq7Mv/6/test.go, but this can obviously change across runs) does not exist by the time ALE gets around to doing its magic. I think what may be happening here is that neovim is destroying the file at that location before ale can get around to running gofmt or goimports on the buffer contents. This is further corroborated by the fact that when the file is re-opened, no fixes can be observed, suggesting that the fixer either never ran or ALE was not able to fix the buffer successfully in time for the write.

EDIT: I just tried this out with a .h file, related to my st configuration, and got this error on :wq:

st • vim src/config.h

Error detected while processing function <SNR>101_NeoVimCallback[29]..<SNR>118_HandleExit[41]..<SNR>118_RunFixer[57]..ale#fix#ApplyFixes:
line   12:
The file was changed before fixing finished⏎

All other symptoms of this issue are the same, but seems like the error is slightly different. In either case, this is hinting at some sort of timing/race problem. The :ALEInfo output for this is littered with issues since this is basically someone else's config.h who breaks a lot of rules for cpplint, so I've gutted out most of those warnings/errors:

Current Filetype: cpp
Available Linters: ['ccls', 'clang', 'clangcheck', 'clangd', 'clangtidy', 'clazy', 'cppcheck', 'cpplint', 'cquery', 'fl
awfinder', 'gcc']
   Linter Aliases:
'gcc' -> ['g++']
  Enabled Linters: ['ccls', 'clang', 'clangcheck', 'clangd', 'clangtidy', 'clazy', 'cppcheck', 'cpplint', 'cquery', 'fl
awfinder', 'gcc']
 Suggested Fixers:
  'clang-format' - Fix C/C++ files with clang-format.
  'remove_trailing_lines' - Remove all blank lines at the end of a file.
  'trim_whitespace' - Remove all trailing whitespace characters at the end of every line.
  'uncrustify' - Fix C, C++, C#, ObjectiveC, ObjectiveC++, D, Java, Pawn, and VALA files with uncrustify.
 Linter Variables:
let g:ale_cpp_ccls_executable = 'ccls'
let g:ale_cpp_ccls_init_options = {}
let g:ale_cpp_clang_executable = 'clang++'
let g:ale_cpp_clang_options = '-std=c++14 -Wall'
let g:ale_cpp_clangcheck_executable = 'clang-check'
let g:ale_cpp_clangcheck_options = ''
let g:ale_cpp_clangd_executable = 'clangd'
let g:ale_cpp_clangd_options = ''
let g:ale_cpp_clangtidy_checks = ['*']
let g:ale_cpp_clangtidy_executable = 'clang-tidy'
let g:ale_cpp_clangtidy_options = ''
let g:ale_cpp_clazy_checks = ['level1']
let g:ale_cpp_clazy_executable = 'clazy-standalone'
let g:ale_cpp_clazy_options = ''
let g:ale_cpp_cppcheck_executable = 'cppcheck'
let g:ale_cpp_cppcheck_options = '--enable=style'
let g:ale_cpp_cpplint_executable = 'cpplint'
let g:ale_cpp_cpplint_options = ''
let g:ale_cpp_cquery_cache_directory = '/home/may/.cache/cquery'
let g:ale_cpp_cquery_executable = 'cquery'
let g:ale_cpp_flawfinder_executable = 'flawfinder'
let g:ale_cpp_flawfinder_minlevel = 1
let g:ale_cpp_flawfinder_options = ''
let g:ale_cpp_gcc_executable = 'gcc'
let g:ale_cpp_gcc_options = '-std=c++14 -Wall'
 Global Variables:
let g:ale_cache_executable_check_failures = v:null
let g:ale_change_sign_column_color = 0
let g:ale_command_wrapper = ''
let g:ale_completion_delay = v:null
let g:ale_completion_enabled = 0
let g:ale_completion_max_suggestions = v:null
let g:ale_echo_cursor = 1
let g:ale_echo_msg_error_str = 'Error'
let g:ale_echo_msg_format = '%linter% says %s'
let g:ale_echo_msg_info_str = 'Info'
let g:ale_echo_msg_warning_str = 'Warning'
let g:ale_enabled = 1
let g:ale_fix_on_save = 1
let g:ale_fixers = {'go': ['goimports'], 'cpp': ['clang-format']}
let g:ale_history_enabled = 1
let g:ale_history_log_output = 1
let g:ale_keep_list_window_open = 0
let g:ale_lint_delay = 200
let g:ale_lint_on_enter = 1
let g:ale_lint_on_filetype_changed = 1
let g:ale_lint_on_save = 1
let g:ale_lint_on_text_changed = 'always'
let g:ale_lint_on_insert_leave = 0
let g:ale_linter_aliases = {}
let g:ale_linters = {}
let g:ale_linters_explicit = 0
let g:ale_list_window_size = 10
let g:ale_list_vertical = 0
let g:ale_loclist_msg_format = '%linter% says %s'
let g:ale_max_buffer_history_size = 20
let g:ale_max_signs = -1
let g:ale_maximum_file_size = v:null
let g:ale_open_list = 0
let g:ale_pattern_options = v:null
let g:ale_pattern_options_enabled = v:null
let g:ale_set_balloons = 0
let g:ale_set_highlights = 1
let g:ale_set_loclist = 1
let g:ale_set_quickfix = 0
let g:ale_set_signs = 1
let g:ale_sign_column_always = 0
let g:ale_sign_error = '•'
let g:ale_sign_info = '•'
let g:ale_sign_offset = 1000000
let g:ale_sign_style_error = '•'
let g:ale_sign_style_warning = '•'
let g:ale_sign_warning = '•'
let g:ale_statusline_format = v:null
let g:ale_type_map = {}
let g:ale_use_global_executables = v:null
let g:ale_warn_about_trailing_blank_lines = 1
let g:ale_warn_about_trailing_whitespace = 1
  Command History:
(executable check - failure) clang++
(executable check - failure) clang-check
(executable check - failure) clang-tidy
(executable check - failure) clazy-standalone
(executable check - success) cppcheck
(finished - exit code 0) ['/bin/bash', '-c', '''cppcheck'' -q --language=c++ --enable=style ''/tmp/nvimOx5BvA/2/config.
h''']
<<<NO OUTPUT RETURNED>>>
(executable check - success) cpplint
(finished - exit code 1) ['/bin/bash', '-c', '''cpplint'' ''/home/may/dotfiles/xst/src/config.h''']
<<<OUTPUT STARTS>>>
/home/may/dotfiles/xst/src/config.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyrig
ht Owner>"  [legal/copyright] [5]
/home/may/dotfiles/xst/src/config.h:0:  No #ifndef header guard found, suggested CPP variable is: XST_SRC_CONFIG_H_  [b
uild/header_guard] [5]
/home/may/dotfiles/xst/src/config.h:8:  Complex multi-line /*...*/-style comment found. Lint may give bogus warnings.
Consider replacing these with //-style comments, with #if 0...#endif, or with more clearly structured multi-line commen
ts.  [readability/multiline_comment] [5]
/home/may/dotfiles/xst/src/config.h:8:  At least two spaces is best between code and comments  [whitespace/comments] [2]

...

/home/may/dotfiles/xst/src/config.h:430:  Tab found; better to use spaces  [whitespace/tab] [1]
Done processing /home/may/dotfiles/xst/src/config.h

...

<<<OUTPUT ENDS>>>
(executable check - failure) flawfinder
(executable check - success) gcc
(finished - exit code 1) ['/bin/bash', '-c', '''gcc'' -S -x c++ -fsyntax-only -iquote ''/home/may/dotfiles/xst/src'' -s
td=c++14 -Wall - < ''/tmp/nvimOx5BvA/3/config.h''']
<<<OUTPUT STARTS>>>
<stdin>:9:21: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings]

...

<<<OUTPUT ENDS>>>
(executable check - failure) clang++
(finished - exit code 0) ['/bin/bash', '-c', '''cppcheck'' -q --language=c++ --enable=style ''/tmp/nvimOx5BvA/4/config.
h''']
<<<NO OUTPUT RETURNED>>>
(executable check - failure) flawfinder
(finished - exit code 1) ['/bin/bash', '-c', '''gcc'' -S -x c++ -fsyntax-only -iquote ''/home/may/dotfiles/xst/src'' -s
td=c++14 -Wall - < ''/tmp/nvimOx5BvA/5/config.h''']
<<<OUTPUT STARTS>>>
<stdin>:9:21: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings]

...

<stdin>:423:8: error: ‘uint’ does not name a type
<<<OUTPUT ENDS>>>

Reproducing the bug

  1. Turn on ale_fix_on_save.
  2. Assign a fixer to a language (in this case, for go, using goimports; however, my guess is this issue applies to any language for which a fixer is assigned).
  3. Open a file of that language.
  4. Write changes to the file, or don't, and then run :wq.
  5. Notice the error message.

When browsing for issues pertaining to this, I saw that some people included ale specific configuration lines from their .vimrc for reproduction, here are mine:

let g:airline#extensions#ale#enabled = 1
...
" ale options
" Set the format of the echo msg so we know which thing is complaining
let g:ale_echo_msg_format = '%linter% says %s'
" Set the fixer for cpp files as clang-format
let g:ale_fixers = { 'cpp' : ['clang-format'], 'go': [ 'goimports' ] }
" Auto-fix on buffer write.
let g:ale_fix_on_save = 1
nmap <silent> <leader>aj :ALENext<cr>
nmap <silent> <leader>ak :ALEPrevious<cr>
highlight clear SignColumn
highlight clear ALEErrorSign
highlight clear ALEWarningSign
let g:ale_sign_error = '•'
let g:ale_sign_warning = '•'

Also, this problem seems to have been experienced by at least one other person, at this issue. I searched his username in the issues on this project, but did not find anything, and I didn't see anyone else post an issue about this, so I'm hoping this isn't a duplicate that wastes your time.

Thanks for your hard work on ALE. It has been really useful. :)

Utagai commented 6 years ago

Any idea on this? It's not critical but it is pretty irritating.

@w0rp If you are blocked by other work and/or are too busy in general, let me know. I have never worked with Vim plugins or Vim script, but I can give it a shot, as I am guessing that the bug and its fix is pretty localized.

w0rp commented 6 years ago

I've had the flu for the past few days, but I'm near 100% again now. As always, the fastest way to fix a bug is to try and fix it yourself. I might have a look if I get the time.

Utagai commented 6 years ago

Sorry to hear that @w0rp. Hope you recover fully.

I'll post here again if I end up taking a look. Will probably have to be this coming weekend at best.

Thanks for the update, all I really needed.

w0rp commented 6 years ago

It was just a seasonal flu, I'm fine.

sQVe commented 6 years ago

I'm also seeing this issue consistently @Utagai @w0rp.

w0rp commented 6 years ago

Is it goimports which is causing the issue. The error message is printed whenever something changes a file while ALE is trying to fix it. If goimports is modifying the file, it shouldn't be doing that. If something else is modifying the file, turn that off.

Utagai commented 6 years ago

@w0rp Note that this is happening even with C++. This isn't a Go-specific issue, as I had predicted. (See my original post for details on the C++ case).

This problem persists with gofmt as well.

It's finally worth noting that all three of these tools work perfectly fine with Vim (not neovim). This should suggest that the issue is something else, not the fixers.

From my investigation, this really just boils down to ALE trying to mess with a temporary file created by nvim that is somehow made inaccessible by the time it gets there. In either case, disabling the use of clang-format, gofmt or goimports would be a pretty bad hit to ALE, as these are pretty popular fixers/formatters for their respective languages and never caused issues before.

I'll try to see if there is any sort of setting or flag that we can use to these fixers that may help with stopping any kind of file modification tonight. If I find anything promising, I'll post them here.

w0rp commented 6 years ago

I suppose you'll have to figure out why NeoVim is deleting temporary files before ALE is done with them.

Utagai commented 6 years ago

That sounds about right for the path we need to walk to get to the bottom of this. :)

sQVe commented 6 years ago

@Utagai Just wanted to note that I see this issue with fixers for JavaScript, TypeScript and Haskell too. So it's obvious that the fixers isn't at fault here.

I have no experience with Vim script but let me know if I can help in some way.

Utagai commented 6 years ago

@sQVe Yes, that seems to corroborate my hunch that this is not a fixer problem, but a timing problem with ALE and temporary files. I'll have to look into the code to confirm this, however.

In either case, I also don't know anything about Vim script. I gave this a cursory look, I believe two weeks ago and found it quite dizzying. I had found some kind of resource about debugging plugins in Vim, but it seems like no matter how you look at it, debugging this stuff is a pretty bad experience.

I've found newfound respect for plugin developers. :)

sQVe commented 5 years ago

I noted yesterday when using prettier as a fixer that it actually has troubles inside nvim too. If I save but change something in the file before it fix was finished I get the following error:

Error detected while processing function <SNR>128_NeoVimCallback[29]..<SNR>143_HandleExit[41]..<SNR>143_RunFixer[57]..ale#fix#ApplyFixes:         
line   12:                                                                                                                                        
The file was changed before fixing finished 
Utagai commented 5 years ago

Yup, not surprised, since if my hunch is right, this should appear anytime the fixer runs and nvim decides to switch stuff up. This does mean that the problem is more complicated than I hoped, and if the file is changing even during mid-execution, that is actually pretty bad, because we have no guarantees anymore.

I haven't looked at this yet since my last look, so sorry to anyone following this. :)

On a more serious note, if this is a critical issue for anyone (it is not a big deal for me), then do not wait on me. I will have no problem if someone else decides to take this on before me. :)

Utagai commented 5 years ago

Hi all. I just gave this a pretty hard look for quite some time, and have some findings.

I believe I have confirmed that the temporary file that ale needs is being created, but then deleted before it can be used. I had a script watching a personal $TMPDIR and noticed that the path ale is assuming exists comes into being and then is deleted sometime later. The fact that we can't open the file at exit and it doesn't seem to exist anymore should mean that our hunch was right.

Secondly, I've found some places in the codebase where we seem to be deleting directories/files, but these are not the culprit, because even disabling them does not solve the issue. In order to completely rule out ale, I'd like to know if @w0rp knows of any other places in the codebase where ale may be responsible for deleting tempnames or directories that it creates. My guess is there is not, but I've only ever looked for instances of call delete(...).

Moving on, I also realized that this problem seems to exists in vim too, but the difference being that vim is silent about it. In other words, in neovim, you get an error and fix_on_save does not work, therefore, your source file isn't fixed. In vim your source file is also not fixed, you just don't get the error. Therefore, issue #2014 may actually be under the same umbrella as this issue. @RyanSquared @danielepiccone.

Finally, if there is indeed no other cases where ale deletes its tempnames or directories, then the culprit is probably neovim deleting these temporary directories/files too soon. If this is the case, which I imagine it is, then we need to find a way to avoid vim/neovim's handling of temporary files. Some ideas that came to mind:

  1. Do this temporary file stuff on our own. This is actually an idea that was entertained by git-gutter for a separate issue of theirs, but is obviously not as pretty.
  2. Seeing if it is possible to implement ale_fix_on_save in a way that does not rely on temporary files holding the buffer and instead run it as a straight command, perhaps via system(...) where we run the fixer on the source file, but I'm not sure if this comes with other limitations or issues.
  3. Follow the approach of vim-go. vim-go seems to use temporary files, but implements its 'fix on save' equivalent feature using a BufWritePre rule.

Any thoughts, critique, etc. are welcome. Needless to say: I'm taking a break from looking at Vimscript for a bit :).

w0rp commented 5 years ago

Thanks for all of the information. I wouldn't want to manage creating temporary files in some other way than using tempname, because Vim manages deleting those files after Vim exits, and we can't run plugin code after Vim exits. It might be easier to just make it so you can't fix files on :wq.

Utagai commented 5 years ago

@w0rp That's fine, and like I said, its not a very pretty idea. I'd rather avoid it as well.

What are your thoughts on the other ideas? My guess is that both are not satisfactory for ale, since you haven't said anything about them, but why? I feel like at least the third option is pretty intuitive and the way most people would have implemented something like this in vim if they didn't have ale, and is clearly an approach that at least one other plugin has deemed acceptable (and has made clear is possible).

w0rp commented 5 years ago

BufWritePre won't help because the execution of the commands is non-blocking.

I changed the code so ALE won't try to fix files on :wq any more. I think it just can't work. I updated the documentation to mention that you should save your file, look at the changes if they are any, and then quit the buffer instead.

sQVe commented 5 years ago

@w0rp This will not solve the case where the file had changed before the fixer is finished though.

w0rp commented 5 years ago

That should continue to be an error. The message is there to tell you something else modified the buffer, so ALE can't safely continue modifying the file.

Utagai commented 5 years ago

@w0rp The non-blocking thing is indeed a little unfortunate, but I actually personally have no issue with it. In either case, it isn't too hard to write my own BufWritePre so this is fine with me. Thanks!

laoshaw commented 4 years ago

I am seeing the same message when editing js files:

let arr=[] my cursor is in the middle of the bracket, I hit enter key in insert mode(vim8.1) and I saw the red error message before it disappears quickly. if I put anything inside [] before hit enter key it works fine, it came up each time when there is a blank [], {} followed by enter key. For () etc there is no issue.

pokir commented 2 years ago

I got the same error in nvim v0.7.2

My work around was to put this in my .vimrc:

let g:ale_fix_on_save = 1
cnoreabbrev wq autocmd User ALEFixPost q<CR>:w

However the second command has some other side effects (for example it will replace :sleep wq with :sleep autocmd User ALEFixPost q<CR>:w).