From e70e310ce0134a2baff8292bd4e86c4a1dac564c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 6 Nov 2023 15:24:32 -0600 Subject: [PATCH] fix: Report more detailed semver errors For `cargo install` we'll now show a more specific parse error for semver, much like other parts of cargo. This came out of my work on #12801. I was looking at what might be appropriate to put in a `cargo-util-semver` crate and realized we have the `ToSemver` trait that exists but doesn't do much, so I dropped it. --- Cargo.lock | 1 + crates/xtask-bump-check/Cargo.toml | 3 ++- crates/xtask-bump-check/src/xtask.rs | 3 +-- src/bin/cargo/commands/install.rs | 3 +-- src/cargo/core/package_id.rs | 10 ++++---- src/cargo/sources/registry/index.rs | 2 +- src/cargo/util/mod.rs | 2 -- src/cargo/util/to_semver.rs | 36 ---------------------------- src/cargo/util/toml/mod.rs | 10 +++----- tests/testsuite/install_upgrade.rs | 2 +- triagebot.toml | 1 - 11 files changed, 15 insertions(+), 58 deletions(-) delete mode 100644 src/cargo/util/to_semver.rs diff --git a/Cargo.lock b/Cargo.lock index 91625381266..cdc5f59b94e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3785,6 +3785,7 @@ dependencies = [ "cargo-util", "clap", "git2", + "semver", "tracing", "tracing-subscriber", ] diff --git a/crates/xtask-bump-check/Cargo.toml b/crates/xtask-bump-check/Cargo.toml index e878f7dda66..c8a472adcc4 100644 --- a/crates/xtask-bump-check/Cargo.toml +++ b/crates/xtask-bump-check/Cargo.toml @@ -11,5 +11,6 @@ cargo.workspace = true cargo-util.workspace = true clap.workspace = true git2.workspace = true -tracing.workspace = true +semver.workspace = true tracing-subscriber.workspace = true +tracing.workspace = true diff --git a/crates/xtask-bump-check/src/xtask.rs b/crates/xtask-bump-check/src/xtask.rs index f2ff29974a9..b99ac8b3249 100644 --- a/crates/xtask-bump-check/src/xtask.rs +++ b/crates/xtask-bump-check/src/xtask.rs @@ -24,7 +24,6 @@ use cargo::core::Workspace; use cargo::sources::source::QueryKind; use cargo::util::cache_lock::CacheLockMode; use cargo::util::command_prelude::*; -use cargo::util::ToSemver; use cargo::CargoResult; use cargo_util::ProcessBuilder; @@ -277,7 +276,7 @@ fn beta_and_stable_branch(repo: &git2::Repository) -> CargoResult<[git2::Branch< tracing::trace!("branch `{name}` is not in the format of `/rust-`"); continue; }; - let Ok(version) = version.to_semver() else { + let Ok(version) = version.parse::() else { tracing::trace!("branch `{name}` is not a valid semver: `{version}`"); continue; }; diff --git a/src/bin/cargo/commands/install.rs b/src/bin/cargo/commands/install.rs index b51c021f567..016ba5d5ce5 100644 --- a/src/bin/cargo/commands/install.rs +++ b/src/bin/cargo/commands/install.rs @@ -6,7 +6,6 @@ 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 itertools::Itertools; @@ -264,7 +263,7 @@ fn parse_semver_flag(v: &str) -> CargoResult { ), } } else { - match v.to_semver() { + match v.trim().parse() { Ok(v) => Ok(VersionReq::exact(&v)), Err(e) => { let mut msg = e.to_string(); diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index 33c2c7a8530..3e9c03a470b 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -12,7 +12,7 @@ use serde::ser; use crate::core::SourceId; use crate::util::interning::InternedString; -use crate::util::{CargoResult, ToSemver}; +use crate::util::CargoResult; static PACKAGE_ID_CACHE: OnceLock>> = OnceLock::new(); @@ -82,7 +82,7 @@ impl<'de> de::Deserialize<'de> for PackageId { let (field, rest) = rest .split_once(' ') .ok_or_else(|| de::Error::custom("invalid serialized PackageId"))?; - let version = field.to_semver().map_err(de::Error::custom)?; + let version = field.parse().map_err(de::Error::custom)?; let url = strip_parens(rest).ok_or_else(|| de::Error::custom("invalid serialized PackageId"))?; @@ -123,12 +123,12 @@ impl Hash for PackageId { } impl PackageId { - pub fn new( + pub fn new( name: impl Into, - version: T, + version: &str, sid: SourceId, ) -> CargoResult { - let v = version.to_semver()?; + let v = version.parse()?; Ok(PackageId::pure(name.into(), v, sid)) } diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index 35b59c01ee9..00f21d669e6 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -935,7 +935,7 @@ impl IndexSummary { } = serde_json::from_slice(line)?; let v = v.unwrap_or(1); tracing::trace!("json parsed registry {}/{}", name, vers); - let pkgid = PackageId::new(name, &vers, source_id)?; + let pkgid = PackageId::pure(name.into(), vers.clone(), source_id); let deps = deps .into_iter() .map(|dep| dep.into_dep(source_id)) diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index e73dc42be1f..3a30bcb39c2 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -23,7 +23,6 @@ pub use self::queue::Queue; pub use self::restricted_names::validate_package_name; pub use self::rustc::Rustc; pub use self::semver_ext::{OptVersionReq, PartialVersion, RustVersion, VersionExt, VersionReqExt}; -pub use self::to_semver::ToSemver; pub use self::vcs::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo}; pub use self::workspace::{ add_path_args, path_args, print_available_benches, print_available_binaries, @@ -62,7 +61,6 @@ pub mod restricted_names; pub mod rustc; mod semver_ext; pub mod style; -pub mod to_semver; pub mod toml; pub mod toml_mut; mod vcs; diff --git a/src/cargo/util/to_semver.rs b/src/cargo/util/to_semver.rs deleted file mode 100644 index 3cc9e5706ac..00000000000 --- a/src/cargo/util/to_semver.rs +++ /dev/null @@ -1,36 +0,0 @@ -use crate::util::errors::CargoResult; -use semver::Version; - -pub trait ToSemver { - fn to_semver(self) -> CargoResult; -} - -impl ToSemver for Version { - fn to_semver(self) -> CargoResult { - Ok(self) - } -} - -impl<'a> ToSemver for &'a str { - fn to_semver(self) -> CargoResult { - match Version::parse(self.trim()) { - Ok(v) => Ok(v), - Err(..) => Err(anyhow::format_err!( - "cannot parse '{}' as a SemVer version", - self - )), - } - } -} - -impl<'a> ToSemver for &'a String { - fn to_semver(self) -> CargoResult { - (**self).to_semver() - } -} - -impl<'a> ToSemver for &'a Version { - fn to_semver(self) -> CargoResult { - Ok(self.clone()) - } -} diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 120dfcf84d2..35aa3c50f17 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -536,7 +536,7 @@ impl schema::TomlManifest { version .clone() .unwrap_or_else(|| semver::Version::new(0, 0, 0)), - )?; + ); let edition = if let Some(edition) = package.edition.clone() { let edition: Edition = edition @@ -1596,12 +1596,8 @@ impl schema::InheritableFields { } impl schema::TomlPackage { - pub fn to_package_id( - &self, - source_id: SourceId, - version: semver::Version, - ) -> CargoResult { - PackageId::new(&self.name, version, source_id) + pub fn to_package_id(&self, source_id: SourceId, version: semver::Version) -> PackageId { + PackageId::pure(self.name.as_str().into(), version, source_id) } } diff --git a/tests/testsuite/install_upgrade.rs b/tests/testsuite/install_upgrade.rs index 580117f5cdc..fe4f8c6c705 100644 --- a/tests/testsuite/install_upgrade.rs +++ b/tests/testsuite/install_upgrade.rs @@ -230,7 +230,7 @@ fn ambiguous_version_no_longer_allowed() { cargo_process("install foo --version=1.0") .with_stderr( "\ -[ERROR] invalid value '1.0' for '--version ': cannot parse '1.0' as a SemVer version +[ERROR] invalid value '1.0' for '--version ': unexpected end of input while parsing minor version number tip: if you want to specify SemVer range, add an explicit qualifier, like '^1.0' diff --git a/triagebot.toml b/triagebot.toml index d005a68805b..cdf1090a10e 100644 --- a/triagebot.toml +++ b/triagebot.toml @@ -201,7 +201,6 @@ trigger_files = ["src/cargo/util/auth/"] trigger_files = [ "crates/semver-check", "src/cargo/util/semver_ext.rs", - "src/cargo/util/to_semver.rs", ] [autolabel."A-source-replacement"]