fix(parser): Clean up remove types

The remove functions no longer return `Arc` but the core type, at the cost of requiring `Clone`. I originally held off on this in clap-rs#3732 in the hope of gracefully transition the derive and requiring `Clone` would have been a breaking change but when it came to clap-rs#3734, I didn't find a way to make it work without a breaking change, so I made it opt-in. This means I can force the `Clone` requirement now. I added the requirement for `Clone` everywhere else in the hopes that in the future, we can drop the `Arc` without a breaking change.
epage · May 24, 2022 · ed45de2 · ed45de2
1 parent bf86f76
commit ed45de2
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 23 deletions.
diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs
@@ -542,7 +542,7 @@ fn gen_parsers(
         _ if attrs.custom_value_parser() => (
             quote_spanned!(span=> remove_one::<#convert_type>),
             quote_spanned!(span=> remove_many::<#convert_type>),
-            quote!(|s| ::std::sync::Arc::try_unwrap(s).unwrap_or_else(|arc| (*arc).clone())),
+            quote!(|s| s),
             quote_spanned!(func.span()=> |s| ::std::result::Result::Ok::<_, clap::Error>(s)),
         ),
         FromStr => (

diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs
@@ -530,7 +530,6 @@ fn gen_from_arg_matches(
                             .remove_many::<#str_ty>("")
                             .expect("unexpected type")
                             .into_iter().flatten()  // `""` isn't present, bug in `unstable-v4`
-                            .map(|f| ::std::sync::Arc::try_unwrap(f).unwrap_or_else(|arc| (*arc).clone()))
                             .map(#str_ty::from)
                     )
                     .collect::<::std::vec::Vec<_>>()

diff --git a/examples/derive_ref/flatten_hand_args.rs b/examples/derive_ref/flatten_hand_args.rs
@@ -19,8 +19,7 @@ impl FromArgMatches for CliArgs {
             bar: matches.is_present("bar"),
             quuz: matches
                 .remove_one::<String>("quuz")
-                .expect("matches definition")
-                .map(|quuz| std::sync::Arc::try_unwrap(quuz).unwrap_or_else(|arc| (*arc).clone())),
+                .expect("matches definition"),
         })
     }
     fn update_from_arg_matches(&mut self, matches: &ArgMatches) -> Result<(), Error> {
@@ -34,7 +33,7 @@ impl FromArgMatches for CliArgs {
             .remove_one::<String>("quuz")
             .expect("matches definition")
         {
-            self.quuz = Some(std::sync::Arc::try_unwrap(quuz).unwrap_or_else(|arc| (*arc).clone()));
+            self.quuz = Some(quuz);
         }
         Ok(())
     }

diff --git a/src/builder/value_parser.rs b/src/builder/value_parser.rs
@@ -531,7 +531,7 @@ pub trait AnyValueParser: private::AnyValueParserSealed {
 
 impl<T, P> AnyValueParser for P
 where
-    T: std::any::Any + Send + Sync + 'static,
+    T: std::any::Any + Clone + Send + Sync + 'static,
     P: TypedValueParser<Value = T>,
 {
     fn parse_ref(
@@ -1664,7 +1664,7 @@ pub mod via_prelude {
     }
     impl<FromStr> ValueParserViaFromStr for AutoValueParser<FromStr>
     where
-        FromStr: std::str::FromStr + std::any::Any + Send + Sync + 'static,
+        FromStr: std::str::FromStr + std::any::Any + Clone + Send + Sync + 'static,
         <FromStr as std::str::FromStr>::Err:
             Into<Box<dyn std::error::Error + Send + Sync + 'static>>,
     {

diff --git a/src/parser/matches/any_value.rs b/src/parser/matches/any_value.rs
@@ -7,21 +7,24 @@ pub struct AnyValue {
 }
 
 impl AnyValue {
-    pub(crate) fn new<V: std::any::Any + Send + Sync + 'static>(inner: V) -> Self {
+    pub(crate) fn new<V: std::any::Any + Clone + Send + Sync + 'static>(inner: V) -> Self {
         let id = AnyValueId::of::<V>();
         let inner = std::sync::Arc::new(inner);
         Self { inner, id }
     }
 
-    pub(crate) fn downcast_ref<T: std::any::Any>(&self) -> Option<&T> {
+    pub(crate) fn downcast_ref<T: std::any::Any + Clone + Send + Sync + 'static>(
+        &self,
+    ) -> Option<&T> {
         self.inner.downcast_ref::<T>()
     }
 
-    pub(crate) fn downcast_into<T: std::any::Any + Send + Sync>(
-        self,
-    ) -> Result<std::sync::Arc<T>, Self> {
+    pub(crate) fn downcast_into<T: std::any::Any + Clone + Send + Sync>(self) -> Result<T, Self> {
         let id = self.id;
-        std::sync::Arc::downcast::<T>(self.inner).map_err(|inner| Self { inner, id })
+        let value =
+            std::sync::Arc::downcast::<T>(self.inner).map_err(|inner| Self { inner, id })?;
+        let value = std::sync::Arc::try_unwrap(value).unwrap_or_else(|arc| (*arc).clone());
+        Ok(value)
     }
 
     pub(crate) fn type_id(&self) -> AnyValueId {

diff --git a/src/parser/matches/arg_matches.rs b/src/parser/matches/arg_matches.rs
@@ -115,7 +115,7 @@ impl ArgMatches {
     /// [`ArgMatches::values_of`]: ArgMatches::values_of()
     /// [`default_value`]: crate::Arg::default_value()
     /// [`occurrences_of`]: crate::ArgMatches::occurrences_of()
-    pub fn get_one<T: Any + Send + Sync + 'static>(
+    pub fn get_one<T: Any + Clone + Send + Sync + 'static>(
         &self,
         name: &str,
     ) -> Result<Option<&T>, MatchesError> {
@@ -162,7 +162,7 @@ impl ArgMatches {
     ///     .collect();
     /// assert_eq!(vals, [22, 80, 2020]);
     /// ```
-    pub fn get_many<T: Any + Send + Sync + 'static>(
+    pub fn get_many<T: Any + Clone + Send + Sync + 'static>(
         &self,
         name: &str,
     ) -> Result<Option<impl Iterator<Item = &T>>, MatchesError> {
@@ -248,7 +248,6 @@ impl ArgMatches {
     ///     ]);
     /// let vals: String = m.remove_one("file")
     ///     .expect("`file` is a `String`")
-    ///     .map(|f| std::sync::Arc::try_unwrap(f).expect("no clones made"))
     ///     .expect("`file`is required");
     /// assert_eq!(vals, "file.txt");
     /// ```
@@ -257,10 +256,10 @@ impl ArgMatches {
     /// [`ArgMatches::values_of`]: ArgMatches::values_of()
     /// [`default_value`]: crate::Arg::default_value()
     /// [`occurrences_of`]: crate::ArgMatches::occurrences_of()
-    pub fn remove_one<T: Any + Send + Sync + 'static>(
+    pub fn remove_one<T: Any + Clone + Send + Sync + 'static>(
         &mut self,
         name: &str,
-    ) -> Result<Option<std::sync::Arc<T>>, MatchesError> {
+    ) -> Result<Option<T>, MatchesError> {
         let id = Id::from(name);
         match self.try_remove_arg_t::<T>(&id)? {
             Some(values) => Ok(values
@@ -295,14 +294,13 @@ impl ArgMatches {
     /// let vals: Vec<String> = m.remove_many("file")
     ///     .expect("`file` is a `String`")
     ///     .expect("`file`is required")
-    ///     .map(|f| std::sync::Arc::try_unwrap(f).expect("no clones made"))
     ///     .collect();
     /// assert_eq!(vals, ["file1.txt", "file2.txt", "file3.txt", "file4.txt"]);
     /// ```
-    pub fn remove_many<T: Any + Send + Sync + 'static>(
+    pub fn remove_many<T: Any + Clone + Send + Sync + 'static>(
         &mut self,
         name: &str,
-    ) -> Result<Option<impl Iterator<Item = std::sync::Arc<T>>>, MatchesError> {
+    ) -> Result<Option<impl Iterator<Item = T>>, MatchesError> {
         let id = Id::from(name);
         match self.try_remove_arg_t::<T>(&id)? {
             Some(values) => Ok(Some(
@@ -1286,8 +1284,7 @@ impl ArgMatches {
     ///          let ext_args: Vec<String> = sub_m.remove_many("")
     ///             .expect("`file` is a `String`")
     ///             .expect("`file`is required")
-    ///             .map(|f| std::sync::Arc::try_unwrap(f).expect("no clones made"))
-    ///     .collect();
+    ///             .collect();
     ///          assert_eq!(external, "subcmd");
     ///          assert_eq!(ext_args, ["--option", "value", "-fff", "--flag"]);
     ///     },