add argparse unit tests and fix some bugs

This implements some unit tests for the new `argparse` command and fixes
a couple of bugs those tests brought to light.

Fixes #4190

src/builtin_argparse.cpp

@@ -173,12 +173,12 @@ static int parse_exclusive_args(argparse_cmd_opts_t &opts, io_streams_t &streams
 static bool parse_flag_modifiers(argparse_cmd_opts_t &opts, option_spec_t *opt_spec,
                                  const wcstring &option_spec, const wchar_t *s,
                                  io_streams_t &streams) {
-    if (*s == '=') {
+    if (*s == L'=') {
         s++;
-        if (*s == '?') {
+        if (*s == L'?') {
             opt_spec->num_allowed = -1;  // optional arg
             s++;
-        } else if (*s == '+') {
+        } else if (*s == L'+') {
             opt_spec->num_allowed = 2;  // mandatory arg and can appear more than once
             s++;
         } else {
@@ -208,9 +208,9 @@ static bool parse_option_spec(argparse_cmd_opts_t &opts, wcstring option_spec,
     const wchar_t *s = option_spec.c_str();
     option_spec_t *opt_spec = new option_spec_t(*s++);
 
-    if (*s == '/') {
+    if (*s == L'/') {
         s++;  // the struct is initialized assuming short_flag_valid should be true
-    } else if (*s == '-') {
+    } else if (*s == L'-') {
         opt_spec->short_flag_valid = false;
         s++;
     } else {
@@ -219,16 +219,17 @@ static bool parse_option_spec(argparse_cmd_opts_t &opts, wcstring option_spec,
         return parse_flag_modifiers(opts, opt_spec, option_spec, s, streams);
     }
 
+    // Collect the long flag name.
     const wchar_t *e = s;
-    while (*e && *e != '+' && *e != '=') e++;
+    while (*e && (*e == L'-' || *e == L'_' || iswalnum(*e))) e++;
     if (e == s) {
         streams.err.append_format(BUILTIN_ERR_INVALID_OPT_SPEC, opts.name.c_str(),
                                   option_spec.c_str(), *(s - 1));
         return false;
     }
-
     opt_spec->long_flag = wcstring(s, e - s);
     opts.long_to_short_flag.emplace(opt_spec->long_flag, opt_spec->short_flag);
+
     return parse_flag_modifiers(opts, opt_spec, option_spec, e, streams);
 }
 
@@ -236,7 +237,7 @@ static int collect_option_specs(argparse_cmd_opts_t &opts, int *optind, int argc
                                 io_streams_t &streams) {
     wchar_t *cmd = argv[0];
 
-    while (*optind < argc) {
+    while (true) {
         if (wcscmp(L"--", argv[*optind]) == 0) {
             ++*optind;
             break;
@@ -246,7 +247,10 @@ static int collect_option_specs(argparse_cmd_opts_t &opts, int *optind, int argc
             return STATUS_CMD_ERROR;
         }
 
-        ++*optind;
+        if (++*optind == argc) {
+            streams.err.append_format(_(L"%ls: Missing -- separator\n"), cmd);
+            return STATUS_INVALID_ARGS;
+        }
     }
 
     if (opts.options.empty()) {
@@ -317,6 +321,12 @@ static int parse_cmd_opts(argparse_cmd_opts_t &opts, int *optind,  //!OCLINT(hig
         }
     }
 
+    if (argc == w.woptind || wcscmp(L"--", argv[w.woptind - 1]) == 0) {
+        // The user didn't specify any option specs.
+        streams.err.append_format(_(L"%ls: No option specs were provided\n"), cmd);
+        return STATUS_INVALID_ARGS;
+    }
+
     *optind = w.woptind;
     return collect_option_specs(opts, optind, argc, argv, streams);
 }
@@ -417,9 +427,8 @@ static int argparse_parse_args(argparse_cmd_opts_t &opts, const wcstring_list_t
     const wchar_t *cmd = opts.name.c_str();
     int argc = static_cast<int>(args.size());
 
-    // This is awful but we need to convert our wcstring_list_t to a <wchar_t **> that can be passed
-    // to w.wgetopt_long(). Furthermore, because we're dynamically allocating the array of pointers
-    // we need to ensure the memory for the data structure is freed when we leave this scope.
+    // We need to convert our wcstring_list_t to a <wchar_t **> that can be used by wgetopt_long().
+    // This ensures the memory for the data structure is freed when we leave this scope.
     null_terminated_array_t<wchar_t> argv_container(args);
     auto argv = (wchar_t **)argv_container.get();
 
@@ -433,7 +442,17 @@ static int argparse_parse_args(argparse_cmd_opts_t &opts, const wcstring_list_t
     update_bool_flag_counts(opts);
 
     for (int i = optind; argv[i]; i++) opts.argv.push_back(argv[i]);
-    if (opts.min_args > 0 && opts.argv.size() < opts.min_args) {
+
+    return STATUS_CMD_OK;
+}
+
+static int check_min_max_args_constraints(argparse_cmd_opts_t &opts, const wcstring_list_t &args,
+                                          parser_t &parser, io_streams_t &streams) {
+    UNUSED(parser);
+    const wchar_t *cmd = opts.name.c_str();
+    int argc = static_cast<int>(args.size());
+
+    if (opts.argv.size() < opts.min_args) {
         streams.err.append_format(BUILTIN_ERR_MIN_ARG_COUNT1, cmd, opts.min_args, opts.argv.size());
         return STATUS_CMD_ERROR;
     }
@@ -466,11 +485,13 @@ int builtin_argparse(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         return STATUS_CMD_OK;
     }
 
+#if 0
     if (optind == argc) {
         // Apparently we weren't handed any arguments to be parsed according to the option specs we
         // just collected. So there isn't anything for us to do.
         return STATUS_CMD_OK;
     }
+#endif
 
     wcstring_list_t args;
     args.push_back(opts.name);
@@ -482,6 +503,9 @@ int builtin_argparse(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     retval = argparse_parse_args(opts, args, parser, streams);
     if (retval != STATUS_CMD_OK) return retval;
 
+    retval = check_min_max_args_constraints(opts, args, parser, streams);
+    if (retval != STATUS_CMD_OK) return retval;
+
     for (auto it : opts.options) {
         option_spec_t *opt_spec = it.second;
         if (!opt_spec->num_seen) continue;

tests/argparse.err

@@ -0,0 +1,16 @@
+# No args is an error
+argparse: No option specs were provided
+# Missing -- is an error
+argparse: Missing -- separator
+# Flags but no option specs is an error
+argparse: No option specs were provided
+# Invalid option specs
+argparse: Invalid option spec 'h-' at char '-'
+argparse: Invalid option spec '+help' at char 'h'
+argparse: Invalid option spec 'h/help:' at char ':'
+argparse: Invalid option spec 'h-help::' at char ':'
+argparse: Invalid option spec 'h-help=x' at char 'x'
+# --max-args and --min-args work
+min-max: Expected at least 1 args, got only 0
+min-max: Expected at most 3 args, got 4
+min-max: Expected at most 1 args, got 2

tests/argparse.in

@@ -0,0 +1,66 @@
+# Test the `argparse` builtin.
+
+##########
+# Start by verifying a bunch of error conditions.
+
+echo '# No args is an error' >&2
+argparse
+
+echo '# Missing -- is an error' >&2
+argparse h/help
+
+echo '# Flags but no option specs is an error' >&2
+argparse -s -- hello
+
+echo '# Invalid option specs' >&2
+argparse h-
+argparse +help
+argparse h/help:
+argparse h-help::
+argparse h-help=x
+
+echo '# --max-args and --min-args work' >&2
+begin
+    argparse --name min-max --min-args 1 h/help --
+    argparse --name min-max --min-args 1 --max-args 3 h/help -- arg1
+    argparse --name min-max --min-args 1 --max-args 3 h/help -- arg1 arg2
+    argparse --name min-max --min-args 1 --max-args 3 h/help -- --help arg1 arg2 arg3
+    argparse --name min-max --min-args 1 --max-args 3 h/help -- arg1 arg2 -h arg3 arg4
+    argparse --name min-max --max-args 1 h/help --
+    argparse --name min-max --max-args 1 h/help -- arg1
+    argparse --name min-max --max-args 1 h/help -- arg1 arg2
+end
+
+##########
+# Now verify that validly formed invocations work as expected.
+
+echo '# No args'
+argparse h/help --
+
+echo '# One arg and no matching flags'
+begin
+    argparse h/help -- help
+    set -l
+    show $argv
+end
+
+echo '# Five args with two matching a flag'
+begin
+    argparse h/help -- help --help me -h 'a lot more'
+    set -l
+    show $argv
+end
+
+echo '# Required, optional, and multiple flags'
+begin
+    argparse 'h/help' 'a/abc=' 'd/def=?' 'g/ghk=+' -- help --help me --ghk=g1 --abc=ABC --ghk g2 --d -g g3
+    set -l
+    show $argv
+end
+
+echo '# --stop-nonopt works'
+begin
+    argparse --stop-nonopt 'h/help' 'a/abc=' -- -a A1 -h --abc A2 non-opt 'second non-opt' --help
+    set -l
+    show $argv
+end

tests/argparse.out

@@ -0,0 +1,36 @@
+# No args
+# One arg and no matching flags
+argv help
+count=1
+|help|
+# Five args with two matching a flag
+_flag_h 2
+_flag_help 2
+argv 'help'  'me'  'a lot more'
+count=3
+|help|
+|me|
+|a lot more|
+# Required, optional, and multiple flags
+_flag_a ABC
+_flag_abc ABC
+_flag_d
+_flag_def
+_flag_g 'g1'  'g2'  'g3'
+_flag_ghk 'g1'  'g2'  'g3'
+_flag_h 1
+_flag_help 1
+argv 'help'  'me'
+count=2
+|help|
+|me|
+# --stop-nonopt works
+_flag_a A2
+_flag_abc A2
+_flag_h 1
+_flag_help 1
+argv 'non-opt'  'second non-opt'  '--help'
+count=3
+|non-opt|
+|second non-opt|
+|--help|

tests/test_functions/show.fish

@@ -0,0 +1,4 @@
+function show
+    echo count=(count $argv)
+    printf '|%s|\n' $argv
+end