From 964005457c481e87085be82649aaa05c7cb98ae4 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Mon, 5 Feb 2024 08:36:10 -0600 Subject: [PATCH] use impl IntoIterator to avoid clones (#24) Co-authored-by: Jacob Finkelman --- examples/caching_dependency_provider.rs | 40 +++++++++++++++---------- src/internal/core.rs | 6 ++-- src/internal/incompatibility.rs | 11 ++++--- src/lib.rs | 4 +-- src/solver.rs | 31 +++++++++++-------- src/type_aliases.rs | 10 +------ tests/proptest.rs | 9 +++--- tests/sat_dependency_provider.rs | 4 +-- 8 files changed, 63 insertions(+), 52 deletions(-) diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs index ae04886a..03010753 100644 --- a/examples/caching_dependency_provider.rs +++ b/examples/caching_dependency_provider.rs @@ -29,27 +29,37 @@ impl> DependencyProvider for CachingDependenc &self, package: &DP::P, version: &DP::V, - ) -> Result, DP::Err> { + ) -> Result + Clone, Self::M>, DP::Err> + { let mut cache = self.cached_dependencies.borrow_mut(); match cache.get_dependencies(package, version) { Ok(Dependencies::Unavailable(_)) => { - let dependencies = self.remote_dependencies.get_dependencies(package, version); - match dependencies { - Ok(Dependencies::Available(dependencies)) => { - cache.add_dependencies( - package.clone(), - version.clone(), - dependencies.clone(), - ); - Ok(Dependencies::Available(dependencies)) - } - Ok(Dependencies::Unavailable(reason)) => Ok(Dependencies::Unavailable(reason)), - error @ Err(_) => error, - } + // Code below to end the borrow. + } + Ok(dependencies) => { + return Ok(match dependencies { + Dependencies::Unavailable(reason) => Dependencies::Unavailable(reason), + Dependencies::Available(available) => Dependencies::Available( + available.into_iter().collect::>(), + ), + }) } - Ok(dependencies) => Ok(dependencies), Err(_) => unreachable!(), } + let dependencies = self.remote_dependencies.get_dependencies(package, version); + match dependencies { + Ok(Dependencies::Available(dependencies)) => { + cache.add_dependencies(package.clone(), version.clone(), dependencies.clone()); + Ok(Dependencies::Available( + dependencies + .clone() + .into_iter() + .collect::>(), + )) + } + Ok(Dependencies::Unavailable(reason)) => Ok(Dependencies::Unavailable(reason)), + Err(err) => Err(err), + } } fn choose_version(&self, package: &DP::P, range: &DP::VS) -> Result, DP::Err> { diff --git a/src/internal/core.rs b/src/internal/core.rs index fab2d69b..e443b0a5 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 fb45ca48..4d477371 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -139,10 +139,10 @@ impl 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([ @@ -150,7 +150,7 @@ impl Incompatibilit (p2.clone(), Term::Negative(set2.clone())), ]) }, - kind: Kind::FromDependencyOf(package, versions, p2.clone(), set2.clone()), + kind: Kind::FromDependencyOf(package, versions, p2, set2), } } @@ -194,7 +194,10 @@ impl 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 index 8b327e2b..63f1153f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -96,8 +96,8 @@ //! &self, //! package: &String, //! version: &SemanticVersion, -//! ) -> Result, Infallible> { -//! unimplemented!() +//! ) -> Result + Clone, Self::M>, Infallible> { +//! Ok(Dependencies::Available([])) //! } //! //! type Err = Infallible; diff --git a/src/solver.rs b/src/solver.rs index 07e99740..782ebbf0 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}; @@ -158,10 +158,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, @@ -169,11 +169,11 @@ pub fn resolve( // 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, ); @@ -189,11 +189,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 with the reason why they are missing. Unavailable(M), /// Container for all available package versions. - Available(DependencyConstraints), + Available(T), } /// Trait that allows the algorithm to retrieve available packages and their dependencies. @@ -280,7 +280,10 @@ pub trait DependencyProvider { &self, package: &Self::P, version: &Self::V, - ) -> Result, Self::Err>; + ) -> Result< + Dependencies + Clone, Self::M>, + Self::Err, + >; /// This is called fairly regularly during the resolution, /// if it returns an Err then resolution will be terminated. @@ -304,7 +307,7 @@ pub trait DependencyProvider { )] #[cfg_attr(feature = "serde", serde(transparent))] pub struct OfflineDependencyProvider { - dependencies: Map>>, + dependencies: Map>>, } impl OfflineDependencyProvider { @@ -354,8 +357,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) } } @@ -393,12 +396,14 @@ impl DependencyProvider for OfflineDependencyProvide &self, package: &P, version: &VS::V, - ) -> Result, Infallible> { + ) -> Result + 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.iter().map(|(p, vs)| (p.clone(), vs.clone()))) + } }) } } diff --git a/src/type_aliases.rs b/src/type_aliases.rs index 8b893d18..25850ac1 100644 --- a/src/type_aliases.rs +++ b/src/type_aliases.rs @@ -10,18 +10,10 @@ 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 33e5cece..b73820bf 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -37,7 +37,7 @@ impl DependencyProvider for OldestVersionsDependency &self, p: &P, v: &VS::V, - ) -> Result, Infallible> { + ) -> Result + Clone, Self::M>, Infallible> { self.0.get_dependencies(p, v) } @@ -90,7 +90,8 @@ impl DependencyProvider for TimeoutDependencyProvider Result, DP::Err> { + ) -> Result + Clone, DP::M>, DP::Err> + { self.dp.get_dependencies(p, v) } @@ -352,8 +353,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 ec496c1b..0f44b5e8 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))