Merge pull request #3783 from epage/refactor

refactor(derive): Prepare for action support

clap_derive/src/attrs.rs

@@ -64,10 +64,10 @@ impl Attrs {
         res.push_attrs(attrs);
         res.push_doc_comment(attrs, "about");
 
-        if let Some(parser) = res.value_parser.as_ref() {
+        if let Some(value_parser) = res.value_parser.as_ref() {
             abort!(
-                parser.span(),
-                "`value_parse` attribute is only allowed on fields"
+                value_parser.span(),
+                "`value_parser` attribute is only allowed on fields"
             );
         }
         if let Some(parser) = res.parser.as_ref() {
@@ -104,10 +104,10 @@ impl Attrs {
 
         match &*res.kind {
             Kind::Flatten => {
-                if let Some(parser) = res.value_parser.as_ref() {
+                if let Some(value_parser) = res.value_parser.as_ref() {
                     abort!(
-                        parser.span(),
-                        "`value_parse` attribute is only allowed flattened entry"
+                        value_parser.span(),
+                        "`value_parser` attribute is not allowed for flattened entry"
                     );
                 }
                 if let Some(parser) = res.parser.as_ref() {
@@ -130,10 +130,10 @@ impl Attrs {
             Kind::ExternalSubcommand => (),
 
             Kind::Subcommand(_) => {
-                if let Some(parser) = res.value_parser.as_ref() {
+                if let Some(value_parser) = res.value_parser.as_ref() {
                     abort!(
-                        parser.span(),
-                        "`value_parse` attribute is only allowed for subcommand"
+                        value_parser.span(),
+                        "`value_parser` attribute is not allowed for subcommand"
                     );
                 }
                 if let Some(parser) = res.parser.as_ref() {
@@ -204,10 +204,10 @@ impl Attrs {
         res.push_attrs(&variant.attrs);
         res.push_doc_comment(&variant.attrs, "help");
 
-        if let Some(parser) = res.value_parser.as_ref() {
+        if let Some(value_parser) = res.value_parser.as_ref() {
             abort!(
-                parser.span(),
-                "`value_parse` attribute is only allowed on fields"
+                value_parser.span(),
+                "`value_parser` attribute is only allowed on fields"
             );
         }
         if let Some(parser) = res.parser.as_ref() {
@@ -244,10 +244,10 @@ impl Attrs {
 
         match &*res.kind {
             Kind::Flatten => {
-                if let Some(parser) = res.value_parser.as_ref() {
+                if let Some(value_parser) = res.value_parser.as_ref() {
                     abort!(
-                        parser.span(),
-                        "`value_parse` attribute is not allowed for flattened entry"
+                        value_parser.span(),
+                        "`value_parser` attribute is not allowed for flattened entry"
                     );
                 }
                 if let Some(parser) = res.parser.as_ref() {
@@ -274,10 +274,10 @@ impl Attrs {
             }
 
             Kind::Subcommand(_) => {
-                if let Some(parser) = res.value_parser.as_ref() {
+                if let Some(value_parser) = res.value_parser.as_ref() {
                     abort!(
-                        parser.span(),
-                        "`value_parse` attribute is not allowed for subcommand"
+                        value_parser.span(),
+                        "`value_parser` attribute is not allowed for subcommand"
                     );
                 }
                 if let Some(parser) = res.parser.as_ref() {
@@ -330,7 +330,7 @@ impl Attrs {
                     if let Some(value_parser) = res.value_parser.as_ref() {
                         abort!(
                             value_parser.span(),
-                            "`value_parse` attribute conflicts with `parse` attribute"
+                            "`value_parser` attribute conflicts with `parse` attribute"
                         );
                     }
                     match *ty {
@@ -721,7 +721,7 @@ impl Attrs {
             .unwrap_or_else(|| self.parser(field_type).value_parser())
     }
 
-    pub fn custom_value_parser(&self) -> bool {
+    pub fn ignore_parser(&self) -> bool {
         self.value_parser.is_some()
     }
 
@@ -958,7 +958,7 @@ impl Parser {
             ),
             ParserKind::FromFlag => Method::new(
                 func,
-                quote_spanned! { self.kind.span()=> clap::ValueParser::bool()},
+                quote_spanned! { self.kind.span()=> clap::builder::ValueParser::bool()},
             ),
         }
     }

clap_derive/src/derives/args.rs

@@ -244,14 +244,14 @@ pub fn gen_augment(
                 let convert_type = inner_type(&field.ty);
 
                 let parser = attrs.parser(&field.ty);
-                let occurrences = *parser.kind == ParserKind::FromOccurrences;
-                let flag = *parser.kind == ParserKind::FromFlag;
 
                 let value_parser = attrs.value_parser(&field.ty);
                 let func = &parser.func;
 
+                let mut occurrences = false;
+                let mut flag = false;
                 let validator = match *parser.kind {
-                    _ if attrs.custom_value_parser() || attrs.is_enum() => quote!(),
+                    _ if attrs.ignore_parser() || attrs.is_enum() => quote!(),
                     ParserKind::TryFromStr => quote_spanned! { func.span()=>
                         .validator(|s| {
                             #func(s)
@@ -261,14 +261,19 @@ pub fn gen_augment(
                     ParserKind::TryFromOsStr => quote_spanned! { func.span()=>
                         .validator_os(|s| #func(s).map(|_: #convert_type| ()))
                     },
-                    ParserKind::FromStr
-                    | ParserKind::FromOsStr
-                    | ParserKind::FromFlag
-                    | ParserKind::FromOccurrences => quote!(),
+                    ParserKind::FromStr | ParserKind::FromOsStr => quote!(),
+                    ParserKind::FromFlag => {
+                        flag = true;
+                        quote!()
+                    }
+                    ParserKind::FromOccurrences => {
+                        occurrences = true;
+                        quote!()
+                    }
                 };
 
                 let value_name = attrs.value_name();
-                let possible_values = if attrs.is_enum() && !attrs.custom_value_parser() {
+                let possible_values = if attrs.is_enum() && !attrs.ignore_parser() {
                     gen_arg_enum_possible_values(convert_type)
                 } else {
                     quote!()
@@ -524,25 +529,33 @@ fn gen_parsers(
     let span = parser.kind.span();
     let convert_type = inner_type(&field.ty);
     let id = attrs.id();
+    let mut flag = false;
+    let mut occurrences = false;
     let (get_one, get_many, deref, mut parse) = match *parser.kind {
-        FromOccurrences => (
-            quote_spanned!(span=> occurrences_of),
-            quote!(),
-            quote!(|s| ::std::ops::Deref::deref(s)),
-            func.clone(),
-        ),
-        FromFlag => (
-            quote!(),
-            quote!(),
-            quote!(|s| ::std::ops::Deref::deref(s)),
-            func.clone(),
-        ),
-        _ if attrs.custom_value_parser() => (
+        _ if attrs.ignore_parser() => (
             quote_spanned!(span=> remove_one::<#convert_type>),
             quote_spanned!(span=> remove_many::<#convert_type>),
             quote!(|s| s),
             quote_spanned!(func.span()=> |s| ::std::result::Result::Ok::<_, clap::Error>(s)),
         ),
+        FromOccurrences => {
+            occurrences = true;
+            (
+                quote_spanned!(span=> occurrences_of),
+                quote!(),
+                quote!(|s| ::std::ops::Deref::deref(s)),
+                func.clone(),
+            )
+        }
+        FromFlag => {
+            flag = true;
+            (
+                quote!(),
+                quote!(),
+                quote!(|s| ::std::ops::Deref::deref(s)),
+                func.clone(),
+            )
+        }
         FromStr => (
             quote_spanned!(span=> get_one::<String>),
             quote_spanned!(span=> get_many::<String>),
@@ -568,16 +581,14 @@ fn gen_parsers(
             quote_spanned!(func.span()=> |s| #func(s).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #id, err)))),
         ),
     };
-    if attrs.is_enum() && !attrs.custom_value_parser() {
+    if attrs.is_enum() && !attrs.ignore_parser() {
         let ci = attrs.ignore_case();
 
         parse = quote_spanned! { convert_type.span()=>
             |s| <#convert_type as clap::ArgEnum>::from_str(s, #ci).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #id, err)))
         }
     }
 
-    let flag = *parser.kind == ParserKind::FromFlag;
-    let occurrences = *parser.kind == ParserKind::FromOccurrences;
     // Give this identifier the same hygiene
     // as the `arg_matches` parameter definition. This
     // allows us to refer to `arg_matches` within a `quote_spanned` block

examples/tutorial_derive/01_quick.rs

@@ -26,7 +26,7 @@ enum Commands {
     /// does testing things
     Test {
         /// lists test values
-        #[clap(short, long, value_parser)]
+        #[clap(short, long)]
         list: bool,
     },
 }

examples/tutorial_derive/03_01_flag_bool.rs

@@ -3,7 +3,7 @@ use clap::Parser;
 #[derive(Parser)]
 #[clap(author, version, about, long_about = None)]
 struct Cli {
-    #[clap(short, long, value_parser)]
+    #[clap(short, long)]
     verbose: bool,
 }
 

src/parser/error.rs

@@ -18,11 +18,6 @@ pub enum MatchesError {
     UnknownArgument {
         // Missing `id` but blocked on a public id type which will hopefully come with `unstable-v4`
     },
-    /// Present argument must have one value
-    #[non_exhaustive]
-    ExpectedOne {
-        // Missing `id` but blocked on a public id type which will hopefully come with `unstable-v4`
-    },
 }
 
 impl MatchesError {
@@ -56,9 +51,6 @@ impl std::fmt::Display for MatchesError {
             Self::UnknownArgument {} => {
                 writeln!(f, "Unknown argument or group id.  Make sure you are using the argument id and not the short or long flags")
             }
-            Self::ExpectedOne {} => {
-                writeln!(f, "Present argument must have one value.  Make sure you are using the correct lookup (`get_one` vs `get_many`)")
-            }
         }
     }
 }

src/parser/matches/arg_matches.rs

@@ -1063,11 +1063,8 @@ impl ArgMatches {
     ) -> Result<Option<&T>, MatchesError> {
         let id = Id::from(name);
         let arg = self.try_get_arg_t::<T>(&id)?;
-        let value = match arg.map(|a| a.first()) {
-            Some(Some(value)) => value,
-            Some(None) => {
-                return Err(MatchesError::ExpectedOne {});
-            }
+        let value = match arg.and_then(|a| a.first()) {
+            Some(value) => value,
             None => {
                 return Ok(None);
             }

tests/builder/env.rs

@@ -38,10 +38,7 @@ fn env_bool_literal() {
     let m = r.unwrap();
     assert!(m.is_present("present"));
     assert_eq!(m.occurrences_of("present"), 0);
-    assert!(matches!(
-        m.try_get_one::<String>("present").unwrap_err(),
-        clap::parser::MatchesError::ExpectedOne { .. }
-    ));
+    assert_eq!(m.get_one::<String>("present").map(|v| v.as_str()), None);
     assert!(!m.is_present("negated"));
     assert!(!m.is_present("absent"));
 }

tests/builder/groups.rs

@@ -111,10 +111,7 @@ fn group_single_flag() {
 
     let m = res.unwrap();
     assert!(m.is_present("grp"));
-    assert!(matches!(
-        m.try_get_one::<String>("grp").unwrap_err(),
-        clap::parser::MatchesError::ExpectedOne { .. }
-    ));
+    assert!(m.get_one::<String>("grp").map(|v| v.as_str()).is_none());
 }
 
 #[test]

tests/builder/ignore_errors.rs

@@ -49,10 +49,7 @@ fn multiple_args_and_final_arg_without_value() {
         Some("file")
     );
     assert!(m.is_present("f"));
-    assert!(matches!(
-        m.try_get_one::<String>("stuff").unwrap_err(),
-        clap::parser::MatchesError::ExpectedOne { .. }
-    ));
+    assert_eq!(m.get_one::<String>("stuff").map(|v| v.as_str()), None);
 }
 
 #[test]
@@ -79,10 +76,7 @@ fn multiple_args_and_intermittent_arg_without_value() {
         Some("file")
     );
     assert!(m.is_present("f"));
-    assert!(matches!(
-        m.try_get_one::<String>("stuff").unwrap_err(),
-        clap::parser::MatchesError::ExpectedOne { .. }
-    ));
+    assert_eq!(m.get_one::<String>("stuff").map(|v| v.as_str()), None);
 }
 
 #[test]
@@ -124,10 +118,7 @@ fn subcommand() {
         sub_m.is_present("test"),
         "expected subcommand to be present due to partial parsing"
     );
-    assert!(matches!(
-        sub_m.try_get_one::<String>("test").unwrap_err(),
-        clap::parser::MatchesError::ExpectedOne { .. }
-    ));
+    assert_eq!(sub_m.get_one::<String>("test").map(|v| v.as_str()), None);
     assert_eq!(
         sub_m.get_one::<String>("stuff").map(|v| v.as_str()),
         Some("some other val")