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

Automatic execution of generic fixers always sets v:errmsg #3021

Closed
lifecrisis opened this issue Feb 26, 2020 · 2 comments
Closed

Automatic execution of generic fixers always sets v:errmsg #3021

lifecrisis opened this issue Feb 26, 2020 · 2 comments
Labels

Comments

@lifecrisis
Copy link
Contributor

lifecrisis commented Feb 26, 2020

The following minimal configuration is necessary to reproduce this:

let g:ale_fixers = {}
let g:ale_fixers['vim'] = ['trim_whitespace']
let g:ale_fix_on_save = 1

To reproduce the problem:

  1. vim foo.vim
  2. :echo v:errmsg (you should see nothing)
  3. :write
  4. :echo v:errmsg

As you can see, ALE pollutes this variable unnecessarily.

The problem is due to this function from autoload/ale/util.vim:

function! s:LoadArgCount(function) abort
    let l:Function = a:function

    redir => l:output
        silent! function Function
    redir END

    if !exists('l:output')
        return 0
    endif

    let l:match = matchstr(split(l:output, "\n")[0], '\v\([^)]+\)')[1:-2]
    let l:arg_list = filter(split(l:match, ', '), 'v:val isnot# ''...''')

    return len(l:arg_list)
endfunction

The problem is that :silent! still sets v:errmsg.

I recommend that this function be changed to:

function! s:LoadArgCount(function) abort
    try
        let l:output = execute('function a:function')
    catch /E123/
        return 0
    endtry

    let l:match = matchstr(split(l:output, "\n")[0], '\v\([^)]+\)')[1:-2]
    let l:arg_list = filter(split(l:match, ', '), 'v:val isnot# ''...''')

    return len(l:arg_list)
endfunction

Notes...

  • I removed an unnecessary local variable (l:Function).
  • Also, the original if statement is useless, so it can go too (:redir always creates the variable, so it will always exist).
  • Using :redir is passé. I think that execute() should be preferred here.

Let me know if you want a PR, and I'll submit one.

@lifecrisis lifecrisis added the bug label Feb 26, 2020
lifecrisis added a commit to lifecrisis/ale that referenced this issue Mar 4, 2020
Previously, this function would always set "v:errmsg" on the first
call with a given function.  This is because autoloaded functions
are not defined on the first call.

A number of improvements have been made:
 - a useless local function ("l:Function") is removed
 - the "execute()" builtin captures the output, instead of ":redir"
 - a ":try" block handles the case where a function is not defined
 - a useless ":if" is removed since ":redir" always defines the var
 - confusing quoting is re-written (remove double "'" chars)

Fixes: dense-analysis#3021
@lifecrisis
Copy link
Contributor Author

This is such a simple fix... I went ahead and took the liberty of submitting a pull request.

@w0rp w0rp closed this as completed in 8f7ccdc Mar 4, 2020
@w0rp
Copy link
Member

w0rp commented Mar 4, 2020

I just so happened to notice this in IRC, so I merged this. Thanks for pointing that out. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants