use consistent mechanism to validate var names

Builtin commands that validate var names should use a consistent
mechanism. I noticed that builtin_read() had it's own custom code that
differed slightly from wcsvarname().

Fixes #3569

src/builtin.cpp

@@ -21,6 +21,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <limits.h>
+#include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -78,6 +79,27 @@ bool builtin_data_t::operator<(const builtin_data_t *other) const {
     return wcscmp(this->name, other->name) < 0;
 }
 
+static void builtin_append_format(wcstring &str, const wchar_t *fmt, ...) {
+    va_list ap;
+    va_start(ap, fmt);
+    append_formatv(str, fmt, ap);
+    va_end(ap);
+}
+
+bool builtin_is_valid_varname(const wchar_t *varname, wcstring &errstr, const wchar_t *cmd) {
+    const wchar_t *invalid_char = wcsvarname(varname);
+    if (!invalid_char) {
+        return true;
+    }
+
+    if (*invalid_char == L'\0') {
+        builtin_append_format(errstr, BUILTIN_ERR_VARNAME_ZERO, cmd);
+    } else {
+        builtin_append_format(errstr, BUILTIN_ERR_VARCHAR, cmd, *invalid_char);
+    }
+    return false;
+}
+
 /// Counts the number of arguments in the specified null-terminated array
 int builtin_count_args(const wchar_t *const *argv) {
     int argc;
@@ -1938,21 +1960,13 @@ static int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv)
     }
 
     // Verify all variable names.
+    wcstring errstr;
     for (i = w.woptind; i < argc; i++) {
-        wchar_t *src;
-
-        if (!wcslen(argv[i])) {
-            streams.err.append_format(BUILTIN_ERR_VARNAME_ZERO, argv[0]);
+        if (!builtin_is_valid_varname(argv[i], errstr, argv[0])) {
+            streams.err.append(errstr);
+            builtin_print_help(parser, streams, argv[0], streams.err);
             return STATUS_BUILTIN_ERROR;
         }
-
-        for (src = argv[i]; *src; src++) {
-            if ((!iswalnum(*src)) && (*src != L'_')) {
-                streams.err.append_format(BUILTIN_ERR_VARCHAR, argv[0], *src);
-                builtin_print_help(parser, streams, argv[0], streams.err);
-                return STATUS_BUILTIN_ERROR;
-            }
-        }
     }
 
     // The call to reader_readline may change woptind, so we save it away here.

src/builtin.h

@@ -109,6 +109,7 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
 void builtin_print_help(parser_t &parser, io_streams_t &streams, const wchar_t *cmd,
                         output_stream_t &b);
 int builtin_count_args(const wchar_t *const *argv);
+bool builtin_is_valid_varname(const wchar_t *varname, wcstring &errstr, const wchar_t *cmd);
 
 void builtin_unknown_option(parser_t &parser, io_streams_t &streams, const wchar_t *cmd,
                             const wchar_t *opt);

src/builtin_set.cpp

@@ -524,18 +524,11 @@ int builtin_set(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         *wcschr(dest, L'[') = 0;
     }
 
-    if (!wcslen(dest)) {
-        free(dest);
-        streams.err.append_format(BUILTIN_ERR_VARNAME_ZERO, argv[0]);
+    wcstring errstr;
+    if (!builtin_is_valid_varname(dest, errstr, argv[0])) {
+        streams.err.append(errstr);
         builtin_print_help(parser, streams, argv[0], streams.err);
-        return 1;
-    }
-
-    if ((bad_char = wcsvarname(dest))) {
-        streams.err.append_format(BUILTIN_ERR_VARCHAR, argv[0], *bad_char);
-        builtin_print_help(parser, streams, argv[0], streams.err);
-        free(dest);
-        return 1;
+        return STATUS_BUILTIN_ERROR;
     }
 
     // Set assignment can work in two modes, either using slices or using the whole array. We detect

src/env_universal_common.cpp

@@ -230,7 +230,7 @@ static bool append_file_entry(fish_message_type_t type, const wcstring &key_in,
     result->push_back(' ');
 
     // Append variable name like "fish_color_cwd".
-    if (wcsvarname(key_in.c_str())) {
+    if (wcsvarname(key_in)) {
         debug(0, L"Illegal variable name: '%ls'", key_in.c_str());
         success = false;
     }

src/parse_util.cpp

@@ -847,7 +847,7 @@ void parse_util_expand_variable_error(const wcstring &token, size_t global_token
             if (closing_bracket != wcstring::npos) {
                 size_t var_start = dollar_pos + 2, var_end = closing_bracket;
                 var_name = wcstring(token, var_start, var_end - var_start);
-                looks_like_variable = !var_name.empty() && wcsvarname(var_name.c_str()) == NULL;
+                looks_like_variable = wcsvarname(var_name) == NULL;
             }
             if (looks_like_variable) {
                 append_syntax_error(

src/wutil.cpp

@@ -484,6 +484,7 @@ int fish_iswgraph(wint_t wc) {
 ///
 /// \return null if this is a valid name, and a pointer to the first invalid character otherwise.
 const wchar_t *wcsvarname(const wchar_t *str) {
+    if (str[0] == L'\0') return str;
     while (*str) {
         if ((!fish_iswalnum(*str)) && (*str != L'_')) {
             return str;