Skip to content

Commit

Permalink
allow depending on the empty set
Browse files Browse the repository at this point in the history
  • Loading branch information
@Eh2406
Eh2406 committed Oct 4, 2023
1 parent cf18239 commit 596620b
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 96 deletions.
15 changes: 0 additions & 15 deletions src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,6 @@ pub enum PubGrubError<P: Package, VS: VersionSet> {
source: Box<dyn std::error::Error>,
},

/// 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.
Expand Down
10 changes: 4 additions & 6 deletions src/internal/core.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,6 @@ impl<P: Package, VS: VersionSet> State<P, VS> {
new_incompats_id_range
}

/// Check if an incompatibility is terminal.
pub fn is_terminal(&self, incompatibility: &Incompatibility<P, VS>) -> bool {
incompatibility.is_terminal(&self.root_package, &self.root_version)
}

/// Unit propagation is the core mechanism of the solving algorithm.
/// CF <https://github.com/dart-lang/pub/blob/master/doc/solver.md#unit-propagation>
pub fn unit_propagation(&mut self, package: P) -> Result<(), PubGrubError<P, VS>> {
Expand Down Expand Up @@ -240,7 +235,10 @@ impl<P: Package, VS: VersionSet> State<P, VS> {
/// It may not be trivial since those incompatibilities
/// may already have derived others.
fn merge_incompatibility(&mut self, id: IncompId<P, VS>) {
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()
Expand Down
12 changes: 8 additions & 4 deletions src/internal/incompatibility.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,14 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> {
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()),
}
}
Expand Down
25 changes: 1 addition & 24 deletions src/solver.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,36 +178,13 @@ pub fn resolve<P: Package, VS: VersionSet>(
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,
Expand Down
77 changes: 32 additions & 45 deletions tests/proptest.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -123,20 +123,15 @@ fn string_names() -> impl Strategy<Value = String> {
/// This strategy has a high probability of having valid dependencies
pub fn registry_strategy<N: Package + Ord>(
name: impl Strategy<Value = N>,
bad_name: N,
) -> impl Strategy<Value = (OfflineDependencyProvider<N, NumVS>, 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::<Vec<_>>());

let list_of_crates_with_versions = btree_map(name, list_of_versions, 1..=max_crates);
Expand Down Expand Up @@ -169,16 +164,12 @@ pub fn registry_strategy<N: Package + Ord>(
)
.prop_map(
move |(crate_vers_by_name, raw_dependencies, reverse_alphabetical, complicated_len)| {
let mut list_of_pkgid: Vec<((N, NumberVersion), Option<Vec<(N, NumVS)>>)> =
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();
Expand All @@ -191,24 +182,24 @@ pub fn registry_strategy<N: Package + Ord>(
}
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::<N, NumVS>::new();
Expand All @@ -224,11 +215,7 @@ pub fn registry_strategy<N: Package + Ord>(
.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)
Expand All @@ -244,7 +231,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
Expand Down Expand Up @@ -291,7 +278,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);
Expand All @@ -301,7 +288,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);
Expand All @@ -310,7 +297,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 {
Expand All @@ -325,7 +312,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);
Expand All @@ -347,7 +334,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 {
Expand All @@ -363,7 +350,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::<prop::sample::Index>(), any::<prop::sample::Index>(), any::<prop::sample::Index>()), 1..10)
) {
let packages: Vec<_> = dependency_provider.packages().collect();
Expand Down Expand Up @@ -415,7 +402,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::<prop::sample::Index>(), 1..10)
) {
let all_versions: Vec<(u16, NumberVersion)> = dependency_provider
Expand Down
4 changes: 2 additions & 2 deletions tests/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 { .. })
));
}

Expand Down

0 comments on commit 596620b

Please sign in to comment.