From d9a92c2beacdd7868b38f495af79b6b940def8c1 Mon Sep 17 00:00:00 2001 From: Arkadiy Paronyan Date: Fri, 13 Oct 2017 16:34:21 +0200 Subject: [PATCH] CLI: Reject invalid argument values rather than ignore them (#6723) (#6747) * CLI: Reject invalid argument values rather than ignore them * Fix grumbles --- parity/cli/mod.rs | 10 +++++++++ parity/cli/usage.rs | 52 ++++++++++++++++++++++++++++----------------- 2 files changed, 43 insertions(+), 19 deletions(-) diff --git a/parity/cli/mod.rs b/parity/cli/mod.rs index 18d800546..774256d02 100644 --- a/parity/cli/mod.rs +++ b/parity/cli/mod.rs @@ -1203,6 +1203,16 @@ mod tests { use toml; use clap::{ErrorKind as ClapErrorKind}; + + #[test] + fn should_reject_invalid_values() { + let args = Args::parse(&["parity", "--cache=20"]); + assert!(args.is_ok()); + + let args = Args::parse(&["parity", "--cache=asd"]); + assert!(args.is_err()); + } + #[test] fn should_parse_args_and_flags() { let args = Args::parse(&["parity", "--no-warp"]).unwrap(); diff --git a/parity/cli/usage.rs b/parity/cli/usage.rs index dd370bdbc..83b878ee1 100644 --- a/parity/cli/usage.rs +++ b/parity/cli/usage.rs @@ -32,6 +32,20 @@ macro_rules! otry { ) } +macro_rules! return_if_parse_error { + ($e:expr) => ( + match $e { + Err(clap_error @ ClapError { kind: ClapErrorKind::ValueValidation, .. }) => { + return Err(clap_error); + }, + + // Otherwise, if $e is ClapErrorKind::ArgumentNotFound or Ok(), + // then convert to Option + _ => $e.ok() + } + ) +} + macro_rules! if_option { (Option<$type:ty>, THEN {$($then:tt)*} ELSE {$($otherwise:tt)*}) => ( $($then)* @@ -139,7 +153,7 @@ macro_rules! usage { use std::{fs, io, process}; use std::io::{Read, Write}; use util::version; - use clap::{Arg, App, SubCommand, AppSettings, Error as ClapError}; + use clap::{Arg, App, SubCommand, AppSettings, Error as ClapError, ErrorKind as ClapErrorKind}; use helpers::replace_home; use std::ffi::OsStr; use std::collections::HashMap; @@ -563,23 +577,23 @@ macro_rules! usage { raw_args.$flag = matches.is_present(stringify!($flag)); )* $( - raw_args.$arg = if_option!( + raw_args.$arg = return_if_parse_error!(if_option!( $($arg_type_tt)+, THEN { if_option_vec!( $($arg_type_tt)+, - THEN { values_t!(matches, stringify!($arg), inner_option_vec_type!($($arg_type_tt)+)).ok() } - ELSE { value_t!(matches, stringify!($arg), inner_option_type!($($arg_type_tt)+)).ok() } + THEN { values_t!(matches, stringify!($arg), inner_option_vec_type!($($arg_type_tt)+)) } + ELSE { value_t!(matches, stringify!($arg), inner_option_type!($($arg_type_tt)+)) } ) } ELSE { if_vec!( $($arg_type_tt)+, - THEN { values_t!(matches, stringify!($arg), inner_vec_type!($($arg_type_tt)+)).ok() } - ELSE { value_t!(matches, stringify!($arg), $($arg_type_tt)+).ok() } + THEN { values_t!(matches, stringify!($arg), inner_vec_type!($($arg_type_tt)+)) } + ELSE { value_t!(matches, stringify!($arg), $($arg_type_tt)+) } ) } - ); + )); )* )* @@ -594,23 +608,23 @@ macro_rules! usage { )* // Subcommand arguments $( - raw_args.$subc_arg = if_option!( + raw_args.$subc_arg = return_if_parse_error!(if_option!( $($subc_arg_type_tt)+, THEN { if_option_vec!( $($subc_arg_type_tt)+, - THEN { values_t!(submatches, stringify!($subc_arg), inner_option_vec_type!($($subc_arg_type_tt)+)).ok() } - ELSE { value_t!(submatches, stringify!($subc_arg), inner_option_type!($($subc_arg_type_tt)+)).ok() } + THEN { values_t!(submatches, stringify!($subc_arg), inner_option_vec_type!($($subc_arg_type_tt)+)) } + ELSE { value_t!(submatches, stringify!($subc_arg), inner_option_type!($($subc_arg_type_tt)+)) } ) } ELSE { if_vec!( $($subc_arg_type_tt)+, - THEN { values_t!(submatches, stringify!($subc_arg), inner_vec_type!($($subc_arg_type_tt)+)).ok() } - ELSE { value_t!(submatches, stringify!($subc_arg), $($subc_arg_type_tt)+).ok() } + THEN { values_t!(submatches, stringify!($subc_arg), inner_vec_type!($($subc_arg_type_tt)+)) } + ELSE { value_t!(submatches, stringify!($subc_arg), $($subc_arg_type_tt)+) } ) } - ); + )); )* // Sub-subcommands @@ -624,23 +638,23 @@ macro_rules! usage { )* // Sub-subcommand arguments $( - raw_args.$subc_subc_arg = if_option!( + raw_args.$subc_subc_arg = return_if_parse_error!(if_option!( $($subc_subc_arg_type_tt)+, THEN { if_option_vec!( $($subc_subc_arg_type_tt)+, - THEN { values_t!(subsubmatches, stringify!($subc_subc_arg), inner_option_vec_type!($($subc_subc_arg_type_tt)+)).ok() } - ELSE { value_t!(subsubmatches, stringify!($subc_subc_arg), inner_option_type!($($subc_subc_arg_type_tt)+)).ok() } + THEN { values_t!(subsubmatches, stringify!($subc_subc_arg), inner_option_vec_type!($($subc_subc_arg_type_tt)+)) } + ELSE { value_t!(subsubmatches, stringify!($subc_subc_arg), inner_option_type!($($subc_subc_arg_type_tt)+)) } ) } ELSE { if_vec!( $($subc_subc_arg_type_tt)+, - THEN { values_t!(subsubmatches, stringify!($subc_subc_arg), inner_vec_type!($($subc_subc_arg_type_tt)+)).ok() } - ELSE { value_t!(subsubmatches, stringify!($subc_subc_arg), $($subc_subc_arg_type_tt)+).ok() } + THEN { values_t!(subsubmatches, stringify!($subc_subc_arg), inner_vec_type!($($subc_subc_arg_type_tt)+)) } + ELSE { value_t!(subsubmatches, stringify!($subc_subc_arg), $($subc_subc_arg_type_tt)+) } ) } - ); + )); )* } else {