refactor!: define Dependencies type

examples/caching_dependency_provider.rs

@@ -1,14 +1,13 @@
 // SPDX-License-Identifier: MPL-2.0
 
-use pubgrub::package::Package;
-use pubgrub::range::Range;
-use pubgrub::solver::{resolve, DependencyProvider, OfflineDependencyProvider};
-use pubgrub::type_aliases::Map;
-use pubgrub::version::{NumberVersion, Version};
 use std::cell::RefCell;
 use std::error::Error;
 use std::hash::Hash;
 
+use pubgrub::package::Package;
+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>> {
@@ -40,20 +39,21 @@ impl<P: Package, V: Version + Hash, DP: DependencyProvider<P, V>> DependencyProv
         &self,
         package: &P,
         version: &V,
-    ) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
+    ) -> Result<Dependencies<P, V>, Box<dyn Error>> {
         let mut cache = self.cached_dependencies.borrow_mut();
         match cache.get_dependencies(package, version) {
-            Ok(None) => {
+            Ok(Dependencies::Unavailable) => {
                 let dependencies = self.remote_dependencies.get_dependencies(package, version);
                 match dependencies {
-                    Ok(dependencies) => {
+                    Ok(Dependencies::Known(dependencies)) => {
                         cache.add_dependencies(
                             package.clone(),
                             version.clone(),
-                            dependencies.clone().unwrap_or_default(),
+                            dependencies.clone().into_iter(),
                         );
-                        Ok(dependencies)
+                        Ok(Dependencies::Known(dependencies))
                     }
+                    Ok(Dependencies::Unavailable) => Ok(Dependencies::Unavailable),
                     error @ Err(_) => error,
                 }
             }

src/internal/incompatibility.rs

@@ -9,6 +9,7 @@ use std::fmt;
 use crate::package::Package;
 use crate::range::Range;
 use crate::report::{DefaultStringReporter, DerivationTree, Derived, External};
+use crate::solver::AllowedVersions;
 use crate::term::{self, Term};
 use crate::type_aliases::Map;
 use crate::version::Version;
@@ -112,7 +113,7 @@ impl<P: Package, V: Version> Incompatibility<P, V> {
         start_id: usize,
         package: P,
         version: V,
-        deps: &Map<P, Range<V>>,
+        deps: &AllowedVersions<P, V>,
     ) -> Vec<Self> {
         deps.iter()
             .enumerate()

src/lib.rs

@@ -75,11 +75,9 @@
 //! and [SemanticVersion](version::SemanticVersion) for versions.
 //! This may be done quite easily by implementing the two following functions.
 //! ```
-//! # use pubgrub::solver::DependencyProvider;
+//! # use pubgrub::solver::{DependencyProvider, Dependencies};
 //! # use pubgrub::version::SemanticVersion;
 //! # use std::error::Error;
-//! # use pubgrub::range::Range;
-//! # use pubgrub::type_aliases::Map;
 //! #
 //! # struct MyDependencyProvider;
 //! #
@@ -95,7 +93,7 @@
 //!         &self,
 //!         package: &String,
 //!         version: &SemanticVersion,
-//!     ) -> Result<Option<Map<String, Range<SemanticVersion>>>, Box<dyn Error>> {
+//!     ) -> Result<Dependencies<String, SemanticVersion>, Box<dyn Error>> {
 //!         unimplemented!()
 //!     }
 //! }

src/solver.rs

@@ -68,6 +68,7 @@
 use std::collections::BTreeSet as Set;
 use std::error::Error;
 use std::hash::Hash;
+use std::iter::FromIterator;
 
 use crate::error::PubGrubError;
 use crate::internal::core::State;
@@ -135,13 +136,13 @@ pub fn resolve<P: Package, V: Version>(
                 version: v.clone(),
                 source: err,
             })? {
-            None => {
+            Dependencies::Unavailable => {
                 state.add_incompatibility(|id| {
                     Incompatibility::unavailable_dependencies(id, p.clone(), v.clone())
                 });
                 continue;
             }
-            Some(x) => x,
+            Dependencies::Known(x) => x,
         };
 
         // Add that package and version if the dependencies are not problematic.
@@ -171,6 +172,58 @@ pub fn resolve<P: Package, V: Version>(
     }
 }
 
+/// An enum used by [DependencyProvider] that holds information about package dependencies.
+/// For each [Package] there is a [Range] of concrete versions it allows as a dependency.
+#[derive(Clone)]
+pub enum Dependencies<P: Package, V: Version> {
+    /// Package dependencies are unavailable.
+    Unavailable,
+    /// Container for all available package versions.
+    Known(AllowedVersions<P, V>),
+}
+
+/// Subtype of [Dependencies] which holds information about
+/// all possible versions a given package can accept.
+/// There is a difference in semantics between an empty [Map<P, Range<V>>](crate::type_aliases::Map)
+/// inside [AllowedVersions] and [Dependencies::Unavailable]:
+/// the former means the package has no dependencies and it is a known fact,
+/// while the latter means they could not be fetched by [DependencyProvider].
+#[derive(Debug, Clone)]
+#[cfg_attr(
+    feature = "serde",
+    derive(serde::Serialize, serde::Deserialize),
+    serde(transparent)
+)]
+pub struct AllowedVersions<P: Package, V: Version>(Map<P, Range<V>>);
+
+impl<P: Package, V: Version> Default for AllowedVersions<P, V> {
+    fn default() -> Self {
+        AllowedVersions(Map::default())
+    }
+}
+
+impl<P: Package, V: Version> AllowedVersions<P, V> {
+    /// An iterator over inner values of [Map<P, Range<V>>](crate::type_aliases::Map)
+    pub fn iter(&self) -> impl Iterator<Item = (&P, &Range<V>)> {
+        self.0.iter()
+    }
+}
+
+impl<P: Package, V: Version> IntoIterator for AllowedVersions<P, V> {
+    type Item = (P, Range<V>);
+    type IntoIter = std::collections::hash_map::IntoIter<P, Range<V>>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.0.into_iter()
+    }
+}
+
+impl<P: Package, V: Version> FromIterator<(P, Range<V>)> for AllowedVersions<P, V> {
+    fn from_iter<T: IntoIterator<Item = (P, Range<V>)>>(iter: T) -> Self {
+        AllowedVersions(iter.into_iter().collect())
+    }
+}
+
 /// Trait that allows the algorithm to retrieve available packages and their dependencies.
 /// An implementor needs to be supplied to the [resolve] function.
 pub trait DependencyProvider<P: Package, V: Version> {
@@ -180,12 +233,12 @@ pub trait DependencyProvider<P: Package, V: Version> {
     fn list_available_versions(&self, package: &P) -> Result<Vec<V>, Box<dyn Error>>;
 
     /// Retrieves the package dependencies.
-    /// Return None if its dependencies are unknown.
+    /// Return [Dependencies::Unavailable] if its dependencies are unknown.
     fn get_dependencies(
         &self,
         package: &P,
         version: &V,
-    ) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>>;
+    ) -> Result<Dependencies<P, V>, Box<dyn Error>>;
 
     /// This is called fairly regularly during the resolution,
     /// if it returns an Err then resolution will be terminated.
@@ -202,7 +255,7 @@ pub trait DependencyProvider<P: Package, V: Version> {
 #[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>>>>,
+    dependencies: Map<P, Map<V, AllowedVersions<P, V>>>,
 }
 
 impl<P: Package, V: Version + Hash> OfflineDependencyProvider<P, V> {
@@ -253,8 +306,9 @@ impl<P: Package, V: Version + Hash> OfflineDependencyProvider<P, V> {
     }
 
     /// Lists dependencies of a given package and version.
-    /// Returns [None] if no information is available regarding that package and version pair.
-    fn dependencies(&self, package: &P, version: &V) -> Option<Map<P, Range<V>>> {
+    /// Returns [Dependencies::Unavailable] if no information is available
+    /// regarding that package and version pair.
+    fn dependencies(&self, package: &P, version: &V) -> Option<AllowedVersions<P, V>> {
         self.dependencies
             .get(package)?
             .get(version)
@@ -277,7 +331,10 @@ impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OfflineDependen
         &self,
         package: &P,
         version: &V,
-    ) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
-        Ok(self.dependencies(package, version))
+    ) -> Result<Dependencies<P, V>, Box<dyn Error>> {
+        Ok(match self.dependencies(package, version) {
+            None => Dependencies::Unavailable,
+            Some(dependencies) => Dependencies::Known(dependencies),
+        })
     }
 }

tests/proptest.rs

@@ -2,21 +2,21 @@
 
 use std::{collections::BTreeSet as Set, error::Error};
 
+use proptest::collection::{btree_map, vec};
+use proptest::prelude::*;
+use proptest::sample::Index;
+use proptest::string::string_regex;
+
 use pubgrub::range::Range;
-use pubgrub::solver::{resolve, DependencyProvider, OfflineDependencyProvider};
-use pubgrub::type_aliases::Map;
+use pubgrub::solver::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider};
 use pubgrub::{
     error::PubGrubError, package::Package, report::DefaultStringReporter, report::Reporter,
     version::NumberVersion, version::Version,
 };
 
-use proptest::collection::{btree_map, vec};
-use proptest::prelude::*;
-use proptest::sample::Index;
-use proptest::string::string_regex;
+use crate::sat_dependency_provider::SatResolve;
 
 mod sat_dependency_provider;
-use crate::sat_dependency_provider::SatResolve;
 
 /// The same as DP but takes versions from the opposite end:
 /// if DP returns versions from newest to oldest, this returns them from oldest to newest.
@@ -33,7 +33,7 @@ impl<P: Package, V: Version, DP: DependencyProvider<P, V>> DependencyProvider<P,
         })
     }
 
-    fn get_dependencies(&self, p: &P, v: &V) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
+    fn get_dependencies(&self, p: &P, v: &V) -> Result<Dependencies<P, V>, Box<dyn Error>> {
         self.0.get_dependencies(p, v)
     }
 }
@@ -65,7 +65,7 @@ impl<P: Package, V: Version, DP: DependencyProvider<P, V>> DependencyProvider<P,
         self.dp.list_available_versions(p)
     }
 
-    fn get_dependencies(&self, p: &P, v: &V) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
+    fn get_dependencies(&self, p: &P, v: &V) -> Result<Dependencies<P, V>, Box<dyn Error>> {
         self.dp.get_dependencies(p, v)
     }
 
@@ -357,27 +357,46 @@ proptest! {
     fn prop_removing_a_dep_cant_break(
         (dependency_provider, cases) in registry_strategy(0u16..665, 666),
         indexes_to_remove in prop::collection::vec((any::<prop::sample::Index>(), any::<prop::sample::Index>(), any::<prop::sample::Index>()), 1..10)
-    )  {
+    ) {
         let packages: Vec<_> = dependency_provider.packages().collect();
         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 = dependency_provider.list_available_versions(package).unwrap();
+            let versions = dependency_provider
+                .list_available_versions(package)
+                .unwrap();
             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();
+            let dependencies: Vec<(u16, Range<NumberVersion>)> = match dependency_provider
+                .get_dependencies(package, version)
+                .unwrap()
+            {
+                Dependencies::Unavailable => panic!(),
+                Dependencies::Known(d) => d.into_iter().collect(),
+            };
+            if !dependencies.is_empty() {
+                let dependency = dep_idx.get(&dependencies).0;
                 removed_provider.add_dependencies(
                     **package,
                     *version,
-                    dependencys.into_iter().filter(|x| x.0 != dependency)
+                    dependencies.into_iter().filter(|x| x.0 != dependency),
                 )
             }
         }
         for (name, ver) in cases {
-            if resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver).is_ok() {
+            if resolve(
+                &TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000),
+                name,
+                ver,
+            )
+            .is_ok()
+            {
                 prop_assert!(
-                    resolve(&TimeoutDependencyProvider::new(removed_provider.clone(), 50_000), name, ver).is_ok(),
+                    resolve(
+                        &TimeoutDependencyProvider::new(removed_provider.clone(), 50_000),
+                        name,
+                        ver
+                    )
+                    .is_ok(),
                     "full index worked for `{} = \"={}\"` but removing some deps broke it!",
                     name,
                     ver,
@@ -412,9 +431,11 @@ proptest! {
                         if used.get(&n) == Some(&v) // it was used
                            || to_remove.get(&(n, v)).is_none() // or it is not one to be removed
                         {
-                            smaller_dependency_provider.add_dependencies(n, v,
-                                dependency_provider.get_dependencies(&n, &v).unwrap().unwrap()
-                            )
+                            let deps = match dependency_provider.get_dependencies(&n, &v).unwrap() {
+                                Dependencies::Unavailable => panic!(),
+                                Dependencies::Known(deps) => deps,
+                            };
+                            smaller_dependency_provider.add_dependencies(n, v, deps)
                         }
                     }
                     prop_assert!(
@@ -432,9 +453,11 @@ proptest! {
                     for &(n, v) in &all_versions {
                         if to_remove.get(&(n, v)).is_none() // it is not one to be removed
                         {
-                            smaller_dependency_provider.add_dependencies(n, v,
-                                dependency_provider.get_dependencies(&n, &v).unwrap().unwrap()
-                            )
+                            let deps = match dependency_provider.get_dependencies(&n, &v).unwrap() {
+                                Dependencies::Unavailable => panic!(),
+                                Dependencies::Known(deps) => deps,
+                            };
+                            smaller_dependency_provider.add_dependencies(n, v, deps)
                         }
                     }
                     prop_assert!(

tests/sat_dependency_provider.rs

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: MPL-2.0
 
 use pubgrub::package::Package;
-use pubgrub::solver::{DependencyProvider, OfflineDependencyProvider};
+use pubgrub::solver::{Dependencies, DependencyProvider, OfflineDependencyProvider};
 use pubgrub::type_aliases::Map;
 use pubgrub::version::Version;
 use std::hash::Hash;
@@ -76,7 +76,11 @@ impl<P: Package, V: Version + Hash> SatResolve<P, V> {
 
         // active packages need each of there `deps` to be satisfied
         for (p, v, var) in &all_versions {
-            for (p1, range) in dp.get_dependencies(p, v).unwrap().unwrap() {
+            let deps = match dp.get_dependencies(p, v).unwrap() {
+                Dependencies::Unavailable => panic!(),
+                Dependencies::Known(d) => d,
+            };
+            for (p1, range) in deps.iter() {
                 let empty_vec = vec![];
                 let mut matches: Vec<varisat::Lit> = all_versions_by_p
                     .get(&p1)