fix(api, lua): handle setting v: variables properly (#25325)

src/nvim/api/private/helpers.c

@@ -21,8 +21,10 @@
 #include "nvim/buffer_defs.h"
 #include "nvim/eval/typval.h"
 #include "nvim/eval/typval_defs.h"
+#include "nvim/eval/vars.h"
 #include "nvim/ex_eval.h"
 #include "nvim/garray.h"
+#include "nvim/globals.h"
 #include "nvim/highlight_group.h"
 #include "nvim/lua/executor.h"
 #include "nvim/map.h"
@@ -235,8 +237,7 @@ Object dict_set_var(dict_T *dict, String key, Object value, bool del, bool retva
     // Delete the key
     if (di == NULL) {
       // Doesn't exist, fail
-      api_set_error(err, kErrorTypeValidation, "Key not found: %s",
+      api_set_error(err, kErrorTypeValidation, "Key not found: %s", key.data);
-                    key.data);
     } else {
       // Notify watchers
       if (watched) {
@@ -265,13 +266,23 @@ Object dict_set_var(dict_T *dict, String key, Object value, bool del, bool retva
       di = tv_dict_item_alloc_len(key.data, key.size);
       tv_dict_add(dict, di);
     } else {
-      if (watched) {
-        tv_copy(&di->di_tv, &oldtv);
-      }
       // Return the old value
       if (retval) {
         rv = vim_to_object(&di->di_tv);
       }
+      bool type_error = false;
+      if (dict == &vimvardict
+          && !before_set_vvar(key.data, di, &tv, true, watched, &type_error)) {
+        tv_clear(&tv);
+        if (type_error) {
+          api_set_error(err, kErrorTypeValidation,
+                        "Setting v:%s to value with wrong type", key.data);
+        }
+        return rv;
+      }
+      if (watched) {
+        tv_copy(&di->di_tv, &oldtv);
+      }
       tv_clear(&di->di_tv);
     }
 

src/nvim/eval/vars.c

@@ -52,8 +52,8 @@
 
 static const char *e_letunexp = N_("E18: Unexpected characters in :let");
 static const char *e_lock_unlock = N_("E940: Cannot lock or unlock variable %s");
-static const char e_setting_str_to_value_with_wrong_type[]
+static const char e_setting_v_str_to_value_with_wrong_type[]
-  = N_("E963: Setting %s to value with wrong type");
+  = N_("E963: Setting v:%s to value with wrong type");
 static const char e_cannot_use_heredoc_here[]
   = N_("E991: Cannot use =<< here");
 
@@ -1389,6 +1389,62 @@ static void list_one_var_a(const char *prefix, const char *name, const ptrdiff_t
   }
 }
 
+/// Additional handling for setting a v: variable.
+///
+/// @return  true if the variable should be set normally,
+///          false if nothing else needs to be done.
+bool before_set_vvar(const char *const varname, dictitem_T *const di, typval_T *const tv,
+                     const bool copy, const bool watched, bool *const type_error)
+{
+  if (di->di_tv.v_type == VAR_STRING) {
+    typval_T oldtv = TV_INITIAL_VALUE;
+    if (watched) {
+      tv_copy(&di->di_tv, &oldtv);
+    }
+    XFREE_CLEAR(di->di_tv.vval.v_string);
+    if (copy || tv->v_type != VAR_STRING) {
+      const char *const val = tv_get_string(tv);
+      // Careful: when assigning to v:errmsg and tv_get_string()
+      // causes an error message the variable will already be set.
+      if (di->di_tv.vval.v_string == NULL) {
+        di->di_tv.vval.v_string = xstrdup(val);
+      }
+    } else {
+      // Take over the string to avoid an extra alloc/free.
+      di->di_tv.vval.v_string = tv->vval.v_string;
+      tv->vval.v_string = NULL;
+    }
+    // Notify watchers
+    if (watched) {
+      tv_dict_watcher_notify(&vimvardict, varname, &di->di_tv, &oldtv);
+      tv_clear(&oldtv);
+    }
+    return false;
+  } else if (di->di_tv.v_type == VAR_NUMBER) {
+    typval_T oldtv = TV_INITIAL_VALUE;
+    if (watched) {
+      tv_copy(&di->di_tv, &oldtv);
+    }
+    di->di_tv.vval.v_number = tv_get_number(tv);
+    if (strcmp(varname, "searchforward") == 0) {
+      set_search_direction(di->di_tv.vval.v_number ? '/' : '?');
+    } else if (strcmp(varname, "hlsearch") == 0) {
+      no_hlsearch = !di->di_tv.vval.v_number;
+      redraw_all_later(UPD_SOME_VALID);
+    }
+    // Notify watchers
+    if (watched) {
+      tv_dict_watcher_notify(&vimvardict, varname, &di->di_tv, &oldtv);
+      tv_clear(&oldtv);
+    }
+    return false;
+  } else if (di->di_tv.v_type != tv->v_type) {
+    *type_error = true;
+    return false;
+  }
+  return true;
+}
+
 /// Set variable to the given value
 ///
 /// If the variable already exists, the value is updated. Otherwise the variable
@@ -1418,31 +1474,29 @@ void set_var_const(const char *name, const size_t name_len, typval_T *const tv,
                    const bool is_const)
   FUNC_ATTR_NONNULL_ALL
 {
-  dictitem_T *v;
-  hashtab_T *ht;
-  dict_T *dict;
-
   const char *varname;
-  ht = find_var_ht_dict(name, name_len, &varname, &dict);
+  dict_T *dict;
+  hashtab_T *ht = find_var_ht_dict(name, name_len, &varname, &dict);
   const bool watched = tv_dict_is_watched(dict);
 
   if (ht == NULL || *varname == NUL) {
     semsg(_(e_illvar), name);
     return;
   }
-  v = find_var_in_ht(ht, 0, varname, name_len - (size_t)(varname - name), true);
+  const size_t varname_len = name_len - (size_t)(varname - name);
+  dictitem_T *di = find_var_in_ht(ht, 0, varname, varname_len, true);
 
   // Search in parent scope which is possible to reference from lambda
-  if (v == NULL) {
+  if (di == NULL) {
-    v = find_var_in_scoped_ht(name, name_len, true);
+    di = find_var_in_scoped_ht(name, name_len, true);
   }
 
-  if (tv_is_func(*tv) && var_wrong_func_name(name, v == NULL)) {
+  if (tv_is_func(*tv) && var_wrong_func_name(name, di == NULL)) {
     return;
   }
 
   typval_T oldtv = TV_INITIAL_VALUE;
-  if (v != NULL) {
+  if (di != NULL) {
     if (is_const) {
       emsg(_(e_cannot_mod));
       return;
@@ -1452,9 +1506,9 @@ void set_var_const(const char *name, const size_t name_len, typval_T *const tv,
     // - Whether the variable is read-only
     // - Whether the variable value is locked
     // - Whether the variable is locked
-    if (var_check_ro(v->di_flags, name, name_len)
+    if (var_check_ro(di->di_flags, name, name_len)
-        || value_check_lock(v->di_tv.v_lock, name, name_len)
+        || value_check_lock(di->di_tv.v_lock, name, name_len)
-        || var_check_lock(v->di_flags, name, name_len)) {
+        || var_check_lock(di->di_flags, name, name_len)) {
       return;
     }
 
@@ -1462,42 +1516,19 @@ void set_var_const(const char *name, const size_t name_len, typval_T *const tv,
 
     // Handle setting internal v: variables separately where needed to
     // prevent changing the type.
-    if (is_vimvarht(ht)) {
+    bool type_error = false;
-      if (v->di_tv.v_type == VAR_STRING) {
+    if (is_vimvarht(ht)
-        XFREE_CLEAR(v->di_tv.vval.v_string);
+        && !before_set_vvar(varname, di, tv, copy, watched, &type_error)) {
-        if (copy || tv->v_type != VAR_STRING) {
+      if (type_error) {
-          const char *const val = tv_get_string(tv);
+        semsg(_(e_setting_v_str_to_value_with_wrong_type), varname);
-
-          // Careful: when assigning to v:errmsg and tv_get_string()
-          // causes an error message the variable will already be set.
-          if (v->di_tv.vval.v_string == NULL) {
-            v->di_tv.vval.v_string = xstrdup(val);
-          }
-        } else {
-          // Take over the string to avoid an extra alloc/free.
-          v->di_tv.vval.v_string = tv->vval.v_string;
-          tv->vval.v_string = NULL;
       }
       return;
-      } else if (v->di_tv.v_type == VAR_NUMBER) {
-        v->di_tv.vval.v_number = tv_get_number(tv);
-        if (strcmp(varname, "searchforward") == 0) {
-          set_search_direction(v->di_tv.vval.v_number ? '/' : '?');
-        } else if (strcmp(varname, "hlsearch") == 0) {
-          no_hlsearch = !v->di_tv.vval.v_number;
-          redraw_all_later(UPD_SOME_VALID);
-        }
-        return;
-      } else if (v->di_tv.v_type != tv->v_type) {
-        semsg(_(e_setting_str_to_value_with_wrong_type), name);
-        return;
-      }
     }
 
     if (watched) {
-      tv_copy(&v->di_tv, &oldtv);
+      tv_copy(&di->di_tv, &oldtv);
     }
-    tv_clear(&v->di_tv);
+    tv_clear(&di->di_tv);
   } else {  // Add a new variable.
     // Can't add "v:" or "a:" variable.
     if (is_vimvarht(ht) || ht == get_funccal_args_ht()) {
@@ -1513,28 +1544,28 @@ void set_var_const(const char *name, const size_t name_len, typval_T *const tv,
     // Make sure dict is valid
     assert(dict != NULL);
 
-    v = xmalloc(offsetof(dictitem_T, di_key) + strlen(varname) + 1);
+    di = xmalloc(offsetof(dictitem_T, di_key) + varname_len + 1);
-    STRCPY(v->di_key, varname);
+    memcpy(di->di_key, varname, varname_len + 1);
-    if (hash_add(ht, v->di_key) == FAIL) {
+    if (hash_add(ht, di->di_key) == FAIL) {
-      xfree(v);
+      xfree(di);
       return;
     }
-    v->di_flags = DI_FLAGS_ALLOC;
+    di->di_flags = DI_FLAGS_ALLOC;
     if (is_const) {
-      v->di_flags |= DI_FLAGS_LOCK;
+      di->di_flags |= DI_FLAGS_LOCK;
     }
   }
 
   if (copy || tv->v_type == VAR_NUMBER || tv->v_type == VAR_FLOAT) {
-    tv_copy(tv, &v->di_tv);
+    tv_copy(tv, &di->di_tv);
   } else {
-    v->di_tv = *tv;
+    di->di_tv = *tv;
-    v->di_tv.v_lock = VAR_UNLOCKED;
+    di->di_tv.v_lock = VAR_UNLOCKED;
     tv_init(tv);
   }
 
   if (watched) {
-    tv_dict_watcher_notify(dict, v->di_key, &v->di_tv, &oldtv);
+    tv_dict_watcher_notify(dict, di->di_key, &di->di_tv, &oldtv);
     tv_clear(&oldtv);
   }
 
@@ -1542,7 +1573,7 @@ void set_var_const(const char *name, const size_t name_len, typval_T *const tv,
     // Like :lockvar! name: lock the value and what it contains, but only
     // if the reference count is up to one.  That locks only literal
     // values.
-    tv_item_lock(&v->di_tv, DICT_MAXNEST, true, true);
+    tv_item_lock(&di->di_tv, DICT_MAXNEST, true, true);
   }
 }
 

src/nvim/lua/stdlib.c

@@ -24,6 +24,7 @@
 #include "nvim/buffer_defs.h"
 #include "nvim/eval/typval.h"
 #include "nvim/eval/typval_defs.h"
+#include "nvim/eval/vars.h"
 #include "nvim/ex_eval.h"
 #include "nvim/fold.h"
 #include "nvim/globals.h"
@@ -397,6 +398,15 @@ int nlua_setvar(lua_State *lstate)
       di = tv_dict_item_alloc_len(key.data, key.size);
       tv_dict_add(dict, di);
     } else {
+      bool type_error = false;
+      if (dict == &vimvardict
+          && !before_set_vvar(key.data, di, &tv, true, watched, &type_error)) {
+        tv_clear(&tv);
+        if (type_error) {
+          return luaL_error(lstate, "Setting v:%s to value with wrong type", key.data);
+        }
+        return 0;
+      }
       if (watched) {
         tv_copy(&di->di_tv, &oldtv);
       }

test/functional/api/vim_spec.lua

@@ -1350,15 +1350,59 @@ describe('API', function()
     end)
 
     it('nvim_get_vvar, nvim_set_vvar', function()
-      -- Set readonly v: var.
+      eq('Key is read-only: count', pcall_err(request, 'nvim_set_vvar', 'count', 42))
-      eq('Key is read-only: count',
+      eq('Dictionary is locked', pcall_err(request, 'nvim_set_vvar', 'nosuchvar', 42))
-        pcall_err(request, 'nvim_set_vvar', 'count', 42))
-      -- Set non-existent v: var.
-      eq('Dictionary is locked',
-        pcall_err(request, 'nvim_set_vvar', 'nosuchvar', 42))
-      -- Set writable v: var.
       meths.set_vvar('errmsg', 'set by API')
       eq('set by API', meths.get_vvar('errmsg'))
+      meths.set_vvar('errmsg', 42)
+      eq('42', eval('v:errmsg'))
+      meths.set_vvar('oldfiles', { 'one', 'two' })
+      eq({ 'one', 'two' }, eval('v:oldfiles'))
+      meths.set_vvar('oldfiles', {})
+      eq({}, eval('v:oldfiles'))
+      eq('Setting v:oldfiles to value with wrong type', pcall_err(meths.set_vvar, 'oldfiles', 'a'))
+      eq({}, eval('v:oldfiles'))
+
+      feed('i foo foo foo<Esc>0/foo<CR>')
+      eq({1, 1}, meths.win_get_cursor(0))
+      eq(1, eval('v:searchforward'))
+      feed('n')
+      eq({1, 5}, meths.win_get_cursor(0))
+      meths.set_vvar('searchforward', 0)
+      eq(0, eval('v:searchforward'))
+      feed('n')
+      eq({1, 1}, meths.win_get_cursor(0))
+      meths.set_vvar('searchforward', 1)
+      eq(1, eval('v:searchforward'))
+      feed('n')
+      eq({1, 5}, meths.win_get_cursor(0))
+
+      local screen = Screen.new(60, 3)
+      screen:set_default_attr_ids({
+        [0] = {bold = true, foreground = Screen.colors.Blue},
+        [1] = {background = Screen.colors.Yellow},
+      })
+      screen:attach()
+      eq(1, eval('v:hlsearch'))
+      screen:expect{grid=[[
+         {1:foo} {1:^foo} {1:foo}                                                |
+        {0:~                                                           }|
+                                                                    |
+      ]]}
+      meths.set_vvar('hlsearch', 0)
+      eq(0, eval('v:hlsearch'))
+      screen:expect{grid=[[
+         foo ^foo foo                                                |
+        {0:~                                                           }|
+                                                                    |
+      ]]}
+      meths.set_vvar('hlsearch', 1)
+      eq(1, eval('v:hlsearch'))
+      screen:expect{grid=[[
+         {1:foo} {1:^foo} {1:foo}                                                |
+        {0:~                                                           }|
+                                                                    |
+      ]]}
     end)
 
     it('vim_set_var returns the old value', function()

test/functional/lua/vim_spec.lua

@@ -1491,8 +1491,60 @@ describe('lua stdlib', function()
     eq(NIL, funcs.luaeval "vim.v.null")
     matches([[attempt to index .* nil value]],
        pcall_err(exec_lua, 'return vim.v[0].progpath'))
-    eq('Key is read-only: count', pcall_err(exec_lua, 'vim.v.count = 42'))
+    eq('Key is read-only: count', pcall_err(exec_lua, [[vim.v.count = 42]]))
-    eq('Dictionary is locked', pcall_err(exec_lua, 'vim.v.nosuchvar = 42'))
+    eq('Dictionary is locked', pcall_err(exec_lua, [[vim.v.nosuchvar = 42]]))
+    eq('Key is fixed: errmsg', pcall_err(exec_lua, [[vim.v.errmsg = nil]]))
+    exec_lua([[vim.v.errmsg = 'set by Lua']])
+    eq('set by Lua', eval('v:errmsg'))
+    exec_lua([[vim.v.errmsg = 42]])
+    eq('42', eval('v:errmsg'))
+    exec_lua([[vim.v.oldfiles = { 'one', 'two' }]])
+    eq({ 'one', 'two' }, eval('v:oldfiles'))
+    exec_lua([[vim.v.oldfiles = {}]])
+    eq({}, eval('v:oldfiles'))
+    eq('Setting v:oldfiles to value with wrong type', pcall_err(exec_lua, [[vim.v.oldfiles = 'a']]))
+    eq({}, eval('v:oldfiles'))
+
+    feed('i foo foo foo<Esc>0/foo<CR>')
+    eq({1, 1}, meths.win_get_cursor(0))
+    eq(1, eval('v:searchforward'))
+    feed('n')
+    eq({1, 5}, meths.win_get_cursor(0))
+    exec_lua([[vim.v.searchforward = 0]])
+    eq(0, eval('v:searchforward'))
+    feed('n')
+    eq({1, 1}, meths.win_get_cursor(0))
+    exec_lua([[vim.v.searchforward = 1]])
+    eq(1, eval('v:searchforward'))
+    feed('n')
+    eq({1, 5}, meths.win_get_cursor(0))
+
+    local screen = Screen.new(60, 3)
+    screen:set_default_attr_ids({
+      [0] = {bold = true, foreground = Screen.colors.Blue},
+      [1] = {background = Screen.colors.Yellow},
+    })
+    screen:attach()
+    eq(1, eval('v:hlsearch'))
+    screen:expect{grid=[[
+       {1:foo} {1:^foo} {1:foo}                                                |
+      {0:~                                                           }|
+                                                                  |
+    ]]}
+    exec_lua([[vim.v.hlsearch = 0]])
+    eq(0, eval('v:hlsearch'))
+    screen:expect{grid=[[
+       foo ^foo foo                                                |
+      {0:~                                                           }|
+                                                                  |
+    ]]}
+    exec_lua([[vim.v.hlsearch = 1]])
+    eq(1, eval('v:hlsearch'))
+    screen:expect{grid=[[
+       {1:foo} {1:^foo} {1:foo}                                                |
+      {0:~                                                           }|
+                                                                  |
+    ]]}
   end)
 
   it('vim.bo', function()