Pass an iterator to add_incompatibility_from_dependencies (#226)

* Relax dependencies to iterator instead of a hashmap to avoid clones Previously, `Dependencies::Available` was hardcoded to a hash map. By relaxing this to `impl IntoIterator<_> + Clone`, we avoid converting from uv's ordered `Vec<(_, _)>` to a `HashMap<_, _>` and avoid cloning. ## Design considerations We implement this using the return type `Dependencies<impl IntoIterator<Item = (DP::P, DP::VS)> + Clone, DP::M>`. This allows using `get_dependencies` without knowing the actual (which a generic bound would require) or adding another associated type to the dependency provider. The `impl` bound also captures all input lifetimes, keeping `p` and `v` borrowed. We could bind the iterator to only `&self` instead, but this would only move two clones (package and version) to the implementer. Co-authored-by: Jacob Finkelman <[email protected]> Co-authored-by: Zanie <[email protected]> Co-authored-by: Charlie Marsh <[email protected]> * Avoid clone * Don't change `DependencyProvider` * Avoid changing the public API for the dependencies internal iterator --------- Co-authored-by: Jacob Finkelman <[email protected]> Co-authored-by: Zanie <[email protected]> Co-authored-by: Charlie Marsh <[email protected]>
pubgrub-rs · Jun 5, 2024 · 749abfa · 749abfa
1 parent f041854
commit 749abfa
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 11 deletions.
diff --git a/src/internal/core.rs b/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.
@@ -84,12 +84,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()),

diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs
@@ -137,18 +137,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),
         }
     }
 
@@ -190,7 +190,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()),
+            ),
         ));
     }
 

diff --git a/src/lib.rs b/src/lib.rs
@@ -73,7 +73,7 @@
 //! # use pubgrub::solver::{DependencyProvider, Dependencies};
 //! # use pubgrub::version::SemanticVersion;
 //! # use pubgrub::range::Range;
-//! # use pubgrub::type_aliases::Map;
+//! # use pubgrub::type_aliases::{DependencyConstraints, Map};
 //! # use std::error::Error;
 //! # use std::borrow::Borrow;
 //! # use std::convert::Infallible;
@@ -97,7 +97,7 @@
 //!         package: &String,
 //!         version: &SemanticVersion,
 //!     ) -> Result<Dependencies<String, SemVS, Self::M>, Infallible> {
-//!         unimplemented!()
+//!         Ok(Dependencies::Available(DependencyConstraints::default()))
 //!     }
 //!
 //!     type Err = Infallible;

diff --git a/src/solver.rs b/src/solver.rs
@@ -169,11 +169,11 @@ pub fn resolve<DP: DependencyProvider>(
 
             // Add that package and version if the dependencies are not problematic.
             let dep_incompats =
-                state.add_incompatibility_from_dependencies(p.clone(), v.clone(), &dependencies);
+                state.add_incompatibility_from_dependencies(p.clone(), v.clone(), dependencies);
 
             state.partial_solution.add_version(
                 p.clone(),
-                v,
+                v.clone(),
                 dep_incompats,
                 &state.incompatibility_store,
             );