Auto merge of #13537 - epage:msrv-compat, r=Eh2406

fix: Consistently compare MSRVs

### What does this PR try to resolve?

Currently, we use several strategies to evaluate an MSRV
- Relying in `impl Ord for RustVersion` (`dep_msrv <= pkg_msrv`)
- Converting to version requirements
  - Decrementing a version

This consolidates around one strategy: `RustVersion::is_compatible_with`
- Ensure the comparisons have the same behavior
- Centralize knowledge of how to handle pre-release rustc
- Losslessly allow comparing with either rustc or workspace msrv

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

Refactors are split out

I didn't go through and verify if or how the different approaches varied in behavior, instead consolidating on the one, so only unit tests around the consolidated behavior were added rather than trying to hit all of the corner cases within the various ways `RustVersion` is used.

### Additional information

Author: bors

crates/cargo-util-schemas/src/manifest.rs renamed to crates/cargo-util-schemas/src/manifest/mod.rs

@@ -16,11 +16,13 @@ use serde::{Deserialize, Serialize};
 use serde_untagged::UntaggedEnumVisitor;
 
 use crate::core::PackageIdSpec;
-use crate::core::PartialVersion;
-use crate::core::PartialVersionError;
 use crate::restricted_names;
 
+mod rust_version;
+
 pub use crate::restricted_names::NameValidationError;
+pub use rust_version::RustVersion;
+pub use rust_version::RustVersionError;
 
 /// This type is used to deserialize `Cargo.toml` files.
 #[derive(Debug, Deserialize, Serialize)]
@@ -1399,79 +1401,6 @@ pub enum TomlLintLevel {
     Allow,
 }
 
-#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Debug, serde::Serialize)]
-#[serde(transparent)]
-pub struct RustVersion(PartialVersion);
-
-impl std::ops::Deref for RustVersion {
-    type Target = PartialVersion;
-
-    fn deref(&self) -> &Self::Target {
-        &self.0
-    }
-}
-
-impl std::str::FromStr for RustVersion {
-    type Err = RustVersionError;
-
-    fn from_str(value: &str) -> Result<Self, Self::Err> {
-        let partial = value.parse::<PartialVersion>();
-        let partial = partial.map_err(RustVersionErrorKind::PartialVersion)?;
-        partial.try_into()
-    }
-}
-
-impl TryFrom<PartialVersion> for RustVersion {
-    type Error = RustVersionError;
-
-    fn try_from(partial: PartialVersion) -> Result<Self, Self::Error> {
-        if partial.pre.is_some() {
-            return Err(RustVersionErrorKind::Prerelease.into());
-        }
-        if partial.build.is_some() {
-            return Err(RustVersionErrorKind::BuildMetadata.into());
-        }
-        Ok(Self(partial))
-    }
-}
-
-impl<'de> serde::Deserialize<'de> for RustVersion {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: serde::Deserializer<'de>,
-    {
-        UntaggedEnumVisitor::new()
-            .expecting("SemVer version")
-            .string(|value| value.parse().map_err(serde::de::Error::custom))
-            .deserialize(deserializer)
-    }
-}
-
-impl Display for RustVersion {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.0.fmt(f)
-    }
-}
-
-/// Error parsing a [`RustVersion`].
-#[derive(Debug, thiserror::Error)]
-#[error(transparent)]
-pub struct RustVersionError(#[from] RustVersionErrorKind);
-
-/// Non-public error kind for [`RustVersionError`].
-#[non_exhaustive]
-#[derive(Debug, thiserror::Error)]
-enum RustVersionErrorKind {
-    #[error("unexpected prerelease field, expected a version like \"1.32\"")]
-    Prerelease,
-
-    #[error("unexpected build field, expected a version like \"1.32\"")]
-    BuildMetadata,
-
-    #[error(transparent)]
-    PartialVersion(#[from] PartialVersionError),
-}
-
 #[derive(Copy, Clone, Debug)]
 pub struct InvalidCargoFeatures {}
 

crates/cargo-util-schemas/src/manifest/rust_version.rs

@@ -0,0 +1,173 @@
+use std::fmt;
+use std::fmt::Display;
+
+use serde_untagged::UntaggedEnumVisitor;
+
+use crate::core::PartialVersion;
+use crate::core::PartialVersionError;
+
+#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Debug, serde::Serialize)]
+#[serde(transparent)]
+pub struct RustVersion(PartialVersion);
+
+impl RustVersion {
+    pub fn is_compatible_with(&self, rustc: &PartialVersion) -> bool {
+        let msrv = self.0.to_caret_req();
+        // Remove any pre-release identifiers for easier comparison
+        let rustc = semver::Version {
+            major: rustc.major,
+            minor: rustc.minor.unwrap_or_default(),
+            patch: rustc.patch.unwrap_or_default(),
+            pre: Default::default(),
+            build: Default::default(),
+        };
+        msrv.matches(&rustc)
+    }
+
+    pub fn into_partial(self) -> PartialVersion {
+        self.0
+    }
+
+    pub fn as_partial(&self) -> &PartialVersion {
+        &self.0
+    }
+}
+
+impl std::str::FromStr for RustVersion {
+    type Err = RustVersionError;
+
+    fn from_str(value: &str) -> Result<Self, Self::Err> {
+        let partial = value.parse::<PartialVersion>();
+        let partial = partial.map_err(RustVersionErrorKind::PartialVersion)?;
+        partial.try_into()
+    }
+}
+
+impl TryFrom<semver::Version> for RustVersion {
+    type Error = RustVersionError;
+
+    fn try_from(version: semver::Version) -> Result<Self, Self::Error> {
+        let version = PartialVersion::from(version);
+        Self::try_from(version)
+    }
+}
+
+impl TryFrom<PartialVersion> for RustVersion {
+    type Error = RustVersionError;
+
+    fn try_from(partial: PartialVersion) -> Result<Self, Self::Error> {
+        if partial.pre.is_some() {
+            return Err(RustVersionErrorKind::Prerelease.into());
+        }
+        if partial.build.is_some() {
+            return Err(RustVersionErrorKind::BuildMetadata.into());
+        }
+        Ok(Self(partial))
+    }
+}
+
+impl<'de> serde::Deserialize<'de> for RustVersion {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        UntaggedEnumVisitor::new()
+            .expecting("SemVer version")
+            .string(|value| value.parse().map_err(serde::de::Error::custom))
+            .deserialize(deserializer)
+    }
+}
+
+impl Display for RustVersion {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+/// Error parsing a [`RustVersion`].
+#[derive(Debug, thiserror::Error)]
+#[error(transparent)]
+pub struct RustVersionError(#[from] RustVersionErrorKind);
+
+/// Non-public error kind for [`RustVersionError`].
+#[non_exhaustive]
+#[derive(Debug, thiserror::Error)]
+enum RustVersionErrorKind {
+    #[error("unexpected prerelease field, expected a version like \"1.32\"")]
+    Prerelease,
+
+    #[error("unexpected build field, expected a version like \"1.32\"")]
+    BuildMetadata,
+
+    #[error(transparent)]
+    PartialVersion(#[from] PartialVersionError),
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn is_compatible_with_rustc() {
+        let cases = &[
+            ("1", "1.70.0", true),
+            ("1.30", "1.70.0", true),
+            ("1.30.10", "1.70.0", true),
+            ("1.70", "1.70.0", true),
+            ("1.70.0", "1.70.0", true),
+            ("1.70.1", "1.70.0", false),
+            ("1.70", "1.70.0-nightly", true),
+            ("1.70.0", "1.70.0-nightly", true),
+            ("1.71", "1.70.0", false),
+            ("2", "1.70.0", false),
+        ];
+        let mut passed = true;
+        for (msrv, rustc, expected) in cases {
+            let msrv: RustVersion = msrv.parse().unwrap();
+            let rustc = PartialVersion::from(semver::Version::parse(rustc).unwrap());
+            if msrv.is_compatible_with(&rustc) != *expected {
+                println!("failed: {msrv} is_compatible_with {rustc} == {expected}");
+                passed = false;
+            }
+        }
+        assert!(passed);
+    }
+
+    #[test]
+    fn is_compatible_with_workspace_msrv() {
+        let cases = &[
+            ("1", "1", true),
+            ("1", "1.70", true),
+            ("1", "1.70.0", true),
+            ("1.30", "1", false),
+            ("1.30", "1.70", true),
+            ("1.30", "1.70.0", true),
+            ("1.30.10", "1", false),
+            ("1.30.10", "1.70", true),
+            ("1.30.10", "1.70.0", true),
+            ("1.70", "1", false),
+            ("1.70", "1.70", true),
+            ("1.70", "1.70.0", true),
+            ("1.70.0", "1", false),
+            ("1.70.0", "1.70", true),
+            ("1.70.0", "1.70.0", true),
+            ("1.70.1", "1", false),
+            ("1.70.1", "1.70", false),
+            ("1.70.1", "1.70.0", false),
+            ("1.71", "1", false),
+            ("1.71", "1.70", false),
+            ("1.71", "1.70.0", false),
+            ("2", "1.70.0", false),
+        ];
+        let mut passed = true;
+        for (dep_msrv, ws_msrv, expected) in cases {
+            let dep_msrv: RustVersion = dep_msrv.parse().unwrap();
+            let ws_msrv = ws_msrv.parse::<RustVersion>().unwrap().into_partial();
+            if dep_msrv.is_compatible_with(&ws_msrv) != *expected {
+                println!("failed: {dep_msrv} is_compatible_with {ws_msrv} == {expected}");
+                passed = false;
+            }
+        }
+        assert!(passed);
+    }
+}

src/cargo/core/resolver/version_prefs.rs

@@ -4,7 +4,7 @@
 use std::cmp::Ordering;
 use std::collections::{HashMap, HashSet};
 
-use cargo_util_schemas::manifest::RustVersion;
+use cargo_util_schemas::core::PartialVersion;
 
 use crate::core::{Dependency, PackageId, Summary};
 use crate::util::interning::InternedString;
@@ -21,7 +21,7 @@ pub struct VersionPreferences {
     try_to_use: HashSet<PackageId>,
     prefer_patch_deps: HashMap<InternedString, HashSet<Dependency>>,
     version_ordering: VersionOrdering,
-    max_rust_version: Option<RustVersion>,
+    max_rust_version: Option<PartialVersion>,
 }
 
 #[derive(Copy, Clone, Default, PartialEq, Eq, Hash, Debug)]
@@ -49,7 +49,7 @@ impl VersionPreferences {
         self.version_ordering = ordering;
     }
 
-    pub fn max_rust_version(&mut self, ver: Option<RustVersion>) {
+    pub fn max_rust_version(&mut self, ver: Option<PartialVersion>) {
         self.max_rust_version = ver;
     }
 
@@ -92,8 +92,8 @@ impl VersionPreferences {
                     (Some(a), Some(b)) if a == b => {}
                     // Primary comparison
                     (Some(a), Some(b)) => {
-                        let a_is_compat = a <= max_rust_version;
-                        let b_is_compat = b <= max_rust_version;
+                        let a_is_compat = a.is_compatible_with(max_rust_version);
+                        let b_is_compat = b.is_compatible_with(max_rust_version);
                         match (a_is_compat, b_is_compat) {
                             (true, true) => {}   // fallback
                             (false, false) => {} // fallback
@@ -103,14 +103,14 @@ impl VersionPreferences {
                     }
                     // Prioritize `None` over incompatible
                     (None, Some(b)) => {
-                        if b <= max_rust_version {
+                        if b.is_compatible_with(max_rust_version) {
                             return Ordering::Greater;
                         } else {
                             return Ordering::Less;
                         }
                     }
                     (Some(a), None) => {
-                        if a <= max_rust_version {
+                        if a.is_compatible_with(max_rust_version) {
                             return Ordering::Less;
                         } else {
                             return Ordering::Greater;

src/cargo/ops/cargo_add/mod.rs

@@ -619,21 +619,13 @@ fn get_latest_dependency(
                 let (req_msrv, is_msrv) = spec
                     .rust_version()
                     .cloned()
-                    .map(|msrv| CargoResult::Ok((msrv.clone(), true)))
+                    .map(|msrv| CargoResult::Ok((msrv.clone().into_partial(), true)))
                     .unwrap_or_else(|| {
                         let rustc = gctx.load_global_rustc(None)?;
 
                         // Remove any pre-release identifiers for easier comparison
-                        let current_version = &rustc.version;
-                        let untagged_version = RustVersion::try_from(PartialVersion {
-                            major: current_version.major,
-                            minor: Some(current_version.minor),
-                            patch: Some(current_version.patch),
-                            pre: None,
-                            build: None,
-                        })
-                        .unwrap();
-                        Ok((untagged_version, false))
+                        let rustc_version = rustc.version.clone().into();
+                        Ok((rustc_version, false))
                     })?;
 
                 let msrvs = possibilities
@@ -702,11 +694,16 @@ ignoring {dependency}@{latest_version} (which requires rustc {latest_rust_versio
 /// - `msrvs` is sorted by version
 fn latest_compatible<'s>(
     msrvs: &[(&'s Summary, Option<&RustVersion>)],
-    req_msrv: &RustVersion,
+    pkg_msrv: &PartialVersion,
 ) -> Option<&'s Summary> {
     msrvs
         .iter()
-        .filter(|(_, v)| v.as_ref().map(|msrv| req_msrv >= *msrv).unwrap_or(true))
+        .filter(|(_, dep_msrv)| {
+            dep_msrv
+                .as_ref()
+                .map(|dep_msrv| dep_msrv.is_compatible_with(pkg_msrv))
+                .unwrap_or(true)
+        })
         .map(|(s, _)| s)
         .last()
         .copied()