refactor set builtin

This completes the refactoring of the `set` builtin. It also removes a
seemingly never used feature of the `set` command. It also eliminates all
the lint warnings about this module.

Fixes #4236

CHANGELOG.md

@@ -4,6 +4,7 @@ This section is for changes merged to the `major` branch that are not also merge
 ## Notable fixes and improvements
 - `read` now requires at least one var name (#4220).
 - Local exported (`set -lx`) vars are now visible to functions (#1091).
+- `set x[1] x[2] a b` is no longer valid syntax (#4236).
 
 ## Other significant changes
 

doc_src/set.txt

@@ -44,7 +44,7 @@ The following options are available:
 
 If a variable is set to more than one value, the variable will be an array with the specified elements. If a variable is set to zero elements, it will become an array with zero elements.
 
-If the variable name is one or more array elements, such as `PATH[1 3 7]`, only those array elements specified will be changed. When array indices are specified to `set`, multiple arguments may be used to specify additional indexes, e.g. `set PATH[1] PATH[4] /bin /sbin`. If you specify a negative index when expanding or assigning to an array variable, the index will be calculated from the end of the array. For example, the index -1 means the last index of an array.
+If the variable name is one or more array elements, such as `PATH[1 3 7]`, only those array elements specified will be changed. If you specify a negative index when expanding or assigning to an array variable, the index will be calculated from the end of the array. For example, the index -1 means the last index of an array.
 
 The scoping rules when creating or updating a variable are:
 
@@ -92,3 +92,7 @@ if set python_path (type -p python)
 end
 # Outputs the path to Python if `type -p` returns true.
 \endfish
+
+\subsection set-notes Notes
+
+Fish versions prior to 3.0 allowed you to write `set PATH[1 4] /bin /sbin` as `set PATH[1] PATH[4] /bin /sbin`. That alternative syntax is ambiguous and inconsistent. Also, no fish script in the fish project used it. Nor could we find any 3rd-party scripts in the Oh-My-Fish or Fisherman package managers that used that alternative syntax. So it was removed in the 3.0 release.

src/builtin_set.cpp

@@ -8,7 +8,6 @@
 #include <sys/stat.h>
 #include <unistd.h>
 #include <wchar.h>
-#include <wctype.h>
 
 #include <algorithm>
 #include <iterator>
@@ -60,14 +59,12 @@ static const struct woption long_options[] = {{L"export", no_argument, NULL, 'x'
                                               {NULL, 0, NULL, 0}};
 
 // Error message for invalid path operations.
-#define BUILTIN_SET_PATH_ERROR L"%ls: Warning: $%ls entry \"%ls\" is not valid (%s)\n"
+#define BUILTIN_SET_PATH_ERROR _(L"%ls: Warning: $%ls entry \"%ls\" is not valid (%s)\n")
-
 // Hint for invalid path operation with a colon.
-#define BUILTIN_SET_PATH_HINT L"%ls: Did you mean 'set %ls $%ls %ls'?\n"
+#define BUILTIN_SET_PATH_HINT _(L"%ls: Did you mean 'set %ls $%ls %ls'?\n")
-
+#define BUILTIN_SET_MISMATCHED_ARGS _(L"%ls: You provided %d indexes but %d values\n")
-// Error for mismatch between index count and elements.
+#define BUILTIN_SET_ERASE_NO_VAR _(L"%ls: Erase needs a variable name\n")
-#define BUILTIN_SET_ARG_COUNT \
+#define BUILTIN_SET_ARRAY_BOUNDS_ERR _(L"%ls: Array index out of bounds\n")
-    L"%ls: The number of variable indexes does not match the number of values\n"
 
 // Test if the specified variable should be subject to path validation.
 static const wcstring_list_t path_variables({L"PATH", L"CDPATH"});
@@ -143,7 +140,7 @@ static int parse_cmd_opts(set_cmd_opts_t &opts, int *optind,  //!OCLINT(high ncs
     return STATUS_CMD_OK;
 }
 
-static int validate_cmd_opts(const wchar_t *cmd, set_cmd_opts_t &opts, int optind, int argc,
+static int validate_cmd_opts(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
                              parser_t &parser, io_streams_t &streams) {
     // Can't query and erase or list.
     if (opts.query && (opts.erase || opts.list)) {
@@ -173,8 +170,8 @@ static int validate_cmd_opts(const wchar_t *cmd, set_cmd_opts_t &opts, int optin
         return STATUS_INVALID_ARGS;
     }
 
-    if (optind == argc && opts.erase) {
+    if (argc == 0 && opts.erase) {
-        streams.err.append_format(_(L"%ls: Erase needs a variable name\n"), cmd);
+        streams.err.append_format(BUILTIN_SET_ERASE_NO_VAR, cmd);
         builtin_print_help(parser, streams, cmd, streams.err);
         return STATUS_INVALID_ARGS;
     }
@@ -182,100 +179,119 @@ static int validate_cmd_opts(const wchar_t *cmd, set_cmd_opts_t &opts, int optin
     return STATUS_CMD_OK;
 }
 
+// Check if we are setting a uvar and a global of the same name exists. See
+// https://github.com/fish-shell/fish-shell/issues/806
+static int check_global_scope_exists(const wchar_t *cmd, set_cmd_opts_t &opts, const wchar_t *dest,
+                                     io_streams_t &streams) {
+    if (opts.universal) {
+        env_var_t global_dest = env_get_string(dest, ENV_GLOBAL);
+        if (!global_dest.missing()) {
+            streams.err.append_format(
+                _(L"%ls: Warning: universal scope selected, but a global variable '%ls' exists.\n"),
+                cmd, dest);
+        }
+    }
+
+    return STATUS_CMD_OK;
+}
+
+// Fix for https://github.com/fish-shell/fish-shell/issues/199 . Return success if any path setting
+// succeeds.
+static int my_env_path_setup(const wchar_t *cmd, const wchar_t *key,  //!OCLINT(npath complexity)
+                             const wcstring_list_t &list, io_streams_t &streams) {
+    bool any_success = false;
+
+    // Don't bother validating (or complaining about) values that are already present. When
+    // determining already-present values, use ENV_DEFAULT instead of the passed-in scope because
+    // in:
+    //
+    //   set -l PATH stuff $PATH
+    //
+    // where we are temporarily shadowing a variable, we want to compare against the shadowed value,
+    // not the (missing) local value. Also don't bother to complain about relative paths, which
+    // don't start with /.
+    wcstring_list_t existing_values;
+    const env_var_t existing_variable = env_get_string(key, ENV_DEFAULT);
+    if (!existing_variable.missing_or_empty())
+        tokenize_variable_array(existing_variable, existing_values);
+
+    for (size_t i = 0; i < list.size(); i++) {
+        const wcstring &dir = list.at(i);
+        if (!string_prefixes_string(L"/", dir) || contains(existing_values, dir)) {
+            any_success = true;
+            continue;
+        }
+
+        bool show_hint = false;
+        bool error = false;
+        struct stat buff;
+        if (wstat(dir, &buff) == -1) {
+            error = true;
+        } else if (!S_ISDIR(buff.st_mode)) {
+            error = true;
+            errno = ENOTDIR;
+        } else if (waccess(dir, X_OK) == -1) {
+            error = true;
+        }
+
+        if (!error) {
+            any_success = true;
+        } else {
+            streams.err.append_format(BUILTIN_SET_PATH_ERROR, cmd, key, dir.c_str(),
+                                      strerror(errno));
+            const wchar_t *colon = wcschr(dir.c_str(), L':');
+            if (colon && *(colon + 1)) show_hint = true;
+        }
+
+        if (show_hint) {
+            streams.err.append_format(BUILTIN_SET_PATH_HINT, cmd, key, key,
+                                      wcschr(dir.c_str(), L':') + 1);
+        }
+    }
+
+    // Fail at setting the path if we tried to set it to something non-empty, but it wound up
+    // empty.
+    if (!list.empty() && !any_success) return STATUS_CMD_ERROR;
+    return STATUS_CMD_OK;
+}
+
 /// Call env_set. If this is a path variable, e.g. PATH, validate the elements. On error, print a
 /// description of the problem to stderr.
-static int my_env_set(const wchar_t *key, const wcstring_list_t &list, int scope,
+static int my_env_set(const wchar_t *cmd, const wchar_t *key, const wcstring_list_t &list,
-                      io_streams_t &streams) {
+                      int scope, io_streams_t &streams) {
+    int retval;
+
     if (is_path_variable(key)) {
-        // Fix for https://github.com/fish-shell/fish-shell/issues/199 . Return success if any path
+        retval = my_env_path_setup(cmd, key, list, streams);
-        // setting succeeds.
+        if (retval != STATUS_CMD_OK) return retval;
-        bool any_success = false;
-
-        // Don't bother validating (or complaining about) values that are already present. When
-        // determining already-present values, use ENV_DEFAULT instead of the passed-in scope
-        // because in:
-        //
-        //   set -l PATH stuff $PATH
-        //
-        // where we are temporarily shadowing a variable, we want to compare against the shadowed
-        // value, not the (missing) local value. Also don't bother to complain about relative paths,
-        // which don't start with /.
-        wcstring_list_t existing_values;
-        const env_var_t existing_variable = env_get_string(key, ENV_DEFAULT);
-        if (!existing_variable.missing_or_empty())
-            tokenize_variable_array(existing_variable, existing_values);
-
-        for (size_t i = 0; i < list.size(); i++) {
-            const wcstring &dir = list.at(i);
-            if (!string_prefixes_string(L"/", dir) || contains(existing_values, dir)) {
-                any_success = true;
-                continue;
-            }
-
-            int show_hint = 0;
-            bool error = false;
-            struct stat buff;
-            if (wstat(dir, &buff) == -1) {
-                error = true;
-            } else if (!S_ISDIR(buff.st_mode)) {
-                error = true;
-                errno = ENOTDIR;
-            } else if (waccess(dir, X_OK) == -1) {
-                error = true;
-            }
-
-            if (!error) {
-                any_success = true;
-            } else {
-                streams.err.append_format(_(BUILTIN_SET_PATH_ERROR), L"set", key, dir.c_str(),
-                                          strerror(errno));
-                const wchar_t *colon = wcschr(dir.c_str(), L':');
-
-                if (colon && *(colon + 1)) {
-                    show_hint = 1;
-                }
-            }
-
-            if (show_hint) {
-                streams.err.append_format(_(BUILTIN_SET_PATH_HINT), L"set", key, key,
-                                          wcschr(dir.c_str(), L':') + 1);
-            }
-        }
-
-        // Fail at setting the path if we tried to set it to something non-empty, but it wound up
-        // empty.
-        if (!list.empty() && !any_success) {
-            return STATUS_CMD_ERROR;
-        }
     }
 
     // We don't check `val->empty()` because an array var with a single empty string will be
     // "empty". A truly empty array var is set to the special value `ENV_NULL`.
     auto val = list_to_array_val(list);
-    int retcode = env_set(key, *val == ENV_NULL ? NULL : val->c_str(), scope | ENV_USER);
+    retval = env_set(key, *val == ENV_NULL ? NULL : val->c_str(), scope | ENV_USER);
-    switch (retcode) {
+    switch (retval) {
         case ENV_OK: {
-            retcode = STATUS_CMD_OK;
+            retval = STATUS_CMD_OK;
             break;
         }
         case ENV_PERM: {
             streams.err.append_format(_(L"%ls: Tried to change the read-only variable '%ls'\n"),
-                                      L"set", key);
+                                      cmd, key);
-            retcode = STATUS_CMD_ERROR;
+            retval = STATUS_CMD_ERROR;
             break;
         }
         case ENV_SCOPE: {
             streams.err.append_format(
-                _(L"%ls: Tried to set the special variable '%ls' with the wrong scope\n"), L"set",
+                _(L"%ls: Tried to set the special variable '%ls' with the wrong scope\n"), cmd,
                 key);
-            retcode = STATUS_CMD_ERROR;
+            retval = STATUS_CMD_ERROR;
             break;
         }
         case ENV_INVALID: {
             streams.err.append_format(
-                _(L"%ls: Tried to set the special variable '%ls' to an invalid value\n"), L"set",
+                _(L"%ls: Tried to set the special variable '%ls' to an invalid value\n"), cmd, key);
-                key);
+            retval = STATUS_CMD_ERROR;
-            retcode = STATUS_CMD_ERROR;
             break;
         }
         default: {
@@ -284,70 +300,55 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &list, int scope
         }
     }
 
-    return retcode;
+    return retval;
 }
 
-/// Extract indexes from a destination argument of the form name[index1 index2...]
+/// Extract indexes from an argument of the form `var_name[index1 index2...]`.
 ///
-/// \param indexes the list to insert the new indexes into
+/// Inputs:
-/// \param src the source string to parse
+///   indexes: the list to insert the new indexes into
-/// \param name the name of the element. Return null if the name in \c src does not match this name
+///   src: The source string to parse. This must be a var spec of the form "var[...]"
-/// \param var_count the number of elements in the array to parse.
 ///
-/// \return the total number of indexes parsed, or -1 on error
+/// Returns:
-static int parse_index(std::vector<long> &indexes, const wchar_t *src, const wchar_t *name,
+///   The total number of indexes parsed, or -1 on error. If any indexes were found the `src` string
-                       size_t var_count, io_streams_t &streams) {
+///   is modified to omit the index expression leaving just the var name.
-    size_t len;
+static int parse_index(std::vector<long> &indexes, wchar_t *src, int scope, io_streams_t &streams) {
+    wchar_t *p = wcschr(src, L'[');
+    if (!p) return 0;  // no slices so nothing for us to do
+    *p = L'\0';        // split the var name from the indexes/slices
+    p++;
+
+    env_var_t var_str = env_get_string(src, scope);
+    wcstring_list_t var;
+    if (!var_str.missing()) tokenize_variable_array(var_str, var);
+
     int count = 0;
-    const wchar_t *src_orig = src;
 
-    if (src == 0) {
+    while (*p != L']') {
-        return 0;
-    }
-
-    while (*src != L'\0' && (iswalnum(*src) || *src == L'_')) src++;
-
-    if (*src != L'[') {
-        streams.err.append_format(_(BUILTIN_SET_ARG_COUNT), L"set");
-        return 0;
-    }
-
-    len = src - src_orig;
-    if ((wcsncmp(src_orig, name, len) != 0) || (wcslen(name) != (len))) {
-        streams.err.append_format(
-            _(L"%ls: Multiple variable names specified in single call (%ls and %.*ls)\n"), L"set",
-            name, len, src_orig);
-        return 0;
-    }
-
-    src++;
-    while (iswspace(*src)) src++;
-
-    while (*src != L']') {
         const wchar_t *end;
-        long l_ind = fish_wcstol(src, &end);
+        long l_ind = fish_wcstol(p, &end);
         if (errno > 0) {  // ignore errno == -1 meaning the int did not end with a '\0'
             streams.err.append_format(_(L"%ls: Invalid index starting at '%ls'\n"), L"set", src);
-            return 0;
+            return -1;
         }
+        p = (wchar_t *)end;
 
-        if (l_ind < 0) l_ind = var_count + l_ind + 1;
+        // Convert negative index to a positive index.
+        if (l_ind < 0) l_ind = var.size() + l_ind + 1;
 
-        src = end;  //!OCLINT(parameter reassignment)
+        if (*p == L'.' && *(p + 1) == L'.') {
-        if (*src == L'.' && *(src + 1) == L'.') {
+            p += 2;
-            src += 2;
+            long l_ind2 = fish_wcstol(p, &end);
-            long l_ind2 = fish_wcstol(src, &end);
             if (errno > 0) {  // ignore errno == -1 meaning the int did not end with a '\0'
-                return 1;
+                return -1;
             }
-            src = end;  //!OCLINT(parameter reassignment)
+            p = (wchar_t *)end;
+
+            // Convert negative index to a positive index.
+            if (l_ind2 < 0) l_ind2 = var.size() + l_ind2 + 1;
 
-            if (l_ind2 < 0) {
-                l_ind2 = var_count + l_ind2 + 1;
-            }
             int direction = l_ind2 < l_ind ? -1 : 1;
             for (long jjj = l_ind; jjj * direction <= l_ind2 * direction; jjj += direction) {
-                // debug(0, L"Expand range [set]: %i\n", jjj);
                 indexes.push_back(jjj);
                 count++;
             }
@@ -355,8 +356,6 @@ static int parse_index(std::vector<long> &indexes, const wchar_t *src, const wch
             indexes.push_back(l_ind);
             count++;
         }
-
-        while (iswspace(*src)) src++;
     }
 
     return count;
@@ -364,10 +363,8 @@ static int parse_index(std::vector<long> &indexes, const wchar_t *src, const wch
 
 static int update_values(wcstring_list_t &list, std::vector<long> &indexes,
                          wcstring_list_t &values) {
-    size_t i;
-
     // Replace values where needed.
-    for (i = 0; i < indexes.size(); i++) {
+    for (size_t i = 0; i < indexes.size(); i++) {
         // The '- 1' below is because the indices in fish are one-based, but the vector uses
         // zero-based indices.
         long ind = indexes[i] - 1;
@@ -379,7 +376,6 @@ static int update_values(wcstring_list_t &list, std::vector<long> &indexes,
             list.resize(ind + 1);
         }
 
-        // free((void *) al_get(list, ind));
         list[ind] = newv;
     }
 
@@ -414,10 +410,15 @@ static int compute_scope(set_cmd_opts_t &opts) {
     return scope;
 }
 
-/// Print the names of all environment variables in the scope, with or without shortening, with or
+/// Print the names of all environment variables in the scope. It will include the values unless the
-/// without values, with or without escaping
+/// `set --list` flag was used.
-static int builtin_set_list(const wchar_t *cmd, set_cmd_opts_t &opts, int optind, int argc,
+static int builtin_set_list(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
                             wchar_t **argv, parser_t &parser, io_streams_t &streams) {
+    UNUSED(cmd);
+    UNUSED(argc);
+    UNUSED(argv);
+    UNUSED(parser);
+
     bool names_only = opts.list;
     wcstring_list_t names = env_get_names(compute_scope(opts));
     sort(names.begin(), names.end());
@@ -451,42 +452,33 @@ static int builtin_set_list(const wchar_t *cmd, set_cmd_opts_t &opts, int optind
 }
 
 // Query mode. Return the number of variables that do not exist out of the specified variables.
-static int builtin_set_query(const wchar_t *cmd, set_cmd_opts_t &opts, int optind, int argc,
+static int builtin_set_query(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
                              wchar_t **argv, parser_t &parser, io_streams_t &streams) {
     int retval = 0;
+    int scope = compute_scope(opts);
 
-    for (int i = optind; i < argc; i++) {
+    for (int i = 0; i < argc; i++) {
         wchar_t *arg = argv[i];
-        bool slice = false;
-
         wchar_t *dest = wcsdup(arg);
         assert(dest);
 
-        if (wchar_t *p = wcschr(dest, L'[')) {
+        std::vector<long> indexes;
-            *p = L'\0';
+        int idx_count = parse_index(indexes, dest, scope, streams);
-            slice = true;
+        if (idx_count == -1) {
+            free(dest);
+            builtin_print_help(parser, streams, cmd, streams.err);
+            return STATUS_CMD_ERROR;
         }
 
-        if (slice) {
+        if (idx_count) {
-            std::vector<long> indexes;
             wcstring_list_t result;
-            size_t j;
+            env_var_t dest_str = env_get_string(dest, scope);
-
-            env_var_t dest_str = env_get_string(dest, compute_scope(opts));
             if (!dest_str.missing()) tokenize_variable_array(dest_str, result);
 
-            if (!parse_index(indexes, arg, dest, result.size(), streams)) {
+            for (auto idx : indexes) {
-                builtin_print_help(parser, streams, cmd, streams.err);
+                if (idx < 1 || (size_t)idx > result.size()) retval++;
-                return STATUS_CMD_ERROR;
             }
-
+        } else if (!env_exist(arg, scope)) {
-            for (j = 0; j < indexes.size(); j++) {
-                long idx = indexes[j];
-                if (idx < 1 || (size_t)idx > result.size()) {
-                    retval++;
-                }
-            }
-        } else if (!env_exist(arg, compute_scope(opts))) {
             retval++;
         }
 
@@ -496,6 +488,118 @@ static int builtin_set_query(const wchar_t *cmd, set_cmd_opts_t &opts, int optin
     return retval;
 }
 
+/// Erase a variable.
+static int builtin_set_erase(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
+                             wchar_t **argv, parser_t &parser, io_streams_t &streams) {
+    if (argc != 1) {
+        streams.err.append_format(BUILTIN_ERR_ARG_COUNT2, cmd, L"--erase", 1, argc);
+        builtin_print_help(parser, streams, cmd, streams.err);
+        return STATUS_CMD_ERROR;
+    }
+
+    int scope = compute_scope(opts);  // calculate the variable scope based on the provided options
+    wchar_t *dest = argv[0];
+
+    std::vector<long> indexes;
+    int idx_count = parse_index(indexes, dest, scope, streams);
+    if (idx_count == -1) {
+        builtin_print_help(parser, streams, cmd, streams.err);
+        return STATUS_CMD_ERROR;
+    }
+
+    int retval;
+    if (!valid_var_name(dest)) {
+        streams.err.append_format(BUILTIN_ERR_VARNAME, cmd, dest);
+        builtin_print_help(parser, streams, cmd, streams.err);
+        return STATUS_INVALID_ARGS;
+    }
+
+    if (idx_count == 0) {  // unset the var
+        retval = env_remove(dest, scope);
+    } else {  // remove just the specified indexes of the var
+        const env_var_t dest_var = env_get_string(dest, scope);
+        if (dest_var.missing()) return STATUS_CMD_ERROR;
+        wcstring_list_t result;
+        tokenize_variable_array(dest_var, result);
+        erase_values(result, indexes);
+        retval = my_env_set(cmd, dest, result, scope, streams);
+    }
+
+    if (retval != STATUS_CMD_OK) return retval;
+    return check_global_scope_exists(cmd, opts, dest, streams);
+}
+
+/// This handles the more difficult case of setting individual slices of a var.
+static int set_slices(const wchar_t *cmd, set_cmd_opts_t &opts, const wchar_t *dest,
+                      std::vector<long> &indexes, int argc, wchar_t **argv, parser_t &parser,
+                      io_streams_t &streams) {
+    UNUSED(parser);
+
+    if (indexes.size() != static_cast<size_t>(argc)) {
+        streams.err.append_format(BUILTIN_SET_MISMATCHED_ARGS, cmd, indexes.size(), argc);
+    }
+
+    int scope = compute_scope(opts);  // calculate the variable scope based on the provided options
+    wcstring_list_t result;
+    const env_var_t dest_str = env_get_string(dest, scope);
+    if (!dest_str.missing()) tokenize_variable_array(dest_str, result);
+
+    // Slice indexes have been calculated, do the actual work.
+    wcstring_list_t new_values;
+    for (int i = 0; i < argc; i++) new_values.push_back(argv[i]);
+
+    int retval = update_values(result, indexes, new_values);
+    if (retval != STATUS_CMD_OK) {
+        streams.err.append_format(BUILTIN_SET_ARRAY_BOUNDS_ERR, cmd);
+        return retval;
+    }
+
+    retval = my_env_set(cmd, dest, result, scope, streams);
+    if (retval != STATUS_CMD_OK) return retval;
+    return check_global_scope_exists(cmd, opts, dest, streams);
+}
+
+/// Set a variable.
+static int builtin_set_set(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
+                           wchar_t **argv, parser_t &parser, io_streams_t &streams) {
+    if (argc == 0) {
+        streams.err.append_format(BUILTIN_ERR_MIN_ARG_COUNT1, cmd, 1);
+        builtin_print_help(parser, streams, cmd, streams.err);
+        return STATUS_INVALID_ARGS;
+    }
+
+    int retval;
+    int scope = compute_scope(opts);  // calculate the variable scope based on the provided options
+    wchar_t *dest = argv[0];
+    argv++;
+    argc--;
+
+    std::vector<long> indexes;
+    int idx_count = parse_index(indexes, dest, scope, streams);
+    if (idx_count == -1) {
+        builtin_print_help(parser, streams, cmd, streams.err);
+        return STATUS_INVALID_ARGS;
+    }
+
+    if (!valid_var_name(dest)) {
+        streams.err.append_format(BUILTIN_ERR_VARNAME, cmd, dest);
+        builtin_print_help(parser, streams, cmd, streams.err);
+        return STATUS_INVALID_ARGS;
+    }
+
+    if (idx_count != 0) {
+        // Handle the uncommon case of setting specific slices of a var.
+        return set_slices(cmd, opts, dest, indexes, argc, argv, parser, streams);
+    }
+
+    // This is the simple, common, case. Set the var to the specified values.
+    wcstring_list_t values;
+    for (int i = 0; i < argc; i++) values.push_back(argv[i]);
+    retval = my_env_set(cmd, dest, values, scope, streams);
+    if (retval != STATUS_CMD_OK) return retval;
+    return check_global_scope_exists(cmd, opts, dest, streams);
+}
+
 /// The set builtin creates, updates, and erases (removes, deletes) variables.
 int builtin_set(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     const int incoming_exit_status = proc_get_last_status();
@@ -506,126 +610,29 @@ int builtin_set(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     int optind;
     int retval = parse_cmd_opts(opts, &optind, argc, argv, parser, streams);
     if (retval != STATUS_CMD_OK) return retval;
+    argv += optind;
+    argc -= optind;
 
     if (opts.print_help) {
         builtin_print_help(parser, streams, cmd, streams.out);
         return STATUS_CMD_OK;
     }
 
-    retval = validate_cmd_opts(cmd, opts, optind, argc, parser, streams);
+    retval = validate_cmd_opts(cmd, opts, argc, parser, streams);
     if (retval != STATUS_CMD_OK) return retval;
 
-    if (opts.query) return builtin_set_query(cmd, opts, optind, argc, argv, parser, streams);
+    if (opts.query) {
-    if (opts.list || optind == argc) {
+        retval = builtin_set_query(cmd, opts, argc, argv, parser, streams);
-        return builtin_set_list(cmd, opts, optind, argc, argv, parser, streams);
+    } else if (opts.erase) {
-    }
+        retval = builtin_set_erase(cmd, opts, argc, argv, parser, streams);
-
+    } else if (opts.list) {  // explicit list the vars we know about
-    // Variables used for performing the actual work.
+        retval = builtin_set_list(cmd, opts, argc, argv, parser, streams);
-    int scope = compute_scope(opts);  // calculate the variable scope based on the provided options
+    } else if (argc == 0) {  // implicit list the vars we know about
-    wchar_t *dest = wcsdup(argv[optind]);
+        retval = builtin_set_list(cmd, opts, argc, argv, parser, streams);
-    assert(dest);
-
-    bool slice = false;
-    if (wchar_t *p = wcschr(dest, L'[')) {
-        *p = L'\0';
-        slice = true;
-    }
-
-    if (!valid_var_name(dest)) {
-        streams.err.append_format(BUILTIN_ERR_VARNAME, cmd, dest);
-        builtin_print_help(parser, streams, cmd, streams.err);
-        return STATUS_INVALID_ARGS;
-    }
-
-    // Set assignment can work in two modes, either using slices or using the whole array. We detect
-    // which mode is used here.
-    if (slice) {
-        std::vector<long> indexes;
-        wcstring_list_t result;
-
-        const env_var_t dest_str = env_get_string(dest, scope);
-        if (!dest_str.missing()) {
-            tokenize_variable_array(dest_str, result);
-        } else if (opts.erase) {
-            retval = STATUS_CMD_ERROR;
-        }
-
-        if (retval == STATUS_CMD_OK) {
-            for (; optind < argc; optind++) {
-                if (!parse_index(indexes, argv[optind], dest, result.size(), streams)) {
-                    builtin_print_help(parser, streams, cmd, streams.err);
-                    retval = STATUS_CMD_ERROR;
-                    break;
-                }
-
-                size_t idx_count = indexes.size();
-                size_t val_count = argc - optind - 1;
-
-                if (!opts.erase) {
-                    if (val_count < idx_count) {
-                        streams.err.append_format(_(BUILTIN_SET_ARG_COUNT), cmd);
-                        builtin_print_help(parser, streams, cmd, streams.err);
-                        retval = STATUS_CMD_ERROR;
-                        break;
-                    }
-                    if (val_count == idx_count) {
-                        optind++;
-                        break;
-                    }
-                }
-            }
-        }
-
-        if (retval == STATUS_CMD_OK) {
-            // Slice indexes have been calculated, do the actual work.
-            if (opts.erase) {
-                erase_values(result, indexes);
-                my_env_set(dest, result, scope, streams);
-            } else {
-                wcstring_list_t value;
-
-                while (optind < argc) {
-                    value.push_back(argv[optind++]);
-                }
-
-                if (update_values(result, indexes, value)) {
-                    streams.err.append_format(L"%ls: ", cmd);
-                    streams.err.append_format(ARRAY_BOUNDS_ERR);
-                    streams.err.push_back(L'\n');
-                }
-
-                my_env_set(dest, result, scope, streams);
-            }
-        }
     } else {
-        optind++;
+        retval = builtin_set_set(cmd, opts, argc, argv, parser, streams);
-        // No slicing.
-        if (opts.erase) {
-            if (optind != argc) {
-                streams.err.append_format(_(L"%ls: Values cannot be specfied with erase\n"), cmd);
-                builtin_print_help(parser, streams, cmd, streams.err);
-                retval = STATUS_CMD_ERROR;
-            } else {
-                retval = env_remove(dest, scope);
-            }
-        } else {
-            wcstring_list_t val;
-            for (int i = optind; i < argc; i++) val.push_back(argv[i]);
-            retval = my_env_set(dest, val, scope, streams);
-        }
     }
 
-    // Check if we are setting variables above the effective scope. See
-    // https://github.com/fish-shell/fish-shell/issues/806
-    env_var_t global_dest = env_get_string(dest, ENV_GLOBAL);
-    if (opts.universal && !global_dest.missing()) {
-        streams.err.append_format(
-            _(L"%ls: Warning: universal scope selected, but a global variable '%ls' exists.\n"),
-            L"set", dest);
-    }
-
-    free(dest);
-
     if (retval == STATUS_CMD_OK && opts.preserve_failure_exit_status) retval = incoming_exit_status;
     return retval;
 }

src/expand.h

@@ -89,9 +89,6 @@ enum expand_error_t {
     EXPAND_WILDCARD_MATCH
 };
 
-/// Error issued on array out of bounds.
-#define ARRAY_BOUNDS_ERR _(L"Array index out of bounds")
-
 /// Perform various forms of expansion on in, such as tilde expansion (\~USER becomes the users home
 /// directory), variable expansion (\$VAR_NAME becomes the value of the environment variable
 /// VAR_NAME), cmdsubst expansion and wildcard expansion. The results are inserted into the list

tests/test4.in

@@ -161,10 +161,10 @@ end;
 
 set -U -e baz
 
-echo "Verify subcommand statuses"
+echo "# Verify subcommand statuses"
 echo (false) $status (true) $status (false) $status
 
-echo "Verify that set passes through exit status, except when passed -n or -q or -e"
+echo "# Verify that set passes through exit status, except when passed -n or -q or -e"
 false ; set foo bar ; echo 1 $status # passthrough
 true ; set foo bar ; echo 2 $status # passthrough
 false ; set -q foo ; echo 3 $status # no passthrough
@@ -178,7 +178,7 @@ false ; set foo (echo A; true) ; echo 10 $status $foo
 true ; set foo (echo B; false) ; echo 11 $status $foo
 true
 
-echo "Verify set -ql behavior" # see 2502
+echo "# Verify set -ql behavior" # see 2502
 function setql_check
   set -l setql_foo val
   if set -ql setql_foo

tests/test4.out

@@ -20,9 +20,9 @@ Test 19 pass
 Test 20 pass
 Test 21 pass
 Test 22 pass
-Verify subcommand statuses
+# Verify subcommand statuses
 1 0 1
-Verify that set passes through exit status, except when passed -n or -q or -e
+# Verify that set passes through exit status, except when passed -n or -q or -e
 1 1
 2 0
 3 0
@@ -34,5 +34,5 @@ Verify that set passes through exit status, except when passed -n or -q or -e
 9 121
 10 0 A
 11 1 B
-Verify set -ql behavior
+# Verify set -ql behavior
 Pass