From addbaf184891d66a2dfd93d241a66d13bfe5de86 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 --- examples/caching_dependency_provider.rs | 87 ------------------------- 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 | 16 +++-- tests/sat_dependency_provider.rs | 4 +- 8 files changed, 46 insertions(+), 125 deletions(-) delete mode 100644 examples/caching_dependency_provider.rs diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs deleted file mode 100644 index 2ff232ae..00000000 --- a/examples/caching_dependency_provider.rs +++ /dev/null @@ -1,87 +0,0 @@ -// SPDX-License-Identifier: MPL-2.0 - -use std::cell::RefCell; - -use pubgrub::package::Package; -use pubgrub::range::Range; -use pubgrub::solver::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider}; -use pubgrub::version::NumberVersion; -use pubgrub::version_set::VersionSet; - -type NumVS = Range; - -// An example implementing caching dependency provider that will -// store queried dependencies in memory and check them before querying more from remote. -struct CachingDependencyProvider> { - remote_dependencies: DP, - cached_dependencies: RefCell>, -} - -impl> - CachingDependencyProvider -{ - pub fn new(remote_dependencies_provider: DP) -> Self { - CachingDependencyProvider { - remote_dependencies: remote_dependencies_provider, - cached_dependencies: RefCell::new(OfflineDependencyProvider::new()), - } - } -} - -impl> DependencyProvider - for CachingDependencyProvider -{ - // Caches dependencies if they were already queried - fn get_dependencies( - &self, - package: &P, - version: &VS::V, - ) -> Result, 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) => Ok(Dependencies::Unavailable), - error @ Err(_) => error, - } - } - Ok(dependencies) => Ok(dependencies), - Err(_) => unreachable!(), - } - } - - fn choose_version(&self, package: &P, range: &VS) -> Result, DP::Err> { - self.remote_dependencies.choose_version(package, range) - } - - type Priority = DP::Priority; - - fn prioritize(&self, package: &P, range: &VS) -> Self::Priority { - self.remote_dependencies.prioritize(package, range) - } - - type Err = DP::Err; -} - -fn main() { - // Simulating remote provider locally. - let mut remote_dependencies_provider = OfflineDependencyProvider::<&str, NumVS>::new(); - - // Add dependencies as needed. Here only root package is added. - remote_dependencies_provider.add_dependencies("root", 1, Vec::new()); - - let caching_dependencies_provider = - CachingDependencyProvider::new(remote_dependencies_provider); - - let solution = resolve(&caching_dependencies_provider, "root", 1); - println!("Solution: {:?}", solution); -} diff --git a/src/internal/core.rs b/src/internal/core.rs index ec73647d..0143eb57 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::package::Package; use crate::report::DerivationTree; -use crate::type_aliases::{DependencyConstraints, Map, Set}; +use crate::type_aliases::{Map, Set}; use crate::version_set::VersionSet; /// Current state of the PubGrub algorithm. @@ -83,12 +83,12 @@ impl State { &mut self, package: P, version: VS::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(), VS::singleton(version.clone()), diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index e8648d35..5d561326 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 76752e3a..f3acf520 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -103,8 +103,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 7a6afdcd..742bda52 100644 --- a/src/solver.rs +++ b/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>( )); 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>( 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>( /// 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. @@ -261,7 +261,7 @@ pub trait DependencyProvider { &self, package: &P, version: &VS::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. @@ -285,7 +285,7 @@ pub trait DependencyProvider { )] #[cfg_attr(feature = "serde", serde(transparent))] pub struct OfflineDependencyProvider { - dependencies: Map>>, + dependencies: Map>>, } impl OfflineDependencyProvider { @@ -335,8 +335,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) } } @@ -368,11 +368,15 @@ impl DependencyProvider for OfflineDependency 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 ca12407b..2676a3cb 100644 --- a/src/type_aliases.rs +++ b/src/type_aliases.rs @@ -8,13 +8,5 @@ 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; - -/// 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 = Map; diff --git a/tests/proptest.rs b/tests/proptest.rs index 76db5a3a..ecf64756 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -35,7 +35,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) } @@ -81,7 +85,11 @@ impl TimeoutDependencyProvider { impl> DependencyProvider for TimeoutDependencyProvider { - fn get_dependencies(&self, p: &P, v: &VS::V) -> Result, DP::Err> { + fn get_dependencies( + &self, + p: &P, + v: &VS::V, + ) -> Result + Clone>, DP::Err> { self.dp.get_dependencies(p, v) } @@ -339,8 +347,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 32b8b509..10dbde09 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -71,10 +71,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))