refactor(options)!: use OptVal for option defaults #26691

Problem: We use `void *` for option default values, which is confusing and can cause problems with type-correctness. It also doesn't accomodate for multitype options. On top of that, it also leads to default boolean option values not behaving correctly on big endian systems.

Solution: Use `OptVal` for option default values.

BREAKING CHANGE:
- `:set {option}<` removes the local value for all global-local options instead of just string global-local options.
- `:setlocal {option}<` copies the global value to the local value for number and boolean global-local options instead of removing the local value.

1 files changed, 186 insertions, 238 deletions

diff --git a/src/nvim/option.c b/src/nvim/option.c
index 5f0115b46c..a237689eb0 100644
--- a/src/nvim/option.c
+++ b/src/nvim/option.c
@@ -176,7 +176,7 @@ static int p_paste_dep_opts[] = {
 void set_init_tablocal(void)
 {
   // susy baka: cmdheight calls itself OPT_GLOBAL but is really tablocal!
-  p_ch = options[kOptCmdheight].def_val.number;
+  p_ch = options[kOptCmdheight].def_val.data.number;
 }
 
 /// Initialize the 'shell' option to a default value.
@@ -291,8 +291,9 @@ static void set_init_default_cdpath(void)
     }
   }
   buf[j] = NUL;
-  options[kOptCdpath].def_val.string = buf;
+  options[kOptCdpath].def_val = CSTR_AS_OPTVAL(buf);
   options[kOptCdpath].flags |= P_DEF_ALLOCED;
+
   xfree(cdpath);
 }
 
@@ -317,12 +318,13 @@ static void set_init_expand_env(void)
       p = option_expand(opt_idx, NULL);
     }
     if (p != NULL) {
-      p = xstrdup(p);
-      *(char **)opt->var = p;
+      set_option_varp(opt_idx, opt->var, CSTR_TO_OPTVAL(p), opt->flags & P_ALLOCED);
+      opt->flags |= P_ALLOCED;
+
       if (opt->flags & P_DEF_ALLOCED) {
-        xfree(opt->def_val.string);
+        optval_free(opt->def_val);
       }
-      opt->def_val.string = p;
+      opt->def_val = CSTR_TO_OPTVAL(p);
       opt->flags |= P_DEF_ALLOCED;
     }
   }
@@ -430,71 +432,54 @@ void set_init_1(bool clean_arg)
   set_helplang_default(get_mess_lang());
 }
 
-/// Set an option to its default value.
-/// This does not take care of side effects!
+/// Get default value for option, based on the option's type and scope.
 ///
-/// @param opt_flags OPT_FREE, OPT_LOCAL and/or OPT_GLOBAL.
+/// @param  opt_idx    Option index in options[] table.
+/// @param  opt_flags  Option flags.
 ///
-/// TODO(famiu): Refactor this when def_val uses OptVal.
-static void set_option_default(const OptIndex opt_idx, int opt_flags)
+/// @return Default value of option for the scope specified in opt_flags.
+static OptVal get_option_default(const OptIndex opt_idx, int opt_flags)
 {
-  bool both = (opt_flags & (OPT_LOCAL | OPT_GLOBAL)) == 0;
-
-  // pointer to variable for current option
   vimoption_T *opt = &options[opt_idx];
-  void *varp = get_varp_scope(opt, both ? OPT_LOCAL : opt_flags);
-  uint32_t flags = opt->flags;
-  if (varp != NULL) {       // skip hidden option, nothing to do for it
-    if (option_has_type(opt_idx, kOptValTypeString)) {
-      // Use set_option_direct() for local options to handle freeing and allocating the value.
-      if (opt->indir != PV_NONE) {
-        set_option_direct(opt_idx, CSTR_AS_OPTVAL(opt->def_val.string), opt_flags, 0);
-      } else {
-        if (flags & P_ALLOCED) {
-          free_string_option(*(char **)(varp));
-        }
-        *(char **)varp = opt->def_val.string;
-        opt->flags &= ~P_ALLOCED;
-      }
-    } else if (option_has_type(opt_idx, kOptValTypeNumber)) {
-      if (opt->indir == PV_SCROLL) {
-        win_comp_scroll(curwin);
-      } else {
-        OptInt def_val = opt->def_val.number;
-        if ((OptInt *)varp == &curwin->w_p_so
-            || (OptInt *)varp == &curwin->w_p_siso) {
-          // 'scrolloff' and 'sidescrolloff' local values have a
-          // different default value than the global default.
-          *(OptInt *)varp = -1;
-        } else {
-          *(OptInt *)varp = def_val;
-        }
-        // May also set global value for local option.
-        if (both) {
-          *(OptInt *)get_varp_scope(opt, OPT_GLOBAL) = def_val;
-        }
-      }
-    } else {  // boolean
-      *(int *)varp = opt->def_val.boolean;
+  bool is_global_local_option = opt->indir & PV_BOTH;
+
 #ifdef UNIX
-      // 'modeline' defaults to off for root
-      if (opt->indir == PV_ML && getuid() == ROOT_UID) {
-        *(int *)varp = false;
-      }
+  if (opt_idx == kOptModeline && getuid() == ROOT_UID) {
+    // 'modeline' defaults to off for root.
+    return BOOLEAN_OPTVAL(false);
+  }
 #endif
-      // May also set global value for local option.
-      if (both) {
-        *(int *)get_varp_scope(opt, OPT_GLOBAL) =
-          *(int *)varp;
-      }
-    }
 
-    // The default value is not insecure.
-    uint32_t *flagsp = insecure_flag(curwin, opt_idx, opt_flags);
-    *flagsp = *flagsp & ~P_INSECURE;
+  if ((opt_flags & OPT_LOCAL) && is_global_local_option) {
+    // Use unset local value instead of default value for local scope of global-local options.
+    return get_option_unset_value(opt_idx);
+  } else if (option_has_type(opt_idx, kOptValTypeString) && !(opt->flags & P_NO_DEF_EXP)) {
+    // For string options, expand environment variables and ~ since the default value was already
+    // expanded, only required when an environment variable was set later.
+    char *s = option_expand(opt_idx, opt->def_val.data.string.data);
+    return s == NULL ? opt->def_val : CSTR_AS_OPTVAL(s);
+  } else {
+    return opt->def_val;
+  }
+}
+
+/// Set an option to its default value.
+/// This does not take care of side effects!
+///
+/// @param  opt_idx    Option index in options[] table.
+/// @param  opt_flags  Option flags.
+static void set_option_default(const OptIndex opt_idx, int opt_flags)
+{
+  OptVal def_val = get_option_default(opt_idx, opt_flags);
+  set_option_direct(opt_idx, def_val, opt_flags, current_sctx.sc_sid);
+
+  if (opt_idx == kOptScroll) {
+    win_comp_scroll(curwin);
   }
 
-  set_option_sctx(opt_idx, opt_flags, current_sctx);
+  // The default value is not insecure.
+  uint32_t *flagsp = insecure_flag(curwin, opt_idx, opt_flags);
+  *flagsp = *flagsp & ~P_INSECURE;
 }
 
 /// Set all options (except terminal options) to their default value.
@@ -522,6 +507,8 @@ static void set_options_default(int opt_flags)
 /// @param  opt_idx    Option index in options[] table.
 /// @param  val        The value of the option.
 /// @param  allocated  If true, do not copy default as it was already allocated.
+///
+/// TODO(famiu): Remove this.
 static void set_string_default(OptIndex opt_idx, char *val, bool allocated)
   FUNC_ATTR_NONNULL_ALL
 {
@@ -531,10 +518,10 @@ static void set_string_default(OptIndex opt_idx, char *val, bool allocated)
 
   vimoption_T *opt = &options[opt_idx];
   if (opt->flags & P_DEF_ALLOCED) {
-    xfree(opt->def_val.string);
+    optval_free(opt->def_val);
   }
 
-  opt->def_val.string = allocated ? val : xstrdup(val);
+  opt->def_val = CSTR_AS_OPTVAL(allocated ? val : xstrdup(val));
   opt->flags |= P_DEF_ALLOCED;
 }
 
@@ -569,15 +556,6 @@ static char *find_dup_item(char *origval, const char *newval, const size_t newva
   return NULL;
 }
 
-/// Set the Vi-default value of a number option.
-/// Used for 'lines' and 'columns'.
-void set_number_default(OptIndex opt_idx, OptInt val)
-{
-  if (opt_idx != kOptInvalid) {
-    options[opt_idx].def_val.number = val;
-  }
-}
-
 #if defined(EXITFREE)
 /// Free all options.
 void free_all_options(void)
@@ -589,7 +567,7 @@ void free_all_options(void)
         optval_free(optval_from_varp(opt_idx, options[opt_idx].var));
       }
       if (options[opt_idx].flags & P_DEF_ALLOCED) {
-        optval_free(optval_from_varp(opt_idx, &options[opt_idx].def_val));
+        optval_free(options[opt_idx].def_val);
       }
     } else if (options[opt_idx].var != VAR_WIN) {
       // buffer-local option: free global value
@@ -622,7 +600,7 @@ void set_init_2(bool headless)
   if (!option_was_set(kOptWindow)) {
     p_window = Rows - 1;
   }
-  set_number_default(kOptWindow, Rows - 1);
+  options[kOptWindow].def_val = NUMBER_OPTVAL(Rows - 1);
 }
 
 /// Initialize the options, part three: After reading the .vimrc
@@ -640,43 +618,29 @@ void set_init_3(void)
   char *p = (char *)invocation_path_tail(p_sh, &len);
   p = xmemdupz(p, len);
 
-  {
-    //
-    // Default for p_sp is "| tee", for p_srr is ">".
-    // For known shells it is changed here to include stderr.
-    //
-    if (path_fnamecmp(p, "csh") == 0
-        || path_fnamecmp(p, "tcsh") == 0) {
-      if (do_sp) {
-        p_sp = "|& tee";
-        options[kOptShellpipe].def_val.string = p_sp;
-      }
-      if (do_srr) {
-        p_srr = ">&";
-        options[kOptShellredir].def_val.string = p_srr;
-      }
-    } else if (path_fnamecmp(p, "sh") == 0
-               || path_fnamecmp(p, "ksh") == 0
-               || path_fnamecmp(p, "mksh") == 0
-               || path_fnamecmp(p, "pdksh") == 0
-               || path_fnamecmp(p, "zsh") == 0
-               || path_fnamecmp(p, "zsh-beta") == 0
-               || path_fnamecmp(p, "bash") == 0
-               || path_fnamecmp(p, "fish") == 0
-               || path_fnamecmp(p, "ash") == 0
-               || path_fnamecmp(p, "dash") == 0) {
-      // Always use POSIX shell style redirection if we reach this
-      if (do_sp) {
-        p_sp = "2>&1| tee";
-        options[kOptShellpipe].def_val.string = p_sp;
-      }
-      if (do_srr) {
-        p_srr = ">%s 2>&1";
-        options[kOptShellredir].def_val.string = p_srr;
-      }
+  bool is_csh = path_fnamecmp(p, "csh") == 0 || path_fnamecmp(p, "tcsh") == 0;
+  bool is_known_shell = path_fnamecmp(p, "sh") == 0 || path_fnamecmp(p, "ksh") == 0
+                        || path_fnamecmp(p, "mksh") == 0 || path_fnamecmp(p, "pdksh") == 0
+                        || path_fnamecmp(p, "zsh") == 0 || path_fnamecmp(p, "zsh-beta") == 0
+                        || path_fnamecmp(p, "bash") == 0 || path_fnamecmp(p, "fish") == 0
+                        || path_fnamecmp(p, "ash") == 0 || path_fnamecmp(p, "dash") == 0;
+
+  // Default for p_sp is "| tee", for p_srr is ">".
+  // For known shells it is changed here to include stderr.
+  if (is_csh || is_known_shell) {
+    if (do_sp) {
+      const OptVal sp =
+        is_csh ? STATIC_CSTR_AS_OPTVAL("|& tee") : STATIC_CSTR_AS_OPTVAL("2>&1| tee");
+      set_option_direct(kOptShellpipe, sp, 0, SID_NONE);
+      options[kOptShellpipe].def_val = sp;
+    }
+    if (do_srr) {
+      const OptVal srr = is_csh ? STATIC_CSTR_AS_OPTVAL(">&") : STATIC_CSTR_AS_OPTVAL(">%s 2>&1");
+      set_option_direct(kOptShellredir, srr, 0, SID_NONE);
+      options[kOptShellredir].def_val = srr;
     }
-    xfree(p);
   }
+  xfree(p);
 
   if (buf_is_empty(curbuf)) {
     int idx_ffs = find_option("ffs");
@@ -734,12 +698,12 @@ void set_title_defaults(void)
   // icon name.  Saves a bit of time, because the X11 display server does
   // not need to be contacted.
   if (!(options[kOptTitle].flags & P_WAS_SET)) {
-    options[kOptTitle].def_val.boolean = false;
-    p_title = false;
+    options[kOptTitle].def_val = BOOLEAN_OPTVAL(false);
+    p_title = 0;
   }
   if (!(options[kOptIcon].flags & P_WAS_SET)) {
-    options[kOptIcon].def_val.boolean = false;
-    p_icon = false;
+    options[kOptIcon].def_val = BOOLEAN_OPTVAL(false);
+    p_icon = 0;
   }
 }
 
@@ -758,27 +722,6 @@ void ex_set(exarg_T *eap)
   do_set(eap->arg, flags);
 }
 
-/// Get the default value for a string option.
-static char *stropt_get_default_val(OptIndex opt_idx, uint64_t flags)
-{
-  char *newval = options[opt_idx].def_val.string;
-  // expand environment variables and ~ since the default value was
-  // already expanded, only required when an environment variable was set
-  // later
-  if (newval == NULL) {
-    newval = empty_string_option;
-  } else if (!(options[opt_idx].flags & P_NO_DEF_EXP)) {
-    char *s = option_expand(opt_idx, newval);
-    if (s == NULL) {
-      s = newval;
-    }
-    newval = xstrdup(s);
-  } else {
-    newval = xstrdup(newval);
-  }
-  return newval;
-}
-
 /// Copy the new string value into allocated memory for the option.
 /// Can't use set_option_direct(), because we need to remove the backslashes.
 static char *stropt_copy_value(char *origval, char **argp, set_op_T op,
@@ -922,10 +865,7 @@ static void stropt_remove_dupflags(char *newval, uint32_t flags)
   }
 }
 
-/// Get the string value specified for a ":set" command.  The following set
-/// options are supported:
-///     set {opt}&
-///     set {opt}<
+/// Get the string value specified for a ":set" command.  The following set options are supported:
 ///     set {opt}={val}
 ///     set {opt}:{val}
 static char *stropt_get_newval(int nextchar, OptIndex opt_idx, char **argp, void *varp,
@@ -936,61 +876,56 @@ static char *stropt_get_newval(int nextchar, OptIndex opt_idx, char **argp, void
   char *save_arg = NULL;
   char *newval;
   char *s = NULL;
-  if (nextchar == '&') {  // set to default val
-    newval = stropt_get_default_val(opt_idx, flags);
-  } else if (nextchar == '<') {  // set to global val
-    newval = xstrdup(*(char **)get_varp_scope(&(options[opt_idx]), OPT_GLOBAL));
-  } else {
-    arg++;  // jump to after the '=' or ':'
 
-    // Set 'keywordprg' to ":help" if an empty
-    // value was passed to :set by the user.
-    if (varp == &p_kp && (*arg == NUL || *arg == ' ')) {
-      save_arg = arg;
-      arg = ":help";
-    }
+  arg++;  // jump to after the '=' or ':'
 
-    // Copy the new string into allocated memory.
-    newval = stropt_copy_value(origval, &arg, op, flags);
+  // Set 'keywordprg' to ":help" if an empty
+  // value was passed to :set by the user.
+  if (varp == &p_kp && (*arg == NUL || *arg == ' ')) {
+    save_arg = arg;
+    arg = ":help";
+  }
 
-    // Expand environment variables and ~.
-    // Don't do it when adding without inserting a comma.
-    if (op == OP_NONE || (flags & P_COMMA)) {
-      newval = stropt_expand_envvar(opt_idx, origval, newval, op);
-    }
+  // Copy the new string into allocated memory.
+  newval = stropt_copy_value(origval, &arg, op, flags);
 
-    // locate newval[] in origval[] when removing it
-    // and when adding to avoid duplicates
-    int len = 0;
-    if (op == OP_REMOVING || (flags & P_NODUP)) {
-      len = (int)strlen(newval);
-      s = find_dup_item(origval, newval, (size_t)len, flags);
+  // Expand environment variables and ~.
+  // Don't do it when adding without inserting a comma.
+  if (op == OP_NONE || (flags & P_COMMA)) {
+    newval = stropt_expand_envvar(opt_idx, origval, newval, op);
+  }
 
-      // do not add if already there
-      if ((op == OP_ADDING || op == OP_PREPENDING) && s != NULL) {
-        op = OP_NONE;
-        STRCPY(newval, origval);
-      }
+  // locate newval[] in origval[] when removing it
+  // and when adding to avoid duplicates
+  int len = 0;
+  if (op == OP_REMOVING || (flags & P_NODUP)) {
+    len = (int)strlen(newval);
+    s = find_dup_item(origval, newval, (size_t)len, flags);
 
-      // if no duplicate, move pointer to end of original value
-      if (s == NULL) {
-        s = origval + (int)strlen(origval);
-      }
+    // do not add if already there
+    if ((op == OP_ADDING || op == OP_PREPENDING) && s != NULL) {
+      op = OP_NONE;
+      STRCPY(newval, origval);
     }
 
-    // concatenate the two strings; add a ',' if needed
-    if (op == OP_ADDING || op == OP_PREPENDING) {
-      stropt_concat_with_comma(origval, newval, op, flags);
-    } else if (op == OP_REMOVING) {
-      // Remove newval[] from origval[]. (Note: "len" has been set above
-      // and is used here).
-      stropt_remove_val(origval, newval, flags, s, len);
+    // if no duplicate, move pointer to end of original value
+    if (s == NULL) {
+      s = origval + (int)strlen(origval);
     }
+  }
 
-    if (flags & P_FLAGLIST) {
-      // Remove flags that appear twice.
-      stropt_remove_dupflags(newval, flags);
-    }
+  // concatenate the two strings; add a ',' if needed
+  if (op == OP_ADDING || op == OP_PREPENDING) {
+    stropt_concat_with_comma(origval, newval, op, flags);
+  } else if (op == OP_REMOVING) {
+    // Remove newval[] from origval[]. (Note: "len" has been set above
+    // and is used here).
+    stropt_remove_val(origval, newval, flags, s, len);
+  }
+
+  if (flags & P_FLAGLIST) {
+    // Remove flags that appear twice.
+    stropt_remove_dupflags(newval, flags);
   }
 
   if (save_arg != NULL) {
@@ -1152,6 +1087,7 @@ const char *find_option_end(const char *arg, OptIndex *opt_idxp)
 }
 
 /// Get new option value from argp. Allocated OptVal must be freed by caller.
+/// Can unset local value of an option when ":set {option}<" is used.
 static OptVal get_option_newval(OptIndex opt_idx, int opt_flags, set_prefix_T prefix, char **argp,
                                 int nextchar, set_op_T op, uint32_t flags, void *varp, char *errbuf,
                                 const size_t errbuflen, const char **errmsg)
@@ -1166,6 +1102,20 @@ static OptVal get_option_newval(OptIndex opt_idx, int opt_flags, set_prefix_T pr
   OptVal oldval = optval_from_varp(opt_idx, oldval_is_global ? get_varp(opt) : varp);
   OptVal newval = NIL_OPTVAL;
 
+  if (nextchar == '&') {
+    // ":set opt&": Reset to default value.
+    // NOTE: Use OPT_GLOBAL instead of opt_flags to ensure we don't use the unset local value for
+    // global-local options when OPT_LOCAL is used.
+    return optval_copy(get_option_default(opt_idx, OPT_GLOBAL));
+  } else if (nextchar == '<') {
+    // ":set opt<": Reset to global value.
+    // ":setlocal opt<": Copy global value to local value.
+    if (option_is_global_local(opt_idx) && !(opt_flags & OPT_LOCAL)) {
+      unset_option_local_value(opt_idx);
+    }
+    return get_option_value(opt_idx, OPT_GLOBAL);
+  }
+
   switch (oldval.type) {
   case kOptValTypeNil:
     abort();
@@ -1173,8 +1123,6 @@ static OptVal get_option_newval(OptIndex opt_idx, int opt_flags, set_prefix_T pr
     TriState newval_bool;
 
     // ":set opt!": invert
-    // ":set opt&": reset to default value
-    // ":set opt<": reset to global value
     if (nextchar == '!') {
       switch (oldval.data.boolean) {
       case kNone:
@@ -1187,15 +1135,6 @@ static OptVal get_option_newval(OptIndex opt_idx, int opt_flags, set_prefix_T pr
         newval_bool = kTrue;
         break;
       }
-    } else if (nextchar == '&') {
-      newval_bool = TRISTATE_FROM_INT(options[opt_idx].def_val.boolean);
-    } else if (nextchar == '<') {
-      // For 'autoread', kNone means to use global value.
-      if ((int *)varp == &curbuf->b_p_ar && opt_flags == OPT_LOCAL) {
-        newval_bool = kNone;
-      } else {
-        newval_bool = TRISTATE_FROM_INT(*(int *)get_varp_scope(&(options[opt_idx]), OPT_GLOBAL));
-      }
     } else {
       // ":set invopt": invert
       // ":set opt" or ":set noopt": set or reset
@@ -1214,31 +1153,15 @@ static OptVal get_option_newval(OptIndex opt_idx, int opt_flags, set_prefix_T pr
     OptInt newval_num;
 
     // Different ways to set a number option:
-    // &            set to default value
-    // <            set to global value
     // <xx>         accept special key codes for 'wildchar' or 'wildcharm'
     // ^x           accept ctrl key codes for 'wildchar' or 'wildcharm'
     // c            accept any non-digit for 'wildchar' or 'wildcharm'
     // [-]0-9       set number
     // other        error
     arg++;
-    if (nextchar == '&') {
-      newval_num = options[opt_idx].def_val.number;
-    } else if (nextchar == '<') {
-      if ((OptInt *)varp == &curbuf->b_p_ul && opt_flags == OPT_LOCAL) {
-        // for 'undolevels' NO_LOCAL_UNDOLEVEL means using the global newval_num
-        newval_num = NO_LOCAL_UNDOLEVEL;
-      } else if (opt_flags == OPT_LOCAL
-                 && ((OptInt *)varp == &curwin->w_p_siso || (OptInt *)varp == &curwin->w_p_so)) {
-        // for 'scrolloff'/'sidescrolloff' -1 means using the global newval_num
-        newval_num = -1;
-      } else {
-        newval_num = *(OptInt *)get_varp_scope(&(options[opt_idx]), OPT_GLOBAL);
-      }
-    } else if (((OptInt *)varp == &p_wc || (OptInt *)varp == &p_wcm)
-               && (*arg == '<' || *arg == '^'
-                   || (*arg != NUL && (!arg[1] || ascii_iswhite(arg[1]))
-                       && !ascii_isdigit(*arg)))) {
+    if (((OptInt *)varp == &p_wc || (OptInt *)varp == &p_wcm)
+        && (*arg == '<' || *arg == '^'
+            || (*arg != NUL && (!arg[1] || ascii_iswhite(arg[1])) && !ascii_isdigit(*arg)))) {
       newval_num = string_to_key(arg);
       if (newval_num == 0) {
         *errmsg = e_invarg;
@@ -3218,6 +3141,19 @@ bool optval_equal(OptVal o1, OptVal o2)
   UNREACHABLE;
 }
 
+/// Get type of option. Does not support multitype options.
+static OptValType option_get_type(const OptIndex opt_idx)
+{
+  assert(!option_is_multitype(opt_idx));
+
+  // If the option only supports a single type, it means that the index of the option's type flag
+  // corresponds to the value of the type enum. So get the index of the type flag using xctz() and
+  // use that as the option's type.
+  OptValType type = xctz(options[opt_idx].type_flags);
+  assert(type > kOptValTypeNil && type < kOptValTypeSize);
+  return type;
+}
+
 /// Create OptVal from var pointer.
 ///
 /// @param       opt_idx  Option index in options[] table.
@@ -3237,11 +3173,7 @@ OptVal optval_from_varp(OptIndex opt_idx, void *varp)
     return varp == NULL ? NIL_OPTVAL : *(OptVal *)varp;
   }
 
-  // If the option only supports a single type, it means that the index of the option's type flag
-  // corresponds to the value of the type enum. So get the index of the type flag using xctz() and
-  // use that as the option's type.
-  OptValType type = xctz(options[opt_idx].type_flags);
-  assert(type > kOptValTypeNil && type < kOptValTypeSize);
+  OptValType type = option_get_type(opt_idx);
 
   switch (type) {
   case kOptValTypeNil:
@@ -3390,6 +3322,11 @@ bool is_option_hidden(OptIndex opt_idx)
   return opt_idx == kOptInvalid ? false : get_varp(&options[opt_idx]) == NULL;
 }
 
+static inline bool option_is_global_local(OptIndex opt_idx)
+{
+  return opt_idx == kOptInvalid ? false : (options[opt_idx].indir & PV_BOTH);
+}
+
 /// Get option flags.
 ///
 /// @param  opt_idx  Option index in options[] table.
@@ -3588,10 +3525,9 @@ static const char *did_set_option(OptIndex opt_idx, void *varp, OptVal old_value
   opt->flags |= P_ALLOCED;
 
   const bool scope_both = (opt_flags & (OPT_LOCAL | OPT_GLOBAL)) == 0;
-  const bool opt_is_global_local = opt->indir & PV_BOTH;
 
   if (scope_both) {
-    if (opt_is_global_local) {
+    if (option_is_global_local(opt_idx)) {
       // Global option with local value set to use global value.
       // Free the local value and clear it.
       void *varp_local = get_varp_scope(opt, OPT_LOCAL);
@@ -3678,7 +3614,6 @@ static const char *validate_option_value(const OptIndex opt_idx, void *varp, Opt
     if (opt_flags == OPT_GLOBAL) {
       errmsg = _("Cannot unset global option value");
     } else {
-      optval_free(*newval);
       *newval = optval_copy(get_option_unset_value(opt_idx));
     }
   } else if (!option_has_type(opt_idx, newval->type)) {
@@ -3719,25 +3654,28 @@ static const char *set_option(const OptIndex opt_idx, void *varp, OptVal value,
 {
   assert(opt_idx != kOptInvalid);
 
-  const char *errmsg = validate_option_value(opt_idx, varp, &value, opt_flags, errbuf, errbuflen);
+  const char *errmsg = NULL;
 
-  if (errmsg != NULL) {
-    optval_free(value);
-    return errmsg;
+  if (!direct) {
+    errmsg = validate_option_value(opt_idx, varp, &value, opt_flags, errbuf, errbuflen);
+
+    if (errmsg != NULL) {
+      optval_free(value);
+      return errmsg;
+    }
   }
 
   vimoption_T *opt = &options[opt_idx];
   const bool scope_local = opt_flags & OPT_LOCAL;
   const bool scope_global = opt_flags & OPT_GLOBAL;
   const bool scope_both = !scope_local && !scope_global;
-  const bool opt_is_global_local = opt->indir & PV_BOTH;
   // Whether local value of global-local option is unset.
-  // NOTE: When this is true, it also implies that opt_is_global_local is true.
+  // NOTE: When this is true, it also implies that the option is global-local.
   const bool is_opt_local_unset = is_option_local_value_unset(opt_idx);
 
   // When using ":set opt=val" for a global option with a local value the local value will be reset,
   // use the global value here.
-  if (scope_both && opt_is_global_local) {
+  if (scope_both && option_is_global_local(opt_idx)) {
     varp = opt->var;
   }
 
@@ -3823,8 +3761,10 @@ void set_option_direct(OptIndex opt_idx, OptVal value, int opt_flags, scid_T set
   const bool scope_both = (opt_flags & (OPT_LOCAL | OPT_GLOBAL)) == 0;
   void *varp = get_varp_scope(opt, scope_both ? OPT_LOCAL : opt_flags);
 
-  set_option(opt_idx, varp, optval_copy(value), opt_flags, set_sid, true, true, errbuf,
-             sizeof(errbuf));
+  const char *errmsg = set_option(opt_idx, varp, optval_copy(value), opt_flags, set_sid, true, true,
+                                  errbuf, sizeof(errbuf));
+  assert(errmsg == NULL);
+  (void)errmsg;  // ignore unused warning
 }
 
 /// Set option value directly for buffer / window, without processing any side effects.
@@ -3893,6 +3833,17 @@ const char *set_option_value(const OptIndex opt_idx, const OptVal value, int opt
                     sizeof(errbuf));
 }
 
+/// Unset the local value of a global-local option.
+///
+/// @param      opt_idx    Index in options[] table. Must not be kOptInvalid.
+///
+/// @return  NULL on success, an untranslated error message on error.
+static inline const char *unset_option_local_value(const OptIndex opt_idx)
+{
+  assert(option_is_global_local(opt_idx));
+  return set_option_value(opt_idx, get_option_unset_value(opt_idx), OPT_LOCAL);
+}
+
 /// Set the value of an option. Supports TTY options, unlike set_option_value().
 ///
 /// @param      name       Option name. Used for error messages and for setting TTY options.
@@ -4273,7 +4224,7 @@ static int optval_default(OptIndex opt_idx, void *varp)
   }
 
   OptVal current_val = optval_from_varp(opt_idx, varp);
-  OptVal default_val = optval_from_varp(opt_idx, &opt->def_val);
+  OptVal default_val = opt->def_val;
 
   return optval_equal(current_val, default_val);
 }
@@ -5447,7 +5398,7 @@ void reset_modifiable(void)
 {
   curbuf->b_p_ma = false;
   p_ma = false;
-  options[kOptModifiable].def_val.boolean = false;
+  options[kOptModifiable].def_val = BOOLEAN_OPTVAL(false);
 }
 
 /// Set the global value for 'iminsert' to the local value.
@@ -6537,11 +6488,8 @@ static Dict vimoption2dict(vimoption_T *opt, int req_scope, buf_T *buf, win_T *w
   PUT_C(dict, "last_set_linenr", INTEGER_OBJ(last_set.script_ctx.sc_lnum));
   PUT_C(dict, "last_set_chan", INTEGER_OBJ((int64_t)last_set.channel_id));
 
-  // TODO(bfredl): do you even nocp?
-  OptVal def = optval_from_varp(get_opt_idx(opt), &opt->def_val);
-
-  PUT_C(dict, "type", CSTR_AS_OBJ(optval_type_get_name(def.type)));
-  PUT_C(dict, "default", optval_as_object(def));
+  PUT_C(dict, "type", CSTR_AS_OBJ(optval_type_get_name(option_get_type(get_opt_idx(opt)))));
+  PUT_C(dict, "default", optval_as_object(opt->def_val));
   PUT_C(dict, "allows_duplicates", BOOLEAN_OBJ(!(opt->flags & P_NODUP)));
 
   return dict;