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

Fix E523 on asynchronous truncated echo with getchar() #1987

Merged
merged 1 commit into from
Oct 11, 2018

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Oct 10, 2018

Problem

I have a mapping in my vimrc as follows:

nnoremap <silent><expr>m 'i'.nr2char(getchar())."\<Esc>"

The simple mapping gets one character from user with getchar() and put it to buffer. However, it seems that, when using this mapping at the same timing as ALE tries to echo linter warning, it sometimes causes an error as follows:

Error detected while processing function <SNR>184_VimCloseCallback[11]..<SNR>184_VimExitCallback[14]..<SNR>179_HandleExit[50]..ale#engine#Han
dleLoclist[33]..ale#engine#SetResults[26]..ale#cursor#EchoCursorWarning[22]..ale#cursor#TruncatedEcho:
line   15:
E523: Not allowed here
Error detected while processing function <SNR>184_VimCloseCallback:
line   11:
E171: Missing :endif

The similar issue was reported to my clever-f.vim plugin.

rhysd/clever-f.vim#38

Investigation

I did not know when E523 occurs. So I dug Vim source code and found the line causing the error:

https://github.com/vim/vim/blob/b9464821901623f983528acaed9e4dc2cea7387b/src/normal.c#L799

This is a part of :normal command implementation. :normal command shows the error message (text_locked_msg() does) when it is executed while editing cmdline. (getchar() gets user's input via cmdline)

This was usually not a problem since Vim script is a synchronous language. :normal command was executed from insert mode or normal mode generally. While getting user input with getchar(), Vim script could do nothing since it's blocking.

However, situation was changed by asynchronous features such as job or channel. Asynchronous callback is run at arbitrary timing, even while blocking operation such as calling getchar().

So what happens would be:

  1. ALE automatically runs some linters and finds some warnings
  2. User does something which runs getchar()
  3. getchar() waits user input with blocking
  4. ALE outputs warning to command line via ale#cursor#TruncatedEcho()
  5. The function executes :normal and causes E523

Fix

It seems that only ale#cursor#TruncatedEcho causes this issue. And I could not got why TruncatedEcho uses :normal here. It seems that removing normal (Does the last \n do something?). Executing echomsg directly looks working on my environment.
And after this fix, I haven't seen the E523 error any more.

@w0rp
Copy link
Member

w0rp commented Oct 10, 2018

I can't quite explain why :norm! is used here, but I believe it has something to do with messages not being truncated unless that is used in some circumstances. I think we could just try removing it again, and see if anyone hits any issues. It would be good to not have to use it, if there's no good reason for it.

@@ -26,7 +26,7 @@ function! ale#cursor#TruncatedEcho(original_message) abort

" The message is truncated and saved to the history.
setlocal shortmess+=T
exec "norm! :echomsg l:message\n"
echomsg l:message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write execute 'echomsg l:message' here instead. One of the things the script tests for is people leaving stray echo lines lying around, and you can silence the warning by doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll update. Thank you for explanation.

Copy link
Contributor Author

@rhysd rhysd Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fully understanding why the trailing \n is added. If execute "echomsg l:message\n" is better, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference:

Quote: https://vim.fandom.com/wiki/Get_shortened_messages_from_using_echomsg

The second step is tricky. :echo does not shorten long messages. :echomsg, by itself, does not shorten long messages. Only when :echomsg is used under :norm, are long messages truncated. And don't forget trailing \n when using :exe norm.

Removing this has resulted in many people complaining about "Press ENTER" messages on long diagnostic messages: #4141

@rhysd
Copy link
Contributor Author

rhysd commented Oct 11, 2018

Thank you for your review. I addressed it at 577241e.

I think we could just try removing it again, and see if anyone hits any issues. It would be good to not have to use it, if there's no good reason for it.

Sure. Please feel free to revert this commit if some issue occurs.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 11, 2018

Hmm... Workers on Travis CI seem not working:

Status: Downloaded newer image for w0rp/ale:latest
Starting Vim: vim-v8.0.0027...



No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 11, 2018

It seems that this change breaks CI (reverting 577241e at 2cc2dac fixed CI) but I could not why CI failed due to no error message.

@rhysd rhysd force-pushed the fix-truncated-echo branch 2 times, most recently from 51dd007 to 577241e Compare October 11, 2018 07:08
@rhysd
Copy link
Contributor Author

rhysd commented Oct 11, 2018

I understood that :normal is used for canceling command if the command (:echomsg) does not finish.
From help:

If {commands} does not finish a command, the last one
will be aborted as if <Esc> or <C-C> was typed.
This implies that an insert command must be completed

I guess, some unit test hits -- More -- because message is too long and Vim stops due to that.

@rhysd rhysd force-pushed the fix-truncated-echo branch 2 times, most recently from 1345f07 to 952a15f Compare October 11, 2018 07:56
@rhysd
Copy link
Contributor Author

rhysd commented Oct 11, 2018

I completely understood what happens on exec "norm! :echomsg l:message\n".

echomsg l:message hits prompt and prevents user input when l:message is longer than window width. So :normal! is used since :normal! terminates given command with <C-c> if it does not complete. However, normal! :echomsg l:message does not work (it outputs nothing). To avoid this, we need to enter <CR> at last. To do this, execute "norm! :echomsg l:message\n" is necessary.

@w0rp
Copy link
Member

w0rp commented Oct 11, 2018

Aha, I knew there was some reason for it, but I forgot what it was. Maybe the safest thing to do is just catch E523 with execute "norm! :echomsg l:message\n" and skip echoing a message if that error occurs. Manually truncating the text is difficult because there's no way to accurately measure how wide text will be, given different fonts.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 11, 2018

I tried to change the implementation of TruncatedEcho() to truncate the message without shortmess option at 17b02c7. How about this?

unite.vim does the similar thing: https://github.com/Shougo/unite.vim/blob/e68ccde4192b3faa3717897e65dc7dd8ab87524a/autoload/unite/view.vim#L992-L1017

@w0rp
Copy link
Member

w0rp commented Oct 11, 2018

I'm not sure. I trust Vim to truncate messages accurately, but not any Vim script code. I prefer to avoid writing any code for truncating characters if possible.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 11, 2018

I agree with you. I also think using shortmess is better.

How about catching error? I think it's better than showing error message.

try
    exec "norm! :echomsg l:message\n"
catch /^Vim\%((\a\+)\)\=:E523/
    " Should we fallback into manual truncate?
endtry

@w0rp
Copy link
Member

w0rp commented Oct 11, 2018

Yeah, that's what I was suggesting. I think that's a much safer option. The error will be hidden in the minority of cases where it happens, and we know it won't break anything for anyone. I suppose we could fall back to manually truncating the text there too. Then a fallback that will work in the majority of cases will only be tried in a small minority of cases.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 11, 2018

I fixed my patch at 8272eea. Could you review again?

@rhysd rhysd force-pushed the fix-truncated-echo branch 2 times, most recently from 17aa436 to 8272eea Compare October 11, 2018 10:54
@w0rp
Copy link
Member

w0rp commented Oct 11, 2018

Yep, that'll work. Cheers! 🍻

@w0rp w0rp merged commit 8798652 into dense-analysis:master Oct 11, 2018
w0rp pushed a commit that referenced this pull request Oct 11, 2018
wookayin added a commit to wookayin/ale that referenced this pull request Jul 12, 2019
When a linting message is echoed in the command line, mode change was
happening due to the use of ':' (see :help <Cmd>), triggering autocmd
events such as CmdlineEnter and CmdlineLeave.

To avoid that, we can use <Cmd> instead of ':' to avoid mode change.

Reference: dense-analysis#1987
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

Successfully merging this pull request may close these issues.

3 participants