From 75618a077458bb26b7585d6853f987b1d1b11c7f Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 19 Oct 2023 18:07:52 +0000 Subject: [PATCH] feat: allow depending on the empty set --- src/error.rs | 15 ------- src/internal/core.rs | 10 ++--- src/internal/incompatibility.rs | 12 +++-- src/solver.rs | 25 +---------- tests/proptest.rs | 77 ++++++++++++++------------------- tests/tests.rs | 4 +- 6 files changed, 47 insertions(+), 96 deletions(-) diff --git a/src/error.rs b/src/error.rs index b0633022..d27090e2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -30,21 +30,6 @@ pub enum PubGrubError { source: Box, }, - /// Error arising when the implementer of - /// [DependencyProvider](crate::solver::DependencyProvider) - /// returned a dependency on an empty range. - /// This technically means that the package can not be selected, - /// but is clearly some kind of mistake. - #[error("Package {dependent} required by {package} {version} depends on the empty set")] - DependencyOnTheEmptySet { - /// Package whose dependencies we want. - package: P, - /// Version of the package for which we want the dependencies. - version: VS::V, - /// The dependent package that requires us to pick from the empty set. - dependent: P, - }, - /// Error arising when the implementer of /// [DependencyProvider](crate::solver::DependencyProvider) /// returned a dependency on the requested package. diff --git a/src/internal/core.rs b/src/internal/core.rs index f54ae860..973a9b8b 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -90,11 +90,6 @@ impl State { new_incompats_id_range } - /// Check if an incompatibility is terminal. - pub fn is_terminal(&self, incompatibility: &Incompatibility) -> bool { - incompatibility.is_terminal(&self.root_package, &self.root_version) - } - /// Unit propagation is the core mechanism of the solving algorithm. /// CF pub fn unit_propagation(&mut self, package: P) -> Result<(), PubGrubError> { @@ -240,7 +235,10 @@ impl State { /// It may not be trivial since those incompatibilities /// may already have derived others. fn merge_incompatibility(&mut self, id: IncompId) { - for (pkg, _term) in self.incompatibility_store[id].iter() { + for (pkg, term) in self.incompatibility_store[id].iter() { + if cfg!(debug_assertions) { + assert_ne!(term, &crate::term::Term::any()); + } self.incompatibilities .entry(pkg.clone()) .or_default() diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 93861621..b56a3c44 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -109,10 +109,14 @@ impl Incompatibility { let set1 = VS::singleton(version); let (p2, set2) = dep; Self { - package_terms: SmallMap::Two([ - (package.clone(), Term::Positive(set1.clone())), - (p2.clone(), Term::Negative(set2.clone())), - ]), + package_terms: if set2 == &VS::empty() { + SmallMap::One([(package.clone(), Term::Positive(set1.clone()))]) + } else { + SmallMap::Two([ + (package.clone(), Term::Positive(set1.clone())), + (p2.clone(), Term::Negative(set2.clone())), + ]) + }, kind: Kind::FromDependencyOf(package, set1, p2.clone(), set2.clone()), } } diff --git a/src/solver.rs b/src/solver.rs index a38d2871..e28e62de 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -178,36 +178,13 @@ pub fn resolve( version: v.clone(), }); } - Dependencies::Known(x) => { - if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&VS::empty()) { - return Err(PubGrubError::DependencyOnTheEmptySet { - package: p.clone(), - version: v.clone(), - dependent: dependent.clone(), - }); - } - - x - } + Dependencies::Known(x) => x, }; // Add that package and version if the dependencies are not problematic. let dep_incompats = state.add_incompatibility_from_dependencies(p.clone(), v.clone(), &known_dependencies); - // TODO: I don't think this check can actually happen. - // We might want to put it under #[cfg(debug_assertions)]. - let incompatible_self_dependency = state.incompatibility_store[dep_incompats.clone()] - .iter() - .any(|incompat| state.is_terminal(incompat)); - if incompatible_self_dependency { - // For a dependency incompatibility to be terminal, - // it can only mean that root depend on not root? - return Err(PubGrubError::Failure( - "Root package depends on itself at a different version?".into(), - )); - } - state.partial_solution.add_version( p.clone(), v, diff --git a/tests/proptest.rs b/tests/proptest.rs index cb66d3e6..382a6c36 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -13,7 +13,7 @@ use pubgrub::solver::{ use pubgrub::version::{NumberVersion, SemanticVersion}; use pubgrub::version_set::VersionSet; -use proptest::collection::{btree_map, vec}; +use proptest::collection::{btree_map, btree_set, vec}; use proptest::prelude::*; use proptest::sample::Index; use proptest::string::string_regex; @@ -131,20 +131,15 @@ fn string_names() -> impl Strategy { /// This strategy has a high probability of having valid dependencies pub fn registry_strategy( name: impl Strategy, - bad_name: N, ) -> impl Strategy, Vec<(N, NumberVersion)>)> { let max_crates = 40; let max_versions = 15; let shrinkage = 40; let complicated_len = 10usize; - // If this is false than the crate will depend on the nonexistent "bad" - // instead of the complex set we generated for it. - let allow_deps = prop::bool::weighted(0.99); - let a_version = ..(max_versions as u32); - let list_of_versions = btree_map(a_version, allow_deps, 1..=max_versions) + let list_of_versions = btree_set(a_version, 1..=max_versions) .prop_map(move |ver| ver.into_iter().collect::>()); let list_of_crates_with_versions = btree_map(name, list_of_versions, 1..=max_crates); @@ -177,16 +172,12 @@ pub fn registry_strategy( ) .prop_map( move |(crate_vers_by_name, raw_dependencies, reverse_alphabetical, complicated_len)| { - let mut list_of_pkgid: Vec<((N, NumberVersion), Option>)> = + let mut list_of_pkgid: Vec<((N, NumberVersion), Vec<(N, NumVS)>)> = crate_vers_by_name .iter() .flat_map(|(name, vers)| { - vers.iter().map(move |x| { - ( - (name.clone(), NumberVersion::from(x.0)), - if x.1 { Some(vec![]) } else { None }, - ) - }) + vers.iter() + .map(move |x| ((name.clone(), NumberVersion::from(x)), vec![])) }) .collect(); let len_all_pkgid = list_of_pkgid.len(); @@ -199,24 +190,24 @@ pub fn registry_strategy( } let s = &crate_vers_by_name[&dep_name]; let s_last_index = s.len() - 1; - let (c, d) = order_index(c, d, s.len()); - - if let (_, Some(deps)) = &mut list_of_pkgid[b] { - deps.push(( - dep_name, - if c == 0 && d == s_last_index { - Range::full() - } else if c == 0 { - Range::strictly_lower_than(s[d].0 + 1) - } else if d == s_last_index { - Range::higher_than(s[c].0) - } else if c == d { - Range::singleton(s[c].0) - } else { - Range::between(s[c].0, s[d].0 + 1) - }, - )) - } + let (c, d) = order_index(c, d, s.len() + 1); + + list_of_pkgid[b].1.push(( + dep_name, + if c > s_last_index { + Range::empty() + } else if c == 0 && d >= s_last_index { + Range::full() + } else if c == 0 { + Range::strictly_lower_than(s[d] + 1) + } else if d >= s_last_index { + Range::higher_than(s[c]) + } else if c == d { + Range::singleton(s[c]) + } else { + Range::between(s[c], s[d] + 1) + }, + )); } let mut dependency_provider = OfflineDependencyProvider::::new(); @@ -232,11 +223,7 @@ pub fn registry_strategy( .collect(); for ((name, ver), deps) in list_of_pkgid { - dependency_provider.add_dependencies( - name, - ver, - deps.unwrap_or_else(|| vec![(bad_name.clone(), Range::full())]), - ); + dependency_provider.add_dependencies(name, ver, deps); } (dependency_provider, complicated) @@ -252,7 +239,7 @@ fn meta_test_deep_trees_from_strategy() { let mut dis = [0; 21]; - let strategy = registry_strategy(0u16..665, 666); + let strategy = registry_strategy(0u16..665); let mut test_runner = TestRunner::deterministic(); for _ in 0..128 { let (dependency_provider, cases) = strategy @@ -299,7 +286,7 @@ proptest! { #[test] /// This test is mostly for profiling. fn prop_passes_string( - (dependency_provider, cases) in registry_strategy(string_names(), "bad".to_owned()) + (dependency_provider, cases) in registry_strategy(string_names()) ) { for (name, ver) in cases { let _ = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver); @@ -309,7 +296,7 @@ proptest! { #[test] /// This test is mostly for profiling. fn prop_passes_int( - (dependency_provider, cases) in registry_strategy(0u16..665, 666) + (dependency_provider, cases) in registry_strategy(0u16..665) ) { for (name, ver) in cases { let _ = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver); @@ -318,7 +305,7 @@ proptest! { #[test] fn prop_sat_errors_the_same( - (dependency_provider, cases) in registry_strategy(0u16..665, 666) + (dependency_provider, cases) in registry_strategy(0u16..665) ) { let mut sat = SatResolve::new(&dependency_provider); for (name, ver) in cases { @@ -333,7 +320,7 @@ proptest! { #[test] /// This tests whether the algorithm is still deterministic. fn prop_same_on_repeated_runs( - (dependency_provider, cases) in registry_strategy(0u16..665, 666) + (dependency_provider, cases) in registry_strategy(0u16..665) ) { for (name, ver) in cases { let one = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver); @@ -355,7 +342,7 @@ proptest! { /// [ReverseDependencyProvider] changes what order the candidates /// are tried but not the existence of a solution. fn prop_reversed_version_errors_the_same( - (dependency_provider, cases) in registry_strategy(0u16..665, 666) + (dependency_provider, cases) in registry_strategy(0u16..665) ) { let reverse_provider = OldestVersionsDependencyProvider(dependency_provider.clone()); for (name, ver) in cases { @@ -371,7 +358,7 @@ proptest! { #[test] fn prop_removing_a_dep_cant_break( - (dependency_provider, cases) in registry_strategy(0u16..665, 666), + (dependency_provider, cases) in registry_strategy(0u16..665), indexes_to_remove in prop::collection::vec((any::(), any::(), any::()), 1..10) ) { let packages: Vec<_> = dependency_provider.packages().collect(); @@ -423,7 +410,7 @@ proptest! { #[test] fn prop_limited_independence_of_irrelevant_alternatives( - (dependency_provider, cases) in registry_strategy(0u16..665, 666), + (dependency_provider, cases) in registry_strategy(0u16..665), indexes_to_remove in prop::collection::vec(any::(), 1..10) ) { let all_versions: Vec<(u16, NumberVersion)> = dependency_provider diff --git a/tests/tests.rs b/tests/tests.rs index b1d05a4a..81fd5324 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -35,13 +35,13 @@ fn should_always_find_a_satisfier() { dependency_provider.add_dependencies("a", 0, [("b", Range::empty())]); assert!(matches!( resolve(&dependency_provider, "a", 0), - Err(PubGrubError::DependencyOnTheEmptySet { .. }) + Err(PubGrubError::NoSolution { .. }) )); dependency_provider.add_dependencies("c", 0, [("a", Range::full())]); assert!(matches!( resolve(&dependency_provider, "c", 0), - Err(PubGrubError::DependencyOnTheEmptySet { .. }) + Err(PubGrubError::NoSolution { .. }) )); }