From 15ac3c7bb1f5d33fc9c8e86644e343bec61696ae Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Sun, 26 Jun 2022 00:06:15 +0200 Subject: [PATCH] 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 { .. })