From 6ac5bb56b3170333953841ccc8f4423735ff223d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 23 Aug 2023 16:50:37 -0500 Subject: [PATCH 1/3] refactor(add): Pull out rust-version parsing --- src/cargo/ops/cargo_add/mod.rs | 14 ++++----- src/cargo/util/mod.rs | 2 +- src/cargo/util/semver_ext.rs | 53 ++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/src/cargo/ops/cargo_add/mod.rs b/src/cargo/ops/cargo_add/mod.rs index 11ca282ee42..1f812ecbeda 100644 --- a/src/cargo/ops/cargo_add/mod.rs +++ b/src/cargo/ops/cargo_add/mod.rs @@ -34,6 +34,7 @@ use crate::util::toml_mut::dependency::Source; use crate::util::toml_mut::dependency::WorkspaceSource; use crate::util::toml_mut::manifest::DepTable; use crate::util::toml_mut::manifest::LocalManifest; +use crate::util::PartialVersion; use crate::CargoResult; use crate::Config; use crate_spec::CrateSpec; @@ -568,15 +569,10 @@ fn get_latest_dependency( if config.cli_unstable().msrv_policy && honor_rust_version { fn parse_msrv(rust_version: impl AsRef) -> (u64, u64, u64) { - // HACK: `rust-version` is a subset of the `VersionReq` syntax that only ever - // has one comparator with a required minor and optional patch, and uses no - // other features. If in the future this syntax is expanded, this code will need - // to be updated. - let version_req = semver::VersionReq::parse(rust_version.as_ref()).unwrap(); - assert!(version_req.comparators.len() == 1); - let comp = &version_req.comparators[0]; - assert_eq!(comp.op, semver::Op::Caret); - assert_eq!(comp.pre, semver::Prerelease::EMPTY); + let comp = rust_version + .as_ref() + .parse::() + .expect("validated on parsing of manifest"); (comp.major, comp.minor.unwrap_or(0), comp.patch.unwrap_or(0)) } diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 26e97e2d20c..af7dd368781 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -22,7 +22,7 @@ pub use self::progress::{Progress, ProgressStyle}; pub use self::queue::Queue; pub use self::restricted_names::validate_package_name; pub use self::rustc::Rustc; -pub use self::semver_ext::{OptVersionReq, VersionExt, VersionReqExt}; +pub use self::semver_ext::{OptVersionReq, PartialVersion, VersionExt, VersionReqExt}; pub use self::to_semver::ToSemver; pub use self::vcs::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo}; pub use self::workspace::{ diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs index de6d68e16ae..2a210601b46 100644 --- a/src/cargo/util/semver_ext.rs +++ b/src/cargo/util/semver_ext.rs @@ -108,6 +108,59 @@ impl From for OptVersionReq { } } +#[derive(PartialEq, Eq, Hash, Clone, Debug)] +pub struct PartialVersion { + pub major: u64, + pub minor: Option, + pub patch: Option, +} + +impl std::str::FromStr for PartialVersion { + type Err = anyhow::Error; + + fn from_str(value: &str) -> Result { + // HACK: `PartialVersion` is a subset of the `VersionReq` syntax that only ever + // has one comparator with a required minor and optional patch, and uses no + // other features. + let version_req = match semver::VersionReq::parse(value) { + // Exclude semver operators like `^` and pre-release identifiers + Ok(req) if value.chars().all(|c| c.is_ascii_digit() || c == '.') => req, + _ => anyhow::bail!("`rust-version` must be a value like \"1.32\""), + }; + assert_eq!( + version_req.comparators.len(), + 1, + "guarenteed by character check" + ); + let comp = &version_req.comparators[0]; + assert_eq!(comp.op, semver::Op::Caret, "guarenteed by character check"); + assert_eq!( + comp.pre, + semver::Prerelease::EMPTY, + "guarenteed by character check" + ); + Ok(PartialVersion { + major: comp.major, + minor: comp.minor, + patch: comp.patch, + }) + } +} + +impl Display for PartialVersion { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let major = self.major; + write!(f, "{major}")?; + if let Some(minor) = self.minor { + write!(f, ".{minor}")?; + } + if let Some(patch) = self.patch { + write!(f, ".{patch}")?; + } + Ok(()) + } +} + #[cfg(test)] mod tests { use super::*; From 423a3345203d71c63cad28d1e4e31ccc75f6804f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 23 Aug 2023 20:17:20 -0500 Subject: [PATCH 2/3] refactor: Parse, don't validate, rust_version By using the `PartialVersion` type with serde, we get the context for the error automatically. --- crates/resolver-tests/src/lib.rs | 10 ++-- src/cargo/core/compiler/compilation.rs | 3 +- src/cargo/core/manifest.rs | 12 ++--- src/cargo/core/package.rs | 7 +-- src/cargo/core/resolver/version_prefs.rs | 3 +- src/cargo/core/summary.rs | 9 ++-- src/cargo/ops/cargo_add/mod.rs | 8 +-- src/cargo/ops/cargo_compile/mod.rs | 2 +- src/cargo/ops/registry/publish.rs | 3 +- src/cargo/sources/registry/index.rs | 6 ++- src/cargo/util/semver_ext.rs | 54 +++++++++++++++++++- src/cargo/util/toml/mod.rs | 63 ++++++++++++++++++------ tests/testsuite/rust_version.rs | 33 ++++++++++--- 13 files changed, 159 insertions(+), 54 deletions(-) diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index ab34e866310..12c474a0eba 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -15,7 +15,7 @@ use cargo::core::resolver::{self, ResolveOpts, VersionPreferences}; use cargo::core::source::{GitReference, QueryKind, SourceId}; use cargo::core::Resolve; use cargo::core::{Dependency, PackageId, Registry, Summary}; -use cargo::util::{CargoResult, Config, Graph, IntoUrl}; +use cargo::util::{CargoResult, Config, Graph, IntoUrl, PartialVersion}; use proptest::collection::{btree_map, vec}; use proptest::prelude::*; @@ -183,7 +183,7 @@ pub fn resolve_with_config_raw( deps, &BTreeMap::new(), None::<&String>, - None::<&String>, + None::, ) .unwrap(); let opts = ResolveOpts::everything(); @@ -584,7 +584,7 @@ pub fn pkg_dep(name: T, dep: Vec) -> Summary { dep, &BTreeMap::new(), link, - None::<&String>, + None::, ) .unwrap() } @@ -612,7 +612,7 @@ pub fn pkg_loc(name: &str, loc: &str) -> Summary { Vec::new(), &BTreeMap::new(), link, - None::<&String>, + None::, ) .unwrap() } @@ -626,7 +626,7 @@ pub fn remove_dep(sum: &Summary, ind: usize) -> Summary { deps, &BTreeMap::new(), sum.links().map(|a| a.as_str()), - None::<&String>, + None::, ) .unwrap() } diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index b263119b039..b693cfc3125 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -312,6 +312,7 @@ impl<'cfg> Compilation<'cfg> { // crate properties which might require rebuild upon change // consider adding the corresponding properties to the hash // in BuildContext::target_metadata() + let rust_version = pkg.rust_version().as_ref().map(ToString::to_string); cmd.env("CARGO_MANIFEST_DIR", pkg.root()) .env("CARGO_PKG_VERSION_MAJOR", &pkg.version().major.to_string()) .env("CARGO_PKG_VERSION_MINOR", &pkg.version().minor.to_string()) @@ -342,7 +343,7 @@ impl<'cfg> Compilation<'cfg> { .env("CARGO_PKG_AUTHORS", &pkg.authors().join(":")) .env( "CARGO_PKG_RUST_VERSION", - &pkg.rust_version().unwrap_or(&String::new()), + &rust_version.as_deref().unwrap_or_default(), ) .env( "CARGO_PKG_README", diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 5d46a7e06fa..a6ccc07ce3a 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -19,7 +19,7 @@ use crate::core::{Edition, Feature, Features, WorkspaceConfig}; use crate::util::errors::*; use crate::util::interning::InternedString; use crate::util::toml::{TomlManifest, TomlProfiles}; -use crate::util::{short_hash, Config, Filesystem}; +use crate::util::{short_hash, Config, Filesystem, PartialVersion}; pub enum EitherManifest { Real(Manifest), @@ -58,7 +58,7 @@ pub struct Manifest { original: Rc, unstable_features: Features, edition: Edition, - rust_version: Option, + rust_version: Option, im_a_teapot: Option, default_run: Option, metabuild: Option>, @@ -112,7 +112,7 @@ pub struct ManifestMetadata { pub documentation: Option, // URL pub badges: BTreeMap>, pub links: Option, - pub rust_version: Option, + pub rust_version: Option, } #[derive(Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] @@ -401,7 +401,7 @@ impl Manifest { workspace: WorkspaceConfig, unstable_features: Features, edition: Edition, - rust_version: Option, + rust_version: Option, im_a_teapot: Option, default_run: Option, original: Rc, @@ -570,8 +570,8 @@ impl Manifest { self.edition } - pub fn rust_version(&self) -> Option<&str> { - self.rust_version.as_deref() + pub fn rust_version(&self) -> Option { + self.rust_version } pub fn custom_metadata(&self) -> Option<&toml::Value> { diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index c84941462b4..2437cdb2401 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -31,6 +31,7 @@ use crate::util::network::http::http_handle_and_timeout; use crate::util::network::http::HttpTimeout; use crate::util::network::retry::{Retry, RetryResult}; use crate::util::network::sleep::SleepTracker; +use crate::util::PartialVersion; use crate::util::{self, internal, Config, Progress, ProgressStyle}; pub const MANIFEST_PREAMBLE: &str = "\ @@ -103,7 +104,7 @@ pub struct SerializedPackage { #[serde(skip_serializing_if = "Option::is_none")] metabuild: Option>, default_run: Option, - rust_version: Option, + rust_version: Option, } impl Package { @@ -177,7 +178,7 @@ impl Package { self.targets().iter().any(|target| target.proc_macro()) } /// Gets the package's minimum Rust version. - pub fn rust_version(&self) -> Option<&str> { + pub fn rust_version(&self) -> Option { self.manifest().rust_version() } @@ -262,7 +263,7 @@ impl Package { metabuild: self.manifest().metabuild().cloned(), publish: self.publish().as_ref().cloned(), default_run: self.manifest().default_run().map(|s| s.to_owned()), - rust_version: self.rust_version().map(|s| s.to_owned()), + rust_version: self.rust_version(), } } } diff --git a/src/cargo/core/resolver/version_prefs.rs b/src/cargo/core/resolver/version_prefs.rs index bf26d0498f0..a61f7cac37b 100644 --- a/src/cargo/core/resolver/version_prefs.rs +++ b/src/cargo/core/resolver/version_prefs.rs @@ -81,6 +81,7 @@ impl VersionPreferences { mod test { use super::*; use crate::core::SourceId; + use crate::util::PartialVersion; use std::collections::BTreeMap; fn pkgid(name: &str, version: &str) -> PackageId { @@ -103,7 +104,7 @@ mod test { Vec::new(), &features, None::<&String>, - None::<&String>, + None::, ) .unwrap() } diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 1883df33b81..02007335830 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -1,6 +1,7 @@ use crate::core::{Dependency, PackageId, SourceId}; use crate::util::interning::InternedString; use crate::util::CargoResult; +use crate::util::PartialVersion; use anyhow::bail; use semver::Version; use std::collections::{BTreeMap, HashMap, HashSet}; @@ -25,7 +26,7 @@ struct Inner { features: Rc, checksum: Option, links: Option, - rust_version: Option, + rust_version: Option, } impl Summary { @@ -34,7 +35,7 @@ impl Summary { dependencies: Vec, features: &BTreeMap>, links: Option>, - rust_version: Option>, + rust_version: Option, ) -> CargoResult { // ****CAUTION**** If you change anything here that may raise a new // error, be sure to coordinate that change with either the index @@ -56,7 +57,7 @@ impl Summary { features: Rc::new(feature_map), checksum: None, links: links.map(|l| l.into()), - rust_version: rust_version.map(|l| l.into()), + rust_version, }), }) } @@ -87,7 +88,7 @@ impl Summary { self.inner.links } - pub fn rust_version(&self) -> Option { + pub fn rust_version(&self) -> Option { self.inner.rust_version } diff --git a/src/cargo/ops/cargo_add/mod.rs b/src/cargo/ops/cargo_add/mod.rs index 1f812ecbeda..d7292f3c328 100644 --- a/src/cargo/ops/cargo_add/mod.rs +++ b/src/cargo/ops/cargo_add/mod.rs @@ -568,11 +568,7 @@ fn get_latest_dependency( })?; if config.cli_unstable().msrv_policy && honor_rust_version { - fn parse_msrv(rust_version: impl AsRef) -> (u64, u64, u64) { - let comp = rust_version - .as_ref() - .parse::() - .expect("validated on parsing of manifest"); + fn parse_msrv(comp: PartialVersion) -> (u64, u64, u64) { (comp.major, comp.minor.unwrap_or(0), comp.patch.unwrap_or(0)) } @@ -632,7 +628,7 @@ fn get_latest_dependency( fn rust_version_incompat_error( dep: &str, - rust_version: &str, + rust_version: PartialVersion, lowest_rust_version: Option<&Summary>, ) -> anyhow::Error { let mut error_msg = format!( diff --git a/src/cargo/ops/cargo_compile/mod.rs b/src/cargo/ops/cargo_compile/mod.rs index 1247ceda700..12350dc47e2 100644 --- a/src/cargo/ops/cargo_compile/mod.rs +++ b/src/cargo/ops/cargo_compile/mod.rs @@ -492,7 +492,7 @@ pub fn create_bcx<'a, 'cfg>( None => continue, }; - let req = semver::VersionReq::parse(version).unwrap(); + let req = version.caret_req(); if req.matches(&untagged_version) { continue; } diff --git a/src/cargo/ops/registry/publish.rs b/src/cargo/ops/registry/publish.rs index 40ca9fd16f5..776e3813e47 100644 --- a/src/cargo/ops/registry/publish.rs +++ b/src/cargo/ops/registry/publish.rs @@ -372,6 +372,7 @@ fn transmit( ref links, ref rust_version, } = *manifest.metadata(); + let rust_version = rust_version.as_ref().map(ToString::to_string); let readme_content = readme .as_ref() .map(|readme| { @@ -424,7 +425,7 @@ fn transmit( license_file: license_file.clone(), badges: badges.clone(), links: links.clone(), - rust_version: rust_version.clone(), + rust_version, }, tarball, ) diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index 05bfe71af05..1632739c848 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -91,7 +91,9 @@ use crate::core::{PackageId, SourceId, Summary}; use crate::sources::registry::{LoadResponse, RegistryData}; use crate::util::interning::InternedString; use crate::util::IntoUrl; -use crate::util::{internal, CargoResult, Config, Filesystem, OptVersionReq, ToSemver}; +use crate::util::{ + internal, CargoResult, Config, Filesystem, OptVersionReq, PartialVersion, ToSemver, +}; use anyhow::bail; use cargo_util::{paths, registry::make_dep_path}; use semver::Version; @@ -305,7 +307,7 @@ pub struct IndexPackage<'a> { /// /// Added in 2023 (see ), /// can be `None` if published before then or if not set in the manifest. - rust_version: Option, + rust_version: Option, /// The schema version for this entry. /// /// If this is None, it defaults to version `1`. Entries with unknown diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs index 2a210601b46..104bc2363f4 100644 --- a/src/cargo/util/semver_ext.rs +++ b/src/cargo/util/semver_ext.rs @@ -108,13 +108,27 @@ impl From for OptVersionReq { } } -#[derive(PartialEq, Eq, Hash, Clone, Debug)] +#[derive(PartialEq, Eq, Hash, Copy, Clone, Debug)] pub struct PartialVersion { pub major: u64, pub minor: Option, pub patch: Option, } +impl PartialVersion { + pub fn caret_req(&self) -> VersionReq { + VersionReq { + comparators: vec![Comparator { + op: semver::Op::Caret, + major: self.major, + minor: self.minor, + patch: self.patch, + pre: Default::default(), + }], + } + } +} + impl std::str::FromStr for PartialVersion { type Err = anyhow::Error; @@ -125,7 +139,7 @@ impl std::str::FromStr for PartialVersion { let version_req = match semver::VersionReq::parse(value) { // Exclude semver operators like `^` and pre-release identifiers Ok(req) if value.chars().all(|c| c.is_ascii_digit() || c == '.') => req, - _ => anyhow::bail!("`rust-version` must be a value like \"1.32\""), + _ => anyhow::bail!("expected a version like \"1.32\""), }; assert_eq!( version_req.comparators.len(), @@ -161,6 +175,42 @@ impl Display for PartialVersion { } } +impl serde::Serialize for PartialVersion { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.collect_str(self) + } +} + +impl<'de> serde::Deserialize<'de> for PartialVersion { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + struct VersionVisitor; + + impl<'de> serde::de::Visitor<'de> for VersionVisitor { + type Value = PartialVersion; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("SemVer version") + } + + fn visit_str(self, string: &str) -> Result + where + E: serde::de::Error, + { + string.parse().map_err(serde::de::Error::custom) + } + } + + let s = String::deserialize(deserializer)?; + s.parse().map_err(serde::de::Error::custom) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 2f88478a059..0036e54f98a 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -30,7 +30,8 @@ use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY}; use crate::util::errors::{CargoResult, ManifestError}; use crate::util::interning::InternedString; use crate::util::{ - self, config::ConfigRelativePath, validate_package_name, Config, IntoUrl, VersionReqExt, + self, config::ConfigRelativePath, validate_package_name, Config, IntoUrl, PartialVersion, + VersionReqExt, }; pub mod embedded; @@ -1059,7 +1060,7 @@ pub trait WorkspaceInherit { } /// An enum that allows for inheriting keys from a workspace in a Cargo.toml. -#[derive(Serialize, Clone, Debug)] +#[derive(Serialize, Copy, Clone, Debug)] #[serde(untagged)] pub enum MaybeWorkspace { /// The "defined" type, or the type that that is used when not inheriting from a workspace. @@ -1278,6 +1279,42 @@ impl<'de> de::Deserialize<'de> for MaybeWorkspaceString { } } +type MaybeWorkspacePartialVersion = MaybeWorkspace; +impl<'de> de::Deserialize<'de> for MaybeWorkspacePartialVersion { + fn deserialize(d: D) -> Result + where + D: de::Deserializer<'de>, + { + struct Visitor; + + impl<'de> de::Visitor<'de> for Visitor { + type Value = MaybeWorkspacePartialVersion; + + fn expecting(&self, f: &mut fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { + f.write_str("a semver or workspace") + } + + fn visit_string(self, value: String) -> Result + where + E: de::Error, + { + let value = value.parse::().map_err(|e| E::custom(e))?; + Ok(MaybeWorkspacePartialVersion::Defined(value)) + } + + fn visit_map(self, map: V) -> Result + where + V: de::MapAccess<'de>, + { + let mvd = de::value::MapAccessDeserializer::new(map); + TomlWorkspaceField::deserialize(mvd).map(MaybeWorkspace::Workspace) + } + } + + d.deserialize_any(Visitor) + } +} + type MaybeWorkspaceVecString = MaybeWorkspace, TomlWorkspaceField>; impl<'de> de::Deserialize<'de> for MaybeWorkspaceVecString { fn deserialize(d: D) -> Result @@ -1448,7 +1485,7 @@ impl<'de> de::Deserialize<'de> for MaybeWorkspaceLints { } } -#[derive(Deserialize, Serialize, Clone, Debug)] +#[derive(Deserialize, Serialize, Copy, Clone, Debug)] pub struct TomlWorkspaceField { #[serde(deserialize_with = "bool_no_false")] workspace: bool, @@ -1483,7 +1520,7 @@ impl WorkspaceInherit for TomlWorkspaceField { #[serde(rename_all = "kebab-case")] pub struct TomlPackage { edition: Option, - rust_version: Option, + rust_version: Option, name: InternedString, #[serde(deserialize_with = "version_trim_whitespace")] version: MaybeWorkspaceSemverVersion, @@ -1573,7 +1610,7 @@ pub struct InheritableFields { exclude: Option>, include: Option>, #[serde(rename = "rust-version")] - rust_version: Option, + rust_version: Option, // We use skip here since it will never be present when deserializing // and we don't want it present when serializing #[serde(skip)] @@ -1613,7 +1650,7 @@ impl InheritableFields { ("package.license", license -> String), ("package.publish", publish -> VecStringOrBool), ("package.repository", repository -> String), - ("package.rust-version", rust_version -> String), + ("package.rust-version", rust_version -> PartialVersion), ("package.version", version -> semver::Version), } @@ -2047,14 +2084,9 @@ impl TomlManifest { } let rust_version = if let Some(rust_version) = &package.rust_version { - let rust_version = rust_version - .clone() - .resolve("rust_version", || inherit()?.rust_version())?; - let req = match semver::VersionReq::parse(&rust_version) { - // Exclude semver operators like `^` and pre-release identifiers - Ok(req) if rust_version.chars().all(|c| c.is_ascii_digit() || c == '.') => req, - _ => bail!("`rust-version` must be a value like \"1.32\""), - }; + let rust_version = + rust_version.resolve("rust_version", || inherit()?.rust_version())?; + let req = rust_version.caret_req(); if let Some(first_version) = edition.first_version() { let unsupported = semver::Version::new(first_version.major, first_version.minor - 1, 9999); @@ -2335,7 +2367,7 @@ impl TomlManifest { deps, me.features.as_ref().unwrap_or(&empty_features), package.links.as_deref(), - rust_version.as_deref().map(InternedString::new), + rust_version, )?; let metadata = ManifestMetadata { @@ -2405,7 +2437,6 @@ impl TomlManifest { links: package.links.clone(), rust_version: package .rust_version - .clone() .map(|mw| mw.resolve("rust-version", || inherit()?.rust_version())) .transpose()?, }; diff --git a/tests/testsuite/rust_version.rs b/tests/testsuite/rust_version.rs index 91711cf1ad9..60470c5fb61 100644 --- a/tests/testsuite/rust_version.rs +++ b/tests/testsuite/rust_version.rs @@ -44,8 +44,15 @@ fn rust_version_bad_caret() { .cargo("check") .with_status(101) .with_stderr( - "error: failed to parse manifest at `[..]`\n\n\ - Caused by:\n `rust-version` must be a value like \"1.32\"", + "\ +error: failed to parse manifest at `[..]` + +Caused by: + TOML parse error at line 6, column 28 + | + 6 | rust-version = \"^1.43\" + | ^^^^^^^ + expected a version like \"1.32\"", ) .run(); } @@ -70,8 +77,15 @@ fn rust_version_bad_pre_release() { .cargo("check") .with_status(101) .with_stderr( - "error: failed to parse manifest at `[..]`\n\n\ - Caused by:\n `rust-version` must be a value like \"1.32\"", + "\ +error: failed to parse manifest at `[..]` + +Caused by: + TOML parse error at line 6, column 28 + | + 6 | rust-version = \"1.43-beta.1\" + | ^^^^^^^^^^^^^ + expected a version like \"1.32\"", ) .run(); } @@ -96,8 +110,15 @@ fn rust_version_bad_nonsense() { .cargo("check") .with_status(101) .with_stderr( - "error: failed to parse manifest at `[..]`\n\n\ - Caused by:\n `rust-version` must be a value like \"1.32\"", + "\ +error: failed to parse manifest at `[..]` + +Caused by: + TOML parse error at line 6, column 28 + | + 6 | rust-version = \"foodaddle\" + | ^^^^^^^^^^^ + expected a version like \"1.32\"", ) .run(); } From 1701b4e3d8fdfd136ded94af338465a1c05103ba Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 24 Aug 2023 14:12:53 -0500 Subject: [PATCH 3/3] fix(manifest): Improve rust-version error messages Since we have tests for a couple of cases, I figured we could improve the error messages for them. --- src/cargo/util/semver_ext.rs | 16 ++++++++++++++++ tests/testsuite/rust_version.rs | 4 ++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs index 104bc2363f4..bf52f66a32b 100644 --- a/src/cargo/util/semver_ext.rs +++ b/src/cargo/util/semver_ext.rs @@ -136,9 +136,18 @@ impl std::str::FromStr for PartialVersion { // HACK: `PartialVersion` is a subset of the `VersionReq` syntax that only ever // has one comparator with a required minor and optional patch, and uses no // other features. + if is_req(value) { + anyhow::bail!("unexpected version requirement, expected a version like \"1.32\"") + } let version_req = match semver::VersionReq::parse(value) { // Exclude semver operators like `^` and pre-release identifiers Ok(req) if value.chars().all(|c| c.is_ascii_digit() || c == '.') => req, + Err(_) if value.contains('+') => { + anyhow::bail!("unexpected build field, expected a version like \"1.32\"") + } + Err(_) if value.contains('-') => { + anyhow::bail!("unexpected prerelease field, expected a version like \"1.32\"") + } _ => anyhow::bail!("expected a version like \"1.32\""), }; assert_eq!( @@ -211,6 +220,13 @@ impl<'de> serde::Deserialize<'de> for PartialVersion { } } +fn is_req(value: &str) -> bool { + let Some(first) = value.chars().next() else { + return false; + }; + "<>=^~".contains(first) || value.contains('*') || value.contains(',') +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests/testsuite/rust_version.rs b/tests/testsuite/rust_version.rs index 60470c5fb61..1fce679ecb8 100644 --- a/tests/testsuite/rust_version.rs +++ b/tests/testsuite/rust_version.rs @@ -52,7 +52,7 @@ Caused by: | 6 | rust-version = \"^1.43\" | ^^^^^^^ - expected a version like \"1.32\"", + unexpected version requirement, expected a version like \"1.32\"", ) .run(); } @@ -85,7 +85,7 @@ Caused by: | 6 | rust-version = \"1.43-beta.1\" | ^^^^^^^^^^^^^ - expected a version like \"1.32\"", + unexpected prerelease field, expected a version like \"1.32\"", ) .run(); }