feat: remove Hash req on DependencyProvider

examples/caching_dependency_provider.rs

@@ -11,7 +11,7 @@ use std::hash::Hash;
 
 // An example implementing caching dependency provider that will
 // store queried dependencies in memory and check them before querying more from remote.
-struct CachingDependencyProvider<P: Package, V: Version + Hash, DP: DependencyProvider<P, V>> {
+struct CachingDependencyProvider<P: Package, V: Version, DP: DependencyProvider<P, V>> {
     remote_dependencies: DP,
     cached_dependencies: RefCell<OfflineDependencyProvider<P, V>>,
 }
@@ -27,7 +27,7 @@ impl<P: Package, V: Version + Hash, DP: DependencyProvider<P, V>>
     }
 }
 
-impl<P: Package, V: Version + Hash, DP: DependencyProvider<P, V>> DependencyProvider<P, V>
+impl<P: Package, V: Version, DP: DependencyProvider<P, V>> DependencyProvider<P, V>
     for CachingDependencyProvider<P, V, DP>
 {
     // Lists only from remote for simplicity

src/solver.rs

@@ -65,9 +65,8 @@
 //! to satisfy the dependencies of that package and version pair.
 //! If there is no solution, the reason will be provided as clear as possible.
 
-use std::collections::BTreeSet as Set;
+use std::collections::{BTreeMap, BTreeSet as Set};
 use std::error::Error;
-use std::hash::Hash;
 
 use crate::error::PubGrubError;
 use crate::internal::core::State;
@@ -230,11 +229,11 @@ pub trait DependencyProvider<P: Package, V: Version> {
 #[derive(Debug, Clone)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[cfg_attr(feature = "serde", serde(transparent))]
-pub struct OfflineDependencyProvider<P: Package, V: Version + Hash> {
-    dependencies: Map<P, Map<V, Map<P, Range<V>>>>,
+pub struct OfflineDependencyProvider<P: Package, V: Version> {
+    dependencies: Map<P, BTreeMap<V, Map<P, Range<V>>>>,
 }
 
-impl<P: Package, V: Version + Hash> OfflineDependencyProvider<P, V> {
+impl<P: Package, V: Version> OfflineDependencyProvider<P, V> {
     /// Creates an empty OfflineDependencyProvider with no dependencies.
     pub fn new() -> Self {
         Self {
@@ -273,12 +272,10 @@ impl<P: Package, V: Version + Hash> OfflineDependencyProvider<P, V> {
         self.dependencies.keys()
     }
 
-    /// Lists versions of saved packages.
+    /// Lists versions of saved packages in sorted order.
     /// Returns [None] if no information is available regarding that package.
-    pub fn versions(&self, package: &P) -> Option<Set<V>> {
-        self.dependencies
-            .get(package)
-            .map(|k| k.keys().cloned().collect())
+    pub fn versions(&self, package: &P) -> Option<impl Iterator<Item = &V>> {
+        self.dependencies.get(package).map(|k| k.keys())
     }
 
     /// Lists dependencies of a given package and version.
@@ -295,7 +292,7 @@ impl<P: Package, V: Version + Hash> OfflineDependencyProvider<P, V> {
 /// contains all dependency information available in memory.
 /// Packages are picked with the fewest versions contained in the constraints first.
 /// Versions are picked with the newest versions first.
-impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OfflineDependencyProvider<P, V> {
+impl<P: Package, V: Version> DependencyProvider<P, V> for OfflineDependencyProvider<P, V> {
     fn make_decision<T: Borrow<P>>(
         &self,
         packages: impl Iterator<Item = (T, impl Constraints<V>)>,
@@ -323,7 +320,8 @@ impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OfflineDependen
             .iter()
             .flat_map(|k| k.keys())
             .filter(|&v| term.contains(v))
-            .max()
+            .rev()
+            .next()
             .cloned();
         Ok((package, ver))
     }

tests/proptest.rs

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: MPL-2.0
 
-use std::{collections::BTreeSet as Set, error::Error, hash::Hash};
+use std::{collections::BTreeSet as Set, error::Error};
 
 use pubgrub::range::Range;
 use pubgrub::solver::{resolve, DependencyProvider, OfflineDependencyProvider};
@@ -21,9 +21,9 @@ use crate::sat_dependency_provider::SatResolve;
 /// The same as [OfflineDependencyProvider] but takes versions from the opposite end:
 /// if [OfflineDependencyProvider] returns versions from newest to oldest, this returns them from oldest to newest.
 #[derive(Clone)]
-struct OldestDependencyProvider<P: Package, V: Version + Hash>(OfflineDependencyProvider<P, V>);
+struct OldestDependencyProvider<P: Package, V: Version>(OfflineDependencyProvider<P, V>);
 
-impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OldestDependencyProvider<P, V> {
+impl<P: Package, V: Version> DependencyProvider<P, V> for OldestDependencyProvider<P, V> {
     fn make_decision<T: std::borrow::Borrow<P>>(
         &self,
         packages: impl Iterator<Item = (T, impl pubgrub::solver::Constraints<V>)>,
@@ -34,8 +34,8 @@ impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OldestDependenc
             let key = self
                 .0
                 .versions(p.borrow())
-                .iter()
-                .flat_map(|k| k.iter())
+                .into_iter()
+                .flatten()
                 .filter(|&v| term.contains(v))
                 .count();
 
@@ -48,10 +48,10 @@ impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OldestDependenc
         let ver = self
             .0
             .versions(package.borrow())
-            .iter()
-            .flat_map(|k| k.iter())
+            .into_iter()
+            .flatten()
             .filter(|&v| term.contains(v))
-            .min()
+            .next()
             .cloned();
         Ok((package, ver))
     }
@@ -388,14 +388,14 @@ proptest! {
         let mut removed_provider = dependency_provider.clone();
         for (package_idx, version_idx, dep_idx) in indexes_to_remove {
             let package = package_idx.get(&packages);
-            let versions: Vec<_> = dependency_provider.versions(package).unwrap().into_iter().collect();
+            let versions: Vec<_> = dependency_provider.versions(package).unwrap().collect();
             let version = version_idx.get(&versions);
             let dependencys: Vec<_> = dependency_provider.get_dependencies(package, version).unwrap().unwrap().into_iter().collect();
             if !dependencys.is_empty() {
                 let dependency = dep_idx.get(&dependencys).0.clone();
                 removed_provider.add_dependencies(
                     **package,
-                    *version,
+                    **version,
                     dependencys.into_iter().filter(|x| x.0 != dependency)
                 )
             }
@@ -423,8 +423,7 @@ proptest! {
                 dependency_provider
                     .versions(&p)
                     .unwrap()
-                    .into_iter()
-                    .map(move |v| (p, v))
+                    .map(move |v| (p, v.clone()))
             })
             .collect();
         let to_remove: Set<(_, _)> = indexes_to_remove.iter().map(|x| x.get(&all_versions)).cloned().collect();

tests/sat_dependency_provider.rs

@@ -4,7 +4,6 @@ use pubgrub::package::Package;
 use pubgrub::solver::{DependencyProvider, OfflineDependencyProvider};
 use pubgrub::type_aliases::Map;
 use pubgrub::version::Version;
-use std::hash::Hash;
 use varisat::ExtendFormula;
 
 const fn num_bits<T>() -> usize {
@@ -52,7 +51,7 @@ pub struct SatResolve<P: Package, V: Version> {
     all_versions_by_p: Map<P, Vec<(V, varisat::Var)>>,
 }
 
-impl<P: Package, V: Version + Hash> SatResolve<P, V> {
+impl<P: Package, V: Version> SatResolve<P, V> {
     pub fn new(dp: &OfflineDependencyProvider<P, V>) -> Self {
         let mut cnf = varisat::CnfFormula::new();
 
@@ -68,7 +67,7 @@ impl<P: Package, V: Version + Hash> SatResolve<P, V> {
                 all_versions_by_p
                     .entry(p.clone())
                     .or_default()
-                    .push((v, new_var));
+                    .push((v.clone(), new_var));
             }
             // no two versions of the same package
             sat_at_most_one(&mut cnf, &versions_for_p);