use impl IntoIterator to avoide clones (#24)

Co-authored-by: Jacob Finkelman <[email protected]>

src/internal/core.rs

@@ -16,7 +16,7 @@ use crate::internal::partial_solution::{DecisionLevel, PartialSolution};
 use crate::internal::small_vec::SmallVec;
 use crate::report::DerivationTree;
 use crate::solver::DependencyProvider;
-use crate::type_aliases::{DependencyConstraints, IncompDpId, Map};
+use crate::type_aliases::{IncompDpId, Map};
 use crate::version_set::VersionSet;
 
 /// Current state of the PubGrub algorithm.
@@ -85,12 +85,12 @@ impl<DP: DependencyProvider> State<DP> {
         &mut self,
         package: DP::P,
         version: DP::V,
-        deps: &DependencyConstraints<DP::P, DP::VS>,
+        deps: impl IntoIterator<Item = (DP::P, DP::VS)>,
     ) -> std::ops::Range<IncompDpId<DP>> {
         // Create incompatibilities and allocate them in the store.
         let new_incompats_id_range =
             self.incompatibility_store
-                .alloc_iter(deps.iter().map(|dep| {
+                .alloc_iter(deps.into_iter().map(|dep| {
                     Incompatibility::from_dependency(
                         package.clone(),
                         <DP::VS as VersionSet>::singleton(version.clone()),

src/internal/incompatibility.rs

@@ -139,18 +139,18 @@ impl<P: Package, VS: VersionSet, M: Eq + Clone + Debug + Display> Incompatibilit
     }
 
     /// Build an incompatibility from a given dependency.
-    pub fn from_dependency(package: P, versions: VS, dep: (&P, &VS)) -> Self {
+    pub fn from_dependency(package: P, versions: VS, dep: (P, VS)) -> Self {
         let (p2, set2) = dep;
         Self {
-            package_terms: if set2 == &VS::empty() {
+            package_terms: if set2 == VS::empty() {
                 SmallMap::One([(package.clone(), Term::Positive(versions.clone()))])
             } else {
                 SmallMap::Two([
                     (package.clone(), Term::Positive(versions.clone())),
                     (p2.clone(), Term::Negative(set2.clone())),
                 ])
             },
-            kind: Kind::FromDependencyOf(package, versions, p2.clone(), set2.clone()),
+            kind: Kind::FromDependencyOf(package, versions, p2, set2),
         }
     }
 
@@ -193,7 +193,10 @@ impl<P: Package, VS: VersionSet, M: Eq + Clone + Debug + Display> Incompatibilit
                 .unwrap()
                 .unwrap_positive()
                 .union(other.get(p1).unwrap().unwrap_positive()), // It is safe to `simplify` here
-            (p2, dep_term.map_or(&VS::empty(), |v| v.unwrap_negative())),
+            (
+                p2.clone(),
+                dep_term.map_or(VS::empty(), |v| v.unwrap_negative().clone()),
+            ),
         ));
     }
 

src/solver.rs

@@ -71,7 +71,7 @@ use crate::error::PubGrubError;
 pub use crate::internal::core::State;
 pub use crate::internal::incompatibility::{Incompatibility, Kind};
 use crate::package::Package;
-use crate::type_aliases::{DependencyConstraints, Map, SelectedDependencies};
+use crate::type_aliases::{Map, SelectedDependencies};
 use crate::version_set::VersionSet;
 use log::{debug, info};
 
@@ -159,10 +159,10 @@ pub fn resolve<DP: DependencyProvider>(
                     ));
                     continue;
                 }
-                Dependencies::Available(x) if x.contains_key(p) => {
+                Dependencies::Available(x) if x.clone().into_iter().any(|(d, _)| &d == p) => {
                     return Err(PubGrubError::SelfDependency {
                         package: p.clone(),
-                        version: v,
+                        version: v.clone(),
                     });
                 }
                 Dependencies::Available(x) => x,
@@ -172,12 +172,12 @@ pub fn resolve<DP: DependencyProvider>(
             let dep_incompats = state.add_incompatibility_from_dependencies(
                 p.clone(),
                 v.clone(),
-                &known_dependencies,
+                known_dependencies,
             );
 
             state.partial_solution.add_version(
                 p.clone(),
-                v,
+                v.clone(),
                 dep_incompats,
                 &state.incompatibility_store,
             );
@@ -197,7 +197,7 @@ pub enum Dependencies<P: Package, VS: VersionSet, M: Eq + Clone + Debug + Displa
     /// Package dependencies are unavailable with the reason why they are missing.
     Unavailable(M),
     /// Container for all available package versions.
-    Available(DependencyConstraints<P, VS>),
+    Available(T),
 }
 
 /// Trait that allows the algorithm to retrieve available packages and their dependencies.
@@ -284,7 +284,7 @@ pub trait DependencyProvider {
         &self,
         package: &Self::P,
         version: &Self::V,
-    ) -> Result<Dependencies<Self::P, Self::VS, Self::M>, Self::Err>;
+    ) -> Result<Dependencies<impl IntoIterator<Item = (Self::P, Self::VS)> + Clone, Self::M>, Self::Err>;
 
     /// This is called fairly regularly during the resolution,
     /// if it returns an Err then resolution will be terminated.
@@ -308,7 +308,7 @@ pub trait DependencyProvider {
 )]
 #[cfg_attr(feature = "serde", serde(transparent))]
 pub struct OfflineDependencyProvider<P: Package, VS: VersionSet> {
-    dependencies: Map<P, BTreeMap<VS::V, DependencyConstraints<P, VS>>>,
+    dependencies: Map<P, BTreeMap<VS::V, Map<P, VS>>>,
 }
 
 impl<P: Package, VS: VersionSet> OfflineDependencyProvider<P, VS> {
@@ -358,8 +358,8 @@ impl<P: Package, VS: VersionSet> OfflineDependencyProvider<P, VS> {
 
     /// 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: &VS::V) -> Option<DependencyConstraints<P, VS>> {
-        self.dependencies.get(package)?.get(version).cloned()
+    fn dependencies(&self, package: &P, version: &VS::V) -> Option<&Map<P, VS>> {
+        self.dependencies.get(package)?.get(version)
     }
 }
 
@@ -397,12 +397,16 @@ impl<P: Package, VS: VersionSet> DependencyProvider for OfflineDependencyProvide
         &self,
         package: &P,
         version: &VS::V,
-    ) -> Result<Dependencies<P, VS, Self::M>, Infallible> {
+    ) -> Result<Dependencies<impl IntoIterator<Item = (P, VS)> + Clone, Self::M>, Infallible> {
         Ok(match self.dependencies(package, version) {
             None => {
                 Dependencies::Unavailable("its dependencies could not be determined".to_string())
             }
-            Some(dependencies) => Dependencies::Available(dependencies),
+            Some(dependencies) => Dependencies::Available(
+                dependencies
+                    .into_iter()
+                    .map(|(p, vs)| (p.clone(), vs.clone())),
+            ),
         })
     }
 }

src/type_aliases.rs

@@ -10,18 +10,10 @@ pub type Map<K, V> = rustc_hash::FxHashMap<K, V>;
 /// Set implementation used by the library.
 pub type Set<V> = rustc_hash::FxHashSet<V>;
 
-/// Concrete dependencies picked by the library during [resolve](crate::solver::resolve)
-/// from [DependencyConstraints].
+/// Concrete dependencies picked by the library during [resolve](crate::solver::resolve).
 pub type SelectedDependencies<DP> =
     Map<<DP as DependencyProvider>::P, <DP as DependencyProvider>::V>;
 
-/// Holds information about all possible versions a given package can accept.
-/// There is a difference in semantics between an empty map
-/// inside [DependencyConstraints] and [Dependencies::Unavailable](crate::solver::Dependencies::Unavailable):
-/// the former means the package has no dependency and it is a known fact,
-/// while the latter means they could not be fetched by the [DependencyProvider].
-pub type DependencyConstraints<P, VS> = Map<P, VS>;
-
 pub(crate) type IncompDpId<DP> = IncompId<
     <DP as DependencyProvider>::P,
     <DP as DependencyProvider>::VS,

tests/proptest.rs

@@ -352,8 +352,8 @@ fn retain_dependencies<N: Package + Ord, VS: VersionSet>(
             smaller_dependency_provider.add_dependencies(
                 n.clone(),
                 v.clone(),
-                deps.iter().filter_map(|(dep, range)| {
-                    if !retain(n, v, dep) {
+                deps.into_iter().filter_map(|(dep, range)| {
+                    if !retain(n, v, &dep) {
                         None
                     } else {
                         Some((dep.clone(), range.clone()))

tests/sat_dependency_provider.rs

@@ -69,10 +69,10 @@ impl<P: Package, VS: VersionSet> SatResolve<P, VS> {
                 Dependencies::Unavailable(_) => panic!(),
                 Dependencies::Available(d) => d,
             };
-            for (p1, range) in &deps {
+            for (p1, range) in deps {
                 let empty_vec = vec![];
                 let mut matches: Vec<varisat::Lit> = all_versions_by_p
-                    .get(p1)
+                    .get(&p1)
                     .unwrap_or(&empty_vec)
                     .iter()
                     .filter(|(v1, _)| range.contains(v1))