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::package::Package;
 use crate::report::DerivationTree;
-use crate::type_aliases::{DependencyConstraints, Map};
+use crate::type_aliases::Map;
 use crate::version_set::VersionSet;
 
 /// Current state of the PubGrub algorithm.
@@ -82,12 +82,12 @@ impl<P: Package, VS: VersionSet, Priority: Ord + Clone> State<P, VS, Priority> {
         &mut self,
         package: P,
         version: VS::V,
-        deps: &DependencyConstraints<P, VS>,
+        deps: impl IntoIterator<Item = (P, VS)>,
     ) -> std::ops::Range<IncompId<P, VS>> {
         // 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(),
                         VS::singleton(version.clone()),

src/internal/incompatibility.rs

@@ -106,18 +106,18 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> {
     }
 
     /// 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),
         }
     }
 
@@ -159,7 +159,10 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> {
                 .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/lib.rs

@@ -103,8 +103,9 @@
 //!         &self,
 //!         package: &String,
 //!         version: &SemanticVersion,
-//!     ) -> Result<Dependencies<String, SemVS>, Infallible> {
-//!         unimplemented!()
+//!     ) -> Result<Dependencies<impl IntoIterator<Item = (String, SemVS)> + Clone>, Infallible>
+//!     {
+//!         Ok(Dependencies::Available([]))
 //!     }
 //!
 //!     type Err = Infallible;

src/solver.rs

@@ -78,7 +78,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};
 
@@ -167,10 +167,10 @@ pub fn resolve<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>>(
                     ));
                     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,
@@ -180,12 +180,12 @@ pub fn resolve<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>>(
             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,
             );
@@ -201,11 +201,11 @@ pub fn resolve<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>>(
 /// An enum used by [DependencyProvider] that holds information about package dependencies.
 /// For each [Package] there is a set of versions allowed as a dependency.
 #[derive(Clone)]
-pub enum Dependencies<P: Package, VS: VersionSet> {
+pub enum Dependencies<T> {
     /// Package dependencies are unavailable.
     Unavailable,
     /// Container for all available package versions.
-    Available(DependencyConstraints<P, VS>),
+    Available(T),
 }
 
 /// Trait that allows the algorithm to retrieve available packages and their dependencies.
@@ -261,7 +261,7 @@ pub trait DependencyProvider<P: Package, VS: VersionSet> {
         &self,
         package: &P,
         version: &VS::V,
-    ) -> Result<Dependencies<P, VS>, Self::Err>;
+    ) -> Result<Dependencies<impl IntoIterator<Item = (P, VS)> + Clone>, Self::Err>;
 
     /// This is called fairly regularly during the resolution,
     /// if it returns an Err then resolution will be terminated.
@@ -285,7 +285,7 @@ pub trait DependencyProvider<P: Package, VS: VersionSet> {
 )]
 #[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> {
@@ -335,8 +335,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)
     }
 }
 
@@ -368,11 +368,15 @@ impl<P: Package, VS: VersionSet> DependencyProvider<P, VS> for OfflineDependency
     fn get_dependencies(
         &self,
         package: &P,
-        version: &VS::V,
-    ) -> Result<Dependencies<P, VS>, Infallible> {
+        version: &<VS as VersionSet>::V,
+    ) -> Result<Dependencies<impl IntoIterator<Item = (P, VS)> + Clone>, Self::Err> {
         Ok(match self.dependencies(package, version) {
             None => Dependencies::Unavailable,
-            Some(dependencies) => Dependencies::Available(dependencies),
+            Some(dependencies) => Dependencies::Available(
+                dependencies
+                    .into_iter()
+                    .map(|(p, vs)| (p.clone(), vs.clone())),
+            ),
         })
     }
 }

src/type_aliases.rs

@@ -5,13 +5,5 @@
 /// Map implementation used by the library.
 pub type Map<K, V> = rustc_hash::FxHashMap<K, 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<P, V> = Map<P, 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](crate::solver::DependencyProvider).
-pub type DependencyConstraints<P, VS> = Map<P, VS>;

tests/proptest.rs

@@ -35,7 +35,11 @@ struct OldestVersionsDependencyProvider<P: Package, VS: VersionSet>(
 impl<P: Package, VS: VersionSet> DependencyProvider<P, VS>
     for OldestVersionsDependencyProvider<P, VS>
 {
-    fn get_dependencies(&self, p: &P, v: &VS::V) -> Result<Dependencies<P, VS>, Infallible> {
+    fn get_dependencies(
+        &self,
+        p: &P,
+        v: &VS::V,
+    ) -> Result<Dependencies<impl IntoIterator<Item = (P, VS)> + Clone>, Infallible> {
         self.0.get_dependencies(p, v)
     }
 
@@ -81,7 +85,11 @@ impl<DP> TimeoutDependencyProvider<DP> {
 impl<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>> DependencyProvider<P, VS>
     for TimeoutDependencyProvider<DP>
 {
-    fn get_dependencies(&self, p: &P, v: &VS::V) -> Result<Dependencies<P, VS>, DP::Err> {
+    fn get_dependencies(
+        &self,
+        p: &P,
+        v: &VS::V,
+    ) -> Result<Dependencies<impl IntoIterator<Item = (P, VS)> + Clone>, DP::Err> {
         self.dp.get_dependencies(p, v)
     }
 
@@ -339,8 +347,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

@@ -71,10 +71,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))