feat: add a pick_package

src/error.rs

@@ -43,6 +43,11 @@ pub enum PubGrubError<P: Package, V: Version> {
         source: Box<dyn std::error::Error>,
     },
 
+    /// Error arising when the implementer of [DependencyProvider](crate::solver::DependencyProvider)
+    /// returned an error in the method [pick_package](crate::solver::DependencyProvider::pick_package).
+    #[error("Pick package failed")]
+    ErrorPickPackage(Box<dyn std::error::Error>),
+
     /// Error arising when the implementer of [DependencyProvider](crate::solver::DependencyProvider)
     /// returned an error in the method [should_cancel](crate::solver::DependencyProvider::should_cancel).
     #[error("We should cancel")]

src/internal/partial_solution.rs

@@ -3,19 +3,13 @@
 //! The partial solution is the current state
 //! of the solution being built by the algorithm.
 
+use crate::internal::assignment::Assignment::{self, Decision, Derivation};
+use crate::internal::incompatibility::{Incompatibility, Relation};
 use crate::internal::memory::Memory;
 use crate::package::Package;
 use crate::term::Term;
 use crate::type_aliases::Map;
 use crate::version::Version;
-use crate::{
-    error::PubGrubError,
-    internal::incompatibility::{Incompatibility, Relation},
-};
-use crate::{
-    internal::assignment::Assignment::{self, Decision, Derivation},
-    solver::DependencyProvider,
-};
 
 /// The partial solution is the current state
 /// of the solution being built by the algorithm.
@@ -71,6 +65,10 @@ impl<P: Package, V: Version> PartialSolution<P, V> {
         self.memory.extract_solution()
     }
 
+    pub fn term_intersection_for_package(&mut self, package: &P) -> Term<V> {
+        self.memory.term_intersection_for_package(package)
+    }
+
     /// Backtrack the partial solution to a given decision level.
     pub fn backtrack(&mut self, decision_level: usize) {
         // TODO: improve with dichotomic search.
@@ -93,36 +91,13 @@ impl<P: Package, V: Version> PartialSolution<P, V> {
         partial_solution
     }
 
-    /// Heuristic to pick the next package to add to the partial solution.
-    /// This should be a package with a positive derivation but no decision yet.
-    /// If multiple choices are possible, use a heuristic.
-    ///
-    /// Current heuristic employed by this and Pub's implementations is to choose
-    /// the package with the fewest versions matching the outstanding constraint.
-    /// This tends to find conflicts earlier if any exist,
-    /// since these packages will run out of versions to try more quickly.
-    pub fn pick_package(
-        &mut self,
-        dependency_provider: &impl DependencyProvider<P, V>,
-    ) -> Result<Option<(P, Term<V>)>, PubGrubError<P, V>> {
-        let mut out: Option<(P, Term<V>)> = None;
-        let mut min_key = usize::MAX;
-        for (p, term) in self.memory.potential_packages() {
-            let key = dependency_provider
-                .list_available_versions(p)
-                .map_err(|err| PubGrubError::ErrorRetrievingVersions {
-                    package: p.clone(),
-                    source: err,
-                })?
-                .iter()
-                .filter(|&v| term.contains(v))
-                .count();
-            if key < min_key {
-                min_key = key;
-                out = Some((p.clone(), term.clone()));
-            }
+    pub fn potential_packages(&mut self) -> Option<impl Iterator<Item = (&P, &Term<V>)>> {
+        let mut iter = self.memory.potential_packages().peekable();
+        if iter.peek().is_some() {
+            Some(iter)
+        } else {
+            None
         }
-        Ok(out)
     }
 
     /// Pub chooses the latest matching version of the package

src/solver.rs

@@ -75,8 +75,10 @@ use crate::internal::incompatibility::Incompatibility;
 use crate::internal::partial_solution::PartialSolution;
 use crate::package::Package;
 use crate::range::Range;
+use crate::term::Term;
 use crate::type_aliases::Map;
 use crate::version::Version;
+use std::borrow::Borrow;
 
 /// Main function of the library.
 /// Finds a set of packages satisfying dependency bounds for a given package + version pair.
@@ -93,34 +95,39 @@ pub fn resolve<P: Package, V: Version>(
             .should_cancel()
             .map_err(|err| PubGrubError::ErrorShouldCancel(err))?;
 
-        state.unit_propagation(next)?;
+        state.unit_propagation(next.clone())?;
 
-        // Pick the next package.
-        let (p, term) = match state.partial_solution.pick_package(dependency_provider)? {
-            None => {
+        let term = {
+            let iter = state.partial_solution.potential_packages();
+            if iter.is_none() {
+                drop(iter);
                 return state
                     .partial_solution
                     .extract_solution()
                     .ok_or(PubGrubError::Failure(
                         "How did we end up with no package to choose but no solution?".into(),
-                    ))
+                    ));
             }
-            Some(x) => x,
+            next = dependency_provider
+                .pick_package(iter.unwrap())
+                .map_err(PubGrubError::ErrorPickPackage)?
+                .clone();
+            state.partial_solution.term_intersection_for_package(&next)
         };
-        next = p.clone();
+
         let available_versions =
             dependency_provider
-                .list_available_versions(&p)
+                .list_available_versions(&next)
                 .map_err(|err| PubGrubError::ErrorRetrievingVersions {
-                    package: p.clone(),
+                    package: next.clone(),
                     source: err,
                 })?;
 
         // Pick the next compatible version.
         let v = match PartialSolution::<P, V>::pick_version(&available_versions[..], &term) {
             None => {
                 state.add_incompatibility(|id| {
-                    Incompatibility::no_version(id, p.clone(), term.clone())
+                    Incompatibility::no_version(id, next.clone(), term.clone())
                 });
                 continue;
             }
@@ -129,15 +136,15 @@ pub fn resolve<P: Package, V: Version>(
 
         // Retrieve that package dependencies.
         let dependencies = match dependency_provider
-            .get_dependencies(&p, &v)
+            .get_dependencies(&next, &v)
             .map_err(|err| PubGrubError::ErrorRetrievingDependencies {
-                package: p.clone(),
+                package: next.clone(),
                 version: v.clone(),
                 source: err,
             })? {
             None => {
                 state.add_incompatibility(|id| {
-                    Incompatibility::unavailable_dependencies(id, p.clone(), v.clone())
+                    Incompatibility::unavailable_dependencies(id, next.clone(), v.clone())
                 });
                 continue;
             }
@@ -147,9 +154,9 @@ pub fn resolve<P: Package, V: Version>(
         // Add that package and version if the dependencies are not problematic.
         let start_id = state.incompatibility_store.len();
         let dep_incompats =
-            Incompatibility::from_dependencies(start_id, p.clone(), v.clone(), &dependencies);
+            Incompatibility::from_dependencies(start_id, next.clone(), v.clone(), &dependencies);
         if added_dependencies
-            .entry(p.clone())
+            .entry(next.clone())
             .or_default()
             .insert(v.clone())
         {
@@ -167,7 +174,21 @@ pub fn resolve<P: Package, V: Version>(
                 "Root package depends on itself at a different version?".into(),
             ))?;
         }
-        state.partial_solution.add_version(p, v, &dep_incompats);
+        state
+            .partial_solution
+            .add_version(next.clone(), v, &dep_incompats);
+    }
+}
+
+/// A combined view of all the dependencies on a package
+pub trait Constraints<V: Version> {
+    /// Evaluate whether this constraint allows choosing this version.
+    fn contains(&self, version: &V) -> bool;
+}
+
+impl<V: Version> Constraints<V> for &Term<V> {
+    fn contains(&self, version: &V) -> bool {
+        (*self).contains(version)
     }
 }
 
@@ -187,6 +208,30 @@ pub trait DependencyProvider<P: Package, V: Version> {
         version: &V,
     ) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>>;
 
+    /// Chooses which package the [resolve] should decide on next.
+    /// The default implementation picks the package that has the fewest versions contained in the constraints.
+    /// Override to avoid the calls to [list_available_versions], or to use a different heuristic.
+    /// Note: the type `T` ensures that this returns an item from the `packages` argument.
+    fn pick_package<T: Borrow<P>>(
+        &self,
+        packages: impl Iterator<Item = (T, impl Constraints<V>)>,
+    ) -> Result<T, Box<dyn Error>> {
+        let mut out: Option<T> = None;
+        let mut min_key = usize::MAX;
+        for (p, term) in packages {
+            let key = self
+                .list_available_versions(p.borrow())?
+                .iter()
+                .filter(|&v| term.contains(v))
+                .count();
+            if key < min_key {
+                min_key = key;
+                out = Some(p);
+            }
+        }
+        Ok(out.expect("get_dependencies gave us an empty iterator"))
+    }
+
     /// This is called fairly regularly during the resolution,
     /// if it returns an Err then resolution will be terminated.
     /// This is helpful if you want to add some form of early termination like a timeout,
@@ -280,4 +325,27 @@ impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OfflineDependen
     ) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
         Ok(self.dependencies(package, version))
     }
+
+    fn pick_package<T: Borrow<P>>(
+        &self,
+        packages: impl Iterator<Item = (T, impl Constraints<V>)>,
+    ) -> Result<T, Box<dyn Error>> {
+        let mut out: Option<T> = None;
+        let mut min_key = usize::MAX;
+        for (p, term) in packages {
+            let key = self
+                .dependencies
+                .get(p.borrow())
+                .iter()
+                .flat_map(|k| k.keys())
+                .filter(|&v| term.contains(v))
+                .count();
+
+            if key < min_key {
+                min_key = key;
+                out = Some(p);
+            }
+        }
+        Ok(out.expect("get_dependencies gave us an empty iterator"))
+    }
 }