From 022449b5223659d515b78bada7de2fac8718820a Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 16 Dec 2024 08:34:16 -0800 Subject: fix(api): generic error messages, not using TRY_WRAP #31596 Problem: - API functions using `try_start` directly, do not surface the underlying error message, and instead show generic messages. - Error-handling code is duplicated in the API impl. - Failure modes are not tested. Solution: - Use `TRY_WRAP`. - Add tests. --- src/nvim/api/vimscript.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) (limited to 'src/nvim/api/vimscript.c') diff --git a/src/nvim/api/vimscript.c b/src/nvim/api/vimscript.c index 165cc93fbe..0ff2b037ce 100644 --- a/src/nvim/api/vimscript.c +++ b/src/nvim/api/vimscript.c @@ -125,19 +125,17 @@ theend: /// /// On execution error: fails with Vimscript error, updates v:errmsg. /// -/// Prefer using |nvim_cmd()| or |nvim_exec2()| over this. To evaluate multiple lines of Vim script -/// or an Ex command directly, use |nvim_exec2()|. To construct an Ex command using a structured -/// format and then execute it, use |nvim_cmd()|. To modify an Ex command before evaluating it, use -/// |nvim_parse_cmd()| in conjunction with |nvim_cmd()|. +/// Prefer |nvim_cmd()| or |nvim_exec2()| instead. To modify an Ex command in a structured way +/// before executing it, modify the result of |nvim_parse_cmd()| then pass it to |nvim_cmd()|. /// /// @param command Ex command string /// @param[out] err Error details (Vim error), if any void nvim_command(String command, Error *err) FUNC_API_SINCE(1) { - try_start(); - do_cmdline_cmd(command.data); - try_end(err); + TRY_WRAP(err, { + do_cmdline_cmd(command.data); + }); } /// Evaluates a Vimscript |expression|. Dicts and Lists are recursively expanded. @@ -283,13 +281,11 @@ Object nvim_call_dict_function(Object dict, String fn, Array args, Arena *arena, bool mustfree = false; switch (dict.type) { case kObjectTypeString: - try_start(); - if (eval0(dict.data.string.data, &rettv, NULL, &EVALARG_EVALUATE) == FAIL) { - api_set_error(err, kErrorTypeException, - "Failed to evaluate dict expression"); - } - clear_evalarg(&EVALARG_EVALUATE, NULL); - if (try_end(err)) { + TRY_WRAP(err, { + eval0(dict.data.string.data, &rettv, NULL, &EVALARG_EVALUATE); + clear_evalarg(&EVALARG_EVALUATE, NULL); + }); + if (ERROR_SET(err)) { return rv; } // Evaluation of the string arg created a new dict or increased the -- cgit From 6bf2a6fc5bb395b67c88cb26d332f882a106c7ab Mon Sep 17 00:00:00 2001 From: luukvbaal Date: Tue, 17 Dec 2024 13:12:22 +0100 Subject: refactor(api): always use TRY_WRAP #31600 Problem: Two separate try/end wrappers, that only marginally differ by restoring a few variables. Wrappers that don't restore previous state are dangerous to use in "api-fast" functions. Solution: Remove wrappers that don't restore the previous state. Always use TRY_WRAP. --- src/nvim/api/vimscript.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) (limited to 'src/nvim/api/vimscript.c') diff --git a/src/nvim/api/vimscript.c b/src/nvim/api/vimscript.c index 0ff2b037ce..67db615b20 100644 --- a/src/nvim/api/vimscript.c +++ b/src/nvim/api/vimscript.c @@ -78,24 +78,24 @@ String exec_impl(uint64_t channel_id, String src, Dict(exec_opts) *opts, Error * capture_ga = &capture_local; } - try_start(); - if (opts->output) { - msg_silent++; - msg_col = 0; // prevent leading spaces - } + TRY_WRAP(err, { + if (opts->output) { + msg_silent++; + msg_col = 0; // prevent leading spaces + } - const sctx_T save_current_sctx = api_set_sctx(channel_id); + const sctx_T save_current_sctx = api_set_sctx(channel_id); - do_source_str(src.data, "nvim_exec2()"); - if (opts->output) { - capture_ga = save_capture_ga; - msg_silent = save_msg_silent; - // Put msg_col back where it was, since nothing should have been written. - msg_col = save_msg_col; - } + do_source_str(src.data, "nvim_exec2()"); + if (opts->output) { + capture_ga = save_capture_ga; + msg_silent = save_msg_silent; + // Put msg_col back where it was, since nothing should have been written. + msg_col = save_msg_col; + } - current_sctx = save_current_sctx; - try_end(err); + current_sctx = save_current_sctx; + }); if (ERROR_SET(err)) { goto theend; -- cgit From f9eb68f340f9c0dbf3b6b2da3ddbab2d5be21b61 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Wed, 18 Dec 2024 06:05:37 -0800 Subject: fix(coverity): error handling CHECKED_RETURN #31618 CID 516406: Error handling issues (CHECKED_RETURN) /src/nvim/api/vimscript.c: 284 in nvim_call_dict_function() 278 Object rv = OBJECT_INIT; 279 280 typval_T rettv; 281 bool mustfree = false; 282 switch (dict.type) { 283 case kObjectTypeString: >>> CID 516406: Error handling issues (CHECKED_RETURN) >>> Calling "eval0" without checking return value (as is done elsewhere 10 out of 12 times). 284 TRY_WRAP(err, { 285 eval0(dict.data.string.data, &rettv, NULL, &EVALARG_EVALUATE); 286 clear_evalarg(&EVALARG_EVALUATE, NULL); 287 }); 288 if (ERROR_SET(err)) { 289 return rv; --- src/nvim/api/vimscript.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) (limited to 'src/nvim/api/vimscript.c') diff --git a/src/nvim/api/vimscript.c b/src/nvim/api/vimscript.c index 67db615b20..e2aac07258 100644 --- a/src/nvim/api/vimscript.c +++ b/src/nvim/api/vimscript.c @@ -229,10 +229,9 @@ static Object _call_function(String fn, Array args, dict_T *self, Arena *arena, funcexe.fe_selfdict = self; TRY_WRAP(err, { - // call_func() retval is deceptive, ignore it. Instead we set `msg_list` - // (see above) to capture abort-causing non-exception errors. - call_func(fn.data, (int)fn.size, &rettv, (int)args.size, - vim_args, &funcexe); + // call_func() retval is deceptive, ignore it. Instead TRY_WRAP sets `msg_list` to capture + // abort-causing non-exception errors. + (void)call_func(fn.data, (int)fn.size, &rettv, (int)args.size, vim_args, &funcexe); }); if (!ERROR_SET(err)) { @@ -280,18 +279,23 @@ Object nvim_call_dict_function(Object dict, String fn, Array args, Arena *arena, typval_T rettv; bool mustfree = false; switch (dict.type) { - case kObjectTypeString: + case kObjectTypeString: { + int eval_ret; TRY_WRAP(err, { - eval0(dict.data.string.data, &rettv, NULL, &EVALARG_EVALUATE); - clear_evalarg(&EVALARG_EVALUATE, NULL); - }); + eval_ret = eval0(dict.data.string.data, &rettv, NULL, &EVALARG_EVALUATE); + clear_evalarg(&EVALARG_EVALUATE, NULL); + }); if (ERROR_SET(err)) { return rv; } + if (eval_ret != OK) { + abort(); // Should not happen. + } // Evaluation of the string arg created a new dict or increased the // refcount of a dict. Not necessary for a RPC dict. mustfree = true; break; + } case kObjectTypeDict: object_to_vim(dict, &rettv, err); break; -- cgit From 2a7d0ed6145bf3f8b139c2694563f460f829813a Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 23 Dec 2024 05:43:52 -0800 Subject: refactor: iwyu #31637 Result of `make iwyu` (after some "fixups"). --- src/nvim/api/vimscript.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/nvim/api/vimscript.c') diff --git a/src/nvim/api/vimscript.c b/src/nvim/api/vimscript.c index e2aac07258..fc7e7e1a06 100644 --- a/src/nvim/api/vimscript.c +++ b/src/nvim/api/vimscript.c @@ -2,6 +2,7 @@ #include #include #include +#include #include #include "klib/kvec.h" @@ -21,6 +22,7 @@ #include "nvim/garray_defs.h" #include "nvim/globals.h" #include "nvim/memory.h" +#include "nvim/memory_defs.h" #include "nvim/runtime.h" #include "nvim/vim_defs.h" #include "nvim/viml/parser/expressions.h" -- cgit