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

Allow to pass lua closures to vim script functions from lua #6246

Closed
wants to merge 27 commits into from

Conversation

prabirshrestha
Copy link

@prabirshrestha prabirshrestha commented Jun 12, 2020

This is work in progress but lays the foundation to pass lua closures to a vim function in pure lua. Over the next couple of days will be working on the remaining tasks but please feel free to review and provide feedback.

Here is a working demo:

image

This introduces register_cfunc api in c which takes a c callback and state and converts it to a vimscript lambda which can then be called by any vimscript.

Here are remaining work:

  • add tests
  • update documentation with example
  • update register_cfunc to take a dectructor c callback so when the lambda gets freed it can go ahead and do cleanup such as calling lua_unref() and calling free(state).
  • update callback to return int such as FCERR_NONE instead of void.
  • convert vim args to lua args before calling lua closure
  • convert lua return values to vim return value.

Based on looking at neovim source code it seems to treat lua functions as first class so register_cfunc doesn't exist in neovim.

@prabirshrestha
Copy link
Author

@brammool This is now ready for review and merge. I have added tests but not sure why all tests are passing for all builds except huge+asan/gcc. https://travis-ci.org/github/vim/vim/builds/697910811.

This now allows us to seamlessly pass lua callback which will be converted to vim script lambda. Writing lua plugins should be lot easier now.

lua <<EOF
    vim.fn.timer_start(1000, function (timer)
        print(timer)
    end)
EOF

register_cfunc does most of the work. It takes a callback which is called whenever lambda needs to callback to lua. 3nd arg is the state which is of type void* allowing anyone to store any state they want. 2nd arg is a destructor which can be used to cleanup once the lambda get collected by gc. Ideally this function could also be used for future conversions in python, ruby or other scenarios too.

char_u *register_cfunc(cfunc_T cb, cfunc_free_T free_cb, void *state);

luaV_call_lua_func does the actual translation of vim typval args to lua type args and once the lua function is executed it converts the lua return value back to vim type .

Test includes all the scenarios i.e. passing return value of lua function to vimscript, calling lua function from vimscript function.

@prabirshrestha
Copy link
Author

Issue seems to be here. Any thought on how to fix this?

==28363==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 176 byte(s) in 2 object(s) allocated from:
    #0 0x7f2d38a6eb40 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb40)
    #1 0x559fb93dae1d in lalloc /home/travis/build/vim/vim/src/misc2.c:925
    #2 0x559fb93daf9d in alloc_clear /home/travis/build/vim/vim/src/misc2.c:852
    #3 0x559fb9686f17 in register_cfunc /home/travis/build/vim/vim/src/userfunc.c:360
    #4 0x559fb976db68 in luaV_totypval /home/travis/build/vim/vim/src/if_lua.c:607
    #5 0x559fb9770328 in luaV_funcref_call /home/travis/build/vim/vim/src/if_lua.c:1261
    #0 0x7f2d35d5128b in lua_getinfo ??:?
    #1 0x7f2d35d5128b in ?? ??:0

@prabirshrestha prabirshrestha force-pushed the lua-to-funcref branch 2 times, most recently from 290bb2f to d2774ff Compare June 17, 2020 04:24
@prabirshrestha
Copy link
Author

Memory leak is fixed so the PR is ready for review and merge.

There is one test that seems to be failing which also is failing in master and is not related to this PR.

Found errors in Test_import_fails_without_script():

	Caught exception in Test_import_fails_without_script(): Vim(call):E117: Unknown function: RunVimInTerminal @ function RunTheTest[39]..Test_import_fails_without_script, line 12

Also might be worth moving the CI to use github actions instead of travis and appveyor. For vim-lsp we migrated to github actions and it was way faster then travis. This last PR tooks travis almost 15-20mins to even just start.

@brammool
Copy link
Contributor

I would like to hear from users who know Lua and would use this functionality. Comments?

@prabirshrestha
Copy link
Author

My current use case was to rewrite asyncomplete v3 in both vimscript (for older version and backwards compatibility) and lua (for performance). I need to use timers and jobs to pass functions as well as pass the autocomplete callback functions around hence the main reason behind this PR. My first comment in the PR included timer examples as I need it to debounce on keystrokes. There are also some parts of vim-lsp I would like to write in lua for perf reasons. There are also other plugins I would like to write in pure lua that uses jobs. It is also lot easier to work with lua closures that is multiline and contains if else than vimscript.

I have tried several times in the past few years to write lua plugins but always landed up with vimscript because the integration was ugly since there is no good bindings. It felt that lua was more there just for the sake of it rather than creating real plugins. Given that I had so hard time creating lua plugins I definitely understand why there are very few to almost none lua plugins for vim.

Next version of neovim 0.5 is going to ship with LSP protocol by default which is written in lua and as part of that work they have already gone and added these apis so an entire full fledged powerful plugin can be created in lua. In the past few months I have also seen rise of neovim plugins written in pure lua and I expect this to continue to rise once v0.5 ships.

register_cfunc is generic enough to also work for python and ruby which would allow those languages to also extend if needed using this same mechanism. Else one would need to create global functions in vimscript and then use vim.funcref which isn't ideal.

@brammool
Copy link
Contributor

So this works exactly the same as in Neovim?

@prabirshrestha
Copy link
Author

I just tried in neovim latest nightly and doesn't seem to work but seems like it is just a matter of time since there is a PR by @tjdevries. neovim/neovim#12138. This PR seems to be implemented purely in lua but creates a temporary global function in lua starting with __VimlLuaAutoWrapper which can then be called by vimscript. One big difference I noticed in the neovim was that it overrwrite vim.fn and then converts the lua functions which means callback only works for vim.fn*. My PR goes a bit further by being able to convert lua functions to vim lambdas anywhere since it hooks into the lua to vimscript converter function. Based on the description of neovim PR also seems like it only partial solves and is a temporary workaround for now.

@justinmk @tjdevries It will be good if both of you can have a look at this PR and the existing neovim PR and see if we can lock to one of the solution so both vim and neovim uses same code.

@tjdevries
Copy link

@prabirshrestha I was actually following this PR already to see what you ended up with here, and was considering porting (or similiar feature) once I saw how you do it. When I first was writing fn_wrap, I wanted something that wouldn't take any C code. Now I see that it is not impossible to make this feature the correct way (I've written some more lua integration stuff on the neovim side).

My original PR was mainly just a starter idea for how we could at least let people write the code (just use fn_wrap) and then later do it the correct and more difficult way, with no changes from their perspective.

@prabirshrestha What exactly are you looking for from me? I can review the code here and figure out if this makes the most sense. I'd have to investigate some of the differences between vim's if_lua vs. neovim's lua implementation. Whichever way we do it, I would also like to make it so there is no difference in how you'd do it for either codebase in terms of writing plugins.

@brammool As an aside, this is a commonly requested feature when people have been writing all Lua plugins in neovim (which is a common occurence these days, especially after the LSP support was merged into master).

@prabirshrestha
Copy link
Author

@tjdevries Your explanation now makes more sense. I wasn't aware of your PR in neovim side. Was curious if you preferred fn_wrap or this current way with C changes. It was more about the approach rather than reviewing PR.

I do agree that while the implementation can be different the behavior should be the same. Which means the same code should be valid in both vim and neovim.

@tjdevries
Copy link

@prabirshrestha Yeah, I think your proposal is basically:

"Wherever you could pass a funcref in vim, you should be able to pass in a Lua function as well."

(where "wherever" means going through vim.fn? Are there any other places?)

If that's your proposal, that is a good thing for me. What I can do, assuming we agree on the semantics of what should happen there, is whatever tests you write for this PR, I can try porting to our testing infrastructure so that we can try and maintain as few differences as possible -- assuming I'm actually able to implement the feature in nvim 😉

@prabirshrestha
Copy link
Author

Other places besides vim.fn is return values that also needs to be converted.

function VimFunc(factory)
    let f = a:factory()
    return f()
endf
function LuaFactory()
    return function ()
        return 'hi'
    end
end

Basically luaV_totypval is called in to places that is for inputs and outputs of the function.

To sum up any time lua function passes boundries it needs to be auto converted to vim functions.

@tjdevries
Copy link

tjdevries commented Jun 18, 2020

Will you handle passing this and calling this from vimL?

local callable_table = setmetatable({}, {__call = function(...) return 5 end})

@prabirshrestha
Copy link
Author

I only check for case LUA_TFUNCTION:. But supporting __call should be simple as updating here to handle check if has _call.

case LUA_TUSERDATA:

@tjdevries
Copy link

OK, I would vote for supporting it (in the interest of completion). We do not have the file if_lua, but I think I have found the place to at least start the correct solution for nvim.

@tjdevries
Copy link

I made good progress on a branch for this today. I'll ping you (on this PR if still opened) when I get this far. I have to implement a few more things to bring the interface a little closer together (like funcrefs from vim -> lua) and a few other items.

@prabirshrestha
Copy link
Author

prabirshrestha commented Jun 19, 2020

@tjdevries That would be great. Feel free to share the PR. Also added support for __call too with tests.

@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #6246 into master will increase coverage by 0.00%.
The diff coverage is 86.04%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6246   +/-   ##
=======================================
  Coverage   87.78%   87.78%           
=======================================
  Files         143      143           
  Lines      158812   158898   +86     
=======================================
+ Hits       139411   139488   +77     
- Misses      19401    19410    +9     
Impacted Files Coverage Δ
src/userfunc.c 92.94% <83.33%> (-0.17%) ⬇️
src/if_lua.c 87.46% <88.00%> (+0.02%) ⬆️
src/netbeans.c 75.85% <0.00%> (-0.82%) ⬇️
src/testing.c 93.06% <0.00%> (-0.78%) ⬇️
src/if_xcmdsrv.c 88.38% <0.00%> (-0.53%) ⬇️
src/gui_gtk.c 31.05% <0.00%> (-0.28%) ⬇️
src/gui.c 63.92% <0.00%> (-0.06%) ⬇️
src/window.c 89.82% <0.00%> (-0.04%) ⬇️
src/terminal.c 91.01% <0.00%> (+0.03%) ⬆️
src/message.c 88.64% <0.00%> (+0.04%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7acde51...10ae6b0. Read the comment docs.

@prabirshrestha
Copy link
Author

@puremourning seems like I missed your message because you posted directly in google groups instead of github.

That's interesting. I don't use the lua interface, but YCM and Vimspector both make heavy use of the embedded python3, i have been thinking about exposing job and channel objects to the python layer. But it sounds like you've gone a different way here with lua. Would you mind elaborating on how that mechanism would work ?

I haven't looked at YCM source code due to GPL but Vimspector seems to be apache and is using python's subprocess command. Unlike python lua doesn't come with lot of standard libraries which means there is no official way to start a timer or start a process. On the other hand python will never ship with vim functions such as accessing windows/quickfix and so on. And trying to update language interop for every vim feature added is a nightmare on its own. Hence it was very important for me to use expose all the functions of vimscript to lua and vice versa. I also wanted to do it in a generic way so that next time someone adds a new feature to vimscript I don't need to go and update the lua interop. And this PR and vim.fn*` PR allows to achieve this goal. This was done by neovim and I ported it to vim. It is merged in both.

Are you saying you'd be able to do the equivalent of vim.eval ( 'job_start' .... ) and pass a Python function as the, say 'out_cb' (as opposed to a vimscript function which then does a ':py3' to call back into python ?

vim.eval was supported long time back but is an ugly way to deal with cross boundries between vimscript and lua as you need to convert to string and make sure you have quotes and concatenation right. In order to solve this Neovim added vim.call function so you can now use vim.call('has', 'timer'). First param is the function name and the rest of the params are args to that function. vim internally already implements call_vim_function in c which is almost same as vim.call so it makes it very easy to port this to lua. This allows to get all functions including future ones written in vim for free in the lua world. vim.fn is a fancier way to use vim.call in a more natural way and uses lua features to achieve this. Similar to javascript's proxies lua allows us to provide a custom getter when the key is accessed allowing us to intercept and provide custom function on the fly. https://www.lua.org/pil/13.4.4.html. You can use vim.fn.has('timer'). Since has doesn't exist in vim.fn it runs the custom getter and creates a function which internally calls vim.call('has', 'timer') but for the user it is similar to calling any other lua functions. Another advantage of using vim.fn and vim.call is that it will auto translate lua types to vim types and when the function returns value it automatically translates vim types to lua types unlike vim.eval. But if you want to use functions such as job and timers it doesn't auto translate functions since it was never implemented. This PR is the solution for it. You can now call vim functions as lua functions. This doesn't define a custom job function which means you are still responsible for vim.fn.has('nvim') and handling differences between vim and neovim.

local job_start = vim.fn.job_start

local jobid = job_start('whoami', {
    'out_cb' = function (id, msg)
        print(msg)
    end
})

register_cfunc works under similar principles as it is generic and is also designed to work with other languages. There was other alternatives but was lua specific.

char_u *register_cfunc(cfunc_T cb, cfunc_free_T free_cb, void *state);

register_cfunc creates a vim lambda function which means it can be used anywhere functions are supported. The return type is char_u which is the name of lambda. This is how vimscript also internally works. Every lambda you create in vimscript has a name. This is why funcref(lambdaname) also works. Internally lambda is a c struct with some properties and flags. So I created a new flag called FC_CFUNC which allows me to detect that this is a special type of lambda that is written in C. Then I added extra property called cb, cb_free and cb_state. VimScript doesn't know about this internal as it is never exposed. cb is the callback that you can write in c and is a function pointer. cb is where translation of args and return types as well as actual calling needs to happen. cb_free is also a callback that you can write in c and is a function pointer. This is a destructor that can be used to cleanup i.e. when lambda gets garbagecollected you also want to decrement the refcount in lua or free the state you created. Because C doesn't have closure support I need to have context when the cb and cb_free are called so that I can differentiate between lua function 1 and lua function 2. this is where the state comes in. it is a void * which means it can store a pointer to anything. So while looping through lua args and converting it to vim args I malloc a struct with the reference to lua function, notify lua that i have reference to this function so don't garbage collect it and call register_cfunc. Then I modified the call_vim_function to test for FC_CFUNC and call the c function which is of typedef int (*cfunc_T)(int argcount, typval_T *argvars, typval_T *rettv, void *state);. It is the same as how any other vimscript functions gets called except that I added extra state so the callback can now get reference to the actual lua function, convert the args from vimtype to lua type call the lua function convert back the lua return type to vim type and it seamlessly works. When the vimscript lambda gets garbagecollected it then calls the cb_free with the correct state and you then notify lua that you are no longer using the function allowing lua to garbage collect the function and finally free() the state that was malloced.

If you would want this for python there is still a bit work for that needs to be done but this PR and the vim.call PR lays the foundation so others can reuse it.

{on an unrelated note, i suspect that even then this wouldn't work in neovim as neovim's python implementation is totally different and already divergently incompatible with vim, but that's another story for another place}

I'm not worried about neovim and vim compatibility because vim.fn.* was ported from neovim and merged to vim which means it is 100% compatible. This register_cfunc PR I sent to vim already been ported to neovim and the PR can be found at neovim/neovim#12507.

There are few incompaitiblities with neovim api as they don't support vim.eval but instead has vim.api.nvim_eval. These are very small ones though I hope we can align it sooner than later because there are very few to almost no lua plugins now but it is starting to pickup in the neovim world but that will be up to neovim and vim to decide.

@prabirshrestha
Copy link
Author

@brammool Any thoughts on merging this given the PR has been open for almost 2 weeks and no comment from lua plug-in others?
I did spend quite amount of my time searching for lua plugins in vim and it was few of which most of them seems to be archived. I did notice a huge surge in lua plugins for neovim but these seems to be using neovim APIs that are not compatible with vim. For example neovim exposes the entire libuv through their own api and doesn’t use the VimScript version of jobs and timer api. Given that this same PR has been ported to neovim and is open I’m hoping once the PR gets merged in vim and neovim plug-in others can use the same api on both worlds.

@brammool
Copy link
Contributor

I suppose this looks OK. The help change is tiny though. Could do with some more "advertising" for how this works and how easy it is to pass a callback.

@prabirshrestha
Copy link
Author

Updated doc in funcref userdata section. I was having issue on where to put it without creating an new sections.

@andymass
Copy link

Next version of neovim 0.5 is going to ship with LSP protocol by default

@prabirshrestha i am curious, what else is necessary to port a lua plugin like neovim's lsp implementation to vim?

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

Successfully merging this pull request may close these issues.

None yet

5 participants