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

[RFC] Create lua interface #4411

Merged
merged 81 commits into from May 8, 2017

Conversation

Projects
None yet
@ZyX-I
Contributor

ZyX-I commented Mar 4, 2016

  • luaeval()
  • :lua command
  • :luafile command
  • :luado command
  • luaeval() tests
  • :lua tests
  • :lua << tests
  • :luafile tests
  • :luado tests
  • (-) compatibility vim module
  • (-) compatibility vim module tests
  • documentation update
  • print
  • print tests
  • debug.debug (Vim appears to be also overriding this)
  • debug.debug tests

Ref #3823

@marvim marvim added the WIP label Mar 4, 2016

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 4, 2016

Contributor

Blocked by #4131: I think I can reuse encode.c again, it has very useful macros for traversing nested typval_T structures without using any recursion. I guess I can and should rewrite vim_to_object as well, currently it can crash when converting deeply nested objects: e.g. on my system vim_to_object_rec crashes with

nvim --cmd 'let l=[]|call map(range(10000), "extend(g:, {\"l\":[l]})")' --cmd 'call rpcnotify(1, "", l)'

(you may need to increase number inside range()).

Contributor

ZyX-I commented Mar 4, 2016

Blocked by #4131: I think I can reuse encode.c again, it has very useful macros for traversing nested typval_T structures without using any recursion. I guess I can and should rewrite vim_to_object as well, currently it can crash when converting deeply nested objects: e.g. on my system vim_to_object_rec crashes with

nvim --cmd 'let l=[]|call map(range(10000), "extend(g:, {\"l\":[l]})")' --cmd 'call rpcnotify(1, "", l)'

(you may need to increase number inside range()).

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 4, 2016

Member

What do RPC client authors need to know in order to migrate after #4131, if anything? If we can provide clear answers to that, then I say just merge it.

Member

justinmk commented Mar 4, 2016

What do RPC client authors need to know in order to migrate after #4131, if anything? If we can provide clear answers to that, then I say just merge it.

@ianks

This comment has been minimized.

Show comment
Hide comment
@ianks

ianks Mar 4, 2016

Will this bring along with it the functionality generally required for if_lua?

ianks commented Mar 4, 2016

Will this bring along with it the functionality generally required for if_lua?

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 4, 2016

Contributor

@ianks Look at the TODO list.

Contributor

ZyX-I commented Mar 4, 2016

@ianks Look at the TODO list.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 4, 2016

Contributor

@justinmk Where I should place notes regarding migration? Also there is still unresolved question with *set_var, I did not implement last proposal.

Contributor

ZyX-I commented Mar 4, 2016

@justinmk Where I should place notes regarding migration? Also there is still unresolved question with *set_var, I did not implement last proposal.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 4, 2016

Member

@ZyX-I I'm speaking of #4131 the JSON PR. I haven't had time to fully understand the implications of the v:null change for the API, so I am not prepared to explain to client authors why their clients broke (if that happens).

Also there is still unresolved question with *set_var, I did not implement last proposal.

Oh. Ok, I will try to offer an opinion on it in the next days.

Member

justinmk commented Mar 4, 2016

@ZyX-I I'm speaking of #4131 the JSON PR. I haven't had time to fully understand the implications of the v:null change for the API, so I am not prepared to explain to client authors why their clients broke (if that happens).

Also there is still unresolved question with *set_var, I did not implement last proposal.

Oh. Ok, I will try to offer an opinion on it in the next days.

@ianks

This comment has been minimized.

Show comment
Hide comment
@ianks

ianks Mar 4, 2016

I did. I just don't know enough about Neovim internals to know what it all means.

ianks commented Mar 4, 2016

I did. I just don't know enough about Neovim internals to know what it all means.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 4, 2016

Contributor

Because I got no replies whether it is the right path.

Contributor

ZyX-I commented Mar 4, 2016

Because I got no replies whether it is the right path.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 4, 2016

Member

@ianks Answer is "yes", mostly, though I am curious about how userdata will be handled, if at all.

(I'm not opposed to including lua/luajit in-process for nvim, I think it's important enough for the project to offer this as a first-class alternative to VimL, though it does make other language hosts "second class". Note that we should consider allowing lua to be substituted for luajit at build-time if possible, @jamessan has mentioned this for cross-platform support)

Member

justinmk commented Mar 4, 2016

@ianks Answer is "yes", mostly, though I am curious about how userdata will be handled, if at all.

(I'm not opposed to including lua/luajit in-process for nvim, I think it's important enough for the project to offer this as a first-class alternative to VimL, though it does make other language hosts "second class". Note that we should consider allowing lua to be substituted for luajit at build-time if possible, @jamessan has mentioned this for cross-platform support)

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 4, 2016

Contributor

At the current state differences are minor:

  1. Everything which returns converted typval_T (vim_eval, *_set_var, *_get_var) may return nil and boolean objects now and this (especially nil) no longer indicates any error.
  2. Everything which accepts a value which will be converted to typval_T (*_set_var) now converts nil objects to v:null and not 0, similarly boolean objects will be converted to v:true/v:false and not numbers.
  3. *_set_var(name, nil) will not delete the variable, clients need to use *_del_var(name) instead.
  4. *_set_var(name, val) returning nil no longer means that variable did not exist, it now means that variable did not exist or existed, but was set to v:null: that is last thing discussed.

1 and 2 should not break much, but 3 and any way to fix problem in 4 I can imagine will break some functionality.


@justinmk I always said that I am planning to be compatible with lua without jit.

Contributor

ZyX-I commented Mar 4, 2016

At the current state differences are minor:

  1. Everything which returns converted typval_T (vim_eval, *_set_var, *_get_var) may return nil and boolean objects now and this (especially nil) no longer indicates any error.
  2. Everything which accepts a value which will be converted to typval_T (*_set_var) now converts nil objects to v:null and not 0, similarly boolean objects will be converted to v:true/v:false and not numbers.
  3. *_set_var(name, nil) will not delete the variable, clients need to use *_del_var(name) instead.
  4. *_set_var(name, val) returning nil no longer means that variable did not exist, it now means that variable did not exist or existed, but was set to v:null: that is last thing discussed.

1 and 2 should not break much, but 3 and any way to fix problem in 4 I can imagine will break some functionality.


@justinmk I always said that I am planning to be compatible with lua without jit.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 4, 2016

Contributor

@justinmk Also note the branch name. I thought it is good idea to break luaviml branch into something smaller which is why this PR was added, not because I actually want :lua* and luaeval().

Contributor

ZyX-I commented Mar 4, 2016

@justinmk Also note the branch name. I thought it is good idea to break luaviml branch into something smaller which is why this PR was added, not because I actually want :lua* and luaeval().

@DemiMarie

This comment has been minimized.

Show comment
Hide comment
@DemiMarie

DemiMarie Mar 12, 2016

I would like for Lua to be able to add built-in functions to NeoVIM.

DemiMarie commented Mar 12, 2016

I would like for Lua to be able to add built-in functions to NeoVIM.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 17, 2016

Contributor

@justinmk Last commit adds api/ref.c. It does not actually contain any new functionality, but it provides data about how I see Reference type and thus may be reviewed from this point of view.

Contributor

ZyX-I commented Mar 17, 2016

@justinmk Last commit adds api/ref.c. It does not actually contain any new functionality, but it provides data about how I see Reference type and thus may be reviewed from this point of view.

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Mar 20, 2016

Member

What is the ReferenceDef interface for? Is it the different viml container/function types, or do we imagine even more different reference kinds?

Member

bfredl commented Mar 20, 2016

What is the ReferenceDef interface for? Is it the different viml container/function types, or do we imagine even more different reference kinds?

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 20, 2016

Contributor

@bfredl For different types. Specifically for deeper integration between languages, I wanted something like this for Vim: to be able to use Python functions or containers directly from Ruby etc. At a cost of converting Python types to VimL and only then to Ruby, of course.

In the current state using Python objects in Ruby is probably not the best idea (too many IPC calls: first from Ruby to Neovim, then from Neovim to Python, then sending replies back through this chain), though still applicable for some uses. But this allows adding things like partials with no changes on the client side.

I also considered adding Reference ref_environ(void) function which will simply provide interface to Vim environment without adding a set of new functions to the API. Same applies to accessing mappings, signs, autocommands, etc: everything which may have a mapping or sequence interface, it is easier to create “generic” objects which work with any Reference and forget about this than add a bunch of new functions every time access to new Vim internals is added. Note that only mappings and matches have something like a usable API (I mean functions like maparg() and getmatches()/setmatches()), signs and autocommands require you relying on :redir and parsing output of the commands which is both not very reliable and is easier to break when new functionality is added (like <cmd> from original variant of #4419 (currently it was replaced with <cmd> key)).


And another idea: make functions like ref_get_item accept an argument which will cause reference to be coerced to Object without additional request. This would reduce *_*_var (e.g. window_get_var) function family to ref_get_*_scope/*_get_scope (returns scope dictionary, example: ref_get_window_scope (ref.c) / window_get_scope (window.c)), but is clumsier to use: even though it is only +2 API calls (no matter how many times you need to use vim_*_var it adds only two API calls), with window_get_var you do not need to care about unreferencing anything.


I would also ask whether anybody actually wants *_del_* and *_set_* remove previous values? In existing API this is a huge waste of resources because 90% of time you do not need this and if you e.g. delete a large list you will get it serialized and transferred to the client. In the ref_* API it is huge waste of resources only when it comes to deleting a huge slice or deleting a lengthy string because deleting large list from a scope will only make API send a Reference to it. But developers of the clients will have to code sending ref_unref call back or there will be a memory leak on each *_del_* and *_set_* when deleting/replacing containers.

Contributor

ZyX-I commented Mar 20, 2016

@bfredl For different types. Specifically for deeper integration between languages, I wanted something like this for Vim: to be able to use Python functions or containers directly from Ruby etc. At a cost of converting Python types to VimL and only then to Ruby, of course.

In the current state using Python objects in Ruby is probably not the best idea (too many IPC calls: first from Ruby to Neovim, then from Neovim to Python, then sending replies back through this chain), though still applicable for some uses. But this allows adding things like partials with no changes on the client side.

I also considered adding Reference ref_environ(void) function which will simply provide interface to Vim environment without adding a set of new functions to the API. Same applies to accessing mappings, signs, autocommands, etc: everything which may have a mapping or sequence interface, it is easier to create “generic” objects which work with any Reference and forget about this than add a bunch of new functions every time access to new Vim internals is added. Note that only mappings and matches have something like a usable API (I mean functions like maparg() and getmatches()/setmatches()), signs and autocommands require you relying on :redir and parsing output of the commands which is both not very reliable and is easier to break when new functionality is added (like <cmd> from original variant of #4419 (currently it was replaced with <cmd> key)).


And another idea: make functions like ref_get_item accept an argument which will cause reference to be coerced to Object without additional request. This would reduce *_*_var (e.g. window_get_var) function family to ref_get_*_scope/*_get_scope (returns scope dictionary, example: ref_get_window_scope (ref.c) / window_get_scope (window.c)), but is clumsier to use: even though it is only +2 API calls (no matter how many times you need to use vim_*_var it adds only two API calls), with window_get_var you do not need to care about unreferencing anything.


I would also ask whether anybody actually wants *_del_* and *_set_* remove previous values? In existing API this is a huge waste of resources because 90% of time you do not need this and if you e.g. delete a large list you will get it serialized and transferred to the client. In the ref_* API it is huge waste of resources only when it comes to deleting a huge slice or deleting a lengthy string because deleting large list from a scope will only make API send a Reference to it. But developers of the clients will have to code sending ref_unref call back or there will be a memory leak on each *_del_* and *_set_* when deleting/replacing containers.

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Mar 20, 2016

Member

This would reduce *_*_var (e.g. window_get_var) function family to ref_get_*_scope/*_get_scope

We could avoid the extra roundtrip by allowing the client construct the scope reference ( have an EXT type such that the object EXT can be converted to the scope EXT by a client)

Member

bfredl commented Mar 20, 2016

This would reduce *_*_var (e.g. window_get_var) function family to ref_get_*_scope/*_get_scope

We could avoid the extra roundtrip by allowing the client construct the scope reference ( have an EXT type such that the object EXT can be converted to the scope EXT by a client)

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 21, 2016

Contributor

@bfredl I do not think this is a good idea:

  1. Server needs to know that client needs to keep scope dictionary. -1 initial API call is nothing compared to API that leaves client writers with the idea that scope reference does not need to be unreferenced (which is logical because reference was not created by the server or it does not look like it was created: from my point of view this looks like requesting something from a resource that needs to be opened and closed without opening it explicitly which makes me expect that closing it explicitly is also not needed).
  2. Two EXT types for one Reference type is not really good and I do not like to give up an idea “first n bits are capabilities listed in --api-info, everything else is none of your business” for an existing type.
  3. Depending on the implementation this either will make it impossible to compare reference identity by simple EXT data contents comparison or will make code that serializes Reference more complicated: because now you need to check whether given dictionary is the scope one and if yes serialize it one way else another. I fear determining what exactly scope dictionary needs to be serialized to will not be too easy (determining that dictionary is a scope one is easy because this information is saved).
Contributor

ZyX-I commented Mar 21, 2016

@bfredl I do not think this is a good idea:

  1. Server needs to know that client needs to keep scope dictionary. -1 initial API call is nothing compared to API that leaves client writers with the idea that scope reference does not need to be unreferenced (which is logical because reference was not created by the server or it does not look like it was created: from my point of view this looks like requesting something from a resource that needs to be opened and closed without opening it explicitly which makes me expect that closing it explicitly is also not needed).
  2. Two EXT types for one Reference type is not really good and I do not like to give up an idea “first n bits are capabilities listed in --api-info, everything else is none of your business” for an existing type.
  3. Depending on the implementation this either will make it impossible to compare reference identity by simple EXT data contents comparison or will make code that serializes Reference more complicated: because now you need to check whether given dictionary is the scope one and if yes serialize it one way else another. I fear determining what exactly scope dictionary needs to be serialized to will not be too easy (determining that dictionary is a scope one is easy because this information is saved).
@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Mar 21, 2016

Member

As I said before, I don't mean client constructable named references to replace (parts of) your proposal.
I would imagine a named reference to look something like this (NameRef( is here just a name for the EXT object type code, which can contain nested msgpack as its binary data)
NameRef(['gvars']) - g:
NameRef(['bvars', Buffer(objid)]) - b: for buffer EXT obj (or we can have ovars as the object type is understood)
or posibly even as an addon
NameRef(['gvars', mydyct]) - g:mydict
NameRef(['vars', Buffer(objid), 'mydict']) for b:mydict
It is understood that these namerefs are nullable (bvars will fail if the buffer handle was invalid), but otherwise pure (no generic viml eval with side-effects). Remote clients also understand that these cannot be compared for referential equlity, they need to be interned for comparison and for having a permanent handle to the actual object.

These will be converted to References early in the request dispatch logic. No bookkeeping, just a one-shot ext_nameref_to_reference. So this is a pure add-on and can be implemented later as another PR.

Member

bfredl commented Mar 21, 2016

As I said before, I don't mean client constructable named references to replace (parts of) your proposal.
I would imagine a named reference to look something like this (NameRef( is here just a name for the EXT object type code, which can contain nested msgpack as its binary data)
NameRef(['gvars']) - g:
NameRef(['bvars', Buffer(objid)]) - b: for buffer EXT obj (or we can have ovars as the object type is understood)
or posibly even as an addon
NameRef(['gvars', mydyct]) - g:mydict
NameRef(['vars', Buffer(objid), 'mydict']) for b:mydict
It is understood that these namerefs are nullable (bvars will fail if the buffer handle was invalid), but otherwise pure (no generic viml eval with side-effects). Remote clients also understand that these cannot be compared for referential equlity, they need to be interned for comparison and for having a permanent handle to the actual object.

These will be converted to References early in the request dispatch logic. No bookkeeping, just a one-shot ext_nameref_to_reference. So this is a pure add-on and can be implemented later as another PR.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 21, 2016

Contributor

@bfredl I think this is a good idea. Though I would not add it here, such references are not needed for lua interface.

Contributor

ZyX-I commented Mar 21, 2016

@bfredl I think this is a good idea. Though I would not add it here, such references are not needed for lua interface.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Apr 5, 2016

Member

@ZyX-I Regarding the compatibility question:

*_set_var(name, nil) will not delete the variable, clients need to use *_del_var(name) instead.

*_set_var(name, val) returning nil no longer means that variable did not exist, it now means that variable did not exist or existed, but was set to v:null: that is last thing discussed.

We could use a similar approach as in #4083 (6eda7c0): create new functions and deprecate the old ones. But I think we can break back-compat here.

  • Deleting a variable is relatively uncommon task compared to typical task.
    • Doing this via an API call (instead of eval'd VimL) is even less common.
  • Checking the result of a set call is even less common; I would be surprised if anyone uses this feature.

Last time we made a breaking change to the API I just notified client/plugin authors to be aware of the change, and it worked out ok. We can still use this approach, though #4083 and more explicit versioning will be required when Neovim gets more popular.

Member

justinmk commented Apr 5, 2016

@ZyX-I Regarding the compatibility question:

*_set_var(name, nil) will not delete the variable, clients need to use *_del_var(name) instead.

*_set_var(name, val) returning nil no longer means that variable did not exist, it now means that variable did not exist or existed, but was set to v:null: that is last thing discussed.

We could use a similar approach as in #4083 (6eda7c0): create new functions and deprecate the old ones. But I think we can break back-compat here.

  • Deleting a variable is relatively uncommon task compared to typical task.
    • Doing this via an API call (instead of eval'd VimL) is even less common.
  • Checking the result of a set call is even less common; I would be surprised if anyone uses this feature.

Last time we made a breaking change to the API I just notified client/plugin authors to be aware of the change, and it worked out ok. We can still use this approach, though #4083 and more explicit versioning will be required when Neovim gets more popular.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Apr 5, 2016

Contributor

@justinmk

Checking the result of a set call is even less common; I would be surprised if anyone uses this feature.

Why does it exist then? I can simply make set and del void, this will avoid lots of unnecessary allocs/frees on both sides of the channel.


For backward compatibility I know the following approaches that are applicable here:

  1. Break it.
  2. Deprecated functions, removed in the major version. Note: it is better to give out information in the --api-info: at least Python with warnings module may give deprecation warnings without breakage.
  3. Versioned function: you request some API function and some version of it. Old versions of functions are kept, new versions do not need different name. Basically this is like naming set_vim_var set_vim_var__1_0 then replacing with set_vim_var__1_1.
  4. Versioned channels: when connecting to Neovim you request specific API version (or, to be compatible, use a separate API call until which version just before that call was introduced is assumed). Like previous variant, but version suffix is implicit. It is better to use semantic versioning here: when requesting API version 1.1 1.2 and 1.3 may be used as well. No patch version.

Points 3 and 4 may also have an addition “older version is removed in next major release”.

I do not remember API compatibility being discussed somewhere: did I miss some issue or should it be a new one?

Contributor

ZyX-I commented Apr 5, 2016

@justinmk

Checking the result of a set call is even less common; I would be surprised if anyone uses this feature.

Why does it exist then? I can simply make set and del void, this will avoid lots of unnecessary allocs/frees on both sides of the channel.


For backward compatibility I know the following approaches that are applicable here:

  1. Break it.
  2. Deprecated functions, removed in the major version. Note: it is better to give out information in the --api-info: at least Python with warnings module may give deprecation warnings without breakage.
  3. Versioned function: you request some API function and some version of it. Old versions of functions are kept, new versions do not need different name. Basically this is like naming set_vim_var set_vim_var__1_0 then replacing with set_vim_var__1_1.
  4. Versioned channels: when connecting to Neovim you request specific API version (or, to be compatible, use a separate API call until which version just before that call was introduced is assumed). Like previous variant, but version suffix is implicit. It is better to use semantic versioning here: when requesting API version 1.1 1.2 and 1.3 may be used as well. No patch version.

Points 3 and 4 may also have an addition “older version is removed in next major release”.

I do not remember API compatibility being discussed somewhere: did I miss some issue or should it be a new one?

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Apr 5, 2016

Member

Why does it exist then? I can simply make set and del void, this will avoid lots of unnecessary allocs/frees on both sides of the channel.

I can't think of a reason why it would be needed, maybe @tarruda can comment.

Deprecated functions

I suggest we take this approach as far as we can because it is the least clever. "Versioned channels" would be the next step.

#1393 may be used to discuss API compatibility/versioning, though that issue regards "middleware" versions and not the nvim server itself.

Member

justinmk commented Apr 5, 2016

Why does it exist then? I can simply make set and del void, this will avoid lots of unnecessary allocs/frees on both sides of the channel.

I can't think of a reason why it would be needed, maybe @tarruda can comment.

Deprecated functions

I suggest we take this approach as far as we can because it is the least clever. "Versioned channels" would be the next step.

#1393 may be used to discuss API compatibility/versioning, though that issue regards "middleware" versions and not the nvim server itself.

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Apr 5, 2016

Member

Another approach is: adding new parameters with default values for old clients that don't supply it. Though it is probably overblown for this case.

It might be useful for a rplugin to say get and delete a key atomically, but this is probably better served either by a general concept of atomic transactions

vim_exclusive_begin()
get_var(somekey)
del_var(somekey)
vim_exclusive_end()

implemented correctly this could even be streamed (only one roundtrip needed).
With this branch, another alternative for an rplugin to do an atomic transaction is of course to inject some lua code to do it.

Member

bfredl commented Apr 5, 2016

Another approach is: adding new parameters with default values for old clients that don't supply it. Though it is probably overblown for this case.

It might be useful for a rplugin to say get and delete a key atomically, but this is probably better served either by a general concept of atomic transactions

vim_exclusive_begin()
get_var(somekey)
del_var(somekey)
vim_exclusive_end()

implemented correctly this could even be streamed (only one roundtrip needed).
With this branch, another alternative for an rplugin to do an atomic transaction is of course to inject some lua code to do it.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Apr 5, 2016

Contributor

@bfredl This approach is rather limited which is why I did not list it:

  1. You may not use it to change return type. You can have some workarounds (in the current case that would be e.g. having optional argument force returning NIL always), but they are not always applicable.
  2. You may not use it to change type of one of the first arguments.
  3. --api-info does not support this currently, so in the current situation it still will introduce an incompatibility.

I think that the idea of atomic requests (not transactions) can be implemented with the current code rather easily: just make an API call which will receive list of pairs method_name, arguments and run this directly: not much of a problem, you just need handle_request that accepts String plus Array in place of msgpack_object request and runs on_request_event directly in place of queueing it, and on_request_event for this job which returns Object in place of writing serialized msgpack. Since there is a single thread which processes all requests, as well as scripts and user input, this will be atomic. But not transaction: if some API call fails previous calls’ effects are not undone.

Contributor

ZyX-I commented Apr 5, 2016

@bfredl This approach is rather limited which is why I did not list it:

  1. You may not use it to change return type. You can have some workarounds (in the current case that would be e.g. having optional argument force returning NIL always), but they are not always applicable.
  2. You may not use it to change type of one of the first arguments.
  3. --api-info does not support this currently, so in the current situation it still will introduce an incompatibility.

I think that the idea of atomic requests (not transactions) can be implemented with the current code rather easily: just make an API call which will receive list of pairs method_name, arguments and run this directly: not much of a problem, you just need handle_request that accepts String plus Array in place of msgpack_object request and runs on_request_event directly in place of queueing it, and on_request_event for this job which returns Object in place of writing serialized msgpack. Since there is a single thread which processes all requests, as well as scripts and user input, this will be atomic. But not transaction: if some API call fails previous calls’ effects are not undone.

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Apr 5, 2016

Member

Well it will work with a new nvim with a client either statically generated for the old api info, or a client doing no checking to the metadata (like python), but versioned/deprecated functions as your suggestions will definitely scale better when things get Sufficiently Complicated (which they will).

make an API call which will receive list of pairs method_name, arguments and run this directly

This would be very useful, for sure.

Member

bfredl commented Apr 5, 2016

Well it will work with a new nvim with a client either statically generated for the old api info, or a client doing no checking to the metadata (like python), but versioned/deprecated functions as your suggestions will definitely scale better when things get Sufficiently Complicated (which they will).

make an API call which will receive list of pairs method_name, arguments and run this directly

This would be very useful, for sure.

@DemiMarie

This comment has been minimized.

Show comment
Hide comment
@DemiMarie

DemiMarie Apr 6, 2016

Any thoughts on having a Lua API call to construct Lua-implemented functions and commands?

DemiMarie commented Apr 6, 2016

Any thoughts on having a Lua API call to construct Lua-implemented functions and commands?

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I May 8, 2017

Contributor

It appears that I have a bug in ri alias, need to strip off merge-base --is-ancestor check completely:

	ri = "!sh -c 't=\"${1:-master}\"; s=\"${2:-HEAD}\"; mb=\"$(git merge-base \"$t\" \"$s\")\"; if test \"x$mb\" = x ; then o=\"$t\"; else lm=\"$(git log -n1 --merges \"$t..$s\" --pretty=%H)\"; if test \"x$lm\" = x ; then o=\"$mb\"; else o=\"$lm\"; fi; fi; test $# -gt 0 && shift; test $# -gt 0 && shift; git rebase --interactive \"$o\" \"$@\"'"
Contributor

ZyX-I commented May 8, 2017

It appears that I have a bug in ri alias, need to strip off merge-base --is-ancestor check completely:

	ri = "!sh -c 't=\"${1:-master}\"; s=\"${2:-HEAD}\"; mb=\"$(git merge-base \"$t\" \"$s\")\"; if test \"x$mb\" = x ; then o=\"$t\"; else lm=\"$(git log -n1 --merges \"$t..$s\" --pretty=%H)\"; if test \"x$lm\" = x ; then o=\"$mb\"; else o=\"$lm\"; fi; fi; test $# -gt 0 && shift; test $# -gt 0 && shift; git rebase --interactive \"$o\" \"$@\"'"
@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I May 8, 2017

Contributor

Got same hang again in QB:

07:16:43,265 INFO  - ok 119 - autocmd BufEnter triggered by "try|:split <dir>|endtry" in a function
07:16:43,479 WARN  - Vim: Caught deadly signal 'SIGTERM'
07:16:43,479 WARN  -
07:16:43,479 WARN  - Vim: Finished.
07:26:41,604 INFO  - Terminating launched command gracefully...
07:26:41,655 WARN  - Terminated
07:26:41,663 INFO  - # test/functional/autocmd/dirchanged_spec.lua
Contributor

ZyX-I commented May 8, 2017

Got same hang again in QB:

07:16:43,265 INFO  - ok 119 - autocmd BufEnter triggered by "try|:split <dir>|endtry" in a function
07:16:43,479 WARN  - Vim: Caught deadly signal 'SIGTERM'
07:16:43,479 WARN  -
07:16:43,479 WARN  - Vim: Finished.
07:26:41,604 INFO  - Terminating launched command gracefully...
07:26:41,655 WARN  - Terminated
07:26:41,663 INFO  - # test/functional/autocmd/dirchanged_spec.lua
@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk May 8, 2017

Member
07:16:43,265 INFO  - ok 119 - autocmd BufEnter triggered by "try|:split <dir>|endtry" in a function

If you did not merge 7c1a5d1 into this PR that would explain the hang. It's not related to this PR. See #6644 (comment)

Member

justinmk commented May 8, 2017

07:16:43,265 INFO  - ok 119 - autocmd BufEnter triggered by "try|:split <dir>|endtry" in a function

If you did not merge 7c1a5d1 into this PR that would explain the hang. It's not related to this PR. See #6644 (comment)

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I May 8, 2017

Contributor

Other then QB, build successfull.

Contributor

ZyX-I commented May 8, 2017

Other then QB, build successfull.

@justinmk justinmk merged commit 0e873a3 into neovim:master May 8, 2017

2 of 4 checks passed

QuickBuild Build pr-4411 finished with status FAILED
Details
coverage/coveralls Coverage pending from Coveralls.io
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@justinmk justinmk removed the RFC label May 8, 2017

@ZyX-I ZyX-I deleted the ZyX-I:luaviml'/lua branch May 8, 2017

@prabirshrestha

This comment has been minimized.

Show comment
Hide comment
@prabirshrestha

prabirshrestha May 9, 2017

:echo has('lua') seems to return 0 in windows builds (Haven't tried other OS yet). Is this deliberate until the actual release?

prabirshrestha commented May 9, 2017

:echo has('lua') seems to return 0 in windows builds (Haven't tried other OS yet). Is this deliberate until the actual release?

@Shougo

This comment has been minimized.

Show comment
Hide comment
@Shougo

Shougo May 9, 2017

Contributor

:echo has('lua') is 0 in Linux environment.
But :lua print(1+1) works.
I think has('lua') does not work. It seems bug.

Contributor

Shougo commented May 9, 2017

:echo has('lua') is 0 in Linux environment.
But :lua print(1+1) works.
I think has('lua') does not work. It seems bug.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc May 9, 2017

Contributor

@prabirshrestha, @Shougo I think the vim lua module does not currently implement vim's original API, so that is to be expected.

Contributor

fmoralesc commented May 9, 2017

@prabirshrestha, @Shougo I think the vim lua module does not currently implement vim's original API, so that is to be expected.

@Shougo

This comment has been minimized.

Show comment
Hide comment
@Shougo

Shougo May 9, 2017

Contributor

OK.

Contributor

Shougo commented May 9, 2017

OK.

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl May 9, 2017

Member

I get this warning presumably from this PR:

../src/nvim/lua/converter.c: In function ‘nlua_pop_typval’:
../src/nvim/lua/converter.c:323:17: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
                 assert(cur.tv->v_type = VAR_DICT);
Member

bfredl commented May 9, 2017

I get this warning presumably from this PR:

../src/nvim/lua/converter.c: In function ‘nlua_pop_typval’:
../src/nvim/lua/converter.c:323:17: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
                 assert(cur.tv->v_type = VAR_DICT);
@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I May 9, 2017

Contributor

Wondering how did that manage to pass CI.

Contributor

ZyX-I commented May 9, 2017

Wondering how did that manage to pass CI.

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl May 9, 2017

Member

Different compiler versions?

Member

bfredl commented May 9, 2017

Different compiler versions?

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I May 9, 2017

Contributor

Yes, but this is one of the basic warnings. I am surprised how much of compilers we are using did not show it: starting from clang 3.9.1 (on my system) and proceeding with all those CI compilers compiling debug mode.

Contributor

ZyX-I commented May 9, 2017

Yes, but this is one of the basic warnings. I am surprised how much of compilers we are using did not show it: starting from clang 3.9.1 (on my system) and proceeding with all those CI compilers compiling debug mode.

ZyX-I added a commit to ZyX-I/neovim that referenced this pull request May 9, 2017

justinmk added a commit that referenced this pull request May 10, 2017

@@ -309,6 +309,14 @@ include_directories(SYSTEM ${LIBUV_INCLUDE_DIRS})
find_package(Msgpack 1.0.0 REQUIRED)
include_directories(SYSTEM ${MSGPACK_INCLUDE_DIRS})
option(PREFER_LUAJIT "Prefer LuaJIT over Lua when compiling executable. Test library always uses luajit." ON)
find_package(LuaJit REQUIRED)

This comment has been minimized.

@jamessan

jamessan May 10, 2017

Member

If I don't care to run tests, then this REQUIRED prevents me from being able to use cmake -DPREFER_LUAJIT=OFF to simply build nvim with Lua, doesn't it? Shouldn't we instead avoid creating the relevant test targets if luajit isn't available?

@jamessan

jamessan May 10, 2017

Member

If I don't care to run tests, then this REQUIRED prevents me from being able to use cmake -DPREFER_LUAJIT=OFF to simply build nvim with Lua, doesn't it? Shouldn't we instead avoid creating the relevant test targets if luajit isn't available?

This comment has been minimized.

@jamessan

jamessan May 10, 2017

Member

This seems to be what's causing our nightly builds to fail.

@jamessan

jamessan May 10, 2017

Member

This seems to be what's causing our nightly builds to fail.

This comment has been minimized.

@fwalch

fwalch May 13, 2017

Member

I tried to just switch the PPA to LuaJIT, but unfortunately LuaJIT is not supported on ARM64. Opened #6736.

@fwalch

fwalch May 13, 2017

Member

I tried to just switch the PPA to LuaJIT, but unfortunately LuaJIT is not supported on ARM64. Opened #6736.

return 0;
nlua_print_error:
emsgf(_("E5114: Error while converting print argument #%i: %.*s"),
curargidx, errmsg_len, errmsg);

This comment has been minimized.

@oni-link

oni-link May 14, 2017

Contributor

Length argument should be of type int: errmsg_len -> (int)errmsg_len, otherwise macros va_*() parse the parameter list wrong.

@oni-link

oni-link May 14, 2017

Contributor

Length argument should be of type int: errmsg_len -> (int)errmsg_len, otherwise macros va_*() parse the parameter list wrong.

@abhi18av

This comment has been minimized.

Show comment
Hide comment
@abhi18av

abhi18av May 19, 2017

As reported by @Shougo , @prabirshrestha

I'm on macOS 12.4 and :lua print(1+1) doesn't work.

My vim version

❯ vim --version
NVIM v0.2.0
Build type: RelWithDebInfo
Compilation: /usr/local/Homebrew/Library/Homebrew/shims/super/clang -Wconversion -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 -DNVIM_MSGPACK_HAS_FLOAT32 -O2 -g -DDISABLE_LOG -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wvla -fstack-protector-strong -fdiagnostics-color=auto -DINCLUDE_GENERATED_DECLARATIONS -I/tmp/neovim-20170519-14192-4zve0f/neovim-0.2.0/build/config -I/tmp/neovim-20170519-14192-4zve0f/neovim-0.2.0/src -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/opt/gettext/include -I/usr/include -I/usr/include -I/tmp/neovim-20170519-14192-4zve0f/neovim-0.2.0/build/src/nvim/auto -I/tmp/neovim-20170519-14192-4zve0f/neovim-0.2.0/build/include
Compiled by eklavya@abhinavs-MacBook-Pro.local

Optional features included (+) or not (-): +acl   +iconv    +jemalloc +tui
For differences from Vim, see :help vim-differences

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/usr/local/Cellar/neovim/0.2.0/share/nvim"

abhi18av commented May 19, 2017

As reported by @Shougo , @prabirshrestha

I'm on macOS 12.4 and :lua print(1+1) doesn't work.

My vim version

❯ vim --version
NVIM v0.2.0
Build type: RelWithDebInfo
Compilation: /usr/local/Homebrew/Library/Homebrew/shims/super/clang -Wconversion -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 -DNVIM_MSGPACK_HAS_FLOAT32 -O2 -g -DDISABLE_LOG -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wvla -fstack-protector-strong -fdiagnostics-color=auto -DINCLUDE_GENERATED_DECLARATIONS -I/tmp/neovim-20170519-14192-4zve0f/neovim-0.2.0/build/config -I/tmp/neovim-20170519-14192-4zve0f/neovim-0.2.0/src -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/opt/gettext/include -I/usr/include -I/usr/include -I/tmp/neovim-20170519-14192-4zve0f/neovim-0.2.0/build/src/nvim/auto -I/tmp/neovim-20170519-14192-4zve0f/neovim-0.2.0/build/include
Compiled by eklavya@abhinavs-MacBook-Pro.local

Optional features included (+) or not (-): +acl   +iconv    +jemalloc +tui
For differences from Vim, see :help vim-differences

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/usr/local/Cellar/neovim/0.2.0/share/nvim"
@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl May 19, 2017

Member

0.2.0 doesn't have lua, you need 0.2.1-dev (master)

Member

bfredl commented May 19, 2017

0.2.0 doesn't have lua, you need 0.2.1-dev (master)

@abhi18av

This comment has been minimized.

Show comment
Hide comment
@abhi18av

abhi18av commented May 19, 2017

@bfredl Thanks :)

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 8, 2017

NVIM v0.2.1
FEATURES:
0e873a3 Lua(Jit) built-in #4411
5b32bce Windows: `:terminal` #7007
7b0ceb3 UI/API: externalize cmdline #7173
b67f58b UI/API: externalize wildmenu #7454
b23aa1c UI: 'winhighlight' #6597
17531ed UI: command-line coloring (`:help input()-highlight`) #6364
244a1f9 API: execute lua directly from the remote api #6704
45626de API: `get_keymap()` #6236
db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` #7082
dc68538 menu_get() function #6322
9db42d4 :cquit : take an error code argument #7336
9cc185d job-control: serverstart(): support ipv6 #6680
1b7a9bf job-control: sockopen() #6594
6efe84a clipboard: fallback to tmux clipboard #6894
6016ac2 clipboard: customize clipboard with `g:clipboard` #6030
3a86dd5 ruby: override ruby host via `g:ruby_host_prog` #6841
16cce1a debug: $NVIM_LOG_FILE #6827
0cba3da `:checkhealth` built-in, validates $VIMRUNTIME #7399

FIXES:
105d680 TUI: more terminals, improve scroll/resize #6816
cb912a3 :terminal : handle F1-F12, other keys #7241
619838f inccommand: improve performance #6949
04b3c32 inccommand: Fix matches for zero-width #7487
60b1e8a inccommand: multiline, other fixes #7315
f1f7f3b inccommand: Ignore leading modifiers in the command #6967
1551f71 inccommand: fix 'gdefault' lockup #7262
6338199 API: bufhl: support creating new groups #7414
541dde3 API: allow K_EVENT during operator-pending
8c732f7 terminal: adjust for 'number' #7440
5bec946 UI: preserve wildmenu during jobs/events #7110
c349083 UI: disable 'lazyredraw' during ui_refresh. #6259
51808a2 send FocusGained/FocusLost event instead of pseudokey #7221
133f8bc shada: preserve unnamed register on restart #4700
1b70a1d shada: avoid assertion on corrupt shada file #6958
9f534f3 mksession: Restore tab-local working directory #6859
de1084f fix buf_write() crash #7140
7f76986 syntax: register 'Normal' highlight group #6973
6e7a8c3 RPC: close channel if stream was closed #7081
85f3084 clipboard: disallow recursion; show hint only once #7203
8d1ccb6 clipboard: performance, avoid weird edge-cases #7193
01487d4 'titleold' #7358
01e53a5 Windows: better path-handling, separator (slash) hygiene #7349
0f2873c Windows: multibyte startup arguments #7060

CHANGES:
9ff0cc7 :terminal : start in normal-mode #6808
032b088 lower priority of 'cursorcolumn', 'colorcolumn' #7364
2a3bcd1 RPC: Don't delay notifications when request is pending #6544
023f67c :terminal : Do not change 'number', 'relativenumber' #6796
1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets #6915
6720fe2 help: `K` tries Vim help instead of manpage #3104
7068370 help, man.vim: change "outline" map to `gO` #7405

@justinmk justinmk referenced this pull request Nov 8, 2017

Merged

NVIM v0.2.1 #7505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment