fix(derive): Treat default_value_os like default_value

The test went from panicing to not-panicing Fixes clap-rs#3031
epage · Dec 13, 2021 · 7c10b5a · 7c10b5a
1 parent 8eb5538
commit 7c10b5a
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -283,6 +283,7 @@ On top of the clap 2 changes
 - Support `SubcommandsNegateReqs` by allowing required `Option<_>`s ([clap-rs/clap#2255](https://github.com/clap-rs/clap/issues/2255))
 - Infer `AllowInvalidUtf8` based on parser ([clap-rs/clap#751](https://github.com/clap-rs/clap/issues/2255))
 - Gracefully handle empty `authors` field in `Cargo.toml`
+- Don't panic with `default_value_os` but treat it like `default_value` ([clap-rs/clap#3031](https://github.com/clap-rs/clap/issues/3031))
 
 On top of the clap 2 changes
 

diff --git a/clap_derive/src/attrs.rs b/clap_derive/src/attrs.rs
@@ -323,15 +323,15 @@ impl Attrs {
                         if res.is_enum {
                             abort!(field.ty, "`arg_enum` is meaningless for bool")
                         }
-                        if let Some(m) = res.find_method("default_value") {
+                        if let Some(m) = res.find_default_method() {
                             abort!(m.name, "default_value is meaningless for bool")
                         }
                         if let Some(m) = res.find_method("required") {
                             abort!(m.name, "required is meaningless for bool")
                         }
                     }
                     Ty::Option => {
-                        if let Some(m) = res.find_method("default_value") {
+                        if let Some(m) = res.find_default_method() {
                             abort!(m.name, "default_value is meaningless for Option")
                         }
                     }
@@ -557,14 +557,16 @@ impl Attrs {
         }
     }
 
-    pub fn has_method(&self, name: &str) -> bool {
-        self.find_method(name).is_some()
-    }
-
     pub fn find_method(&self, name: &str) -> Option<&Method> {
         self.methods.iter().find(|m| m.name == name)
     }
 
+    pub fn find_default_method(&self) -> Option<&Method> {
+        self.methods
+            .iter()
+            .find(|m| m.name == "default_value" || m.name == "default_value_os")
+    }
+
     /// generate methods from attributes on top of struct or enum
     pub fn initial_top_level_methods(&self) -> TokenStream {
         let help_heading = self.help_heading.as_ref().into_iter();

diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs
@@ -328,7 +328,7 @@ pub fn gen_augment(
                     },
 
                     Ty::Other => {
-                        let required = !attrs.has_method("default_value") && !override_required;
+                        let required = attrs.find_default_method().is_none() && !override_required;
                         quote_spanned! { ty.span()=>
                             .takes_value(true)
                             .value_name(#value_name)

diff --git a/tests/derive/default_value.rs b/tests/derive/default_value.rs
@@ -1,4 +1,4 @@
-use clap::Parser;
+use clap::{IntoApp, Parser};
 
 use crate::utils;
 
@@ -43,3 +43,15 @@ fn auto_default_value_t() {
     let help = utils::get_long_help::<Opt>();
     assert!(help.contains("[default: 0]"));
 }
+
+#[test]
+fn detect_os_variant() {
+    #![allow(unused_parens)] // needed for `as_ref` call
+
+    #[derive(clap::Parser)]
+    pub struct Options {
+        #[clap(default_value_os = ("123".as_ref()))]
+        x: String,
+    }
+    Options::into_app().debug_assert();
+}