From 4209e77c95d561a38973898b9bfa52b47f74ceeb Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 29 Oct 2023 13:25:55 -0700 Subject: [PATCH] Upgrade `pubgrub-rs` version (#220) Upgrades our PubGrub to 8951e37fe923a7edd5a78ed5f49f165b0fdc48de. --- crates/puffin-resolver/src/resolver.rs | 36 ++------- vendor/pubgrub/src/error.rs | 15 ---- vendor/pubgrub/src/internal/core.rs | 10 +-- .../pubgrub/src/internal/incompatibility.rs | 12 ++- .../pubgrub/src/internal/partial_solution.rs | 8 +- vendor/pubgrub/src/lib.rs | 4 +- vendor/pubgrub/src/solver.rs | 25 +------ vendor/pubgrub/tests/proptest.rs | 75 ++++++++----------- vendor/pubgrub/tests/tests.rs | 4 +- 9 files changed, 61 insertions(+), 128 deletions(-) diff --git a/crates/puffin-resolver/src/resolver.rs b/crates/puffin-resolver/src/resolver.rs index f8148a1a4..896ee5bf8 100644 --- a/crates/puffin-resolver/src/resolver.rs +++ b/crates/puffin-resolver/src/resolver.rs @@ -195,27 +195,14 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { )); continue; } - Dependencies::Known(constraints) => { - if constraints.contains_key(package) { - return Err(PubGrubError::SelfDependency { - package: package.clone(), - version: version.clone(), - } - .into()); + Dependencies::Known(constraints) if constraints.contains_key(package) => { + return Err(PubGrubError::SelfDependency { + package: package.clone(), + version: version.clone(), } - if let Some((dependent, _)) = constraints - .iter() - .find(|(_, r)| r == &&Range::::empty()) - { - return Err(PubGrubError::DependencyOnTheEmptySet { - package: package.clone(), - version: version.clone(), - dependent: dependent.clone(), - } - .into()); - } - constraints + .into()); } + Dependencies::Known(constraints) => constraints, }; // Add that package and version if the dependencies are not problematic. @@ -225,17 +212,6 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { &dependencies, ); - 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(), - ) - .into()); - } state.partial_solution.add_version( package.clone(), version, diff --git a/vendor/pubgrub/src/error.rs b/vendor/pubgrub/src/error.rs index b0633022f..d27090e2f 100644 --- a/vendor/pubgrub/src/error.rs +++ b/vendor/pubgrub/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/vendor/pubgrub/src/internal/core.rs b/vendor/pubgrub/src/internal/core.rs index 47d022dd1..b142e8b11 100644 --- a/vendor/pubgrub/src/internal/core.rs +++ b/vendor/pubgrub/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/vendor/pubgrub/src/internal/incompatibility.rs b/vendor/pubgrub/src/internal/incompatibility.rs index fc742b461..592aaf398 100644 --- a/vendor/pubgrub/src/internal/incompatibility.rs +++ b/vendor/pubgrub/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/vendor/pubgrub/src/internal/partial_solution.rs b/vendor/pubgrub/src/internal/partial_solution.rs index 1b683b9d8..c2d88bed1 100644 --- a/vendor/pubgrub/src/internal/partial_solution.rs +++ b/vendor/pubgrub/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, + ) } }, } diff --git a/vendor/pubgrub/src/lib.rs b/vendor/pubgrub/src/lib.rs index fb34c68a8..7c926dd22 100644 --- a/vendor/pubgrub/src/lib.rs +++ b/vendor/pubgrub/src/lib.rs @@ -1,7 +1,5 @@ // SPDX-License-Identifier: MPL-2.0 -#![allow(clippy::all, unreachable_pub)] - //! PubGrub version solving algorithm. //! //! Version solving consists in efficiently finding a set of packages and versions @@ -215,6 +213,8 @@ //! with a cache, you may want to know that some versions //! do not exist in your cache. +#![allow(clippy::all, unreachable_pub)] + pub mod error; pub mod package; pub mod range; diff --git a/vendor/pubgrub/src/solver.rs b/vendor/pubgrub/src/solver.rs index 124b68d7a..2db77cf76 100644 --- a/vendor/pubgrub/src/solver.rs +++ b/vendor/pubgrub/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/vendor/pubgrub/tests/proptest.rs b/vendor/pubgrub/tests/proptest.rs index cb66d3e63..382a6c362 100644 --- a/vendor/pubgrub/tests/proptest.rs +++ b/vendor/pubgrub/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()); + let (c, d) = order_index(c, d, s.len() + 1); - 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) - }, - )) - } + 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/vendor/pubgrub/tests/tests.rs b/vendor/pubgrub/tests/tests.rs index b1d05a4a4..81fd5324c 100644 --- a/vendor/pubgrub/tests/tests.rs +++ b/vendor/pubgrub/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 { .. }) )); }