add ability for argparse to validate args

Fixes #4211

doc_src/argparse.txt

@@ -74,10 +74,28 @@ Each option specification is a string composed of
 
 - `=+` if it requires a value each instance of the flag is saved.
 
+- Optionally a `!` followed by fish script to validate the value. Typically this will be a function to run. If the return status is zero the value for the flag is valid. If non-zero the value is invalid. Any error messages should be written to stdout (not stderr). See the section on <a href="#arparse-validation">Flag Value Validation</a> for more information.
+
 See the <a href="#fish-opt">`fish_opt`</a> command for a friendlier but more verbose way to create option specifications.
 
 In the following examples if a flag is not seen when parsing the arguments then the corresponding _flag_X var(s) will not be set.
 
+\subsection argparse-validation Flag Value Validation
+
+It is common to want to validate the the value provided for an option satisfies some criteria. For example, that it is a valid integer within a specific range. You can always do this after `argparse` returns but you can also request that `argparse` perform the validation by executing arbitrary fish script. To do so simply append an `!` (exclamation-mark) then the fish script to be run. When that code is executed three vars will be defined:
+
+- `_argparse_cmd` will be set to the value of the value of the `argparse --name` value.
+
+- `_flag_name` will be set to the short or long flag that being processed.
+
+- `_flag_value` will be set to the value associated with the flag being processed.
+
+If you do this via a function it should be defined with the `--no-scope-shadowing` flag. Otherwise it won't have access to those variables.
+
+The script should write any error messages to stdout, not stderr. It should return a status of zero if the flag value is valid otherwise a non-zero status to indicate it is invalid.
+
+Fish ships with a `_validate_int` function that accepts a `--min` and `--max` flag. Let's say your command accepts a `-m` or `--max` flag and the minimum allowable value is zero and the maximum is 5. You would define the option like this: `m/max=!_validate_int --min 0 --max 5`. The default if you just call `_validate_int` without those flags is to simply check that the value is a valid integer with no limits on the min or max value allowed.
+
 \subsection argparse-optspec-examples Example OPTION_SPECs
 
 Some OPTION_SPEC examples:

share/functions/_validate_int.fish

@@ -0,0 +1,32 @@
+# This function is intended to be used as a validation command for individual option specifications
+# given to the `argparse` command. It checks that the argument is a valid integer and optionally
+# whether it is in a reasonable range.
+function _validate_int --no-scope-shadowing
+    # Note that we can't use ourself to validate the arguments to the --min and --max flags this
+    # function recognizes.
+    set -l options 'm-min=' 'x-max='
+    argparse -n _argparse_validate_int $options -- $argv
+    or return
+
+    if not string match -qr '^-?\d+$' -- $_flag_value
+        set -l msg (_ "%s: Value '%s' for flag '%s' is not an integer\n")
+        printf $msg $_argparse_cmd $_flag_value $_flag_name
+        return 1
+    end
+
+    if set -q _flag_min
+        and test $_flag_value -lt $_flag_min
+        set -l msg (_ "%s: Value '%s' for flag '%s' less than min allowed of '%s'\n")
+        printf $msg $_argparse_cmd $_flag_value $_flag_name $_flag_min
+        return 1
+    end
+
+    if set -q _flag_max
+        and test $_flag_value -gt $_flag_max
+        set -l msg (_ "%s: Value '%s' for flag '%s' greater than max allowed of '%s'\n")
+        printf $msg $_argparse_cmd $_flag_value $_flag_name $_flag_max
+        return 1
+    end
+
+    return 0
+end

src/builtin_argparse.cpp

@@ -24,12 +24,14 @@
 #include "builtin_argparse.h"
 #include "common.h"
 #include "env.h"
+#include "exec.h"
 #include "fallback.h"  // IWYU pragma: keep
 #include "io.h"
 #include "wgetopt.h"  // IWYU pragma: keep
 #include "wutil.h"    // IWYU pragma: keep
 
 class parser_t;
+const wcstring var_name_prefix = L"_flag_";
 
 #define BUILTIN_ERR_INVALID_OPT_SPEC _(L"%ls: Invalid option spec '%ls' at char '%lc'\n")
 
@@ -37,13 +39,20 @@ class option_spec_t {
    public:
     wchar_t short_flag;
     wcstring long_flag;
+    wcstring validation_command;
     wcstring_list_t vals;
     bool short_flag_valid;
     int num_allowed;
     int num_seen;
 
     option_spec_t(wchar_t s)
-        : short_flag(s), long_flag(), vals(), short_flag_valid(true), num_allowed(0), num_seen(0) {}
+        : short_flag(s),
+          long_flag(),
+          validation_command(),
+          vals(),
+          short_flag_valid(true),
+          num_allowed(0),
+          num_seen(0) {}
 };
 
 class argparse_cmd_opts_t {
@@ -174,9 +183,10 @@ 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 (opt_spec->short_flag == opts.implicit_int_flag && *s) {
-        streams.err.append_format(_(L"%ls: Implicit int short flag '%lc' does not allow modifiers like '%lc'\n"),
-                                  opts.name.c_str(), opt_spec->short_flag, *s);
+    if (opt_spec->short_flag == opts.implicit_int_flag && *s && *s != L'!') {
+        streams.err.append_format(
+            _(L"%ls: Implicit int short flag '%lc' does not allow modifiers like '%lc'\n"),
+            opts.name.c_str(), opt_spec->short_flag, *s);
         return false;
     }
 
@@ -193,12 +203,20 @@ static bool parse_flag_modifiers(argparse_cmd_opts_t &opts, option_spec_t *opt_s
         }
     }
 
-    if (*s) {
+    if (*s == L'!') {
+        s++;
+        opt_spec->validation_command = wcstring(s);
+    } else if (*s) {
         streams.err.append_format(BUILTIN_ERR_INVALID_OPT_SPEC, opts.name.c_str(),
                                   option_spec.c_str(), *s);
         return false;
     }
 
+    // Make sure we have some validation for implicit int flags.
+    if (opt_spec->short_flag == opts.implicit_int_flag && opt_spec->validation_command.empty()) {
+        opt_spec->validation_command = L"_validate_int";
+    }
+
     if (opts.options.find(opt_spec->short_flag) != opts.options.end()) {
         streams.err.append_format(L"%ls: Short flag '%lc' already defined\n", opts.name.c_str(),
                                   opt_spec->short_flag);
@@ -234,8 +252,8 @@ static bool parse_option_spec(argparse_cmd_opts_t &opts, wcstring option_spec,
             return false;
         }
         if (opts.implicit_int_flag) {
-            streams.err.append_format(
-                    _(L"%ls: Implicit int flag '%lc' already defined\n"), opts.name.c_str(), opts.implicit_int_flag);
+            streams.err.append_format(_(L"%ls: Implicit int flag '%lc' already defined\n"),
+                                      opts.name.c_str(), opts.implicit_int_flag);
             return false;
         }
         opts.implicit_int_flag = opt_spec->short_flag;
@@ -248,8 +266,8 @@ static bool parse_option_spec(argparse_cmd_opts_t &opts, wcstring option_spec,
         s++;  // the struct is initialized assuming short_flag_valid should be true
     } else if (*s == L'#') {
         if (opts.implicit_int_flag) {
-            streams.err.append_format(
-                    _(L"%ls: Implicit int flag '%lc' already defined\n"), opts.name.c_str(), opts.implicit_int_flag);
+            streams.err.append_format(_(L"%ls: Implicit int flag '%lc' already defined\n"),
+                                      opts.name.c_str(), opts.implicit_int_flag);
             return false;
         }
         opts.implicit_int_flag = opt_spec->short_flag;
@@ -417,37 +435,91 @@ static void update_bool_flag_counts(argparse_cmd_opts_t &opts) {
     }
 }
 
+static int validate_arg(argparse_cmd_opts_t &opts, option_spec_t *opt_spec, bool is_long_flag,
+                        const wchar_t *woptarg, io_streams_t &streams) {
+    // Obviously if there is no arg validation command we assume the arg is okay.
+    if (opt_spec->validation_command.empty()) return STATUS_CMD_OK;
+
+    wcstring_list_t cmd_output;
+
+    env_push(true);
+    env_set(L"_argparse_cmd", opts.name.c_str(), ENV_LOCAL);
+    if (is_long_flag) {
+        env_set(var_name_prefix + L"name", opt_spec->long_flag.c_str(), ENV_LOCAL);
+    } else {
+        env_set(var_name_prefix + L"name", wcstring(1, opt_spec->short_flag).c_str(), ENV_LOCAL);
+    }
+    env_set(var_name_prefix + L"value", woptarg, ENV_LOCAL);
+
+    int retval = exec_subshell(opt_spec->validation_command, cmd_output, false);
+    for (auto it : cmd_output) {
+        streams.err.append(it);
+        streams.err.push_back(L'\n');
+    }
+    env_pop();
+    return retval;
+}
+
+// Check if it is an implicit integer option. Try to parse and validate it as a valid integer. The
+// code that parses option specs has already ensured that implicit integers have either an explicit
+// or implicit validation function.
+static int check_for_implicit_int(argparse_cmd_opts_t &opts, const wchar_t *val, const wchar_t *cmd,
+                                  const wchar_t *const *argv, wgetopter_t &w, int opt, int long_idx,
+                                  parser_t &parser, io_streams_t &streams) {
+    if (opts.implicit_int_flag == L'\0') {
+        // There is no implicit integer option so report an error.
+        builtin_unknown_option(parser, streams, cmd, argv[w.woptind - 1]);
+        return STATUS_INVALID_ARGS;
+    }
+
+    if (opt == '?') {
+        // It's a flag that might be an implicit integer flag. Try to parse it as an integer to
+        // decide if it is in fact something like `-NNN` or an invalid flag.
+        (void)fish_wcstol(val);
+        if (errno) {
+            builtin_unknown_option(parser, streams, cmd, argv[w.woptind - 1]);
+            return STATUS_INVALID_ARGS;
+        }
+    }
+
+    // Okay, it looks like an integer. See if it passes the validation checks.
+    auto found = opts.options.find(opts.implicit_int_flag);
+    assert(found != opts.options.end());
+    auto opt_spec = found->second;
+    int retval = validate_arg(opts, opt_spec, long_idx != -1, val, streams);
+    if (retval != STATUS_CMD_OK) return retval;
+
+    // It's a valid integer so store it and return success.
+    opt_spec->vals.clear();
+    opt_spec->vals.push_back(wcstring(val));
+    opt_spec->num_seen++;
+    w.nextchar = NULL;
+    return STATUS_CMD_OK;
+}
+
 static int argparse_parse_flags(argparse_cmd_opts_t &opts, const wchar_t *short_options,
                                 const woption *long_options, const wchar_t *cmd, int argc,
                                 wchar_t **argv, int *optind, parser_t &parser,
                                 io_streams_t &streams) {
     int opt;
+    int long_idx = -1;
     wgetopter_t w;
-    while ((opt = w.wgetopt_long(argc, argv, short_options, long_options, NULL)) != -1) {
+    while ((opt = w.wgetopt_long(argc, argv, short_options, long_options, &long_idx)) != -1) {
         if (opt == ':') {
             builtin_missing_argument(parser, streams, cmd, argv[w.woptind - 1]);
             return STATUS_INVALID_ARGS;
         }
+
         if (opt == '?') {
-            auto found = opts.options.find(opts.implicit_int_flag);
-            if (found != opts.options.end()) {
-                // Try to parse it as a number; e.g., "-123".
-                long x = fish_wcstol(argv[w.woptind - 1] + 1);
-                if (!errno) {
-                    wchar_t val[20];
-                    swprintf(val, sizeof val / sizeof val[0], L"%ld", x);
-                    auto opt_spec = found->second;
-                    opt_spec->vals.clear();
-                    opt_spec->vals.push_back(wcstring(val));
-                    opt_spec->num_seen++;
-                    w.nextchar = NULL;
-                    continue;
-                }
-            }
-            builtin_unknown_option(parser, streams, cmd, argv[w.woptind - 1]);
-            return STATUS_INVALID_ARGS;
+            // It's not a recognized flag. See if it's an implicit int flag.
+            int retval = check_for_implicit_int(opts, argv[w.woptind - 1] + 1, cmd, argv, w, opt,
+                                                long_idx, parser, streams);
+            if (retval != STATUS_CMD_OK) return retval;
+            long_idx = -1;
+            continue;
         }
 
+        // It's a recognized flag.
         auto found = opts.options.find(opt);
         assert(found != opts.options.end());
 
@@ -455,7 +527,16 @@ static int argparse_parse_flags(argparse_cmd_opts_t &opts, const wchar_t *short_
         opt_spec->num_seen++;
         if (opt_spec->num_allowed == 0) {
             assert(!w.woptarg);
-        } else if (opt_spec->num_allowed == -1 || opt_spec->num_allowed == 1) {
+            long_idx = -1;
+            continue;
+        }
+
+        if (w.woptarg) {
+            int retval = validate_arg(opts, opt_spec, long_idx != -1, w.woptarg, streams);
+            if (retval != STATUS_CMD_OK) return retval;
+        }
+
+        if (opt_spec->num_allowed == -1 || opt_spec->num_allowed == 1) {
             // We're depending on `wgetopt_long()` to report that a mandatory value is missing if
             // `opt_spec->num_allowed == 1` and thus return ':' so that we don't take this branch if
             // the mandatory arg is missing.
@@ -467,6 +548,8 @@ static int argparse_parse_flags(argparse_cmd_opts_t &opts, const wchar_t *short_
             assert(w.woptarg);
             opt_spec->vals.push_back(w.woptarg);
         }
+
+        long_idx = -1;
     }
 
     *optind = w.woptind;
@@ -533,7 +616,6 @@ static void set_argparse_result_vars(argparse_cmd_opts_t &opts) {
         option_spec_t *opt_spec = it.second;
         if (!opt_spec->num_seen) continue;
 
-        wcstring var_name_prefix = L"_flag_";
         auto val = list_to_array_val(opt_spec->vals);
         if (opt_spec->short_flag_valid) {
             env_set(var_name_prefix + opt_spec->short_flag, *val == ENV_NULL ? NULL : val->c_str(),

tests/argparse.err

@@ -29,3 +29,9 @@ argparse: Long flag 'short' already defined
 argparse: Implicit int flag '#' already defined
 # Defining an implicit int flag with modifiers
 argparse: Implicit int short flag 'v' does not allow modifiers like '='
+# Implicit int flag validation fails
+argparse: Value '765x' for flag 'max' is not an integer
+argparse: Value 'a1' for flag 'm' is not an integer
+# Explicit int flag validation
+argparse: Value '2' for flag 'm' greater than max allowed of '1'
+argparse: Value '-1' for flag 'max' less than min allowed of '0'

tests/argparse.in

@@ -98,3 +98,30 @@ echo '# Should be set to 765'
 for v in (set -l -n); set -e $v; end
 argparse 'm#max' -- argle -987 bargle --max 765
 set -l
+
+##########
+# Verify that flag value validation works.
+
+echo '# Implicit int flag validation fails' >&2
+argparse 'm#max' -- argle --max 765x bargle
+and echo unxpected argparse return status >&2
+argparse 'm#max' -- argle -ma1 bargle
+and echo unxpected argparse return status >&2
+
+echo '# Check the exit status from argparse validation'
+argparse 'm#max!set | grep _flag_; function x; return 57; end; x' -- argle --max=83 bargle 2>&1
+set -l saved_status $status
+test $saved_status -eq 57
+and echo expected argparse return status $saved_status
+
+echo '# Explicit int flag validation' >&2
+# These should fail
+argparse 'm#max!_validate_int --min 1 --max 1' -- argle -m2 bargle
+and echo unexpected argparse return status $status >&2
+argparse 'm#max!_validate_int --min 0 --max 1' -- argle --max=-1 bargle
+and echo unexpected argparse return status $status >&2
+# These should succeed
+argparse 'm/max=!_validate_int --min 0 --max 1' -- argle --max=0 bargle
+or echo unexpected argparse return status $status >&2
+argparse 'm/max=!_validate_int --min 0 --max 1' -- argle --max=1 bargle
+or echo unexpected argparse return status $status >&2

tests/argparse.out

@@ -51,3 +51,7 @@ argv 'argle'  'bargle'
 _flag_m 765
 _flag_max 765
 argv 'argle'  'bargle'
+# Check the exit status from argparse validation
+_flag_name max
+_flag_value 83
+expected argparse return status 57