Auto merge of #12930 - epage:msrv-refactor, r=Eh2406

refactor(resolver): Consolidate logic in `VersionPreferences`

### What does this PR try to resolve?

This makes customizing the resolver less intrusive by putting the logic in `VersionPreferences` and making it easy to add new priority cases to it.

In particular, this is prep for tweaking the MSRV resolver to prefer compatible versions, rather than require them.
See https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Alternative.20MSRV-aware.20resolver.20approach.3F

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

Each step is broken down into its own commit for easier browsing

### Additional information

Author: bors

crates/resolver-tests/src/lib.rs

@@ -12,7 +12,7 @@ use std::task::Poll;
 use std::time::Instant;
 
 use cargo::core::dependency::DepKind;
-use cargo::core::resolver::{self, ResolveOpts, VersionPreferences};
+use cargo::core::resolver::{self, ResolveOpts, VersionOrdering, VersionPreferences};
 use cargo::core::Resolve;
 use cargo::core::{Dependency, PackageId, Registry, Summary};
 use cargo::core::{GitReference, SourceId};
@@ -190,15 +190,17 @@ pub fn resolve_with_config_raw(
     .unwrap();
     let opts = ResolveOpts::everything();
     let start = Instant::now();
-    let max_rust_version = None;
+    let mut version_prefs = VersionPreferences::default();
+    if config.cli_unstable().minimal_versions {
+        version_prefs.version_ordering(VersionOrdering::MinimumVersionsFirst)
+    }
     let resolve = resolver::resolve(
         &[(summary, opts)],
         &[],
         &mut registry,
-        &VersionPreferences::default(),
+        &version_prefs,
         Some(config),
         true,
-        max_rust_version,
     );
 
     // The largest test in our suite takes less then 30 sec.
@@ -982,14 +984,17 @@ fn meta_test_multiple_versions_strategy() {
 
 /// Assert `xs` contains `elems`
 #[track_caller]
-pub fn assert_contains<A: PartialEq>(xs: &[A], elems: &[A]) {
+pub fn assert_contains<A: PartialEq + std::fmt::Debug>(xs: &[A], elems: &[A]) {
     for elem in elems {
-        assert!(xs.contains(elem));
+        assert!(
+            xs.contains(elem),
+            "missing element\nset: {xs:?}\nmissing: {elem:?}"
+        );
     }
 }
 
 #[track_caller]
-pub fn assert_same<A: PartialEq>(a: &[A], b: &[A]) {
-    assert_eq!(a.len(), b.len());
+pub fn assert_same<A: PartialEq + std::fmt::Debug>(a: &[A], b: &[A]) {
+    assert_eq!(a.len(), b.len(), "not equal\n{a:?}\n{b:?}");
     assert_contains(b, a);
 }

src/cargo/core/resolver/dep_cache.rs

@@ -20,7 +20,6 @@ use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry,
 use crate::sources::source::QueryKind;
 use crate::util::errors::CargoResult;
 use crate::util::interning::InternedString;
-use crate::util::RustVersion;
 
 use anyhow::Context as _;
 use std::collections::{BTreeSet, HashMap, HashSet};
@@ -32,16 +31,11 @@ pub struct RegistryQueryer<'a> {
     pub registry: &'a mut (dyn Registry + 'a),
     replacements: &'a [(PackageIdSpec, Dependency)],
     version_prefs: &'a VersionPreferences,
-    /// If set the list of dependency candidates will be sorted by minimal
-    /// versions first. That allows `cargo update -Z minimal-versions` which will
-    /// specify minimum dependency versions to be used.
-    minimal_versions: bool,
-    max_rust_version: Option<RustVersion>,
-    /// a cache of `Candidate`s that fulfil a `Dependency` (and whether `first_minimal_version`)
-    registry_cache: HashMap<(Dependency, bool), Poll<Rc<Vec<Summary>>>>,
+    /// a cache of `Candidate`s that fulfil a `Dependency` (and whether `first_version`)
+    registry_cache: HashMap<(Dependency, Option<VersionOrdering>), Poll<Rc<Vec<Summary>>>>,
     /// a cache of `Dependency`s that are required for a `Summary`
     ///
-    /// HACK: `first_minimal_version` is not kept in the cache key is it is 1:1 with
+    /// HACK: `first_version` is not kept in the cache key is it is 1:1 with
     /// `parent.is_none()` (the first element of the cache key) as it doesn't change through
     /// execution.
     summary_cache: HashMap<
@@ -57,15 +51,11 @@ impl<'a> RegistryQueryer<'a> {
         registry: &'a mut dyn Registry,
         replacements: &'a [(PackageIdSpec, Dependency)],
         version_prefs: &'a VersionPreferences,
-        minimal_versions: bool,
-        max_rust_version: Option<&RustVersion>,
     ) -> Self {
         RegistryQueryer {
             registry,
             replacements,
             version_prefs,
-            minimal_versions,
-            max_rust_version: max_rust_version.cloned(),
             registry_cache: HashMap::new(),
             summary_cache: HashMap::new(),
             used_replacements: HashMap::new(),
@@ -106,23 +96,20 @@ impl<'a> RegistryQueryer<'a> {
     pub fn query(
         &mut self,
         dep: &Dependency,
-        first_minimal_version: bool,
+        first_version: Option<VersionOrdering>,
     ) -> Poll<CargoResult<Rc<Vec<Summary>>>> {
-        let registry_cache_key = (dep.clone(), first_minimal_version);
+        let registry_cache_key = (dep.clone(), first_version);
         if let Some(out) = self.registry_cache.get(&registry_cache_key).cloned() {
             return out.map(Result::Ok);
         }
 
         let mut ret = Vec::new();
         let ready = self.registry.query(dep, QueryKind::Exact, &mut |s| {
-            if self.max_rust_version.is_none() || s.rust_version() <= self.max_rust_version.as_ref()
-            {
-                ret.push(s);
-            }
+            ret.push(s);
         })?;
         if ready.is_pending() {
             self.registry_cache
-                .insert((dep.clone(), first_minimal_version), Poll::Pending);
+                .insert((dep.clone(), first_version), Poll::Pending);
             return Poll::Pending;
         }
         for summary in ret.iter() {
@@ -144,7 +131,7 @@ impl<'a> RegistryQueryer<'a> {
                 Poll::Ready(s) => s.into_iter(),
                 Poll::Pending => {
                     self.registry_cache
-                        .insert((dep.clone(), first_minimal_version), Poll::Pending);
+                        .insert((dep.clone(), first_version), Poll::Pending);
                     return Poll::Pending;
                 }
             };
@@ -215,16 +202,8 @@ impl<'a> RegistryQueryer<'a> {
             }
         }
 
-        // When we attempt versions for a package we'll want to do so in a sorted fashion to pick
-        // the "best candidates" first. VersionPreferences implements this notion.
-        let ordering = if first_minimal_version || self.minimal_versions {
-            VersionOrdering::MinimumVersionsFirst
-        } else {
-            VersionOrdering::MaximumVersionsFirst
-        };
-        let first_version = first_minimal_version;
-        self.version_prefs
-            .sort_summaries(&mut ret, ordering, first_version);
+        let first_version = first_version;
+        self.version_prefs.sort_summaries(&mut ret, first_version);
 
         let out = Poll::Ready(Rc::new(ret));
 
@@ -243,7 +222,7 @@ impl<'a> RegistryQueryer<'a> {
         parent: Option<PackageId>,
         candidate: &Summary,
         opts: &ResolveOpts,
-        first_minimal_version: bool,
+        first_version: Option<VersionOrdering>,
     ) -> ActivateResult<Rc<(HashSet<InternedString>, Rc<Vec<DepInfo>>)>> {
         // if we have calculated a result before, then we can just return it,
         // as it is a "pure" query of its arguments.
@@ -263,24 +242,22 @@ impl<'a> RegistryQueryer<'a> {
         let mut all_ready = true;
         let mut deps = deps
             .into_iter()
-            .filter_map(
-                |(dep, features)| match self.query(&dep, first_minimal_version) {
-                    Poll::Ready(Ok(candidates)) => Some(Ok((dep, candidates, features))),
-                    Poll::Pending => {
-                        all_ready = false;
-                        // we can ignore Pending deps, resolve will be repeatedly called
-                        // until there are none to ignore
-                        None
-                    }
-                    Poll::Ready(Err(e)) => Some(Err(e).with_context(|| {
-                        format!(
-                            "failed to get `{}` as a dependency of {}",
-                            dep.package_name(),
-                            describe_path_in_context(cx, &candidate.package_id()),
-                        )
-                    })),
-                },
-            )
+            .filter_map(|(dep, features)| match self.query(&dep, first_version) {
+                Poll::Ready(Ok(candidates)) => Some(Ok((dep, candidates, features))),
+                Poll::Pending => {
+                    all_ready = false;
+                    // we can ignore Pending deps, resolve will be repeatedly called
+                    // until there are none to ignore
+                    None
+                }
+                Poll::Ready(Err(e)) => Some(Err(e).with_context(|| {
+                    format!(
+                        "failed to get `{}` as a dependency of {}",
+                        dep.package_name(),
+                        describe_path_in_context(cx, &candidate.package_id()),
+                    )
+                })),
+            })
             .collect::<CargoResult<Vec<DepInfo>>>()?;
 
         // Attempt to resolve dependencies with fewer candidates before trying

src/cargo/core/resolver/mod.rs

@@ -71,7 +71,6 @@ use crate::util::config::Config;
 use crate::util::errors::CargoResult;
 use crate::util::network::PollExt;
 use crate::util::profile;
-use crate::util::RustVersion;
 
 use self::context::Context;
 use self::dep_cache::RegistryQueryer;
@@ -139,39 +138,18 @@ pub fn resolve(
     version_prefs: &VersionPreferences,
     config: Option<&Config>,
     check_public_visible_dependencies: bool,
-    mut max_rust_version: Option<&RustVersion>,
 ) -> CargoResult<Resolve> {
     let _p = profile::start("resolving");
-    let minimal_versions = match config {
-        Some(config) => config.cli_unstable().minimal_versions,
-        None => false,
-    };
-    let direct_minimal_versions = match config {
-        Some(config) => config.cli_unstable().direct_minimal_versions,
-        None => false,
+    let first_version = match config {
+        Some(config) if config.cli_unstable().direct_minimal_versions => {
+            Some(VersionOrdering::MinimumVersionsFirst)
+        }
+        _ => None,
     };
-    if !config
-        .map(|c| c.cli_unstable().msrv_policy)
-        .unwrap_or(false)
-    {
-        max_rust_version = None;
-    }
-    let mut registry = RegistryQueryer::new(
-        registry,
-        replacements,
-        version_prefs,
-        minimal_versions,
-        max_rust_version,
-    );
+    let mut registry = RegistryQueryer::new(registry, replacements, version_prefs);
     let cx = loop {
         let cx = Context::new(check_public_visible_dependencies);
-        let cx = activate_deps_loop(
-            cx,
-            &mut registry,
-            summaries,
-            direct_minimal_versions,
-            config,
-        )?;
+        let cx = activate_deps_loop(cx, &mut registry, summaries, first_version, config)?;
         if registry.reset_pending() {
             break cx;
         } else {
@@ -223,7 +201,7 @@ fn activate_deps_loop(
     mut cx: Context,
     registry: &mut RegistryQueryer<'_>,
     summaries: &[(Summary, ResolveOpts)],
-    direct_minimal_versions: bool,
+    first_version: Option<VersionOrdering>,
     config: Option<&Config>,
 ) -> CargoResult<Context> {
     let mut backtrack_stack = Vec::new();
@@ -241,7 +219,7 @@ fn activate_deps_loop(
             registry,
             None,
             summary.clone(),
-            direct_minimal_versions,
+            first_version,
             opts,
         );
         match res {
@@ -441,13 +419,13 @@ fn activate_deps_loop(
                 dep.package_name(),
                 candidate.version()
             );
-            let direct_minimal_version = false; // this is an indirect dependency
+            let first_version = None; // this is an indirect dependency
             let res = activate(
                 &mut cx,
                 registry,
                 Some((&parent, &dep)),
                 candidate,
-                direct_minimal_version,
+                first_version,
                 &opts,
             );
 
@@ -659,7 +637,7 @@ fn activate(
     registry: &mut RegistryQueryer<'_>,
     parent: Option<(&Summary, &Dependency)>,
     candidate: Summary,
-    first_minimal_version: bool,
+    first_version: Option<VersionOrdering>,
     opts: &ResolveOpts,
 ) -> ActivateResult<Option<(DepsFrame, Duration)>> {
     let candidate_pid = candidate.package_id();
@@ -716,7 +694,7 @@ fn activate(
         parent.map(|p| p.0.package_id()),
         &candidate,
         opts,
-        first_minimal_version,
+        first_version,
     )?;
 
     // Record what list of features is active for this package.
@@ -905,14 +883,14 @@ fn generalize_conflicting(
         })
     {
         for critical_parents_dep in critical_parents_deps.iter() {
-            // We only want `first_minimal_version=true` for direct dependencies of workspace
+            // We only want `first_version.is_some()` for direct dependencies of workspace
             // members which isn't the case here as this has a `parent`
-            let first_minimal_version = false;
+            let first_version = None;
             // A dep is equivalent to one of the things it can resolve to.
             // Thus, if all the things it can resolve to have already ben determined
             // to be conflicting, then we can just say that we conflict with the parent.
             if let Some(others) = registry
-                .query(critical_parents_dep, first_minimal_version)
+                .query(critical_parents_dep, first_version)
                 .expect("an already used dep now error!?")
                 .expect("an already used dep now pending!?")
                 .iter()