refactor(resolver): Consolidate logic in VersionPreferences

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()