[RFC] execute lua directly from the remote API #6704

bfredl · 2017-05-09T07:00:52Z

Should be NOEVAL, but is not yet to facilitate quick testing.

ZyX-I · 2017-05-09T15:05:04Z

src/nvim/lua/executor.c

+  }
+
+  if (lua_pcall(lstate, (int)args->size, 1, 0)) {
+    api_set_error(err, kErrorTypeException, "Error while calling lua chunk");


Missing error string, it is on top of the stack: see nlua_error.

thanks, fixed.

bfredl · 2017-05-11T06:24:23Z

added docs and tests, marking RFC.

justinmk · 2017-05-11T07:02:47Z

test/functional/api/vim_spec.lua

+    end)
+
+    it('reports errors', function()
+      meths.execute_lua('vim.api.nvim_set_var("test", 3)', {})


justinmk · 2017-05-11T07:04:57Z

test/functional/api/vim_spec.lua

+      eq({false, 'Error executing lua: [string "<nvim>"]:1: '..
+                 "attempt to call global 'bork' (a nil value)"},
+         meth_pcall(meths.execute_lua, 'bork()', {}))
+    end)


Maybe test an arity error or some other error involving the args array.

what error? lua just adds or subtracts nil as it sees fit. Maybe, if there any valid msgpack Object that can't be translated to valid lua, but I can't think of any...

meth_pcall(meths.execute_lua, '(function() return "foo" end)()', {42, 'baz'}))

that doesn't even use the arguments.

That was intentional...

so what would the test test? A nested function that takes no arguments and gets no arguments? That is not an error.

justinmk · 2017-05-11T07:08:29Z

test/functional/api/vim_spec.lua

@@ -81,6 +81,26 @@ describe('api', function()
    end)
  end)

+  describe('nvim_execute_lua', function()


Is it worth testing with call_atomic ?

why? how behaves it differently with call_atomic? Also this is to some extent intended as an alternative to call_atomic in cases it is too primitive.

I don't know, it depends on interactions that I didn't evaluate. Also, adding test coverage exercises future accidental interactions.

Also this is to some extent intended as an alternative to call_atomic in cases it is too primitive.

My intention is to recommend Lua for any server-side coordination logic, who knows what kind of things will be batched in call_atomic.

By that argument we could test any api function through call_atomic. Why do we expect execute_lua to have any special "interactions"?

If lua is used for batching and coordination, then how is call_atomic needed?

If lua is used for batching and coordination, then how is call_atomic needed?

I think that's a good observation. It's usually good to have a single (powerful) way to do things. If one has both, users/script writers will often think about which is best/most efficient/... for their situation, perhaps spending needless time on this.

Yes, for instance for add_highlight I will probably recommend lua over call_atomic. There should probably be a standard runtime lua function for "append some text to a buffer with certain highlights" instead of every rplugin maintaining an output buffer needs to reimplement it.

justinmk · 2017-05-11T07:56:36Z

~~I suggest naming it nvim_eval_lua for parallel with nvim_eval.~~ (edit: Though it executes statements, not just expressions... hmm. But it always returns a "result". So it's fair to treat it as "eval".)

re #4411 (comment)

why an Array? external api clients can just provide a blob of code, they don't need to deal with escaping NL or NUL or anything.

What happens to NUL bytes in the code argument? Should at least have a test for it.

fmoralesc · 2017-05-11T08:06:52Z

Re: #6704 (comment) Where are the docs?

ZyX-I · 2017-05-11T09:05:11Z

I suggest naming it nvim_eval_lua for parallel with nvim_eval. (edit: Though it executes statements, not just expressions... hmm. But it always returns a "result". So it's fair to treat it as "eval".)

I would expect nvim_eval_lua to behave like luaeval(), so I think current naming is fine.

justinmk · 2017-05-11T09:11:29Z

I would expect nvim_eval_lua to behave like luaeval(), so I think current naming is fine.

Do we have any reason to add luaeval-like API function? If not we should stick to API conventions rather than lua conventions.

bfredl · 2017-05-11T09:11:40Z

it executes a block of code, it does not eval an expression.

bfredl · 2017-05-11T09:12:30Z

@fmoralesc in api/vim.c, api.txt will be auto-generated on release (or whenever someone feels for it).

bfredl · 2017-05-11T09:14:05Z

What happens to NUL bytes in the code argument?

It happens whatever luajit decides to do. Messing with linelists in the api does not help. But a test would be good.

justinmk · 2017-05-11T09:20:00Z

it executes a block of code, it does not eval an expression.

Doesn't it? What does meth_pcall(meths.execute_lua, '1+2', {})) return?

bfredl · 2017-05-11T09:21:09Z

nil

ZyX-I · 2017-05-11T09:21:52Z

@bfredl lua -e '1+2' yields “lua: (command line):1: unexpected symbol near '1'”.

bfredl · 2017-05-11T09:22:46Z

@ZyX-I the point was: it does not return 3.

ZyX-I · 2017-05-11T09:23:57Z

@bfredl My point is: it may not return 3, yet it is not a correct statement, it is only a correct expression.

bfredl · 2017-05-11T09:25:19Z

it won't return the value of an correct expression either (not sure why lua makes a difference), for that one needs return

justinmk · 2017-05-11T09:28:43Z

@ZyX-I makes a good point, it will avoid confusion if we raise an error for invalid statements, I would guess people might try nvim_execute_lua, '1+2', {}).

bfredl · 2017-05-11T09:29:38Z

but that is what lua does for us...

justinmk · 2017-05-11T09:30:59Z

Ok, I made a wrong assumption then, because the tests cover 'a+*b' (invalid expression) but not a valid expression (invalid statement).

bfredl · 2017-05-11T09:31:31Z

but a test would be good, and a note in the docs.

ZyX-I · 2017-05-11T09:31:31Z

But I expect that to error out, like from the command-line, not return nil. I am not sure what is the cause here, I do not see you doing something wrong. :lua 1+2 errors out though.

ZyX-I · 2017-05-11T09:33:16Z

And :lua is using nearly the same code.

With one exception: I have two arguments and do lua_pop(…, 2). You have four, but only lua_pop(…, 3).

ZyX-I · 2017-05-11T09:38:28Z

src/nvim/lua/executor.c

+  Object *retval = (Object *)lua_touserdata(lstate, 3);
+  Error *err = (Error *)lua_touserdata(lstate, 4);
+
+  lua_pop(lstate, 3);


This should be 4.

bfredl · 2017-05-11T09:45:05Z

It does behave constitently. I just guessed nil because I didn't know an arbitrary subset of expressions are not statements. Den 11 maj 2017 11:38 fm skrev "Nikolai Aleksandrovich Pavlov" < notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In src/nvim/lua/executor.c <#6704 (comment)>: > @@ -373,6 +373,46 @@ static int nlua_eval_lua_string(lua_State *const lstate) return 0; } +/// Evaluate lua string +/// +/// Expects four values on the stack: string to evaluate, pointer to args array, +/// and locations where result and error are saved, respectively. Always +/// returns nothing (from the lua point of view). +static int nlua_exec_lua_string_api(lua_State *const lstate) + FUNC_ATTR_NONNULL_ALL +{ + const String *str = (const String *)lua_touserdata(lstate, 1); + const Array *args = (const Array *)lua_touserdata(lstate, 2); + Object *retval = (Object *)lua_touserdata(lstate, 3); + Error *err = (Error *)lua_touserdata(lstate, 4); + + lua_pop(lstate, 3); This should be 4. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6704 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTMoJKm8Pwz8WkMjLeVbzwyRWFbsGaVks5r4tccgaJpZM4NU5hw> .

brcolow · 2017-05-11T23:06:56Z

src/nvim/api/vim.c

@@ -254,6 +255,25 @@ Object nvim_call_function(String fname, Array args, Error *err)
  return rv;
 }

+/// Execute lua code. Parameters might be passed, they are available inside
+/// the chunk as `...`. They chunk can return a value.


"They chunk" => "The chunk"

brcolow · 2017-05-11T23:07:48Z

src/nvim/api/vim.c

+/// the chunk as `...`. They chunk can return a value.
+///
+/// To evaluate an expression, it must be prefixed with "return ".
+/// For instance, to call a lua function with arguments and return value,


"return value" => "return a value"

Or maybe "return its' value" ? (it being the function's result...)

"arguments and return value" should have been a noun phrase, but as was ambiguous (even if both readings kinda make sense, as you see) I will rewrite it

bfredl · 2017-05-12T18:01:38Z

coredump in oldtests test 17, probably unrelated:

++ lldb -Q -o 'bt all' -f /Users/travis/build/neovim/neovim/build/bin/nvim -c /cores//core.20212
(lldb) target create "/Users/travis/build/neovim/neovim/build/bin/nvim" --core "/cores//core.20212"
warning: (x86_64) /cores/core.20212 load command 104 LC_SEGMENT_64 has a fileoff + filesize (0x2b9d7000) that extends beyond the end of the file (0x2b9d6000), the segment will be truncated to match
warning: (x86_64) /cores/core.20212 load command 105 LC_SEGMENT_64 has a fileoff (0x2b9d7000) that extends beyond the end of the file (0x2b9d6000), ignoring this section
Core file '/cores/core.20212' (x86_64) was loaded.
(lldb) bt all
* thread #1: tid = 0x0000, 0x00007fff8ee938ba libsystem_platform.dylib`_platform_strcmp + 186, stop reason = signal SIGSTOP
  * frame #0: 0x00007fff8ee938ba libsystem_platform.dylib`_platform_strcmp + 186
    frame #1: 0x000000010f497b48 nvim`in_history(type=0, str="call delete(\"./Xbase.a\")", move_to_front=1, sep=0) + 168 at ex_getln.c:4621
    frame #2: 0x000000010f497983 nvim`add_to_history(histype=0, new_entry="call delete(\"./Xbase.a\")", in_map=1, sep=0) + 323 at ex_getln.c:4727
    frame #3: 0x000000010f492d53 nvim`command_line_enter(firstc=58, count=1, indent=0) + 2035 at ex_getln.c:302
    frame #4: 0x000000010f492551 nvim`getcmdline(firstc=58, count=1, indent=0) + 33 at ex_getln.c:1672
    frame #5: 0x000000010f49328a nvim`getexline(c=58, cookie=0x0000000000000000, indent=0) + 74 at ex_getln.c:1839
    frame #6: 0x000000010f473cc4 nvim`do_cmdline(cmdline=0x0000000000000000, fgetline=(nvim`getexline at ex_getln.c:1835), cookie=0x0000000000000000, flags=0) + 2068 at ex_docmd.c:526
    frame #++ test /Users/travis/build/neovim/neovim/build/bin/nvim '!=' quiet

justinmk · 2017-05-13T11:41:34Z

Any objection to naming it simply nvim_lua ?

bfredl · 2017-05-13T11:42:52Z

methods should be preferably be verb pharses. lua is not a verb.

FEATURES: 0e873a3 Lua(Jit) built-in neovim#4411 5b32bce Windows: `:terminal` neovim#7007 7b0ceb3 UI/API: externalize cmdline neovim#7173 b67f58b UI/API: externalize wildmenu neovim#7454 b23aa1c UI: 'winhighlight' neovim#6597 17531ed UI: command-line coloring (`:help input()-highlight`) neovim#6364 244a1f9 API: execute lua directly from the remote api neovim#6704 45626de API: `get_keymap()` neovim#6236 db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` neovim#7082 dc68538 menu_get() function neovim#6322 9db42d4 :cquit : take an error code argument neovim#7336 9cc185d job-control: serverstart(): support ipv6 neovim#6680 1b7a9bf job-control: sockopen() neovim#6594 6efe84a clipboard: fallback to tmux clipboard neovim#6894 6016ac2 clipboard: customize clipboard with `g:clipboard` neovim#6030 3a86dd5 ruby: override ruby host via `g:ruby_host_prog` neovim#6841 16cce1a debug: $NVIM_LOG_FILE neovim#6827 0cba3da `:checkhealth` built-in, validates $VIMRUNTIME neovim#7399 FIXES: 105d680 TUI: more terminals, improve scroll/resize neovim#6816 cb912a3 :terminal : handle F1-F12, other keys neovim#7241 619838f inccommand: improve performance neovim#6949 04b3c32 inccommand: Fix matches for zero-width neovim#7487 60b1e8a inccommand: multiline, other fixes neovim#7315 f1f7f3b inccommand: Ignore leading modifiers in the command neovim#6967 1551f71 inccommand: fix 'gdefault' lockup neovim#7262 6338199 API: bufhl: support creating new groups neovim#7414 541dde3 API: allow K_EVENT during operator-pending 8c732f7 terminal: adjust for 'number' neovim#7440 5bec946 UI: preserve wildmenu during jobs/events neovim#7110 c349083 UI: disable 'lazyredraw' during ui_refresh. neovim#6259 51808a2 send FocusGained/FocusLost event instead of pseudokey neovim#7221 133f8bc shada: preserve unnamed register on restart neovim#4700 1b70a1d shada: avoid assertion on corrupt shada file neovim#6958 9f534f3 mksession: Restore tab-local working directory neovim#6859 de1084f fix buf_write() crash neovim#7140 7f76986 syntax: register 'Normal' highlight group neovim#6973 6e7a8c3 RPC: close channel if stream was closed neovim#7081 85f3084 clipboard: disallow recursion; show hint only once neovim#7203 8d1ccb6 clipboard: performance, avoid weird edge-cases neovim#7193 01487d4 'titleold' neovim#7358 01e53a5 Windows: better path-handling, separator (slash) hygiene neovim#7349 0f2873c Windows: multibyte startup arguments neovim#7060 CHANGES: 9ff0cc7 :terminal : start in normal-mode neovim#6808 032b088 lower priority of 'cursorcolumn', 'colorcolumn' neovim#7364 2a3bcd1 RPC: Don't delay notifications when request is pending neovim#6544 023f67c :terminal : Do not change 'number', 'relativenumber' neovim#6796 1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets neovim#6915 6720fe2 help: `K` tries Vim help instead of manpage neovim#3104 7068370 help, man.vim: change "outline" map to `gO` neovim#7405

ZyX-I reviewed May 9, 2017

View reviewed changes

bfredl force-pushed the luaexec branch 3 times, most recently from 20b6216 to 408fa77 Compare May 11, 2017 06:24

bfredl changed the title ~~[WIP] execute lua directly from the remote API~~ [RFC] execute lua directly from the remote API May 11, 2017

bfredl force-pushed the luaexec branch from 408fa77 to 7f127f7 Compare May 11, 2017 06:25

justinmk reviewed May 11, 2017

View reviewed changes

ZyX-I reviewed May 11, 2017

View reviewed changes

bfredl force-pushed the luaexec branch from 7f127f7 to 675800d Compare May 11, 2017 12:10

bfredl force-pushed the luaexec branch from 675800d to 20f1258 Compare May 11, 2017 12:23

brcolow reviewed May 11, 2017

View reviewed changes

API version bump

12fb634

bfredl force-pushed the luaexec branch from 20f1258 to 6d352fa Compare May 12, 2017 14:00

bfredl force-pushed the luaexec branch 2 times, most recently from b6e0bea to b0b242a Compare May 13, 2017 10:11

justinmk approved these changes May 13, 2017

View reviewed changes

bfredl force-pushed the luaexec branch from b0b242a to f424189 Compare May 13, 2017 13:03

api: execute lua directly from the remote api

f424189

bfredl merged commit 244a1f9 into neovim:master May 13, 2017

justinmk mentioned this pull request Nov 8, 2017

NVIM v0.2.1 #7505

Merged

[RFC] execute lua directly from the remote API #6704

[RFC] execute lua directly from the remote API #6704

Conversation

bfredl commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfredl commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfredl May 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk commented May 11, 2017 • edited

fmoralesc commented May 11, 2017

ZyX-I commented May 11, 2017

justinmk commented May 11, 2017

bfredl commented May 11, 2017

bfredl commented May 11, 2017

bfredl commented May 11, 2017

justinmk commented May 11, 2017

bfredl commented May 11, 2017

ZyX-I commented May 11, 2017

bfredl commented May 11, 2017

ZyX-I commented May 11, 2017

bfredl commented May 11, 2017

justinmk commented May 11, 2017

bfredl commented May 11, 2017

justinmk commented May 11, 2017

bfredl commented May 11, 2017

ZyX-I commented May 11, 2017

ZyX-I commented May 11, 2017

Choose a reason for hiding this comment

bfredl commented May 11, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfredl commented May 12, 2017

justinmk commented May 13, 2017

bfredl commented May 13, 2017

bfredl May 13, 2017 •

edited

justinmk commented May 11, 2017 •

edited