From 167a2383b9966ac227a77b0221088246e14ce75a Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 16 Dec 2024 04:00:20 -0800 Subject: fix(api): not using TRY_WRAP, generic error messages #31595 Problem: - API functions using `try_start` directly instead of `TRY_WRAP`, do not surface the underlying error message, and instead show generic things like "Failed to set buffer". - Error handling code is duplicated in the API impl, instead of delegating to the vim buffer/window handling logic. Solution: - Use `TRY_WRAP`. --- src/nvim/api/private/helpers.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'src/nvim/api/private/helpers.c') diff --git a/src/nvim/api/private/helpers.c b/src/nvim/api/private/helpers.c index 8ddaecc58e..5bf66a092f 100644 --- a/src/nvim/api/private/helpers.c +++ b/src/nvim/api/private/helpers.c @@ -101,9 +101,9 @@ bool try_leave(const TryState *const tstate, Error *const err) return ret; } -/// Start block that may cause vimscript exceptions +/// Starts a block that may cause Vimscript exceptions; must be mirrored by `try_end()` call. /// -/// Each try_start() call should be mirrored by try_end() call. +/// Note: use `TRY_WRAP` instead (except in `FUNC_API_FAST` functions such as nvim_get_runtime_file). /// /// To be used as a replacement of `:try … catch … endtry` in C code, in cases /// when error flag could not already be set. If there may be pending error @@ -114,8 +114,9 @@ void try_start(void) trylevel++; } -/// End try block, set the error message if any and return true if an error -/// occurred. +/// Ends a `try_start` block; sets error message if any and returns true if an error occurred. +/// +/// Note: use `TRY_WRAP` instead (except in `FUNC_API_FAST` functions such as nvim_get_runtime_file). /// /// @param err Pointer to the stack-allocated error object /// @return true if an error occurred -- 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/private/helpers.c | 79 ++++++++++++++---------------------------- 1 file changed, 26 insertions(+), 53 deletions(-) (limited to 'src/nvim/api/private/helpers.c') diff --git a/src/nvim/api/private/helpers.c b/src/nvim/api/private/helpers.c index 5bf66a092f..4389ae3b35 100644 --- a/src/nvim/api/private/helpers.c +++ b/src/nvim/api/private/helpers.c @@ -42,8 +42,10 @@ /// Start block that may cause Vimscript exceptions while evaluating another code /// -/// Used when caller is supposed to be operating when other Vimscript code is being -/// processed and that “other Vimscript code” must not be affected. +/// Used just in case caller is supposed to be operating when other Vimscript code +/// is being processed and that “other Vimscript code” must not be affected. +/// +/// @warning Avoid calling directly; use TRY_WRAP instead. /// /// @param[out] tstate Location where try state should be saved. void try_enter(TryState *const tstate) @@ -55,75 +57,33 @@ void try_enter(TryState *const tstate) .current_exception = current_exception, .msg_list = (const msglist_T *const *)msg_list, .private_msg_list = NULL, - .trylevel = trylevel, .got_int = got_int, .did_throw = did_throw, .need_rethrow = need_rethrow, .did_emsg = did_emsg, }; + // `msg_list` controls the collection of abort-causing non-exception errors, + // which would otherwise be ignored. This pattern is from do_cmdline(). msg_list = &tstate->private_msg_list; current_exception = NULL; - trylevel = 1; got_int = false; did_throw = false; need_rethrow = false; did_emsg = false; -} - -/// End try block, set the error message if any and restore previous state -/// -/// @warning Return is consistent with most functions (false on error), not with -/// try_end (true on error). -/// -/// @param[in] tstate Previous state to restore. -/// @param[out] err Location where error should be saved. -/// -/// @return false if error occurred, true otherwise. -bool try_leave(const TryState *const tstate, Error *const err) - FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT -{ - const bool ret = !try_end(err); - assert(trylevel == 0); - assert(!need_rethrow); - assert(!got_int); - assert(!did_throw); - assert(!did_emsg); - assert(msg_list == &tstate->private_msg_list); - assert(*msg_list == NULL); - assert(current_exception == NULL); - msg_list = (msglist_T **)tstate->msg_list; - current_exception = tstate->current_exception; - trylevel = tstate->trylevel; - got_int = tstate->got_int; - did_throw = tstate->did_throw; - need_rethrow = tstate->need_rethrow; - did_emsg = tstate->did_emsg; - return ret; -} - -/// Starts a block that may cause Vimscript exceptions; must be mirrored by `try_end()` call. -/// -/// Note: use `TRY_WRAP` instead (except in `FUNC_API_FAST` functions such as nvim_get_runtime_file). -/// -/// To be used as a replacement of `:try … catch … endtry` in C code, in cases -/// when error flag could not already be set. If there may be pending error -/// state at the time try_start() is executed which needs to be preserved, -/// try_enter()/try_leave() pair should be used instead. -void try_start(void) -{ trylevel++; } -/// Ends a `try_start` block; sets error message if any and returns true if an error occurred. +/// Ends a `try_enter` block; sets error message if any. /// -/// Note: use `TRY_WRAP` instead (except in `FUNC_API_FAST` functions such as nvim_get_runtime_file). +/// @warning Avoid calling directly; use TRY_WRAP instead. /// -/// @param err Pointer to the stack-allocated error object -/// @return true if an error occurred -bool try_end(Error *err) +/// @param[out] err Pointer to the stack-allocated error object +void try_leave(const TryState *const tstate, Error *const err) + FUNC_ATTR_NONNULL_ALL { // Note: all globals manipulated here should be saved/restored in // try_enter/try_leave. + assert(trylevel > 0); trylevel--; // Set by emsg(), affects aborting(). See also enter_cleanup(). @@ -166,7 +126,20 @@ bool try_end(Error *err) discard_current_exception(); } - return ERROR_SET(err); + assert(msg_list == &tstate->private_msg_list); + assert(*msg_list == NULL); + assert(current_exception == NULL); + assert(!got_int); + assert(!did_throw); + assert(!need_rethrow); + assert(!did_emsg); + // Restore the exception context. + msg_list = (msglist_T **)tstate->msg_list; + current_exception = tstate->current_exception; + got_int = tstate->got_int; + did_throw = tstate->did_throw; + need_rethrow = tstate->need_rethrow; + did_emsg = tstate->did_emsg; } /// Recursively expands a vimscript value in a dict -- 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/private/helpers.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'src/nvim/api/private/helpers.c') diff --git a/src/nvim/api/private/helpers.c b/src/nvim/api/private/helpers.c index 4389ae3b35..78aa7c00f7 100644 --- a/src/nvim/api/private/helpers.c +++ b/src/nvim/api/private/helpers.c @@ -31,13 +31,10 @@ #include "nvim/msgpack_rpc/unpacker.h" #include "nvim/pos_defs.h" #include "nvim/types_defs.h" -#include "nvim/ui.h" -#include "nvim/ui_defs.h" -#include "nvim/version.h" #ifdef INCLUDE_GENERATED_DECLARATIONS # include "api/private/api_metadata.generated.h" -# include "api/private/helpers.c.generated.h" +# include "api/private/helpers.c.generated.h" // IWYU pragma: keep #endif /// Start block that may cause Vimscript exceptions while evaluating another code -- cgit From ead5683ff9994c0fbfc6c38e0911d9455777550b Mon Sep 17 00:00:00 2001 From: Luuk van Baal Date: Tue, 7 Jan 2025 14:20:45 +0100 Subject: feat(api): add err field to nvim_echo() opts Problem: We want to deprecate `nvim_err_write(ln)()` but there is no obvious replacement (from Lua). Meanwhile we already have `nvim_echo()` with an `opts` argument. Solution: Add `err` argument to `nvim_echo()` that directly maps to `:echoerr`. --- src/nvim/api/private/helpers.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/nvim/api/private/helpers.c') diff --git a/src/nvim/api/private/helpers.c b/src/nvim/api/private/helpers.c index 78aa7c00f7..c98635f8fd 100644 --- a/src/nvim/api/private/helpers.c +++ b/src/nvim/api/private/helpers.c @@ -776,7 +776,7 @@ char *api_typename(ObjectType t) UNREACHABLE; } -HlMessage parse_hl_msg(Array chunks, Error *err) +HlMessage parse_hl_msg(Array chunks, bool is_err, Error *err) { HlMessage hl_msg = KV_INITIAL_VALUE; for (size_t i = 0; i < chunks.size; i++) { @@ -791,7 +791,7 @@ HlMessage parse_hl_msg(Array chunks, Error *err) String str = copy_string(chunk.items[0].data.string, NULL); - int hl_id = 0; + int hl_id = is_err ? HLF_E : 0; if (chunk.size == 2) { hl_id = object_to_hl_id(chunk.items[1], "text highlight", err); } -- cgit