fix: Consistently compare MSRVs

We used several strategies
- Relying in `impl Ord for RustVersion`
- 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

Author: epage

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

@@ -10,10 +10,25 @@ use crate::core::PartialVersionError;
 #[serde(transparent)]
 pub struct RustVersion(PartialVersion);
 
-impl std::ops::Deref for RustVersion {
-    type Target = 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
+    }
 
-    fn deref(&self) -> &Self::Target {
+    pub fn as_partial(&self) -> &PartialVersion {
         &self.0
     }
 }
@@ -28,6 +43,15 @@ impl std::str::FromStr for RustVersion {
     }
 }
 
+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;
 
@@ -78,3 +102,72 @@ enum RustVersionErrorKind {
     #[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,14 +694,14 @@ ignoring {dependency}@{latest_version} (which requires rustc {latest_rust_versio
 /// - `msrvs` is sorted by version
 fn latest_compatible<'s>(
     msrvs: &[(&'s Summary, Option<&RustVersion>)],
-    pkg_msrv: &RustVersion,
+    pkg_msrv: &PartialVersion,
 ) -> Option<&'s Summary> {
     msrvs
         .iter()
         .filter(|(_, dep_msrv)| {
             dep_msrv
                 .as_ref()
-                .map(|dep_msrv| pkg_msrv >= *dep_msrv)
+                .map(|dep_msrv| dep_msrv.is_compatible_with(pkg_msrv))
                 .unwrap_or(true)
         })
         .map(|(s, _)| s)

src/cargo/ops/cargo_compile/mod.rs

@@ -480,13 +480,7 @@ pub fn create_bcx<'a, 'gctx>(
     }
 
     if honor_rust_version {
-        // Remove any pre-release identifiers for easier comparison
-        let rustc_version = &target_data.rustc.version;
-        let rustc_version_untagged = semver::Version::new(
-            rustc_version.major,
-            rustc_version.minor,
-            rustc_version.patch,
-        );
+        let rustc_version = target_data.rustc.version.clone().into();
 
         let mut incompatible = Vec::new();
         let mut local_incompatible = false;
@@ -495,8 +489,7 @@ pub fn create_bcx<'a, 'gctx>(
                 continue;
             };
 
-            let pkg_msrv_req = pkg_msrv.to_caret_req();
-            if pkg_msrv_req.matches(&rustc_version_untagged) {
+            if pkg_msrv.is_compatible_with(&rustc_version) {
                 continue;
             }
 

src/cargo/ops/cargo_install.rs

@@ -15,6 +15,7 @@ use crate::{drop_println, ops};
 
 use anyhow::{bail, Context as _};
 use cargo_util::paths;
+use cargo_util_schemas::core::PartialVersion;
 use itertools::Itertools;
 use semver::VersionReq;
 use tempfile::Builder as TempFileBuilder;
@@ -66,7 +67,7 @@ impl<'gctx> InstallablePackage<'gctx> {
         force: bool,
         no_track: bool,
         needs_update_if_source_is_index: bool,
-        current_rust_version: Option<&semver::Version>,
+        current_rust_version: Option<&PartialVersion>,
     ) -> CargoResult<Option<Self>> {
         if let Some(name) = krate {
             if name == "." {
@@ -625,15 +626,7 @@ pub fn install(
 
     let current_rust_version = if opts.honor_rust_version {
         let rustc = gctx.load_global_rustc(None)?;
-
-        // Remove any pre-release identifiers for easier comparison
-        let current_version = &rustc.version;
-        let untagged_version = semver::Version::new(
-            current_version.major,
-            current_version.minor,
-            current_version.patch,
-        );
-        Some(untagged_version)
+        Some(rustc.version.clone().into())
     } else {
         None
     };

src/cargo/ops/common_for_install_and_uninstall.rs

@@ -8,6 +8,7 @@ use std::task::Poll;
 
 use anyhow::{bail, format_err, Context as _};
 use cargo_util::paths;
+use cargo_util_schemas::core::PartialVersion;
 use ops::FilterRule;
 use serde::{Deserialize, Serialize};
 
@@ -569,7 +570,7 @@ pub fn select_dep_pkg<T>(
     dep: Dependency,
     gctx: &GlobalContext,
     needs_update: bool,
-    current_rust_version: Option<&semver::Version>,
+    current_rust_version: Option<&PartialVersion>,
 ) -> CargoResult<Package>
 where
     T: Source,
@@ -596,8 +597,7 @@ where
     {
         Some(summary) => {
             if let (Some(current), Some(msrv)) = (current_rust_version, summary.rust_version()) {
-                let msrv_req = msrv.to_caret_req();
-                if !msrv_req.matches(current) {
+                if !msrv.is_compatible_with(current) {
                     let name = summary.name();
                     let ver = summary.version();
                     let extra = if dep.source_id().is_registry() {
@@ -616,7 +616,7 @@ where
                             .filter(|summary| {
                                 summary
                                     .rust_version()
-                                    .map(|msrv| msrv.to_caret_req().matches(current))
+                                    .map(|msrv| msrv.is_compatible_with(current))
                                     .unwrap_or(true)
                             })
                             .max_by_key(|s| s.package_id())
@@ -689,7 +689,7 @@ pub fn select_pkg<T, F>(
     dep: Option<Dependency>,
     mut list_all: F,
     gctx: &GlobalContext,
-    current_rust_version: Option<&semver::Version>,
+    current_rust_version: Option<&PartialVersion>,
 ) -> CargoResult<Package>
 where
     T: Source,

src/cargo/ops/resolve.rs

@@ -73,6 +73,7 @@ use crate::util::cache_lock::CacheLockMode;
 use crate::util::errors::CargoResult;
 use crate::util::CanonicalUrl;
 use anyhow::Context as _;
+use cargo_util_schemas::manifest::RustVersion;
 use std::collections::{HashMap, HashSet};
 use tracing::{debug, trace};
 
@@ -318,7 +319,7 @@ pub fn resolve_with_previous<'gctx>(
         version_prefs.version_ordering(VersionOrdering::MinimumVersionsFirst)
     }
     if ws.gctx().cli_unstable().msrv_policy {
-        version_prefs.max_rust_version(ws.rust_version().cloned());
+        version_prefs.max_rust_version(ws.rust_version().cloned().map(RustVersion::into_partial));
     }
 
     // This is a set of PackageIds of `[patch]` entries, and some related locked PackageIds, for

src/cargo/util/toml/mod.rs

@@ -9,7 +9,6 @@ use crate::AlreadyPrintedError;
 use anyhow::{anyhow, bail, Context as _};
 use cargo_platform::Platform;
 use cargo_util::paths;
-use cargo_util_schemas::core::PartialVersion;
 use cargo_util_schemas::manifest;
 use cargo_util_schemas::manifest::RustVersion;
 use itertools::Itertools;
@@ -581,11 +580,9 @@ pub fn to_real_manifest(
             .with_context(|| "failed to parse the `edition` key")?;
         package.edition = Some(manifest::InheritableField::Value(edition.to_string()));
         if let Some(pkg_msrv) = &rust_version {
-            let pkg_msrv_req = pkg_msrv.to_caret_req();
             if let Some(edition_msrv) = edition.first_version() {
-                let unsupported =
-                    semver::Version::new(edition_msrv.major, edition_msrv.minor - 1, 9999);
-                if pkg_msrv_req.matches(&unsupported) {
+                let edition_msrv = RustVersion::try_from(edition_msrv).unwrap();
+                if !edition_msrv.is_compatible_with(pkg_msrv.as_partial()) {
                     bail!(
                         "rust-version {} is older than first version ({}) required by \
                             the specified edition ({})",
@@ -603,9 +600,9 @@ pub fn to_real_manifest(
                 .iter()
                 .filter(|e| {
                     e.first_version()
-                        .map(|edition_msrv| {
-                            let edition_msrv = PartialVersion::from(edition_msrv);
-                            edition_msrv <= **pkg_msrv
+                        .map(|e| {
+                            let e = RustVersion::try_from(e).unwrap();
+                            e.is_compatible_with(pkg_msrv.as_partial())
                         })
                         .unwrap_or_default()
                 })