feat(options)!: disallow setting hidden options #28400

Problem:
There are three different ways of marking an option as hidden, `enable_if
= false`, `hidden = true` and `immutable = true`. These also have different
behaviors. Options hidden with `enable_if = false` can't have their value
fetched using Vim script or the API, but options hidden with `hidden = true` or
`immutable = true` can. On the other hand, options with `hidden = true` do not
error when trying to set their value, but options with `immutable = true` do.

Solution:
Remove `enable_if = false`, remove the `hidden` property for options, and use
`immutable = true` to mark an option as hidden instead. Also make hidden option
variable pointers always point to the default value, which allows fetching the
value of every hidden option using Vim script and the API. This does also mean
that trying to set a hidden option will now give an error instead of just being
ignored.

7 files changed, 65 insertions, 78 deletions

diff --git a/src/nvim/api/deprecated.c b/src/nvim/api/deprecated.c
index 5c0d9c0cea..b3ba832fec 100644
--- a/src/nvim/api/deprecated.c
+++ b/src/nvim/api/deprecated.c
@@ -647,10 +647,6 @@ static bool option_has_scope(OptIndex opt_idx, OptReqScope req_scope)
 
   vimoption_T *opt = get_option(opt_idx);
 
-  // Hidden option.
-  if (opt->var == NULL) {
-    return false;
-  }
   // TTY option.
   if (is_tty_option(opt->fullname)) {
     return req_scope == kOptReqGlobal;
diff --git a/src/nvim/api/options.c b/src/nvim/api/options.c
index 1a0edd551e..96866d80ba 100644
--- a/src/nvim/api/options.c
+++ b/src/nvim/api/options.c
@@ -80,7 +80,7 @@ static int validate_option_value_args(Dict(option) *opts, char *name, OptIndex *
   *opt_idxp = find_option(name);
   int flags = get_option_attrs(*opt_idxp);
   if (flags == 0) {
-    // hidden or unknown option
+    // unknown option
     api_set_error(err, kErrorTypeValidation, "Unknown option '%s'", name);
   } else if (*req_scope == kOptReqBuf || *req_scope == kOptReqWin) {
     // if 'buf' or 'win' is passed, make sure the option supports it
@@ -175,7 +175,6 @@ Object nvim_get_option_value(String name, Dict(option) *opts, Error *err)
   }
 
   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
@@ -189,7 +188,7 @@ Object nvim_get_option_value(String name, Dict(option) *opts, Error *err)
     goto err;
   }
 
-  VALIDATE_S(!hidden && value.type != kOptValTypeNil, "option", name.data, {
+  VALIDATE_S(value.type != kOptValTypeNil, "option", name.data, {
     goto err;
   });
 
diff --git a/src/nvim/generators/gen_options.lua b/src/nvim/generators/gen_options.lua
index 8397a434e4..92349b5298 100644
--- a/src/nvim/generators/gen_options.lua
+++ b/src/nvim/generators/gen_options.lua
@@ -191,23 +191,17 @@ local function dump_option(i, o)
     w(get_cond(o.enable_if))
   end
 
-  -- An option cannot be both hidden and immutable.
-  assert(not o.hidden or not o.immutable)
-
-  local has_var = true
   if o.varname then
     w('    .var=&' .. o.varname)
-  elseif o.hidden or o.immutable then
-    -- Hidden and immutable options can directly point to the default value.
+  elseif o.immutable then
+    -- Immutable options can directly point to the default value.
     w(('    .var=&options[%u].def_val.data'):format(i - 1))
   elseif #o.scope == 1 and o.scope[1] == 'window' then
     w('    .var=VAR_WIN')
   else
-    has_var = false
+    -- Option must be immutable or have a variable.
+    assert(false)
   end
-  -- `enable_if = false` should be present iff there is no variable.
-  assert((o.enable_if == false) == not has_var)
-  w('    .hidden=' .. (o.hidden and 'true' or 'false'))
   w('    .immutable=' .. (o.immutable and 'true' or 'false'))
   if #o.scope == 1 and o.scope[1] == 'global' then
     w('    .indir=PV_NONE')
@@ -237,7 +231,10 @@ local function dump_option(i, o)
   end
   if o.enable_if then
     w('#else')
-    w('    .var=NULL')
+    -- Hidden option directly points to default value.
+    w(('    .var=&options[%u].def_val.data'):format(i - 1))
+    -- Option is always immutable on the false branch of `enable_if`.
+    w('    .immutable=true')
     w('    .indir=PV_NONE')
     w('#endif')
   end
diff --git a/src/nvim/option.c b/src/nvim/option.c
index 5d2e1ce4c6..783ec0abf4 100644
--- a/src/nvim/option.c
+++ b/src/nvim/option.c
@@ -565,13 +565,16 @@ static char *find_dup_item(char *origval, const char *newval, const size_t newva
 void free_all_options(void)
 {
   for (OptIndex opt_idx = 0; opt_idx < kOptIndexCount; opt_idx++) {
-    if (options[opt_idx].indir == PV_NONE) {
+    bool hidden = is_option_hidden(opt_idx);
+
+    if (options[opt_idx].indir == PV_NONE || hidden) {
       // global option: free value and default value.
-      if (options[opt_idx].var != NULL) {
+      // hidden option: free default value only.
+      if (!hidden) {
         optval_free(optval_from_varp(opt_idx, options[opt_idx].var));
       }
     } else if (options[opt_idx].var != VAR_WIN) {
-      // buffer-local option: free global value
+      // buffer-local option: free global value.
       optval_free(optval_from_varp(opt_idx, options[opt_idx].var));
     }
     optval_free(options[opt_idx].def_val);
@@ -1244,16 +1247,6 @@ static void do_one_set_option(int opt_flags, char **argp, bool *did_show, char *
   uint32_t flags = 0;  // flags for current option
   void *varp = NULL;  // pointer to variable for current option
 
-  if (options[opt_idx].var == NULL) {  // hidden option: skip
-    // Only give an error message when requesting the value of
-    // a hidden option, ignore setting it.
-    if (vim_strchr("=:!&<", nextchar) == NULL
-        && (!option_has_type(opt_idx, kOptValTypeBoolean) || nextchar == '?')) {
-      *errmsg = e_unsupportedoption;
-    }
-    return;
-  }
-
   flags = options[opt_idx].flags;
   varp = get_varp_scope(&(options[opt_idx]), opt_flags);
 
@@ -1325,11 +1318,6 @@ static void do_one_set_option(int opt_flags, char **argp, bool *did_show, char *
     }
   }
 
-  // Don't try to change hidden option.
-  if (varp == NULL) {
-    return;
-  }
-
   OptVal newval = get_option_newval(opt_idx, opt_flags, prefix, argp, nextchar, op, flags, varp,
                                     errbuf, errbuflen, errmsg);
 
@@ -1587,7 +1575,7 @@ char *find_shada_parameter(int type)
 static char *option_expand(OptIndex opt_idx, char *val)
 {
   // if option doesn't need expansion nothing to do
-  if (!(options[opt_idx].flags & kOptFlagExpand) || options[opt_idx].var == NULL) {
+  if (!(options[opt_idx].flags & kOptFlagExpand) || is_option_hidden(opt_idx)) {
     return NULL;
   }
 
@@ -2863,6 +2851,8 @@ static const char *validate_num_option(OptIndex opt_idx, void *varp, OptInt *new
     } else if (value > p_wiw) {
       return e_winwidth;
     }
+  } else if (varp == &p_mco) {
+    *newval = MAX_MCO;
   } else if (varp == &p_titlelen) {
     if (value < 0) {
       return e_positive;
@@ -3161,6 +3151,7 @@ static OptValType option_get_type(const OptIndex opt_idx)
 ///
 /// @return Option value stored in varp.
 OptVal optval_from_varp(OptIndex opt_idx, void *varp)
+  FUNC_ATTR_NONNULL_ARG(2)
 {
   // Special case: 'modified' is b_changed, but we also want to consider it set when 'ff' or 'fenc'
   // changed.
@@ -3170,7 +3161,7 @@ OptVal optval_from_varp(OptIndex opt_idx, void *varp)
 
   if (option_is_multitype(opt_idx)) {
     // Multitype options are stored as OptVal.
-    return varp == NULL ? NIL_OPTVAL : *(OptVal *)varp;
+    return *(OptVal *)varp;
   }
 
   OptValType type = option_get_type(opt_idx);
@@ -3179,11 +3170,11 @@ OptVal optval_from_varp(OptIndex opt_idx, void *varp)
   case kOptValTypeNil:
     return NIL_OPTVAL;
   case kOptValTypeBoolean:
-    return BOOLEAN_OPTVAL(varp == NULL ? false : TRISTATE_FROM_INT(*(int *)varp));
+    return BOOLEAN_OPTVAL(TRISTATE_FROM_INT(*(int *)varp));
   case kOptValTypeNumber:
-    return NUMBER_OPTVAL(varp == NULL ? 0 : *(OptInt *)varp);
+    return NUMBER_OPTVAL(*(OptInt *)varp);
   case kOptValTypeString:
-    return STRING_OPTVAL(varp == NULL ? (String)STRING_INIT : cstr_as_string(*(char **)varp));
+    return STRING_OPTVAL(cstr_as_string(*(char **)varp));
   }
   UNREACHABLE;
 }
@@ -3319,7 +3310,10 @@ static char *option_get_valid_types(OptIndex opt_idx)
 /// @return  True if option is hidden, false otherwise. Returns false if option name is invalid.
 bool is_option_hidden(OptIndex opt_idx)
 {
-  return opt_idx == kOptInvalid ? false : get_varp(&options[opt_idx]) == NULL;
+  // Hidden options are always immutable and point to their default value
+  return opt_idx == kOptInvalid
+         ? false
+         : (options[opt_idx].immutable && options[opt_idx].var == &options[opt_idx].def_val.data);
 }
 
 static inline bool option_is_global_local(OptIndex opt_idx)
@@ -3459,8 +3453,8 @@ static const char *did_set_option(OptIndex opt_idx, void *varp, OptVal old_value
     .os_win = curwin
   };
 
-  if (direct || opt->hidden) {
-    // Don't do any extra processing if setting directly or if option is hidden.
+  if (direct) {
+    // Don't do any extra processing if setting directly.
   }
   // Disallow changing immutable options.
   else if (opt->immutable && !optval_equal(old_value, new_value)) {
@@ -3489,7 +3483,7 @@ static const char *did_set_option(OptIndex opt_idx, void *varp, OptVal old_value
 
   // If option is hidden or if an error is detected, restore the previous value and don't do any
   // further processing.
-  if (opt->hidden || errmsg != NULL) {
+  if (errmsg != NULL) {
     set_option_varp(opt_idx, varp, old_value, true);
     // When resetting some values, need to act on it.
     if (restore_chartab) {
@@ -3749,7 +3743,7 @@ void set_option_direct(OptIndex opt_idx, OptVal value, int opt_flags, scid_T set
 
   vimoption_T *opt = get_option(opt_idx);
 
-  if (opt->var == NULL) {
+  if (is_option_hidden(opt_idx)) {
     return;
   }
 
@@ -3960,11 +3954,6 @@ int get_option_attrs(OptIndex opt_idx)
 
   vimoption_T *opt = get_option(opt_idx);
 
-  // Hidden option
-  if (opt->var == NULL) {
-    return 0;
-  }
-
   int attrs = 0;
 
   if (opt->indir == PV_NONE || (opt->indir & PV_BOTH)) {
@@ -3976,6 +3965,7 @@ int get_option_attrs(OptIndex opt_idx)
     attrs |= SOPT_BUF;
   }
 
+  assert(attrs != 0);
   return attrs;
 }
 
@@ -4143,8 +4133,8 @@ static int optval_default(OptIndex opt_idx, void *varp)
 {
   vimoption_T *opt = &options[opt_idx];
 
-  // Hidden or immutable options always use their default value.
-  if (varp == NULL || opt->hidden || opt->immutable) {
+  // Hidden options always use their default value.
+  if (is_option_hidden(opt_idx)) {
     return true;
   }
 
@@ -4554,9 +4544,9 @@ void *get_option_varp_scope_from(OptIndex opt_idx, int scope, buf_T *buf, win_T
 
 void *get_varp_from(vimoption_T *p, buf_T *buf, win_T *win)
 {
-  // hidden option, always return NULL
-  if (p->var == NULL) {
-    return NULL;
+  // hidden options always use the same var pointer
+  if (is_option_hidden(get_opt_idx(p))) {
+    return p->var;
   }
 
   switch ((int)p->indir) {
@@ -5456,7 +5446,7 @@ void set_context_in_set_cmd(expand_T *xp, char *arg, int opt_flags)
       }
       nextchar = *p;
       opt_idx = find_option_len(arg, (size_t)(p - arg));
-      if (opt_idx == kOptInvalid || options[opt_idx].var == NULL) {
+      if (opt_idx == kOptInvalid || is_option_hidden(opt_idx)) {
         xp->xp_context = EXPAND_NOTHING;
         return;
       }
@@ -5680,7 +5670,7 @@ int ExpandSettings(expand_T *xp, regmatch_T *regmatch, char *fuzzystr, int *numM
     char *str;
     for (OptIndex opt_idx = 0; opt_idx < kOptIndexCount; opt_idx++) {
       str = options[opt_idx].fullname;
-      if (options[opt_idx].var == NULL) {
+      if (is_option_hidden(opt_idx)) {
         continue;
       }
       if (xp->xp_context == EXPAND_BOOL_SETTINGS
@@ -5928,6 +5918,7 @@ int ExpandSettingSubtract(expand_T *xp, regmatch_T *regmatch, int *numMatches, c
 static void option_value2string(vimoption_T *opt, int scope)
 {
   void *varp = get_varp_scope(opt, scope);
+  assert(varp != NULL);
 
   if (option_has_type(get_opt_idx(opt), kOptValTypeNumber)) {
     OptInt wc = 0;
@@ -5944,9 +5935,8 @@ static void option_value2string(vimoption_T *opt, int scope)
     }
   } else {  // string
     varp = *(char **)(varp);
-    if (varp == NULL) {  // Just in case.
-      NameBuff[0] = NUL;
-    } else if (opt->flags & kOptFlagExpand) {
+
+    if (opt->flags & kOptFlagExpand) {
       home_replace(NULL, varp, NameBuff, MAXPATHL, false);
     } else {
       xstrlcpy(NameBuff, varp, MAXPATHL);
diff --git a/src/nvim/option.h b/src/nvim/option.h
index 9b74429467..138d90da97 100644
--- a/src/nvim/option.h
+++ b/src/nvim/option.h
@@ -49,7 +49,6 @@ typedef struct {
                             ///< buffer-local option: global value
   idopt_T indir;            ///< global option: PV_NONE;
                             ///< local option: indirect option index
-  bool hidden;              ///< option is hidden, any attempt to set its value will be ignored.
   bool immutable;           ///< option is immutable, trying to set its value will give an error.
 
   /// callback function to invoke after an option is modified to validate and
diff --git a/src/nvim/option_vars.h b/src/nvim/option_vars.h
index d59a4549d1..435f919ec0 100644
--- a/src/nvim/option_vars.h
+++ b/src/nvim/option_vars.h
@@ -530,6 +530,7 @@ EXTERN char *p_mef;             ///< 'makeef'
 EXTERN char *p_mp;              ///< 'makeprg'
 EXTERN char *p_mps;             ///< 'matchpairs'
 EXTERN OptInt p_mat;            ///< 'matchtime'
+EXTERN OptInt p_mco;            ///< 'maxcombine'
 EXTERN OptInt p_mfd;            ///< 'maxfuncdepth'
 EXTERN OptInt p_mmd;            ///< 'maxmapdepth'
 EXTERN OptInt p_mmp;            ///< 'maxmempattern'
diff --git a/src/nvim/options.lua b/src/nvim/options.lua
index 3c21436c3a..71f04a4dde 100644
--- a/src/nvim/options.lua
+++ b/src/nvim/options.lua
@@ -13,7 +13,7 @@
 --- @field list? 'comma'|'onecomma'|'commacolon'|'onecommacolon'|'flags'|'flagscomma'
 --- @field scope vim.option_scope[]
 --- @field deny_duplicates? boolean
---- @field enable_if? string|false
+--- @field enable_if? string
 --- @field defaults? vim.option_defaults
 --- @field secure? true
 --- @field noglob? true
@@ -87,11 +87,11 @@ return {
     {
       abbreviation = 'al',
       defaults = { if_true = 224 },
-      enable_if = false,
       full_name = 'aleph',
       scope = { 'global' },
       short_desc = N_('ASCII code of the letter Aleph (Hebrew)'),
       type = 'number',
+      immutable = true,
     },
     {
       abbreviation = 'ari',
@@ -789,11 +789,11 @@ return {
            current	Use the current directory.
            {path}	Use the specified directory
       ]=],
-      enable_if = false,
       full_name = 'browsedir',
       scope = { 'global' },
       short_desc = N_('which directory to start browsing in'),
       type = 'string',
+      immutable = true,
     },
     {
       abbreviation = 'bh',
@@ -1493,7 +1493,7 @@ return {
       cb = 'did_set_completeslash',
       defaults = { if_true = '' },
       desc = [=[
-        		only for MS-Windows
+        		only modifiable in MS-Windows
         When this option is set it overrules 'shellslash' for completion:
         - When this option is set to "slash", a forward slash is used for path
           completion in insert mode. This is useful when editing HTML tag, or
@@ -3791,12 +3791,12 @@ return {
         	try to keep 'lines' and 'columns' the same when adding and
         	removing GUI components.
       ]=],
-      enable_if = false,
       full_name = 'guioptions',
       list = 'flags',
       scope = { 'global' },
       short_desc = N_('GUI: Which components and options are used'),
       type = 'string',
+      immutable = true,
     },
     {
       abbreviation = 'gtl',
@@ -3816,13 +3816,13 @@ return {
         present in 'guioptions'.  For the non-GUI tab pages line 'tabline' is
         used.
       ]=],
-      enable_if = false,
       full_name = 'guitablabel',
       modelineexpr = true,
       redraw = { 'current_window' },
       scope = { 'global' },
       short_desc = N_('GUI: custom label for a tab page'),
       type = 'string',
+      immutable = true,
     },
     {
       abbreviation = 'gtt',
@@ -3835,12 +3835,12 @@ return {
         	let &guitabtooltip = "line one\nline two"
         <
       ]=],
-      enable_if = false,
       full_name = 'guitabtooltip',
       redraw = { 'current_window' },
       scope = { 'global' },
       short_desc = N_('GUI: custom tooltip for a tab page'),
       type = 'string',
+      immutable = true,
     },
     {
       abbreviation = 'hf',
@@ -4082,11 +4082,11 @@ return {
         English characters directly, e.g., when it's used to type accented
         characters with dead keys.
       ]=],
-      enable_if = false,
       full_name = 'imcmdline',
       scope = { 'global' },
       short_desc = N_('use IM when starting to edit a command line'),
       type = 'boolean',
+      immutable = true,
     },
     {
       abbreviation = 'imd',
@@ -4100,11 +4100,11 @@ return {
         Currently this option is on by default for SGI/IRIX machines.  This
         may change in later releases.
       ]=],
-      enable_if = false,
       full_name = 'imdisable',
       scope = { 'global' },
       short_desc = N_('do not use the IM in any mode'),
       type = 'boolean',
+      immutable = true,
     },
     {
       abbreviation = 'imi',
@@ -5236,7 +5236,7 @@ return {
       scope = { 'global' },
       short_desc = N_('maximum nr of combining characters displayed'),
       type = 'number',
-      hidden = true,
+      varname = 'p_mco',
     },
     {
       abbreviation = 'mfd',
@@ -5732,13 +5732,13 @@ return {
         indicate no input when the hit-enter prompt is displayed (since
         clicking the mouse has no effect in this state.)
       ]=],
-      enable_if = false,
       full_name = 'mouseshape',
       list = 'onecomma',
       scope = { 'global' },
       short_desc = N_('shape of the mouse pointer in different modes'),
       tags = { 'E547' },
       type = 'string',
+      immutable = true,
     },
     {
       abbreviation = 'mouset',
@@ -5898,11 +5898,11 @@ return {
         Note that on Windows editing "aux.h", "lpt1.txt" and the like also
         result in editing a device.
       ]=],
-      enable_if = false,
       full_name = 'opendevice',
       scope = { 'global' },
       short_desc = N_('allow reading/writing devices on MS-Windows'),
       type = 'boolean',
+      immutable = true,
     },
     {
       abbreviation = 'opfunc',
@@ -5975,11 +5975,11 @@ return {
     {
       abbreviation = 'pt',
       defaults = { if_true = '' },
-      enable_if = false,
       full_name = 'pastetoggle',
       scope = { 'global' },
       short_desc = N_('No description'),
       type = 'string',
+      immutable = true,
     },
     {
       abbreviation = 'pex',
@@ -7270,9 +7270,14 @@ return {
     {
       abbreviation = 'ssl',
       cb = 'did_set_shellslash',
-      defaults = { if_true = false },
+      defaults = {
+        condition = 'MSWIN',
+        if_true = false,
+        if_false = true,
+        doc = 'on, Windows: off',
+      },
       desc = [=[
-        		only for MS-Windows
+        		only modifiable in MS-Windows
         When set, a forward slash is used when expanding file names.  This is
         useful when a Unix-like shell is used instead of cmd.exe.  Backward
         slashes can still be typed, but they are changed to forward slashes by
@@ -8885,11 +8890,11 @@ return {
     {
       abbreviation = 'tenc',
       defaults = { if_true = '' },
-      enable_if = false,
       full_name = 'termencoding',
       scope = { 'global' },
       short_desc = N_('Terminal encoding'),
       type = 'string',
+      immutable = true,
     },
     {
       abbreviation = 'tgc',