From 86447f2a391c0aa25c56acb3a8b6ce10aac305b6 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Thu, 25 Jan 2024 08:46:57 -0600 Subject: [PATCH] Consolidate `UnusableDependencies` into a generic `Unavailable` incompatibility (#20) * Consolidate `UnusableDependencies` into a generic `Unavailable` incompatibility * Docs --- examples/caching_dependency_provider.rs | 8 ++-- examples/unsat_root_message_no_version.rs | 13 ++---- src/internal/incompatibility.rs | 33 ++++---------- src/report.rs | 54 +++++------------------ src/solver.rs | 19 ++++---- src/type_aliases.rs | 2 +- tests/proptest.rs | 12 ++--- tests/sat_dependency_provider.rs | 4 +- 8 files changed, 46 insertions(+), 99 deletions(-) diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs index c4d82e20..2ff232ae 100644 --- a/examples/caching_dependency_provider.rs +++ b/examples/caching_dependency_provider.rs @@ -39,18 +39,18 @@ impl> DependencyProvid ) -> Result, DP::Err> { let mut cache = self.cached_dependencies.borrow_mut(); match cache.get_dependencies(package, version) { - Ok(Dependencies::Unknown) => { + Ok(Dependencies::Unavailable) => { let dependencies = self.remote_dependencies.get_dependencies(package, version); match dependencies { - Ok(Dependencies::Known(dependencies)) => { + Ok(Dependencies::Available(dependencies)) => { cache.add_dependencies( package.clone(), version.clone(), dependencies.clone(), ); - Ok(Dependencies::Known(dependencies)) + Ok(Dependencies::Available(dependencies)) } - Ok(Dependencies::Unknown) => Ok(Dependencies::Unknown), + Ok(Dependencies::Unavailable) => Ok(Dependencies::Unavailable), error @ Err(_) => error, } } diff --git a/examples/unsat_root_message_no_version.rs b/examples/unsat_root_message_no_version.rs index 294ff569..6e133129 100644 --- a/examples/unsat_root_message_no_version.rs +++ b/examples/unsat_root_message_no_version.rs @@ -73,18 +73,11 @@ impl ReportFormatter> for CustomReportFormatter format!("there is no version of {package} in {set}") } } - External::UnavailableDependencies(package, set) => { + External::Unavailable(package, set, reason) => { if set == &Range::full() { - format!("dependencies of {package} are unavailable") + format!("dependencies of {package} are unavailable because {reason}") } else { - format!("dependencies of {package} at version {set} are unavailable") - } - } - External::UnusableDependencies(package, set, ..) => { - if set == &Range::full() { - format!("dependencies of {package} are unusable") - } else { - format!("dependencies of {package} at version {set} are unusable") + format!("dependencies of {package} at version {set} are unavailable because {reason}") } } External::FromDependencyOf(package, package_set, dependency, dependency_set) => { diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 233813c9..cddc2892 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -45,10 +45,8 @@ pub enum Kind { NotRoot(P, VS::V), /// There are no versions in the given range for this package. NoVersions(P, VS), - /// Dependencies of the package are unavailable for versions in that range. - UnavailableDependencies(P, VS), - /// Dependencies of the package are unusable for versions in that range. - UnusableDependencies(P, VS, Option), + /// The package is unavailable for versions in the range. A string reason is included. + Unavailable(P, VS, String), /// Incompatibility coming from the dependencies of a given package. FromDependencyOf(P, VS, P, VS), /// Derived from two causes. Stores cause ids. @@ -99,23 +97,11 @@ impl Incompatibility { /// Create an incompatibility to remember /// that a package version is not selectable - /// because its list of dependencies is unavailable. - pub fn unavailable_dependencies(package: P, version: VS::V) -> Self { + pub fn unavailable(package: P, version: VS::V, reason: String) -> Self { let set = VS::singleton(version); Self { package_terms: SmallMap::One([(package.clone(), Term::Positive(set.clone()))]), - kind: Kind::UnavailableDependencies(package, set), - } - } - - /// Create an incompatibility to remember - /// that a package version is not selectable - /// because its dependencies are not usable. - pub fn unusable_dependencies(package: P, version: VS::V, reason: Option) -> Self { - let set = VS::singleton(version); - Self { - package_terms: SmallMap::One([(package.clone(), Term::Positive(set.clone()))]), - kind: Kind::UnusableDependencies(package, set, reason), + kind: Kind::Unavailable(package, set, reason), } } @@ -259,11 +245,8 @@ impl Incompatibility { Kind::NoVersions(package, set) => { DerivationTree::External(External::NoVersions(package.clone(), set.clone())) } - Kind::UnavailableDependencies(package, set) => DerivationTree::External( - External::UnavailableDependencies(package.clone(), set.clone()), - ), - Kind::UnusableDependencies(package, set, reason) => DerivationTree::External( - External::UnusableDependencies(package.clone(), set.clone(), reason.clone()), + Kind::Unavailable(package, set, reason) => DerivationTree::External( + External::Unavailable(package.clone(), set.clone(), reason.clone()), ), Kind::FromDependencyOf(package, set, dep_package, dep_set) => { DerivationTree::External(External::FromDependencyOf( @@ -339,12 +322,12 @@ pub mod tests { let mut store = Arena::new(); let i1 = store.alloc(Incompatibility { package_terms: SmallMap::Two([("p1", t1.clone()), ("p2", t2.negate())]), - kind: Kind::UnavailableDependencies("0", Range::full()) + kind: Kind::Unavailable("0", Range::full(), "foo".to_string()) }); let i2 = store.alloc(Incompatibility { package_terms: SmallMap::Two([("p2", t2), ("p3", t3.clone())]), - kind: Kind::UnavailableDependencies("0", Range::full()) + kind: Kind::Unavailable("0", Range::full(), "bar".to_string()) }); let mut i3 = Map::default(); diff --git a/src/report.rs b/src/report.rs index 54261c17..e3ea4ec8 100644 --- a/src/report.rs +++ b/src/report.rs @@ -48,10 +48,8 @@ pub enum External { NotRoot(P, VS::V), /// There are no versions in the given set for this package. NoVersions(P, VS), - /// Dependencies of the package are unavailable for versions in that set. - UnavailableDependencies(P, VS), - /// Dependencies of the package are unusable for versions in that set. - UnusableDependencies(P, VS, Option), + /// The package is unusable in the given set. A string reason is included. + Unavailable(P, VS, String), /// Incompatibility coming from the dependencies of a given package. FromDependencyOf(P, VS, P, VS), } @@ -85,8 +83,7 @@ impl DerivationTree { } External::NoVersions(p, _) | External::NotRoot(p, _) - | External::UnavailableDependencies(p, _) - | External::UnusableDependencies(p, ..) => { + | External::Unavailable(p, ..) => { packages.insert(p); } }, @@ -146,16 +143,8 @@ impl DerivationTree { DerivationTree::External(External::NoVersions(_, r)) => Some(DerivationTree::External( External::NoVersions(package, set.union(&r)), )), - DerivationTree::External(External::UnavailableDependencies(_, r)) => Some( - DerivationTree::External(External::UnavailableDependencies(package, set.union(&r))), - ), - DerivationTree::External(External::UnusableDependencies(_, r, reason)) => { - Some(DerivationTree::External(External::UnusableDependencies( - package, - set.union(&r), - reason, - ))) - } + // Cannot be merged because the reason may not match + DerivationTree::External(External::Unavailable(_, _, _)) => None, DerivationTree::External(External::FromDependencyOf(p1, r1, p2, r2)) => { if p1 == package { Some(DerivationTree::External(External::FromDependencyOf( @@ -190,40 +179,21 @@ impl fmt::Display for External { write!(f, "there is no version of {} in {}", package, set) } } - Self::UnavailableDependencies(package, set) => { + Self::Unavailable(package, set, reason) => { if set == &VS::full() { - write!(f, "dependencies of {} are unavailable", package) + write!( + f, + "dependencies of {} are unavailable because {reason}", + package + ) } else { write!( f, - "dependencies of {} at version {} are unavailable", + "dependencies of {} at version {} are unavailable because {reason}", package, set ) } } - Self::UnusableDependencies(package, set, reason) => { - if let Some(reason) = reason { - if set == &VS::full() { - write!(f, "dependencies of {} are unusable: {reason}", package) - } else { - write!( - f, - "dependencies of {} at version {} are unusable: {reason}", - package, set - ) - } - } else { - if set == &VS::full() { - write!(f, "dependencies of {} are unusable", package) - } else { - write!( - f, - "dependencies of {} at version {} are unusable", - package, set - ) - } - } - } Self::FromDependencyOf(p, set_p, dep, set_dep) => { if set_p == &VS::full() && set_dep == &VS::full() { write!(f, "{} depends on {}", p, dep) diff --git a/src/solver.rs b/src/solver.rs index dc944960..6bb7f02a 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -158,20 +158,21 @@ pub fn resolve>( })?; let known_dependencies = match dependencies { - Dependencies::Unknown => { - state.add_incompatibility(Incompatibility::unavailable_dependencies( + Dependencies::Unavailable => { + state.add_incompatibility(Incompatibility::unavailable( p.clone(), v.clone(), + "its dependencies could not be determined".to_string(), )); continue; } - Dependencies::Known(x) if x.contains_key(p) => { + Dependencies::Available(x) if x.contains_key(p) => { return Err(PubGrubError::SelfDependency { package: p.clone(), version: v, }); } - Dependencies::Known(x) => x, + Dependencies::Available(x) => x, }; // Add that package and version if the dependencies are not problematic. @@ -201,9 +202,9 @@ pub fn resolve>( #[derive(Clone)] pub enum Dependencies { /// Package dependencies are unavailable. - Unknown, + Unavailable, /// Container for all available package versions. - Known(DependencyConstraints), + Available(DependencyConstraints), } /// Trait that allows the algorithm to retrieve available packages and their dependencies. @@ -254,7 +255,7 @@ pub trait DependencyProvider { fn choose_version(&self, package: &P, range: &VS) -> Result, Self::Err>; /// Retrieves the package dependencies. - /// Return [Dependencies::Unknown] if its dependencies are unknown. + /// Return [Dependencies::Unavailable] if its dependencies are not available. fn get_dependencies( &self, package: &P, @@ -369,8 +370,8 @@ impl DependencyProvider for OfflineDependency version: &VS::V, ) -> Result, Infallible> { Ok(match self.dependencies(package, version) { - None => Dependencies::Unknown, - Some(dependencies) => Dependencies::Known(dependencies), + None => Dependencies::Unavailable, + Some(dependencies) => Dependencies::Available(dependencies), }) } } diff --git a/src/type_aliases.rs b/src/type_aliases.rs index 11cc37c7..03db94ce 100644 --- a/src/type_aliases.rs +++ b/src/type_aliases.rs @@ -11,7 +11,7 @@ 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::Unknown](crate::solver::Dependencies::Unknown): +/// 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 24dd64a4..76db5a3a 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -309,8 +309,8 @@ fn retain_versions( continue; } let deps = match dependency_provider.get_dependencies(n, v).unwrap() { - Dependencies::Unknown => panic!(), - Dependencies::Known(deps) => deps, + Dependencies::Unavailable => panic!(), + Dependencies::Available(deps) => deps, }; smaller_dependency_provider.add_dependencies(n.clone(), v.clone(), deps) } @@ -333,8 +333,8 @@ fn retain_dependencies( for n in dependency_provider.packages() { for v in dependency_provider.versions(n).unwrap() { let deps = match dependency_provider.get_dependencies(n, v).unwrap() { - Dependencies::Unknown => panic!(), - Dependencies::Known(deps) => deps, + Dependencies::Unavailable => panic!(), + Dependencies::Available(deps) => deps, }; smaller_dependency_provider.add_dependencies( n.clone(), @@ -504,8 +504,8 @@ proptest! { .get_dependencies(package, version) .unwrap() { - Dependencies::Unknown => panic!(), - Dependencies::Known(d) => d.into_iter().collect(), + Dependencies::Unavailable => panic!(), + Dependencies::Available(d) => d.into_iter().collect(), }; if !dependencies.is_empty() { to_remove.insert((package, **version, dep_idx.get(&dependencies).0)); diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index 8aecefe9..32b8b509 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -68,8 +68,8 @@ impl SatResolve { // active packages need each of there `deps` to be satisfied for (p, v, var) in &all_versions { let deps = match dp.get_dependencies(p, v).unwrap() { - Dependencies::Unknown => panic!(), - Dependencies::Known(d) => d, + Dependencies::Unavailable => panic!(), + Dependencies::Available(d) => d, }; for (p1, range) in &deps { let empty_vec = vec![];