From 4e1303823bbdd2a224f78f5fd03ec36905adff83 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Sun, 16 Jul 2017 21:10:22 -0700 Subject: [PATCH] add ability for `argparse` to validate args Fixes #4211 --- doc_src/argparse.txt | 18 ++++ share/functions/_validate_int.fish | 32 +++++++ src/builtin_argparse.cpp | 140 +++++++++++++++++++++++------ tests/argparse.err | 6 ++ tests/argparse.in | 27 ++++++ tests/argparse.out | 4 + 6 files changed, 198 insertions(+), 29 deletions(-) create mode 100644 share/functions/_validate_int.fish diff --git a/doc_src/argparse.txt b/doc_src/argparse.txt index b33198af1..ae763382d 100644 --- a/doc_src/argparse.txt +++ b/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 Flag Value Validation for more information. + See the `fish_opt` 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: diff --git a/share/functions/_validate_int.fish b/share/functions/_validate_int.fish new file mode 100644 index 000000000..629cf4c73 --- /dev/null +++ b/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 diff --git a/src/builtin_argparse.cpp b/src/builtin_argparse.cpp index f414a44dc..363328d8c 100644 --- a/src/builtin_argparse.cpp +++ b/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(), diff --git a/tests/argparse.err b/tests/argparse.err index c5b5d9d92..37bcf54ca 100644 --- a/tests/argparse.err +++ b/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' diff --git a/tests/argparse.in b/tests/argparse.in index cf467fc9d..24b9f7979 100644 --- a/tests/argparse.in +++ b/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 diff --git a/tests/argparse.out b/tests/argparse.out index 2031ae09a..79c13b377 100644 --- a/tests/argparse.out +++ b/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