From dabc20e189d8e948d37fb812992e1093edf9484b Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sun, 11 Jul 2021 04:32:56 +0200 Subject: [PATCH 01/20] docs: release manual process (#100) --- release.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 release.md diff --git a/release.md b/release.md new file mode 100644 index 00000000..fdbd1a1f --- /dev/null +++ b/release.md @@ -0,0 +1,28 @@ +# Creation of a new release + +This is taking the 0.2.1 release as an example. + +## GitHub stuff + +- Checkout the prep-v0.2.1 branch +- Update the release date in the changelog and push to the PR. +- Squash merge the PR to the dev branch +- Check that the merged PRĀ is passing the tests on the dev branch +- Pull the updated dev locally +- Switch to the release branch +- Merge locally dev into release in fast-forward mode, we want to keep the history of commits and the merge point. +- `git tag -a v0.2.1 -m "v0.2.1: mostly perf improvements"` +- (Optional) cryptographically sign the tag +- On GitHub, edit the branch protection setting for release: uncheck include admin, and save +- Push release to github: git push --follow-tags +- Reset the release branch protection to include admins +- On GitHub, create a release from that tag. + +## Crates.io stuff + +- `cargo publish --dry-run` +- `cargo publish` + +## Community stuff + +Talk about the awesome new features of the new release online. From a2d2c65b4cbca1d10f12a397e51cb96ace0ca22e Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Mon, 5 Jul 2021 16:30:11 -0700 Subject: [PATCH 02/20] docs: more detailed msg in "cant happen" branch of satisfier (#103) Example: thread 'main' panicked at 'internal error: entered unreachable code: while processing package trio: accum_term = 0.19 isn't a subset of incompat_term = 0.19.0, which means the last assignment should have been a decision, but instead it was a derivation. This shouldn't be possible! (Maybe your Version ordering is broken?)', /home/njs/src/pubgrub/src/internal/partial_solution.rs:411:17 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Fixes: gh-102 --- src/internal/partial_solution.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index ad454244..84bfc5f8 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -407,7 +407,16 @@ impl PackageAssignments { self.highest_decision_level, ), AssignmentsIntersection::Derivations(_) => { - panic!("This must be a decision") + unreachable!( + concat!( + "while processing package {}: ", + "accum_term = {} isn't a subset of incompat_term = {}, ", + "which means the last assignment should have been a decision, ", + "but instead it was a derivation. This shouldn't be possible! ", + "(Maybe your Version ordering is broken?)" + ), + package, accum_term, incompat_term + ) } } } From 7f3ae14f2cbb55b6dbeb19bd2b487eef45b7280f Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sun, 8 Aug 2021 10:12:34 +0200 Subject: [PATCH 03/20] refactor: make clippy happy (#106) --- src/internal/incompatibility.rs | 4 +-- src/internal/partial_solution.rs | 2 +- src/solver.rs | 59 ++++++++++++++++---------------- src/term.rs | 2 +- 4 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index acf900b8..b1d44d5f 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -152,7 +152,7 @@ impl Incompatibility { false } else { let (package, term) = self.package_terms.iter().next().unwrap(); - (package == root_package) && term.contains(&root_version) + (package == root_package) && term.contains(root_version) } } @@ -220,7 +220,7 @@ impl<'a, P: Package, V: Version + 'a> Incompatibility { pub fn relation(&self, terms: impl Fn(&P) -> Option<&'a Term>) -> Relation

{ let mut relation = Relation::Satisfied; for (package, incompat_term) in self.package_terms.iter() { - match terms(package).map(|term| incompat_term.relation_with(&term)) { + match terms(package).map(|term| incompat_term.relation_with(term)) { Some(term::Relation::Satisfied) => {} Some(term::Relation::Contradicted) => { return Relation::Contradicted(package.clone()); diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 84bfc5f8..6d6a7ca3 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -223,7 +223,7 @@ impl PartialSolution { pa.dated_derivations .iter() .fold(Term::any(), |acc, dated_derivation| { - let term = store[dated_derivation.cause].get(&p).unwrap().negate(); + let term = store[dated_derivation.cause].get(p).unwrap().negate(); acc.intersection(&term) }), ); diff --git a/src/solver.rs b/src/solver.rs index 62015911..40366bb6 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -137,38 +137,37 @@ pub fn resolve( { // Retrieve that package dependencies. let p = &next; - let dependencies = - match dependency_provider - .get_dependencies(&p, &v) - .map_err(|err| PubGrubError::ErrorRetrievingDependencies { - package: p.clone(), - version: v.clone(), - source: err, - })? { - Dependencies::Unknown => { - state.add_incompatibility(Incompatibility::unavailable_dependencies( - p.clone(), - v.clone(), - )); - continue; + let dependencies = match dependency_provider.get_dependencies(p, &v).map_err(|err| { + PubGrubError::ErrorRetrievingDependencies { + package: p.clone(), + version: v.clone(), + source: err, + } + })? { + Dependencies::Unknown => { + state.add_incompatibility(Incompatibility::unavailable_dependencies( + p.clone(), + v.clone(), + )); + continue; + } + Dependencies::Known(x) => { + if x.contains_key(p) { + return Err(PubGrubError::SelfDependency { + package: p.clone(), + version: v.clone(), + }); } - Dependencies::Known(x) => { - if x.contains_key(&p) { - return Err(PubGrubError::SelfDependency { - package: p.clone(), - version: v.clone(), - }); - } - if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&Range::none()) { - return Err(PubGrubError::DependencyOnTheEmptySet { - package: p.clone(), - version: v.clone(), - dependent: dependent.clone(), - }); - } - x + if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&Range::none()) { + return Err(PubGrubError::DependencyOnTheEmptySet { + package: p.clone(), + version: v.clone(), + dependent: dependent.clone(), + }); } - }; + x + } + }; // Add that package and version if the dependencies are not problematic. let dep_incompats = diff --git a/src/term.rs b/src/term.rs index bc038acf..ad8158fd 100644 --- a/src/term.rs +++ b/src/term.rs @@ -162,7 +162,7 @@ impl<'a, V: 'a + Version> Term { impl AsRef> for Term { fn as_ref(&self) -> &Term { - &self + self } } From 27c1da7740a776a5c49e242b6d67dfaf3c73391d Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 21 Aug 2021 23:10:32 +0200 Subject: [PATCH 04/20] feat: logging to help debugging (#107) * feat: add logging to help debugging * debug: impl Display for partial solution * fix cherry picking * Fix display of assignments in partial_solution * debug: nits Co-authored-by: Jacob Finkelman --- Cargo.toml | 2 ++ src/internal/core.rs | 6 ++++ src/internal/partial_solution.rs | 62 ++++++++++++++++++++++++++++++++ src/solver.rs | 7 ++++ tests/examples.rs | 16 +++++++++ 5 files changed, 93 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 1f6b32fb..a796249e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,12 +23,14 @@ include = ["Cargo.toml", "LICENSE", "README.md", "src/**", "tests/**", "examples thiserror = "1.0" rustc-hash = "1.1.0" serde = { version = "1.0", features = ["derive"], optional = true } +log = "0.4.14" # for debug logs in tests [dev-dependencies] proptest = "0.10.1" ron = "0.6" varisat = "0.2.2" criterion = "0.3" +env_logger = "0.9.0" [[bench]] name = "large_case" diff --git a/src/internal/core.rs b/src/internal/core.rs index f923850a..c59472d5 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -115,6 +115,10 @@ impl State { // If the partial solution satisfies the incompatibility // we must perform conflict resolution. Relation::Satisfied => { + log::info!( + "Start conflict resolution because incompat satisfied:\n {}", + current_incompat + ); conflict_id = Some(incompat_id); break; } @@ -183,6 +187,7 @@ impl State { current_incompat_changed, previous_satisfier_level, ); + log::info!("backtrack to {:?}", previous_satisfier_level); return Ok((package, current_incompat_id)); } SameDecisionLevels { satisfier_cause } => { @@ -192,6 +197,7 @@ impl State { &package, &self.incompatibility_store, ); + log::info!("prior cause: {}", prior_cause); current_incompat_id = self.incompatibility_store.alloc(prior_cause); current_incompat_changed = true; } diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 6d6a7ca3..bc8e6885 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -3,6 +3,8 @@ //! A Memory acts like a structured partial solution //! where terms are regrouped by package in a [Map](crate::type_aliases::Map). +use std::fmt::Display; + use crate::internal::arena::Arena; use crate::internal::incompatibility::{IncompId, Incompatibility, Relation}; use crate::internal::small_map::SmallMap; @@ -32,6 +34,24 @@ pub struct PartialSolution { package_assignments: Map>, } +impl Display for PartialSolution { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut assignments: Vec<_> = self + .package_assignments + .iter() + .map(|(p, pa)| format!("{}: {}", p, pa)) + .collect(); + assignments.sort(); + write!( + f, + "next_global_index: {}\ncurrent_decision_level: {:?}\npackage_assignements:\n{}", + self.next_global_index, + self.current_decision_level, + assignments.join("\t\n") + ) + } +} + /// Package assignments contain the potential decision and derivations /// that have already been made for a given package, /// as well as the intersection of terms by all of these. @@ -43,6 +63,24 @@ struct PackageAssignments { assignments_intersection: AssignmentsIntersection, } +impl Display for PackageAssignments { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let derivations: Vec<_> = self + .dated_derivations + .iter() + .map(|dd| dd.to_string()) + .collect(); + write!( + f, + "decision range: {:?}..{:?}\nderivations:\n {}\n,assignments_intersection: {}", + self.smallest_decision_level, + self.highest_decision_level, + derivations.join("\n "), + self.assignments_intersection + ) + } +} + #[derive(Clone, Debug)] pub struct DatedDerivation { global_index: u32, @@ -50,12 +88,29 @@ pub struct DatedDerivation { cause: IncompId, } +impl Display for DatedDerivation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{:?}, cause: {:?}", self.decision_level, self.cause) + } +} + #[derive(Clone, Debug)] enum AssignmentsIntersection { Decision((u32, V, Term)), Derivations(Term), } +impl Display for AssignmentsIntersection { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Decision((lvl, version, _)) => { + write!(f, "Decision: level {}, v = {}", lvl, version) + } + Self::Derivations(term) => write!(f, "Derivations term: {}", term), + } + } +} + #[derive(Clone, Debug)] pub enum SatisfierSearch { DifferentDecisionLevels { @@ -258,7 +313,14 @@ impl PartialSolution { // Check none of the dependencies (new_incompatibilities) // would create a conflict (be satisfied). if store[new_incompatibilities].iter().all(not_satisfied) { + log::info!("add_decision: {} @ {}", package, version); self.add_decision(package, version); + } else { + log::info!( + "not adding {} @ {} because of its dependencies", + package, + version + ); } } diff --git a/src/solver.rs b/src/solver.rs index 40366bb6..4e360a56 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -92,7 +92,12 @@ pub fn resolve( .should_cancel() .map_err(|err| PubGrubError::ErrorInShouldCancel(err))?; + log::info!("unit_propagation: {}", &next); state.unit_propagation(next)?; + log::debug!( + "Partial solution after unit propagation: {}", + state.partial_solution + ); let potential_packages = state.partial_solution.potential_packages(); if potential_packages.is_none() { @@ -109,6 +114,7 @@ pub fn resolve( let decision = dependency_provider .choose_package_version(potential_packages.unwrap()) .map_err(PubGrubError::ErrorChoosingPackageVersion)?; + log::info!("DP chose: {} @ {:?}", decision.0, decision.1); next = decision.0.clone(); // Pick the next compatible version. @@ -194,6 +200,7 @@ pub fn resolve( } else { // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied // terms and can add the decision directly. + log::info!("add_decision (not first time): {} @ {}", &next, v); state.partial_solution.add_decision(next.clone(), v); } } diff --git a/tests/examples.rs b/tests/examples.rs index 29901caf..1add530e 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -5,9 +5,21 @@ use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::type_aliases::Map; use pubgrub::version::{NumberVersion, SemanticVersion}; +use log::LevelFilter; +use std::io::Write; + +fn init_log() { + let _ = env_logger::builder() + .filter_level(LevelFilter::Trace) + .format(|buf, record| writeln!(buf, "{}", record.args())) + .is_test(true) + .try_init(); +} + #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#no-conflicts fn no_conflict() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( @@ -38,6 +50,7 @@ fn no_conflict() { #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#avoiding-conflict-during-decision-making fn avoiding_conflict_during_decision_making() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( @@ -73,6 +86,7 @@ fn avoiding_conflict_during_decision_making() { #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#performing-conflict-resolution fn conflict_resolution() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( @@ -106,6 +120,7 @@ fn conflict_resolution() { #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#conflict-resolution-with-a-partial-satisfier fn conflict_with_partial_satisfier() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); #[rustfmt::skip] // root 1.0.0 depends on foo ^1.0.0 and target ^2.0.0 @@ -171,6 +186,7 @@ fn conflict_with_partial_satisfier() { /// /// Solution: a0, b0, c0, d0 fn double_choices() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); dependency_provider.add_dependencies("a", 0, vec![("b", Range::any()), ("c", Range::any())]); dependency_provider.add_dependencies("b", 0, vec![("d", Range::exact(0))]); From ca782bc83480ede22465d8ad7d5ca9689e41c6f0 Mon Sep 17 00:00:00 2001 From: Marcin Puc <5671049+tranzystorek-io@users.noreply.github.com> Date: Mon, 23 Aug 2021 15:45:03 +0200 Subject: [PATCH 05/20] refactor: use arrays to provide dependency lists (#109) --- examples/branching_error_reporting.rs | 18 ++++---- examples/caching_dependency_provider.rs | 2 +- examples/doc_interface.rs | 8 ++-- examples/doc_interface_error.rs | 34 +++++++-------- examples/doc_interface_semantic.rs | 28 ++++++------ examples/linear_error_reporting.rs | 10 ++--- src/lib.rs | 8 ++-- tests/examples.rs | 58 ++++++++++++------------- tests/proptest.rs | 8 ++-- tests/sat_dependency_provider.rs | 2 +- tests/tests.rs | 16 +++---- 11 files changed, 96 insertions(+), 96 deletions(-) diff --git a/examples/branching_error_reporting.rs b/examples/branching_error_reporting.rs index c1daa1bf..9d258dcc 100644 --- a/examples/branching_error_reporting.rs +++ b/examples/branching_error_reporting.rs @@ -13,13 +13,13 @@ fn main() { // root 1.0.0 depends on foo ^1.0.0 dependency_provider.add_dependencies( "root", (1, 0, 0), - vec![("foo", Range::between((1, 0, 0), (2, 0, 0)))], + [("foo", Range::between((1, 0, 0), (2, 0, 0)))], ); #[rustfmt::skip] // foo 1.0.0 depends on a ^1.0.0 and b ^1.0.0 dependency_provider.add_dependencies( "foo", (1, 0, 0), - vec![ + [ ("a", Range::between((1, 0, 0), (2, 0, 0))), ("b", Range::between((1, 0, 0), (2, 0, 0))), ], @@ -28,7 +28,7 @@ fn main() { // foo 1.1.0 depends on x ^1.0.0 and y ^1.0.0 dependency_provider.add_dependencies( "foo", (1, 1, 0), - vec![ + [ ("x", Range::between((1, 0, 0), (2, 0, 0))), ("y", Range::between((1, 0, 0), (2, 0, 0))), ], @@ -37,20 +37,20 @@ fn main() { // a 1.0.0 depends on b ^2.0.0 dependency_provider.add_dependencies( "a", (1, 0, 0), - vec![("b", Range::between((2, 0, 0), (3, 0, 0)))], + [("b", Range::between((2, 0, 0), (3, 0, 0)))], ); // b 1.0.0 and 2.0.0 have no dependencies. - dependency_provider.add_dependencies("b", (1, 0, 0), vec![]); - dependency_provider.add_dependencies("b", (2, 0, 0), vec![]); + dependency_provider.add_dependencies("b", (1, 0, 0), []); + dependency_provider.add_dependencies("b", (2, 0, 0), []); #[rustfmt::skip] // x 1.0.0 depends on y ^2.0.0. dependency_provider.add_dependencies( "x", (1, 0, 0), - vec![("y", Range::between((2, 0, 0), (3, 0, 0)))], + [("y", Range::between((2, 0, 0), (3, 0, 0)))], ); // y 1.0.0 and 2.0.0 have no dependencies. - dependency_provider.add_dependencies("y", (1, 0, 0), vec![]); - dependency_provider.add_dependencies("y", (2, 0, 0), vec![]); + dependency_provider.add_dependencies("y", (1, 0, 0), []); + dependency_provider.add_dependencies("y", (2, 0, 0), []); // Run the algorithm. match resolve(&dependency_provider, "root", (1, 0, 0)) { diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs index bac730ea..003e2519 100644 --- a/examples/caching_dependency_provider.rs +++ b/examples/caching_dependency_provider.rs @@ -49,7 +49,7 @@ impl> DependencyProvider::new(); dependency_provider.add_dependencies( - "root", 1, vec![("menu", Range::any()), ("icons", Range::any())], + "root", 1, [("menu", Range::any()), ("icons", Range::any())], ); - dependency_provider.add_dependencies("menu", 1, vec![("dropdown", Range::any())]); - dependency_provider.add_dependencies("dropdown", 1, vec![("icons", Range::any())]); - dependency_provider.add_dependencies("icons", 1, vec![]); + dependency_provider.add_dependencies("menu", 1, [("dropdown", Range::any())]); + dependency_provider.add_dependencies("dropdown", 1, [("icons", Range::any())]); + dependency_provider.add_dependencies("icons", 1, []); // Run the algorithm. let solution = resolve(&dependency_provider, "root", 1); diff --git a/examples/doc_interface_error.rs b/examples/doc_interface_error.rs index 0ef0f1ec..0d98f257 100644 --- a/examples/doc_interface_error.rs +++ b/examples/doc_interface_error.rs @@ -17,57 +17,57 @@ use pubgrub::version::SemanticVersion; fn main() { let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); // Direct dependencies: menu and icons. - dependency_provider.add_dependencies("root", (1, 0, 0), vec![ + dependency_provider.add_dependencies("root", (1, 0, 0), [ ("menu", Range::any()), ("icons", Range::exact((1, 0, 0))), ("intl", Range::exact((5, 0, 0))), ]); // Dependencies of the menu lib. - dependency_provider.add_dependencies("menu", (1, 0, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 0, 0), [ ("dropdown", Range::strictly_lower_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 1, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 1, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 2, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 2, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 3, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 3, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 4, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 4, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 5, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 5, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); // Dependencies of the dropdown lib. - dependency_provider.add_dependencies("dropdown", (1, 8, 0), vec![ + dependency_provider.add_dependencies("dropdown", (1, 8, 0), [ ("intl", Range::exact((3, 0, 0))), ]); - dependency_provider.add_dependencies("dropdown", (2, 0, 0), vec![ + dependency_provider.add_dependencies("dropdown", (2, 0, 0), [ ("icons", Range::exact((2, 0, 0))), ]); - dependency_provider.add_dependencies("dropdown", (2, 1, 0), vec![ + dependency_provider.add_dependencies("dropdown", (2, 1, 0), [ ("icons", Range::exact((2, 0, 0))), ]); - dependency_provider.add_dependencies("dropdown", (2, 2, 0), vec![ + dependency_provider.add_dependencies("dropdown", (2, 2, 0), [ ("icons", Range::exact((2, 0, 0))), ]); - dependency_provider.add_dependencies("dropdown", (2, 3, 0), vec![ + dependency_provider.add_dependencies("dropdown", (2, 3, 0), [ ("icons", Range::exact((2, 0, 0))), ]); // Icons have no dependencies. - dependency_provider.add_dependencies("icons", (1, 0, 0), vec![]); - dependency_provider.add_dependencies("icons", (2, 0, 0), vec![]); + dependency_provider.add_dependencies("icons", (1, 0, 0), []); + dependency_provider.add_dependencies("icons", (2, 0, 0), []); // Intl have no dependencies. - dependency_provider.add_dependencies("intl", (3, 0, 0), vec![]); - dependency_provider.add_dependencies("intl", (4, 0, 0), vec![]); - dependency_provider.add_dependencies("intl", (5, 0, 0), vec![]); + dependency_provider.add_dependencies("intl", (3, 0, 0), []); + dependency_provider.add_dependencies("intl", (4, 0, 0), []); + dependency_provider.add_dependencies("intl", (5, 0, 0), []); // Run the algorithm. match resolve(&dependency_provider, "root", (1, 0, 0)) { diff --git a/examples/doc_interface_semantic.rs b/examples/doc_interface_semantic.rs index b4c352e1..d284aaa8 100644 --- a/examples/doc_interface_semantic.rs +++ b/examples/doc_interface_semantic.rs @@ -16,49 +16,49 @@ use pubgrub::version::SemanticVersion; fn main() { let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); // Direct dependencies: menu and icons. - dependency_provider.add_dependencies("root", (1, 0, 0), vec![ + dependency_provider.add_dependencies("root", (1, 0, 0), [ ("menu", Range::any()), ("icons", Range::exact((1, 0, 0))), ]); // Dependencies of the menu lib. - dependency_provider.add_dependencies("menu", (1, 0, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 0, 0), [ ("dropdown", Range::strictly_lower_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 1, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 1, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 2, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 2, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 3, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 3, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 4, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 4, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); - dependency_provider.add_dependencies("menu", (1, 5, 0), vec![ + dependency_provider.add_dependencies("menu", (1, 5, 0), [ ("dropdown", Range::higher_than((2, 0, 0))), ]); // Dependencies of the dropdown lib. - dependency_provider.add_dependencies("dropdown", (1, 8, 0), vec![]); - dependency_provider.add_dependencies("dropdown", (2, 0, 0), vec![ + dependency_provider.add_dependencies("dropdown", (1, 8, 0), []); + dependency_provider.add_dependencies("dropdown", (2, 0, 0), [ ("icons", Range::exact((2, 0, 0))), ]); - dependency_provider.add_dependencies("dropdown", (2, 1, 0), vec![ + dependency_provider.add_dependencies("dropdown", (2, 1, 0), [ ("icons", Range::exact((2, 0, 0))), ]); - dependency_provider.add_dependencies("dropdown", (2, 2, 0), vec![ + dependency_provider.add_dependencies("dropdown", (2, 2, 0), [ ("icons", Range::exact((2, 0, 0))), ]); - dependency_provider.add_dependencies("dropdown", (2, 3, 0), vec![ + dependency_provider.add_dependencies("dropdown", (2, 3, 0), [ ("icons", Range::exact((2, 0, 0))), ]); // Icons has no dependency. - dependency_provider.add_dependencies("icons", (1, 0, 0), vec![]); - dependency_provider.add_dependencies("icons", (2, 0, 0), vec![]); + dependency_provider.add_dependencies("icons", (1, 0, 0), []); + dependency_provider.add_dependencies("icons", (2, 0, 0), []); // Run the algorithm. match resolve(&dependency_provider, "root", (1, 0, 0)) { diff --git a/examples/linear_error_reporting.rs b/examples/linear_error_reporting.rs index 0673fe36..fa38c538 100644 --- a/examples/linear_error_reporting.rs +++ b/examples/linear_error_reporting.rs @@ -13,7 +13,7 @@ fn main() { // root 1.0.0 depends on foo ^1.0.0 and baz ^1.0.0 dependency_provider.add_dependencies( "root", (1, 0, 0), - vec![ + [ ("foo", Range::between((1, 0, 0), (2, 0, 0))), ("baz", Range::between((1, 0, 0), (2, 0, 0))), ], @@ -22,17 +22,17 @@ fn main() { // foo 1.0.0 depends on bar ^2.0.0 dependency_provider.add_dependencies( "foo", (1, 0, 0), - vec![("bar", Range::between((2, 0, 0), (3, 0, 0)))], + [("bar", Range::between((2, 0, 0), (3, 0, 0)))], ); #[rustfmt::skip] // bar 2.0.0 depends on baz ^3.0.0 dependency_provider.add_dependencies( "bar", (2, 0, 0), - vec![("baz", Range::between((3, 0, 0), (4, 0, 0)))], + [("baz", Range::between((3, 0, 0), (4, 0, 0)))], ); // baz 1.0.0 and 3.0.0 have no dependencies - dependency_provider.add_dependencies("baz", (1, 0, 0), vec![]); - dependency_provider.add_dependencies("baz", (3, 0, 0), vec![]); + dependency_provider.add_dependencies("baz", (1, 0, 0), []); + dependency_provider.add_dependencies("baz", (3, 0, 0), []); // Run the algorithm. match resolve(&dependency_provider, "root", (1, 0, 0)) { diff --git a/src/lib.rs b/src/lib.rs index 7a6e1737..40b61032 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -53,11 +53,11 @@ //! let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); //! //! dependency_provider.add_dependencies( -//! "root", 1, vec![("menu", Range::any()), ("icons", Range::any())], +//! "root", 1, [("menu", Range::any()), ("icons", Range::any())], //! ); -//! dependency_provider.add_dependencies("menu", 1, vec![("dropdown", Range::any())]); -//! dependency_provider.add_dependencies("dropdown", 1, vec![("icons", Range::any())]); -//! dependency_provider.add_dependencies("icons", 1, vec![]); +//! dependency_provider.add_dependencies("menu", 1, [("dropdown", Range::any())]); +//! dependency_provider.add_dependencies("dropdown", 1, [("icons", Range::any())]); +//! dependency_provider.add_dependencies("icons", 1, []); //! //! // Run the algorithm. //! let solution = resolve(&dependency_provider, "root", 1).unwrap(); diff --git a/tests/examples.rs b/tests/examples.rs index 1add530e..5d66db79 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -24,15 +24,15 @@ fn no_conflict() { #[rustfmt::skip] dependency_provider.add_dependencies( "root", (1, 0, 0), - vec![("foo", Range::between((1, 0, 0), (2, 0, 0)))], + [("foo", Range::between((1, 0, 0), (2, 0, 0)))], ); #[rustfmt::skip] dependency_provider.add_dependencies( "foo", (1, 0, 0), - vec![("bar", Range::between((1, 0, 0), (2, 0, 0)))], + [("bar", Range::between((1, 0, 0), (2, 0, 0)))], ); - dependency_provider.add_dependencies("bar", (1, 0, 0), vec![]); - dependency_provider.add_dependencies("bar", (2, 0, 0), vec![]); + dependency_provider.add_dependencies("bar", (1, 0, 0), []); + dependency_provider.add_dependencies("bar", (2, 0, 0), []); // Run the algorithm. let computed_solution = resolve(&dependency_provider, "root", (1, 0, 0)).unwrap(); @@ -55,7 +55,7 @@ fn avoiding_conflict_during_decision_making() { #[rustfmt::skip] dependency_provider.add_dependencies( "root", (1, 0, 0), - vec![ + [ ("foo", Range::between((1, 0, 0), (2, 0, 0))), ("bar", Range::between((1, 0, 0), (2, 0, 0))), ], @@ -63,12 +63,12 @@ fn avoiding_conflict_during_decision_making() { #[rustfmt::skip] dependency_provider.add_dependencies( "foo", (1, 1, 0), - vec![("bar", Range::between((2, 0, 0), (3, 0, 0)))], + [("bar", Range::between((2, 0, 0), (3, 0, 0)))], ); - dependency_provider.add_dependencies("foo", (1, 0, 0), vec![]); - dependency_provider.add_dependencies("bar", (1, 0, 0), vec![]); - dependency_provider.add_dependencies("bar", (1, 1, 0), vec![]); - dependency_provider.add_dependencies("bar", (2, 0, 0), vec![]); + dependency_provider.add_dependencies("foo", (1, 0, 0), []); + dependency_provider.add_dependencies("bar", (1, 0, 0), []); + dependency_provider.add_dependencies("bar", (1, 1, 0), []); + dependency_provider.add_dependencies("bar", (2, 0, 0), []); // Run the algorithm. let computed_solution = resolve(&dependency_provider, "root", (1, 0, 0)).unwrap(); @@ -91,18 +91,18 @@ fn conflict_resolution() { #[rustfmt::skip] dependency_provider.add_dependencies( "root", (1, 0, 0), - vec![("foo", Range::higher_than((1, 0, 0)))], + [("foo", Range::higher_than((1, 0, 0)))], ); #[rustfmt::skip] dependency_provider.add_dependencies( "foo", (2, 0, 0), - vec![("bar", Range::between((1, 0, 0), (2, 0, 0)))], + [("bar", Range::between((1, 0, 0), (2, 0, 0)))], ); - dependency_provider.add_dependencies("foo", (1, 0, 0), vec![]); + dependency_provider.add_dependencies("foo", (1, 0, 0), []); #[rustfmt::skip] dependency_provider.add_dependencies( "bar", (1, 0, 0), - vec![("foo", Range::between((1, 0, 0), (2, 0, 0)))], + [("foo", Range::between((1, 0, 0), (2, 0, 0)))], ); // Run the algorithm. @@ -126,7 +126,7 @@ fn conflict_with_partial_satisfier() { // root 1.0.0 depends on foo ^1.0.0 and target ^2.0.0 dependency_provider.add_dependencies( "root", (1, 0, 0), - vec![ + [ ("foo", Range::between((1, 0, 0), (2, 0, 0))), ("target", Range::between((2, 0, 0), (3, 0, 0))), ], @@ -135,33 +135,33 @@ fn conflict_with_partial_satisfier() { // foo 1.1.0 depends on left ^1.0.0 and right ^1.0.0 dependency_provider.add_dependencies( "foo", (1, 1, 0), - vec![ + [ ("left", Range::between((1, 0, 0), (2, 0, 0))), ("right", Range::between((1, 0, 0), (2, 0, 0))), ], ); - dependency_provider.add_dependencies("foo", (1, 0, 0), vec![]); + dependency_provider.add_dependencies("foo", (1, 0, 0), []); #[rustfmt::skip] // left 1.0.0 depends on shared >=1.0.0 dependency_provider.add_dependencies( "left", (1, 0, 0), - vec![("shared", Range::higher_than((1, 0, 0)))], + [("shared", Range::higher_than((1, 0, 0)))], ); #[rustfmt::skip] // right 1.0.0 depends on shared <2.0.0 dependency_provider.add_dependencies( "right", (1, 0, 0), - vec![("shared", Range::strictly_lower_than((2, 0, 0)))], + [("shared", Range::strictly_lower_than((2, 0, 0)))], ); - dependency_provider.add_dependencies("shared", (2, 0, 0), vec![]); + dependency_provider.add_dependencies("shared", (2, 0, 0), []); #[rustfmt::skip] // shared 1.0.0 depends on target ^1.0.0 dependency_provider.add_dependencies( "shared", (1, 0, 0), - vec![("target", Range::between((1, 0, 0), (2, 0, 0)))], + [("target", Range::between((1, 0, 0), (2, 0, 0)))], ); - dependency_provider.add_dependencies("target", (2, 0, 0), vec![]); - dependency_provider.add_dependencies("target", (1, 0, 0), vec![]); + dependency_provider.add_dependencies("target", (2, 0, 0), []); + dependency_provider.add_dependencies("target", (1, 0, 0), []); // Run the algorithm. let computed_solution = resolve(&dependency_provider, "root", (1, 0, 0)).unwrap(); @@ -188,12 +188,12 @@ fn conflict_with_partial_satisfier() { fn double_choices() { init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); - dependency_provider.add_dependencies("a", 0, vec![("b", Range::any()), ("c", Range::any())]); - dependency_provider.add_dependencies("b", 0, vec![("d", Range::exact(0))]); - dependency_provider.add_dependencies("b", 1, vec![("d", Range::exact(1))]); - dependency_provider.add_dependencies("c", 0, vec![]); - dependency_provider.add_dependencies("c", 1, vec![("d", Range::exact(2))]); - dependency_provider.add_dependencies("d", 0, vec![]); + dependency_provider.add_dependencies("a", 0, [("b", Range::any()), ("c", Range::any())]); + dependency_provider.add_dependencies("b", 0, [("d", Range::exact(0))]); + dependency_provider.add_dependencies("b", 1, [("d", Range::exact(1))]); + dependency_provider.add_dependencies("c", 0, []); + dependency_provider.add_dependencies("c", 1, [("d", Range::exact(2))]); + dependency_provider.add_dependencies("d", 0, []); // Solution. let mut expected_solution = Map::default(); diff --git a/tests/proptest.rs b/tests/proptest.rs index d2da3730..637d8ff1 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -89,7 +89,7 @@ impl> DependencyProvider::new(); - dependency_provider.add_dependencies(0, 0, vec![(666, Range::any())]); + dependency_provider.add_dependencies(0, 0, [(666, Range::any())]); // Run the algorithm. let _ = resolve( @@ -333,8 +333,8 @@ proptest! { (Ok(l), Ok(r)) => assert_eq!(l, r), (Err(PubGrubError::NoSolution(derivation_l)), Err(PubGrubError::NoSolution(derivation_r))) => { prop_assert_eq!( - DefaultStringReporter::report(&derivation_l), - DefaultStringReporter::report(&derivation_r) + DefaultStringReporter::report(derivation_l), + DefaultStringReporter::report(derivation_r) )}, _ => panic!("not the same result") } @@ -423,7 +423,7 @@ proptest! { dependency_provider .versions(&p) .unwrap() - .map(move |v| (p, v.clone())) + .map(move |&v| (p, v)) }) .collect(); let to_remove: Set<(_, _)> = indexes_to_remove.iter().map(|x| x.get(&all_versions)).cloned().collect(); diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index fa964ca8..1a21d5f0 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -82,7 +82,7 @@ impl SatResolve { 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)) diff --git a/tests/tests.rs b/tests/tests.rs index 7aeed03b..d9bf2d06 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -9,12 +9,12 @@ use pubgrub::version::NumberVersion; fn same_result_on_repeated_runs() { let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); - dependency_provider.add_dependencies("c", 0, vec![]); - dependency_provider.add_dependencies("c", 2, vec![]); - dependency_provider.add_dependencies("b", 0, vec![]); - dependency_provider.add_dependencies("b", 1, vec![("c", Range::between(0, 1))]); + dependency_provider.add_dependencies("c", 0, []); + dependency_provider.add_dependencies("c", 2, []); + dependency_provider.add_dependencies("b", 0, []); + dependency_provider.add_dependencies("b", 1, [("c", Range::between(0, 1))]); - dependency_provider.add_dependencies("a", 0, vec![("b", Range::any()), ("c", Range::any())]); + dependency_provider.add_dependencies("a", 0, [("b", Range::any()), ("c", Range::any())]); let name = "a"; let ver = NumberVersion(0); @@ -30,13 +30,13 @@ fn same_result_on_repeated_runs() { #[test] fn should_always_find_a_satisfier() { let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); - dependency_provider.add_dependencies("a", 0, vec![("b", Range::none())]); + dependency_provider.add_dependencies("a", 0, [("b", Range::none())]); assert!(matches!( resolve(&dependency_provider, "a", 0), Err(PubGrubError::DependencyOnTheEmptySet { .. }) )); - dependency_provider.add_dependencies("c", 0, vec![("a", Range::any())]); + dependency_provider.add_dependencies("c", 0, [("a", Range::any())]); assert!(matches!( resolve(&dependency_provider, "c", 0), Err(PubGrubError::DependencyOnTheEmptySet { .. }) @@ -46,7 +46,7 @@ fn should_always_find_a_satisfier() { #[test] fn cannot_depend_on_self() { let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); - dependency_provider.add_dependencies("a", 0, vec![("a", Range::any())]); + dependency_provider.add_dependencies("a", 0, [("a", Range::any())]); assert!(matches!( resolve(&dependency_provider, "a", 0), Err(PubGrubError::SelfDependency { .. }) From c597f74c9db8172ebd60f8406ab94ed9b6155742 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 19 May 2022 13:50:58 -0400 Subject: [PATCH 06/20] feat: from and as RangeBounds (#105) --- examples/branching_error_reporting.rs | 14 +++--- examples/doc_interface_error.rs | 12 ++--- examples/doc_interface_semantic.rs | 12 ++--- examples/linear_error_reporting.rs | 8 ++-- src/range.rs | 64 +++++++++++++++++++++++++++ src/version.rs | 29 ++++++++++++ 6 files changed, 116 insertions(+), 23 deletions(-) diff --git a/examples/branching_error_reporting.rs b/examples/branching_error_reporting.rs index 9d258dcc..2b43f8e1 100644 --- a/examples/branching_error_reporting.rs +++ b/examples/branching_error_reporting.rs @@ -13,15 +13,15 @@ fn main() { // root 1.0.0 depends on foo ^1.0.0 dependency_provider.add_dependencies( "root", (1, 0, 0), - [("foo", Range::between((1, 0, 0), (2, 0, 0)))], + [("foo", Range::from_range_bounds((1, 0, 0)..(2, 0, 0)))], ); #[rustfmt::skip] // foo 1.0.0 depends on a ^1.0.0 and b ^1.0.0 dependency_provider.add_dependencies( "foo", (1, 0, 0), [ - ("a", Range::between((1, 0, 0), (2, 0, 0))), - ("b", Range::between((1, 0, 0), (2, 0, 0))), + ("a", Range::from_range_bounds((1, 0, 0)..(2, 0, 0))), + ("b", Range::from_range_bounds((1, 0, 0)..(2, 0, 0))), ], ); #[rustfmt::skip] @@ -29,15 +29,15 @@ fn main() { dependency_provider.add_dependencies( "foo", (1, 1, 0), [ - ("x", Range::between((1, 0, 0), (2, 0, 0))), - ("y", Range::between((1, 0, 0), (2, 0, 0))), + ("x", Range::from_range_bounds((1, 0, 0)..(2, 0, 0))), + ("y", Range::from_range_bounds((1, 0, 0)..(2, 0, 0))), ], ); #[rustfmt::skip] // a 1.0.0 depends on b ^2.0.0 dependency_provider.add_dependencies( "a", (1, 0, 0), - [("b", Range::between((2, 0, 0), (3, 0, 0)))], + [("b", Range::from_range_bounds((2, 0, 0)..(3, 0, 0)))], ); // b 1.0.0 and 2.0.0 have no dependencies. dependency_provider.add_dependencies("b", (1, 0, 0), []); @@ -46,7 +46,7 @@ fn main() { // x 1.0.0 depends on y ^2.0.0. dependency_provider.add_dependencies( "x", (1, 0, 0), - [("y", Range::between((2, 0, 0), (3, 0, 0)))], + [("y", Range::from_range_bounds((2, 0, 0)..(3, 0, 0)))], ); // y 1.0.0 and 2.0.0 have no dependencies. dependency_provider.add_dependencies("y", (1, 0, 0), []); diff --git a/examples/doc_interface_error.rs b/examples/doc_interface_error.rs index 0d98f257..990a0f19 100644 --- a/examples/doc_interface_error.rs +++ b/examples/doc_interface_error.rs @@ -25,22 +25,22 @@ fn main() { // Dependencies of the menu lib. dependency_provider.add_dependencies("menu", (1, 0, 0), [ - ("dropdown", Range::strictly_lower_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds(..(2, 0, 0))), ]); dependency_provider.add_dependencies("menu", (1, 1, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); dependency_provider.add_dependencies("menu", (1, 2, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); dependency_provider.add_dependencies("menu", (1, 3, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); dependency_provider.add_dependencies("menu", (1, 4, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); dependency_provider.add_dependencies("menu", (1, 5, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); // Dependencies of the dropdown lib. diff --git a/examples/doc_interface_semantic.rs b/examples/doc_interface_semantic.rs index d284aaa8..2a8331ed 100644 --- a/examples/doc_interface_semantic.rs +++ b/examples/doc_interface_semantic.rs @@ -23,22 +23,22 @@ fn main() { // Dependencies of the menu lib. dependency_provider.add_dependencies("menu", (1, 0, 0), [ - ("dropdown", Range::strictly_lower_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds(..(2, 0, 0))), ]); dependency_provider.add_dependencies("menu", (1, 1, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); dependency_provider.add_dependencies("menu", (1, 2, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); dependency_provider.add_dependencies("menu", (1, 3, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); dependency_provider.add_dependencies("menu", (1, 4, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); dependency_provider.add_dependencies("menu", (1, 5, 0), [ - ("dropdown", Range::higher_than((2, 0, 0))), + ("dropdown", Range::from_range_bounds((2, 0, 0)..)), ]); // Dependencies of the dropdown lib. diff --git a/examples/linear_error_reporting.rs b/examples/linear_error_reporting.rs index fa38c538..195ff388 100644 --- a/examples/linear_error_reporting.rs +++ b/examples/linear_error_reporting.rs @@ -14,21 +14,21 @@ fn main() { dependency_provider.add_dependencies( "root", (1, 0, 0), [ - ("foo", Range::between((1, 0, 0), (2, 0, 0))), - ("baz", Range::between((1, 0, 0), (2, 0, 0))), + ("foo", Range::from_range_bounds((1, 0, 0)..(2, 0, 0))), + ("baz", Range::from_range_bounds((1, 0, 0)..(2, 0, 0))), ], ); #[rustfmt::skip] // foo 1.0.0 depends on bar ^2.0.0 dependency_provider.add_dependencies( "foo", (1, 0, 0), - [("bar", Range::between((2, 0, 0), (3, 0, 0)))], + [("bar", Range::from_range_bounds((2, 0, 0)..(3, 0, 0)))], ); #[rustfmt::skip] // bar 2.0.0 depends on baz ^3.0.0 dependency_provider.add_dependencies( "bar", (2, 0, 0), - [("baz", Range::between((3, 0, 0), (4, 0, 0)))], + [("baz", Range::from_range_bounds((3, 0, 0)..(4, 0, 0)))], ); // baz 1.0.0 and 3.0.0 have no dependencies dependency_provider.add_dependencies("baz", (1, 0, 0), []); diff --git a/src/range.rs b/src/range.rs index 8de8b3ff..5b1f9bbf 100644 --- a/src/range.rs +++ b/src/range.rs @@ -16,6 +16,7 @@ use std::cmp::Ordering; use std::fmt; +use std::ops::{Bound, RangeBounds}; use crate::internal::small_vec::SmallVec; use crate::version::Version; @@ -85,6 +86,31 @@ impl Range { Self::none() } } + + /// Construct a simple range from anything that impls [RangeBounds] like `v1..v2`. + pub fn from_range_bounds(bounds: R) -> Self + where + R: RangeBounds, + for<'a> &'a IV: Into, + { + let start = match bounds.start_bound() { + Bound::Included(s) => s.into(), + Bound::Excluded(s) => s.into().bump(), + Bound::Unbounded => V::lowest(), + }; + let end = match bounds.end_bound() { + Bound::Included(e) => Some(e.into().bump()), + Bound::Excluded(e) => Some(e.into()), + Bound::Unbounded => None, + }; + if end.is_some() && end.as_ref() <= Some(&start) { + Self::none() + } else { + Self { + segments: SmallVec::one((start, end)), + } + } + } } // Set operations. @@ -260,6 +286,24 @@ impl Range { pub fn lowest_version(&self) -> Option { self.segments.first().map(|(start, _)| start).cloned() } + + /// Convert to something that can be used with + /// [BTreeMap::range](std::collections::BTreeMap::range). + /// All versions contained in self, will be in the output, + /// but there may be versions in the output that are not contained in self. + /// Returns None if the range is empty. + pub fn bounding_range(&self) -> Option<(Bound<&V>, Bound<&V>)> { + self.segments.first().map(|(start, _)| { + let end = { + self.segments + .last() + .and_then(|(_, l)| l.as_ref()) + .map(Bound::Excluded) + .unwrap_or(Bound::Unbounded) + }; + (Bound::Included(start), end) + }) + } } // REPORT ###################################################################### @@ -405,5 +449,25 @@ pub mod tests { fn contains_intersection(range in strategy(), version in version_strat()) { assert_eq!(range.contains(&version), range.intersection(&Range::exact(version)) != Range::none()); } + + #[test] + fn contains_bounding_range(range in strategy(), version in version_strat()) { + if range.contains(&version) { + assert!(range.bounding_range().map(|b| b.contains(&version)).unwrap_or(false)); + } + } + + #[test] + fn from_range_bounds(range in any::<(Bound, Bound)>(), version in version_strat()) { + let rv: Range = Range::from_range_bounds(range); + assert_eq!(range.contains(&version.0), rv.contains(&version)); + } + + #[test] + fn from_range_bounds_round_trip(range in any::<(Bound, Bound)>()) { + let rv: Range = Range::from_range_bounds(range); + let rv2: Range = rv.bounding_range().map(Range::from_range_bounds::<_, NumberVersion>).unwrap_or_else(Range::none); + assert_eq!(rv, rv2); + } } } diff --git a/src/version.rs b/src/version.rs index c7d749ee..bf0524b4 100644 --- a/src/version.rs +++ b/src/version.rs @@ -80,6 +80,21 @@ impl From<(u32, u32, u32)> for SemanticVersion { } } +// Convert a &(major, minor, patch) into a version. +impl From<&(u32, u32, u32)> for SemanticVersion { + fn from(tuple: &(u32, u32, u32)) -> Self { + let (major, minor, patch) = *tuple; + Self::new(major, minor, patch) + } +} + +// Convert an &version into a version. +impl From<&SemanticVersion> for SemanticVersion { + fn from(v: &SemanticVersion) -> Self { + *v + } +} + // Convert a version into a tuple (major, minor, patch). impl From for (u32, u32, u32) { fn from(v: SemanticVersion) -> Self { @@ -237,6 +252,20 @@ impl From for NumberVersion { } } +// Convert an &usize into a version. +impl From<&u32> for NumberVersion { + fn from(v: &u32) -> Self { + Self(*v) + } +} + +// Convert an &version into a version. +impl From<&NumberVersion> for NumberVersion { + fn from(v: &NumberVersion) -> Self { + *v + } +} + // Convert a version into an usize. impl From for u32 { fn from(version: NumberVersion) -> Self { From f9c82386958d7b711c7c09ebef8d11bb5b252510 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 24 May 2022 17:12:37 +0200 Subject: [PATCH 07/20] feat: use a VersionSet trait instead of Range (#108) * refactor: introduce a trait for sets of versions * refactor: port report and incompatibility to version set * refactor: port errors to version set * refactor: port partial solution to version set * refactor: move DependencyConstraints to type_aliases * refactor: port core to version set * refactor: port solver to version set * refactor: replace old modules with ones based on version_set * refactor: update tests to version_set * feat: add serde bounds to OfflineDependencyProvider * refactor: update proptest.rs to VersionSet * docs: fix links * refactor: allow clippy type_complexity * Small docs changes --- examples/branching_error_reporting.rs | 4 +- examples/caching_dependency_provider.rs | 27 +++-- examples/doc_interface.rs | 4 +- examples/doc_interface_error.rs | 4 +- examples/doc_interface_semantic.rs | 4 +- examples/linear_error_reporting.rs | 4 +- src/error.rs | 12 +- src/internal/core.rs | 45 ++++--- src/internal/incompatibility.rs | 86 +++++++------- src/internal/partial_solution.rs | 93 +++++++-------- src/lib.rs | 26 ++-- src/range.rs | 23 ++++ src/report.rs | 152 ++++++++++++------------ src/solver.rs | 100 ++++++++-------- src/term.rs | 74 ++++++------ src/type_aliases.rs | 9 +- src/version_set.rs | 60 ++++++++++ tests/examples.rs | 13 +- tests/proptest.rs | 77 ++++++------ tests/sat_dependency_provider.rs | 16 +-- tests/tests.rs | 8 +- 21 files changed, 477 insertions(+), 364 deletions(-) create mode 100644 src/version_set.rs diff --git a/examples/branching_error_reporting.rs b/examples/branching_error_reporting.rs index 2b43f8e1..d4dfb719 100644 --- a/examples/branching_error_reporting.rs +++ b/examples/branching_error_reporting.rs @@ -6,9 +6,11 @@ use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::version::SemanticVersion; +type SemVS = Range; + // https://github.com/dart-lang/pub/blob/master/doc/solver.md#branching-error-reporting fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); #[rustfmt::skip] // root 1.0.0 depends on foo ^1.0.0 dependency_provider.add_dependencies( diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs index 003e2519..cb278942 100644 --- a/examples/caching_dependency_provider.rs +++ b/examples/caching_dependency_provider.rs @@ -6,16 +6,21 @@ use std::error::Error; use pubgrub::package::Package; use pubgrub::range::Range; use pubgrub::solver::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider}; -use pubgrub::version::{NumberVersion, Version}; +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> { +struct CachingDependencyProvider> { remote_dependencies: DP, - cached_dependencies: RefCell>, + cached_dependencies: RefCell>, } -impl> CachingDependencyProvider { +impl> + CachingDependencyProvider +{ pub fn new(remote_dependencies_provider: DP) -> Self { CachingDependencyProvider { remote_dependencies: remote_dependencies_provider, @@ -24,13 +29,13 @@ impl> CachingDependencyProv } } -impl> DependencyProvider - for CachingDependencyProvider +impl> DependencyProvider + for CachingDependencyProvider { - fn choose_package_version, U: std::borrow::Borrow>>( + fn choose_package_version, U: std::borrow::Borrow>( &self, packages: impl Iterator, - ) -> Result<(T, Option), Box> { + ) -> Result<(T, Option), Box> { self.remote_dependencies.choose_package_version(packages) } @@ -38,8 +43,8 @@ impl> DependencyProvider Result, Box> { + version: &VS::V, + ) -> Result, Box> { let mut cache = self.cached_dependencies.borrow_mut(); match cache.get_dependencies(package, version) { Ok(Dependencies::Unknown) => { @@ -65,7 +70,7 @@ impl> DependencyProvider::new(); + 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()); diff --git a/examples/doc_interface.rs b/examples/doc_interface.rs index bddef439..d409dcce 100644 --- a/examples/doc_interface.rs +++ b/examples/doc_interface.rs @@ -4,13 +4,15 @@ use pubgrub::range::Range; use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::version::NumberVersion; +type NumVS = Range; + // `root` depends on `menu` and `icons` // `menu` depends on `dropdown` // `dropdown` depends on `icons` // `icons` has no dependency #[rustfmt::skip] fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, NumVS>::new(); dependency_provider.add_dependencies( "root", 1, [("menu", Range::any()), ("icons", Range::any())], ); diff --git a/examples/doc_interface_error.rs b/examples/doc_interface_error.rs index 990a0f19..a3d7e61e 100644 --- a/examples/doc_interface_error.rs +++ b/examples/doc_interface_error.rs @@ -6,6 +6,8 @@ use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::version::SemanticVersion; +type SemVS = Range; + // `root` depends on `menu`, `icons 1.0.0` and `intl 5.0.0` // `menu 1.0.0` depends on `dropdown < 2.0.0` // `menu >= 1.1.0` depends on `dropdown >= 2.0.0` @@ -15,7 +17,7 @@ use pubgrub::version::SemanticVersion; // `intl` has no dependency #[rustfmt::skip] fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); // Direct dependencies: menu and icons. dependency_provider.add_dependencies("root", (1, 0, 0), [ ("menu", Range::any()), diff --git a/examples/doc_interface_semantic.rs b/examples/doc_interface_semantic.rs index 2a8331ed..cce059bc 100644 --- a/examples/doc_interface_semantic.rs +++ b/examples/doc_interface_semantic.rs @@ -6,6 +6,8 @@ use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::version::SemanticVersion; +type SemVS = Range; + // `root` depends on `menu` and `icons 1.0.0` // `menu 1.0.0` depends on `dropdown < 2.0.0` // `menu >= 1.1.0` depends on `dropdown >= 2.0.0` @@ -14,7 +16,7 @@ use pubgrub::version::SemanticVersion; // `icons` has no dependency #[rustfmt::skip] fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); // Direct dependencies: menu and icons. dependency_provider.add_dependencies("root", (1, 0, 0), [ ("menu", Range::any()), diff --git a/examples/linear_error_reporting.rs b/examples/linear_error_reporting.rs index 195ff388..8624fe2a 100644 --- a/examples/linear_error_reporting.rs +++ b/examples/linear_error_reporting.rs @@ -6,9 +6,11 @@ use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::version::SemanticVersion; +type SemVS = Range; + // https://github.com/dart-lang/pub/blob/master/doc/solver.md#linear-error-reporting fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); #[rustfmt::skip] // root 1.0.0 depends on foo ^1.0.0 and baz ^1.0.0 dependency_provider.add_dependencies( diff --git a/src/error.rs b/src/error.rs index 0553d8de..1098706c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -6,14 +6,14 @@ use thiserror::Error; use crate::package::Package; use crate::report::DerivationTree; -use crate::version::Version; +use crate::version_set::VersionSet; /// Errors that may occur while solving dependencies. #[derive(Error, Debug)] -pub enum PubGrubError { +pub enum PubGrubError { /// There is no solution for this set of dependencies. #[error("No solution")] - NoSolution(DerivationTree), + NoSolution(DerivationTree), /// Error arising when the implementer of /// [DependencyProvider](crate::solver::DependencyProvider) @@ -24,7 +24,7 @@ pub enum PubGrubError { /// Package whose dependencies we want. package: P, /// Version of the package for which we want the dependencies. - version: V, + version: VS::V, /// Error raised by the implementer of /// [DependencyProvider](crate::solver::DependencyProvider). source: Box, @@ -40,7 +40,7 @@ pub enum PubGrubError { /// Package whose dependencies we want. package: P, /// Version of the package for which we want the dependencies. - version: V, + version: VS::V, /// The dependent package that requires us to pick from the empty set. dependent: P, }, @@ -55,7 +55,7 @@ pub enum PubGrubError { /// Package whose dependencies we want. package: P, /// Version of the package for which we want the dependencies. - version: V, + version: VS::V, }, /// Error arising when the implementer of diff --git a/src/internal/core.rs b/src/internal/core.rs index c59472d5..f54ae860 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -15,28 +15,27 @@ use crate::internal::partial_solution::{DecisionLevel, PartialSolution}; use crate::internal::small_vec::SmallVec; use crate::package::Package; use crate::report::DerivationTree; -use crate::solver::DependencyConstraints; -use crate::type_aliases::Map; -use crate::version::Version; +use crate::type_aliases::{DependencyConstraints, Map}; +use crate::version_set::VersionSet; /// Current state of the PubGrub algorithm. #[derive(Clone)] -pub struct State { +pub struct State { root_package: P, - root_version: V, + root_version: VS::V, - incompatibilities: Map>>, + incompatibilities: Map>>, /// Store the ids of incompatibilities that are already contradicted /// and will stay that way until the next conflict and backtrack is operated. - contradicted_incompatibilities: rustc_hash::FxHashSet>, + contradicted_incompatibilities: rustc_hash::FxHashSet>, /// Partial solution. /// TODO: remove pub. - pub partial_solution: PartialSolution, + pub partial_solution: PartialSolution, /// The store is the reference storage for all incompatibilities. - pub incompatibility_store: Arena>, + pub incompatibility_store: Arena>, /// This is a stack of work to be done in `unit_propagation`. /// It can definitely be a local variable to that method, but @@ -44,9 +43,9 @@ pub struct State { unit_propagation_buffer: SmallVec

, } -impl State { +impl State { /// Initialization of PubGrub state. - pub fn init(root_package: P, root_version: V) -> Self { + pub fn init(root_package: P, root_version: VS::V) -> Self { let mut incompatibility_store = Arena::new(); let not_root_id = incompatibility_store.alloc(Incompatibility::not_root( root_package.clone(), @@ -66,7 +65,7 @@ impl State { } /// Add an incompatibility to the state. - pub fn add_incompatibility(&mut self, incompat: Incompatibility) { + pub fn add_incompatibility(&mut self, incompat: Incompatibility) { let id = self.incompatibility_store.alloc(incompat); self.merge_incompatibility(id); } @@ -75,9 +74,9 @@ impl State { pub fn add_incompatibility_from_dependencies( &mut self, package: P, - version: V, - deps: &DependencyConstraints, - ) -> std::ops::Range> { + version: VS::V, + deps: &DependencyConstraints, + ) -> std::ops::Range> { // Create incompatibilities and allocate them in the store. let new_incompats_id_range = self .incompatibility_store @@ -92,13 +91,13 @@ impl State { } /// Check if an incompatibility is terminal. - pub fn is_terminal(&self, incompatibility: &Incompatibility) -> bool { + 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> { + pub fn unit_propagation(&mut self, package: P) -> Result<(), PubGrubError> { self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package); while let Some(current_package) = self.unit_propagation_buffer.pop() { @@ -162,8 +161,8 @@ impl State { /// CF fn conflict_resolution( &mut self, - incompatibility: IncompId, - ) -> Result<(P, IncompId), PubGrubError> { + incompatibility: IncompId, + ) -> Result<(P, IncompId), PubGrubError> { let mut current_incompat_id = incompatibility; let mut current_incompat_changed = false; loop { @@ -209,7 +208,7 @@ impl State { /// Backtracking. fn backtrack( &mut self, - incompat: IncompId, + incompat: IncompId, incompat_changed: bool, decision_level: DecisionLevel, ) { @@ -240,7 +239,7 @@ impl State { /// Here we do the simple stupid thing of just growing the Vec. /// It may not be trivial since those incompatibilities /// may already have derived others. - fn merge_incompatibility(&mut self, id: IncompId) { + fn merge_incompatibility(&mut self, id: IncompId) { for (pkg, _term) in self.incompatibility_store[id].iter() { self.incompatibilities .entry(pkg.clone()) @@ -251,12 +250,12 @@ impl State { // Error reporting ######################################################### - fn build_derivation_tree(&self, incompat: IncompId) -> DerivationTree { + fn build_derivation_tree(&self, incompat: IncompId) -> DerivationTree { let shared_ids = self.find_shared_ids(incompat); Incompatibility::build_derivation_tree(incompat, &shared_ids, &self.incompatibility_store) } - fn find_shared_ids(&self, incompat: IncompId) -> Set> { + fn find_shared_ids(&self, incompat: IncompId) -> Set> { let mut all_ids = Set::new(); let mut shared_ids = Set::new(); let mut stack = vec![incompat]; diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index b1d44d5f..dd093a08 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -9,10 +9,9 @@ use std::fmt; use crate::internal::arena::{Arena, Id}; use crate::internal::small_map::SmallMap; use crate::package::Package; -use crate::range::Range; use crate::report::{DefaultStringReporter, DerivationTree, Derived, External}; use crate::term::{self, Term}; -use crate::version::Version; +use crate::version_set::VersionSet; /// An incompatibility is a set of terms for different packages /// that should never be satisfied all together. @@ -30,26 +29,26 @@ use crate::version::Version; /// during conflict resolution. More about all this in /// [PubGrub documentation](https://github.com/dart-lang/pub/blob/master/doc/solver.md#incompatibility). #[derive(Debug, Clone)] -pub struct Incompatibility { - package_terms: SmallMap>, - kind: Kind, +pub struct Incompatibility { + package_terms: SmallMap>, + kind: Kind, } /// Type alias of unique identifiers for incompatibilities. -pub type IncompId = Id>; +pub type IncompId = Id>; #[derive(Debug, Clone)] -enum Kind { +enum Kind { /// Initial incompatibility aiming at picking the root package for the first decision. - NotRoot(P, V), + NotRoot(P, VS::V), /// There are no versions in the given range for this package. - NoVersions(P, Range), + NoVersions(P, VS), /// Dependencies of the package are unavailable for versions in that range. - UnavailableDependencies(P, Range), + UnavailableDependencies(P, VS), /// Incompatibility coming from the dependencies of a given package. - FromDependencyOf(P, Range, P, Range), + FromDependencyOf(P, VS, P, VS), /// Derived from two causes. Stores cause ids. - DerivedFrom(IncompId, IncompId), + DerivedFrom(IncompId, IncompId), } /// A Relation describes how a set of terms can be compared to an incompatibility. @@ -69,52 +68,52 @@ pub enum Relation { Inconclusive, } -impl Incompatibility { +impl Incompatibility { /// Create the initial "not Root" incompatibility. - pub fn not_root(package: P, version: V) -> Self { + pub fn not_root(package: P, version: VS::V) -> Self { Self { package_terms: SmallMap::One([( package.clone(), - Term::Negative(Range::exact(version.clone())), + Term::Negative(VS::singleton(version.clone())), )]), kind: Kind::NotRoot(package, version), } } /// Create an incompatibility to remember - /// that a given range does not contain any version. - pub fn no_versions(package: P, term: Term) -> Self { - let range = match &term { + /// that a given set does not contain any version. + pub fn no_versions(package: P, term: Term) -> Self { + let set = match &term { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), }; Self { package_terms: SmallMap::One([(package.clone(), term)]), - kind: Kind::NoVersions(package, range), + kind: Kind::NoVersions(package, set), } } /// 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: V) -> Self { - let range = Range::exact(version); + pub fn unavailable_dependencies(package: P, version: VS::V) -> Self { + let set = VS::singleton(version); Self { - package_terms: SmallMap::One([(package.clone(), Term::Positive(range.clone()))]), - kind: Kind::UnavailableDependencies(package, range), + package_terms: SmallMap::One([(package.clone(), Term::Positive(set.clone()))]), + kind: Kind::UnavailableDependencies(package, set), } } /// Build an incompatibility from a given dependency. - pub fn from_dependency(package: P, version: V, dep: (&P, &Range)) -> Self { - let range1 = Range::exact(version); - let (p2, range2) = dep; + pub fn from_dependency(package: P, version: VS::V, dep: (&P, &VS)) -> Self { + let set1 = VS::singleton(version); + let (p2, set2) = dep; Self { package_terms: SmallMap::Two([ - (package.clone(), Term::Positive(range1.clone())), - (p2.clone(), Term::Negative(range2.clone())), + (package.clone(), Term::Positive(set1.clone())), + (p2.clone(), Term::Negative(set2.clone())), ]), - kind: Kind::FromDependencyOf(package, range1, p2.clone(), range2.clone()), + kind: Kind::FromDependencyOf(package, set1, p2.clone(), set2.clone()), } } @@ -145,7 +144,7 @@ impl Incompatibility { /// Check if an incompatibility should mark the end of the algorithm /// because it satisfies the root package. - pub fn is_terminal(&self, root_package: &P, root_version: &V) -> bool { + pub fn is_terminal(&self, root_package: &P, root_version: &VS::V) -> bool { if self.package_terms.len() == 0 { true } else if self.package_terms.len() > 1 { @@ -157,12 +156,12 @@ impl Incompatibility { } /// Get the term related to a given package (if it exists). - pub fn get(&self, package: &P) -> Option<&Term> { + pub fn get(&self, package: &P) -> Option<&Term> { self.package_terms.get(package) } /// Iterate over packages. - pub fn iter(&self) -> impl Iterator)> { + pub fn iter(&self) -> impl Iterator)> { self.package_terms.iter() } @@ -181,7 +180,7 @@ impl Incompatibility { self_id: Id, shared_ids: &Set>, store: &Arena, - ) -> DerivationTree { + ) -> DerivationTree { match &store[self_id].kind { Kind::DerivedFrom(id1, id2) => { let cause1 = Self::build_derivation_tree(*id1, shared_ids, store); @@ -197,27 +196,27 @@ impl Incompatibility { Kind::NotRoot(package, version) => { DerivationTree::External(External::NotRoot(package.clone(), version.clone())) } - Kind::NoVersions(package, range) => { - DerivationTree::External(External::NoVersions(package.clone(), range.clone())) + Kind::NoVersions(package, set) => { + DerivationTree::External(External::NoVersions(package.clone(), set.clone())) } - Kind::UnavailableDependencies(package, range) => DerivationTree::External( - External::UnavailableDependencies(package.clone(), range.clone()), + Kind::UnavailableDependencies(package, set) => DerivationTree::External( + External::UnavailableDependencies(package.clone(), set.clone()), ), - Kind::FromDependencyOf(package, range, dep_package, dep_range) => { + Kind::FromDependencyOf(package, set, dep_package, dep_set) => { DerivationTree::External(External::FromDependencyOf( package.clone(), - range.clone(), + set.clone(), dep_package.clone(), - dep_range.clone(), + dep_set.clone(), )) } } } } -impl<'a, P: Package, V: Version + 'a> Incompatibility { +impl<'a, P: Package, VS: VersionSet + 'a> Incompatibility { /// CF definition of Relation enum. - pub fn relation(&self, terms: impl Fn(&P) -> Option<&'a Term>) -> Relation

{ + pub fn relation(&self, terms: impl Fn(&P) -> Option<&'a Term>) -> Relation

{ let mut relation = Relation::Satisfied; for (package, incompat_term) in self.package_terms.iter() { match terms(package).map(|term| incompat_term.relation_with(term)) { @@ -243,7 +242,7 @@ impl<'a, P: Package, V: Version + 'a> Incompatibility { } } -impl fmt::Display for Incompatibility { +impl fmt::Display for Incompatibility { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( f, @@ -258,6 +257,7 @@ impl fmt::Display for Incompatibility { #[cfg(test)] pub mod tests { use super::*; + use crate::range::Range; use crate::term::tests::strategy as term_strat; use crate::type_aliases::Map; use proptest::prelude::*; diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index bc8e6885..23960a45 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -9,10 +9,9 @@ use crate::internal::arena::Arena; use crate::internal::incompatibility::{IncompId, Incompatibility, Relation}; use crate::internal::small_map::SmallMap; use crate::package::Package; -use crate::range::Range; use crate::term::Term; use crate::type_aliases::{Map, SelectedDependencies}; -use crate::version::Version; +use crate::version_set::VersionSet; use super::small_vec::SmallVec; @@ -28,13 +27,13 @@ impl DecisionLevel { /// The partial solution contains all package assignments, /// organized by package and historically ordered. #[derive(Clone, Debug)] -pub struct PartialSolution { +pub struct PartialSolution { next_global_index: u32, current_decision_level: DecisionLevel, - package_assignments: Map>, + package_assignments: Map>, } -impl Display for PartialSolution { +impl Display for PartialSolution { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let mut assignments: Vec<_> = self .package_assignments @@ -56,14 +55,14 @@ impl Display for PartialSolution { /// that have already been made for a given package, /// as well as the intersection of terms by all of these. #[derive(Clone, Debug)] -struct PackageAssignments { +struct PackageAssignments { smallest_decision_level: DecisionLevel, highest_decision_level: DecisionLevel, - dated_derivations: SmallVec>, - assignments_intersection: AssignmentsIntersection, + dated_derivations: SmallVec>, + assignments_intersection: AssignmentsIntersection, } -impl Display for PackageAssignments { +impl Display for PackageAssignments { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let derivations: Vec<_> = self .dated_derivations @@ -82,25 +81,25 @@ impl Display for PackageAssignments { } #[derive(Clone, Debug)] -pub struct DatedDerivation { +pub struct DatedDerivation { global_index: u32, decision_level: DecisionLevel, - cause: IncompId, + cause: IncompId, } -impl Display for DatedDerivation { +impl Display for DatedDerivation { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{:?}, cause: {:?}", self.decision_level, self.cause) } } #[derive(Clone, Debug)] -enum AssignmentsIntersection { - Decision((u32, V, Term)), - Derivations(Term), +enum AssignmentsIntersection { + Decision((u32, VS::V, Term)), + Derivations(Term), } -impl Display for AssignmentsIntersection { +impl Display for AssignmentsIntersection { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::Decision((lvl, version, _)) => { @@ -112,16 +111,16 @@ impl Display for AssignmentsIntersection { } #[derive(Clone, Debug)] -pub enum SatisfierSearch { +pub enum SatisfierSearch { DifferentDecisionLevels { previous_satisfier_level: DecisionLevel, }, SameDecisionLevels { - satisfier_cause: IncompId, + satisfier_cause: IncompId, }, } -impl PartialSolution { +impl PartialSolution { /// Initialize an empty PartialSolution. pub fn empty() -> Self { Self { @@ -132,7 +131,7 @@ impl PartialSolution { } /// Add a decision. - pub fn add_decision(&mut self, package: P, version: V) { + pub fn add_decision(&mut self, package: P, version: VS::V) { // Check that add_decision is never used in the wrong context. if cfg!(debug_assertions) { match self.package_assignments.get_mut(&package) { @@ -165,8 +164,8 @@ impl PartialSolution { pub fn add_derivation( &mut self, package: P, - cause: IncompId, - store: &Arena>, + cause: IncompId, + store: &Arena>, ) { use std::collections::hash_map::Entry; let term = store[cause].get(&package).unwrap().negate(); @@ -208,7 +207,7 @@ impl PartialSolution { /// selected version (no "decision") /// and if it contains at least one positive derivation term /// in the partial solution. - pub fn potential_packages(&self) -> Option)>> { + pub fn potential_packages(&self) -> Option> { let mut iter = self .package_assignments .iter() @@ -224,7 +223,7 @@ impl PartialSolution { /// If a partial solution has, for every positive derivation, /// a corresponding decision that satisfies that assignment, /// it's a total solution and version solving has succeeded. - pub fn extract_solution(&self) -> Option> { + pub fn extract_solution(&self) -> Option> { let mut solution = Map::default(); for (p, pa) in &self.package_assignments { match &pa.assignments_intersection { @@ -245,7 +244,7 @@ impl PartialSolution { pub fn backtrack( &mut self, decision_level: DecisionLevel, - store: &Arena>, + store: &Arena>, ) { self.current_decision_level = decision_level; self.package_assignments.retain(|p, pa| { @@ -295,12 +294,12 @@ impl PartialSolution { pub fn add_version( &mut self, package: P, - version: V, - new_incompatibilities: std::ops::Range>, - store: &Arena>, + version: VS::V, + new_incompatibilities: std::ops::Range>, + store: &Arena>, ) { let exact = Term::exact(version.clone()); - let not_satisfied = |incompat: &Incompatibility| { + let not_satisfied = |incompat: &Incompatibility| { incompat.relation(|p| { if p == &package { Some(&exact) @@ -325,12 +324,12 @@ impl PartialSolution { } /// Check if the terms in the partial solution satisfy the incompatibility. - pub fn relation(&self, incompat: &Incompatibility) -> Relation

{ + pub fn relation(&self, incompat: &Incompatibility) -> Relation

{ incompat.relation(|package| self.term_intersection_for_package(package)) } /// Retrieve intersection of terms related to package. - pub fn term_intersection_for_package(&self, package: &P) -> Option<&Term> { + pub fn term_intersection_for_package(&self, package: &P) -> Option<&Term> { self.package_assignments .get(package) .map(|pa| pa.assignments_intersection.term()) @@ -339,9 +338,9 @@ impl PartialSolution { /// Figure out if the satisfier and previous satisfier are of different decision levels. pub fn satisfier_search( &self, - incompat: &Incompatibility, - store: &Arena>, - ) -> (P, SatisfierSearch) { + incompat: &Incompatibility, + store: &Arena>, + ) -> (P, SatisfierSearch) { let satisfied_map = Self::find_satisfier(incompat, &self.package_assignments, store); let (satisfier_package, &(satisfier_index, _, satisfier_decision_level)) = satisfied_map .iter() @@ -380,9 +379,9 @@ impl PartialSolution { /// It would be nice if we could get rid of it, but I don't know if then it will be possible /// to return a coherent previous_satisfier_level. fn find_satisfier( - incompat: &Incompatibility, - package_assignments: &Map>, - store: &Arena>, + incompat: &Incompatibility, + package_assignments: &Map>, + store: &Arena>, ) -> SmallMap { let mut satisfied = SmallMap::Empty; for (package, incompat_term) in incompat.iter() { @@ -399,11 +398,11 @@ impl PartialSolution { /// such that incompatibility is satisfied by the partial solution up to /// and including that assignment plus satisfier. fn find_previous_satisfier( - incompat: &Incompatibility, + incompat: &Incompatibility, satisfier_package: &P, mut satisfied_map: SmallMap, - package_assignments: &Map>, - store: &Arena>, + package_assignments: &Map>, + store: &Arena>, ) -> DecisionLevel { // First, let's retrieve the previous derivations and the initial accum_term. let satisfier_pa = package_assignments.get(satisfier_package).unwrap(); @@ -437,13 +436,13 @@ impl PartialSolution { } } -impl PackageAssignments { +impl PackageAssignments { fn satisfier( &self, package: &P, - incompat_term: &Term, - start_term: Term, - store: &Arena>, + incompat_term: &Term, + start_term: Term, + store: &Arena>, ) -> (usize, u32, DecisionLevel) { // Term where we accumulate intersections until incompat_term is satisfied. let mut accum_term = start_term; @@ -484,9 +483,9 @@ impl PackageAssignments { } } -impl AssignmentsIntersection { +impl AssignmentsIntersection { /// Returns the term intersection of all assignments (decision included). - fn term(&self) -> &Term { + fn term(&self) -> &Term { match self { Self::Decision((_, _, term)) => term, Self::Derivations(term) => term, @@ -500,7 +499,7 @@ impl AssignmentsIntersection { fn potential_package_filter<'a, P: Package>( &'a self, package: &'a P, - ) -> Option<(&'a P, &'a Range)> { + ) -> Option<(&'a P, &'a VS)> { match self { Self::Decision(_) => None, Self::Derivations(term_intersection) => { diff --git a/src/lib.rs b/src/lib.rs index 40b61032..bc34599b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -49,8 +49,10 @@ //! # use pubgrub::solver::{OfflineDependencyProvider, resolve}; //! # use pubgrub::version::NumberVersion; //! # use pubgrub::range::Range; -//! # -//! let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); +//! +//! type NumVS = Range; +//! +//! let mut dependency_provider = OfflineDependencyProvider::<&str, NumVS>::new(); //! //! dependency_provider.add_dependencies( //! "root", 1, [("menu", Range::any()), ("icons", Range::any())], @@ -84,8 +86,10 @@ //! # //! # struct MyDependencyProvider; //! # -//! impl DependencyProvider for MyDependencyProvider { -//! fn choose_package_version, U: Borrow>>(&self,packages: impl Iterator) -> Result<(T, Option), Box> { +//! type SemVS = Range; +//! +//! impl DependencyProvider for MyDependencyProvider { +//! fn choose_package_version, U: Borrow>(&self,packages: impl Iterator) -> Result<(T, Option), Box> { //! unimplemented!() //! } //! @@ -93,7 +97,7 @@ //! &self, //! package: &String, //! version: &SemanticVersion, -//! ) -> Result, Box> { +//! ) -> Result, Box> { //! unimplemented!() //! } //! } @@ -153,13 +157,13 @@ //! [Output](crate::report::Reporter::Output) type and a single method. //! ``` //! # use pubgrub::package::Package; -//! # use pubgrub::version::Version; +//! # use pubgrub::version_set::VersionSet; //! # use pubgrub::report::DerivationTree; //! # -//! pub trait Reporter { +//! pub trait Reporter { //! type Output; //! -//! fn report(derivation_tree: &DerivationTree) -> Self::Output; +//! fn report(derivation_tree: &DerivationTree) -> Self::Output; //! } //! ``` //! Implementing a [Reporter](crate::report::Reporter) may involve a lot of heuristics @@ -173,8 +177,11 @@ //! # use pubgrub::report::{DefaultStringReporter, Reporter}; //! # use pubgrub::error::PubGrubError; //! # use pubgrub::version::NumberVersion; +//! # use pubgrub::range::Range; +//! # +//! # type NumVS = Range; //! # -//! # let dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); +//! # let dependency_provider = OfflineDependencyProvider::<&str, NumVS>::new(); //! # let root_package = "root"; //! # let root_version = 1; //! # @@ -217,5 +224,6 @@ pub mod solver; pub mod term; pub mod type_aliases; pub mod version; +pub mod version_set; mod internal; diff --git a/src/range.rs b/src/range.rs index 5b1f9bbf..b0ca3bc4 100644 --- a/src/range.rs +++ b/src/range.rs @@ -20,6 +20,29 @@ use std::ops::{Bound, RangeBounds}; use crate::internal::small_vec::SmallVec; use crate::version::Version; +use crate::version_set::VersionSet; + +impl VersionSet for Range { + type V = V; + // Constructors + fn empty() -> Self { + Range::none() + } + fn singleton(v: Self::V) -> Self { + Range::exact(v) + } + // Operations + fn complement(&self) -> Self { + self.negate() + } + fn intersection(&self, other: &Self) -> Self { + self.intersection(other) + } + // Membership + fn contains(&self, v: &Self::V) -> bool { + self.contains(v) + } +} /// A Range is a set of versions. #[derive(Debug, Clone, Eq, PartialEq)] diff --git a/src/report.rs b/src/report.rs index 07dec364..94db9c3c 100644 --- a/src/report.rs +++ b/src/report.rs @@ -7,50 +7,49 @@ use std::fmt; use std::ops::{Deref, DerefMut}; use crate::package::Package; -use crate::range::Range; use crate::term::Term; use crate::type_aliases::Map; -use crate::version::Version; +use crate::version_set::VersionSet; /// Reporter trait. -pub trait Reporter { +pub trait Reporter { /// Output type of the report. type Output; /// Generate a report from the derivation tree /// describing the resolution failure. - fn report(derivation_tree: &DerivationTree) -> Self::Output; + fn report(derivation_tree: &DerivationTree) -> Self::Output; } /// Derivation tree resulting in the impossibility /// to solve the dependencies of our root package. #[derive(Debug, Clone)] -pub enum DerivationTree { +pub enum DerivationTree { /// External incompatibility. - External(External), + External(External), /// Incompatibility derived from two others. - Derived(Derived), + Derived(Derived), } /// Incompatibilities that are not derived from others, /// they have their own reason. #[derive(Debug, Clone)] -pub enum External { +pub enum External { /// Initial incompatibility aiming at picking the root package for the first decision. - NotRoot(P, V), - /// There are no versions in the given range for this package. - NoVersions(P, Range), - /// Dependencies of the package are unavailable for versions in that range. - UnavailableDependencies(P, Range), + 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), /// Incompatibility coming from the dependencies of a given package. - FromDependencyOf(P, Range, P, Range), + FromDependencyOf(P, VS, P, VS), } /// Incompatibility derived from two others. #[derive(Debug, Clone)] -pub struct Derived { +pub struct Derived { /// Terms of the incompatibility. - pub terms: Map>, + pub terms: Map>, /// Indicate if that incompatibility is present multiple times /// in the derivation tree. /// If that is the case, it has a unique id, provided in that option. @@ -58,12 +57,12 @@ pub struct Derived { /// and refer to the explanation for the other times. pub shared_id: Option, /// First cause. - pub cause1: Box>, + pub cause1: Box>, /// Second cause. - pub cause2: Box>, + pub cause2: Box>, } -impl DerivationTree { +impl DerivationTree { /// Merge the [NoVersions](External::NoVersions) external incompatibilities /// with the other one they are matched with /// in a derived incompatibility. @@ -100,7 +99,7 @@ impl DerivationTree { } } - fn merge_no_versions(self, package: P, range: Range) -> Option { + fn merge_no_versions(self, package: P, set: VS) -> Option { match self { // TODO: take care of the Derived case. // Once done, we can remove the Option. @@ -109,19 +108,16 @@ impl DerivationTree { panic!("How did we end up with a NoVersions merged with a NotRoot?") } DerivationTree::External(External::NoVersions(_, r)) => Some(DerivationTree::External( - External::NoVersions(package, range.union(&r)), + External::NoVersions(package, set.union(&r)), )), - DerivationTree::External(External::UnavailableDependencies(_, r)) => { - Some(DerivationTree::External(External::UnavailableDependencies( - package, - range.union(&r), - ))) - } + DerivationTree::External(External::UnavailableDependencies(_, r)) => Some( + DerivationTree::External(External::UnavailableDependencies(package, set.union(&r))), + ), DerivationTree::External(External::FromDependencyOf(p1, r1, p2, r2)) => { if p1 == package { Some(DerivationTree::External(External::FromDependencyOf( p1, - r1.union(&range), + r1.union(&set), p2, r2, ))) @@ -130,7 +126,7 @@ impl DerivationTree { p1, r1, p2, - r2.union(&range), + r2.union(&set), ))) } } @@ -138,39 +134,39 @@ impl DerivationTree { } } -impl fmt::Display for External { +impl fmt::Display for External { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::NotRoot(package, version) => { write!(f, "we are solving dependencies of {} {}", package, version) } - Self::NoVersions(package, range) => { - if range == &Range::any() { + Self::NoVersions(package, set) => { + if set == &VS::full() { write!(f, "there is no available version for {}", package) } else { - write!(f, "there is no version of {} in {}", package, range) + write!(f, "there is no version of {} in {}", package, set) } } - Self::UnavailableDependencies(package, range) => { - if range == &Range::any() { + Self::UnavailableDependencies(package, set) => { + if set == &VS::full() { write!(f, "dependencies of {} are unavailable", package) } else { write!( f, "dependencies of {} at version {} are unavailable", - package, range + package, set ) } } - Self::FromDependencyOf(p, range_p, dep, range_dep) => { - if range_p == &Range::any() && range_dep == &Range::any() { + Self::FromDependencyOf(p, set_p, dep, set_dep) => { + if set_p == &VS::full() && set_dep == &VS::full() { write!(f, "{} depends on {}", p, dep) - } else if range_p == &Range::any() { - write!(f, "{} depends on {} {}", p, dep, range_dep) - } else if range_dep == &Range::any() { - write!(f, "{} {} depends on {}", p, range_p, dep) + } else if set_p == &VS::full() { + write!(f, "{} depends on {} {}", p, dep, set_dep) + } else if set_dep == &VS::full() { + write!(f, "{} {} depends on {}", p, set_p, dep) } else { - write!(f, "{} {} depends on {} {}", p, range_p, dep, range_dep) + write!(f, "{} {} depends on {} {}", p, set_p, dep, set_dep) } } } @@ -198,7 +194,7 @@ impl DefaultStringReporter { } } - fn build_recursive(&mut self, derived: &Derived) { + fn build_recursive(&mut self, derived: &Derived) { self.build_recursive_helper(derived); if let Some(id) = derived.shared_id { if self.shared_with_ref.get(&id) == None { @@ -208,7 +204,7 @@ impl DefaultStringReporter { }; } - fn build_recursive_helper(&mut self, current: &Derived) { + fn build_recursive_helper(&mut self, current: &Derived) { match (current.cause1.deref(), current.cause2.deref()) { (DerivationTree::External(external1), DerivationTree::External(external2)) => { // Simplest case, we just combine two external incompatibilities. @@ -285,11 +281,11 @@ impl DefaultStringReporter { /// /// The result will depend on the fact that the derived incompatibility /// has already been explained or not. - fn report_one_each( + fn report_one_each( &mut self, - derived: &Derived, - external: &External, - current_terms: &Map>, + derived: &Derived, + external: &External, + current_terms: &Map>, ) { match self.line_ref_of(derived.shared_id) { Some(ref_id) => self.lines.push(Self::explain_ref_and_external( @@ -303,11 +299,11 @@ impl DefaultStringReporter { } /// Report one derived (without a line ref yet) and one external. - fn report_recurse_one_each( + fn report_recurse_one_each( &mut self, - derived: &Derived, - external: &External, - current_terms: &Map>, + derived: &Derived, + external: &External, + current_terms: &Map>, ) { match (derived.cause1.deref(), derived.cause2.deref()) { // If the derived cause has itself one external prior cause, @@ -341,10 +337,10 @@ impl DefaultStringReporter { // String explanations ##################################################### /// Simplest case, we just combine two external incompatibilities. - fn explain_both_external( - external1: &External, - external2: &External, - current_terms: &Map>, + fn explain_both_external( + external1: &External, + external2: &External, + current_terms: &Map>, ) -> String { // TODO: order should be chosen to make it more logical. format!( @@ -356,12 +352,12 @@ impl DefaultStringReporter { } /// Both causes have already been explained so we use their refs. - fn explain_both_ref( + fn explain_both_ref( ref_id1: usize, - derived1: &Derived, + derived1: &Derived, ref_id2: usize, - derived2: &Derived, - current_terms: &Map>, + derived2: &Derived, + current_terms: &Map>, ) -> String { // TODO: order should be chosen to make it more logical. format!( @@ -377,11 +373,11 @@ impl DefaultStringReporter { /// One cause is derived (already explained so one-line), /// the other is a one-line external cause, /// and finally we conclude with the current incompatibility. - fn explain_ref_and_external( + fn explain_ref_and_external( ref_id: usize, - derived: &Derived, - external: &External, - current_terms: &Map>, + derived: &Derived, + external: &External, + current_terms: &Map>, ) -> String { // TODO: order should be chosen to make it more logical. format!( @@ -394,9 +390,9 @@ impl DefaultStringReporter { } /// Add an external cause to the chain of explanations. - fn and_explain_external( - external: &External, - current_terms: &Map>, + fn and_explain_external( + external: &External, + current_terms: &Map>, ) -> String { format!( "And because {}, {}.", @@ -406,10 +402,10 @@ impl DefaultStringReporter { } /// Add an already explained incompat to the chain of explanations. - fn and_explain_ref( + fn and_explain_ref( ref_id: usize, - derived: &Derived, - current_terms: &Map>, + derived: &Derived, + current_terms: &Map>, ) -> String { format!( "And because {} ({}), {}.", @@ -420,10 +416,10 @@ impl DefaultStringReporter { } /// Add an already explained incompat to the chain of explanations. - fn and_explain_prior_and_external( - prior_external: &External, - external: &External, - current_terms: &Map>, + fn and_explain_prior_and_external( + prior_external: &External, + external: &External, + current_terms: &Map>, ) -> String { format!( "And because {} and {}, {}.", @@ -434,7 +430,7 @@ impl DefaultStringReporter { } /// Try to print terms of an incompatibility in a human-readable way. - pub fn string_terms(terms: &Map>) -> String { + pub fn string_terms(terms: &Map>) -> String { let terms_vec: Vec<_> = terms.iter().collect(); match terms_vec.as_slice() { [] => "version solving failed".into(), @@ -469,10 +465,10 @@ impl DefaultStringReporter { } } -impl Reporter for DefaultStringReporter { +impl Reporter for DefaultStringReporter { type Output = String; - fn report(derivation_tree: &DerivationTree) -> Self::Output { + fn report(derivation_tree: &DerivationTree) -> Self::Output { match derivation_tree { DerivationTree::External(external) => external.to_string(), DerivationTree::Derived(derived) => { diff --git a/src/solver.rs b/src/solver.rs index 4e360a56..846f220c 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -44,9 +44,12 @@ //! # use pubgrub::solver::{resolve, OfflineDependencyProvider}; //! # use pubgrub::version::NumberVersion; //! # use pubgrub::error::PubGrubError; +//! # use pubgrub::range::Range; //! # -//! # fn try_main() -> Result<(), PubGrubError<&'static str, NumberVersion>> { -//! # let dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); +//! # type NumVS = Range; +//! # +//! # fn try_main() -> Result<(), PubGrubError<&'static str, NumVS>> { +//! # let dependency_provider = OfflineDependencyProvider::<&str, NumVS>::new(); //! # let package = "root"; //! # let version = 1; //! let solution = resolve(&dependency_provider, package, version)?; @@ -73,19 +76,18 @@ use crate::error::PubGrubError; use crate::internal::core::State; use crate::internal::incompatibility::Incompatibility; use crate::package::Package; -use crate::range::Range; -use crate::type_aliases::{Map, SelectedDependencies}; -use crate::version::Version; +use crate::type_aliases::{DependencyConstraints, Map, SelectedDependencies}; +use crate::version_set::VersionSet; /// Main function of the library. /// Finds a set of packages satisfying dependency bounds for a given package + version pair. -pub fn resolve( - dependency_provider: &impl DependencyProvider, +pub fn resolve( + dependency_provider: &impl DependencyProvider, package: P, - version: impl Into, -) -> Result, PubGrubError> { + version: impl Into, +) -> Result, PubGrubError> { let mut state = State::init(package.clone(), version.into()); - let mut added_dependencies: Map> = Map::default(); + let mut added_dependencies: Map> = Map::default(); let mut next = package; loop { dependency_provider @@ -164,7 +166,7 @@ pub fn resolve( version: v.clone(), }); } - if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&Range::none()) { + if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&VS::empty()) { return Err(PubGrubError::DependencyOnTheEmptySet { package: p.clone(), version: v.clone(), @@ -207,31 +209,23 @@ pub fn resolve( } /// An enum used by [DependencyProvider] that holds information about package dependencies. -/// For each [Package] there is a [Range] of concrete versions it allows as a dependency. +/// 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. Unknown, /// Container for all available package versions. - Known(DependencyConstraints), + Known(DependencyConstraints), } -/// Subtype of [Dependencies] which holds information about -/// all possible versions a given package can accept. -/// There is a difference in semantics between an empty [Map>](crate::type_aliases::Map) -/// inside [DependencyConstraints] and [Dependencies::Unknown]: -/// the former means the package has no dependencies and it is a known fact, -/// while the latter means they could not be fetched by [DependencyProvider]. -pub type DependencyConstraints = Map>; - /// Trait that allows the algorithm to retrieve available packages and their dependencies. /// An implementor needs to be supplied to the [resolve] function. -pub trait DependencyProvider { +pub trait DependencyProvider { /// [Decision making](https://github.com/dart-lang/pub/blob/master/doc/solver.md#decision-making) /// is the process of choosing the next package /// and version that will be appended to the partial solution. /// Every time such a decision must be made, - /// potential valid packages and version ranges are preselected by the resolver, + /// potential valid packages and sets of versions are preselected by the resolver, /// and the dependency provider must choose. /// /// The strategy employed to choose such package and version @@ -252,18 +246,19 @@ pub trait DependencyProvider { /// of the available versions in preference order for any package. /// /// Note: the type `T` ensures that this returns an item from the `packages` argument. - fn choose_package_version, U: Borrow>>( + #[allow(clippy::type_complexity)] + fn choose_package_version, U: Borrow>( &self, potential_packages: impl Iterator, - ) -> Result<(T, Option), Box>; + ) -> Result<(T, Option), Box>; /// Retrieves the package dependencies. /// Return [Dependencies::Unknown] if its dependencies are unknown. fn get_dependencies( &self, package: &P, - version: &V, - ) -> Result, Box>; + version: &VS::V, + ) -> Result, Box>; /// This is called fairly regularly during the resolution, /// if it returns an Err then resolution will be terminated. @@ -282,38 +277,44 @@ pub trait DependencyProvider { /// The helper finds the package from the `packages` argument with the fewest versions from /// `list_available_versions` contained in the constraints. Then takes that package and finds the /// first version contained in the constraints. -pub fn choose_package_with_fewest_versions( +pub fn choose_package_with_fewest_versions( list_available_versions: F, potential_packages: impl Iterator, -) -> (T, Option) +) -> (T, Option) where T: Borrow

, - U: Borrow>, - I: Iterator, + U: Borrow, + I: Iterator, F: Fn(&P) -> I, { - let count_valid = |(p, range): &(T, U)| { + let count_valid = |(p, set): &(T, U)| { list_available_versions(p.borrow()) - .filter(|v| range.borrow().contains(v.borrow())) + .filter(|v| set.borrow().contains(v.borrow())) .count() }; - let (pkg, range) = potential_packages + let (pkg, set) = potential_packages .min_by_key(count_valid) .expect("potential_packages gave us an empty iterator"); - let version = - list_available_versions(pkg.borrow()).find(|v| range.borrow().contains(v.borrow())); + let version = list_available_versions(pkg.borrow()).find(|v| set.borrow().contains(v.borrow())); (pkg, version) } /// A basic implementation of [DependencyProvider]. #[derive(Debug, Clone, Default)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +#[cfg_attr( + feature = "serde", + serde(bound( + serialize = "VS::V: serde::Serialize, VS: serde::Serialize, P: serde::Serialize", + deserialize = "VS::V: serde::Deserialize<'de>, VS: serde::Deserialize<'de>, P: serde::Deserialize<'de>" + )) +)] #[cfg_attr(feature = "serde", serde(transparent))] -pub struct OfflineDependencyProvider { - dependencies: Map>>, +pub struct OfflineDependencyProvider { + dependencies: Map>>, } -impl OfflineDependencyProvider { +impl OfflineDependencyProvider { /// Creates an empty OfflineDependencyProvider with no dependencies. pub fn new() -> Self { Self { @@ -331,10 +332,10 @@ impl OfflineDependencyProvider { /// The API does not allow to add dependencies one at a time to uphold an assumption that /// [OfflineDependencyProvider.get_dependencies(p, v)](OfflineDependencyProvider::get_dependencies) /// provides all dependencies of a given package (p) and version (v) pair. - pub fn add_dependencies)>>( + pub fn add_dependencies>( &mut self, package: P, - version: impl Into, + version: impl Into, dependencies: I, ) { let package_deps = dependencies.into_iter().collect(); @@ -354,13 +355,13 @@ impl OfflineDependencyProvider { /// Lists versions of saved packages in sorted order. /// Returns [None] if no information is available regarding that package. - pub fn versions(&self, package: &P) -> Option> { + pub fn versions(&self, package: &P) -> Option> { self.dependencies.get(package).map(|k| k.keys()) } /// 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: &V) -> Option> { + fn dependencies(&self, package: &P, version: &VS::V) -> Option> { self.dependencies.get(package)?.get(version).cloned() } } @@ -369,11 +370,12 @@ impl OfflineDependencyProvider { /// contains all dependency information available in memory. /// Packages are picked with the fewest versions contained in the constraints first. /// Versions are picked with the newest versions first. -impl DependencyProvider for OfflineDependencyProvider { - fn choose_package_version, U: Borrow>>( +impl DependencyProvider for OfflineDependencyProvider { + #[allow(clippy::type_complexity)] + fn choose_package_version, U: Borrow>( &self, potential_packages: impl Iterator, - ) -> Result<(T, Option), Box> { + ) -> Result<(T, Option), Box> { Ok(choose_package_with_fewest_versions( |p| { self.dependencies @@ -390,8 +392,8 @@ impl DependencyProvider for OfflineDependencyProvi fn get_dependencies( &self, package: &P, - version: &V, - ) -> Result, Box> { + version: &VS::V, + ) -> Result, Box> { Ok(match self.dependencies(package, version) { None => Dependencies::Unknown, Some(dependencies) => Dependencies::Known(dependencies), diff --git a/src/term.rs b/src/term.rs index ad8158fd..3028dbe1 100644 --- a/src/term.rs +++ b/src/term.rs @@ -3,38 +3,37 @@ //! A term is the fundamental unit of operation of the PubGrub algorithm. //! It is a positive or negative expression regarding a set of versions. -use crate::range::Range; -use crate::version::Version; -use std::fmt; +use crate::version_set::VersionSet; +use std::fmt::{self, Display}; /// A positive or negative expression regarding a set of versions. #[derive(Debug, Clone, Eq, PartialEq)] -pub enum Term { +pub enum Term { /// For example, "1.0.0 <= v < 2.0.0" is a positive expression /// that is evaluated true if a version is selected /// and comprised between version 1.0.0 and version 2.0.0. - Positive(Range), + Positive(VS), /// The term "not v < 3.0.0" is a negative expression /// that is evaluated true if a version is selected >= 3.0.0 /// or if no version is selected at all. - Negative(Range), + Negative(VS), } /// Base methods. -impl Term { +impl Term { /// A term that is always true. pub(crate) fn any() -> Self { - Self::Negative(Range::none()) + Self::Negative(VS::empty()) } /// A term that is never true. pub(crate) fn empty() -> Self { - Self::Positive(Range::none()) + Self::Positive(VS::empty()) } /// A positive term containing exactly that version. - pub(crate) fn exact(version: V) -> Self { - Self::Positive(Range::exact(version)) + pub(crate) fn exact(version: VS::V) -> Self { + Self::Positive(VS::singleton(version)) } /// Simply check if a term is positive. @@ -50,41 +49,41 @@ impl Term { /// the opposite of the evaluation of the original one. pub(crate) fn negate(&self) -> Self { match self { - Self::Positive(range) => Self::Negative(range.clone()), - Self::Negative(range) => Self::Positive(range.clone()), + Self::Positive(set) => Self::Negative(set.clone()), + Self::Negative(set) => Self::Positive(set.clone()), } } /// Evaluate a term regarding a given choice of version. - pub(crate) fn contains(&self, v: &V) -> bool { + pub(crate) fn contains(&self, v: &VS::V) -> bool { match self { - Self::Positive(range) => range.contains(v), - Self::Negative(range) => !(range.contains(v)), + Self::Positive(set) => set.contains(v), + Self::Negative(set) => !(set.contains(v)), } } - /// Unwrap the range contains in a positive term. - /// Will panic if used on a negative range. - pub(crate) fn unwrap_positive(&self) -> &Range { + /// Unwrap the set contained in a positive term. + /// Will panic if used on a negative set. + pub(crate) fn unwrap_positive(&self) -> &VS { match self { - Self::Positive(range) => range, - _ => panic!("Negative term cannot unwrap positive range"), + Self::Positive(set) => set, + _ => panic!("Negative term cannot unwrap positive set"), } } } /// Set operations with terms. -impl Term { +impl Term { /// Compute the intersection of two terms. /// If at least one term is positive, the intersection is also positive. - pub(crate) fn intersection(&self, other: &Term) -> Term { + pub(crate) fn intersection(&self, other: &Self) -> Self { match (self, other) { (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.intersection(r2)), (Self::Positive(r1), Self::Negative(r2)) => { - Self::Positive(r1.intersection(&r2.negate())) + Self::Positive(r1.intersection(&r2.complement())) } (Self::Negative(r1), Self::Positive(r2)) => { - Self::Positive(r1.negate().intersection(r2)) + Self::Positive(r1.complement().intersection(r2)) } (Self::Negative(r1), Self::Negative(r2)) => Self::Negative(r1.union(r2)), } @@ -92,14 +91,14 @@ impl Term { /// Compute the union of two terms. /// If at least one term is negative, the union is also negative. - pub(crate) fn union(&self, other: &Term) -> Term { + pub(crate) fn union(&self, other: &Self) -> Self { (self.negate().intersection(&other.negate())).negate() } /// Indicate if this term is a subset of another term. /// Just like for sets, we say that t1 is a subset of t2 /// if and only if t1 āˆ© t2 = t1. - pub(crate) fn subset_of(&self, other: &Term) -> bool { + pub(crate) fn subset_of(&self, other: &Self) -> bool { self == &self.intersection(other) } } @@ -120,7 +119,7 @@ pub(crate) enum Relation { } /// Relation between terms. -impl<'a, V: 'a + Version> Term { +impl Term { /// Check if a set of terms satisfies this term. /// /// We say that a set of terms S "satisfies" a term t @@ -129,7 +128,7 @@ impl<'a, V: 'a + Version> Term { /// It turns out that this can also be expressed with set operations: /// S satisfies t if and only if ā‹‚ S āŠ† t #[cfg(test)] - fn satisfied_by(&self, terms_intersection: &Term) -> bool { + fn satisfied_by(&self, terms_intersection: &Self) -> bool { terms_intersection.subset_of(self) } @@ -142,13 +141,13 @@ impl<'a, V: 'a + Version> Term { /// S contradicts t if and only if ā‹‚ S is disjoint with t /// S contradicts t if and only if (ā‹‚ S) ā‹‚ t = āˆ… #[cfg(test)] - fn contradicted_by(&self, terms_intersection: &Term) -> bool { + fn contradicted_by(&self, terms_intersection: &Self) -> bool { terms_intersection.intersection(self) == Self::empty() } /// Check if a set of terms satisfies or contradicts a given term. /// Otherwise the relation is inconclusive. - pub(crate) fn relation_with(&self, other_terms_intersection: &Term) -> Relation { + pub(crate) fn relation_with(&self, other_terms_intersection: &Self) -> Relation { let full_intersection = self.intersection(other_terms_intersection); if &full_intersection == other_terms_intersection { Relation::Satisfied @@ -160,19 +159,19 @@ impl<'a, V: 'a + Version> Term { } } -impl AsRef> for Term { - fn as_ref(&self) -> &Term { +impl AsRef for Term { + fn as_ref(&self) -> &Self { self } } // REPORT ###################################################################### -impl fmt::Display for Term { +impl Display for Term { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::Positive(range) => write!(f, "{}", range), - Self::Negative(range) => write!(f, "Not ( {} )", range), + Self::Positive(set) => write!(f, "{}", set), + Self::Negative(set) => write!(f, "Not ( {} )", set), } } } @@ -182,10 +181,11 @@ impl fmt::Display for Term { #[cfg(test)] pub mod tests { use super::*; + use crate::range::Range; use crate::version::NumberVersion; use proptest::prelude::*; - pub fn strategy() -> impl Strategy> { + pub fn strategy() -> impl Strategy>> { prop_oneof![ crate::range::tests::strategy().prop_map(Term::Positive), crate::range::tests::strategy().prop_map(Term::Negative), diff --git a/src/type_aliases.rs b/src/type_aliases.rs index d1cc378a..11cc37c7 100644 --- a/src/type_aliases.rs +++ b/src/type_aliases.rs @@ -6,5 +6,12 @@ pub type Map = rustc_hash::FxHashMap; /// Concrete dependencies picked by the library during [resolve](crate::solver::resolve) -/// from [DependencyConstraints](crate::solver::DependencyConstraints) +/// from [DependencyConstraints]. 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): +/// 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/src/version_set.rs b/src/version_set.rs new file mode 100644 index 00000000..501ec700 --- /dev/null +++ b/src/version_set.rs @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: MPL-2.0 + +//! As its name suggests, the [VersionSet] trait describes sets of versions. +//! +//! One needs to define +//! - the associate type for versions, +//! - two constructors for the empty set and a singleton set, +//! - the complement and intersection set operations, +//! - and a function to evaluate membership of versions. +//! +//! Two functions are automatically derived, thanks to the mathematical properties of sets. +//! You can overwrite those implementations, but we highly recommend that you don't, +//! except if you are confident in a correct implementation that brings much performance gains. +//! +//! It is also extremely important that the `Eq` trait is correctly implemented. +//! In particular, you can only use `#[derive(Eq, PartialEq)]` if `Eq` is strictly equivalent to the +//! structural equality, i.e. if version sets have canonical representations. +//! Such problems may arise if your implementations of `complement()` and `intersection()` do not +//! return canonical representations so be careful there. + +use std::fmt::{Debug, Display}; + +/// Trait describing sets of versions. +pub trait VersionSet: Debug + Display + Clone + Eq { + /// Version type associated with the sets manipulated. + type V: Debug + Display + Clone + Ord; + + // Constructors + /// Constructor for an empty set containing no version. + fn empty() -> Self; + /// Constructor for a set containing exactly one version. + fn singleton(v: Self::V) -> Self; + + // Operations + /// Compute the complement of this set. + fn complement(&self) -> Self; + /// Compute the intersection with another set. + fn intersection(&self, other: &Self) -> Self; + + // Membership + /// Evaluate membership of a version in this set. + fn contains(&self, v: &Self::V) -> bool; + + // Automatically implemented functions ########################### + + /// Constructor for the set containing all versions. + /// Automatically implemented as `Self::empty().complement()`. + fn full() -> Self { + Self::empty().complement() + } + + /// Compute the union with another set. + /// Thanks to set properties, this is automatically implemented as: + /// `self.complement().intersection(&other.complement()).complement()` + fn union(&self, other: &Self) -> Self { + self.complement() + .intersection(&other.complement()) + .complement() + } +} diff --git a/tests/examples.rs b/tests/examples.rs index 5d66db79..9040bc23 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -5,6 +5,9 @@ use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::type_aliases::Map; use pubgrub::version::{NumberVersion, SemanticVersion}; +type NumVS = Range; +type SemVS = Range; + use log::LevelFilter; use std::io::Write; @@ -20,7 +23,7 @@ fn init_log() { /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#no-conflicts fn no_conflict() { init_log(); - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( "root", (1, 0, 0), @@ -51,7 +54,7 @@ fn no_conflict() { /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#avoiding-conflict-during-decision-making fn avoiding_conflict_during_decision_making() { init_log(); - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( "root", (1, 0, 0), @@ -87,7 +90,7 @@ fn avoiding_conflict_during_decision_making() { /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#performing-conflict-resolution fn conflict_resolution() { init_log(); - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( "root", (1, 0, 0), @@ -121,7 +124,7 @@ fn conflict_resolution() { /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#conflict-resolution-with-a-partial-satisfier fn conflict_with_partial_satisfier() { init_log(); - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); #[rustfmt::skip] // root 1.0.0 depends on foo ^1.0.0 and target ^2.0.0 dependency_provider.add_dependencies( @@ -187,7 +190,7 @@ fn conflict_with_partial_satisfier() { /// Solution: a0, b0, c0, d0 fn double_choices() { init_log(); - let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, NumVS>::new(); dependency_provider.add_dependencies("a", 0, [("b", Range::any()), ("c", Range::any())]); dependency_provider.add_dependencies("b", 0, [("d", Range::exact(0))]); dependency_provider.add_dependencies("b", 1, [("d", Range::exact(1))]); diff --git a/tests/proptest.rs b/tests/proptest.rs index 637d8ff1..fa775720 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -10,7 +10,8 @@ use pubgrub::solver::{ choose_package_with_fewest_versions, resolve, Dependencies, DependencyProvider, OfflineDependencyProvider, }; -use pubgrub::version::{NumberVersion, Version}; +use pubgrub::version::{NumberVersion, SemanticVersion}; +use pubgrub::version_set::VersionSet; use proptest::collection::{btree_map, vec}; use proptest::prelude::*; @@ -24,20 +25,24 @@ mod sat_dependency_provider; /// The same as [OfflineDependencyProvider] but takes versions from the opposite end: /// if [OfflineDependencyProvider] returns versions from newest to oldest, this returns them from oldest to newest. #[derive(Clone)] -struct OldestVersionsDependencyProvider(OfflineDependencyProvider); +struct OldestVersionsDependencyProvider( + OfflineDependencyProvider, +); -impl DependencyProvider for OldestVersionsDependencyProvider { - fn choose_package_version, U: std::borrow::Borrow>>( +impl DependencyProvider + for OldestVersionsDependencyProvider +{ + fn choose_package_version, U: std::borrow::Borrow>( &self, potential_packages: impl Iterator, - ) -> Result<(T, Option), Box> { + ) -> Result<(T, Option), Box> { Ok(choose_package_with_fewest_versions( |p| self.0.versions(p).into_iter().flatten().cloned(), potential_packages, )) } - fn get_dependencies(&self, p: &P, v: &V) -> Result, Box> { + fn get_dependencies(&self, p: &P, v: &VS::V) -> Result, Box> { self.0.get_dependencies(p, v) } } @@ -62,17 +67,17 @@ impl TimeoutDependencyProvider { } } -impl> DependencyProvider +impl> DependencyProvider for TimeoutDependencyProvider { - fn choose_package_version, U: std::borrow::Borrow>>( + fn choose_package_version, U: std::borrow::Borrow>( &self, potential_packages: impl Iterator, - ) -> Result<(T, Option), Box> { + ) -> Result<(T, Option), Box> { self.dp.choose_package_version(potential_packages) } - fn get_dependencies(&self, p: &P, v: &V) -> Result, Box> { + fn get_dependencies(&self, p: &P, v: &VS::V) -> Result, Box> { self.dp.get_dependencies(p, v) } @@ -85,10 +90,13 @@ impl> DependencyProvider; +type SemVS = Range; + #[test] #[should_panic] fn should_cancel_can_panic() { - let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); dependency_provider.add_dependencies(0, 0, [(666, Range::any())]); // Run the algorithm. @@ -116,12 +124,7 @@ fn string_names() -> impl Strategy { pub fn registry_strategy( name: impl Strategy, bad_name: N, -) -> impl Strategy< - Value = ( - OfflineDependencyProvider, - Vec<(N, NumberVersion)>, - ), -> { +) -> impl Strategy, Vec<(N, NumberVersion)>)> { let max_crates = 40; let max_versions = 15; let shrinkage = 40; @@ -166,20 +169,18 @@ 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)>>, - )> = 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 }, - ) + let mut list_of_pkgid: Vec<((N, NumberVersion), Option>)> = + 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 }, + ) + }) }) - }) - .collect(); + .collect(); let len_all_pkgid = list_of_pkgid.len(); for (a, b, (c, d)) in raw_dependencies { let (a, b) = order_index(a, b, len_all_pkgid); @@ -210,7 +211,7 @@ pub fn registry_strategy( } } - let mut dependency_provider = OfflineDependencyProvider::::new(); + let mut dependency_provider = OfflineDependencyProvider::::new(); let complicated_len = std::cmp::min(complicated_len, list_of_pkgid.len()); let complicated: Vec<_> = if reverse_alphabetical { @@ -373,7 +374,7 @@ proptest! { .versions(package) .unwrap().collect(); let version = version_idx.get(&versions); - let dependencies: Vec<(u16, Range)> = match dependency_provider + let dependencies: Vec<(u16, NumVS)> = match dependency_provider .get_dependencies(package, version) .unwrap() { @@ -432,7 +433,7 @@ proptest! { Ok(used) => { // If resolution was successful, then unpublishing a version of a crate // that was not selected should not change that. - let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); + let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); for &(n, v) in &all_versions { if used.get(&n) == Some(&v) // it was used || to_remove.get(&(n, v)).is_none() // or it is not one to be removed @@ -455,7 +456,7 @@ proptest! { Err(_) => { // If resolution was unsuccessful, then it should stay unsuccessful // even if any version of a crate is unpublished. - let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); + let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); for &(n, v) in &all_versions { if to_remove.get(&(n, v)).is_none() // it is not one to be removed { @@ -488,7 +489,7 @@ fn large_case() { eprintln!("{}", name); let data = std::fs::read_to_string(&case).unwrap(); if name.ends_with("u16_NumberVersion.ron") { - let dependency_provider: OfflineDependencyProvider = + let dependency_provider: OfflineDependencyProvider = ron::de::from_str(&data).unwrap(); let mut sat = SatResolve::new(&dependency_provider); for p in dependency_provider.packages() { @@ -501,10 +502,8 @@ fn large_case() { } } } else if name.ends_with("str_SemanticVersion.ron") { - let dependency_provider: OfflineDependencyProvider< - &str, - pubgrub::version::SemanticVersion, - > = ron::de::from_str(&data).unwrap(); + let dependency_provider: OfflineDependencyProvider<&str, SemVS> = + ron::de::from_str(&data).unwrap(); let mut sat = SatResolve::new(&dependency_provider); for p in dependency_provider.packages() { for n in dependency_provider.versions(p).unwrap() { diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index 1a21d5f0..97ecab3e 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -3,7 +3,7 @@ use pubgrub::package::Package; use pubgrub::solver::{Dependencies, DependencyProvider, OfflineDependencyProvider}; use pubgrub::type_aliases::{Map, SelectedDependencies}; -use pubgrub::version::Version; +use pubgrub::version_set::VersionSet; use varisat::ExtendFormula; const fn num_bits() -> usize { @@ -46,17 +46,17 @@ fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Va /// /// The SAT library does not optimize for the newer version, /// so the selected packages may not match the real resolver. -pub struct SatResolve { +pub struct SatResolve { solver: varisat::Solver<'static>, - all_versions_by_p: Map>, + all_versions_by_p: Map>, } -impl SatResolve { - pub fn new(dp: &OfflineDependencyProvider) -> Self { +impl SatResolve { + pub fn new(dp: &OfflineDependencyProvider) -> Self { let mut cnf = varisat::CnfFormula::new(); let mut all_versions = vec![]; - let mut all_versions_by_p: Map> = Map::default(); + let mut all_versions_by_p: Map> = Map::default(); for p in dp.packages() { let mut versions_for_p = vec![]; @@ -110,7 +110,7 @@ impl SatResolve { } } - pub fn sat_resolve(&mut self, name: &P, ver: &V) -> bool { + pub fn sat_resolve(&mut self, name: &P, ver: &VS::V) -> bool { if let Some(vers) = self.all_versions_by_p.get(name) { if let Some((_, var)) = vers.iter().find(|(v, _)| v == ver) { self.solver.assume(&[var.positive()]); @@ -126,7 +126,7 @@ impl SatResolve { } } - pub fn sat_is_valid_solution(&mut self, pids: &SelectedDependencies) -> bool { + pub fn sat_is_valid_solution(&mut self, pids: &SelectedDependencies) -> bool { let mut assumption = vec![]; for (p, vs) in &self.all_versions_by_p { diff --git a/tests/tests.rs b/tests/tests.rs index d9bf2d06..77e4385b 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -5,9 +5,11 @@ use pubgrub::range::Range; use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::version::NumberVersion; +type NumVS = Range; + #[test] fn same_result_on_repeated_runs() { - let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); dependency_provider.add_dependencies("c", 0, []); dependency_provider.add_dependencies("c", 2, []); @@ -29,7 +31,7 @@ fn same_result_on_repeated_runs() { #[test] fn should_always_find_a_satisfier() { - let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); dependency_provider.add_dependencies("a", 0, [("b", Range::none())]); assert!(matches!( resolve(&dependency_provider, "a", 0), @@ -45,7 +47,7 @@ fn should_always_find_a_satisfier() { #[test] fn cannot_depend_on_self() { - let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); dependency_provider.add_dependencies("a", 0, [("a", Range::any())]); assert!(matches!( resolve(&dependency_provider, "a", 0), From 15ac3c7bb1f5d33fc9c8e86644e343bec61696ae Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Sun, 26 Jun 2022 00:06:15 +0200 Subject: [PATCH 08/20] refactor: replace Range with a bounded implementation (#112) * refactor: replace Range with a bounded implementation * fix: rewrite range proptest strategy * fix: deserialize SmallVec without Vec alloc * fix: remove not_equals * fix: re-add union and remove early out * fix: renamed V to VS in bench * refactor: simpler intersection Co-authored-by: Jacob Finkelman * test: use deltas for range strategy Co-authored-by: Jacob Finkelman * docs(range): added comment about discrete values * More restrictive for valid random range generation * Remove duplicate check in check_invariants * Add warning about non-unique ranges representations * Rename start_bounded into start_unbounded Co-authored-by: Jacob Finkelman Co-authored-by: Matthieu Pizenberg --- benches/large_case.rs | 17 +- examples/doc_interface.rs | 6 +- examples/doc_interface_error.rs | 16 +- examples/doc_interface_semantic.rs | 12 +- src/internal/incompatibility.rs | 4 +- src/internal/small_vec.rs | 75 ++- src/lib.rs | 6 +- src/range.rs | 711 +++++++++++++++++------------ src/term.rs | 3 +- tests/examples.rs | 8 +- tests/proptest.rs | 8 +- tests/tests.rs | 8 +- 12 files changed, 529 insertions(+), 345 deletions(-) diff --git a/benches/large_case.rs b/benches/large_case.rs index 476228c8..8efdaa9e 100644 --- a/benches/large_case.rs +++ b/benches/large_case.rs @@ -5,16 +5,19 @@ extern crate criterion; use self::criterion::*; use pubgrub::package::Package; +use pubgrub::range::Range; use pubgrub::solver::{resolve, OfflineDependencyProvider}; -use pubgrub::version::{NumberVersion, SemanticVersion, Version}; +use pubgrub::version::{NumberVersion, SemanticVersion}; +use pubgrub::version_set::VersionSet; use serde::de::Deserialize; -use std::hash::Hash; -fn bench<'a, P: Package + Deserialize<'a>, V: Version + Hash + Deserialize<'a>>( +fn bench<'a, P: Package + Deserialize<'a>, VS: VersionSet + Deserialize<'a>>( b: &mut Bencher, case: &'a str, -) { - let dependency_provider: OfflineDependencyProvider = ron::de::from_str(&case).unwrap(); +) where + ::V: Deserialize<'a>, +{ + let dependency_provider: OfflineDependencyProvider = ron::de::from_str(&case).unwrap(); b.iter(|| { for p in dependency_provider.packages() { @@ -35,11 +38,11 @@ fn bench_nested(c: &mut Criterion) { let data = std::fs::read_to_string(&case).unwrap(); if name.ends_with("u16_NumberVersion.ron") { group.bench_function(name, |b| { - bench::(b, &data); + bench::>(b, &data); }); } else if name.ends_with("str_SemanticVersion.ron") { group.bench_function(name, |b| { - bench::<&str, SemanticVersion>(b, &data); + bench::<&str, Range>(b, &data); }); } } diff --git a/examples/doc_interface.rs b/examples/doc_interface.rs index d409dcce..ca6bcb93 100644 --- a/examples/doc_interface.rs +++ b/examples/doc_interface.rs @@ -14,10 +14,10 @@ type NumVS = Range; fn main() { let mut dependency_provider = OfflineDependencyProvider::<&str, NumVS>::new(); dependency_provider.add_dependencies( - "root", 1, [("menu", Range::any()), ("icons", Range::any())], + "root", 1, [("menu", Range::full()), ("icons", Range::full())], ); - dependency_provider.add_dependencies("menu", 1, [("dropdown", Range::any())]); - dependency_provider.add_dependencies("dropdown", 1, [("icons", Range::any())]); + dependency_provider.add_dependencies("menu", 1, [("dropdown", Range::full())]); + dependency_provider.add_dependencies("dropdown", 1, [("icons", Range::full())]); dependency_provider.add_dependencies("icons", 1, []); // Run the algorithm. diff --git a/examples/doc_interface_error.rs b/examples/doc_interface_error.rs index a3d7e61e..a78a3eb3 100644 --- a/examples/doc_interface_error.rs +++ b/examples/doc_interface_error.rs @@ -20,9 +20,9 @@ fn main() { let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); // Direct dependencies: menu and icons. dependency_provider.add_dependencies("root", (1, 0, 0), [ - ("menu", Range::any()), - ("icons", Range::exact((1, 0, 0))), - ("intl", Range::exact((5, 0, 0))), + ("menu", Range::full()), + ("icons", Range::singleton((1, 0, 0))), + ("intl", Range::singleton((5, 0, 0))), ]); // Dependencies of the menu lib. @@ -47,19 +47,19 @@ fn main() { // Dependencies of the dropdown lib. dependency_provider.add_dependencies("dropdown", (1, 8, 0), [ - ("intl", Range::exact((3, 0, 0))), + ("intl", Range::singleton((3, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 0, 0), [ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 1, 0), [ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 2, 0), [ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 3, 0), [ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); // Icons have no dependencies. diff --git a/examples/doc_interface_semantic.rs b/examples/doc_interface_semantic.rs index cce059bc..17ff3c09 100644 --- a/examples/doc_interface_semantic.rs +++ b/examples/doc_interface_semantic.rs @@ -19,8 +19,8 @@ fn main() { let mut dependency_provider = OfflineDependencyProvider::<&str, SemVS>::new(); // Direct dependencies: menu and icons. dependency_provider.add_dependencies("root", (1, 0, 0), [ - ("menu", Range::any()), - ("icons", Range::exact((1, 0, 0))), + ("menu", Range::full()), + ("icons", Range::singleton((1, 0, 0))), ]); // Dependencies of the menu lib. @@ -46,16 +46,16 @@ fn main() { // Dependencies of the dropdown lib. dependency_provider.add_dependencies("dropdown", (1, 8, 0), []); dependency_provider.add_dependencies("dropdown", (2, 0, 0), [ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 1, 0), [ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 2, 0), [ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 3, 0), [ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); // Icons has no dependency. diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index dd093a08..93861621 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -276,12 +276,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::any()) + kind: Kind::UnavailableDependencies("0", Range::full()) }); let i2 = store.alloc(Incompatibility { package_terms: SmallMap::Two([("p2", t2), ("p3", t3.clone())]), - kind: Kind::UnavailableDependencies("0", Range::any()) + kind: Kind::UnavailableDependencies("0", Range::full()) }); let mut i3 = Map::default(); diff --git a/src/internal/small_vec.rs b/src/internal/small_vec.rs index 2c3fe4f4..c2a178a9 100644 --- a/src/internal/small_vec.rs +++ b/src/internal/small_vec.rs @@ -1,4 +1,5 @@ use std::fmt; +use std::hash::{Hash, Hasher}; use std::ops::Deref; #[derive(Clone)] @@ -108,6 +109,13 @@ impl fmt::Debug for SmallVec { } } +impl Hash for SmallVec { + fn hash(&self, state: &mut H) { + self.len().hash(state); + Hash::hash_slice(self.as_slice(), state); + } +} + #[cfg(feature = "serde")] impl serde::Serialize for SmallVec { fn serialize(&self, s: S) -> Result { @@ -118,13 +126,70 @@ impl serde::Serialize for SmallVec { #[cfg(feature = "serde")] impl<'de, T: serde::Deserialize<'de>> serde::Deserialize<'de> for SmallVec { fn deserialize>(d: D) -> Result { - let items: Vec = serde::Deserialize::deserialize(d)?; + struct SmallVecVisitor { + marker: std::marker::PhantomData, + } + + impl<'de, T> serde::de::Visitor<'de> for SmallVecVisitor + where + T: serde::Deserialize<'de>, + { + type Value = SmallVec; + + fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + formatter.write_str("a sequence") + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: serde::de::SeqAccess<'de>, + { + let mut values = SmallVec::empty(); + while let Some(value) = seq.next_element()? { + values.push(value); + } + Ok(values) + } + } + + let visitor = SmallVecVisitor { + marker: Default::default(), + }; + d.deserialize_seq(visitor) + } +} + +impl IntoIterator for SmallVec { + type Item = T; + type IntoIter = SmallVecIntoIter; - let mut v = Self::empty(); - for item in items { - v.push(item); + fn into_iter(self) -> Self::IntoIter { + match self { + SmallVec::Empty => SmallVecIntoIter::Empty, + SmallVec::One(a) => SmallVecIntoIter::One(IntoIterator::into_iter(a)), + SmallVec::Two(a) => SmallVecIntoIter::Two(IntoIterator::into_iter(a)), + SmallVec::Flexible(v) => SmallVecIntoIter::Flexible(IntoIterator::into_iter(v)), + } + } +} + +pub enum SmallVecIntoIter { + Empty, + One(<[T; 1] as IntoIterator>::IntoIter), + Two(<[T; 2] as IntoIterator>::IntoIter), + Flexible( as IntoIterator>::IntoIter), +} + +impl Iterator for SmallVecIntoIter { + type Item = T; + + fn next(&mut self) -> Option { + match self { + SmallVecIntoIter::Empty => None, + SmallVecIntoIter::One(it) => it.next(), + SmallVecIntoIter::Two(it) => it.next(), + SmallVecIntoIter::Flexible(it) => it.next(), } - Ok(v) } } diff --git a/src/lib.rs b/src/lib.rs index bc34599b..86debdcb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -55,10 +55,10 @@ //! let mut dependency_provider = OfflineDependencyProvider::<&str, NumVS>::new(); //! //! dependency_provider.add_dependencies( -//! "root", 1, [("menu", Range::any()), ("icons", Range::any())], +//! "root", 1, [("menu", Range::full()), ("icons", Range::full())], //! ); -//! dependency_provider.add_dependencies("menu", 1, [("dropdown", Range::any())]); -//! dependency_provider.add_dependencies("dropdown", 1, [("icons", Range::any())]); +//! dependency_provider.add_dependencies("menu", 1, [("dropdown", Range::full())]); +//! dependency_provider.add_dependencies("dropdown", 1, [("icons", Range::full())]); //! dependency_provider.add_dependencies("icons", 1, []); //! //! // Run the algorithm. diff --git a/src/range.rs b/src/range.rs index b0ca3bc4..8d900814 100644 --- a/src/range.rs +++ b/src/range.rs @@ -7,354 +7,425 @@ //! of the ranges building blocks. //! //! Those building blocks are: -//! - [none()](Range::none): the empty set -//! - [any()](Range::any): the set of all possible versions -//! - [exact(v)](Range::exact): the set containing only the version v +//! - [empty()](Range::empty): the empty set +//! - [full()](Range::full): the set of all possible versions +//! - [singleton(v)](Range::singleton): the set containing only the version v //! - [higher_than(v)](Range::higher_than): the set defined by `v <= versions` +//! - [strictly_higher_than(v)](Range::strictly_higher_than): the set defined by `v < versions` +//! - [lower_than(v)](Range::lower_than): the set defined by `versions <= v` //! - [strictly_lower_than(v)](Range::strictly_lower_than): the set defined by `versions < v` //! - [between(v1, v2)](Range::between): the set defined by `v1 <= versions < v2` - -use std::cmp::Ordering; -use std::fmt; -use std::ops::{Bound, RangeBounds}; - -use crate::internal::small_vec::SmallVec; -use crate::version::Version; -use crate::version_set::VersionSet; - -impl VersionSet for Range { - type V = V; - // Constructors - fn empty() -> Self { - Range::none() - } - fn singleton(v: Self::V) -> Self { - Range::exact(v) - } - // Operations - fn complement(&self) -> Self { - self.negate() - } - fn intersection(&self, other: &Self) -> Self { - self.intersection(other) - } - // Membership - fn contains(&self, v: &Self::V) -> bool { - self.contains(v) - } -} - -/// A Range is a set of versions. -#[derive(Debug, Clone, Eq, PartialEq)] -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +//! +//! Ranges can be created from any type that implements [`Ord`] + [`Clone`]. +//! +//! In order to advance the solver front, comparisons of versions sets are necessary in the algorithm. +//! To do those comparisons between two sets S1 and S2 we use the mathematical property that S1 āŠ‚ S2 if and only if S1 āˆ© S2 == S1. +//! We can thus compute an intersection and evaluate an equality to answer if S1 is a subset of S2. +//! But this means that the implementation of equality must be correct semantically. +//! In practice, if equality is derived automatically, this means sets must have unique representations. +//! +//! By migrating from a custom representation for discrete sets in v0.2 +//! to a generic bounded representation for continuous sets in v0.3 +//! we are potentially breaking that assumption in two ways: +//! +//! 1. Minimal and maximal `Unbounded` values can be replaced by their equivalent if it exists. +//! 2. Simplifying adjacent bounds of discrete sets cannot be detected and automated in the generic intersection code. +//! +//! An example for each can be given when `T` is `u32`. +//! First, we can have both segments `S1 = (Unbounded, Included(42u32))` and `S2 = (Included(0), Included(42u32))` +//! that represent the same segment but are structurally different. +//! Thus, a derived equality check would answer `false` to `S1 == S2` while it's true. +//! +//! Second both segments `S1 = (Included(1), Included(5))` and `S2 = (Included(1), Included(3)) + (Included(4), Included(5))` are equal. +//! But without asking the user to provide a `bump` function for discrete sets, +//! the algorithm is not able tell that the space between the right `Included(3)` bound and the left `Included(4)` bound is empty. +//! Thus the algorithm is not able to reduce S2 to its canonical S1 form while computing sets operations like intersections in the generic code. +//! +//! This is likely to lead to user facing theoretically correct but practically nonsensical ranges, +//! like (Unbounded, Excluded(0)) or (Excluded(6), Excluded(7)). +//! In general nonsensical inputs often lead to hard to track bugs. +//! But as far as we can tell this should work in practice. +//! So for now this crate only provides an implementation for continuous ranges. +//! With the v0.3 api the user could choose to bring back the discrete implementation from v0.2, as documented in the guide. +//! If doing so regularly fixes bugs seen by users, we will bring it back into the core library. +//! If we do not see practical bugs, or we get a formal proof that the code cannot lead to error states, then we may remove this warning. + +use crate::{internal::small_vec::SmallVec, version_set::VersionSet}; +use std::ops::RangeBounds; +use std::{ + fmt::{Debug, Display, Formatter}, + ops::Bound::{self, Excluded, Included, Unbounded}, +}; + +/// A Range represents multiple intervals of a continuous range of monotone increasing +/// values. +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +#[cfg_attr(feature = "serde", derive(serde::Serialize))] #[cfg_attr(feature = "serde", serde(transparent))] -pub struct Range { +pub struct Range { segments: SmallVec>, } -type Interval = (V, Option); +type Interval = (Bound, Bound); -// Range building blocks. -impl Range { +impl Range { /// Empty set of versions. - pub fn none() -> Self { + pub fn empty() -> Self { Self { segments: SmallVec::empty(), } } - /// Set of all possible versions. - pub fn any() -> Self { - Self::higher_than(V::lowest()) + /// Set of all possible versions + pub fn full() -> Self { + Self { + segments: SmallVec::one((Unbounded, Unbounded)), + } } - /// Set containing exactly one version. - pub fn exact(v: impl Into) -> Self { - let v = v.into(); + /// Set of all versions higher or equal to some version + pub fn higher_than(v: impl Into) -> Self { Self { - segments: SmallVec::one((v.clone(), Some(v.bump()))), + segments: SmallVec::one((Included(v.into()), Unbounded)), } } - /// Set of all versions higher or equal to some version. - pub fn higher_than(v: impl Into) -> Self { + /// Set of all versions higher to some version + pub fn strictly_higher_than(v: impl Into) -> Self { Self { - segments: SmallVec::one((v.into(), None)), + segments: SmallVec::one((Excluded(v.into()), Unbounded)), } } - /// Set of all versions strictly lower than some version. + /// Set of all versions lower to some version pub fn strictly_lower_than(v: impl Into) -> Self { + Self { + segments: SmallVec::one((Unbounded, Excluded(v.into()))), + } + } + + /// Set of all versions lower or equal to some version + pub fn lower_than(v: impl Into) -> Self { + Self { + segments: SmallVec::one((Unbounded, Included(v.into()))), + } + } + + /// Set of versions greater or equal to `v1` but less than `v2`. + pub fn between(v1: impl Into, v2: impl Into) -> Self { + Self { + segments: SmallVec::one((Included(v1.into()), Excluded(v2.into()))), + } + } +} + +impl Range { + /// Set containing exactly one version + pub fn singleton(v: impl Into) -> Self { let v = v.into(); - if v == V::lowest() { - Self::none() - } else { - Self { - segments: SmallVec::one((V::lowest(), Some(v))), + Self { + segments: SmallVec::one((Included(v.clone()), Included(v))), + } + } + + /// Returns the complement of this Range. + pub fn complement(&self) -> Self { + match self.segments.first() { + // Complement of āˆ… is āˆž + None => Self::full(), + + // Complement of āˆž is āˆ… + Some((Unbounded, Unbounded)) => Self::empty(), + + // First high bound is +āˆž + Some((Included(v), Unbounded)) => Self::strictly_lower_than(v.clone()), + Some((Excluded(v), Unbounded)) => Self::lower_than(v.clone()), + + Some((Unbounded, Included(v))) => { + Self::negate_segments(Excluded(v.clone()), &self.segments[1..]) + } + Some((Unbounded, Excluded(v))) => { + Self::negate_segments(Included(v.clone()), &self.segments[1..]) } + Some((Included(_), Included(_))) + | Some((Included(_), Excluded(_))) + | Some((Excluded(_), Included(_))) + | Some((Excluded(_), Excluded(_))) => Self::negate_segments(Unbounded, &self.segments), } } - /// Set of all versions comprised between two given versions. - /// The lower bound is included and the higher bound excluded. - /// `v1 <= v < v2`. - pub fn between(v1: impl Into, v2: impl Into) -> Self { - let v1 = v1.into(); - let v2 = v2.into(); - if v1 < v2 { - Self { - segments: SmallVec::one((v1, Some(v2))), + /// Helper function performing the negation of intervals in segments. + fn negate_segments(start: Bound, segments: &[Interval]) -> Self { + let mut complement_segments: SmallVec> = SmallVec::empty(); + let mut start = start; + for (v1, v2) in segments { + complement_segments.push(( + start, + match v1 { + Included(v) => Excluded(v.clone()), + Excluded(v) => Included(v.clone()), + Unbounded => unreachable!(), + }, + )); + start = match v2 { + Included(v) => Excluded(v.clone()), + Excluded(v) => Included(v.clone()), + Unbounded => Unbounded, + } + } + if !matches!(start, Unbounded) { + complement_segments.push((start, Unbounded)); + } + + Self { + segments: complement_segments, + } + } +} + +impl Range { + /// Convert to something that can be used with + /// [BTreeMap::range](std::collections::BTreeMap::range). + /// All versions contained in self, will be in the output, + /// but there may be versions in the output that are not contained in self. + /// Returns None if the range is empty. + pub fn bounding_range(&self) -> Option<(Bound<&V>, Bound<&V>)> { + self.segments.first().map(|(start, _)| { + let end = self + .segments + .last() + .expect("if there is a first element, there must be a last element"); + (bound_as_ref(start), bound_as_ref(&end.1)) + }) + } + + /// Returns true if the this Range contains the specified value. + pub fn contains(&self, v: &V) -> bool { + for segment in self.segments.iter() { + if match segment { + (Unbounded, Unbounded) => true, + (Unbounded, Included(end)) => v <= end, + (Unbounded, Excluded(end)) => v < end, + (Included(start), Unbounded) => v >= start, + (Included(start), Included(end)) => v >= start && v <= end, + (Included(start), Excluded(end)) => v >= start && v < end, + (Excluded(start), Unbounded) => v > start, + (Excluded(start), Included(end)) => v > start && v <= end, + (Excluded(start), Excluded(end)) => v > start && v < end, + } { + return true; } - } else { - Self::none() } + false } /// Construct a simple range from anything that impls [RangeBounds] like `v1..v2`. pub fn from_range_bounds(bounds: R) -> Self where R: RangeBounds, - for<'a> &'a IV: Into, + IV: Clone + Into, { let start = match bounds.start_bound() { - Bound::Included(s) => s.into(), - Bound::Excluded(s) => s.into().bump(), - Bound::Unbounded => V::lowest(), + Included(v) => Included(v.clone().into()), + Excluded(v) => Excluded(v.clone().into()), + Unbounded => Unbounded, }; let end = match bounds.end_bound() { - Bound::Included(e) => Some(e.into().bump()), - Bound::Excluded(e) => Some(e.into()), - Bound::Unbounded => None, + Included(v) => Included(v.clone().into()), + Excluded(v) => Excluded(v.clone().into()), + Unbounded => Unbounded, }; - if end.is_some() && end.as_ref() <= Some(&start) { - Self::none() - } else { + if valid_segment(&start, &end) { Self { segments: SmallVec::one((start, end)), } + } else { + Self::empty() } } -} - -// Set operations. -impl Range { - // Negate ################################################################## - /// Compute the complement set of versions. - pub fn negate(&self) -> Self { - match self.segments.first() { - None => Self::any(), // Complement of āˆ… is * - - // First high bound is +āˆž - Some((v, None)) => { - // Complement of * is āˆ… - if v == &V::lowest() { - Self::none() - // Complement of "v <= _" is "_ < v" - } else { - Self::strictly_lower_than(v.clone()) + fn check_invariants(self) -> Self { + if cfg!(debug_assertions) { + for p in self.segments.as_slice().windows(2) { + match (&p[0].1, &p[1].0) { + (Included(l_end), Included(r_start)) => assert!(l_end < r_start), + (Included(l_end), Excluded(r_start)) => assert!(l_end < r_start), + (Excluded(l_end), Included(r_start)) => assert!(l_end < r_start), + (Excluded(l_end), Excluded(r_start)) => assert!(l_end <= r_start), + (_, Unbounded) => panic!(), + (Unbounded, _) => panic!(), } } - - // First high bound is not +āˆž - Some((v1, Some(v2))) => { - if v1 == &V::lowest() { - Self::negate_segments(v2.clone(), &self.segments[1..]) - } else { - Self::negate_segments(V::lowest(), &self.segments) - } + for (s, e) in self.segments.iter() { + assert!(valid_segment(s, e)); } } + self } +} - /// Helper function performing the negation of intervals in segments. - /// For example: - /// [ (v1, None) ] => [ (start, Some(v1)) ] - /// [ (v1, Some(v2)) ] => [ (start, Some(v1)), (v2, None) ] - fn negate_segments(start: V, segments: &[Interval]) -> Range { - let mut complement_segments = SmallVec::empty(); - let mut start = Some(start); - for (v1, maybe_v2) in segments { - // start.unwrap() is fine because `segments` is not exposed, - // and our usage guaranties that only the last segment may contain a None. - complement_segments.push((start.unwrap(), Some(v1.to_owned()))); - start = maybe_v2.to_owned(); - } - if let Some(last) = start { - complement_segments.push((last, None)); - } - - Self { - segments: complement_segments, - } +/// Implementation of [`Bound::as_ref`] which is currently marked as unstable. +fn bound_as_ref(bound: &Bound) -> Bound<&V> { + match bound { + Included(v) => Included(v), + Excluded(v) => Excluded(v), + Unbounded => Unbounded, } +} - // Union and intersection ################################################## +fn valid_segment(start: &Bound, end: &Bound) -> bool { + match (start, end) { + (Included(s), Included(e)) => s <= e, + (Included(s), Excluded(e)) => s < e, + (Excluded(s), Included(e)) => s < e, + (Excluded(s), Excluded(e)) => s < e, + (Unbounded, _) | (_, Unbounded) => true, + } +} - /// Compute the union of two sets of versions. +impl Range { + /// Computes the union of this `Range` and another. pub fn union(&self, other: &Self) -> Self { - self.negate().intersection(&other.negate()).negate() + self.complement() + .intersection(&other.complement()) + .complement() + .check_invariants() } - /// Compute the intersection of two sets of versions. + /// Computes the intersection of two sets of versions. pub fn intersection(&self, other: &Self) -> Self { - let mut segments = SmallVec::empty(); - let mut left_iter = self.segments.iter(); - let mut right_iter = other.segments.iter(); - let mut left = left_iter.next(); - let mut right = right_iter.next(); - loop { - match (left, right) { - // Both left and right still contain a finite interval: - (Some((l1, Some(l2))), Some((r1, Some(r2)))) => { - if l2 <= r1 { - // Intervals are disjoint, progress on the left. - left = left_iter.next(); - } else if r2 <= l1 { - // Intervals are disjoint, progress on the right. - right = right_iter.next(); - } else { - // Intervals are not disjoint. - let start = l1.max(r1).to_owned(); - if l2 < r2 { - segments.push((start, Some(l2.to_owned()))); - left = left_iter.next(); - } else { - segments.push((start, Some(r2.to_owned()))); - right = right_iter.next(); - } - } - } + let mut segments: SmallVec> = SmallVec::empty(); + let mut left_iter = self.segments.iter().peekable(); + let mut right_iter = other.segments.iter().peekable(); + + while let (Some((left_start, left_end)), Some((right_start, right_end))) = + (left_iter.peek(), right_iter.peek()) + { + let start = match (left_start, right_start) { + (Included(l), Included(r)) => Included(std::cmp::max(l, r)), + (Excluded(l), Excluded(r)) => Excluded(std::cmp::max(l, r)), + + (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if i <= e => Excluded(e), + (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if e < i => Included(i), + (s, Unbounded) | (Unbounded, s) => bound_as_ref(s), + _ => unreachable!(), + } + .cloned(); + let end = match (left_end, right_end) { + (Included(l), Included(r)) => Included(std::cmp::min(l, r)), + (Excluded(l), Excluded(r)) => Excluded(std::cmp::min(l, r)), + + (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if i >= e => Excluded(e), + (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if e > i => Included(i), + (s, Unbounded) | (Unbounded, s) => bound_as_ref(s), + _ => unreachable!(), + } + .cloned(); + left_iter.next_if(|(_, e)| e == &end); + right_iter.next_if(|(_, e)| e == &end); + if valid_segment(&start, &end) { + segments.push((start, end)) + } + } - // Right contains an infinite interval: - (Some((l1, Some(l2))), Some((r1, None))) => match l2.cmp(r1) { - Ordering::Less => { - left = left_iter.next(); - } - Ordering::Equal => { - for l in left_iter.cloned() { - segments.push(l) - } - break; - } - Ordering::Greater => { - let start = l1.max(r1).to_owned(); - segments.push((start, Some(l2.to_owned()))); - for l in left_iter.cloned() { - segments.push(l) - } - break; - } - }, + Self { segments }.check_invariants() + } +} - // Left contains an infinite interval: - (Some((l1, None)), Some((r1, Some(r2)))) => match r2.cmp(l1) { - Ordering::Less => { - right = right_iter.next(); - } - Ordering::Equal => { - for r in right_iter.cloned() { - segments.push(r) - } - break; - } - Ordering::Greater => { - let start = l1.max(r1).to_owned(); - segments.push((start, Some(r2.to_owned()))); - for r in right_iter.cloned() { - segments.push(r) - } - break; - } - }, +impl VersionSet for Range { + type V = T; - // Both sides contain an infinite interval: - (Some((l1, None)), Some((r1, None))) => { - let start = l1.max(r1).to_owned(); - segments.push((start, None)); - break; - } + fn empty() -> Self { + Range::empty() + } - // Left or right has ended. - _ => { - break; - } - } - } + fn singleton(v: Self::V) -> Self { + Range::singleton(v) + } - Self { segments } + fn complement(&self) -> Self { + Range::complement(self) } -} -// Other useful functions. -impl Range { - /// Check if a range contains a given version. - pub fn contains(&self, version: &V) -> bool { - for (v1, maybe_v2) in &self.segments { - match maybe_v2 { - None => return v1 <= version, - Some(v2) => { - if version < v1 { - return false; - } else if version < v2 { - return true; - } - } - } - } - false + fn intersection(&self, other: &Self) -> Self { + Range::intersection(self, other) } - /// Return the lowest version in the range (if there is one). - pub fn lowest_version(&self) -> Option { - self.segments.first().map(|(start, _)| start).cloned() + fn contains(&self, v: &Self::V) -> bool { + Range::contains(self, v) } - /// Convert to something that can be used with - /// [BTreeMap::range](std::collections::BTreeMap::range). - /// All versions contained in self, will be in the output, - /// but there may be versions in the output that are not contained in self. - /// Returns None if the range is empty. - pub fn bounding_range(&self) -> Option<(Bound<&V>, Bound<&V>)> { - self.segments.first().map(|(start, _)| { - let end = { - self.segments - .last() - .and_then(|(_, l)| l.as_ref()) - .map(Bound::Excluded) - .unwrap_or(Bound::Unbounded) - }; - (Bound::Included(start), end) - }) + fn full() -> Self { + Range::full() + } + + fn union(&self, other: &Self) -> Self { + Range::union(self, other) } } // REPORT ###################################################################### -impl fmt::Display for Range { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self.segments.as_slice() { - [] => write!(f, "āˆ…"), - [(start, None)] if start == &V::lowest() => write!(f, "āˆ—"), - [(start, None)] => write!(f, "{} <= v", start), - [(start, Some(end))] if end == &start.bump() => write!(f, "{}", start), - [(start, Some(end))] if start == &V::lowest() => write!(f, "v < {}", end), - [(start, Some(end))] => write!(f, "{} <= v < {}", start, end), - more_than_one_interval => { - let string_intervals: Vec<_> = more_than_one_interval - .iter() - .map(interval_to_string) - .collect(); - write!(f, "{}", string_intervals.join(" ")) +impl Display for Range { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + if self.segments.is_empty() { + write!(f, "āˆ…")?; + } else { + for (idx, segment) in self.segments.iter().enumerate() { + if idx > 0 { + write!(f, ", ")?; + } + match segment { + (Unbounded, Unbounded) => write!(f, "*")?, + (Unbounded, Included(v)) => write!(f, "<={v}")?, + (Unbounded, Excluded(v)) => write!(f, "<{v}")?, + (Included(v), Unbounded) => write!(f, ">={v}")?, + (Included(v), Included(b)) => { + if v == b { + write!(f, "{v}")? + } else { + write!(f, ">={v},<={b}")? + } + } + (Included(v), Excluded(b)) => write!(f, ">={v}, <{b}")?, + (Excluded(v), Unbounded) => write!(f, ">{v}")?, + (Excluded(v), Included(b)) => write!(f, ">{v}, <={b}")?, + (Excluded(v), Excluded(b)) => write!(f, ">{v}, <{b}")?, + }; } } + Ok(()) } } -fn interval_to_string((start, maybe_end): &Interval) -> String { - match maybe_end { - Some(end) => format!("[ {}, {} [", start, end), - None => format!("[ {}, āˆž [", start), +// SERIALIZATION ############################################################### + +#[cfg(feature = "serde")] +impl<'de, V: serde::Deserialize<'de>> serde::Deserialize<'de> for Range { + fn deserialize>(deserializer: D) -> Result { + // This enables conversion from the "old" discrete implementation of `Range` to the new + // bounded one. + // + // Serialization is always performed in the new format. + #[derive(serde::Deserialize)] + #[serde(untagged)] + enum EitherInterval { + B(Bound, Bound), + D(V, Option), + } + + let bounds: SmallVec> = serde::Deserialize::deserialize(deserializer)?; + + let mut segments = SmallVec::Empty; + for i in bounds { + match i { + EitherInterval::B(l, r) => segments.push((l, r)), + EitherInterval::D(l, Some(r)) => segments.push((Included(l), Excluded(r))), + EitherInterval::D(l, None) => segments.push((Included(l), Unbounded)), + } + } + + Ok(Range { segments }) } } @@ -364,28 +435,74 @@ fn interval_to_string((start, maybe_end): &Interval) -> String { pub mod tests { use proptest::prelude::*; - use crate::version::NumberVersion; - use super::*; - pub fn strategy() -> impl Strategy> { - prop::collection::vec(any::(), 0..10).prop_map(|mut vec| { - vec.sort_unstable(); - vec.dedup(); - let mut pair_iter = vec.chunks_exact(2); - let mut segments = SmallVec::empty(); - while let Some([v1, v2]) = pair_iter.next() { - segments.push((NumberVersion(*v1), Some(NumberVersion(*v2)))); - } - if let [v] = pair_iter.remainder() { - segments.push((NumberVersion(*v), None)); - } - Range { segments } - }) + /// Generate version sets from a random vector of deltas between bounds. + /// Each bound is randomly inclusive or exclusive. + pub fn strategy() -> impl Strategy> { + ( + any::(), + prop::collection::vec(any::<(u32, bool)>(), 1..10), + ) + .prop_map(|(start_unbounded, deltas)| { + let mut start = if start_unbounded { + Some(Unbounded) + } else { + None + }; + let mut largest: u32 = 0; + let mut last_bound_was_inclusive = false; + let mut segments = SmallVec::Empty; + for (delta, inclusive) in deltas { + // Add the offset to the current bound + largest = match largest.checked_add(delta) { + Some(s) => s, + None => { + // Skip this offset, if it would result in a too large bound. + continue; + } + }; + + let current_bound = if inclusive { + Included(largest) + } else { + Excluded(largest) + }; + + // If we already have a start bound, the next offset defines the complete range. + // If we don't have a start bound, we have to generate one. + if let Some(start_bound) = start.take() { + // If the delta from the start bound is 0, the only authorized configuration is + // Included(x), Included(x) + if delta == 0 && !(matches!(start_bound, Included(_)) && inclusive) { + start = Some(start_bound); + continue; + } + last_bound_was_inclusive = inclusive; + segments.push((start_bound, current_bound)); + } else { + // If the delta from the end bound of the last range is 0 and + // any of the last ending or current starting bound is inclusive, + // we skip the delta because they basically overlap. + if delta == 0 && (last_bound_was_inclusive || inclusive) { + continue; + } + start = Some(current_bound); + } + } + + // If we still have a start bound, but didnt have enough deltas to complete another + // segment, we add an unbounded upperbound. + if let Some(start_bound) = start { + segments.push((start_bound, Unbounded)); + } + + return Range { segments }.check_invariants(); + }) } - fn version_strat() -> impl Strategy { - any::().prop_map(NumberVersion) + fn version_strat() -> impl Strategy { + any::() } proptest! { @@ -394,17 +511,17 @@ pub mod tests { #[test] fn negate_is_different(range in strategy()) { - assert_ne!(range.negate(), range); + assert_ne!(range.complement(), range); } #[test] fn double_negate_is_identity(range in strategy()) { - assert_eq!(range.negate().negate(), range); + assert_eq!(range.complement().complement(), range); } #[test] fn negate_contains_opposite(range in strategy(), version in version_strat()) { - assert_ne!(range.contains(&version), range.negate().contains(&version)); + assert_ne!(range.contains(&version), range.complement().contains(&version)); } // Testing intersection ---------------------------- @@ -416,12 +533,12 @@ pub mod tests { #[test] fn intersection_with_any_is_identity(range in strategy()) { - assert_eq!(Range::any().intersection(&range), range); + assert_eq!(Range::full().intersection(&range), range); } #[test] fn intersection_with_none_is_none(range in strategy()) { - assert_eq!(Range::none().intersection(&range), Range::none()); + assert_eq!(Range::empty().intersection(&range), Range::empty()); } #[test] @@ -436,7 +553,7 @@ pub mod tests { #[test] fn intesection_of_complements_is_none(range in strategy()) { - assert_eq!(range.negate().intersection(&range), Range::none()); + assert_eq!(range.complement().intersection(&range), Range::empty()); } #[test] @@ -448,7 +565,7 @@ pub mod tests { #[test] fn union_of_complements_is_any(range in strategy()) { - assert_eq!(range.negate().union(&range), Range::any()); + assert_eq!(range.complement().union(&range), Range::full()); } #[test] @@ -460,17 +577,17 @@ pub mod tests { #[test] fn always_contains_exact(version in version_strat()) { - assert!(Range::exact(version).contains(&version)); + assert!(Range::singleton(version).contains(&version)); } #[test] fn contains_negation(range in strategy(), version in version_strat()) { - assert_ne!(range.contains(&version), range.negate().contains(&version)); + assert_ne!(range.contains(&version), range.complement().contains(&version)); } #[test] fn contains_intersection(range in strategy(), version in version_strat()) { - assert_eq!(range.contains(&version), range.intersection(&Range::exact(version)) != Range::none()); + assert_eq!(range.contains(&version), range.intersection(&Range::singleton(version)) != Range::empty()); } #[test] @@ -482,14 +599,14 @@ pub mod tests { #[test] fn from_range_bounds(range in any::<(Bound, Bound)>(), version in version_strat()) { - let rv: Range = Range::from_range_bounds(range); - assert_eq!(range.contains(&version.0), rv.contains(&version)); + let rv: Range<_> = Range::from_range_bounds(range); + assert_eq!(range.contains(&version), rv.contains(&version)); } #[test] fn from_range_bounds_round_trip(range in any::<(Bound, Bound)>()) { - let rv: Range = Range::from_range_bounds(range); - let rv2: Range = rv.bounding_range().map(Range::from_range_bounds::<_, NumberVersion>).unwrap_or_else(Range::none); + let rv: Range = Range::from_range_bounds(range); + let rv2: Range = rv.bounding_range().map(Range::from_range_bounds::<_, u32>).unwrap_or_else(Range::empty); assert_eq!(rv, rv2); } } diff --git a/src/term.rs b/src/term.rs index 3028dbe1..cf7aa6f7 100644 --- a/src/term.rs +++ b/src/term.rs @@ -182,10 +182,9 @@ impl Display for Term { pub mod tests { use super::*; use crate::range::Range; - use crate::version::NumberVersion; use proptest::prelude::*; - pub fn strategy() -> impl Strategy>> { + pub fn strategy() -> impl Strategy>> { prop_oneof![ crate::range::tests::strategy().prop_map(Term::Positive), crate::range::tests::strategy().prop_map(Term::Negative), diff --git a/tests/examples.rs b/tests/examples.rs index 9040bc23..9c11d4fe 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -191,11 +191,11 @@ fn conflict_with_partial_satisfier() { fn double_choices() { init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, NumVS>::new(); - dependency_provider.add_dependencies("a", 0, [("b", Range::any()), ("c", Range::any())]); - dependency_provider.add_dependencies("b", 0, [("d", Range::exact(0))]); - dependency_provider.add_dependencies("b", 1, [("d", Range::exact(1))]); + dependency_provider.add_dependencies("a", 0, [("b", Range::full()), ("c", Range::full())]); + dependency_provider.add_dependencies("b", 0, [("d", Range::singleton(0))]); + dependency_provider.add_dependencies("b", 1, [("d", Range::singleton(1))]); dependency_provider.add_dependencies("c", 0, []); - dependency_provider.add_dependencies("c", 1, [("d", Range::exact(2))]); + dependency_provider.add_dependencies("c", 1, [("d", Range::singleton(2))]); dependency_provider.add_dependencies("d", 0, []); // Solution. diff --git a/tests/proptest.rs b/tests/proptest.rs index fa775720..8ec22e80 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -97,7 +97,7 @@ type SemVS = Range; #[should_panic] fn should_cancel_can_panic() { let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); - dependency_provider.add_dependencies(0, 0, [(666, Range::any())]); + dependency_provider.add_dependencies(0, 0, [(666, Range::full())]); // Run the algorithm. let _ = resolve( @@ -197,13 +197,13 @@ pub fn registry_strategy( deps.push(( dep_name, if c == 0 && d == s_last_index { - Range::any() + 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::exact(s[c].0) + Range::singleton(s[c].0) } else { Range::between(s[c].0, s[d].0 + 1) }, @@ -227,7 +227,7 @@ pub fn registry_strategy( dependency_provider.add_dependencies( name, ver, - deps.unwrap_or_else(|| vec![(bad_name.clone(), Range::any())]), + deps.unwrap_or_else(|| vec![(bad_name.clone(), Range::full())]), ); } diff --git a/tests/tests.rs b/tests/tests.rs index 77e4385b..b1d05a4a 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -16,7 +16,7 @@ fn same_result_on_repeated_runs() { dependency_provider.add_dependencies("b", 0, []); dependency_provider.add_dependencies("b", 1, [("c", Range::between(0, 1))]); - dependency_provider.add_dependencies("a", 0, [("b", Range::any()), ("c", Range::any())]); + dependency_provider.add_dependencies("a", 0, [("b", Range::full()), ("c", Range::full())]); let name = "a"; let ver = NumberVersion(0); @@ -32,13 +32,13 @@ fn same_result_on_repeated_runs() { #[test] fn should_always_find_a_satisfier() { let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); - dependency_provider.add_dependencies("a", 0, [("b", Range::none())]); + dependency_provider.add_dependencies("a", 0, [("b", Range::empty())]); assert!(matches!( resolve(&dependency_provider, "a", 0), Err(PubGrubError::DependencyOnTheEmptySet { .. }) )); - dependency_provider.add_dependencies("c", 0, [("a", Range::any())]); + dependency_provider.add_dependencies("c", 0, [("a", Range::full())]); assert!(matches!( resolve(&dependency_provider, "c", 0), Err(PubGrubError::DependencyOnTheEmptySet { .. }) @@ -48,7 +48,7 @@ fn should_always_find_a_satisfier() { #[test] fn cannot_depend_on_self() { let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); - dependency_provider.add_dependencies("a", 0, [("a", Range::any())]); + dependency_provider.add_dependencies("a", 0, [("a", Range::full())]); assert!(matches!( resolve(&dependency_provider, "a", 0), Err(PubGrubError::SelfDependency { .. }) From 78562c53a6927e2e38d7208f4fd238511329bdf6 Mon Sep 17 00:00:00 2001 From: Kian-Meng Ang Date: Sat, 30 Jul 2022 01:40:36 +0800 Subject: [PATCH 09/20] docs: fix typos (#119) --- CHANGELOG.md | 2 +- src/internal/small_vec.rs | 6 +++--- src/range.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51f58e14..abe44da9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,7 +48,7 @@ The gist of it is: #### Added -- Links to code items in the code documenation. +- Links to code items in the code documentation. - New `"serde"` feature that allows serializing some library types, useful for making simple reproducible bug reports. - New variants for `error::PubGrubError` which are `DependencyOnTheEmptySet`, `SelfDependency`, `ErrorChoosingPackageVersion` and `ErrorInShouldCancel`. diff --git a/src/internal/small_vec.rs b/src/internal/small_vec.rs index c2a178a9..4c2a97df 100644 --- a/src/internal/small_vec.rs +++ b/src/internal/small_vec.rs @@ -202,11 +202,11 @@ pub mod tests { proptest! { #[test] - fn push_and_pop(comands: Vec>) { + fn push_and_pop(commands: Vec>) { let mut v = vec![]; let mut sv = SmallVec::Empty; - for comand in comands { - match comand { + for command in commands { + match command { Some(i) => { v.push(i); sv.push(i); diff --git a/src/range.rs b/src/range.rs index 8d900814..fe2aaef4 100644 --- a/src/range.rs +++ b/src/range.rs @@ -491,7 +491,7 @@ pub mod tests { } } - // If we still have a start bound, but didnt have enough deltas to complete another + // If we still have a start bound, but didn't have enough deltas to complete another // segment, we add an unbounded upperbound. if let Some(start_bound) = start { segments.push((start_bound, Unbounded)); From afe2d3c9c7c9f80bb6745d3f550224eeed0a1cea Mon Sep 17 00:00:00 2001 From: Patrick Elsen Date: Sun, 11 Jun 2023 15:02:21 +0200 Subject: [PATCH 10/20] refactor: clean up src/solver.rs (#127) - Reduces nesting, to increase legibility of code - Turns expect into error - Removes unwrap() --- src/solver.rs | 153 ++++++++++++++++++++++++++------------------------ 1 file changed, 81 insertions(+), 72 deletions(-) diff --git a/src/solver.rs b/src/solver.rs index 846f220c..275ba1d9 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -78,6 +78,7 @@ use crate::internal::incompatibility::Incompatibility; use crate::package::Package; use crate::type_aliases::{DependencyConstraints, Map, SelectedDependencies}; use crate::version_set::VersionSet; +use log::{debug, info}; /// Main function of the library. /// Finds a set of packages satisfying dependency bounds for a given package + version pair. @@ -94,36 +95,37 @@ pub fn resolve( .should_cancel() .map_err(|err| PubGrubError::ErrorInShouldCancel(err))?; - log::info!("unit_propagation: {}", &next); + info!("unit_propagation: {}", &next); state.unit_propagation(next)?; - log::debug!( + + debug!( "Partial solution after unit propagation: {}", state.partial_solution ); - let potential_packages = state.partial_solution.potential_packages(); - if potential_packages.is_none() { - drop(potential_packages); - // The borrow checker did not like using a match on potential_packages. - // This `if ... is_none ... drop` is a workaround. - // I believe this is a case where Polonius could help, when and if it lands in rustc. + let Some(potential_packages) = state.partial_solution.potential_packages() else { return state.partial_solution.extract_solution().ok_or_else(|| { PubGrubError::Failure( "How did we end up with no package to choose but no solution?".into(), ) }); - } + }; + let decision = dependency_provider - .choose_package_version(potential_packages.unwrap()) + .choose_package_version(potential_packages) .map_err(PubGrubError::ErrorChoosingPackageVersion)?; - log::info!("DP chose: {} @ {:?}", decision.0, decision.1); + info!("DP chose: {} @ {:?}", decision.0, decision.1); + next = decision.0.clone(); // Pick the next compatible version. let term_intersection = state .partial_solution .term_intersection_for_package(&next) - .expect("a package was chosen but we don't have a term."); + .ok_or_else(|| { + PubGrubError::Failure("a package was chosen but we don't have a term.".into()) + })?; + let v = match decision.1 { None => { let inc = Incompatibility::no_versions(next.clone(), term_intersection.clone()); @@ -132,79 +134,86 @@ pub fn resolve( } Some(x) => x, }; + if !term_intersection.contains(&v) { return Err(PubGrubError::ErrorChoosingPackageVersion( "choose_package_version picked an incompatible version".into(), )); } - if added_dependencies + let is_new_dependency = added_dependencies .entry(next.clone()) .or_default() - .insert(v.clone()) - { - // Retrieve that package dependencies. - let p = &next; - let dependencies = match dependency_provider.get_dependencies(p, &v).map_err(|err| { - PubGrubError::ErrorRetrievingDependencies { + .insert(v.clone()); + + if !is_new_dependency { + // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied + // terms and can add the decision directly. + info!("add_decision (not first time): {} @ {}", &next, v); + state.partial_solution.add_decision(next.clone(), v); + continue; + } + + // Retrieve that package dependencies. + let p = &next; + let dependencies = dependency_provider.get_dependencies(p, &v).map_err(|err| { + PubGrubError::ErrorRetrievingDependencies { + package: p.clone(), + version: v.clone(), + source: err, + } + })?; + + let known_dependencies = match dependencies { + Dependencies::Unknown => { + state.add_incompatibility(Incompatibility::unavailable_dependencies( + p.clone(), + v.clone(), + )); + continue; + } + Dependencies::Known(x) if x.contains_key(p) => { + return Err(PubGrubError::SelfDependency { package: p.clone(), version: v.clone(), - source: err, - } - })? { - Dependencies::Unknown => { - state.add_incompatibility(Incompatibility::unavailable_dependencies( - p.clone(), - v.clone(), - )); - continue; - } - Dependencies::Known(x) => { - if x.contains_key(p) { - return Err(PubGrubError::SelfDependency { - package: p.clone(), - version: v.clone(), - }); - } - 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) => { + if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&VS::empty()) { + return Err(PubGrubError::DependencyOnTheEmptySet { + package: p.clone(), + version: v.clone(), + dependent: dependent.clone(), + }); } - }; - - // Add that package and version if the dependencies are not problematic. - let dep_incompats = - state.add_incompatibility_from_dependencies(p.clone(), v.clone(), &dependencies); - - // TODO: I don't think this check can actually happen. - // We might want to put it under #[cfg(debug_assertions)]. - if state.incompatibility_store[dep_incompats.clone()] - .iter() - .any(|incompat| state.is_terminal(incompat)) - { - // 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(), - )); + + x } - state.partial_solution.add_version( - p.clone(), - v, - dep_incompats, - &state.incompatibility_store, - ); - } else { - // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied - // terms and can add the decision directly. - log::info!("add_decision (not first time): {} @ {}", &next, v); - state.partial_solution.add_decision(next.clone(), v); + }; + + // 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, + dep_incompats, + &state.incompatibility_store, + ); } } From 879fd6ba9d97c4472b3ec590ca08853a7643928a Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 2 Oct 2023 16:14:32 +0000 Subject: [PATCH 11/20] refactor: make CI green --- src/internal/arena.rs | 2 +- src/internal/partial_solution.rs | 6 +++--- src/package.rs | 6 +++--- src/report.rs | 4 ++-- src/solver.rs | 8 ++++---- tests/proptest.rs | 2 +- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/internal/arena.rs b/src/internal/arena.rs index 75d04c82..7b4fc160 100644 --- a/src/internal/arena.rs +++ b/src/internal/arena.rs @@ -55,7 +55,7 @@ impl Id { } fn from(n: u32) -> Self { Self { - raw: n as u32, + raw: n, _ty: PhantomData, } } diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 23960a45..1b683b9d 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MPL-2.0 //! A Memory acts like a structured partial solution -//! where terms are regrouped by package in a [Map](crate::type_aliases::Map). +//! where terms are regrouped by package in a [Map]. use std::fmt::Display; @@ -147,7 +147,7 @@ impl PartialSolution { } } self.current_decision_level = self.current_decision_level.increment(); - let mut pa = self + let pa = self .package_assignments .get_mut(&package) .expect("Derivations must already exist"); @@ -177,7 +177,7 @@ impl PartialSolution { self.next_global_index += 1; match self.package_assignments.entry(package) { Entry::Occupied(mut occupied) => { - let mut pa = occupied.get_mut(); + let pa = occupied.get_mut(); pa.highest_decision_level = self.current_decision_level; match &mut pa.assignments_intersection { // Check that add_derivation is never called in the wrong context. diff --git a/src/package.rs b/src/package.rs index e36b91ed..36c6069e 100644 --- a/src/package.rs +++ b/src/package.rs @@ -2,16 +2,16 @@ //! Trait for identifying packages. //! Automatically implemented for traits implementing -//! [Clone] + [Eq] + [Hash] + [Debug] + [Display](std::fmt::Display). +//! [Clone] + [Eq] + [Hash] + [Debug] + [Display]. use std::fmt::{Debug, Display}; use std::hash::Hash; /// Trait for identifying packages. /// Automatically implemented for types already implementing -/// [Clone] + [Eq] + [Hash] + [Debug] + [Display](std::fmt::Display). +/// [Clone] + [Eq] + [Hash] + [Debug] + [Display]. pub trait Package: Clone + Eq + Hash + Debug + Display {} /// Automatically implement the Package trait for any type -/// that already implement [Clone] + [Eq] + [Hash] + [Debug] + [Display](std::fmt::Display). +/// that already implement [Clone] + [Eq] + [Hash] + [Debug] + [Display]. impl Package for T {} diff --git a/src/report.rs b/src/report.rs index 94db9c3c..ff0b2d3f 100644 --- a/src/report.rs +++ b/src/report.rs @@ -197,7 +197,7 @@ impl DefaultStringReporter { fn build_recursive(&mut self, derived: &Derived) { self.build_recursive_helper(derived); if let Some(id) = derived.shared_id { - if self.shared_with_ref.get(&id) == None { + if self.shared_with_ref.get(&id).is_none() { self.add_line_ref(); self.shared_with_ref.insert(id, self.ref_count); } @@ -260,7 +260,7 @@ impl DefaultStringReporter { // and finally conclude. (None, None) => { self.build_recursive(derived1); - if derived1.shared_id != None { + if derived1.shared_id.is_some() { self.lines.push("".into()); self.build_recursive(current); } else { diff --git a/src/solver.rs b/src/solver.rs index 275ba1d9..3962f3d5 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -27,8 +27,8 @@ //! //! The algorithm is generic and works for any type of dependency system //! as long as packages (P) and versions (V) implement -//! the [Package](crate::package::Package) and [Version](crate::version::Version) traits. -//! [Package](crate::package::Package) is strictly equivalent and automatically generated +//! the [Package] and [Version](crate::version::Version) traits. +//! [Package] is strictly equivalent and automatically generated //! for any type that implement [Clone] + [Eq] + [Hash] + [Debug] + [Display](std::fmt::Display). //! [Version](crate::version::Version) simply states that versions are ordered, //! that there should be @@ -298,13 +298,13 @@ where { let count_valid = |(p, set): &(T, U)| { list_available_versions(p.borrow()) - .filter(|v| set.borrow().contains(v.borrow())) + .filter(|v| set.borrow().contains(v)) .count() }; let (pkg, set) = potential_packages .min_by_key(count_valid) .expect("potential_packages gave us an empty iterator"); - let version = list_available_versions(pkg.borrow()).find(|v| set.borrow().contains(v.borrow())); + let version = list_available_versions(pkg.borrow()).find(|v| set.borrow().contains(v)); (pkg, version) } diff --git a/tests/proptest.rs b/tests/proptest.rs index 8ec22e80..34c84c97 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -507,7 +507,7 @@ fn large_case() { let mut sat = SatResolve::new(&dependency_provider); for p in dependency_provider.packages() { for n in dependency_provider.versions(p).unwrap() { - if let Ok(s) = resolve(&dependency_provider, p.clone(), n.clone()) { + if let Ok(s) = resolve(&dependency_provider, p, n.clone()) { assert!(sat.sat_is_valid_solution(&s)); } else { assert!(!sat.sat_resolve(p, &n)); From 29c48fb9f3daa11bd02794edd55060d0b01ee705 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 19 Oct 2023 13:42:35 -0400 Subject: [PATCH 12/20] feat: extend error type to Send + Sync (#136) --- examples/caching_dependency_provider.rs | 4 ++-- src/error.rs | 6 +++--- src/lib.rs | 4 ++-- src/solver.rs | 10 +++++----- tests/proptest.rs | 18 +++++++++++++----- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs index cb278942..bec7d2ab 100644 --- a/examples/caching_dependency_provider.rs +++ b/examples/caching_dependency_provider.rs @@ -35,7 +35,7 @@ impl> DependencyProvid fn choose_package_version, U: std::borrow::Borrow>( &self, packages: impl Iterator, - ) -> Result<(T, Option), Box> { + ) -> Result<(T, Option), Box> { self.remote_dependencies.choose_package_version(packages) } @@ -44,7 +44,7 @@ impl> DependencyProvid &self, package: &P, version: &VS::V, - ) -> Result, Box> { + ) -> Result, Box> { let mut cache = self.cached_dependencies.borrow_mut(); match cache.get_dependencies(package, version) { Ok(Dependencies::Unknown) => { diff --git a/src/error.rs b/src/error.rs index 1098706c..b0633022 100644 --- a/src/error.rs +++ b/src/error.rs @@ -27,7 +27,7 @@ pub enum PubGrubError { version: VS::V, /// Error raised by the implementer of /// [DependencyProvider](crate::solver::DependencyProvider). - source: Box, + source: Box, }, /// Error arising when the implementer of @@ -63,12 +63,12 @@ pub enum PubGrubError { /// returned an error in the method /// [choose_package_version](crate::solver::DependencyProvider::choose_package_version). #[error("Decision making failed")] - ErrorChoosingPackageVersion(Box), + ErrorChoosingPackageVersion(Box), /// Error arising when the implementer of [DependencyProvider](crate::solver::DependencyProvider) /// returned an error in the method [should_cancel](crate::solver::DependencyProvider::should_cancel). #[error("We should cancel")] - ErrorInShouldCancel(Box), + ErrorInShouldCancel(Box), /// Something unexpected happened. #[error("{0}")] diff --git a/src/lib.rs b/src/lib.rs index 86debdcb..3934b8f3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -89,7 +89,7 @@ //! type SemVS = Range; //! //! impl DependencyProvider for MyDependencyProvider { -//! fn choose_package_version, U: Borrow>(&self,packages: impl Iterator) -> Result<(T, Option), Box> { +//! fn choose_package_version, U: Borrow>(&self,packages: impl Iterator) -> Result<(T, Option), Box> { //! unimplemented!() //! } //! @@ -97,7 +97,7 @@ //! &self, //! package: &String, //! version: &SemanticVersion, -//! ) -> Result, Box> { +//! ) -> Result, Box> { //! unimplemented!() //! } //! } diff --git a/src/solver.rs b/src/solver.rs index 3962f3d5..a38d2871 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -259,7 +259,7 @@ pub trait DependencyProvider { fn choose_package_version, U: Borrow>( &self, potential_packages: impl Iterator, - ) -> Result<(T, Option), Box>; + ) -> Result<(T, Option), Box>; /// Retrieves the package dependencies. /// Return [Dependencies::Unknown] if its dependencies are unknown. @@ -267,14 +267,14 @@ pub trait DependencyProvider { &self, package: &P, version: &VS::V, - ) -> Result, Box>; + ) -> Result, Box>; /// This is called fairly regularly during the resolution, /// if it returns an Err then resolution will be terminated. /// This is helpful if you want to add some form of early termination like a timeout, /// or you want to add some form of user feedback if things are taking a while. /// If not provided the resolver will run as long as needed. - fn should_cancel(&self) -> Result<(), Box> { + fn should_cancel(&self) -> Result<(), Box> { Ok(()) } } @@ -384,7 +384,7 @@ impl DependencyProvider for OfflineDependency fn choose_package_version, U: Borrow>( &self, potential_packages: impl Iterator, - ) -> Result<(T, Option), Box> { + ) -> Result<(T, Option), Box> { Ok(choose_package_with_fewest_versions( |p| { self.dependencies @@ -402,7 +402,7 @@ impl DependencyProvider for OfflineDependency &self, package: &P, version: &VS::V, - ) -> Result, Box> { + ) -> Result, Box> { Ok(match self.dependencies(package, version) { None => Dependencies::Unknown, Some(dependencies) => Dependencies::Known(dependencies), diff --git a/tests/proptest.rs b/tests/proptest.rs index 34c84c97..cb66d3e6 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -35,14 +35,18 @@ impl DependencyProvider fn choose_package_version, U: std::borrow::Borrow>( &self, potential_packages: impl Iterator, - ) -> Result<(T, Option), Box> { + ) -> Result<(T, Option), Box> { Ok(choose_package_with_fewest_versions( |p| self.0.versions(p).into_iter().flatten().cloned(), potential_packages, )) } - fn get_dependencies(&self, p: &P, v: &VS::V) -> Result, Box> { + fn get_dependencies( + &self, + p: &P, + v: &VS::V, + ) -> Result, Box> { self.0.get_dependencies(p, v) } } @@ -73,15 +77,19 @@ impl> DependencyProvid fn choose_package_version, U: std::borrow::Borrow>( &self, potential_packages: impl Iterator, - ) -> Result<(T, Option), Box> { + ) -> Result<(T, Option), Box> { self.dp.choose_package_version(potential_packages) } - fn get_dependencies(&self, p: &P, v: &VS::V) -> Result, Box> { + fn get_dependencies( + &self, + p: &P, + v: &VS::V, + ) -> Result, Box> { self.dp.get_dependencies(p, v) } - fn should_cancel(&self) -> Result<(), Box> { + fn should_cancel(&self) -> Result<(), Box> { assert!(self.start_time.elapsed().as_secs() < 60); let calls = self.call_count.get(); assert!(calls < self.max_calls); From 75618a077458bb26b7585d6853f987b1d1b11c7f Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 19 Oct 2023 18:07:52 +0000 Subject: [PATCH 13/20] 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 { .. }) )); } From 8951e37fe923a7edd5a78ed5f49f165b0fdc48de Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 25 Oct 2023 11:29:47 +0200 Subject: [PATCH 14/20] feat: more verbose debug assert --- src/internal/partial_solution.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 1b683b9d..c2d88bed 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -141,7 +141,13 @@ impl PartialSolution { AssignmentsIntersection::Decision(_) => panic!("Already existing decision"), // Cannot be called if the versions is not contained in the terms intersection. AssignmentsIntersection::Derivations(term) => { - debug_assert!(term.contains(&version)) + debug_assert!( + term.contains(&version), + "{}: {} was expected to be contained in {}", + package, + version, + term, + ) } }, } From 67e3be24778ea09b3cb4210e724cbd3be11452d4 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Sun, 22 Oct 2023 21:09:17 -0400 Subject: [PATCH 15/20] test: prop test for error report and refactor --- src/solver.rs | 2 +- tests/proptest.rs | 233 +++++++++++++++++++++---------- tests/sat_dependency_provider.rs | 43 +++--- 3 files changed, 186 insertions(+), 92 deletions(-) diff --git a/src/solver.rs b/src/solver.rs index e28e62de..7354ff7d 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -175,7 +175,7 @@ pub fn resolve( Dependencies::Known(x) if x.contains_key(p) => { return Err(PubGrubError::SelfDependency { package: p.clone(), - version: v.clone(), + version: v, }); } Dependencies::Known(x) => x, diff --git a/tests/proptest.rs b/tests/proptest.rs index 382a6c36..adea37b2 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -5,11 +5,12 @@ use std::{collections::BTreeSet as Set, error::Error}; use pubgrub::error::PubGrubError; use pubgrub::package::Package; use pubgrub::range::Range; -use pubgrub::report::{DefaultStringReporter, Reporter}; +use pubgrub::report::{DefaultStringReporter, DerivationTree, External, Reporter}; use pubgrub::solver::{ choose_package_with_fewest_versions, resolve, Dependencies, DependencyProvider, OfflineDependencyProvider, }; +use pubgrub::type_aliases::SelectedDependencies; use pubgrub::version::{NumberVersion, SemanticVersion}; use pubgrub::version_set::VersionSet; @@ -98,6 +99,18 @@ impl> DependencyProvid } } +fn timeout_resolve>( + dependency_provider: DP, + name: P, + version: impl Into, +) -> Result, PubGrubError> { + resolve( + &TimeoutDependencyProvider::new(dependency_provider, 50_000), + name, + version, + ) +} + type NumVS = Range; type SemVS = Range; @@ -269,6 +282,110 @@ fn meta_test_deep_trees_from_strategy() { ); } +/// Removes versions from the dependency provider where the retain function returns false. +/// Solutions are constructed as a set of versions. +/// If there are fewer versions available, there are fewer valid solutions available. +/// If there was no solution to a resolution in the original dependency provider, +/// then there must still be no solution with some options removed. +/// If there was a solution to a resolution in the original dependency provider, +/// there may not be a solution after versions are removes iif removed versions were critical for all valid solutions. +fn retain_versions( + dependency_provider: &OfflineDependencyProvider, + mut retain: impl FnMut(&N, &VS::V) -> bool, +) -> OfflineDependencyProvider { + let mut smaller_dependency_provider = OfflineDependencyProvider::new(); + + for n in dependency_provider.packages() { + for v in dependency_provider.versions(n).unwrap() { + if !retain(n, v) { + continue; + } + let deps = match dependency_provider.get_dependencies(&n, &v).unwrap() { + Dependencies::Unknown => panic!(), + Dependencies::Known(deps) => deps, + }; + smaller_dependency_provider.add_dependencies(n.clone(), v.clone(), deps) + } + } + smaller_dependency_provider +} + +/// Removes dependencies from the dependency provider where the retain function returns false. +/// Solutions are constraned by having to fulfill all the dependencies. +/// If there are fewer dependencies required, there are more valid solutions. +/// If there was a solution to a resolution in the original dependency provider, +/// then there must still be a solution after dependencies are removed. +/// If there was no solution to a resolution in the original dependency provider, +/// there may now be a solution after dependencies are removed. +fn retain_dependencies( + dependency_provider: &OfflineDependencyProvider, + mut retain: impl FnMut(&N, &VS::V, &N) -> bool, +) -> OfflineDependencyProvider { + let mut smaller_dependency_provider = OfflineDependencyProvider::new(); + 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, + }; + smaller_dependency_provider.add_dependencies( + n.clone(), + v.clone(), + deps.iter().filter_map(|(dep, range)| { + if !retain(n, v, dep) { + None + } else { + Some((dep.clone(), range.clone())) + } + }), + ); + } + } + smaller_dependency_provider +} + +fn errors_the_same_with_only_report_dependencies( + dependency_provider: OfflineDependencyProvider, + name: N, + ver: NumberVersion, +) { + let Err(PubGrubError::NoSolution(tree)) = + timeout_resolve(dependency_provider.clone(), name.clone(), ver) + else { + return; + }; + + fn recursive( + to_retain: &mut Vec<(N, VS, N)>, + tree: &DerivationTree, + ) { + match tree { + DerivationTree::External(External::FromDependencyOf(n1, vs1, n2, _)) => { + to_retain.push((n1.clone(), vs1.clone(), n2.clone())); + } + DerivationTree::Derived(d) => { + recursive(to_retain, &*d.cause1); + recursive(to_retain, &*d.cause2); + } + _ => {} + } + } + + let mut to_retain = Vec::new(); + recursive(&mut to_retain, &tree); + + let removed_provider = retain_dependencies(&dependency_provider, |p, v, d| { + to_retain + .iter() + .any(|(n1, vs1, n2)| n1 == p && vs1.contains(v) && n2 == d) + }); + + assert!( + timeout_resolve(removed_provider.clone(), name, ver).is_err(), + "The full index errored filtering to only dependencies in the derivation tree succeeded" + ); +} + proptest! { #![proptest_config(ProptestConfig { max_shrink_iters: @@ -289,7 +406,7 @@ proptest! { (dependency_provider, cases) in registry_strategy(string_names()) ) { for (name, ver) in cases { - let _ = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver); + _ = timeout_resolve(dependency_provider.clone(), name, ver); } } @@ -299,7 +416,7 @@ proptest! { (dependency_provider, cases) in registry_strategy(0u16..665) ) { for (name, ver) in cases { - let _ = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver); + _ = timeout_resolve(dependency_provider.clone(), name, ver); } } @@ -309,11 +426,17 @@ proptest! { ) { let mut sat = SatResolve::new(&dependency_provider); for (name, ver) in cases { - if let Ok(s) = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver) { - prop_assert!(sat.sat_is_valid_solution(&s)); - } else { - prop_assert!(!sat.sat_resolve(&name, &ver)); - } + let res = timeout_resolve(dependency_provider.clone(), name, ver); + sat.check_resolve(&res, &name, &ver); + } + } + + #[test] + fn prop_errors_the_same_with_only_report_dependencies( + (dependency_provider, cases) in registry_strategy(0u16..665) + ) { + for (name, ver) in cases { + errors_the_same_with_only_report_dependencies(dependency_provider.clone(), name, ver); } } @@ -323,9 +446,9 @@ proptest! { (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); + let one = timeout_resolve(dependency_provider.clone(), name, ver); for _ in 0..3 { - match (&one, &resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver)) { + match (&one, &timeout_resolve(dependency_provider.clone(), name, ver)) { (Ok(l), Ok(r)) => assert_eq!(l, r), (Err(PubGrubError::NoSolution(derivation_l)), Err(PubGrubError::NoSolution(derivation_r))) => { prop_assert_eq!( @@ -346,8 +469,8 @@ proptest! { ) { let reverse_provider = OldestVersionsDependencyProvider(dependency_provider.clone()); for (name, ver) in cases { - let l = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver); - let r = resolve(&TimeoutDependencyProvider::new(reverse_provider.clone(), 50_000), name, ver); + let l = timeout_resolve(dependency_provider.clone(), name, ver); + let r = timeout_resolve(reverse_provider.clone(), name, ver); match (&l, &r) { (Ok(_), Ok(_)) => (), (Err(_), Err(_)) => (), @@ -362,7 +485,7 @@ proptest! { indexes_to_remove in prop::collection::vec((any::(), any::(), any::()), 1..10) ) { let packages: Vec<_> = dependency_provider.packages().collect(); - let mut removed_provider = dependency_provider.clone(); + let mut to_remove = Set::new(); for (package_idx, version_idx, dep_idx) in indexes_to_remove { let package = package_idx.get(&packages); let versions: Vec<_> = dependency_provider @@ -377,29 +500,17 @@ proptest! { Dependencies::Known(d) => d.into_iter().collect(), }; if !dependencies.is_empty() { - let dependency = dep_idx.get(&dependencies).0; - removed_provider.add_dependencies( - **package, - **version, - dependencies.into_iter().filter(|x| x.0 != dependency), - ) + to_remove.insert((package, **version, dep_idx.get(&dependencies).0)); } } + let removed_provider = retain_dependencies( + &dependency_provider, + |p, v, d| {!to_remove.contains(&(&p, *v, *d))} + ); for (name, ver) in cases { - if resolve( - &TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), - name, - ver, - ) - .is_ok() - { + if timeout_resolve(dependency_provider.clone(), name, ver).is_ok() { prop_assert!( - resolve( - &TimeoutDependencyProvider::new(removed_provider.clone(), 50_000), - name, - ver - ) - .is_ok(), + timeout_resolve(removed_provider.clone(), name, ver).is_ok(), "full index worked for `{} = \"={}\"` but removing some deps broke it!", name, ver, @@ -424,24 +535,16 @@ proptest! { .collect(); let to_remove: Set<(_, _)> = indexes_to_remove.iter().map(|x| x.get(&all_versions)).cloned().collect(); for (name, ver) in cases { - match resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver) { + match timeout_resolve(dependency_provider.clone(), name, ver) { Ok(used) => { // If resolution was successful, then unpublishing a version of a crate // that was not selected should not change that. - let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); - for &(n, v) in &all_versions { - if used.get(&n) == Some(&v) // it was used - || to_remove.get(&(n, v)).is_none() // or it is not one to be removed - { - let deps = match dependency_provider.get_dependencies(&n, &v).unwrap() { - Dependencies::Unknown => panic!(), - Dependencies::Known(deps) => deps, - }; - smaller_dependency_provider.add_dependencies(n, v, deps) - } - } + let smaller_dependency_provider = retain_versions(&dependency_provider, |n, v| { + used.get(&n) == Some(&v) // it was used + || to_remove.get(&(*n, *v)).is_none() // or it is not one to be removed + }); prop_assert!( - resolve(&TimeoutDependencyProvider::new(smaller_dependency_provider.clone(), 50_000), name, ver).is_ok(), + timeout_resolve(smaller_dependency_provider.clone(), name, ver).is_ok(), "unpublishing {:?} stopped `{} = \"={}\"` from working", to_remove, name, @@ -451,19 +554,11 @@ proptest! { Err(_) => { // If resolution was unsuccessful, then it should stay unsuccessful // even if any version of a crate is unpublished. - let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); - for &(n, v) in &all_versions { - if to_remove.get(&(n, v)).is_none() // it is not one to be removed - { - let deps = match dependency_provider.get_dependencies(&n, &v).unwrap() { - Dependencies::Unknown => panic!(), - Dependencies::Known(deps) => deps, - }; - smaller_dependency_provider.add_dependencies(n, v, deps) - } - } + let smaller_dependency_provider = retain_versions(&dependency_provider, |n, v| { + to_remove.get(&(*n, *v)).is_some() // it is one to be removed + }); prop_assert!( - resolve(&TimeoutDependencyProvider::new(smaller_dependency_provider.clone(), 50_000), name, ver).is_err(), + timeout_resolve(smaller_dependency_provider.clone(), name, ver).is_err(), "full index did not work for `{} = \"={}\"` but unpublishing {:?} fixed it!", name, ver, @@ -481,19 +576,17 @@ fn large_case() { for case in std::fs::read_dir("test-examples").unwrap() { let case = case.unwrap().path(); let name = case.file_name().unwrap().to_string_lossy(); - eprintln!("{}", name); + eprint!("{} ", name); let data = std::fs::read_to_string(&case).unwrap(); + let start_time = std::time::Instant::now(); if name.ends_with("u16_NumberVersion.ron") { let dependency_provider: OfflineDependencyProvider = ron::de::from_str(&data).unwrap(); let mut sat = SatResolve::new(&dependency_provider); for p in dependency_provider.packages() { - for n in dependency_provider.versions(p).unwrap() { - if let Ok(s) = resolve(&dependency_provider, p.clone(), n.clone()) { - assert!(sat.sat_is_valid_solution(&s)); - } else { - assert!(!sat.sat_resolve(p, &n)); - } + for v in dependency_provider.versions(p).unwrap() { + let res = resolve(&dependency_provider, p.clone(), v); + sat.check_resolve(&res, p, v); } } } else if name.ends_with("str_SemanticVersion.ron") { @@ -501,14 +594,12 @@ fn large_case() { ron::de::from_str(&data).unwrap(); let mut sat = SatResolve::new(&dependency_provider); for p in dependency_provider.packages() { - for n in dependency_provider.versions(p).unwrap() { - if let Ok(s) = resolve(&dependency_provider, p, n.clone()) { - assert!(sat.sat_is_valid_solution(&s)); - } else { - assert!(!sat.sat_resolve(p, &n)); - } + for v in dependency_provider.versions(p).unwrap() { + let res = resolve(&dependency_provider, p.clone(), v); + sat.check_resolve(&res, p, v); } } } + eprintln!(" in {}s", start_time.elapsed().as_secs()) } } diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index 97ecab3e..2bfb21e9 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -1,23 +1,12 @@ // SPDX-License-Identifier: MPL-2.0 +use pubgrub::error::PubGrubError; use pubgrub::package::Package; use pubgrub::solver::{Dependencies, DependencyProvider, OfflineDependencyProvider}; use pubgrub::type_aliases::{Map, SelectedDependencies}; use pubgrub::version_set::VersionSet; use varisat::ExtendFormula; -const fn num_bits() -> usize { - std::mem::size_of::() * 8 -} - -fn log_bits(x: usize) -> usize { - if x == 0 { - return 0; - } - assert!(x > 0); - (num_bits::() as u32 - x.leading_zeros()) as usize -} - fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Var]) { if vars.len() <= 1 { return; @@ -32,7 +21,8 @@ fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Va } // use the "Binary Encoding" from // https://www.it.uu.se/research/group/astra/ModRef10/papers/Alan%20M.%20Frisch%20and%20Paul%20A.%20Giannoros.%20SAT%20Encodings%20of%20the%20At-Most-k%20Constraint%20-%20ModRef%202010.pdf - let bits: Vec = solver.new_var_iter(log_bits(vars.len())).collect(); + let len_bits = vars.len().ilog2() as usize + 1; + let bits: Vec = solver.new_var_iter(len_bits).collect(); for (i, p) in vars.iter().enumerate() { for (j, &bit) in bits.iter().enumerate() { solver.add_clause(&[p.negative(), bit.lit(((1 << j) & i) > 0)]); @@ -110,7 +100,7 @@ impl SatResolve { } } - pub fn sat_resolve(&mut self, name: &P, ver: &VS::V) -> bool { + pub fn resolve(&mut self, name: &P, ver: &VS::V) -> bool { if let Some(vers) = self.all_versions_by_p.get(name) { if let Some((_, var)) = vers.iter().find(|(v, _)| v == ver) { self.solver.assume(&[var.positive()]); @@ -126,16 +116,13 @@ impl SatResolve { } } - pub fn sat_is_valid_solution(&mut self, pids: &SelectedDependencies) -> bool { + pub fn is_valid_solution(&mut self, pids: &SelectedDependencies) -> bool { let mut assumption = vec![]; for (p, vs) in &self.all_versions_by_p { + let pid_for_p = pids.get(p); for (v, var) in vs { - assumption.push(if pids.get(p) == Some(v) { - var.positive() - } else { - var.negative() - }) + assumption.push(var.lit(pid_for_p == Some(v))) } } @@ -145,4 +132,20 @@ impl SatResolve { .solve() .expect("docs say it can't error in default config") } + + pub fn check_resolve( + &mut self, + res: &Result, PubGrubError>, + p: &P, + v: &VS::V, + ) { + match res { + Ok(s) => { + assert!(self.is_valid_solution(s)); + } + Err(_) => { + assert!(!self.resolve(p, v)); + } + } + } } From eb73235f847f738f38bd9ab51296aac7c264e97c Mon Sep 17 00:00:00 2001 From: 0xAtticus Date: Fri, 3 Nov 2023 11:50:42 +0100 Subject: [PATCH 16/20] feat: move to edition 2021 --- Cargo.toml | 2 +- src/internal/small_vec.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a796249e..1898eb96 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,7 @@ authors = [ "Alex Tokarev ", "Jacob Finkelman ", ] -edition = "2018" +edition = "2021" description = "PubGrub version solving algorithm" readme = "README.md" repository = "https://github.com/pubgrub-rs/pubgrub" diff --git a/src/internal/small_vec.rs b/src/internal/small_vec.rs index 4c2a97df..fa720435 100644 --- a/src/internal/small_vec.rs +++ b/src/internal/small_vec.rs @@ -166,9 +166,9 @@ impl IntoIterator for SmallVec { fn into_iter(self) -> Self::IntoIter { match self { SmallVec::Empty => SmallVecIntoIter::Empty, - SmallVec::One(a) => SmallVecIntoIter::One(IntoIterator::into_iter(a)), - SmallVec::Two(a) => SmallVecIntoIter::Two(IntoIterator::into_iter(a)), - SmallVec::Flexible(v) => SmallVecIntoIter::Flexible(IntoIterator::into_iter(v)), + SmallVec::One(a) => SmallVecIntoIter::One(a.into_iter()), + SmallVec::Two(a) => SmallVecIntoIter::Two(a.into_iter()), + SmallVec::Flexible(v) => SmallVecIntoIter::Flexible(v.into_iter()), } } } From 6e84368dd043ccbecfadadd5682fb828bcf967a6 Mon Sep 17 00:00:00 2001 From: Sandy Vanderbleek Date: Sun, 5 Nov 2023 20:19:05 -0500 Subject: [PATCH 17/20] ci: add merge queue to workflow and skip commit check outside (#146) --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bf7df723..aba6531b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,6 +1,7 @@ name: CI on: pull_request: + merge_group: push: branches: [ release, dev ] schedule: [ cron: "0 6 * * 4" ] @@ -103,6 +104,7 @@ jobs: check_commit_conventions: name: Commit messages follow project guidelines runs-on: ubuntu-latest + if: github.event_name != 'pull_request' || github.event.action == 'enqueued' steps: - uses: actions/checkout@v2 with: From fe309ffb63b2f3ce9b35eb7746b2350cd704515e Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Sun, 5 Nov 2023 21:05:42 -0500 Subject: [PATCH 18/20] perf!: use a priority queue (#104) BREAKING CHANGE: Changes the API of DependencyProvider --- Cargo.toml | 2 + examples/caching_dependency_provider.rs | 21 ++- src/error.rs | 2 +- src/internal/core.rs | 6 +- src/internal/partial_solution.rs | 135 ++++++++++----- src/lib.rs | 26 +-- src/solver.rs | 212 +++++++++++------------- src/version.rs | 2 +- tests/proptest.rs | 56 ++++--- 9 files changed, 264 insertions(+), 198 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1898eb96..a4c94e69 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,8 @@ include = ["Cargo.toml", "LICENSE", "README.md", "src/**", "tests/**", "examples # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] +indexmap = "2.0.2" +priority-queue = "1.1.1" thiserror = "1.0" rustc-hash = "1.1.0" serde = { version = "1.0", features = ["derive"], optional = true } diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs index bec7d2ab..6ef8e893 100644 --- a/examples/caching_dependency_provider.rs +++ b/examples/caching_dependency_provider.rs @@ -32,13 +32,6 @@ impl> impl> DependencyProvider for CachingDependencyProvider { - fn choose_package_version, U: std::borrow::Borrow>( - &self, - packages: impl Iterator, - ) -> Result<(T, Option), Box> { - self.remote_dependencies.choose_package_version(packages) - } - // Caches dependencies if they were already queried fn get_dependencies( &self, @@ -66,6 +59,20 @@ impl> DependencyProvid error @ Err(_) => error, } } + + fn choose_version( + &self, + package: &P, + range: &VS, + ) -> Result, Box> { + 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) + } } fn main() { diff --git a/src/error.rs b/src/error.rs index d27090e2..15a410e3 100644 --- a/src/error.rs +++ b/src/error.rs @@ -46,7 +46,7 @@ pub enum PubGrubError { /// Error arising when the implementer of /// [DependencyProvider](crate::solver::DependencyProvider) /// returned an error in the method - /// [choose_package_version](crate::solver::DependencyProvider::choose_package_version). + /// [choose_version](crate::solver::DependencyProvider::choose_version). #[error("Decision making failed")] ErrorChoosingPackageVersion(Box), diff --git a/src/internal/core.rs b/src/internal/core.rs index 973a9b8b..06e3ae21 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -20,7 +20,7 @@ use crate::version_set::VersionSet; /// Current state of the PubGrub algorithm. #[derive(Clone)] -pub struct State { +pub struct State { root_package: P, root_version: VS::V, @@ -32,7 +32,7 @@ pub struct State { /// Partial solution. /// TODO: remove pub. - pub partial_solution: PartialSolution, + pub partial_solution: PartialSolution, /// The store is the reference storage for all incompatibilities. pub incompatibility_store: Arena>, @@ -43,7 +43,7 @@ pub struct State { unit_propagation_buffer: SmallVec

, } -impl State { +impl State { /// Initialization of PubGrub state. pub fn init(root_package: P, root_version: VS::V) -> Self { let mut incompatibility_store = Arena::new(); diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index c2d88bed..057dea13 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -1,20 +1,26 @@ // SPDX-License-Identifier: MPL-2.0 //! A Memory acts like a structured partial solution -//! where terms are regrouped by package in a [Map]. +//! where terms are regrouped by package in a [Map](crate::type_aliases::Map). use std::fmt::Display; +use std::hash::BuildHasherDefault; + +use priority_queue::PriorityQueue; +use rustc_hash::FxHasher; use crate::internal::arena::Arena; use crate::internal::incompatibility::{IncompId, Incompatibility, Relation}; use crate::internal::small_map::SmallMap; use crate::package::Package; use crate::term::Term; -use crate::type_aliases::{Map, SelectedDependencies}; +use crate::type_aliases::SelectedDependencies; use crate::version_set::VersionSet; use super::small_vec::SmallVec; +type FnvIndexMap = indexmap::IndexMap>; + #[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] pub struct DecisionLevel(pub u32); @@ -27,13 +33,29 @@ impl DecisionLevel { /// The partial solution contains all package assignments, /// organized by package and historically ordered. #[derive(Clone, Debug)] -pub struct PartialSolution { +pub struct PartialSolution { next_global_index: u32, current_decision_level: DecisionLevel, - package_assignments: Map>, + /// `package_assignments` is primarily a HashMap from a package to its + /// `PackageAssignments`. But it can also keep the items in an order. + /// We maintain three sections in this order: + /// 1. `[..current_decision_level]` Are packages that have had a decision made sorted by the `decision_level`. + /// This makes it very efficient to extract the solution, And to backtrack to a particular decision level. + /// 2. `[current_decision_level..changed_this_decision_level]` Are packages that have **not** had there assignments + /// changed since the last time `prioritize` has bean called. Within this range there is no sorting. + /// 3. `[changed_this_decision_level..]` Containes all packages that **have** had there assignments changed since + /// the last time `prioritize` has bean called. The inverse is not necessarily true, some packages in the range + /// did not have a change. Within this range there is no sorting. + package_assignments: FnvIndexMap>, + /// `prioritized_potential_packages` is primarily a HashMap from a package with no desition and a positive assignment + /// to its `Priority`. But, it also maintains a max heap of packages by `Priority` order. + prioritized_potential_packages: PriorityQueue>, + changed_this_decision_level: usize, } -impl Display for PartialSolution { +impl Display + for PartialSolution +{ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let mut assignments: Vec<_> = self .package_assignments @@ -120,13 +142,15 @@ pub enum SatisfierSearch { }, } -impl PartialSolution { +impl PartialSolution { /// Initialize an empty PartialSolution. pub fn empty() -> Self { Self { next_global_index: 0, current_decision_level: DecisionLevel(0), - package_assignments: Map::default(), + package_assignments: FnvIndexMap::default(), + prioritized_potential_packages: PriorityQueue::default(), + changed_this_decision_level: 0, } } @@ -151,11 +175,16 @@ impl PartialSolution { } }, } + assert_eq!( + self.changed_this_decision_level, + self.package_assignments.len() + ); } + let new_idx = self.current_decision_level.0 as usize; self.current_decision_level = self.current_decision_level.increment(); - let pa = self + let (old_idx, _, pa) = self .package_assignments - .get_mut(&package) + .get_full_mut(&package) .expect("Derivations must already exist"); pa.highest_decision_level = self.current_decision_level; pa.assignments_intersection = AssignmentsIntersection::Decision(( @@ -163,6 +192,10 @@ impl PartialSolution { version.clone(), Term::exact(version), )); + // Maintain that the beginning of the `package_assignments` Have all decisions in sorted order. + if new_idx != old_idx { + self.package_assignments.swap_indices(new_idx, old_idx); + } self.next_global_index += 1; } @@ -173,7 +206,7 @@ impl PartialSolution { cause: IncompId, store: &Arena>, ) { - use std::collections::hash_map::Entry; + use indexmap::map::Entry; let term = store[cause].get(&package).unwrap().negate(); let dated_derivation = DatedDerivation { global_index: self.next_global_index, @@ -181,8 +214,10 @@ impl PartialSolution { cause, }; self.next_global_index += 1; + let pa_last_index = self.package_assignments.len().saturating_sub(1); match self.package_assignments.entry(package) { Entry::Occupied(mut occupied) => { + let idx = occupied.index(); let pa = occupied.get_mut(); pa.highest_decision_level = self.current_decision_level; match &mut pa.assignments_intersection { @@ -192,11 +227,21 @@ impl PartialSolution { } AssignmentsIntersection::Derivations(t) => { *t = t.intersection(&term); + if t.is_positive() { + // we can use `swap_indices` to make `changed_this_decision_level` only go down by 1 + // but the copying is slower then the larger search + self.changed_this_decision_level = + std::cmp::min(self.changed_this_decision_level, idx); + } } } pa.dated_derivations.push(dated_derivation); } Entry::Vacant(v) => { + if term.is_positive() { + self.changed_this_decision_level = + std::cmp::min(self.changed_this_decision_level, pa_last_index); + } v.insert(PackageAssignments { smallest_decision_level: self.current_decision_level, highest_decision_level: self.current_decision_level, @@ -207,43 +252,48 @@ impl PartialSolution { } } - /// Extract potential packages for the next iteration of unit propagation. - /// Return `None` if there is no suitable package anymore, which stops the algorithm. - /// A package is a potential pick if there isn't an already - /// selected version (no "decision") - /// and if it contains at least one positive derivation term - /// in the partial solution. - pub fn potential_packages(&self) -> Option> { - let mut iter = self - .package_assignments + pub fn pick_highest_priority_pkg( + &mut self, + prioritizer: impl Fn(&P, &VS) -> Priority, + ) -> Option

{ + let check_all = self.changed_this_decision_level + == self.current_decision_level.0.saturating_sub(1) as usize; + let current_decision_level = self.current_decision_level; + let prioritized_potential_packages = &mut self.prioritized_potential_packages; + self.package_assignments + .get_range(self.changed_this_decision_level..) + .unwrap() .iter() + .filter(|(_, pa)| { + // We only actually need to update the package if its Been changed + // since the last time we called prioritize. + // Which means it's highest decision level is the current decision level, + // or if we backtracked in the mean time. + check_all || pa.highest_decision_level == current_decision_level + }) .filter_map(|(p, pa)| pa.assignments_intersection.potential_package_filter(p)) - .peekable(); - if iter.peek().is_some() { - Some(iter) - } else { - None - } + .for_each(|(p, r)| { + let priority = prioritizer(p, r); + prioritized_potential_packages.push(p.clone(), priority); + }); + self.changed_this_decision_level = self.package_assignments.len(); + prioritized_potential_packages.pop().map(|(p, _)| p) } /// If a partial solution has, for every positive derivation, /// a corresponding decision that satisfies that assignment, /// it's a total solution and version solving has succeeded. - pub fn extract_solution(&self) -> Option> { - let mut solution = Map::default(); - for (p, pa) in &self.package_assignments { - match &pa.assignments_intersection { - AssignmentsIntersection::Decision((_, v, _)) => { - solution.insert(p.clone(), v.clone()); - } - AssignmentsIntersection::Derivations(term) => { - if term.is_positive() { - return None; - } + pub fn extract_solution(&self) -> SelectedDependencies { + self.package_assignments + .iter() + .take(self.current_decision_level.0 as usize) + .map(|(p, pa)| match &pa.assignments_intersection { + AssignmentsIntersection::Decision((_, v, _)) => (p.clone(), v.clone()), + AssignmentsIntersection::Derivations(_) => { + panic!("Derivations in the Decision part") } - } - } - Some(solution) + }) + .collect() } /// Backtrack the partial solution to a given decision level. @@ -290,6 +340,9 @@ impl PartialSolution { true } }); + // Throw away all stored priority levels, And mark that they all need to be recomputed. + self.prioritized_potential_packages.clear(); + self.changed_this_decision_level = self.current_decision_level.0.saturating_sub(1) as usize; } /// We can add the version to the partial solution as a decision @@ -386,7 +439,7 @@ impl PartialSolution { /// to return a coherent previous_satisfier_level. fn find_satisfier( incompat: &Incompatibility, - package_assignments: &Map>, + package_assignments: &FnvIndexMap>, store: &Arena>, ) -> SmallMap { let mut satisfied = SmallMap::Empty; @@ -407,7 +460,7 @@ impl PartialSolution { incompat: &Incompatibility, satisfier_package: &P, mut satisfied_map: SmallMap, - package_assignments: &Map>, + package_assignments: &FnvIndexMap>, store: &Arena>, ) -> DecisionLevel { // First, let's retrieve the previous derivations and the initial accum_term. diff --git a/src/lib.rs b/src/lib.rs index 3934b8f3..5f61fb51 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -75,7 +75,7 @@ //! trait for our own type. //! Let's say that we will use [String] for packages, //! and [SemanticVersion](version::SemanticVersion) for versions. -//! This may be done quite easily by implementing the two following functions. +//! This may be done quite easily by implementing the three following functions. //! ``` //! # use pubgrub::solver::{DependencyProvider, Dependencies}; //! # use pubgrub::version::SemanticVersion; @@ -89,7 +89,12 @@ //! type SemVS = Range; //! //! impl DependencyProvider for MyDependencyProvider { -//! fn choose_package_version, U: Borrow>(&self,packages: impl Iterator) -> Result<(T, Option), Box> { +//! fn choose_version(&self, package: &String, range: &SemVS) -> Result, Box> { +//! unimplemented!() +//! } +//! +//! type Priority = usize; +//! fn prioritize(&self, package: &String, range: &SemVS) -> Self::Priority { //! unimplemented!() //! } //! @@ -104,18 +109,17 @@ //! ``` //! //! The first method -//! [choose_package_version](crate::solver::DependencyProvider::choose_package_version) -//! chooses a package and available version compatible with the provided options. -//! A helper function -//! [choose_package_with_fewest_versions](crate::solver::choose_package_with_fewest_versions) -//! is provided for convenience -//! in cases when lists of available versions for packages are easily obtained. -//! The strategy of that helper function consists in choosing the package -//! with the fewest number of compatible versions to speed up resolution. +//! [choose_version](crate::solver::DependencyProvider::choose_version) +//! chooses a version compatible with the provided range for a package. +//! The second method +//! [prioritize](crate::solver::DependencyProvider::prioritize) +//! in which order different packages should be chosen. +//! Usually prioritizing packages +//! with the fewest number of compatible versions speeds up resolution. //! But in general you are free to employ whatever strategy suits you best //! to pick a package and a version. //! -//! The second method [get_dependencies](crate::solver::DependencyProvider::get_dependencies) +//! The third method [get_dependencies](crate::solver::DependencyProvider::get_dependencies) //! aims at retrieving the dependencies of a given package at a given version. //! Returns [None] if dependencies are unknown. //! diff --git a/src/solver.rs b/src/solver.rs index 7354ff7d..a1e5e06c 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -68,7 +68,7 @@ //! to satisfy the dependencies of that package and version pair. //! If there is no solution, the reason will be provided as clear as possible. -use std::borrow::Borrow; +use std::cmp::Reverse; use std::collections::{BTreeMap, BTreeSet as Set}; use std::error::Error; @@ -103,30 +103,27 @@ pub fn resolve( state.partial_solution ); - let Some(potential_packages) = state.partial_solution.potential_packages() else { - return state.partial_solution.extract_solution().ok_or_else(|| { - PubGrubError::Failure( - "How did we end up with no package to choose but no solution?".into(), - ) - }); + let Some(highest_priority_pkg) = state + .partial_solution + .pick_highest_priority_pkg(|p, r| dependency_provider.prioritize(p, r)) + else { + return Ok(state.partial_solution.extract_solution()); }; + next = highest_priority_pkg; - let decision = dependency_provider - .choose_package_version(potential_packages) - .map_err(PubGrubError::ErrorChoosingPackageVersion)?; - info!("DP chose: {} @ {:?}", decision.0, decision.1); - - next = decision.0.clone(); - - // Pick the next compatible version. let term_intersection = state .partial_solution .term_intersection_for_package(&next) .ok_or_else(|| { PubGrubError::Failure("a package was chosen but we don't have a term.".into()) })?; + let decision = dependency_provider + .choose_version(&next, term_intersection.unwrap_positive()) + .map_err(PubGrubError::ErrorChoosingPackageVersion)?; + info!("DP chose: {} @ {:?}", next, decision); - let v = match decision.1 { + // Pick the next compatible version. + let v = match decision { None => { let inc = Incompatibility::no_versions(next.clone(), term_intersection.clone()); state.add_incompatibility(inc); @@ -146,51 +143,53 @@ pub fn resolve( .or_default() .insert(v.clone()); - if !is_new_dependency { + if is_new_dependency { + // Retrieve that package dependencies. + let p = &next; + let dependencies = dependency_provider.get_dependencies(p, &v).map_err(|err| { + PubGrubError::ErrorRetrievingDependencies { + package: p.clone(), + version: v.clone(), + source: err, + } + })?; + + let known_dependencies = match dependencies { + Dependencies::Unknown => { + state.add_incompatibility(Incompatibility::unavailable_dependencies( + p.clone(), + v.clone(), + )); + continue; + } + Dependencies::Known(x) if x.contains_key(p) => { + return Err(PubGrubError::SelfDependency { + package: p.clone(), + version: v, + }); + } + 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, + ); + + state.partial_solution.add_version( + p.clone(), + v, + dep_incompats, + &state.incompatibility_store, + ); + } else { // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied // terms and can add the decision directly. info!("add_decision (not first time): {} @ {}", &next, v); state.partial_solution.add_decision(next.clone(), v); - continue; } - - // Retrieve that package dependencies. - let p = &next; - let dependencies = dependency_provider.get_dependencies(p, &v).map_err(|err| { - PubGrubError::ErrorRetrievingDependencies { - package: p.clone(), - version: v.clone(), - source: err, - } - })?; - - let known_dependencies = match dependencies { - Dependencies::Unknown => { - state.add_incompatibility(Incompatibility::unavailable_dependencies( - p.clone(), - v.clone(), - )); - continue; - } - Dependencies::Known(x) if x.contains_key(p) => { - return Err(PubGrubError::SelfDependency { - package: p.clone(), - version: v, - }); - } - 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); - - state.partial_solution.add_version( - p.clone(), - v, - dep_incompats, - &state.incompatibility_store, - ); } } @@ -210,11 +209,15 @@ pub trait DependencyProvider { /// [Decision making](https://github.com/dart-lang/pub/blob/master/doc/solver.md#decision-making) /// is the process of choosing the next package /// and version that will be appended to the partial solution. - /// Every time such a decision must be made, - /// potential valid packages and sets of versions are preselected by the resolver, - /// and the dependency provider must choose. /// - /// The strategy employed to choose such package and version + /// Every time such a decision must be made, the resolver looks at all the potential valid + /// packages that have changed, and a asks the dependency provider how important each one is. + /// For each one it calls `prioritize` with the name of the package and the current set of + /// acceptable versions. + /// The resolver will then pick the package with the highes priority from all the potential valid + /// packages. + /// + /// The strategy employed to prioritize packages /// cannot change the existence of a solution or not, /// but can drastically change the performances of the solver, /// or the properties of the solution. @@ -227,16 +230,24 @@ pub trait DependencyProvider { /// > since these packages will run out of versions to try more quickly. /// > But there's likely room for improvement in these heuristics. /// - /// A helper function [choose_package_with_fewest_versions] is provided to ease - /// implementations of this method if you can produce an iterator - /// of the available versions in preference order for any package. + /// Note: the resolver may call this even when the range has not change, + /// if it is more efficient for the resolveres internal data structures. + fn prioritize(&self, package: &P, range: &VS) -> Self::Priority; + /// The type returned from `prioritize`. The resolver does not care what type this is + /// as long as it can pick a largest one and clone it. /// - /// Note: the type `T` ensures that this returns an item from the `packages` argument. - #[allow(clippy::type_complexity)] - fn choose_package_version, U: Borrow>( + /// [std::cmp::Reverse] can be useful if you want to pick the package with + /// the fewest versions that match the outstanding constraint. + type Priority: Ord + Clone; + + /// Once the resolver has found the highest `Priority` package from all potential valid + /// packages, it needs to know what vertion of that package to use. The most common pattern + /// is to select the largest vertion that the range contains. + fn choose_version( &self, - potential_packages: impl Iterator, - ) -> Result<(T, Option), Box>; + package: &P, + range: &VS, + ) -> Result, Box>; /// Retrieves the package dependencies. /// Return [Dependencies::Unknown] if its dependencies are unknown. @@ -256,35 +267,6 @@ pub trait DependencyProvider { } } -/// This is a helper function to make it easy to implement -/// [DependencyProvider::choose_package_version]. -/// It takes a function `list_available_versions` that takes a package and returns an iterator -/// of the available versions in preference order. -/// The helper finds the package from the `packages` argument with the fewest versions from -/// `list_available_versions` contained in the constraints. Then takes that package and finds the -/// first version contained in the constraints. -pub fn choose_package_with_fewest_versions( - list_available_versions: F, - potential_packages: impl Iterator, -) -> (T, Option) -where - T: Borrow

, - U: Borrow, - I: Iterator, - F: Fn(&P) -> I, -{ - let count_valid = |(p, set): &(T, U)| { - list_available_versions(p.borrow()) - .filter(|v| set.borrow().contains(v)) - .count() - }; - let (pkg, set) = potential_packages - .min_by_key(count_valid) - .expect("potential_packages gave us an empty iterator"); - let version = list_available_versions(pkg.borrow()).find(|v| set.borrow().contains(v)); - (pkg, version) -} - /// A basic implementation of [DependencyProvider]. #[derive(Debug, Clone, Default)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] @@ -354,25 +336,29 @@ impl OfflineDependencyProvider { /// An implementation of [DependencyProvider] that /// contains all dependency information available in memory. -/// Packages are picked with the fewest versions contained in the constraints first. +/// Currently packages are picked with the fewest versions contained in the constraints first. +/// But, that may change in new versions if better heuristics are found. /// Versions are picked with the newest versions first. impl DependencyProvider for OfflineDependencyProvider { - #[allow(clippy::type_complexity)] - fn choose_package_version, U: Borrow>( + fn choose_version( &self, - potential_packages: impl Iterator, - ) -> Result<(T, Option), Box> { - Ok(choose_package_with_fewest_versions( - |p| { - self.dependencies - .get(p) - .into_iter() - .flat_map(|k| k.keys()) - .rev() - .cloned() - }, - potential_packages, - )) + package: &P, + range: &VS, + ) -> Result, Box> { + Ok(self + .dependencies + .get(package) + .and_then(|versions| versions.keys().rev().find(|v| range.contains(v)).cloned())) + } + + type Priority = Reverse; + fn prioritize(&self, package: &P, range: &VS) -> Self::Priority { + Reverse( + self.dependencies + .get(package) + .map(|versions| versions.keys().filter(|v| range.contains(v)).count()) + .unwrap_or(0), + ) } fn get_dependencies( diff --git a/src/version.rs b/src/version.rs index bf0524b4..6f67c7de 100644 --- a/src/version.rs +++ b/src/version.rs @@ -121,7 +121,7 @@ impl SemanticVersion { } /// Error creating [SemanticVersion] from [String]. -#[derive(Error, Debug, PartialEq)] +#[derive(Error, Debug, PartialEq, Eq)] pub enum VersionParseError { /// [SemanticVersion] must contain major, minor, patch versions. #[error("version {full_version} must contain 3 numbers separated by dot")] diff --git a/tests/proptest.rs b/tests/proptest.rs index adea37b2..017efed0 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -6,10 +6,7 @@ use pubgrub::error::PubGrubError; use pubgrub::package::Package; use pubgrub::range::Range; use pubgrub::report::{DefaultStringReporter, DerivationTree, External, Reporter}; -use pubgrub::solver::{ - choose_package_with_fewest_versions, resolve, Dependencies, DependencyProvider, - OfflineDependencyProvider, -}; +use pubgrub::solver::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider}; use pubgrub::type_aliases::SelectedDependencies; use pubgrub::version::{NumberVersion, SemanticVersion}; use pubgrub::version_set::VersionSet; @@ -33,16 +30,6 @@ struct OldestVersionsDependencyProvider( impl DependencyProvider for OldestVersionsDependencyProvider { - fn choose_package_version, U: std::borrow::Borrow>( - &self, - potential_packages: impl Iterator, - ) -> Result<(T, Option), Box> { - Ok(choose_package_with_fewest_versions( - |p| self.0.versions(p).into_iter().flatten().cloned(), - potential_packages, - )) - } - fn get_dependencies( &self, p: &P, @@ -50,6 +37,26 @@ impl DependencyProvider ) -> Result, Box> { self.0.get_dependencies(p, v) } + + fn choose_version( + &self, + package: &P, + range: &VS, + ) -> Result, Box> { + Ok(self + .0 + .versions(package) + .into_iter() + .flatten() + .find(|&v| range.contains(v)) + .cloned()) + } + + type Priority = as DependencyProvider>::Priority; + + fn prioritize(&self, package: &P, range: &VS) -> Self::Priority { + self.0.prioritize(package, range) + } } /// The same as DP but it has a timeout. @@ -75,13 +82,6 @@ impl TimeoutDependencyProvider { impl> DependencyProvider for TimeoutDependencyProvider { - fn choose_package_version, U: std::borrow::Borrow>( - &self, - potential_packages: impl Iterator, - ) -> Result<(T, Option), Box> { - self.dp.choose_package_version(potential_packages) - } - fn get_dependencies( &self, p: &P, @@ -97,6 +97,20 @@ impl> DependencyProvid self.call_count.set(calls + 1); Ok(()) } + + fn choose_version( + &self, + package: &P, + range: &VS, + ) -> Result, Box> { + self.dp.choose_version(package, range) + } + + type Priority = DP::Priority; + + fn prioritize(&self, package: &P, range: &VS) -> Self::Priority { + self.dp.prioritize(package, range) + } } fn timeout_resolve>( From acfbe994f072b36ec3bbb82c858370165c128233 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 8 Nov 2023 10:57:15 -0500 Subject: [PATCH 19/20] refactor: as_ref is stable as of 1.65 and "," is ambiguous (#147) --- src/range.rs | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/range.rs b/src/range.rs index fe2aaef4..be7ff250 100644 --- a/src/range.rs +++ b/src/range.rs @@ -195,7 +195,7 @@ impl Range { .segments .last() .expect("if there is a first element, there must be a last element"); - (bound_as_ref(start), bound_as_ref(&end.1)) + (start.as_ref(), end.1.as_ref()) }) } @@ -264,15 +264,6 @@ impl Range { } } -/// Implementation of [`Bound::as_ref`] which is currently marked as unstable. -fn bound_as_ref(bound: &Bound) -> Bound<&V> { - match bound { - Included(v) => Included(v), - Excluded(v) => Excluded(v), - Unbounded => Unbounded, - } -} - fn valid_segment(start: &Bound, end: &Bound) -> bool { match (start, end) { (Included(s), Included(e)) => s <= e, @@ -307,7 +298,7 @@ impl Range { (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if i <= e => Excluded(e), (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if e < i => Included(i), - (s, Unbounded) | (Unbounded, s) => bound_as_ref(s), + (s, Unbounded) | (Unbounded, s) => s.as_ref(), _ => unreachable!(), } .cloned(); @@ -317,7 +308,7 @@ impl Range { (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if i >= e => Excluded(e), (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if e > i => Included(i), - (s, Unbounded) | (Unbounded, s) => bound_as_ref(s), + (s, Unbounded) | (Unbounded, s) => s.as_ref(), _ => unreachable!(), } .cloned(); @@ -373,7 +364,7 @@ impl Display for Range { } else { for (idx, segment) in self.segments.iter().enumerate() { if idx > 0 { - write!(f, ", ")?; + write!(f, " | ")?; } match segment { (Unbounded, Unbounded) => write!(f, "*")?, @@ -384,7 +375,7 @@ impl Display for Range { if v == b { write!(f, "{v}")? } else { - write!(f, ">={v},<={b}")? + write!(f, ">={v}, <={b}")? } } (Included(v), Excluded(b)) => write!(f, ">={v}, <{b}")?, From c0baa08ba25de37751042d171df8ad88945e28c5 Mon Sep 17 00:00:00 2001 From: Zanie Date: Thu, 16 Nov 2023 13:14:38 -0600 Subject: [PATCH 20/20] Add GitHub workflow to automatically tag each commit on `main` --- .github/workflows/permaref.yaml | 40 +++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 .github/workflows/permaref.yaml diff --git a/.github/workflows/permaref.yaml b/.github/workflows/permaref.yaml new file mode 100644 index 00000000..21e05507 --- /dev/null +++ b/.github/workflows/permaref.yaml @@ -0,0 +1,40 @@ +# Automatically creates a tag for each commit to `main` so when we rebase +# changes on top of the upstream, we retain permanent references to each +# previous commit so they are not orphaned and eventually deleted. +name: Create permanent reference + +on: + push: + branches: + - "main" + +jobs: + release: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v3 + + - name: Get the permanent ref number + id: get_version + run: | + # Enable pipefail so git command failures do not result in null versions downstream + set -x + + echo ::set-output name=LAST_PERMA_NUMBER::$(\ + git ls-remote --tags --refs --sort="v:refname" \ + https://github.com/zanieb/pubgrub.git | grep "tags/perma-" | tail -n1 | sed 's/.*\/perma-//' \ + ) + + - name: Configure Git + run: | + git config user.name "$GITHUB_ACTOR" + git config user.email "$GITHUB_ACTOR@users.noreply.github.com" + + - name: Create and push the new tag + run: | + TAG="perma-$((LAST_PERMA_NUMBER + 1))" + git tag -a "$TAG" -m "Automatically created on push to `main`" + git push origin "$TAG" + env: + LAST_PERMA_NUMBER: ${{ steps.get_version.outputs.LAST_PERMA_NUMBER }}