refactor: Parse, don't validate, rust_version

epage · epage · commit 423a3345203d · 2023-08-24T14:15:35.000-05:00
By using the `PartialVersion` type with serde, we get the context for
the error automatically.
diff --git 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::<PartialVersion>,
     )
     .unwrap();
     let opts = ResolveOpts::everything();
@@ -584,7 +584,7 @@ pub fn pkg_dep<T: ToPkgId>(name: T, dep: Vec<Dependency>) -> Summary {
         dep,
         &BTreeMap::new(),
         link,
-        None::<&String>,
+        None::<PartialVersion>,
     )
     .unwrap()
 }
@@ -612,7 +612,7 @@ pub fn pkg_loc(name: &str, loc: &str) -> Summary {
         Vec::new(),
         &BTreeMap::new(),
         link,
-        None::<&String>,
+        None::<PartialVersion>,
     )
     .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::<PartialVersion>,
     )
     .unwrap()
 }
diff --git 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
@@ -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<TomlManifest>,
     unstable_features: Features,
     edition: Edition,
-    rust_version: Option<String>,
+    rust_version: Option<PartialVersion>,
     im_a_teapot: Option<bool>,
     default_run: Option<String>,
     metabuild: Option<Vec<String>>,
@@ -112,7 +112,7 @@ pub struct ManifestMetadata {
     pub documentation: Option<String>, // URL
     pub badges: BTreeMap<String, BTreeMap<String, String>>,
     pub links: Option<String>,
-    pub rust_version: Option<String>,
+    pub rust_version: Option<PartialVersion>,
 }
 
 #[derive(Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
@@ -401,7 +401,7 @@ impl Manifest {
         workspace: WorkspaceConfig,
         unstable_features: Features,
         edition: Edition,
-        rust_version: Option<String>,
+        rust_version: Option<PartialVersion>,
         im_a_teapot: Option<bool>,
         default_run: Option<String>,
         original: Rc<TomlManifest>,
@@ -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<PartialVersion> {
+        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
@@ -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<Vec<String>>,
     default_run: Option<String>,
-    rust_version: Option<String>,
+    rust_version: Option<PartialVersion>,
 }
 
 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<PartialVersion> {
         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
@@ -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::<PartialVersion>,
         )
         .unwrap()
     }
diff --git 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<FeatureMap>,
     checksum: Option<String>,
     links: Option<InternedString>,
-    rust_version: Option<InternedString>,
+    rust_version: Option<PartialVersion>,
 }
 
 impl Summary {
@@ -34,7 +35,7 @@ impl Summary {
         dependencies: Vec<Dependency>,
         features: &BTreeMap<InternedString, Vec<InternedString>>,
         links: Option<impl Into<InternedString>>,
-        rust_version: Option<impl Into<InternedString>>,
+        rust_version: Option<PartialVersion>,
     ) -> CargoResult<Summary> {
         // ****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<InternedString> {
+    pub fn rust_version(&self) -> Option<PartialVersion> {
         self.inner.rust_version
     }
 
diff --git 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<str>) -> (u64, u64, u64) {
-                    let comp = rust_version
-                        .as_ref()
-                        .parse::<PartialVersion>()
-                        .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
@@ -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
@@ -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
@@ -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 <https://github.com/rust-lang/crates.io/pull/6267>),
     /// can be `None` if published before then or if not set in the manifest.
-    rust_version: Option<InternedString>,
+    rust_version: Option<PartialVersion>,
     /// 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
@@ -108,13 +108,27 @@ impl From<VersionReq> for OptVersionReq {
     }
 }
 
-#[derive(PartialEq, Eq, Hash, Clone, Debug)]
+#[derive(PartialEq, Eq, Hash, Copy, Clone, Debug)]
 pub struct PartialVersion {
     pub major: u64,
     pub minor: Option<u64>,
     pub patch: Option<u64>,
 }
 
+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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        serializer.collect_str(self)
+    }
+}
+
+impl<'de> serde::Deserialize<'de> for PartialVersion {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    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<E>(self, string: &str) -> Result<Self::Value, E>
+            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
diff --git a/tests/testsuite/rust_version.rs b/tests/testsuite/rust_version.rs

Original file line number	Diff line number	Diff line change
`@@ -31,6 +31,7 @@ use crate::util::network::http::http_handle_and_timeout;`
`31`	`31`	`use crate::util::network::http::HttpTimeout;`
`32`	`32`	`use crate::util::network::retry::{Retry, RetryResult};`
`33`	`33`	`use crate::util::network::sleep::SleepTracker;`
	`34`	`+use crate::util::PartialVersion;`
`34`	`35`	`use crate::util::{self, internal, Config, Progress, ProgressStyle};`
`35`	`36`
`36`	`37`	`pub const MANIFEST_PREAMBLE: &str = "\`
`@@ -103,7 +104,7 @@ pub struct SerializedPackage {`
`103`	`104`	`#[serde(skip_serializing_if = "Option::is_none")]`
`104`	`105`	`metabuild: Option<Vec<String>>,`
`105`	`106`	`default_run: Option<String>,`
`106`		`- rust_version: Option<String>,`
	`107`	`+ rust_version: Option<PartialVersion>,`
`107`	`108`	`}`
`108`	`109`
`109`	`110`	`impl Package {`
`@@ -177,7 +178,7 @@ impl Package {`
`177`	`178`	`self.targets().iter().any(\|target\| target.proc_macro())`
`178`	`179`	`}`
`179`	`180`	`/// Gets the package's minimum Rust version.`
`180`		`- pub fn rust_version(&self) -> Option<&str> {`
	`181`	`+ pub fn rust_version(&self) -> Option<PartialVersion> {`
`181`	`182`	`self.manifest().rust_version()`
`182`	`183`	`}`
`183`	`184`
`@@ -262,7 +263,7 @@ impl Package {`
`262`	`263`	`metabuild: self.manifest().metabuild().cloned(),`
`263`	`264`	`publish: self.publish().as_ref().cloned(),`
`264`	`265`	`default_run: self.manifest().default_run().map(\|s\| s.to_owned()),`
`265`		`- rust_version: self.rust_version().map(\|s\| s.to_owned()),`
	`266`	`+ rust_version: self.rust_version(),`
`266`	`267`	`}`
`267`	`268`	`}`
`268`	`269`	`}`
Original file line number	Diff line number	Diff line change
`@@ -492,7 +492,7 @@ pub fn create_bcx<'a, 'cfg>(`
`492`	`492`	`None => continue,`
`493`	`493`	`};`
`494`	`494`
`495`		`- let req = semver::VersionReq::parse(version).unwrap();`
	`495`	`+ let req = version.caret_req();`
`496`	`496`	`if req.matches(&untagged_version) {`
`497`	`497`	`continue;`
`498`	`498`	`}`