Merge pull request #4222 from epage/size

fix: Misc fixes found when optimizing code size

CHANGELOG.md

@@ -229,6 +229,7 @@ Easier to catch changes:
   - `#[clap(value_parser)]` and `#[clap(action)]` are now redundant
 - *(derive)* `subcommand_required(true).arg_required_else_help(true)` is set instead of `SubcommandRequiredElseHelp` to give more meaningful errors when subcommands are missing and to reduce redundancy (#3280)
 - *(derive)* Remove `arg_enum` attribute in favor of `value_enum` to match the new name (we didn't have support in v3 to mark it deprecated) (#4127)
+- *(derive)* `next_help_heading` can now leak out of a `#[clap(flatten)]`, like all other command settings
 - *(parser)* Assert when the CLI looksup an unknown args when external subcommand support is enabled to help catch bugs (#3703)
 - *(assert)* Sometimes `Arg::default_missing_value` didn't require `num_args(0..=N)`, now it does (#4023)
 - *(assert)* Leading dashes in `Arg::long` are no longer allowed (#3691)
@@ -238,6 +239,7 @@ Easier to catch changes:
 - *(assert)* Ensure `overrides_with` IDs are valid
 - *(assert)* Ensure no self-`overrides_with` now that Actions replace it
 - *(assert)* Ensure subcommand names are not duplicated
+- *(assert)* Assert on `mut_arg` receiving an invalid arg ID or `mut_subcommand` receiving an invalid command name
 
 ### Compatibility
 

clap_derive/src/derives/args.rs

@@ -194,26 +194,21 @@ pub fn gen_augment(
             }
             Kind::Flatten => {
                 let ty = &field.ty;
-                let old_heading_var = format_ident!("__clap_old_heading");
                 let next_help_heading = item.next_help_heading();
                 let next_display_order = item.next_display_order();
                 if override_required {
                     Some(quote_spanned! { kind.span()=>
-                        let #old_heading_var = #app_var.get_next_help_heading().map(|s| clap::builder::Str::from(s.to_owned()));
                         let #app_var = #app_var
                             #next_help_heading
                             #next_display_order;
                         let #app_var = <#ty as clap::Args>::augment_args_for_update(#app_var);
-                        let #app_var = #app_var.next_help_heading(clap::builder::Resettable::from(#old_heading_var));
                     })
                 } else {
                     Some(quote_spanned! { kind.span()=>
-                        let #old_heading_var = #app_var.get_next_help_heading().map(|s| clap::builder::Str::from(s.to_owned()));
                         let #app_var = #app_var
                             #next_help_heading
                             #next_display_order;
                         let #app_var = <#ty as clap::Args>::augment_args(#app_var);
-                        let #app_var = #app_var.next_help_heading(clap::builder::Resettable::from(#old_heading_var));
                     })
                 }
             }

clap_derive/src/derives/parser.rs

@@ -29,6 +29,7 @@ use crate::item::Name;
 
 pub fn derive_parser(input: &DeriveInput) -> TokenStream {
     let ident = &input.ident;
+    let pkg_name = std::env::var("CARGO_PKG_NAME").ok().unwrap_or_default();
 
     match input.data {
         Data::Struct(DataStruct {
@@ -37,9 +38,7 @@ pub fn derive_parser(input: &DeriveInput) -> TokenStream {
         }) => {
             dummies::parser_struct(ident);
 
-            let name = Name::Assigned(quote!(std::env::var("CARGO_PKG_NAME")
-                .ok()
-                .unwrap_or_default()));
+            let name = Name::Assigned(quote!(#pkg_name));
             let item = Item::from_args_struct(input, name);
             let fields = fields
                 .named
@@ -57,9 +56,7 @@ pub fn derive_parser(input: &DeriveInput) -> TokenStream {
         }) => {
             dummies::parser_struct(ident);
 
-            let name = Name::Assigned(quote!(std::env::var("CARGO_PKG_NAME")
-                .ok()
-                .unwrap_or_default()));
+            let name = Name::Assigned(quote!(#pkg_name));
             let item = Item::from_args_struct(input, name);
             let fields = Punctuated::<Field, Comma>::new();
             let fields = fields
@@ -74,9 +71,7 @@ pub fn derive_parser(input: &DeriveInput) -> TokenStream {
         Data::Enum(ref e) => {
             dummies::parser_enum(ident);
 
-            let name = Name::Assigned(quote!(std::env::var("CARGO_PKG_NAME")
-                .ok()
-                .unwrap_or_default()));
+            let name = Name::Assigned(quote!(#pkg_name));
             let item = Item::from_subcommand_enum(input, name);
             let variants = e
                 .variants

clap_derive/src/derives/subcommand.rs

@@ -181,28 +181,23 @@ fn gen_augment(
                         } else {
                             quote!()
                         };
-                        let old_heading_var = format_ident!("__clap_old_heading");
                         let next_help_heading = item.next_help_heading();
                         let next_display_order = item.next_display_order();
                         let subcommand = if override_required {
                             quote! {
                                 #deprecations
-                                let #old_heading_var = #app_var.get_next_help_heading().map(|s| clap::builder::Str::from(s.to_owned()));
                                 let #app_var = #app_var
                                     #next_help_heading
                                     #next_display_order;
                                 let #app_var = <#ty as clap::Subcommand>::augment_subcommands_for_update(#app_var);
-                                let #app_var = #app_var.next_help_heading(clap::builder::Resettable::from(#old_heading_var));
                             }
                         } else {
                             quote! {
                                 #deprecations
-                                let #old_heading_var = #app_var.get_next_help_heading().map(|s| clap::builder::Str::from(s.to_owned()));
                                 let #app_var = #app_var
                                     #next_help_heading
                                     #next_display_order;
                                 let #app_var = <#ty as clap::Subcommand>::augment_subcommands(#app_var);
-                                let #app_var = #app_var.next_help_heading(clap::builder::Resettable::from(#old_heading_var));
                             }
                         };
                         Some(subcommand)

examples/tutorial_builder/03_05_default_values.md

@@ -2,19 +2,19 @@
 $ 03_05_default_values --help
 A simple to use, efficient, and full-featured Command Line Argument Parser
 
-Usage: 03_05_default_values[EXE] [NAME]
+Usage: 03_05_default_values[EXE] [PORT]
 
 Arguments:
-  [NAME]  [default: alice]
+  [PORT]  [default: 2020]
 
 Options:
   -h, --help     Print help information
   -V, --version  Print version information
 
 $ 03_05_default_values
-name: "alice"
+port: 2020
 
-$ 03_05_default_values bob
-name: "bob"
+$ 03_05_default_values 22
+port: 22
 
 ```

examples/tutorial_builder/03_05_default_values.rs

@@ -1,14 +1,18 @@
-use clap::{arg, command};
+use clap::{arg, command, value_parser};
 
 fn main() {
     let matches = command!() // requires `cargo` feature
-        .arg(arg!([NAME]).default_value("alice"))
+        .arg(
+            arg!([PORT])
+                .value_parser(value_parser!(u16))
+                .default_value("2020"),
+        )
         .get_matches();
 
     println!(
-        "name: {:?}",
+        "port: {:?}",
         matches
-            .get_one::<String>("NAME")
+            .get_one::<u16>("PORT")
             .expect("default ensures there is always a value")
     );
 }

examples/tutorial_derive/03_05_default_values.md

@@ -2,19 +2,19 @@
 $ 03_05_default_values_derive --help
 A simple to use, efficient, and full-featured Command Line Argument Parser
 
-Usage: 03_05_default_values_derive[EXE] [NAME]
+Usage: 03_05_default_values_derive[EXE] [PORT]
 
 Arguments:
-  [NAME]  [default: alice]
+  [PORT]  [default: 2020]
 
 Options:
   -h, --help     Print help information
   -V, --version  Print version information
 
 $ 03_05_default_values_derive
-name: "alice"
+port: 2020
 
-$ 03_05_default_values_derive bob
-name: "bob"
+$ 03_05_default_values_derive 22
+port: 22
 
 ```

examples/tutorial_derive/03_05_default_values.rs

@@ -3,12 +3,12 @@ use clap::Parser;
 #[derive(Parser)]
 #[command(author, version, about, long_about = None)]
 struct Cli {
-    #[arg(default_value_t = String::from("alice"))]
-    name: String,
+    #[arg(default_value_t = 2020)]
+    port: u16,
 }
 
 fn main() {
     let cli = Cli::parse();
 
-    println!("name: {:?}", cli.name);
+    println!("port: {:?}", cli.port);
 }

src/builder/arg.rs

@@ -146,7 +146,7 @@ impl Arg {
     #[must_use]
     pub fn short(mut self, s: impl IntoResettable<char>) -> Self {
         if let Some(s) = s.into_resettable().into_option() {
-            assert!(s != '-', "short option name cannot be `-`");
+            debug_assert!(s != '-', "short option name cannot be `-`");
             self.short = Some(s);
         } else {
             self.short = None;
@@ -241,7 +241,7 @@ impl Arg {
     #[must_use]
     pub fn short_alias(mut self, name: impl IntoResettable<char>) -> Self {
         if let Some(name) = name.into_resettable().into_option() {
-            assert!(name != '-', "short alias name cannot be `-`");
+            debug_assert!(name != '-', "short alias name cannot be `-`");
             self.short_aliases.push((name, false));
         } else {
             self.short_aliases.clear();
@@ -301,7 +301,7 @@ impl Arg {
     #[must_use]
     pub fn short_aliases(mut self, names: impl IntoIterator<Item = char>) -> Self {
         for s in names {
-            assert!(s != '-', "short alias name cannot be `-`");
+            debug_assert!(s != '-', "short alias name cannot be `-`");
             self.short_aliases.push((s, false));
         }
         self
@@ -357,7 +357,7 @@ impl Arg {
     #[must_use]
     pub fn visible_short_alias(mut self, name: impl IntoResettable<char>) -> Self {
         if let Some(name) = name.into_resettable().into_option() {
-            assert!(name != '-', "short alias name cannot be `-`");
+            debug_assert!(name != '-', "short alias name cannot be `-`");
             self.short_aliases.push((name, true));
         } else {
             self.short_aliases.clear();
@@ -412,7 +412,7 @@ impl Arg {
     #[must_use]
     pub fn visible_short_aliases(mut self, names: impl IntoIterator<Item = char>) -> Self {
         for n in names {
-            assert!(n != '-', "short alias name cannot be `-`");
+            debug_assert!(n != '-', "short alias name cannot be `-`");
             self.short_aliases.push((n, true));
         }
         self

src/builder/command.rs

@@ -195,10 +195,6 @@ impl Command {
     /// [arguments]: Arg
     #[must_use]
     pub fn args(mut self, args: impl IntoIterator<Item = impl Into<Arg>>) -> Self {
-        let args = args.into_iter();
-        let (lower, _) = args.size_hint();
-        self.args.reserve(lower);
-
         for arg in args {
             self = self.arg(arg);
         }
@@ -209,6 +205,10 @@ impl Command {
     ///
     /// This can be useful for modifying the auto-generated help or version arguments.
     ///
+    /// # Panics
+    ///
+    /// If the argument is undefined
+    ///
     /// # Examples
     ///
     /// ```rust
@@ -231,15 +231,16 @@ impl Command {
     /// assert!(res.is_ok());
     /// ```
     #[must_use]
-    pub fn mut_arg<F>(mut self, arg_id: impl Into<Id>, f: F) -> Self
+    #[cfg_attr(debug_assertions, track_caller)]
+    pub fn mut_arg<F>(mut self, arg_id: impl AsRef<str>, f: F) -> Self
     where
         F: FnOnce(Arg) -> Arg,
     {
-        let id = arg_id.into();
+        let id = arg_id.as_ref();
         let a = self
             .args
-            .remove_by_name(id.as_str())
-            .unwrap_or_else(|| Arg::new(id));
+            .remove_by_name(id)
+            .unwrap_or_else(|| panic!("Argument `{}` is undefined", id));
 
         self.args.push(f(a));
         self
@@ -250,6 +251,10 @@ impl Command {
     /// This can be useful for modifying auto-generated arguments of nested subcommands with
     /// [`Command::mut_arg`].
     ///
+    /// # Panics
+    ///
+    /// If the subcommand is undefined
+    ///
     /// # Examples
     ///
     /// ```rust
@@ -279,7 +284,7 @@ impl Command {
         let subcmd = if let Some(idx) = pos {
             self.subcommands.remove(idx)
         } else {
-            Self::new(Str::from(name.to_owned()))
+            panic!("Command `{}` is undefined", name)
         };
 
         self.subcommands.push(f(subcmd));
@@ -407,10 +412,6 @@ impl Command {
     /// [`IntoIterator`]: std::iter::IntoIterator
     #[must_use]
     pub fn subcommands(mut self, subcmds: impl IntoIterator<Item = impl Into<Self>>) -> Self {
-        let subcmds = subcmds.into_iter();
-        let (lower, _) = subcmds.size_hint();
-        self.subcommands.reserve(lower);
-
         for subcmd in subcmds {
             self = self.subcommand(subcmd);
         }
@@ -2219,7 +2220,7 @@ impl Command {
     #[must_use]
     pub fn short_flag_alias(mut self, name: impl IntoResettable<char>) -> Self {
         if let Some(name) = name.into_resettable().into_option() {
-            assert!(name != '-', "short alias name cannot be `-`");
+            debug_assert!(name != '-', "short alias name cannot be `-`");
             self.short_flag_aliases.push((name, false));
         } else {
             self.short_flag_aliases.clear();
@@ -2310,7 +2311,7 @@ impl Command {
     #[must_use]
     pub fn short_flag_aliases(mut self, names: impl IntoIterator<Item = char>) -> Self {
         for s in names {
-            assert!(s != '-', "short alias name cannot be `-`");
+            debug_assert!(s != '-', "short alias name cannot be `-`");
             self.short_flag_aliases.push((s, false));
         }
         self
@@ -2402,7 +2403,7 @@ impl Command {
     #[must_use]
     pub fn visible_short_flag_alias(mut self, name: impl IntoResettable<char>) -> Self {
         if let Some(name) = name.into_resettable().into_option() {
-            assert!(name != '-', "short alias name cannot be `-`");
+            debug_assert!(name != '-', "short alias name cannot be `-`");
             self.short_flag_aliases.push((name, true));
         } else {
             self.short_flag_aliases.clear();
@@ -2491,7 +2492,7 @@ impl Command {
     #[must_use]
     pub fn visible_short_flag_aliases(mut self, names: impl IntoIterator<Item = char>) -> Self {
         for s in names {
-            assert!(s != '-', "short alias name cannot be `-`");
+            debug_assert!(s != '-', "short alias name cannot be `-`");
             self.short_flag_aliases.push((s, true));
         }
         self
@@ -4041,7 +4042,7 @@ impl Command {
                 .map(|arg| arg.get_id().clone())
                 .collect();
 
-            assert!(args_missing_help.is_empty(),
+            debug_assert!(args_missing_help.is_empty(),
                     "Command::help_expected is enabled for the Command {}, but at least one of its arguments does not have either `help` or `long_help` set. List of such arguments: {}",
                     self.name,
                     args_missing_help.join(", ")
@@ -4216,7 +4217,7 @@ impl Command {
     }
 
     fn _copy_subtree_for_help(&self) -> Command {
-        let mut cmd = Command::new(self.get_name().to_string())
+        let mut cmd = Command::new(self.name.clone())
             .hide(self.is_hide_set())
             .global_setting(AppSettings::DisableHelpFlag)
             .global_setting(AppSettings::DisableVersionFlag)
@@ -4299,7 +4300,7 @@ impl Command {
 
     #[inline]
     pub(crate) fn contains_short(&self, s: char) -> bool {
-        assert!(
+        debug_assert!(
             self.is_set(AppSettings::Built),
             "If Command::_build hasn't been called, manually search through Arg shorts"
         );

src/mkeymap.rs

@@ -90,11 +90,6 @@ impl MKeyMap {
         self.keys.iter().any(|x| x.key == key)
     }
 
-    /// Reserves capacity for at least additional more elements to be inserted
-    pub(crate) fn reserve(&mut self, additional: usize) {
-        self.args.reserve(additional);
-    }
-
     /// Push an argument in the map.
     pub(crate) fn push(&mut self, new_arg: Arg) {
         self.args.push(new_arg);

src/parser/arg_matcher.rs

@@ -32,7 +32,7 @@ impl ArgMatcher {
                 #[cfg(debug_assertions)]
                 valid_subcommands: _cmd
                     .get_subcommands()
-                    .map(|sc| sc.get_name().to_owned())
+                    .map(|sc| sc.get_name_str().clone())
                     .collect(),
                 ..Default::default()
             },

src/parser/matches/arg_matches.rs

@@ -66,7 +66,7 @@ pub struct ArgMatches {
     #[cfg(debug_assertions)]
     pub(crate) valid_args: Vec<Id>,
     #[cfg(debug_assertions)]
-    pub(crate) valid_subcommands: Vec<String>,
+    pub(crate) valid_subcommands: Vec<Str>,
     pub(crate) args: FlatMap<Id, MatchedArg>,
     pub(crate) subcommand: Option<Box<SubCommand>>,
 }
@@ -924,8 +924,7 @@ impl ArgMatches {
     pub fn is_valid_subcommand(&self, _name: &str) -> bool {
         #[cfg(debug_assertions)]
         {
-            let _name = _name.to_owned();
-            _name == String::default() || self.valid_subcommands.contains(&_name)
+            _name.is_empty() || self.valid_subcommands.iter().any(|s| *s == _name)
         }
         #[cfg(not(debug_assertions))]
         {
@@ -1060,8 +1059,8 @@ impl ArgMatches {
         arg: &str,
     ) -> Result<Option<MatchedArg>, MatchesError> {
         self.verify_arg(arg)?;
-        let matched = match self.args.remove(arg) {
-            Some(matched) => matched,
+        let (id, matched) = match self.args.remove_entry(arg) {
+            Some((id, matched)) => (id, matched),
             None => {
                 return Ok(None);
             }
@@ -1072,7 +1071,7 @@ impl ArgMatches {
         if actual == expected {
             Ok(Some(matched))
         } else {
-            self.args.insert(Id::from(arg.to_owned()), matched);
+            self.args.insert(id, matched);
             Err(MatchesError::Downcast { actual, expected })
         }
     }
@@ -1094,8 +1093,7 @@ impl ArgMatches {
     fn verify_arg(&self, _arg: &str) -> Result<(), MatchesError> {
         #[cfg(debug_assertions)]
         {
-            let _arg = Id::from(_arg.to_owned());
-            if _arg == Id::EXTERNAL || self.valid_args.contains(&_arg) {
+            if _arg == Id::EXTERNAL || self.valid_args.iter().any(|s| *s == _arg) {
             } else {
                 debug!(
                     "`{:?}` is not an id of an argument or a group.\n\
@@ -1114,8 +1112,7 @@ impl ArgMatches {
     fn get_arg<'s>(&'s self, arg: &str) -> Option<&'s MatchedArg> {
         #[cfg(debug_assertions)]
         {
-            let arg = Id::from(arg.to_owned());
-            if arg == Id::EXTERNAL || self.valid_args.contains(&arg) {
+            if arg == Id::EXTERNAL || self.valid_args.iter().any(|s| *s == arg) {
             } else {
                 panic!(
                     "`{:?}` is not an id of an argument or a group.\n\
@@ -1134,8 +1131,7 @@ impl ArgMatches {
     fn get_subcommand(&self, name: &str) -> Option<&SubCommand> {
         #[cfg(debug_assertions)]
         {
-            let name = name.to_owned();
-            if name == String::default() || self.valid_subcommands.contains(&name) {
+            if name.is_empty() || self.valid_subcommands.iter().any(|s| *s == name) {
             } else {
                 panic!("`{}` is not a name of a subcommand.", name);
             }

src/util/flat_map.rs

@@ -53,6 +53,14 @@ impl<K: PartialEq + Eq, V> FlatMap<K, V> {
     }
 
     pub fn remove<Q: ?Sized>(&mut self, key: &Q) -> Option<V>
+    where
+        K: Borrow<Q>,
+        Q: std::hash::Hash + Eq,
+    {
+        self.remove_entry(key).map(|(_, v)| v)
+    }
+
+    pub fn remove_entry<Q: ?Sized>(&mut self, key: &Q) -> Option<(K, V)>
     where
         K: Borrow<Q>,
         Q: std::hash::Hash + Eq,
@@ -62,8 +70,9 @@ impl<K: PartialEq + Eq, V> FlatMap<K, V> {
             .iter()
             .enumerate()
             .find_map(|(i, k)| (k.borrow() == key).then(|| i))?;
-        self.keys.remove(index);
-        Some(self.values.remove(index))
+        let key = self.keys.remove(index);
+        let value = self.values.remove(index);
+        Some((key, value))
     }
 
     pub(crate) fn is_empty(&self) -> bool {

tests/builder/version.rs

@@ -291,13 +291,9 @@ fn version_required() {
 }
 
 #[test]
+#[should_panic = "Argument `version` is undefined"]
 fn mut_arg_version_no_auto_version() {
-    let res = common()
-        .mut_arg("version", |v| v.short('z').action(ArgAction::SetTrue))
-        .try_get_matches_from("foo -z".split(' '));
-
-    assert!(res.is_ok(), "{}", res.unwrap_err());
-    assert_eq!(res.unwrap().get_one::<bool>("version").copied(), Some(true));
+    let _ = common().mut_arg("version", |v| v.short('z').action(ArgAction::SetTrue));
 }
 
 #[cfg(debug_assertions)]

tests/derive/help.rs

@@ -131,11 +131,11 @@ fn app_help_heading_flattened() {
         .unwrap();
     assert_eq!(should_be_in_section_b.get_help_heading(), Some("HEADING B"));
 
-    let should_be_in_default_section = cmd
+    let should_be_in_section_b = cmd
         .get_arguments()
         .find(|a| a.get_id() == "should_be_in_default_section")
         .unwrap();
-    assert_eq!(should_be_in_default_section.get_help_heading(), None);
+    assert_eq!(should_be_in_section_b.get_help_heading(), Some("HEADING B"));
 
     let sub_a_two = cmd.find_subcommand("sub-a-two").unwrap();