"au FileType dirvish windo" raises E121, E95

[lacygoill]

Describe the bug

Installing an autocmd listening to FileType dirvish and which executes a :windo command may raise E121 and E95.

To Reproduce

Write this in /tmp/vimrc:

set rtp-=~/.vim
set rtp-=~/.vim/after
set rtp^=~/.vim/plugged/vim-dirvish
au FileType dirvish windo "

You may need to adapt the path to vim-dirvish if you don't use vim-plug.

Start Vim with this shell command:

vim -Nu /tmp/vimrc

Execute this Ex command:

:sp | Dirvish

E121 and E95 are raised:

Error detected while processing function dirvish#open[39]..<SNR>16_open_dir:
line   47:
E121: Undefined variable: b:dirvish
line   25:
E95: Buffer with this name already exists

Expected behavior

No error is raised.

Environment

• Dirvish version: 9c12328df924ddb875ee1e5c9fc0f939b62d6a6c
• Vim version: 8.2 Included patches: 1-100
• OS: Ubuntu 16.04.6 LTS
• Terminal: XTerm(322)

Additional context

It seems any :windo command can reproduce the issue (:windo let var = 123, :windo echo 'test', ...).

---

As a workaround, :windo can be replaced by a combination of map() and win_execute():

au FileType dirvish call map(range(1, winnr('$')), {_,v -> win_execute(win_getid(v), 'echom printf("command run in window %d", winnr())', '')})

[lacygoill]

Since there exists a workaround, I close the issue. I just thought it was worth reporting it, in case one day someone else is affected.

[justinmk]

I think this is worth fixing.

[lacygoill]

The issue comes from this line: https://github.com/justinmk/vim-dirvish/blob/0a53fadc22ab6df6aed9cc580c9e498715870522/autoload/dirvish.vim#L459

Right before, b:dirvish exists; right after it doesn't exist anymore. This is because the current window and buffer have changed after :windo " has been run. You can check by adding a breakpoint in dirvish#open:

" in /tmp/vimrc
set rtp-=~/.vim
set rtp-=~/.vim/after
set rtp^=~/.vim/plugged/vim-dirvish
au FileType dirvish noa windo "

# start Vim with this shell command
vim -Nu /tmp/vimrc +'breakadd func dirvish#open' +'sp|Dirvish'

function dirvish#open
line 1: if &autochdir
>f
function <SNR>15_on_bufenter[4]..dirvish#open
line 1: if &autochdir
>n
...
Enter 19 times
...
function <SNR>15_on_bufenter[4]..dirvish#open
line 39: call s:open_dir(d, reloading)
>s
function <SNR>15_on_bufenter[4]..dirvish#open[39]..<SNR>15_open_dir
line 1: let d = a:d
>n
...
Enter 27 times
...
function <SNR>15_on_bufenter[4]..dirvish#open[39]..<SNR>15_open_dir
line 49: setlocal filetype=dirvish
>echo [winnr(), bufnr('')]
[1, 1]
>n
>echo [winnr(), bufnr('')]
[2, 2]

Now that I re-read the issue, I'm thinking that such an error is the user's fault, not dirvish's fault. FileType is meant to customize a buffer whose filetype has been set; it works under the assumption that the buffer for which the filetype was detected is the same as the buffer in which the autocmds are run. If one of your FileType autocmd changes the current window/buffer, I think it breaks this assumption for all the subsequent FileType autocmds. So there's no guarantee that any of them work as expected anymore.

If a user installs an autocmd running :windo on FileType, I think they should make sure the current window is restored immediately afterward.

In any case, this patch should fix the issue:

diff --git a/autoload/dirvish.vim b/autoload/dirvish.vim
index d600e36..85e5b03 100644
--- a/autoload/dirvish.vim
+++ b/autoload/dirvish.vim
@@ -456,7 +456,9 @@ function! s:open_dir(d, reload) abort
     call s:buf_render(b:dirvish._dir, get(b:dirvish, 'lastpath', ''))
     " Set up Dirvish before any other `FileType dirvish` handler.
     exe 'source '.fnameescape(s:srcdir.'/ftplugin/dirvish.vim')
+    let curwin = winnr()
     setlocal filetype=dirvish
+    exe s:noau curwin.'wincmd w'
     let b:dirvish._c = b:changedtick
     call s:apply_icons()
   endif

I can send a PR if you want, but I'm not sure it's worth fixing the issue. It's not even 100% reliable; e.g. if you use :tabdo windo, you'll probably need to invoke win_getid() and win_gotoid(), but those were only added in 7.4.1557 while the README mentions that the plugin supports Vim 7.2. So, you would need to also save the current tab:

diff --git a/autoload/dirvish.vim b/autoload/dirvish.vim
index d600e36..60e66d6 100644
--- a/autoload/dirvish.vim
+++ b/autoload/dirvish.vim
@@ -456,7 +456,10 @@ function! s:open_dir(d, reload) abort
     call s:buf_render(b:dirvish._dir, get(b:dirvish, 'lastpath', ''))
     " Set up Dirvish before any other `FileType dirvish` handler.
     exe 'source '.fnameescape(s:srcdir.'/ftplugin/dirvish.vim')
+    let [curtab, curwin] = [tabpagenr(), winnr()]
     setlocal filetype=dirvish
+    exe s:noau 'tabnext' curtab '|' s:noau curwin.'wincmd w'
     let b:dirvish._c = b:changedtick
     call s:apply_icons()
   endif

And what if the user installs some weird FileType autocmd which loads a different buffer? You would also need to save/restore the current buffer around the setlocal filetype=dirvish line...

[lacygoill]

Now that I re-read the issue, I'm thinking that such an error is the user's fault, not dirvish's fault.

What I mean is that the user should never write something like this:

au FileType foo windo ...

But this:

au FileType foo let curwin = win_getid() | exe 'windo ...' | call win_gotoid(curwin)

[lacygoill]

Actually, it's not even allowed to focus a different window when FileType is fired. From :h FileType:

Navigating to another window or buffer is not allowed.

Not sure why Vim allows :windo on FileType in general then.

[justinmk]

Thanks, that is super helpful. Perhaps an assertion after setlocal filetype=dirvish would at least help avoid confusion?

diff --git a/autoload/dirvish.vim b/autoload/dirvish.vim
index d600e36..85e5b03 100644
--- a/autoload/dirvish.vim
+++ b/autoload/dirvish.vim
@@ -456,7 +456,9 @@ function! s:open_dir(d, reload) abort
     call s:buf_render(b:dirvish._dir, get(b:dirvish, 'lastpath', ''))
     " Set up Dirvish before any other `FileType dirvish` handler.
     exe 'source '.fnameescape(s:srcdir.'/ftplugin/dirvish.vim')
+    let curwin = winnr()
     setlocal filetype=dirvish
+    if curwin != winnr() | throw 'FileType autocmd changed the window' | endif
     let b:dirvish._c = b:changedtick
     call s:apply_icons()
   endif