From 52297b341c2551ca6d7889b85da380e800050d51 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 21 Feb 2024 21:00:38 -0500 Subject: [PATCH] use impl IntoIterator to avoide clones (#24) Co-authored-by: Jacob Finkelman --- src/internal/core.rs | 6 +++--- src/internal/incompatibility.rs | 11 +++++++---- src/lib.rs | 5 +++-- src/solver.rs | 32 ++++++++++++++++++-------------- src/type_aliases.rs | 10 +--------- tests/proptest.rs | 12 ++++++++---- tests/sat_dependency_provider.rs | 4 ++-- 7 files changed, 42 insertions(+), 38 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index d79ab839..c7a2abdd 100644 --- 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. @@ -85,12 +85,12 @@ impl State { &mut self, package: DP::P, version: DP::V, - deps: &DependencyConstraints, + deps: impl IntoIterator, ) -> std::ops::Range> { // 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(), ::singleton(version.clone()), diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 629582d9..c061ec68 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -109,10 +109,10 @@ impl Incompatibility { } /// 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([ @@ -120,7 +120,7 @@ impl Incompatibility { (p2.clone(), Term::Negative(set2.clone())), ]) }, - kind: Kind::FromDependencyOf(package, versions, p2.clone(), set2.clone()), + kind: Kind::FromDependencyOf(package, versions, p2, set2), } } @@ -163,7 +163,10 @@ impl Incompatibility { .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 index 780aa28d..17c764e4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -96,8 +96,9 @@ //! &self, //! package: &String, //! version: &SemanticVersion, -//! ) -> Result, Infallible> { -//! unimplemented!() +//! ) -> Result + Clone>, Infallible> +//! { +//! Ok(Dependencies::Available([])) //! } //! //! type Err = Infallible; diff --git a/src/solver.rs b/src/solver.rs index 885812a5..e62f2f78 100644 --- a/src/solver.rs +++ b/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( )); 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( 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, ); @@ -193,11 +193,11 @@ pub fn resolve( /// 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 { +pub enum Dependencies { /// Package dependencies are unavailable. Unavailable, /// Container for all available package versions. - Available(DependencyConstraints), + Available(T), } /// Trait that allows the algorithm to retrieve available packages and their dependencies. @@ -267,7 +267,7 @@ pub trait DependencyProvider { &self, package: &Self::P, version: &Self::V, - ) -> Result, Self::Err>; + ) -> Result + Clone>, Self::Err>; /// This is called fairly regularly during the resolution, /// if it returns an Err then resolution will be terminated. @@ -291,7 +291,7 @@ pub trait DependencyProvider { )] #[cfg_attr(feature = "serde", serde(transparent))] pub struct OfflineDependencyProvider { - dependencies: Map>>, + dependencies: Map>>, } impl OfflineDependencyProvider { @@ -341,8 +341,8 @@ impl OfflineDependencyProvider { /// 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> { - self.dependencies.get(package)?.get(version).cloned() + fn dependencies(&self, package: &P, version: &VS::V) -> Option<&Map> { + self.dependencies.get(package)?.get(version) } } @@ -378,11 +378,15 @@ impl DependencyProvider for OfflineDependencyProvide fn get_dependencies( &self, package: &P, - version: &VS::V, - ) -> Result, Infallible> { + version: &::V, + ) -> Result + 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())), + ), }) } } diff --git a/src/type_aliases.rs b/src/type_aliases.rs index 21fbf9f7..a70540c7 100644 --- a/src/type_aliases.rs +++ b/src/type_aliases.rs @@ -10,17 +10,9 @@ pub type Map = rustc_hash::FxHashMap; /// Set implementation used by the library. pub type Set = rustc_hash::FxHashSet; -/// 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 = 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]. -pub type DependencyConstraints = Map; - pub(crate) type IncompDpId = IncompId<::P, ::VS>; diff --git a/tests/proptest.rs b/tests/proptest.rs index 12746e67..1e5b5457 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -31,7 +31,11 @@ struct OldestVersionsDependencyProvider( ); impl DependencyProvider for OldestVersionsDependencyProvider { - fn get_dependencies(&self, p: &P, v: &VS::V) -> Result, Infallible> { + fn get_dependencies( + &self, + p: &P, + v: &VS::V, + ) -> Result + Clone>, Infallible> { self.0.get_dependencies(p, v) } @@ -83,7 +87,7 @@ impl DependencyProvider for TimeoutDependencyProvider Result, DP::Err> { + ) -> Result + Clone>, DP::Err> { self.dp.get_dependencies(p, v) } @@ -344,8 +348,8 @@ fn retain_dependencies( 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())) diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index c389fee6..297809b3 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -69,10 +69,10 @@ impl SatResolve { 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 = all_versions_by_p - .get(p1) + .get(&p1) .unwrap_or(&empty_vec) .iter() .filter(|(v1, _)| range.contains(v1))