Auto merge of #12547 - epage:install, r=weihanglo

refactor(install): Move value parsing to clap

### What does this PR try to resolve?

Originally, I thought this was going to help some of my other work but it won't.  Instead I decided to finish and post this PR in case there was interest since `@weihanglo` expressed interest in using `Arg::value_parser` more and this demonstrates that.

### How should we test and review this PR?

A commit at a time.  There seemed to be existing tests for some of the error changes.

src/bin/cargo/commands/install.rs

@@ -1,24 +1,27 @@
 use crate::command_prelude::*;
 
 use anyhow::anyhow;
+use anyhow::bail;
+use anyhow::format_err;
 use cargo::core::{GitReference, SourceId, Workspace};
 use cargo::ops;
 use cargo::util::IntoUrl;
+use cargo::util::ToSemver;
+use cargo::util::VersionReqExt;
+use cargo::CargoResult;
+use semver::VersionReq;
 
 use cargo_util::paths;
 
 pub fn cli() -> Command {
     subcommand("install")
         .about("Install a Rust binary. Default location is $HOME/.cargo/bin")
-        .arg(
-            Arg::new("crate")
-                .value_parser(clap::builder::NonEmptyStringValueParser::new())
-                .num_args(0..),
-        )
+        .arg(Arg::new("crate").value_parser(parse_crate).num_args(0..))
         .arg(
             opt("version", "Specify a version to install")
                 .alias("vers")
                 .value_name("VERSION")
+                .value_parser(parse_semver_flag)
                 .requires("crate"),
         )
         .arg(
@@ -102,11 +105,12 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     // but not `Config::reload_rooted_at` which is always cwd)
     let path = path.map(|p| paths::normalize_path(&p));
 
-    let version = args.get_one::<String>("version").map(String::as_str);
+    let version = args.get_one::<VersionReq>("version");
     let krates = args
-        .get_many::<String>("crate")
+        .get_many::<CrateVersion>("crate")
         .unwrap_or_default()
-        .map(|k| resolve_crate(k, version))
+        .cloned()
+        .map(|(krate, local_version)| resolve_crate(krate, local_version, version))
         .collect::<crate::CargoResult<Vec<_>>>()?;
 
     for (crate_name, _) in krates.iter() {
@@ -190,20 +194,86 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     Ok(())
 }
 
-fn resolve_crate<'k>(
-    mut krate: &'k str,
-    mut version: Option<&'k str>,
-) -> crate::CargoResult<(&'k str, Option<&'k str>)> {
-    if let Some((k, v)) = krate.split_once('@') {
-        if version.is_some() {
-            anyhow::bail!("cannot specify both `@{v}` and `--version`");
-        }
+type CrateVersion = (String, Option<VersionReq>);
+
+fn parse_crate(krate: &str) -> crate::CargoResult<CrateVersion> {
+    let (krate, version) = if let Some((k, v)) = krate.split_once('@') {
         if k.is_empty() {
             // by convention, arguments starting with `@` are response files
-            anyhow::bail!("missing crate name for `@{v}`");
+            anyhow::bail!("missing crate name before '@'");
         }
-        krate = k;
-        version = Some(v);
+        let krate = k.to_owned();
+        let version = Some(parse_semver_flag(v)?);
+        (krate, version)
+    } else {
+        let krate = krate.to_owned();
+        let version = None;
+        (krate, version)
+    };
+
+    if krate.is_empty() {
+        anyhow::bail!("crate name is empty");
     }
+
+    Ok((krate, version))
+}
+
+/// Parses x.y.z as if it were =x.y.z, and gives CLI-specific error messages in the case of invalid
+/// values.
+fn parse_semver_flag(v: &str) -> CargoResult<VersionReq> {
+    // If the version begins with character <, >, =, ^, ~ parse it as a
+    // version range, otherwise parse it as a specific version
+    let first = v
+        .chars()
+        .next()
+        .ok_or_else(|| format_err!("no version provided for the `--version` flag"))?;
+
+    let is_req = "<>=^~".contains(first) || v.contains('*');
+    if is_req {
+        match v.parse::<VersionReq>() {
+            Ok(v) => Ok(v),
+            Err(_) => bail!(
+                "the `--version` provided, `{}`, is \
+                     not a valid semver version requirement\n\n\
+                     Please have a look at \
+                     https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html \
+                     for the correct format",
+                v
+            ),
+        }
+    } else {
+        match v.to_semver() {
+            Ok(v) => Ok(VersionReq::exact(&v)),
+            Err(e) => {
+                let mut msg = e.to_string();
+
+                // If it is not a valid version but it is a valid version
+                // requirement, add a note to the warning
+                if v.parse::<VersionReq>().is_ok() {
+                    msg.push_str(&format!(
+                        "\n\n  tip: if you want to specify semver range, \
+                             add an explicit qualifier, like '^{}'",
+                        v
+                    ));
+                }
+                bail!(msg);
+            }
+        }
+    }
+}
+
+fn resolve_crate(
+    krate: String,
+    local_version: Option<VersionReq>,
+    version: Option<&VersionReq>,
+) -> crate::CargoResult<CrateVersion> {
+    let version = match (local_version, version) {
+        (Some(_), Some(_)) => {
+            anyhow::bail!("cannot specify both `@<VERSION>` and `--version <VERSION>`");
+        }
+        (Some(l), None) => Some(l),
+        (None, Some(g)) => Some(g.to_owned()),
+        (None, None) => None,
+    };
     Ok((krate, version))
 }

src/cargo/ops/cargo_install.rs

@@ -11,10 +11,10 @@ use crate::ops::{common_for_install_and_uninstall::*, FilterRule};
 use crate::ops::{CompileFilter, Packages};
 use crate::sources::{GitSource, PathSource, SourceConfigMap};
 use crate::util::errors::CargoResult;
-use crate::util::{Config, Filesystem, Rustc, ToSemver, VersionReqExt};
+use crate::util::{Config, Filesystem, Rustc};
 use crate::{drop_println, ops};
 
-use anyhow::{bail, format_err, Context as _};
+use anyhow::{bail, Context as _};
 use cargo_util::paths;
 use itertools::Itertools;
 use semver::VersionReq;
@@ -38,12 +38,12 @@ impl Drop for Transaction {
     }
 }
 
-struct InstallablePackage<'cfg, 'a> {
+struct InstallablePackage<'cfg> {
     config: &'cfg Config,
     opts: ops::CompileOptions,
     root: Filesystem,
     source_id: SourceId,
-    vers: Option<&'a str>,
+    vers: Option<VersionReq>,
     force: bool,
     no_track: bool,
 
@@ -53,7 +53,7 @@ struct InstallablePackage<'cfg, 'a> {
     target: String,
 }
 
-impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
+impl<'cfg> InstallablePackage<'cfg> {
     // Returns pkg to install. None if pkg is already installed
     pub fn new(
         config: &'cfg Config,
@@ -62,12 +62,12 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
         krate: Option<&str>,
         source_id: SourceId,
         from_cwd: bool,
-        vers: Option<&'a str>,
-        original_opts: &'a ops::CompileOptions,
+        vers: Option<&VersionReq>,
+        original_opts: &ops::CompileOptions,
         force: bool,
         no_track: bool,
         needs_update_if_source_is_index: bool,
-    ) -> CargoResult<Option<InstallablePackage<'cfg, 'a>>> {
+    ) -> CargoResult<Option<Self>> {
         if let Some(name) = krate {
             if name == "." {
                 bail!(
@@ -82,8 +82,8 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
         let pkg = {
             let dep = {
                 if let Some(krate) = krate {
-                    let vers = if let Some(vers_flag) = vers {
-                        Some(parse_semver_flag(vers_flag)?.to_string())
+                    let vers = if let Some(vers) = vers {
+                        Some(vers.to_string())
                     } else if source_id.is_registry() {
                         // Avoid pre-release versions from crate.io
                         // unless explicitly asked for
@@ -234,7 +234,7 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
             opts,
             root,
             source_id,
-            vers,
+            vers: vers.cloned(),
             force,
             no_track,
 
@@ -604,7 +604,7 @@ Consider enabling some of the needed features by passing, e.g., `--features=\"{e
 pub fn install(
     config: &Config,
     root: Option<&str>,
-    krates: Vec<(&str, Option<&str>)>,
+    krates: Vec<(String, Option<VersionReq>)>,
     source_id: SourceId,
     from_cwd: bool,
     opts: &ops::CompileOptions,
@@ -617,9 +617,9 @@ pub fn install(
 
     let (installed_anything, scheduled_error) = if krates.len() <= 1 {
         let (krate, vers) = krates
-            .into_iter()
+            .iter()
             .next()
-            .map(|(k, v)| (Some(k), v))
+            .map(|(k, v)| (Some(k.as_str()), v.as_ref()))
             .unwrap_or((None, None));
         let installable_pkg = InstallablePackage::new(
             config, root, map, krate, source_id, from_cwd, vers, opts, force, no_track, true,
@@ -637,18 +637,18 @@ pub fn install(
         let mut did_update = false;
 
         let pkgs_to_install: Vec<_> = krates
-            .into_iter()
+            .iter()
             .filter_map(|(krate, vers)| {
                 let root = root.clone();
                 let map = map.clone();
                 match InstallablePackage::new(
                     config,
                     root,
                     map,
-                    Some(krate),
+                    Some(krate.as_str()),
                     source_id,
                     from_cwd,
-                    vers,
+                    vers.as_ref(),
                     opts,
                     force,
                     no_track,
@@ -660,12 +660,12 @@ pub fn install(
                     }
                     Ok(None) => {
                         // Already installed
-                        succeeded.push(krate);
+                        succeeded.push(krate.as_str());
                         None
                     }
                     Err(e) => {
                         crate::display_error(&e, &mut config.shell());
-                        failed.push(krate);
+                        failed.push(krate.as_str());
                         // We assume an update was performed if we got an error.
                         did_update = true;
                         None
@@ -805,54 +805,6 @@ fn make_ws_rustc_target<'cfg>(
     Ok((ws, rustc, target))
 }
 
-/// Parses x.y.z as if it were =x.y.z, and gives CLI-specific error messages in the case of invalid
-/// values.
-fn parse_semver_flag(v: &str) -> CargoResult<VersionReq> {
-    // If the version begins with character <, >, =, ^, ~ parse it as a
-    // version range, otherwise parse it as a specific version
-    let first = v
-        .chars()
-        .next()
-        .ok_or_else(|| format_err!("no version provided for the `--version` flag"))?;
-
-    let is_req = "<>=^~".contains(first) || v.contains('*');
-    if is_req {
-        match v.parse::<VersionReq>() {
-            Ok(v) => Ok(v),
-            Err(_) => bail!(
-                "the `--version` provided, `{}`, is \
-                     not a valid semver version requirement\n\n\
-                     Please have a look at \
-                     https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html \
-                     for the correct format",
-                v
-            ),
-        }
-    } else {
-        match v.to_semver() {
-            Ok(v) => Ok(VersionReq::exact(&v)),
-            Err(e) => {
-                let mut msg = format!(
-                    "the `--version` provided, `{}`, is \
-                         not a valid semver version: {}\n",
-                    v, e
-                );
-
-                // If it is not a valid version but it is a valid version
-                // requirement, add a note to the warning
-                if v.parse::<VersionReq>().is_ok() {
-                    msg.push_str(&format!(
-                        "\nif you want to specify semver range, \
-                             add an explicit qualifier, like ^{}",
-                        v
-                    ));
-                }
-                bail!(msg);
-            }
-        }
-    }
-}
-
 /// Display a list of installed binaries.
 pub fn install_list(dst: Option<&str>, config: &Config) -> CargoResult<()> {
     let root = resolve_root(dst, config)?;

tests/testsuite/install.rs

@@ -1600,8 +1600,13 @@ fn inline_version_without_name() {
     pkg("foo", "0.1.2");
 
     cargo_process("install @0.1.1")
-        .with_status(101)
-        .with_stderr("error: missing crate name for `@0.1.1`")
+        .with_status(1)
+        .with_stderr(
+            "error: invalid value '@0.1.1' for '[crate]...': missing crate name before '@'
+
+For more information, try '--help'.
+",
+        )
         .run();
 }
 
@@ -1612,7 +1617,7 @@ fn inline_and_explicit_version() {
 
     cargo_process("install [email protected] --version 0.1.1")
         .with_status(101)
-        .with_stderr("error: cannot specify both `@0.1.1` and `--version`")
+        .with_stderr("error: cannot specify both `@<VERSION>` and `--version <VERSION>`")
         .run();
 }
 
@@ -1827,7 +1832,7 @@ fn install_empty_argument() {
     cargo_process("install")
         .arg("")
         .with_status(1)
-        .with_stderr_contains("[ERROR] a value is required for '[crate]...' but none was supplied")
+        .with_stderr_contains("[ERROR] invalid value '' for '[crate]...': crate name is empty")
         .run();
 }
 

tests/testsuite/install_upgrade.rs

@@ -230,12 +230,14 @@ fn ambiguous_version_no_longer_allowed() {
     cargo_process("install foo --version=1.0")
         .with_stderr(
             "\
-[ERROR] the `--version` provided, `1.0`, is not a valid semver version: cannot parse '1.0' as a semver
+[ERROR] invalid value '1.0' for '--version <VERSION>': cannot parse '1.0' as a semver
 
-if you want to specify semver range, add an explicit qualifier, like ^1.0
+  tip: if you want to specify semver range, add an explicit qualifier, like '^1.0'
+
+For more information, try '--help'.
 ",
         )
-        .with_status(101)
+        .with_status(1)
         .run();
 }