Merge pull request clap-rs#3746 from epage/fixes

fix(parser): Increase compatibility between old/new approach

clap_derive/src/attrs.rs

@@ -805,7 +805,12 @@ impl ValueParser {
             Self::Explicit(method) => method,
             Self::Implicit(ident) => {
                 let func = Ident::new("value_parser", ident.span());
-                Method::new(func, quote!(clap::value_parser!(#inner_type)))
+                Method::new(
+                    func,
+                    quote_spanned! { ident.span()=>
+                        clap::value_parser!(#inner_type)
+                    },
+                )
             }
         }
     }
@@ -955,14 +960,23 @@ impl Parser {
     fn value_parser(&self) -> Method {
         let func = Ident::new("value_parser", self.kind.span());
         match *self.kind {
-            ParserKind::FromStr | ParserKind::TryFromStr => {
-                Method::new(func, quote!(clap::builder::ValueParser::string()))
-            }
-            ParserKind::FromOsStr | ParserKind::TryFromOsStr => {
-                Method::new(func, quote!(clap::builder::ValueParser::os_string()))
-            }
-            ParserKind::FromOccurrences => Method::new(func, quote!(clap::value_parser!(usize))),
-            ParserKind::FromFlag => Method::new(func, quote!(clap::ValueParser::bool())),
+            ParserKind::FromStr | ParserKind::TryFromStr => Method::new(
+                func,
+                quote_spanned! { self.kind.span()=>
+                clap::builder::ValueParser::string()},
+            ),
+            ParserKind::FromOsStr | ParserKind::TryFromOsStr => Method::new(
+                func,
+                quote_spanned! { self.kind.span()=> clap::builder::ValueParser::os_string()},
+            ),
+            ParserKind::FromOccurrences => Method::new(
+                func,
+                quote_spanned! { self.kind.span()=> clap::value_parser!(usize)},
+            ),
+            ParserKind::FromFlag => Method::new(
+                func,
+                quote_spanned! { self.kind.span()=> clap::ValueParser::bool()},
+            ),
         }
     }
 }

examples/cargo-example.md

@@ -37,9 +37,9 @@ OPTIONS:
 Then to directly invoke the command, run:
 ```console
 $ cargo-example example
-None
+Ok(None)
 
 $ cargo-example example --manifest-path Cargo.toml
-Some("Cargo.toml")
+Ok(Some("Cargo.toml"))
 
 ```

examples/cargo-example.rs

@@ -8,16 +8,14 @@ fn main() {
             clap::command!("example").arg(
                 clap::arg!(--"manifest-path" <PATH>)
                     .required(false)
-                    .allow_invalid_utf8(true),
+                    .value_parser(clap::value_parser!(std::path::PathBuf)),
             ),
         );
     let matches = cmd.get_matches();
     let matches = match matches.subcommand() {
         Some(("example", matches)) => matches,
         _ => unreachable!("clap should ensure we don't get here"),
     };
-    let manifest_path = matches
-        .value_of_os("manifest-path")
-        .map(std::path::PathBuf::from);
+    let manifest_path = matches.get_one::<std::path::PathBuf>("manifest-path");
     println!("{:?}", manifest_path);
 }

examples/derive_ref/README.md

@@ -274,16 +274,16 @@ Notes:
   - Implies `arg.takes_value(true).allow_invalid_utf8(true)`
 - `from_occurrences`:
   - Implies `arg.takes_value(false).multiple_occurrences(true)`
-  - Reads from `clap::ArgMatches::occurrences_of` rather than a `value_of` function
+  - Reads from `clap::ArgMatches::occurrences_of` rather than a `get_one` function
     - Note: operations on values, like `default_value`, are unlikely to do what you want
 - `from_flag`
   - Implies `arg.takes_value(false)`
-  - Reads from `clap::ArgMatches::is_present` rather than a `value_of` function
+  - Reads from `clap::ArgMatches::is_present` rather than a `get_one` function
     - Note: operations on values, like `default_value`, are unlikely to do what you want
 
 **Warning:**
-- To support non-UTF8 paths, you must use `parse(from_os_str)`, otherwise
-  `clap` will use `clap::ArgMatches::value_of` with `PathBuf::FromStr`.
+- To support non-UTF8 paths, you should use `#[clap(value_parser)]` otherwise
+  `clap` will parse it as a `String` which will fail on some paths.
 
 ## Doc Comments
 

examples/derive_ref/flatten_hand_args.rs

@@ -10,17 +10,31 @@ struct CliArgs {
 
 impl FromArgMatches for CliArgs {
     fn from_arg_matches(matches: &ArgMatches) -> Result<Self, Error> {
+        let mut matches = matches.clone();
+        Self::from_arg_matches_mut(&mut matches)
+    }
+    fn from_arg_matches_mut(matches: &mut ArgMatches) -> Result<Self, Error> {
         Ok(Self {
             foo: matches.is_present("foo"),
             bar: matches.is_present("bar"),
-            quuz: matches.value_of("quuz").map(|quuz| quuz.to_owned()),
+            quuz: matches
+                .remove_one::<String>("quuz")
+                .expect("matches definition")
+                .map(|quuz| std::sync::Arc::try_unwrap(quuz).unwrap_or_else(|arc| (*arc).clone())),
         })
     }
     fn update_from_arg_matches(&mut self, matches: &ArgMatches) -> Result<(), Error> {
+        let mut matches = matches.clone();
+        self.update_from_arg_matches_mut(&mut matches)
+    }
+    fn update_from_arg_matches_mut(&mut self, matches: &mut ArgMatches) -> Result<(), Error> {
         self.foo |= matches.is_present("foo");
         self.bar |= matches.is_present("bar");
-        if let Some(quuz) = matches.value_of("quuz") {
-            self.quuz = Some(quuz.to_owned());
+        if let Some(quuz) = matches
+            .remove_one::<String>("quuz")
+            .expect("matches definition")
+        {
+            self.quuz = Some(std::sync::Arc::try_unwrap(quuz).unwrap_or_else(|arc| (*arc).clone()));
         }
         Ok(())
     }

examples/escaped-positional.rs

@@ -1,26 +1,44 @@
 // Note: this requires the `cargo` feature
 
-use clap::{arg, command};
+use clap::{arg, command, value_parser};
 
 fn main() {
     let matches = command!()
         .arg(arg!(eff: -f))
-        .arg(arg!(pea: -p <PEAR>).required(false))
         .arg(
-            arg!(slop: [SLOP]).multiple_occurrences(true).last(true), // Indicates that `slop` is only accessible after `--`.
+            arg!(pea: -p <PEAR>)
+                .required(false)
+                .value_parser(value_parser!(String)),
+        )
+        .arg(
+            // Indicates that `slop` is only accessible after `--`.
+            arg!(slop: [SLOP])
+                .multiple_occurrences(true)
+                .last(true)
+                .value_parser(value_parser!(String)),
         )
         .get_matches();
 
     // This is what will happen with `myprog -f -p=bob -- sloppy slop slop`...
-    println!("-f used: {:?}", matches.is_present("eff")); // -f used: true
-    println!("-p's value: {:?}", matches.value_of("pea")); // -p's value: Some("bob")
+
+    // -f used: true
+    println!("-f used: {:?}", matches.is_present("eff"));
+    // -p's value: Some("bob")
+    println!(
+        "-p's value: {:?}",
+        matches
+            .get_one::<String>("pea")
+            .expect("matches definition")
+    );
+    // 'slops' values: Some(["sloppy", "slop", "slop"])
     println!(
         "'slops' values: {:?}",
         matches
-            .values_of("slop")
+            .get_many::<String>("slop")
+            .expect("matches definition")
             .map(|vals| vals.collect::<Vec<_>>())
             .unwrap_or_default()
-    ); // 'slops' values: Some(["sloppy", "slop", "slop"])
+    );
 
     // Continued program logic goes here...
 }

examples/git.rs

@@ -1,5 +1,6 @@
 // Note: this requires the `cargo` feature
 
+use std::ffi::OsString;
 use std::path::PathBuf;
 
 use clap::{arg, Command};
@@ -27,7 +28,7 @@ fn cli() -> Command<'static> {
             Command::new("add")
                 .about("adds things")
                 .arg_required_else_help(true)
-                .arg(arg!(<PATH> ... "Stuff to add").allow_invalid_utf8(true)),
+                .arg(arg!(<PATH> ... "Stuff to add").value_parser(clap::value_parser!(PathBuf))),
         )
         .subcommand(
             Command::new("stash")
@@ -50,36 +51,49 @@ fn main() {
         Some(("clone", sub_matches)) => {
             println!(
                 "Cloning {}",
-                sub_matches.value_of("REMOTE").expect("required")
+                sub_matches
+                    .get_one::<String>("REMOTE")
+                    .expect("matches definition")
+                    .expect("required")
             );
         }
         Some(("push", sub_matches)) => {
             println!(
                 "Pushing to {}",
-                sub_matches.value_of("REMOTE").expect("required")
+                sub_matches
+                    .get_one::<String>("REMOTE")
+                    .expect("matches definition")
+                    .expect("required")
             );
         }
         Some(("add", sub_matches)) => {
             let paths = sub_matches
-                .values_of_os("PATH")
-                .unwrap_or_default()
-                .map(PathBuf::from)
+                .get_many::<PathBuf>("PATH")
+                .expect("matches definition")
+                .into_iter()
+                .flatten()
                 .collect::<Vec<_>>();
             println!("Adding {:?}", paths);
         }
         Some(("stash", sub_matches)) => {
             let stash_command = sub_matches.subcommand().unwrap_or(("push", sub_matches));
             match stash_command {
                 ("apply", sub_matches) => {
-                    let stash = sub_matches.value_of("STASH");
+                    let stash = sub_matches
+                        .get_one::<String>("STASH")
+                        .expect("matches definition");
                     println!("Applying {:?}", stash);
                 }
                 ("pop", sub_matches) => {
-                    let stash = sub_matches.value_of("STASH");
+                    let stash = sub_matches
+                        .get_one::<String>("STASH")
+                        .expect("matches definition");
                     println!("Popping {:?}", stash);
                 }
                 ("push", sub_matches) => {
-                    let message = sub_matches.value_of("message");
+                    let message = sub_matches
+                        .get_one::<String>("message")
+                        .expect("matches definition");
                     println!("Pushing {:?}", message);
                 }
                 (name, _) => {
@@ -89,8 +103,10 @@ fn main() {
         }
         Some((ext, sub_matches)) => {
             let args = sub_matches
-                .values_of_os("")
-                .unwrap_or_default()
+                .get_many::<OsString>("")
+                .expect("matches definition")
+                .into_iter()
+                .flatten()
                 .collect::<Vec<_>>();
             println!("Calling out to {:?} with {:?}", ext, args);
         }

examples/multicall-busybox.rs

@@ -1,6 +1,7 @@
+use std::path::PathBuf;
 use std::process::exit;
 
-use clap::{Arg, Command};
+use clap::{value_parser, Arg, Command};
 
 fn applet_commands() -> [Command<'static>; 2] {
     [
@@ -24,6 +25,7 @@ fn main() {
                         .exclusive(true)
                         .takes_value(true)
                         .default_missing_value("/usr/local/bin")
+                        .value_parser(value_parser!(PathBuf))
                         .use_value_delimiter(false),
                 )
                 .subcommands(applet_commands()),

examples/pacman.rs

@@ -71,13 +71,23 @@ fn main() {
     match matches.subcommand() {
         Some(("sync", sync_matches)) => {
             if sync_matches.is_present("search") {
-                let packages: Vec<_> = sync_matches.values_of("search").unwrap().collect();
+                let packages: Vec<_> = sync_matches
+                    .get_many::<String>("search")
+                    .expect("matches definition")
+                    .expect("is present")
+                    .map(|s| s.as_str())
+                    .collect();
                 let values = packages.join(", ");
                 println!("Searching for {}...", values);
                 return;
             }
 
-            let packages: Vec<_> = sync_matches.values_of("package").unwrap().collect();
+            let packages: Vec<_> = sync_matches
+                .get_many::<String>("package")
+                .expect("matches definition")
+                .expect("is present")
+                .map(|s| s.as_str())
+                .collect();
             let values = packages.join(", ");
 
             if sync_matches.is_present("info") {
@@ -87,11 +97,17 @@ fn main() {
             }
         }
         Some(("query", query_matches)) => {
-            if let Some(packages) = query_matches.values_of("info") {
-                let comma_sep = packages.collect::<Vec<_>>().join(", ");
+            if let Some(packages) = query_matches
+                .get_many::<String>("info")
+                .expect("matches definition")
+            {
+                let comma_sep = packages.map(|s| s.as_str()).collect::<Vec<_>>().join(", ");
                 println!("Retrieving info for {}...", comma_sep);
-            } else if let Some(queries) = query_matches.values_of("search") {
-                let comma_sep = queries.collect::<Vec<_>>().join(", ");
+            } else if let Some(queries) = query_matches
+                .get_many::<String>("search")
+                .expect("matches definition")
+            {
+                let comma_sep = queries.map(|s| s.as_str()).collect::<Vec<_>>().join(", ");
                 println!("Searching Locally for {}...", comma_sep);
             } else {
                 println!("Displaying all locally installed packages...");

examples/tutorial_builder/01_quick.rs

@@ -1,7 +1,8 @@
 // Note: this requires the `cargo` feature
 
-use clap::{arg, command, Command};
-use std::path::Path;
+use std::path::PathBuf;
+
+use clap::{arg, command, value_parser, Command};
 
 fn main() {
     let matches = command!()
@@ -12,8 +13,7 @@ fn main() {
             )
             // We don't have syntax yet for optional options, so manually calling `required`
             .required(false)
-            // Support non-UTF8 paths
-            .allow_invalid_utf8(true),
+            .value_parser(value_parser!(PathBuf)),
         )
         .arg(arg!(
             -d --debug ... "Turn debugging information on"
@@ -26,12 +26,17 @@ fn main() {
         .get_matches();
 
     // You can check the value provided by positional arguments, or option arguments
-    if let Some(name) = matches.value_of("name") {
+    if let Some(name) = matches
+        .get_one::<String>("name")
+        .expect("matches definition")
+    {
         println!("Value for name: {}", name);
     }
 
-    if let Some(raw_config) = matches.value_of_os("config") {
-        let config_path = Path::new(raw_config);
+    if let Some(config_path) = matches
+        .get_one::<PathBuf>("config")
+        .expect("matches definition")
+    {
         println!("Value for config: {}", config_path.display());
     }
 

examples/tutorial_builder/02_app_settings.rs

@@ -11,6 +11,18 @@ fn main() {
         .arg(arg!(--one <VALUE>))
         .get_matches();
 
-    println!("two: {:?}", matches.value_of("two").expect("required"));
-    println!("one: {:?}", matches.value_of("one").expect("required"));
+    println!(
+        "two: {:?}",
+        matches
+            .get_one::<String>("two")
+            .expect("matches definition")
+            .expect("required")
+    );
+    println!(
+        "one: {:?}",
+        matches
+            .get_one::<String>("one")
+            .expect("matches definition")
+            .expect("required")
+    );
 }