feat!: let DependencyProvider prioritize dependencies (#50)

Author: Eh2406

benches/large_case.rs

@@ -4,7 +4,7 @@ use std::time::Duration;
 extern crate criterion;
 use self::criterion::*;
 
-use pubgrub::solver::{resolve, DependencyProvider, OfflineDependencyProvider};
+use pubgrub::solver::{resolve, OfflineDependencyProvider};
 use pubgrub::version::NumberVersion;
 
 fn bench_nested(c: &mut Criterion) {
@@ -20,10 +20,9 @@ fn bench_nested(c: &mut Criterion) {
                 let s = std::fs::read_to_string(&case).unwrap();
                 let dependency_provider: OfflineDependencyProvider<u16, NumberVersion> =
                     ron::de::from_str(&s).unwrap();
-                let all_versions = dependency_provider.list_available_versions(&0).unwrap();
 
                 b.iter(|| {
-                    for &n in &all_versions {
+                    for &n in dependency_provider.versions(&0).unwrap() {
                         let _ = resolve(&dependency_provider, 0, n);
                     }
                 });

examples/caching_dependency_provider.rs

@@ -2,22 +2,20 @@
 
 use std::cell::RefCell;
 use std::error::Error;
-use std::hash::Hash;
 
 use pubgrub::package::Package;
+use pubgrub::range::Range;
 use pubgrub::solver::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider};
 use pubgrub::version::{NumberVersion, Version};
 
 // 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>>,
 }
 
-impl<P: Package, V: Version + Hash, DP: DependencyProvider<P, V>>
-    CachingDependencyProvider<P, V, DP>
-{
+impl<P: Package, V: Version, DP: DependencyProvider<P, V>> CachingDependencyProvider<P, V, DP> {
     pub fn new(remote_dependencies_provider: DP) -> Self {
         CachingDependencyProvider {
             remote_dependencies: remote_dependencies_provider,
@@ -26,12 +24,14 @@ 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
-    fn list_available_versions(&self, package: &P) -> Result<Vec<V>, Box<dyn Error>> {
-        self.remote_dependencies.list_available_versions(package)
+    fn choose_package_version<T: std::borrow::Borrow<P>, U: std::borrow::Borrow<Range<V>>>(
+        &self,
+        packages: impl Iterator<Item = (T, U)>,
+    ) -> Result<(T, Option<V>), Box<dyn Error>> {
+        self.remote_dependencies.choose_package_version(packages)
     }
 
     // Caches dependencies if they were already queried

src/error.rs

@@ -15,19 +15,6 @@ pub enum PubGrubError<P: Package, V: Version> {
     #[error("No solution")]
     NoSolution(DerivationTree<P, V>),
 
-    /// Error arising when the implementer of
-    /// [DependencyProvider](crate::solver::DependencyProvider)
-    /// returned an error in the method
-    /// [list_available_versions](crate::solver::DependencyProvider::list_available_versions).
-    #[error("Retrieving available versions of package {package} failed")]
-    ErrorRetrievingVersions {
-        /// Package for which we want the list of versions.
-        package: P,
-        /// Error raised by the implementer of
-        /// [DependencyProvider](crate::solver::DependencyProvider).
-        source: Box<dyn std::error::Error>,
-    },
-
     /// Error arising when the implementer of
     /// [DependencyProvider](crate::solver::DependencyProvider)
     /// returned an error in the method
@@ -71,6 +58,13 @@ pub enum PubGrubError<P: Package, V: Version> {
         version: V,
     },
 
+    /// Error arising when the implementer of
+    /// [DependencyProvider](crate::solver::DependencyProvider)
+    /// returned an error in the method
+    /// [choose_package_version](crate::solver::DependencyProvider::choose_package_version).
+    #[error("Decision making failed")]
+    ErrorChoosingPackageVersion(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/memory.rs

@@ -5,6 +5,7 @@
 
 use crate::internal::assignment::Assignment::{self, Decision, Derivation};
 use crate::package::Package;
+use crate::range::Range;
 use crate::term::Term;
 use crate::type_aliases::{Map, SelectedDependencies};
 use crate::version::Version;
@@ -102,7 +103,7 @@ impl<P: Package, V: Version> Memory<P, V> {
     /// selected version (no "decision")
     /// and if it contains at least one positive derivation term
     /// in the partial solution.
-    pub fn potential_packages(&mut self) -> impl Iterator<Item = (&P, &Term<V>)> {
+    pub fn potential_packages(&mut self) -> impl Iterator<Item = (&P, &Range<V>)> {
         self.assignments
             .iter_mut()
             .filter_map(|(p, pa)| pa.potential_package_filter(p))
@@ -160,7 +161,7 @@ impl<V: Version> PackageAssignments<V> {
     fn potential_package_filter<'a, P: Package>(
         &'a mut self,
         package: &'a P,
-    ) -> Option<(&'a P, &'a Term<V>)> {
+    ) -> Option<(&'a P, &'a Range<V>)> {
         match self {
             PackageAssignments::Decision(_) => None,
             PackageAssignments::Derivations {
@@ -169,7 +170,7 @@ impl<V: Version> PackageAssignments<V> {
             } => {
                 if intersected.is_positive() || not_intersected_yet.iter().any(|t| t.is_positive())
                 {
-                    Some((package, self.assignment_intersection()))
+                    Some((package, self.assignment_intersection().unwrap_positive()))
                 } else {
                     None
                 }

src/internal/partial_solution.rs

@@ -3,20 +3,14 @@
 //! The partial solution is the current state
 //! of the solution being built by the algorithm.
 
-use crate::internal::incompatibility::PackageTerm;
+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::range::Range;
 use crate::term::Term;
 use crate::type_aliases::{Map, SelectedDependencies};
 use crate::version::Version;
-use crate::{
-    error::PubGrubError,
-    internal::incompatibility::{Incompatibility, Relation},
-};
-use crate::{
-    internal::assignment::Assignment::{self, Decision, Derivation},
-    solver::DependencyProvider,
-};
 
 #[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)]
 pub struct DecisionLevel(u32);
@@ -99,6 +93,11 @@ impl<P: Package, V: Version> PartialSolution<P, V> {
         self.memory.extract_solution()
     }
 
+    /// Compute, cache and retrieve the intersection of all terms for this package.
+    pub fn term_intersection_for_package(&mut self, package: &P) -> Option<&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: DecisionLevel) {
         // TODO: improve with dichotomic search.
@@ -121,49 +120,15 @@ 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<PackageTerm<P, V>>, PubGrubError<P, V>> {
-        let mut out: Option<PackageTerm<P, 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()));
-            }
+    /// Extract potential packages for the next iteration of unit propagation.
+    /// Return `None` if there is no suitable package anymore, which stops the algorithm.
+    pub fn potential_packages(&mut self) -> Option<impl Iterator<Item = (&P, &Range<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
-    /// that match the outstanding constraint.
-    ///
-    /// Here we just pick the first one that satisfies the terms.
-    /// It is the responsibility of the provider of `available_versions`
-    /// to list them with preferred versions first.
-    pub fn pick_version(available_versions: &[V], partial_solution_term: &Term<V>) -> Option<V> {
-        available_versions
-            .iter()
-            .find(|v| partial_solution_term.contains(v))
-            .cloned()
     }
 
     /// We can add the version to the partial solution as a decision

src/lib.rs

@@ -77,15 +77,15 @@
 //! ```
 //! # use pubgrub::solver::{DependencyProvider, Dependencies};
 //! # use pubgrub::version::SemanticVersion;
+//! # use pubgrub::range::Range;
+//! # use pubgrub::type_aliases::Map;
 //! # use std::error::Error;
+//! # use std::borrow::Borrow;
 //! #
 //! # struct MyDependencyProvider;
 //! #
 //! impl DependencyProvider<String, SemanticVersion> for MyDependencyProvider {
-//!     fn list_available_versions(
-//!         &self,
-//!         package: &String
-//!     ) -> Result<Vec<SemanticVersion>, Box<dyn Error>> {
+//!     fn choose_package_version<T: Borrow<String>, U: Borrow<Range<SemanticVersion>>>(&self,packages: impl Iterator<Item=(T, U)>) -> Result<(T, Option<SemanticVersion>), Box<dyn Error>> {
 //!         unimplemented!()
 //!     }
 //!
@@ -98,11 +98,20 @@
 //!     }
 //! }
 //! ```
+//!
 //! The first method
-//! [list_available_versions](crate::solver::DependencyProvider::list_available_versions)
-//! should return all available versions of a package.
-//! The second method
-//! [get_dependencies](crate::solver::DependencyProvider::get_dependencies)
+//! [choose_package_version](crate::solver::DependencyProvider::choose_package_version)
+//! chooses a package and available version compatible with the provided options.
+//! A helper function
+//! [choose_package_with_fewest_versions](crate::solver::choose_package_with_fewest_versions)
+//! is provided for convenience
+//! in cases when lists of available versions for packages are easily obtained.
+//! The strategy of that helper function consists in choosing the package
+//! with the fewest number of compatible versions to speed up resolution.
+//! But in general you are free to employ whatever strategy suits you best
+//! to pick a package and a version.
+//!
+//! The second method [get_dependencies](crate::solver::DependencyProvider::get_dependencies)
 //! aims at retrieving the dependencies of a given package at a given version.
 //! Returns [None] if dependencies are unknown.
 //!