vim-patch:9.0.1631: passing wrong variable type to option gives multiple errors (#24026)

Problem:    Passing a wrong variable type to an option gives multiple errors.
Solution:   Bail out early on failure. (closes vim/vim#12504)

https://github.com/vim/vim/commit/4c7cb372c17a84c8a35254d93eb37cb854cd39da

5 files changed, 106 insertions, 95 deletions

diff --git a/src/nvim/eval/vars.c b/src/nvim/eval/vars.c
index 7ba751490b..35ae558006 100644
--- a/src/nvim/eval/vars.c
+++ b/src/nvim/eval/vars.c
@@ -769,17 +769,28 @@ static char *ex_let_option(char *arg, typval_T *const tv, const bool is_const,
     return NULL;
   }
 
-  bool hidden;
-  bool error;
   const char c1 = *p;
   *p = NUL;
 
-  OptVal curval = get_option_value(arg, NULL, scope, &hidden);
-  OptVal newval = tv_to_optval(tv, arg, scope, &error);
+  uint32_t opt_p_flags;
+  bool hidden;
+  OptVal curval = get_option_value(arg, &opt_p_flags, scope, &hidden);
+  OptVal newval = NIL_OPTVAL;
+  if (curval.type == kOptValTypeNil && arg[0] != 't' && arg[1] != '_') {
+    semsg(_(e_unknown_option2), arg);
+    goto theend;
+  }
+  if (op != NULL && *op != '='
+      && ((curval.type != kOptValTypeString && *op == '.')
+          || (curval.type == kOptValTypeString && *op != '.'))) {
+    semsg(_(e_letwrong), op);
+    goto theend;
+  }
 
-  // Ignore errors for num types
-  if (newval.type != kOptValTypeNumber && newval.type != kOptValTypeBoolean && error) {
-    goto end;
+  bool error;
+  newval = tv_to_optval(tv, arg, opt_p_flags, &error);
+  if (error) {
+    goto theend;
   }
 
   // Don't assume current and new values are of the same type in order to future-proof the code for
@@ -789,61 +800,47 @@ static char *ex_let_option(char *arg, typval_T *const tv, const bool is_const,
   const bool is_string = curval.type == kOptValTypeString && newval.type == kOptValTypeString;
 
   if (op != NULL && *op != '=') {
-    if (!hidden && ((is_num && *op == '.') || (is_string && *op != '.'))) {
-      semsg(_(e_letwrong), op);
-      goto end;
-    } else {
-      // number or bool
-      if (!hidden && is_num) {
-        Integer cur_n = curval.type == kOptValTypeNumber ? curval.data.number : curval.data.boolean;
-        Integer new_n = newval.type == kOptValTypeNumber ? newval.data.number : newval.data.boolean;
-
-        switch (*op) {
-        case '+':
-          new_n = cur_n + new_n; break;
-        case '-':
-          new_n = cur_n - new_n; break;
-        case '*':
-          new_n = cur_n * new_n; break;
-        case '/':
-          new_n = num_divide(cur_n, new_n); break;
-        case '%':
-          new_n = num_modulus(cur_n, new_n); break;
-        }
-
-        // clamp boolean values
-        if (newval.type == kOptValTypeBoolean && (new_n > 1 || new_n < -1)) {
-          new_n = (new_n > 1) ? 1 : -1;
-        }
+    if (!hidden && is_num) {  // number or bool
+      Integer cur_n = curval.type == kOptValTypeNumber ? curval.data.number : curval.data.boolean;
+      Integer new_n = newval.type == kOptValTypeNumber ? newval.data.number : newval.data.boolean;
+
+      switch (*op) {
+      case '+':
+        new_n = cur_n + new_n; break;
+      case '-':
+        new_n = cur_n - new_n; break;
+      case '*':
+        new_n = cur_n * new_n; break;
+      case '/':
+        new_n = num_divide(cur_n, new_n); break;
+      case '%':
+        new_n = num_modulus(cur_n, new_n); break;
+      }
 
-        newval = kOptValTypeNumber ? NUMBER_OPTVAL(new_n) : BOOLEAN_OPTVAL((TriState)new_n);
-      } else if (!hidden && is_string && curval.data.string.data != NULL
-                 && newval.data.string.data != NULL) {
-        // string
-        OptVal newval_old = newval;
-        newval = CSTR_AS_OPTVAL(concat_str(curval.data.string.data, newval.data.string.data));
-        optval_free(newval_old);
+      // clamp boolean values
+      if (newval.type == kOptValTypeBoolean && (new_n > 1 || new_n < -1)) {
+        new_n = (new_n > 1) ? 1 : -1;
       }
+
+      newval = kOptValTypeNumber ? NUMBER_OPTVAL(new_n) : BOOLEAN_OPTVAL((TriState)new_n);
+    } else if (!hidden && is_string
+               && curval.data.string.data != NULL && newval.data.string.data != NULL) {  // string
+      OptVal newval_old = newval;
+      newval = CSTR_AS_OPTVAL(concat_str(curval.data.string.data, newval.data.string.data));
+      optval_free(newval_old);
     }
   }
 
-  // If new value is a string and is NULL, show an error if it's not a hidden option.
-  // For hidden options, just pass the value to `set_option_value` and let it fail silently.
-  if (hidden || newval.type != kOptValTypeString || newval.data.string.data != NULL) {
-    const char *err = set_option_value(arg, newval, scope);
-    arg_end = p;
-    if (err != NULL) {
-      emsg(_(err));
-    }
-  } else {
-    emsg(_(e_stringreq));
+  const char *err = set_option_value(arg, newval, scope);
+  arg_end = p;
+  if (err != NULL) {
+    emsg(_(err));
   }
 
-end:
+theend:
   *p = c1;
   optval_free(curval);
   optval_free(newval);
-
   return arg_end;
 }
 
@@ -1813,25 +1810,18 @@ static void getwinvar(typval_T *argvars, typval_T *rettv, int off)
 ///
 /// @param[in]   tv      typval to convert.
 /// @param[in]   option  Option name.
-/// @param[in]   scope   Option scope.
+/// @param[in]   flags   Option flags.
 /// @param[out]  error   Whether an error occured.
 ///
 /// @return  Typval converted to OptVal. Must be freed by caller.
 ///          Returns NIL_OPTVAL for invalid option name.
-static OptVal tv_to_optval(typval_T *tv, const char *option, int scope, bool *error)
+static OptVal tv_to_optval(typval_T *tv, const char *option, uint32_t flags, bool *error)
 {
   OptVal value = NIL_OPTVAL;
   char nbuf[NUMBUFLEN];
-  uint32_t flags;
   bool err = false;
 
-  OptVal curval = get_option_value(option, &flags, scope, NULL);
-
-  // TODO(famiu): Delegate all of these type-checks to set_option_value()
-  if (curval.type == kOptValTypeNil) {
-    // Invalid option name,
-    value = NIL_OPTVAL;
-  } else if ((flags & P_FUNC) && tv_is_func(*tv)) {
+  if ((flags & P_FUNC) && tv_is_func(*tv)) {
     // If the option can be set to a function reference or a lambda
     // and the passed value is a function reference, then convert it to
     // the name (string) of the function reference.
@@ -1839,29 +1829,31 @@ static OptVal tv_to_optval(typval_T *tv, const char *option, int scope, bool *er
     err = strval == NULL;
     value = CSTR_AS_OPTVAL(strval);
   } else if (flags & (P_NUM | P_BOOL)) {
-    varnumber_T n = tv_get_number_chk(tv, &err);
-    // This could be either "0" or a string that's not a number. So we need to check if it's
-    // actually a number.
+    varnumber_T n = (flags & P_NUM) ? tv_get_number_chk(tv, &err)
+                                    : tv_get_bool_chk(tv, &err);
+    // This could be either "0" or a string that's not a number.
+    // So we need to check if it's actually a number.
     if (!err && tv->v_type == VAR_STRING && n == 0) {
       unsigned idx;
       for (idx = 0; tv->vval.v_string[idx] == '0'; idx++) {}
       if (tv->vval.v_string[idx] != NUL || idx == 0) {
         // There's another character after zeros or the string is empty.
         // In both cases, we are trying to set a num option using a string.
+        err = true;
         semsg(_("E521: Number required: &%s = '%s'"), option, tv->vval.v_string);
       }
     }
     value = (flags & P_NUM) ? NUMBER_OPTVAL(n)
                             : BOOLEAN_OPTVAL(n == 0 ? kFalse : (n >= 1 ? kTrue : kNone));
-  } else if (flags & P_STRING || is_tty_option(option)) {
-    // Avoid setting string option to a boolean.
-    if (tv->v_type == VAR_BOOL) {
-      err = true;
-      emsg(_(e_stringreq));
-    } else {
+  } else if ((flags & P_STRING) || is_tty_option(option)) {
+    // Avoid setting string option to a boolean or a special value.
+    if (tv->v_type != VAR_BOOL && tv->v_type != VAR_SPECIAL) {
       const char *strval = tv_get_string_buf_chk(tv, nbuf);
       err = strval == NULL;
       value = CSTR_TO_OPTVAL(strval);
+    } else if (flags & P_STRING) {
+      err = true;
+      emsg(_(e_stringreq));
     }
   } else {
     abort();  // This should never happen.
@@ -1870,19 +1862,23 @@ static OptVal tv_to_optval(typval_T *tv, const char *option, int scope, bool *er
   if (error != NULL) {
     *error = err;
   }
-  optval_free(curval);
   return value;
 }
 
 /// Set option "varname" to the value of "varp" for the current buffer/window.
 static void set_option_from_tv(const char *varname, typval_T *varp)
 {
+  int opt_idx = findoption(varname);
+  if (opt_idx < 0) {
+    semsg(_(e_unknown_option2), varname);
+    return;
+  }
+  uint32_t opt_p_flags = get_option(opt_idx)->flags;
+
   bool error = false;
-  OptVal value = tv_to_optval(varp, varname, OPT_LOCAL, &error);
+  OptVal value = tv_to_optval(varp, varname, opt_p_flags, &error);
 
-  if (!error && value.type == kOptValTypeNil) {
-    semsg(_(e_unknown_option2), varname);
-  } else if (!error) {
+  if (!error) {
     set_option_value_give_err(varname, value, OPT_LOCAL);
   }
 
diff --git a/src/nvim/option.c b/src/nvim/option.c
index 073a1684fb..e65f33e828 100644
--- a/src/nvim/option.c
+++ b/src/nvim/option.c
@@ -3787,14 +3787,6 @@ bool is_option_allocated(const char *name)
   return idx >= 0 && (options[idx].flags & P_ALLOCED);
 }
 
-/// Return true if "name" is a string option.
-/// Returns false if option "name" does not exist.
-bool is_string_option(const char *name)
-{
-  int idx = findoption(name);
-  return idx >= 0 && (options[idx].flags & P_STRING);
-}
-
 // Translate a string like "t_xx", "<t_xx>" or "<S-Tab>" to a key number.
 // When "has_lt" is true there is a '<' before "*arg_arg".
 // Returns 0 when the key is not recognized.
diff --git a/test/old/testdir/test_let.vim b/test/old/testdir/test_let.vim
index bf119bdeab..a9cc8a14a4 100644
--- a/test/old/testdir/test_let.vim
+++ b/test/old/testdir/test_let.vim
@@ -242,7 +242,7 @@ func Test_let_no_type_checking()
 endfunc
 
 func Test_let_termcap()
-  throw 'skipped: Nvim does not support termcap option'
+  throw 'Skipped: Nvim does not support termcap options'
   " Terminal code
   let old_t_te = &t_te
   let &t_te = "\<Esc>[yes;"
@@ -257,25 +257,45 @@ func Test_let_termcap()
     let &t_k1 = old_t_k1
   endif
 
-  call assert_fails('let x = &t_xx', 'E113')
+  call assert_fails('let x = &t_xx', 'E113:')
   let &t_xx = "yes"
   call assert_equal("yes", &t_xx)
   let &t_xx = ""
-  call assert_fails('let x = &t_xx', 'E113')
+  call assert_fails('let x = &t_xx', 'E113:')
 endfunc
 
 func Test_let_option_error()
   let _w = &tw
   let &tw = 80
-  call assert_fails('let &tw .= 1', 'E734')
+  call assert_fails('let &tw .= 1', ['E734:', 'E734:'])
+  call assert_fails('let &tw .= []', ['E734:', 'E734:'])
+  call assert_fails('let &tw = []', ['E745:', 'E745:'])
+  call assert_fails('let &tw += []', ['E745:', 'E745:'])
   call assert_equal(80, &tw)
   let &tw = _w
 
+  let _w = &autoread
+  let &autoread = 1
+  call assert_fails('let &autoread .= 1', ['E734:', 'E734:'])
+  call assert_fails('let &autoread .= []', ['E734:', 'E734:'])
+  call assert_fails('let &autoread = []', ['E745:', 'E745:'])
+  call assert_fails('let &autoread += []', ['E745:', 'E745:'])
+  call assert_equal(1, &autoread)
+  let &autoread = _w
+
   let _w = &fillchars
   let &fillchars = "vert:|"
-  call assert_fails('let &fillchars += "diff:-"', 'E734')
+  call assert_fails('let &fillchars += "diff:-"', ['E734:', 'E734:'])
+  call assert_fails('let &fillchars += []', ['E734:', 'E734:'])
+  call assert_fails('let &fillchars = []', ['E730:', 'E730:'])
+  call assert_fails('let &fillchars .= []', ['E730:', 'E730:'])
   call assert_equal("vert:|", &fillchars)
   let &fillchars = _w
+
+  call assert_fails('let &nosuchoption = 1', ['E355:', 'E355:'])
+  call assert_fails('let &nosuchoption = ""', ['E355:', 'E355:'])
+  call assert_fails('let &nosuchoption = []', ['E355:', 'E355:'])
+  call assert_fails('let &t_xx = []', ['E730:', 'E730:'])
 endfunc
 
 " Errors with the :let statement
diff --git a/test/old/testdir/test_options.vim b/test/old/testdir/test_options.vim
index ada44e5eed..5a30fbbb16 100644
--- a/test/old/testdir/test_options.vim
+++ b/test/old/testdir/test_options.vim
@@ -380,7 +380,7 @@ func Test_set_completion()
   call assert_equal('"set filetype=' .. getcompletion('a*', 'filetype')->join(), @:)
 endfunc
 
-func Test_set_errors()
+func Test_set_option_errors()
   call assert_fails('set scroll=-1', 'E49:')
   call assert_fails('set backupcopy=', 'E474:')
   call assert_fails('set regexpengine=3', 'E474:')
@@ -482,7 +482,7 @@ func Test_set_errors()
   if has('python') || has('python3')
     call assert_fails('set pyxversion=6', 'E474:')
   endif
-  call assert_fails("let &tabstop='ab'", 'E521:')
+  call assert_fails("let &tabstop='ab'", ['E521:', 'E521:'])
   call assert_fails('set spellcapcheck=%\\(', 'E54:')
   call assert_fails('set sessionoptions=curdir,sesdir', 'E474:')
   call assert_fails('set foldmarker={{{,', 'E474:')
@@ -506,6 +506,12 @@ func Test_set_errors()
   " call assert_fails('set t_#-&', 'E522:')
   call assert_fails('let &formatoptions = "?"', 'E539:')
   call assert_fails('call setbufvar("", "&formatoptions", "?")', 'E539:')
+  call assert_fails('call setwinvar(0, "&scrolloff", [])', ['E745:', 'E745:'])
+  call assert_fails('call setwinvar(0, "&list", [])', ['E745:', 'E745:'])
+  call assert_fails('call setwinvar(0, "&listchars", [])', ['E730:', 'E730:'])
+  call assert_fails('call setwinvar(0, "&nosuchoption", 0)', ['E355:', 'E355:'])
+  call assert_fails('call setwinvar(0, "&nosuchoption", "")', ['E355:', 'E355:'])
+  call assert_fails('call setwinvar(0, "&nosuchoption", [])', ['E355:', 'E355:'])
 endfunc
 
 func CheckWasSet(name)
diff --git a/test/old/testdir/test_vimscript.vim b/test/old/testdir/test_vimscript.vim
index 598b359f1f..62487ae5d5 100644
--- a/test/old/testdir/test_vimscript.vim
+++ b/test/old/testdir/test_vimscript.vim
@@ -6997,10 +6997,7 @@ func Test_compound_assignment_operators()
     call assert_equal(6, &scrolljump)
     let &scrolljump %= 5
     call assert_equal(1, &scrolljump)
-    " A different error is shown due to a change in implementation of option
-    " values.
-    " call assert_fails('let &scrolljump .= "j"', 'E734:')
-    call assert_fails('let &scrolljump .= "j"', 'E521:')
+    call assert_fails('let &scrolljump .= "j"', ['E734:', 'E734:'])
     set scrolljump&vim
 
     let &foldlevelstart = 2