refactor(install): Move version parsing to the CLI

Author: epage

src/bin/cargo/commands/install.rs

@@ -1,9 +1,15 @@
 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;
 
@@ -15,6 +21,7 @@ pub fn cli() -> Command {
             opt("version", "Specify a version to install")
                 .alias("vers")
                 .value_name("VERSION")
+                .value_parser(parse_semver_flag)
                 .requires("crate"),
         )
         .arg(
@@ -98,7 +105,7 @@ 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::<CrateVersion>("crate")
         .unwrap_or_default()
@@ -187,7 +194,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     Ok(())
 }
 
-type CrateVersion = (String, Option<String>);
+type CrateVersion = (String, Option<VersionReq>);
 
 fn parse_crate(krate: &str) -> crate::CargoResult<CrateVersion> {
     let (krate, version) = if let Some((k, v)) = krate.split_once('@') {
@@ -196,7 +203,7 @@ fn parse_crate(krate: &str) -> crate::CargoResult<CrateVersion> {
             anyhow::bail!("missing crate name before '@'");
         }
         let krate = k.to_owned();
-        let version = Some(v.to_owned());
+        let version = Some(parse_semver_flag(v)?);
         (krate, version)
     } else {
         let krate = krate.to_owned();
@@ -211,14 +218,58 @@ fn parse_crate(krate: &str) -> crate::CargoResult<CrateVersion> {
     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<String>,
-    version: Option<&str>,
+    local_version: Option<VersionReq>,
+    version: Option<&VersionReq>,
 ) -> crate::CargoResult<CrateVersion> {
     let version = match (local_version, version) {
-        (Some(l), Some(g)) => {
-            anyhow::bail!("cannot specify both `@{l}` and `--version {g}`");
+        (Some(_), Some(_)) => {
+            anyhow::bail!("cannot specify both `@<VERSION>` and `--version <VERSION>`");
         }
         (Some(l), None) => Some(l),
         (None, Some(g)) => Some(g.to_owned()),

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<(String, Option<String>)>,
+    krates: Vec<(String, Option<VersionReq>)>,
     source_id: SourceId,
     from_cwd: bool,
     opts: &ops::CompileOptions,
@@ -619,7 +619,7 @@ pub fn install(
         let (krate, vers) = krates
             .iter()
             .next()
-            .map(|(k, v)| (Some(k.as_str()), v.as_deref()))
+            .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,
@@ -648,7 +648,7 @@ pub fn install(
                     Some(krate.as_str()),
                     source_id,
                     from_cwd,
-                    vers.as_deref(),
+                    vers.as_ref(),
                     opts,
                     force,
                     no_track,
@@ -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

@@ -1617,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 0.1.1`")
+        .with_stderr("error: cannot specify both `@<VERSION>` and `--version <VERSION>`")
         .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();
 }