Fix breakage w/ plugins that inadvertently trigger ALE in `execute()` #3719

habibalamin · 2021-05-09T23:18:04Z

Vim does not allow the use of :redir from within execute() calls, no matter how deeply within the call chain it occurs. There are sadly plugins that call execute() for whatever reasons, and inadvertently trigger ALE functions that use :redir, thereby raising an E930:

E930: Cannot use :redir inside execute()

We replace all uses of :redir => var with let var = execute(…) to resolve this.

I've been trying to narrow down a minimal test case, and it's turned into a bit of a rabbit hole, as LanguageClient-neovim seems to suppress errors being displayed to the user when they use Solargraph (a Ruby language server), but not when they use Haskell Language Server. See autozimu/LanguageClient-neovim#1223 for the issue describing this.

That said, you can try this with an empty Haskell file using this vimrc:

" vimrc

let g:LanguageClient_showCompletionDocs = 1

call plug#begin('~/.vim/plugged')
Plug 'dense-analysis/ale'
Plug 'autozimu/LanguageClient-neovim', {
  \ 'branch': 'next',
  \ 'do': './install.sh'
\ }
let g:LanguageClient_serverCommands = {}
if executable('haskell-language-server-wrapper')
  let g:LanguageClient_serverCommands['haskell'] =
    \ ['haskell-language-server-wrapper', '--lsp']
endif
call plug#end()

Just type Prelude., then press C-x C-o. Make sure to run all this in an empty folder for best reproducibility. Also, wait a few seconds after opening the Haskell file before attempting completion to let the language server launch. It seems if you try to complete before the language server has launched, even after it's launched, a second completion attempt won't work (possibly a bug in itself).

If you want to reproduce this without another plugin, you can use vim -Nu <(echo "set rtp+=path/to/ale")

and just run these commands in order:

let pop_win_id = popup_create(['# Title', '', 'Body'], { 'line': 0, 'col': 0, 'padding': [2, 2, 2, 2], 'moved': 'any' })
call setbufvar(winbufnr(pop_win_id), '&filetype', 'markdown')
call win_execute(pop_win_id, 'doautocmd InsertLeave')

You should see the first time:

Error detected while processing InsertLeave Autocommands for "*"..function ale#Queue[33]..<SNR>16_Lint[20]..ale#engine#RunLinters[4]..<SNR>32_GetLintFileValues[27]..<lambda>4[1]..<SNR>32_RunLinters[18]..<SNR>32_RunLinter[6]..<SNR>32_RunIfExecutable[43]..<SNR>32_RunJob[27]..ale#command#Run[17]..ale#command#FormatCommand[40]..<SNR>33_TemporaryFilename[6]..ale#filetypes#GuessExtension[1]..<SNR>35_GetCachedExtensionMap[2]..ale#filetypes#LoadExtensionMap:
line    3:
E930: Cannot use :redir inside execute()
Error detected while processing InsertLeave Autocommands for "*"..function ale#Queue[33]..<SNR>16_Lint[20]..ale#engine#RunLinters[4]..<SNR>32_GetLintFileValues[27]..<lambda>4[1]..<SNR>32_RunLinters[18]..<SNR>32_RunLinter[6]..<SNR>32_RunIfExecutable[43]..<SNR>32_RunJob[27]..ale#command#Run:
line   17:
E714: List required

Every subsequent time before restarting Vim (which I believe is because the extension map is no longer a blank hash, so it's using the now cached corrupt value (another bug)):

Error detected while processing InsertLeave Autocommands for "*"..function ale#Queue[33]..<SNR>16_Lint[20]..ale#engine#RunLinters[4]..<SNR>32_GetLintFileValues[27]..<lambda>6[1]..<SNR>32_RunLinters[18]..<SNR>32_RunLinter[6]..<SNR>32_RunIfExecutable[43]..<SNR>32_RunJob[27]..ale#command#Run[17]..ale#command#FormatCommand[40]..<SNR>33_TemporaryFilename[6]..ale#filetypes#GuessExtension:
line    2:
E896: Argument of get() must be a List, Dictionary or Blob
Error detected while processing InsertLeave Autocommands for "*"..function ale#Queue[33]..<SNR>16_Lint[20]..ale#engine#RunLinters[4]..<SNR>32_GetLintFileValues[27]..<lambda>6[1]..<SNR>32_RunLinters[18]..<SNR>32_RunLinter[6]..<SNR>32_RunIfExecutable[43]..<SNR>32_RunJob[27]..ale#command#Run:
line   17:
E714: List required
Press ENTER or type command to continue

I've tried creating a Vader test of the second method of reproduction (both with Do and Execute), but I can't seem to reproduce it in a Vader test. I thought I did a few days ago, but now I'm not so sure. I had to use Do, because for some reason, it seemed as if I had to be in insert mode to do the third line to prevent the popup window disappearing after I pressed colon, and it also seemed as if the second line wasn't necessary.

Now, I've been no longer able to reproduce without the second line, and being in insert mode doesn't seem to matter, both with the minimal vimrcs and my usual vimrc. This has been an exhausting bunch of bugs to track down and deal with (see linked issue for full details), so I'm not down for any more, but if a Vader test is really necessary for this PR, I'll keep trying, but I can't promise anything.


---

According to @hsanson, the use of `:redir`, in at least the case of the linked PR, may not be an arbitrary decision:

>@w0rp I do not think the use of redir in this part of the code was an arbitrary decision. Would prefer that you review this MR and #3648 before merging them.
— https://github.com/dense-analysis/ale/pull/3691#issuecomment-818671627

How much stock should be put in that statement is not clear, as Vim's documentation says of `execute()`:

>execute({command} [, {silent}])					*execute()*
>
>		[…]
>
>		This is equivalent to: >
>			redir => var
>			{command}
>			redir END

Related PR: #3691

Vim does not allow the use of `:redir` from within `execute()` calls, no matter how deeply within the call chain. There are, sadly, plugins which call `execute()` for whatever reason and inadvertently trigger functions that use `:redir`, thereby raising an `E930`: >E930: Cannot use :redir inside execute() We replace all uses of `:redir => var` with `let var = execute(…)`, that resolves this. --- In `ale#sign#SetUpDefaultColumnWithoutErrorsHighlight()`, the use of the `0verbose` prefix had to be removed, as `execute()` doesn't accept it in its second argument. In fact: >execute({command} [, {silent}]) *execute()* > >[…] > > The optional {silent} argument can have these values: > "" no `:silent` used > "silent" `:silent` used > "silent!" `:silent!` used Given that `verbose` is already set to `0` on the previous line, the use of `0verbose` seems unnecessary. >function! ale#sign#SetUpDefaultColumnWithoutErrorsHighlight() abort > let l:verbose = &verbose > set verbose=0 > redir => l:output > 0verbose silent highlight SignColumn > redir end > let l:output = execute('highlight SignColumn', 'silent') > let &verbose = l:verbose >[…] The history of this part of the code shows that the `0verbose` was added first, then the let-verbose-set-verbose-let-verbose dance. I believe the `0verbose`, after that part was added, became unnecessary, but it wasn't removed. The commit which added the dance is bafe1c0, and the `0verbose` was added in d3ed1e5. Vim docs on `:[count]verbose` say: >:[count]verb[ose] {command} > Execute {command} with 'verbose' set to [count]. If > [count] is omitted one is used. ":0verbose" can be > used to set 'verbose' to zero.

habibalamin · 2021-05-15T01:17:15Z

@w0rp not sure what's up with the AppVeyor failure, is there a way to see why it failed?

habibalamin · 2021-05-16T17:38:41Z

There is a bit of a problem:

 function! ale#sign#SetUpDefaultColumnWithoutErrorsHighlight() abort
      let l:verbose = &verbose
      set verbose=0
-     redir => l:output
-         0verbose silent highlight SignColumn
-     redir end
+     let l:output = execute('highlight SignColumn', '0verbose silent')
      let &verbose = l:verbose

This diff is wrong, because execute() doesn't accept 0verbose in its second argument. In fact:

execute({command} [, {silent}])					*execute()*

[…]

		The optional {silent} argument can have these values:
			""		no `:silent` used
			"silent"	`:silent` used
			"silent!"	`:silent!` used

Given that verbose is already set to 0 on the previous line (set verbose=0), the use of 0verbose seems unnecessary. The history of this part of the code shows that the 0verbose was added first, then the let-verbose-set-verbose-let-verbose dance. I think after that was added, the 0verbose prefix became unnecessary but wasn't removed.

Vim docs on :[count]verbose say:

:[count]verb[ose] {command}
			Execute {command} with 'verbose' set to [count].  If
			[count] is omitted one is used. ":0verbose" can be
			used to set 'verbose' to zero.

habibalamin · 2021-05-16T19:43:44Z

I've squashed the bugfix commit into the first one, might as well not commit something which breaks the test suite, even if it's not in master directly.

hsanson · 2021-05-23T13:02:23Z

MR #3648 should fix this issue, if it does this MR can be closed.

habibalamin · 2021-05-23T15:43:07Z

Thanks @hsanson. I'd already tried the linked PR before making this one.

It didn't work for me, which is not surprising, as it's not changing the function where my debugging lead to for my issue. The particular issue that I was facing which prompted the PR was with ale#filetypes#LoadExtensionMap(), so the linked PR doesn't help me at all.

However, this PR should fix all issues of incompatibilities with other plugins that are due to :redir being used within execute() family functions.

autoload/ale/sign.vim

w0rp

This is sensible and will fix an edge case.

w0rp · 2021-06-19T10:56:52Z

Cheers! 🍻

habibalamin mentioned this pull request May 15, 2021

LanguageClient-neovim treats errors differently depending on the language server autozimu/LanguageClient-neovim#1223

Open

habibalamin marked this pull request as ready for review May 15, 2021 00:52

w0rp reviewed Jun 19, 2021

View reviewed changes

autoload/ale/sign.vim Outdated Show resolved Hide resolved

Fix line continuation indentation

845bfec

w0rp approved these changes Jun 19, 2021

View reviewed changes

w0rp merged commit ad27e92 into dense-analysis:master Jun 19, 2021

w0rp added a commit that referenced this pull request Jun 19, 2021

#3719 - Prevent redir being used again in future

a566a5d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix breakage w/ plugins that inadvertently trigger ALE in `execute()` #3719

Fix breakage w/ plugins that inadvertently trigger ALE in `execute()` #3719

habibalamin commented May 9, 2021 •

edited

habibalamin commented May 15, 2021

habibalamin commented May 16, 2021

habibalamin commented May 16, 2021 •

edited

hsanson commented May 23, 2021

habibalamin commented May 23, 2021

w0rp left a comment

w0rp commented Jun 19, 2021

Fix breakage w/ plugins that inadvertently trigger ALE in execute() #3719

Fix breakage w/ plugins that inadvertently trigger ALE in execute() #3719

Conversation

habibalamin commented May 9, 2021 • edited

habibalamin commented May 15, 2021

habibalamin commented May 16, 2021

habibalamin commented May 16, 2021 • edited

hsanson commented May 23, 2021

habibalamin commented May 23, 2021

w0rp left a comment

Choose a reason for hiding this comment

w0rp commented Jun 19, 2021

Fix breakage w/ plugins that inadvertently trigger ALE in `execute()` #3719

Fix breakage w/ plugins that inadvertently trigger ALE in `execute()` #3719

habibalamin commented May 9, 2021 •

edited

habibalamin commented May 16, 2021 •

edited