From 61031d6f63cf199f6b6b8e3dc5b73184f76898ab Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 12 Jun 2023 17:53:08 +0000 Subject: [PATCH] test: prop test for error report and refactor --- src/internal/arena.rs | 2 +- src/internal/partial_solution.rs | 4 +- src/report.rs | 4 +- src/solver.rs | 4 +- tests/proptest.rs | 233 +++++++++++++++++++++---------- tests/sat_dependency_provider.rs | 43 +++--- 6 files changed, 192 insertions(+), 98 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..27926eae 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -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/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..a2d61766 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -175,14 +175,14 @@ 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) => { if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&VS::empty()) { return Err(PubGrubError::DependencyOnTheEmptySet { package: p.clone(), - version: v.clone(), + version: v, dependent: dependent.clone(), }); } diff --git a/tests/proptest.rs b/tests/proptest.rs index 8ec22e80..650f6c6d 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; @@ -90,6 +91,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; @@ -274,6 +287,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: @@ -294,7 +411,7 @@ proptest! { (dependency_provider, cases) in registry_strategy(string_names(), "bad".to_owned()) ) { for (name, ver) in cases { - let _ = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver); + _ = timeout_resolve(dependency_provider.clone(), name, ver); } } @@ -304,7 +421,7 @@ proptest! { (dependency_provider, cases) in registry_strategy(0u16..665, 666) ) { for (name, ver) in cases { - let _ = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver); + _ = timeout_resolve(dependency_provider.clone(), name, ver); } } @@ -314,11 +431,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, 666) + ) { + for (name, ver) in cases { + errors_the_same_with_only_report_dependencies(dependency_provider.clone(), name, ver); } } @@ -328,9 +451,9 @@ proptest! { (dependency_provider, cases) in registry_strategy(0u16..665, 666) ) { 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!( @@ -351,8 +474,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(_)) => (), @@ -367,7 +490,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 @@ -382,29 +505,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, @@ -429,24 +540,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, @@ -456,19 +559,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, @@ -486,19 +581,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") { @@ -506,14 +599,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.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); } } } + 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)); + } + } + } }