[vim/vim] if_lua: stop on errors in vim.command() and vim.call() (#8678)
Martin Tournoij
This stops execution if vim.command() or vim.call() failed. This fixes
two problems:
The first is the lack of line numbers if something failed:
Error detected while processing command line..function <SNR>48_Cmd:
line 13:
E1174: String required for argument 2
E1174: String required for argument 2
E1174: String required for argument 2
E1174: String required for argument 2
[..repeated many times..]
It's not at all clear what command failed where, and vim.command()
always returns 0. With this, it's:
Error detected while processing command line..function <SNR>49_Cmd:
line 13:
E1174: String required for argument 2
lpeg.vim/lua/lpeg.lua:131: lua: function failed
Which is much more useful.
The second is that if you have some error in a loop you'll be spammed with
errors; this is #8649
The big downside with this patch is that it changes the behaviour; I don't know
if any scripts relied on execution continuing after errors, but they might.
However, Lua itself stops execution out if a (Lua) function failed, vim.eval()
did as well, and it generally just makes sense IMO. It's really confusing and
surprising otherwise
An alternative might be actually returning something from luaV_command();
luaV_call() already does so. You'll have to add "if not vim.command(...) then
error("command failed") end" to everything, which doesn't seem very "Lua-like".
Another point of improvement is that the errors for "vim.fn" are
confusing; for the same as above it prints:
Error detected while processing command line..function <SNR>48_Cmd:
line 13:
E1174: String required for argument 2
[string "vim.fn = setmetatable({}, {..."]:4: lua: call_vim_function failed
Which isn't very good. Need to look in to that.
(I'll update the tests and docs later once the behaviour is decided)
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/8678
Commit Summary
- if_lua: stop on errors in vim.command() and vim.call()
File Changes
- M src/if_lua.c (13)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
codecov[bot]
Codecov Report
Merging #8678 (f6fe0b3) into master (890ee4e) will decrease coverage by
87.64%.
The diff coverage is0.00%.
@@ Coverage Diff @@ ## master #8678 +/- ## =========================================== - Coverage 90.11% 2.46% -87.65% =========================================== Files 150 148 -2 Lines 170208 164983 -5225 =========================================== - Hits 153384 4072 -149312 - Misses 16824 160911 +144087
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | ? |
|
| huge-gcc-none | ? |
|
| huge-gcc-testgui | ? |
|
| huge-gcc-unittests | 2.46% <0.00%> (-0.01%) |
⬇️ |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/if_lua.c | 0.00% <0.00%> (-93.31%) |
⬇️ |
| src/float.c | 0.00% <0.00%> (-99.16%) |
⬇️ |
| src/gui_gtk_f.c | 0.00% <0.00%> (-97.54%) |
⬇️ |
| src/crypt_zip.c | 0.00% <0.00%> (-97.06%) |
⬇️ |
| src/match.c | 0.00% <0.00%> (-96.98%) |
⬇️ |
| src/sha256.c | 0.00% <0.00%> (-96.94%) |
⬇️ |
| src/evalbuffer.c | 0.00% <0.00%> (-96.92%) |
⬇️ |
| src/textprop.c | 0.00% <0.00%> (-96.80%) |
⬇️ |
| src/cmdhist.c | 0.00% <0.00%> (-96.76%) |
⬇️ |
| src/debugger.c | 0.00% <0.00%> (-96.62%) |
⬇️ |
| ... and 137 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 890ee4e...f6fe0b3. Read the comment docs.
Bram Moolenaar
Martin Tournoij wrote:
> This stops execution if vim.command() or vim.call() failed. This fixes
> two problems:
>
> The first is the lack of line numbers if something failed:
>
> Error detected while processing command line..function <SNR>48_Cmd:
> line 13:
> E1174: String required for argument 2
> E1174: String required for argument 2
> E1174: String required for argument 2
> E1174: String required for argument 2
> [..repeated many times..]
>
> always returns 0. With this, it's:
>
> Error detected while processing command line..function <SNR>49_Cmd:
> line 13:
> E1174: String required for argument 2
> lpeg.vim/lua/lpeg.lua:131: lua: function failed
>
> Which is much more useful.
>
> The second is that if you have some error in a loop you'll be spammed with
> errors; this is #8649
>
>
> The big downside with this patch is that it changes the behaviour; I
> don't know if any scripts relied on execution continuing after errors,
> but they might.
users. Unless it happens very infrequent perhaps.
The argument that it keeps going and spits out tons of errors is a
strong one. Perhaps that's worth taking a bit of a risk of breaking
something that relies on ignoring errors.
> However, Lua itself stops execution out if a (Lua) function failed, vim.eval()
> did as well, and it generally just makes sense IMO. It's really confusing and
> surprising otherwise
>
> An alternative might be actually returning something from luaV_command();
> luaV_call() already does so. You'll have to add "if not vim.command(...) then
> error("command failed") end" to everything, which doesn't seem very "Lua-like".
>
> Another point of improvement is that the errors for "vim.fn" are
> confusing; for the same as above it prints:
>
> Error detected while processing command line..function <SNR>48_Cmd:
> line 13:
> E1174: String required for argument 2
> [string "vim.fn = setmetatable({}, {..."]:4: lua: call_vim_function failed
>
> Which isn't very good. Need to look in to that.
>
> (I'll update the tests and docs later once the behaviour is decided)
luaL_error(L, "lua: call_vim_command failed");
Does the string get included somehow? Otherwise it's not very clear.
--
Microsoft's definition of a boolean: TRUE, FALSE, MAYBE
"Embrace and extend"...?
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Martin Tournoij
When using something like this:
luaL_error(L, "lua: call_vim_command failed");
Does the string get included somehow? Otherwise it's not very clear.
It relies on the error message from the error message from do_cmdline_cmd() and call_vim_function(); I tried copying the way Python does it with VimTryStart() and VimTryEnd(), but I couldn't get that to work. It's all a bit confusing as there are quite a few global variables and it's not always clear what gets set when, but msg_list was always NULL.
I can look at it again when we decide which way we want to go with this. I'd hate to break people's scripts, but on the other hand the current behaviour is mostly just annoying during development (in your final version you don't really want any errors), and I don't think all that many people are using the Lua integration, so I think it might be okay, but hard to be sure.
We can also add an option maybe, so you can do e.g. vim.fatalerrors() in your Lua script.
Bram Moolenaar
Martin Tournoij wrote:
> > When using something like this:
> >
> > luaL_error(L, "lua: call_vim_command failed");
> >
> > Does the string get included somehow? Otherwise it's not very clear.
>
> It relies on the error message from the error message from
> `do_cmdline_cmd()` and `call_vim_function()`; I tried copying the way
> get that to work. It's all a bit confusing as there are quite a few
> global variables and it's not always clear what gets set when, but
> `msg_list` was always NULL.
to add the command to the error message. That only works if the string
is used right away, not when the pointer is used later.
> I can look at it again when we decide which way we want to go with
> this. I'd hate to break people's scripts, but on the other hand the
> current behaviour is mostly just annoying during development (in your
> final version you don't really want any errors), and I don't think all
> be okay, but hard to be sure.
>
> We can also add an option maybe, so you can do e.g.
> `vim.fatalerrors()` in your Lua script.
Linux is just like a wigwam: no Windows, no Gates and an Apache inside.
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Martin Tournoij
Oh yeah, adding the actual command is easy: luaL_error() already does sprintf-style formatting. l actually I did that before but removed it because I felt it was a bit noisy for longer commands.
Bram Moolenaar
No progress in a while. I do think that adding more verbose errors will help figure out what went wrong.
We will also need to have some tests for the change.
I don't use Lua myself, perhaps someone who does can try it out and give feedback.
Does Neovim have something like this? They use Lua quite a lot.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
Martin Tournoij
@arp242 pushed 1 commit.
- 8b94e5c if_lua: stop on errors in vim.command() and vim.call()
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@arp242 is this still useful?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Martin Tournoij
Closed #8678.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Martin Tournoij
Probably?
I haven't worked with the Lua integration since 2021, so don't really know what the current status of it is.
I don't really have the time to look at this, and don't think I will in the foreseeable future. So I'll just close it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.