From 9824f8d0ba3550af87e18bccd6d9e847256ca893 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 11 Feb 2022 14:27:18 -0600 Subject: [PATCH 1/6] perf(validate): Remove unnecesarry walking of args --- src/parse/validator.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 6417f910b0e..080227e9802 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -54,16 +54,15 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { } } - let num_user_values = matcher - .arg_names() - .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) - .count(); - if num_user_values == 0 - && matcher.subcommand_name().is_none() - && self.p.app.is_arg_required_else_help_set() - { - let message = self.p.write_help_err()?; - return Err(Error::display_help_error(self.p.app, message)); + if matcher.subcommand_name().is_none() && self.p.app.is_arg_required_else_help_set() { + let num_user_values = matcher + .arg_names() + .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) + .count(); + if num_user_values == 0 { + let message = self.p.write_help_err()?; + return Err(Error::display_help_error(self.p.app, message)); + } } self.validate_conflicts(matcher)?; if !(self.p.app.is_subcommand_negates_reqs_set() && is_subcmd || reqs_validated) { From 8e5ce6c04499a5503efcb43e1211e96a6b0e23c2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 11 Feb 2022 14:36:42 -0600 Subject: [PATCH 2/6] refactor(validate): Drop us to one req check --- src/parse/validator.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 080227e9802..e276f3db8c6 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -23,7 +23,6 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { trailing_values: bool, ) -> ClapResult<()> { debug!("Validator::validate"); - let mut reqs_validated = false; #[cfg(feature = "env")] self.p.add_env(matcher, trailing_values)?; @@ -32,10 +31,8 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { if let ParseState::Opt(a) = parse_state { debug!("Validator::validate: needs_val_of={:?}", a); - self.validate_required(matcher)?; let o = &self.p.app[&a]; - reqs_validated = true; let should_err = if let Some(v) = matcher.args.get(&o.id) { v.all_val_groups_empty() && !(o.min_vals.is_some() && o.min_vals.unwrap() == 0) } else { @@ -65,7 +62,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { } } self.validate_conflicts(matcher)?; - if !(self.p.app.is_subcommand_negates_reqs_set() && is_subcmd || reqs_validated) { + if !(self.p.app.is_subcommand_negates_reqs_set() && is_subcmd) { self.validate_required(matcher)?; } self.validate_matched_args(matcher)?; From c0fd6753ea0ba386a65b0254d7203f8d8b47a18c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 11 Feb 2022 14:53:30 -0600 Subject: [PATCH 3/6] refactor(validate): Clarify subcmd check intent --- src/parse/parser.rs | 9 ++------- src/parse/validator.rs | 6 +++--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 192f3658f6f..9bbf56c1940 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -468,12 +468,7 @@ impl<'help, 'app> Parser<'help, 'app> { matches: sc_m.into_inner(), }); - return Validator::new(self).validate( - parse_state, - subcmd_name.is_some(), - matcher, - trailing_values, - ); + return Validator::new(self).validate(parse_state, matcher, trailing_values); } else { // Start error processing return Err(self.match_arg_error(&arg_os, valid_arg_found, trailing_values)); @@ -501,7 +496,7 @@ impl<'help, 'app> Parser<'help, 'app> { return Err(ClapError::display_help_error(self.app, message)); } - Validator::new(self).validate(parse_state, subcmd_name.is_some(), matcher, trailing_values) + Validator::new(self).validate(parse_state, matcher, trailing_values) } fn match_arg_error( diff --git a/src/parse/validator.rs b/src/parse/validator.rs index e276f3db8c6..50618fb6a67 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -18,11 +18,11 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { pub(crate) fn validate( &mut self, parse_state: ParseState, - is_subcmd: bool, matcher: &mut ArgMatcher, trailing_values: bool, ) -> ClapResult<()> { debug!("Validator::validate"); + let has_subcmd = matcher.subcommand_name().is_some(); #[cfg(feature = "env")] self.p.add_env(matcher, trailing_values)?; @@ -51,7 +51,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { } } - if matcher.subcommand_name().is_none() && self.p.app.is_arg_required_else_help_set() { + if !has_subcmd && self.p.app.is_arg_required_else_help_set() { let num_user_values = matcher .arg_names() .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) @@ -62,7 +62,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { } } self.validate_conflicts(matcher)?; - if !(self.p.app.is_subcommand_negates_reqs_set() && is_subcmd) { + if !(self.p.app.is_subcommand_negates_reqs_set() && has_subcmd) { self.validate_required(matcher)?; } self.validate_matched_args(matcher)?; From e8e469178cd1d5b361159396546a38dfbe21a31e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 11 Feb 2022 14:59:52 -0600 Subject: [PATCH 4/6] fix(validate): Give precedence to ArgRequiredElseHelp --- src/parse/parser.rs | 11 ----------- src/parse/validator.rs | 15 ++++++++++++++- tests/builder/app_settings.rs | 18 +++++++++++++++++- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 9bbf56c1940..45775ec8f1b 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -483,17 +483,6 @@ impl<'help, 'app> Parser<'help, 'app> { .name .clone(); self.parse_subcommand(&sc_name, matcher, it, keep_state)?; - } else if self.app.is_subcommand_required_set() { - let bn = self.app.bin_name.as_ref().unwrap_or(&self.app.name); - return Err(ClapError::missing_subcommand( - self.app, - bn.to_string(), - Usage::new(self.app, &self.required).create_usage_with_title(&[]), - )); - } else if self.is_set(AS::SubcommandRequiredElseHelp) { - debug!("Parser::get_matches_with: SubcommandRequiredElseHelp=true"); - let message = self.write_help_err()?; - return Err(ClapError::display_help_error(self.app, message)); } Validator::new(self).validate(parse_state, matcher, trailing_values) diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 50618fb6a67..aba2888b0f6 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -1,5 +1,5 @@ // Internal -use crate::build::{App, Arg, ArgPredicate, PossibleValue}; +use crate::build::{App, AppSettings, Arg, ArgPredicate, PossibleValue}; use crate::error::{Error, Result as ClapResult}; use crate::output::Usage; use crate::parse::{ArgMatcher, MatchedArg, ParseState, Parser}; @@ -61,6 +61,19 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { return Err(Error::display_help_error(self.p.app, message)); } } + if !has_subcmd && self.p.app.is_subcommand_required_set() { + let bn = self.p.app.bin_name.as_ref().unwrap_or(&self.p.app.name); + return Err(Error::missing_subcommand( + self.p.app, + bn.to_string(), + Usage::new(self.p.app, &self.p.required).create_usage_with_title(&[]), + )); + } else if !has_subcmd && self.p.app.is_set(AppSettings::SubcommandRequiredElseHelp) { + debug!("Validator::new::get_matches_with: SubcommandRequiredElseHelp=true"); + let message = self.p.write_help_err()?; + return Err(Error::display_help_error(self.p.app, message)); + } + self.validate_conflicts(matcher)?; if !(self.p.app.is_subcommand_negates_reqs_set() && has_subcmd) { self.validate_required(matcher)?; diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs index 6b3bedd1e14..68056337278 100644 --- a/tests/builder/app_settings.rs +++ b/tests/builder/app_settings.rs @@ -194,7 +194,7 @@ fn arg_required_else_help() { } #[test] -fn arg_required_else_help_over_reqs() { +fn arg_required_else_help_over_req_arg() { let result = App::new("arg_required") .arg_required_else_help(true) .arg(Arg::new("test").index(1).required(true)) @@ -208,6 +208,22 @@ fn arg_required_else_help_over_reqs() { ); } +#[test] +fn arg_required_else_help_over_req_subcommand() { + let result = App::new("sub_required") + .arg_required_else_help(true) + .subcommand_required(true) + .subcommand(App::new("sub1")) + .try_get_matches_from(vec![""]); + + assert!(result.is_err()); + let err = result.err().unwrap(); + assert_eq!( + err.kind(), + ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand + ); +} + #[test] fn arg_required_else_help_with_default() { let result = App::new("arg_required") From 8f201d8dd67e56da7d654056395ebe0f25d9e337 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 11 Feb 2022 14:40:04 -0600 Subject: [PATCH 5/6] docs: Stop demonstrating SubcommandRequiredElseHelp --- examples/git.rs | 5 +++-- examples/pacman.rs | 5 +++-- examples/tutorial_builder/03_04_subcommands.rs | 9 ++++----- examples/tutorial_builder/README.md | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/examples/git.rs b/examples/git.rs index 8d7108cad67..cfb6848f4ab 100644 --- a/examples/git.rs +++ b/examples/git.rs @@ -2,12 +2,13 @@ use std::path::PathBuf; -use clap::{arg, App, AppSettings}; +use clap::{arg, App}; fn main() { let matches = App::new("git") .about("A fictional versioning CLI") - .setting(AppSettings::SubcommandRequiredElseHelp) + .subcommand_required(true) + .arg_required_else_help(true) .allow_external_subcommands(true) .allow_invalid_utf8_for_external_subcommands(true) .subcommand( diff --git a/examples/pacman.rs b/examples/pacman.rs index 5b42f0d5b23..155e31d6cb2 100644 --- a/examples/pacman.rs +++ b/examples/pacman.rs @@ -1,10 +1,11 @@ -use clap::{App, AppSettings, Arg}; +use clap::{App, Arg}; fn main() { let matches = App::new("pacman") .about("package manager utility") .version("5.2.1") - .setting(AppSettings::SubcommandRequiredElseHelp) + .subcommand_required(true) + .arg_required_else_help(true) .author("Pacman Development Team") // Query subcommand // diff --git a/examples/tutorial_builder/03_04_subcommands.rs b/examples/tutorial_builder/03_04_subcommands.rs index 98d35ef7b3f..d5900824ae6 100644 --- a/examples/tutorial_builder/03_04_subcommands.rs +++ b/examples/tutorial_builder/03_04_subcommands.rs @@ -1,9 +1,10 @@ -use clap::{app_from_crate, arg, App, AppSettings}; +use clap::{app_from_crate, arg, App}; fn main() { let matches = app_from_crate!() .propagate_version(true) - .setting(AppSettings::SubcommandRequiredElseHelp) + .subcommand_required(true) + .arg_required_else_help(true) .subcommand( App::new("add") .about("Adds files to myapp") @@ -16,8 +17,6 @@ fn main() { "'myapp add' was used, name is: {:?}", sub_matches.value_of("NAME") ), - _ => unreachable!( - "Exhausted list of subcommands and SubcommandRequiredElseHelp prevents `None`" - ), + _ => unreachable!("Exhausted list of subcommands and subcommand_required prevents `None`"), } } diff --git a/examples/tutorial_builder/README.md b/examples/tutorial_builder/README.md index 0679f6ff946..b5ebc695de7 100644 --- a/examples/tutorial_builder/README.md +++ b/examples/tutorial_builder/README.md @@ -306,7 +306,7 @@ $ 03_04_subcommands add bob ``` -Because we set `AppSettings::SubcommandRequiredElseHelp`: +Because we set `App::arg_required_else_help`: ```console $ 03_04_subcommands ? failed From 4895a32e81d39c19aa3aeeae50cf9d96cb0acdbc Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 11 Feb 2022 15:16:36 -0600 Subject: [PATCH 6/6] fix: Deprecate SubcommandRequiredElseHelp Now that we can use `SubcommandRequired | ArgRequiredElseHelp`, this setting offers little value but requires we track required subcommands with two different settings. Deprecating as the cost is not worth the benefit anymore. Issue #3280 will see the derive updated --- clap_derive/src/derives/args.rs | 1 + clap_derive/src/derives/into_app.rs | 1 + clap_derive/src/derives/subcommand.rs | 1 + clap_mangen/src/render.rs | 1 + src/build/app_settings.rs | 25 ++++++------------------- src/output/usage.rs | 1 + src/parse/validator.rs | 1 + tests/builder/app_settings.rs | 2 ++ 8 files changed, 14 insertions(+), 19 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 291b6287d24..3bdd32010ee 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -168,6 +168,7 @@ pub fn gen_augment( quote!() } else { quote_spanned! { kind.span()=> + #[allow(deprecated)] let #app_var = #app_var.setting( clap::AppSettings::SubcommandRequiredElseHelp ); diff --git a/clap_derive/src/derives/into_app.rs b/clap_derive/src/derives/into_app.rs index a7842707a93..3df12f5b39e 100644 --- a/clap_derive/src/derives/into_app.rs +++ b/clap_derive/src/derives/into_app.rs @@ -103,6 +103,7 @@ pub fn gen_for_enum(enum_name: &Ident, generics: &Generics, attrs: &[Attribute]) #[deny(clippy::correctness)] impl #impl_generics clap::IntoApp for #enum_name #ty_generics #where_clause { fn into_app<'b>() -> clap::App<'b> { + #[allow(deprecated)] let #app_var = clap::App::new(#name) .setting(clap::AppSettings::SubcommandRequiredElseHelp); ::augment_subcommands(#app_var) diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs index 1e53375fb9a..c6c7738f5c1 100644 --- a/clap_derive/src/derives/subcommand.rs +++ b/clap_derive/src/derives/subcommand.rs @@ -248,6 +248,7 @@ fn gen_augment( let #subcommand_var = clap::App::new(#name); let #subcommand_var = #subcommand_var #initial_app_methods; let #subcommand_var = #arg_block; + #[allow(deprecated)] let #subcommand_var = #subcommand_var.setting(clap::AppSettings::SubcommandRequiredElseHelp); #subcommand_var #final_from_attrs }); diff --git a/clap_mangen/src/render.rs b/clap_mangen/src/render.rs index ced17bcdeea..1515450ed1f 100644 --- a/clap_mangen/src/render.rs +++ b/clap_mangen/src/render.rs @@ -176,6 +176,7 @@ pub(crate) fn after_help(roff: &mut Roff, app: &clap::App) { } fn subcommand_markers(cmd: &clap::App) -> (&'static str, &'static str) { + #[allow(deprecated)] markers(cmd.is_subcommand_required_set() || cmd.is_set(AppSettings::SubcommandRequiredElseHelp)) } diff --git a/src/build/app_settings.rs b/src/build/app_settings.rs index db3735da9fc..d8e8e0b30c0 100644 --- a/src/build/app_settings.rs +++ b/src/build/app_settings.rs @@ -132,25 +132,12 @@ pub enum AppSettings { )] SubcommandRequired, - /// Display help if no [`subcommands`] are present at runtime and exit gracefully (i.e. an - /// empty run such as `$ myprog`). - /// - /// **NOTE:** This should *not* be used with [`AppSettings::SubcommandRequired`] as they do - /// nearly same thing; this prints the help text, and the other prints an error. - /// - /// **NOTE:** If the user specifies arguments at runtime, but no subcommand the help text will - /// still be displayed and exit. If this is *not* the desired result, consider using - /// [`AppSettings::ArgRequiredElseHelp`] instead. - /// - /// # Examples - /// - /// ```rust - /// # use clap::{App, Arg, AppSettings}; - /// App::new("myprog") - /// .setting(AppSettings::SubcommandRequiredElseHelp); - /// ``` - /// - /// [`subcommands`]: crate::App::subcommand() + /// Deprecated, replaced with [`App::subcommand_required`] combined with + /// [`App::arg_required_else_help`]. + #[deprecated( + since = "3.1.0", + note = "Replaced with `App::subcommand_required` combined with `App::arg_required_else_help`" + )] SubcommandRequiredElseHelp, /// Deprecated, replaced with [`App::allow_external_subcommands`] and diff --git a/src/output/usage.rs b/src/output/usage.rs index f8e1650e7e9..2da807c305c 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -123,6 +123,7 @@ impl<'help, 'app> Usage<'help, 'app> { || self.app.is_allow_external_subcommands_set() { let placeholder = self.app.subcommand_value_name.unwrap_or("SUBCOMMAND"); + #[allow(deprecated)] if self.app.is_subcommand_negates_reqs_set() || self.app.is_args_conflicts_with_subcommands_set() { diff --git a/src/parse/validator.rs b/src/parse/validator.rs index aba2888b0f6..7035fa971ac 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -61,6 +61,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { return Err(Error::display_help_error(self.p.app, message)); } } + #[allow(deprecated)] if !has_subcmd && self.p.app.is_subcommand_required_set() { let bn = self.p.app.bin_name.as_ref().unwrap_or(&self.p.app.name); return Err(Error::missing_subcommand( diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs index 68056337278..d7252cf1749 100644 --- a/tests/builder/app_settings.rs +++ b/tests/builder/app_settings.rs @@ -260,6 +260,7 @@ fn arg_required_else_help_error_message() { #[test] fn subcommand_required_else_help() { + #![allow(deprecated)] let result = App::new("test") .setting(AppSettings::SubcommandRequiredElseHelp) .subcommand(App::new("info")) @@ -275,6 +276,7 @@ fn subcommand_required_else_help() { #[test] fn subcommand_required_else_help_error_message() { + #![allow(deprecated)] let app = App::new("test") .setting(AppSettings::SubcommandRequiredElseHelp) .version("1.0")