fix(api): nvim_parse_cmd error message in pcall() #23297

Problem:
nvim_parse_cmd() in pcall() may show an error message (side-effect):

    :lua pcall(vim.api.nvim_parse_cmd, vim.fn.getcmdline(), {})
    E16: Invalid range

Solution:
Avoid emsg() in the nvim_parse_cmd() codepath.

- refactor(api): add error message output parameter to get_address()
- fix: null check emsg() parameter
- refactor: remove emsg_off workaround from do_incsearch_highlighting()
- refactor: remove emsg_off workaround from cmdpreview_may_show()
- refactor: remove remaining calls to emsg() from parse_cmd_address() and get_address()
- (refactor): lint set_cmd_dflall_range()
- refactor: addr_error() - move output parameter to return value

Fix #20339

TODO:

These are the functions called by `get_address()`:

```
nvim_parse_cmd() -> parse_cmdline() -> parse_cmd_address() -> get_address()
    skipwhite()
    addr_error()
    qf_get_cur_idx()
    qf_get_cur_valid_idx()
    qf_get_size()
    qf_get_valid_size()
    mark_get()
    mark_check()
    assert()
    skip_regexp()
    magic_isset()
>   do_search()
>   searchit()
    ascii_isdigit()
    getdigits()
    getdigits_int32()
    compute_buffer_local_count()
    hasFolding()
```

From these functions, I found at least two that call emsg directly:
- do_search()
  - seems to be simple to refactor
- searchit()
  - will be more challenging because it may generate multiple error messages,
    which can't be handled by the current `errormsg` out-parameter.
    For example, it makes multiple calls to `vim_regexec_multi()` in a loop that
    possibly generate error messages, and later `searchit()` itself may generate
    another one:
    - c194acbfc4/src/nvim/search.c (L631-L647)
    - c194acbfc4/src/nvim/search.c (L939-L954)

---------

Co-authored-by: Justin M. Keyes <justinkz@gmail.com>

src/nvim/api/command.c

@@ -109,7 +109,7 @@ Dictionary nvim_parse_cmd(String str, Dictionary opts, Error *err)
   exarg_T ea;
   CmdParseInfo cmdinfo;
   char *cmdline = string_to_cstr(str);
-  char *errormsg = NULL;
+  const char *errormsg = NULL;
 
   if (!parse_cmdline(cmdline, &ea, &cmdinfo, &errormsg)) {
     if (errormsg != NULL) {

src/nvim/eval/funcs.c

@@ -1716,7 +1716,7 @@ static void f_expand(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
       emsg_off++;
     }
     size_t len;
-    char *errormsg = NULL;
+    const char *errormsg = NULL;
     char *result = eval_vars((char *)s, s, &len, NULL, &errormsg, NULL, false);
     if (p_verbose == 0) {
       emsg_off--;
@@ -1781,7 +1781,7 @@ static void f_menu_get(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
 /// Expand all the special characters in a command string.
 static void f_expandcmd(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
 {
-  char *errormsg = NULL;
+  const char *errormsg = NULL;
   bool emsgoff = true;
 
   if (argvars[1].v_type == VAR_DICT

src/nvim/ex_docmd.c

@@ -1342,7 +1342,7 @@ void set_cmd_count(exarg_T *eap, linenr_T count, bool validate)
   }
 }
 
-static int parse_count(exarg_T *eap, char **errormsg, bool validate)
+static int parse_count(exarg_T *eap, const char **errormsg, bool validate)
 {
   // Check for a count.  When accepting a EX_BUFNAME, don't use "123foo" as a
   // count, it's a buffer name.
@@ -1396,7 +1396,7 @@ bool is_cmd_ni(cmdidx_T cmdidx)
 /// @param[out] errormsg Error message, if any
 ///
 /// @return Success or failure
-bool parse_cmdline(char *cmdline, exarg_T *eap, CmdParseInfo *cmdinfo, char **errormsg)
+bool parse_cmdline(char *cmdline, exarg_T *eap, CmdParseInfo *cmdinfo, const char **errormsg)
 {
   char *after_modifier = NULL;
   bool retval = false;
@@ -1564,7 +1564,7 @@ static void shift_cmd_args(exarg_T *eap)
   xfree(oldarglens);
 }
 
-static int execute_cmd0(int *retv, exarg_T *eap, char **errormsg, bool preview)
+static int execute_cmd0(int *retv, exarg_T *eap, const char **errormsg, bool preview)
 {
   // If filename expansion is enabled, expand filenames
   if (eap->argt & EX_XFILE) {
@@ -1649,7 +1649,7 @@ static int execute_cmd0(int *retv, exarg_T *eap, char **errormsg, bool preview)
 /// @param preview Execute command preview callback instead of actual command
 int execute_cmd(exarg_T *eap, CmdParseInfo *cmdinfo, bool preview)
 {
-  char *errormsg = NULL;
+  const char *errormsg = NULL;
   int retv = 0;
 
 #undef ERROR
@@ -1881,7 +1881,7 @@ static bool skip_cmd(const exarg_T *eap)
 static char *do_one_cmd(char **cmdlinep, int flags, cstack_T *cstack, LineGetter fgetline,
                         void *cookie)
 {
-  char *errormsg = NULL;  // error message
+  const char *errormsg = NULL;  // error message
   const int save_reg_executing = reg_executing;
   const bool save_pending_end_reg_executing = pending_end_reg_executing;
 
@@ -2376,7 +2376,7 @@ char *ex_errmsg(const char *const msg, const char *const arg)
 /// - set 'eventignore' to "all" for ":noautocmd"
 ///
 /// @return  FAIL when the command is not to be executed.
-int parse_command_modifiers(exarg_T *eap, char **errormsg, cmdmod_T *cmod, bool skip_only)
+int parse_command_modifiers(exarg_T *eap, const char **errormsg, cmdmod_T *cmod, bool skip_only)
 {
   CLEAR_POINTER(cmod);
 
@@ -2557,7 +2557,10 @@ int parse_command_modifiers(exarg_T *eap, char **errormsg, cmdmod_T *cmod, bool
       if (checkforcmd(&p, "tab", 3)) {
         if (!skip_only) {
           int tabnr = (int)get_address(eap, &eap->cmd, ADDR_TABS, eap->skip, skip_only,
-                                       false, 1);
+                                       false, 1, errormsg);
+          if (eap->cmd == NULL) {
+            return false;
+          }
 
           if (tabnr == MAXLNUM) {
             cmod->cmod_tab = tabpage_index(curtab) + 1;
@@ -2701,7 +2704,7 @@ void undo_cmdmod(cmdmod_T *cmod)
 /// May set the last search pattern, unless "silent" is true.
 ///
 /// @return  FAIL and set "errormsg" or return OK.
-int parse_cmd_address(exarg_T *eap, char **errormsg, bool silent)
+int parse_cmd_address(exarg_T *eap, const char **errormsg, bool silent)
   FUNC_ATTR_NONNULL_ALL
 {
   int address_count = 1;
@@ -2715,7 +2718,7 @@ int parse_cmd_address(exarg_T *eap, char **errormsg, bool silent)
     eap->line2 = get_cmd_default_range(eap);
     eap->cmd = skipwhite(eap->cmd);
     lnum = get_address(eap, &eap->cmd, eap->addr_type, eap->skip, silent,
-                       eap->addr_count == 0, address_count++);
+                       eap->addr_count == 0, address_count++, errormsg);
     if (eap->cmd == NULL) {  // error detected
       goto theend;
     }
@@ -2794,13 +2797,13 @@ int parse_cmd_address(exarg_T *eap, char **errormsg, bool silent)
         eap->cmd++;
         if (!eap->skip) {
           fmark_T *fm = mark_get_visual(curbuf, '<');
-          if (!mark_check(fm)) {
+          if (!mark_check(fm, errormsg)) {
             goto theend;
           }
           assert(fm != NULL);
           eap->line1 = fm->mark.lnum;
           fm = mark_get_visual(curbuf, '>');
-          if (!mark_check(fm)) {
+          if (!mark_check(fm, errormsg)) {
             goto theend;
           }
           assert(fm != NULL);
@@ -3229,29 +3232,30 @@ char *skip_range(const char *cmd, int *ctx)
   return (char *)cmd;
 }
 
-static void addr_error(cmd_addr_T addr_type)
+static const char *addr_error(cmd_addr_T addr_type)
 {
   if (addr_type == ADDR_NONE) {
-    emsg(_(e_norange));
+    return _(e_norange);
   } else {
-    emsg(_(e_invrange));
+    return _(e_invrange);
   }
 }
 
-/// Get a single EX address
+/// Gets a single EX address.
 ///
-/// Set ptr to the next character after the part that was interpreted.
+/// Sets ptr to the next character after the part that was interpreted.
-/// Set ptr to NULL when an error is encountered.
+/// Sets ptr to NULL when an error is encountered (stored in `errormsg`).
-/// This may set the last used search pattern.
+/// May set the last used search pattern.
 ///
 /// @param skip           only skip the address, don't use it
 /// @param silent         no errors or side effects
 /// @param to_other_file  flag: may jump to other file
 /// @param address_count  1 for first, >1 after comma
+/// @param errormsg       Error message, if any
 ///
 /// @return               MAXLNUM when no Ex address was found.
 static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int skip, bool silent,
-                            int to_other_file, int address_count)
+                            int to_other_file, int address_count, const char **errormsg)
   FUNC_ATTR_NONNULL_ALL
 {
   int c;
@@ -3287,7 +3291,7 @@ static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int
       case ADDR_NONE:
       case ADDR_TABS_RELATIVE:
       case ADDR_UNSIGNED:
-        addr_error(addr_type);
+        *errormsg = addr_error(addr_type);
         cmd = NULL;
         goto error;
         break;
@@ -3332,7 +3336,7 @@ static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int
       case ADDR_NONE:
       case ADDR_TABS_RELATIVE:
       case ADDR_UNSIGNED:
-        addr_error(addr_type);
+        *errormsg = addr_error(addr_type);
         cmd = NULL;
         goto error;
         break;
@@ -3357,7 +3361,7 @@ static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int
         goto error;
       }
       if (addr_type != ADDR_LINES) {
-        addr_error(addr_type);
+        *errormsg = addr_error(addr_type);
         cmd = NULL;
         goto error;
       }
@@ -3373,7 +3377,7 @@ static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int
           // Jumped to another file.
           lnum = curwin->w_cursor.lnum;
         } else {
-          if (!mark_check(fm)) {
+          if (!mark_check(fm, errormsg)) {
             cmd = NULL;
             goto error;
           }
@@ -3387,7 +3391,7 @@ static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int
     case '?':                           // '/' or '?' - search
       c = (uint8_t)(*cmd++);
       if (addr_type != ADDR_LINES) {
-        addr_error(addr_type);
+        *errormsg = addr_error(addr_type);
         cmd = NULL;
         goto error;
       }
@@ -3436,7 +3440,7 @@ static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int
     case '\\':                      // "\?", "\/" or "\&", repeat search
       cmd++;
       if (addr_type != ADDR_LINES) {
-        addr_error(addr_type);
+        *errormsg = addr_error(addr_type);
         cmd = NULL;
         goto error;
       }
@@ -3445,7 +3449,7 @@ static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int
       } else if (*cmd == '?' || *cmd == '/') {
         i = RE_SEARCH;
       } else {
-        emsg(_(e_backslash));
+        *errormsg = _(e_backslash);
         cmd = NULL;
         goto error;
       }
@@ -3527,13 +3531,13 @@ static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int
         // "number", "+number" or "-number"
         n = getdigits_int32(&cmd, false, MAXLNUM);
         if (n == MAXLNUM) {
-          emsg(_(e_line_number_out_of_range));
+          *errormsg = _(e_line_number_out_of_range);
           goto error;
         }
       }
 
       if (addr_type == ADDR_TABS_RELATIVE) {
-        emsg(_(e_invrange));
+        *errormsg = _(e_invrange);
         cmd = NULL;
         goto error;
       } else if (addr_type == ADDR_LOADED_BUFFERS || addr_type == ADDR_BUFFERS) {
@@ -3549,7 +3553,7 @@ static linenr_T get_address(exarg_T *eap, char **ptr, cmd_addr_T addr_type, int
           lnum -= n;
         } else {
           if (n >= INT32_MAX - lnum) {
-            emsg(_(e_line_number_out_of_range));
+            *errormsg = _(e_line_number_out_of_range);
             goto error;
           }
           lnum += n;
@@ -3762,7 +3766,7 @@ char *replace_makeprg(exarg_T *eap, char *arg, char **cmdlinep)
 /// When an error is detected, "errormsgp" is set to a non-NULL pointer.
 ///
 /// @return  FAIL for failure, OK otherwise.
-int expand_filename(exarg_T *eap, char **cmdlinep, char **errormsgp)
+int expand_filename(exarg_T *eap, char **cmdlinep, const char **errormsgp)
 {
   // Skip a regexp pattern for ":vimgrep[add] pat file..."
   char *p = skip_grep_pat(eap);
@@ -5855,8 +5859,12 @@ static void ex_put(exarg_T *eap)
 /// Handle ":copy" and ":move".
 static void ex_copymove(exarg_T *eap)
 {
-  long n = get_address(eap, &eap->arg, eap->addr_type, false, false, false, 1);
+  const char *errormsg = NULL;
+  long n = get_address(eap, &eap->arg, eap->addr_type, false, false, false, 1, &errormsg);
   if (eap->arg == NULL) {  // error detected
+    if (errormsg != NULL) {
+      emsg(errormsg);
+    }
     eap->nextcmd = NULL;
     return;
   }
@@ -6770,8 +6778,8 @@ ssize_t find_cmdline_var(const char *src, size_t *usedlen)
 /// @return          an allocated string if a valid match was found.
 ///                  Returns NULL if no match was found.  "usedlen" then still contains the
 ///                  number of characters to skip.
-char *eval_vars(char *src, const char *srcstart, size_t *usedlen, linenr_T *lnump, char **errormsg,
+char *eval_vars(char *src, const char *srcstart, size_t *usedlen, linenr_T *lnump,
-                int *escaped, bool empty_is_error)
+                const char **errormsg, int *escaped, bool empty_is_error)
 {
   char *result;
   char *resultbuf = NULL;
@@ -7044,7 +7052,7 @@ char *expand_sfile(char *arg)
     } else {
       // replace "<sfile>" with the sourced file name, and do ":" stuff
       size_t srclen;
-      char *errormsg;
+      const char *errormsg;
       char *repl = eval_vars(p, result, &srclen, NULL, &errormsg, NULL, true);
       if (errormsg != NULL) {
         if (*errormsg) {

src/nvim/ex_getln.c

@@ -236,7 +236,7 @@ static bool do_incsearch_highlighting(int firstc, int *search_delim, incsearch_s
   bool delim_optional = false;
   int delim;
   char *end;
-  char *dummy;
+  const char *dummy;
   pos_T save_cursor;
   bool use_last_pat;
   bool retval = false;
@@ -261,7 +261,6 @@ static bool do_incsearch_highlighting(int firstc, int *search_delim, incsearch_s
     return false;
   }
 
-  emsg_off++;
   exarg_T ea = {
     .line1 = 1,
     .line2 = 1,
@@ -369,7 +368,6 @@ static bool do_incsearch_highlighting(int firstc, int *search_delim, incsearch_s
   curwin->w_cursor = save_cursor;
   retval = true;
 theend:
-  emsg_off--;
   return retval;
 }
 
@@ -2428,13 +2426,10 @@ static bool cmdpreview_may_show(CommandLineState *s)
   // Copy the command line so we can modify it.
   int cmdpreview_type = 0;
   char *cmdline = xstrdup(ccline.cmdbuff);
-  char *errormsg = NULL;
+  const char *errormsg = NULL;
-  emsg_off++;  // Block errors when parsing the command line, and don't update v:errmsg
   if (!parse_cmdline(cmdline, &ea, &cmdinfo, &errormsg)) {
-    emsg_off--;
     goto end;
   }
-  emsg_off--;
 
   // Check if command is previewable, if not, don't attempt to show preview
   if (!(ea.argt & EX_PREVIEW)) {

src/nvim/mark.c

@@ -541,7 +541,11 @@ MarkMoveRes mark_move_to(fmark_T *fm, MarkMove flags)
 {
   static fmark_T fm_copy = INIT_FMARK;
   MarkMoveRes res = kMarkMoveSuccess;
-  if (!mark_check(fm)) {
+  const char *errormsg = NULL;
+  if (!mark_check(fm, &errormsg)) {
+    if (errormsg != NULL) {
+      emsg(errormsg);
+    }
     res = kMarkMoveFailed;
     goto end;
   }
@@ -557,7 +561,10 @@ MarkMoveRes mark_move_to(fmark_T *fm, MarkMove flags)
       goto end;
     }
     // Check line count now that the **destination buffer is loaded**.
-    if (!mark_check_line_bounds(curbuf, fm)) {
+    if (!mark_check_line_bounds(curbuf, fm, &errormsg)) {
+      if (errormsg != NULL) {
+        emsg(errormsg);
+      }
       res |= kMarkMoveFailed;
       goto end;
     }
@@ -710,43 +717,45 @@ static void fmarks_check_one(xfmark_T *fm, char *name, buf_T *buf)
 
 /// Check the position in @a fm is valid.
 ///
-/// Emit error message and return accordingly.
-///
 /// Checks for:
 /// - NULL raising unknown mark error.
 /// - Line number <= 0 raising mark not set.
 /// - Line number > buffer line count, raising invalid mark.
+///
 /// @param fm[in]  File mark to check.
+/// @param errormsg[out]  Error message, if any.
 ///
 /// @return  true if the mark passes all the above checks, else false.
-bool mark_check(fmark_T *fm)
+bool mark_check(fmark_T *fm, const char **errormsg)
 {
   if (fm == NULL) {
-    emsg(_(e_umark));
+    *errormsg = _(e_umark);
     return false;
   } else if (fm->mark.lnum <= 0) {
     // In both cases it's an error but only raise when equals to 0
     if (fm->mark.lnum == 0) {
-      emsg(_(e_marknotset));
+      *errormsg = _(e_marknotset);
     }
     return false;
   }
   // Only check for valid line number if the buffer is loaded.
-  if (fm->fnum == curbuf->handle && !mark_check_line_bounds(curbuf, fm)) {
+  if (fm->fnum == curbuf->handle && !mark_check_line_bounds(curbuf, fm, errormsg)) {
     return false;
   }
   return true;
 }
 
 /// Check if a mark line number is greater than the buffer line count, and set e_markinval.
+///
 /// @note  Should be done after the buffer is loaded into memory.
 /// @param buf  Buffer where the mark is set.
 /// @param fm  Mark to check.
+/// @param errormsg[out]  Error message, if any.
 /// @return  true if below line count else false.
-bool mark_check_line_bounds(buf_T *buf, fmark_T *fm)
+bool mark_check_line_bounds(buf_T *buf, fmark_T *fm, const char **errormsg)
 {
   if (buf != NULL && fm->mark.lnum > buf->b_ml.ml_line_count) {
-    emsg(_(e_markinval));
+    *errormsg = _(e_markinval);
     return false;
   }
   return true;

src/nvim/path.c

@@ -2129,7 +2129,7 @@ int expand_wildcards_eval(char **pat, int *num_file, char ***file, int flags)
   int ret = FAIL;
   char *eval_pat = NULL;
   char *exp_pat = *pat;
-  char *ignored_msg;
+  const char *ignored_msg;
   size_t usedlen;
   const bool is_cur_alt_file = *exp_pat == '%' || *exp_pat == '#';
   bool star_follows = false;

src/nvim/syntax.c

@@ -3979,7 +3979,7 @@ static void syn_cmd_include(exarg_T *eap, int syncing)
   int sgl_id = 1;
   char *group_name_end;
   char *rest;
-  char *errormsg = NULL;
+  const char *errormsg = NULL;
   int prev_toplvl_grp;
   int prev_syn_inc_tag;
   bool source = false;

test/functional/api/vim_spec.lua

@@ -3467,6 +3467,7 @@ describe('API', function()
       end)
     end)
   end)
+
   describe('nvim_parse_cmd', function()
     it('works', function()
       eq({
@@ -4048,7 +4049,13 @@ describe('API', function()
       meths.cmd(meths.parse_cmd("set cursorline", {}), {})
       eq(true, meths.get_option_value("cursorline", {}))
     end)
+    it('no side-effects (error messages) in pcall() #20339', function()
+      eq({ false, 'Error while parsing command line: E16: Invalid range' },
+        exec_lua([=[return {pcall(vim.api.nvim_parse_cmd, "'<,'>n", {})}]=]))
+      eq('', eval('v:errmsg'))
     end)
+  end)
+
   describe('nvim_cmd', function()
     it('works', function ()
       meths.cmd({ cmd = "set", args = { "cursorline" } }, {})

test/functional/editor/mark_spec.lua

@@ -161,7 +161,7 @@ describe('named marks', function()
     feed('ifoo<Esc>mA')
     command('enew')
     feed('ibar<Esc>')
-    eq('Vim(print):E20: Mark not set', pcall_err(command, [['Aprint]]))
+    eq("Vim(print):E20: Mark not set: 'Aprint", pcall_err(command, [['Aprint]]))
   end)
 
   it("leave a context mark when moving with '", function()