refactor(options): split `get_option_value()` into smaller functions

Problem: Currently, `get_option_value()` returns 3 separate things: The actual value of the option, whether the option is hidden, and the option flags. This makes the function difficult to refactor, modify or otherwise reason about.

Solution: Split `get_option_value()` into 3 functions, each with a single purpose. This also affects `get_option_value_for()`.

6 files changed, 94 insertions, 121 deletions

diff --git a/src/nvim/api/options.c b/src/nvim/api/options.c
index ef7c11131a..53fd8af8b5 100644
--- a/src/nvim/api/options.c
+++ b/src/nvim/api/options.c
@@ -152,23 +152,20 @@ static buf_T *do_ft_buf(char *filetype, aco_save_T *aco, Error *err)
 Object nvim_get_option_value(String name, Dict(option) *opts, Error *err)
   FUNC_API_SINCE(9)
 {
-  Object rv = OBJECT_INIT;
-  OptVal value = NIL_OPTVAL;
-
   int scope = 0;
   OptReqScope req_scope = kOptReqGlobal;
   void *from = NULL;
   char *filetype = NULL;
 
   if (!validate_option_value_args(opts, name.data, &scope, &req_scope, &from, &filetype, err)) {
-    goto err;
+    return (Object)OBJECT_INIT;
   }
 
   aco_save_T aco;
 
   buf_T *ftbuf = do_ft_buf(filetype, &aco, err);
   if (ERROR_SET(err)) {
-    goto err;
+    return (Object)OBJECT_INIT;
   }
 
   if (ftbuf != NULL) {
@@ -176,8 +173,9 @@ Object nvim_get_option_value(String name, Dict(option) *opts, Error *err)
     from = ftbuf;
   }
 
-  bool hidden;
-  value = get_option_value_for(name.data, NULL, scope, &hidden, req_scope, from, err);
+  int opt_idx = findoption(name.data);
+  OptVal value = get_option_value_for(opt_idx, scope, req_scope, from, err);
+  bool hidden = is_option_hidden(opt_idx);
 
   if (ftbuf != NULL) {
     // restore curwin/curbuf and a few other things
@@ -198,7 +196,7 @@ Object nvim_get_option_value(String name, Dict(option) *opts, Error *err)
   return optval_as_object(value);
 err:
   optval_free(value);
-  return rv;
+  return (Object)OBJECT_INIT;
 }
 
 /// Sets the value of an option. The behavior of this function matches that of
@@ -391,6 +389,10 @@ static void restore_option_context(void *const ctx, OptReqScope req_scope)
 ///          See SOPT_* in option_defs.h for other flags.
 int get_option_attrs(char *name)
 {
+  if (is_tty_option(name)) {
+    return SOPT_GLOBAL;
+  }
+
   int opt_idx = findoption(name);
 
   if (opt_idx < 0) {
@@ -399,10 +401,6 @@ int get_option_attrs(char *name)
 
   vimoption_T *opt = get_option(opt_idx);
 
-  if (is_tty_option(opt->fullname)) {
-    return SOPT_GLOBAL;
-  }
-
   // Hidden option
   if (opt->var == NULL) {
     return 0;
@@ -470,14 +468,11 @@ static bool option_has_scope(char *name, OptReqScope req_scope)
 /// buffer-local value depending on opt_scope).
 OptVal get_option_value_strict(char *name, OptReqScope req_scope, void *from, Error *err)
 {
-  OptVal retv = NIL_OPTVAL;
-
   if (!option_has_scope(name, req_scope)) {
-    return retv;
+    return NIL_OPTVAL;
   }
-  if (get_tty_option(name, &retv.data.string.data)) {
-    retv.type = kOptValTypeString;
-    return retv;
+  if (is_tty_option(name)) {
+    return get_tty_option(name);
   }
 
   int opt_idx = findoption(name);
@@ -490,11 +485,11 @@ OptVal get_option_value_strict(char *name, OptReqScope req_scope, void *from, Er
                                       : (req_scope == kOptReqBuf ? (void *)&aco : NULL);
   bool switched = switch_option_context(ctx, req_scope, from, err);
   if (ERROR_SET(err)) {
-    return retv;
+    return NIL_OPTVAL;
   }
 
   char *varp = get_varp_scope(opt, req_scope == kOptReqGlobal ? OPT_GLOBAL : OPT_LOCAL);
-  retv = optval_from_varp(opt_idx, varp);
+  OptVal retv = optval_from_varp(opt_idx, varp);
 
   if (switched) {
     restore_option_context(ctx, req_scope);
@@ -505,7 +500,7 @@ OptVal get_option_value_strict(char *name, OptReqScope req_scope, void *from, Er
 
 /// Get option value for buffer / window.
 ///
-/// @param[in]   name       Option name.
+/// @param       opt_idx    Option index in options[] table.
 /// @param[out]  flagsp     Set to the option flags (P_xxxx) (if not NULL).
 /// @param[in]   scope      Option scope (can be OPT_LOCAL, OPT_GLOBAL or a combination).
 /// @param[out]  hidden     Whether option is hidden.
@@ -514,8 +509,8 @@ OptVal get_option_value_strict(char *name, OptReqScope req_scope, void *from, Er
 /// @param[out]  err        Error message, if any.
 ///
 /// @return  Option value. Must be freed by caller.
-OptVal get_option_value_for(const char *const name, uint32_t *flagsp, int scope, bool *hidden,
-                            const OptReqScope req_scope, void *const from, Error *err)
+OptVal get_option_value_for(int opt_idx, int scope, const OptReqScope req_scope, void *const from,
+                            Error *err)
 {
   switchwin_T switchwin;
   aco_save_T aco;
@@ -527,7 +522,7 @@ OptVal get_option_value_for(const char *const name, uint32_t *flagsp, int scope,
     return NIL_OPTVAL;
   }
 
-  OptVal retv = get_option_value(name, flagsp, scope, hidden);
+  OptVal retv = get_option_value(opt_idx, scope);
 
   if (switched) {
     restore_option_context(ctx, req_scope);
diff --git a/src/nvim/context.c b/src/nvim/context.c
index 28a67061c6..63c0f8c20c 100644
--- a/src/nvim/context.c
+++ b/src/nvim/context.c
@@ -138,7 +138,7 @@ bool ctx_restore(Context *ctx, const int flags)
     free_ctx = true;
   }
 
-  OptVal op_shada = get_option_value("shada", NULL, OPT_GLOBAL, NULL);
+  OptVal op_shada = get_option_value(findoption("shada"), OPT_GLOBAL);
   set_option_value("shada", STATIC_CSTR_AS_OPTVAL("!,'100,%"), OPT_GLOBAL);
 
   if (flags & kCtxRegs) {
diff --git a/src/nvim/eval.c b/src/nvim/eval.c
index ac1461056c..16c1231682 100644
--- a/src/nvim/eval.c
+++ b/src/nvim/eval.c
@@ -3784,37 +3784,32 @@ int eval_option(const char **const arg, typval_T *const rettv, const bool evalua
   }
 
   int ret = OK;
-  bool hidden;
   char c = *option_end;
   *option_end = NUL;
-  OptVal value = get_option_value(*arg, NULL, scope, &hidden);
 
-  if (rettv != NULL) {
-    switch (value.type) {
-    case kOptValTypeNil:
+  bool is_tty_opt = is_tty_option(*arg);
+  int opt_idx = is_tty_opt ? -1 : findoption(*arg);
+
+  if (opt_idx < 0 && !is_tty_opt) {
+    // Only give error if result is going to be used.
+    if (rettv != NULL) {
       semsg(_("E113: Unknown option: %s"), *arg);
-      ret = FAIL;
-      break;
-    case kOptValTypeBoolean:
-      rettv->v_type = VAR_NUMBER;
-      rettv->vval.v_number = value.data.boolean;
-      break;
-    case kOptValTypeNumber:
-      rettv->v_type = VAR_NUMBER;
-      rettv->vval.v_number = value.data.number;
-      break;
-    case kOptValTypeString:
-      rettv->v_type = VAR_STRING;
-      rettv->vval.v_string = value.data.string.data;
-      break;
     }
-  } else {
-    // Value isn't being used, free it.
-    optval_free(value);
 
-    if (value.type == kOptValTypeNil || (working && hidden)) {
-      ret = FAIL;
+    ret = FAIL;
+  } else if (rettv != NULL) {
+    OptVal value = is_tty_opt ? get_tty_option(*arg) : get_option_value(opt_idx, scope);
+    assert(value.type != kOptValTypeNil);
+
+    *rettv = optval_as_tv(value);
+
+    // Convert boolean option value to number for backwards compatibility.
+    if (rettv->v_type == VAR_BOOL) {
+      rettv->v_type = VAR_NUMBER;
+      rettv->vval.v_number = rettv->vval.v_bool == kBoolVarTrue ? 1 : 0;
     }
+  } else if (working && !is_tty_opt && is_option_hidden(opt_idx)) {
+    ret = FAIL;
   }
 
   *option_end = c;                  // put back for error messages
diff --git a/src/nvim/eval/vars.c b/src/nvim/eval/vars.c
index 670ee39f4b..8864edbf69 100644
--- a/src/nvim/eval/vars.c
+++ b/src/nvim/eval/vars.c
@@ -773,11 +773,14 @@ static char *ex_let_option(char *arg, typval_T *const tv, const bool is_const,
   const char c1 = *p;
   *p = NUL;
 
-  uint32_t opt_p_flags;
-  bool hidden;
-  OptVal curval = get_option_value(arg, &opt_p_flags, scope, &hidden);
+  bool is_tty_opt = is_tty_option(arg);
+  int opt_idx = is_tty_opt ? -1 : findoption(arg);
+  uint32_t opt_p_flags = get_option_flags(opt_idx);
+  bool hidden = is_option_hidden(opt_idx);
+  OptVal curval = is_tty_opt ? get_tty_option(arg) : get_option_value(opt_idx, scope);
   OptVal newval = NIL_OPTVAL;
-  if (curval.type == kOptValTypeNil && arg[0] != 't' && arg[1] != '_') {
+
+  if (curval.type == kOptValTypeNil) {
     semsg(_(e_unknown_option2), arg);
     goto theend;
   }
diff --git a/src/nvim/option.c b/src/nvim/option.c
index c9d65c5683..41d66adfc1 100644
--- a/src/nvim/option.c
+++ b/src/nvim/option.c
@@ -3099,51 +3099,36 @@ int findoption_len(const char *const arg, const size_t len)
 bool is_tty_option(const char *name)
   FUNC_ATTR_NONNULL_ALL FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT
 {
-  return (name[0] == 't' && name[1] == '_')
-         || strequal(name, "term")
-         || strequal(name, "ttytype");
+  return (name[0] == 't' && name[1] == '_') || strequal(name, "term") || strequal(name, "ttytype");
 }
 
 #define TCO_BUFFER_SIZE 8
-/// @param name TUI-related option
-/// @param[out,allocated] value option string value
-bool get_tty_option(const char *name, char **value)
+/// Get value of TTY option.
+///
+/// @param  name  Name of TTY option.
+///
+/// @return [allocated] TTY option value. Returns NIL_OPTVAL if option isn't a TTY option.
+OptVal get_tty_option(const char *name)
 {
-  if (strequal(name, "t_Co")) {
-    if (value) {
-      if (t_colors <= 1) {
-        *value = xstrdup("");
-      } else {
-        *value = xmalloc(TCO_BUFFER_SIZE);
-        snprintf(*value, TCO_BUFFER_SIZE, "%d", t_colors);
-      }
-    }
-    return true;
-  }
+  char *value = NULL;
 
-  if (strequal(name, "term")) {
-    if (value) {
-      *value = p_term ? xstrdup(p_term) : xstrdup("nvim");
-    }
-    return true;
-  }
-
-  if (strequal(name, "ttytype")) {
-    if (value) {
-      *value = p_ttytype ? xstrdup(p_ttytype) : xstrdup("nvim");
-    }
-    return true;
-  }
-
-  if (is_tty_option(name)) {
-    if (value) {
-      // XXX: All other t_* options were removed in 3baba1e7.
-      *value = xstrdup("");
+  if (strequal(name, "t_Co")) {
+    if (t_colors <= 1) {
+      value = xstrdup("");
+    } else {
+      value = xmalloc(TCO_BUFFER_SIZE);
+      snprintf(value, TCO_BUFFER_SIZE, "%d", t_colors);
     }
-    return true;
+  } else if (strequal(name, "term")) {
+    value = p_term ? xstrdup(p_term) : xstrdup("nvim");
+  } else if (strequal(name, "ttytype")) {
+    value = p_ttytype ? xstrdup(p_ttytype) : xstrdup("nvim");
+  } else if (is_tty_option(name)) {
+    // XXX: All other t_* options were removed in 3baba1e7.
+    value = xstrdup("");
   }
 
-  return false;
+  return value == NULL ? NIL_OPTVAL : CSTR_AS_OPTVAL(value);
 }
 
 bool set_tty_option(const char *name, char *value)
@@ -3412,7 +3397,7 @@ static OptVal optval_unset_local(int opt_idx, void *varp)
     }
   }
   // For options that aren't global-local, just set the local value to the global value.
-  return get_option_value(opt->fullname, NULL, OPT_GLOBAL, NULL);
+  return get_option_value(opt_idx, OPT_GLOBAL);
 }
 
 /// Get an allocated string containing a list of valid types for an option.
@@ -3458,46 +3443,41 @@ static char *option_get_valid_types(int opt_idx)
 #undef OPTION_ADD_TYPE
 }
 
-/// Gets the value for an option.
+/// Check if option is hidden.
 ///
-/// @param[in]   name    Option name.
-/// @param[out]  flagsp  Set to the option flags (P_xxxx) (if not NULL).
-/// @param[in]   scope   Option scope (can be OPT_LOCAL, OPT_GLOBAL or a combination).
-/// @param[out]  hidden  Whether option is hidden.
+/// @param  opt_idx  Option index in options[] table.
 ///
-/// @return [allocated] Option value. Returns NIL_OPTVAL for invalid options.
-OptVal get_option_value(const char *name, uint32_t *flagsp, int scope, bool *hidden)
+/// @return  True if option is hidden, false otherwise. Returns false if option name is invalid.
+bool is_option_hidden(int opt_idx)
 {
-  // Make sure that hidden and flagsp are never returned uninitialized
-  if (hidden != NULL) {
-    *hidden = false;
-  }
-  if (flagsp != NULL) {
-    *flagsp = 0;
-  }
+  return opt_idx < 0 ? false : get_varp(&options[opt_idx]) == NULL;
+}
 
-  char *str;
-  if (get_tty_option(name, &str)) {
-    return CSTR_AS_OPTVAL(str);
-  }
+/// Get option flags.
+///
+/// @param  opt_idx  Option index in options[] table.
+///
+/// @return  Option flags. Returns 0 for invalid option name.
+uint32_t get_option_flags(int opt_idx)
+{
+  return opt_idx < 0 ? 0 : options[opt_idx].flags;
+}
 
-  int opt_idx = findoption(name);
-  if (opt_idx < 0) {  // option not in the table
+/// Gets the value for an option.
+///
+/// @param       opt_idx  Option index in options[] table.
+/// @param[in]   scope    Option scope (can be OPT_LOCAL, OPT_GLOBAL or a combination).
+///
+/// @return [allocated] Option value. Returns NIL_OPTVAL for invalid option index.
+OptVal get_option_value(int opt_idx, int scope)
+{
+  if (opt_idx < 0) {  // option not in the options[] table.
     return NIL_OPTVAL;
   }
 
   vimoption_T *opt = &options[opt_idx];
   void *varp = get_varp_scope(opt, scope);
 
-  if (hidden != NULL) {
-    *hidden = varp == NULL;
-  }
-
-  if (flagsp != NULL) {
-    // Return the P_xxxx option flags.
-    *flagsp = opt->flags;
-  }
-
   return optval_copy(optval_from_varp(opt_idx, varp));
 }
 
diff --git a/src/nvim/spell.c b/src/nvim/spell.c
index 5065bee347..eb3bcec3ec 100644
--- a/src/nvim/spell.c
+++ b/src/nvim/spell.c
@@ -3218,7 +3218,7 @@ void ex_spelldump(exarg_T *eap)
   if (no_spell_checking(curwin)) {
     return;
   }
-  OptVal spl = get_option_value("spl", NULL, OPT_LOCAL, NULL);
+  OptVal spl = get_option_value(findoption("spl"), OPT_LOCAL);
 
   // Create a new empty buffer in a new window.
   do_cmdline_cmd("new");