From c3a956755fc21f25f5063f2df70e2a7721260a23 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Fri, 22 Apr 2022 21:11:04 +0200 Subject: [PATCH 01/13] refactor: replace Range with a bounded implementation --- benches/large_case.rs | 15 +- 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 | 42 ++ src/lib.rs | 6 +- src/range.rs | 692 +++++++++++++++++------------ src/term.rs | 3 +- tests/examples.rs | 8 +- tests/proptest.rs | 8 +- tests/tests.rs | 8 +- 12 files changed, 500 insertions(+), 320 deletions(-) diff --git a/benches/large_case.rs b/benches/large_case.rs index 476228c8..33d836a1 100644 --- a/benches/large_case.rs +++ b/benches/large_case.rs @@ -5,15 +5,18 @@ 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>, V: VersionSet + Deserialize<'a>>( b: &mut Bencher, case: &'a str, -) { +) where + ::V: Deserialize<'a>, +{ let dependency_provider: OfflineDependencyProvider = ron::de::from_str(&case).unwrap(); b.iter(|| { @@ -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..65db25ac 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 { @@ -128,6 +136,40 @@ impl<'de, T: serde::Deserialize<'de>> serde::Deserialize<'de> for SmallVec { } } +impl IntoIterator for SmallVec { + type Item = T; + type IntoIter = SmallVecIntoIter; + + 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(), + } + } +} + // TESTS ####################################################################### #[cfg(test)] 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..13c96b09 100644 --- a/src/range.rs +++ b/src/range.rs @@ -7,354 +7,458 @@ //! 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))] +use crate::{internal::small_vec::SmallVec, version_set::VersionSet}; +use std::ops::RangeBounds; +use std::{ + cmp::Ordering, + 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 { - let v = v.into(); - if v == V::lowest() { - Self::none() - } else { - Self { - segments: SmallVec::one((V::lowest(), Some(v))), - } + Self { + segments: SmallVec::one((Unbounded, Excluded(v.into()))), } } - /// 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))), - } - } else { - Self::none() + /// 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()))), } } - /// 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 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()))), } } } -// Set operations. -impl Range { - // Negate ################################################################## +impl Range { + /// Set containing exactly one version + pub fn singleton(v: impl Into) -> Self { + let v = v.into(); + Self { + segments: SmallVec::one((Included(v.clone()), Included(v))), + } + } + + /// Set containing all versions expect one + pub fn not_equal(v: impl Into) -> Self { + let v = v.into(); + Self { + segments: SmallVec::Two([(Unbounded, Excluded(v.clone())), (Excluded(v), Unbounded)]), + } + } - /// Compute the complement set of versions. - pub fn negate(&self) -> Self { + /// Returns the complement of this Range. + pub fn complement(&self) -> Self { match self.segments.first() { - None => Self::any(), // Complement of ∅ is * + // Complement of ∅ is ∞ + None => Self::full(), + + // Complement of ∞ is ∅ + Some((Unbounded, Unbounded)) => Self::empty(), // 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()) - } - } + Some((Included(v), Unbounded)) => Self::strictly_lower_than(v.clone()), + Some((Excluded(v), Unbounded)) => Self::lower_than(v.clone()), - // 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) - } + 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), } } /// 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)); + 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 { + if let Some(bounding_range) = self.bounding_range() { + if !bounding_range.contains(v) { + return false; + } + } + + 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; + } + } + false + } - // Union and intersection ################################################## + /// Construct a simple range from anything that impls [RangeBounds] like `v1..v2`. + pub fn from_range_bounds(bounds: R) -> Self + where + R: RangeBounds, + IV: Clone + Into, + { + let start = match bounds.start_bound() { + Included(v) => Included(v.clone().into()), + Excluded(v) => Excluded(v.clone().into()), + Unbounded => Unbounded, + }; + let end = match bounds.end_bound() { + Included(v) => Included(v.clone().into()), + Excluded(v) => Excluded(v.clone().into()), + Unbounded => Unbounded, + }; + match (start, end) { + (Included(a), Included(b)) if b < a => Self::empty(), + (Excluded(a), Excluded(b)) if b < a => Self::empty(), + (Included(a), Excluded(b)) if b <= a => Self::empty(), + (Excluded(a), Included(b)) if b <= a => Self::empty(), + (a, b) => Self { + segments: SmallVec::one((a, b)), + }, + } + } +} - /// Compute the union of two sets of versions. - pub fn union(&self, other: &Self) -> Self { - self.negate().intersection(&other.negate()).negate() +/// 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, } +} - /// Compute the intersection of two sets of versions. +impl Range { + /// Computes the intersection of two sets of versions. pub fn intersection(&self, other: &Self) -> Self { - let mut segments = SmallVec::empty(); + let mut segments: SmallVec> = 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. + while let (Some((left_lower, left_upper)), Some((right_lower, right_upper))) = (left, right) + { + // Check if the left range completely smaller than the right range. + if let ( + Included(left_upper_version) | Excluded(left_upper_version), + Included(right_lower_version) | Excluded(right_lower_version), + ) = (left_upper, right_lower) + { + match left_upper_version.cmp(right_lower_version) { + Ordering::Less => { + // Left range is disjoint from the right range. 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()))); + continue; + } + Ordering::Equal => { + if !matches!((left_upper, right_lower), (Included(_), Included(_))) { + // Left and right are overlapping exactly, but one of the bounds is exclusive, therefor the ranges are disjoint left = left_iter.next(); - } else { - segments.push((start, Some(r2.to_owned()))); - right = right_iter.next(); + continue; } } + Ordering::Greater => { + // Left upper bound is greater than right lower bound, so the lower bound is the right lower bound + } } - - // Right contains an infinite interval: - (Some((l1, Some(l2))), Some((r1, None))) => match l2.cmp(r1) { + } + // Check if the right range completely smaller than the left range. + if let ( + Included(left_lower_version) | Excluded(left_lower_version), + Included(right_upper_version) | Excluded(right_upper_version), + ) = (left_lower, right_upper) + { + match right_upper_version.cmp(left_lower_version) { Ordering::Less => { - left = left_iter.next(); + // Right range is disjoint from the left range. + right = right_iter.next(); + continue; } Ordering::Equal => { - for l in left_iter.cloned() { - segments.push(l) + if !matches!((right_upper, left_lower), (Included(_), Included(_))) { + // Left and right are overlapping exactly, but one of the bounds is exclusive, therefor the ranges are disjoint + right = right_iter.next(); + continue; } - 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; + // Right upper bound is greater than left lower bound, so the lower bound is the left lower bound } + } + } + + // At this point we know there is an overlap between the versions, find the lowest bound + let lower = match (left_lower, right_lower) { + (Unbounded, Included(_) | Excluded(_)) => right_lower.clone(), + (Included(_) | Excluded(_), Unbounded) => left_lower.clone(), + (Unbounded, Unbounded) => Unbounded, + (Included(l) | Excluded(l), Included(r) | Excluded(r)) => match l.cmp(r) { + Ordering::Less => right_lower.clone(), + Ordering::Equal => match (left_lower, right_lower) { + (Included(_), Excluded(v)) => Excluded(v.clone()), + (Excluded(_), Excluded(v)) => Excluded(v.clone()), + (Excluded(v), Included(_)) => Excluded(v.clone()), + (Included(_), Included(v)) => Included(v.clone()), + _ => unreachable!(), + }, + Ordering::Greater => left_lower.clone(), }, + }; - // Left contains an infinite interval: - (Some((l1, None)), Some((r1, Some(r2)))) => match r2.cmp(l1) { + // At this point we know there is an overlap between the versions, find the lowest bound + let upper = match (left_upper, right_upper) { + (Unbounded, Included(_) | Excluded(_)) => { + right = right_iter.next(); + right_upper.clone() + } + (Included(_) | Excluded(_), Unbounded) => { + left = left_iter.next(); + left_upper.clone() + } + (Unbounded, Unbounded) => { + left = left_iter.next(); + right = right_iter.next(); + Unbounded + } + (Included(l) | Excluded(l), Included(r) | Excluded(r)) => match l.cmp(r) { Ordering::Less => { - right = right_iter.next(); + left = left_iter.next(); + left_upper.clone() } - Ordering::Equal => { - for r in right_iter.cloned() { - segments.push(r) + Ordering::Equal => match (left_upper, right_upper) { + (Included(_), Excluded(v)) => { + right = right_iter.next(); + Excluded(v.clone()) } - 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) + (Excluded(_), Excluded(v)) => { + left = left_iter.next(); + right = right_iter.next(); + Excluded(v.clone()) + } + (Excluded(v), Included(_)) => { + left = left_iter.next(); + Excluded(v.clone()) } - break; + (Included(_), Included(v)) => { + left = left_iter.next(); + right = right_iter.next(); + Included(v.clone()) + } + _ => unreachable!(), + }, + Ordering::Greater => { + right = right_iter.next(); + right_upper.clone() } }, + }; - // Both sides contain an infinite interval: - (Some((l1, None)), Some((r1, None))) => { - let start = l1.max(r1).to_owned(); - segments.push((start, None)); - break; - } - - // Left or right has ended. - _ => { - break; - } - } + segments.push((lower, upper)); } Self { segments } } } -// 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 +impl VersionSet for Range { + type V = T; + + fn empty() -> Self { + Range::empty() } - /// 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 singleton(v: Self::V) -> Self { + Range::singleton(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 complement(&self) -> Self { + Range::complement(self) + } + + fn intersection(&self, other: &Self) -> Self { + Range::intersection(self, other) + } + + fn contains(&self, v: &Self::V) -> bool { + Range::contains(self, v) + } + + fn full() -> Self { + Range::full() } } // 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 }) } } @@ -363,29 +467,61 @@ fn interval_to_string((start, maybe_end): &Interval) -> String { #[cfg(test)] pub mod tests { use proptest::prelude::*; - - use crate::version::NumberVersion; + use proptest::test_runner::TestRng; 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 } - }) + pub fn strategy() -> impl Strategy> { + prop::collection::vec(any::(), 0..10) + .prop_map(|mut vec| { + vec.sort_unstable(); + vec.dedup(); + vec + }) + .prop_perturb(|vec, mut rng| { + let mut segments = SmallVec::empty(); + let mut iter = vec.into_iter().peekable(); + if let Some(first) = iter.next() { + fn next_bound>( + iter: &mut I, + rng: &mut TestRng, + ) -> Bound { + if let Some(next) = iter.next() { + if rng.gen_bool(0.5) { + Included(next) + } else { + Excluded(next) + } + } else { + Unbounded + } + } + + let start = if rng.gen_bool(0.3) { + Unbounded + } else { + if rng.gen_bool(0.5) { + Included(first) + } else { + Excluded(first) + } + }; + + let end = next_bound(&mut iter, &mut rng); + segments.push((start, end)); + + while iter.peek().is_some() { + let start = next_bound(&mut iter, &mut rng); + let end = next_bound(&mut iter, &mut rng); + segments.push((start, end)); + } + } + return Range { segments }; + }) } - fn version_strat() -> impl Strategy { - any::().prop_map(NumberVersion) + fn version_strat() -> impl Strategy { + any::() } proptest! { @@ -394,17 +530,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 +552,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 +572,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 +584,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 +596,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 +618,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 530cd9ca00c5ed4cc6218427c3ca2871c65ce6f8 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Sat, 28 May 2022 22:43:43 +0200 Subject: [PATCH 02/13] fix: rewrite range proptest strategy --- src/range.rs | 77 ++++++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 39 deletions(-) diff --git a/src/range.rs b/src/range.rs index 13c96b09..ff2a9652 100644 --- a/src/range.rs +++ b/src/range.rs @@ -467,54 +467,53 @@ impl<'de, V: serde::Deserialize<'de>> serde::Deserialize<'de> for Range { #[cfg(test)] pub mod tests { use proptest::prelude::*; - use proptest::test_runner::TestRng; use super::*; pub fn strategy() -> impl Strategy> { - prop::collection::vec(any::(), 0..10) - .prop_map(|mut vec| { - vec.sort_unstable(); - vec.dedup(); - vec - }) - .prop_perturb(|vec, mut rng| { - let mut segments = SmallVec::empty(); - let mut iter = vec.into_iter().peekable(); - if let Some(first) = iter.next() { - fn next_bound>( - iter: &mut I, - rng: &mut TestRng, - ) -> Bound { - if let Some(next) = iter.next() { - if rng.gen_bool(0.5) { - Included(next) - } else { - Excluded(next) - } - } else { - Unbounded - } - } - - let start = if rng.gen_bool(0.3) { - Unbounded - } else { - if rng.gen_bool(0.5) { - Included(first) + ( + any::<(bool, bool)>(), + prop::collection::vec(any::<(u32, bool)>(), 1..10), + ) + .prop_map(|((start_bounded, end_bounded), mut vec)| { + // Ensure the bounds are increasing and non-repeating + vec.sort_by_key(|(value, _)| *value); + vec.dedup_by_key(|(value, _)| *value); + + // Construct an iterator of bounds instead of values + let mut vec: Vec<_> = vec + .into_iter() + .map(|(value, inclusive)| { + if inclusive { + Included(value) } else { - Excluded(first) + Excluded(value) } - }; + }) + .collect(); - let end = next_bound(&mut iter, &mut rng); - segments.push((start, end)); + // Add the start bound + if !start_bounded { + vec.insert(0, Unbounded); + } - while iter.peek().is_some() { - let start = next_bound(&mut iter, &mut rng); - let end = next_bound(&mut iter, &mut rng); - segments.push((start, end)); + // Add end bound + if !end_bounded { + if (vec.len() % 2) == 0 { + // Drop the last element if it would result in an uneven vec + vec.pop(); } + vec.push(Unbounded); + } else if (vec.len() % 2) == 1 { + // Drop the last element if it would result in an uneven vec + vec.pop(); + } + + let mut segments = SmallVec::empty(); + let mut iter = vec.into_iter(); + while let Some(start) = iter.next() { + let end = iter.next().expect("not an even amount of values"); + segments.push((start, end)); } return Range { segments }; }) From bb2f2c63c723d3eb69147a996dd4cdc0b6929e80 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Sat, 28 May 2022 22:49:56 +0200 Subject: [PATCH 03/13] fix: deserialize SmallVec without Vec alloc --- src/internal/small_vec.rs | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/src/internal/small_vec.rs b/src/internal/small_vec.rs index 65db25ac..c2a178a9 100644 --- a/src/internal/small_vec.rs +++ b/src/internal/small_vec.rs @@ -126,13 +126,36 @@ 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") + } - let mut v = Self::empty(); - for item in items { - v.push(item); + 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) + } } - Ok(v) + + let visitor = SmallVecVisitor { + marker: Default::default(), + }; + d.deserialize_seq(visitor) } } From e72a3f5e3469cd26ca59aa2959491707e93fdd42 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Sat, 28 May 2022 22:50:57 +0200 Subject: [PATCH 04/13] fix: remove not_equals --- src/range.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/range.rs b/src/range.rs index ff2a9652..e20688cd 100644 --- a/src/range.rs +++ b/src/range.rs @@ -95,14 +95,6 @@ impl Range { } } - /// Set containing all versions expect one - pub fn not_equal(v: impl Into) -> Self { - let v = v.into(); - Self { - segments: SmallVec::Two([(Unbounded, Excluded(v.clone())), (Excluded(v), Unbounded)]), - } - } - /// Returns the complement of this Range. pub fn complement(&self) -> Self { match self.segments.first() { From 71fbe52e63c9baf52f9b83e932697f5241330a1c Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Sat, 28 May 2022 23:11:59 +0200 Subject: [PATCH 05/13] fix: re-add union and remove early out --- src/range.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/range.rs b/src/range.rs index e20688cd..6d88cd38 100644 --- a/src/range.rs +++ b/src/range.rs @@ -168,12 +168,6 @@ impl Range { /// Returns true if the this Range contains the specified value. pub fn contains(&self, v: &V) -> bool { - if let Some(bounding_range) = self.bounding_range() { - if !bounding_range.contains(v) { - return false; - } - } - for segment in self.segments.iter() { if match segment { (Unbounded, Unbounded) => true, @@ -230,6 +224,13 @@ fn bound_as_ref(bound: &Bound) -> Bound<&V> { } impl Range { + /// Computes the union of this `Range` and another. + pub fn union(&self, other: &Self) -> Self { + self.complement() + .intersection(&other.complement()) + .complement() + } + /// Computes the intersection of two sets of versions. pub fn intersection(&self, other: &Self) -> Self { let mut segments: SmallVec> = SmallVec::empty(); @@ -387,6 +388,10 @@ impl VersionSet for Range { fn full() -> Self { Range::full() } + + fn union(&self, other: &Self) -> Self { + Range::union(self, other) + } } // REPORT ###################################################################### From a1ea90036f0b06c77cd5aad9065c5bc1a425bad6 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Sat, 28 May 2022 23:13:42 +0200 Subject: [PATCH 06/13] fix: renamed V to VS in bench --- benches/large_case.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/benches/large_case.rs b/benches/large_case.rs index 33d836a1..8efdaa9e 100644 --- a/benches/large_case.rs +++ b/benches/large_case.rs @@ -11,13 +11,13 @@ use pubgrub::version::{NumberVersion, SemanticVersion}; use pubgrub::version_set::VersionSet; use serde::de::Deserialize; -fn bench<'a, P: Package + Deserialize<'a>, V: VersionSet + Deserialize<'a>>( +fn bench<'a, P: Package + Deserialize<'a>, VS: VersionSet + Deserialize<'a>>( b: &mut Bencher, case: &'a str, ) where - ::V: Deserialize<'a>, + ::V: Deserialize<'a>, { - let dependency_provider: OfflineDependencyProvider = ron::de::from_str(&case).unwrap(); + let dependency_provider: OfflineDependencyProvider = ron::de::from_str(&case).unwrap(); b.iter(|| { for p in dependency_provider.packages() { From d14f6faa8c1c5e5d82cceb085cb4e838b45ccd9d Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Tue, 7 Jun 2022 18:15:33 +0200 Subject: [PATCH 07/13] refactor: simpler intersection Co-authored-by: Jacob Finkelman --- src/range.rs | 204 +++++++++++++++++++-------------------------------- 1 file changed, 74 insertions(+), 130 deletions(-) diff --git a/src/range.rs b/src/range.rs index 6d88cd38..f55153de 100644 --- a/src/range.rs +++ b/src/range.rs @@ -19,7 +19,6 @@ use crate::{internal::small_vec::SmallVec, version_set::VersionSet}; use std::ops::RangeBounds; use std::{ - cmp::Ordering, fmt::{Debug, Display, Formatter}, ops::Bound::{self, Excluded, Included, Unbounded}, }; @@ -202,15 +201,40 @@ impl Range { Excluded(v) => Excluded(v.clone().into()), Unbounded => Unbounded, }; - match (start, end) { - (Included(a), Included(b)) if b < a => Self::empty(), - (Excluded(a), Excluded(b)) if b < a => Self::empty(), - (Included(a), Excluded(b)) if b <= a => Self::empty(), - (Excluded(a), Included(b)) if b <= a => Self::empty(), - (a, b) => Self { - segments: SmallVec::one((a, b)), - }, + if valid_segment(&start, &end) { + Self { + segments: SmallVec::one((start, end)), + } + } else { + Self::empty() + } + } + + fn check_invariants(self) -> Self { + if cfg!(debug_assertions) { + for (i, (s, e)) in self.segments.iter().enumerate() { + if matches!(s, Unbounded) && i != 0 { + panic!() + } + if matches!(e, Unbounded) && i != (self.segments.len() - 1) { + panic!() + } + } + 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!(), + } + } + for (s, e) in self.segments.iter() { + assert!(valid_segment(s, e)); + } } + self } } @@ -223,142 +247,62 @@ fn bound_as_ref(bound: &Bound) -> Bound<&V> { } } +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, + } +} + impl Range { /// Computes the union of this `Range` and another. pub fn union(&self, other: &Self) -> Self { self.complement() .intersection(&other.complement()) .complement() + .check_invariants() } /// Computes the intersection of two sets of versions. pub fn intersection(&self, other: &Self) -> Self { let mut segments: SmallVec> = 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(); - while let (Some((left_lower, left_upper)), Some((right_lower, right_upper))) = (left, right) + 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()) { - // Check if the left range completely smaller than the right range. - if let ( - Included(left_upper_version) | Excluded(left_upper_version), - Included(right_lower_version) | Excluded(right_lower_version), - ) = (left_upper, right_lower) - { - match left_upper_version.cmp(right_lower_version) { - Ordering::Less => { - // Left range is disjoint from the right range. - left = left_iter.next(); - continue; - } - Ordering::Equal => { - if !matches!((left_upper, right_lower), (Included(_), Included(_))) { - // Left and right are overlapping exactly, but one of the bounds is exclusive, therefor the ranges are disjoint - left = left_iter.next(); - continue; - } - } - Ordering::Greater => { - // Left upper bound is greater than right lower bound, so the lower bound is the right lower bound - } - } + 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!(), } - // Check if the right range completely smaller than the left range. - if let ( - Included(left_lower_version) | Excluded(left_lower_version), - Included(right_upper_version) | Excluded(right_upper_version), - ) = (left_lower, right_upper) - { - match right_upper_version.cmp(left_lower_version) { - Ordering::Less => { - // Right range is disjoint from the left range. - right = right_iter.next(); - continue; - } - Ordering::Equal => { - if !matches!((right_upper, left_lower), (Included(_), Included(_))) { - // Left and right are overlapping exactly, but one of the bounds is exclusive, therefor the ranges are disjoint - right = right_iter.next(); - continue; - } - } - Ordering::Greater => { - // Right upper bound is greater than left lower bound, so the lower bound is the left lower bound - } - } + .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)) } - - // At this point we know there is an overlap between the versions, find the lowest bound - let lower = match (left_lower, right_lower) { - (Unbounded, Included(_) | Excluded(_)) => right_lower.clone(), - (Included(_) | Excluded(_), Unbounded) => left_lower.clone(), - (Unbounded, Unbounded) => Unbounded, - (Included(l) | Excluded(l), Included(r) | Excluded(r)) => match l.cmp(r) { - Ordering::Less => right_lower.clone(), - Ordering::Equal => match (left_lower, right_lower) { - (Included(_), Excluded(v)) => Excluded(v.clone()), - (Excluded(_), Excluded(v)) => Excluded(v.clone()), - (Excluded(v), Included(_)) => Excluded(v.clone()), - (Included(_), Included(v)) => Included(v.clone()), - _ => unreachable!(), - }, - Ordering::Greater => left_lower.clone(), - }, - }; - - // At this point we know there is an overlap between the versions, find the lowest bound - let upper = match (left_upper, right_upper) { - (Unbounded, Included(_) | Excluded(_)) => { - right = right_iter.next(); - right_upper.clone() - } - (Included(_) | Excluded(_), Unbounded) => { - left = left_iter.next(); - left_upper.clone() - } - (Unbounded, Unbounded) => { - left = left_iter.next(); - right = right_iter.next(); - Unbounded - } - (Included(l) | Excluded(l), Included(r) | Excluded(r)) => match l.cmp(r) { - Ordering::Less => { - left = left_iter.next(); - left_upper.clone() - } - Ordering::Equal => match (left_upper, right_upper) { - (Included(_), Excluded(v)) => { - right = right_iter.next(); - Excluded(v.clone()) - } - (Excluded(_), Excluded(v)) => { - left = left_iter.next(); - right = right_iter.next(); - Excluded(v.clone()) - } - (Excluded(v), Included(_)) => { - left = left_iter.next(); - Excluded(v.clone()) - } - (Included(_), Included(v)) => { - left = left_iter.next(); - right = right_iter.next(); - Included(v.clone()) - } - _ => unreachable!(), - }, - Ordering::Greater => { - right = right_iter.next(); - right_upper.clone() - } - }, - }; - - segments.push((lower, upper)); } - Self { segments } + Self { segments }.check_invariants() } } @@ -614,7 +558,7 @@ 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); + let rv: Range<_> = Range::from_range_bounds(range); assert_eq!(range.contains(&version), rv.contains(&version)); } From c44639a02e08c3065771c80a3905b36af17d131e Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Tue, 7 Jun 2022 18:47:14 +0200 Subject: [PATCH 08/13] test: use deltas for range strategy Co-authored-by: Jacob Finkelman --- src/range.rs | 88 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 51 insertions(+), 37 deletions(-) diff --git a/src/range.rs b/src/range.rs index f55153de..bfafb956 100644 --- a/src/range.rs +++ b/src/range.rs @@ -413,50 +413,64 @@ pub mod tests { pub fn strategy() -> impl Strategy> { ( - any::<(bool, bool)>(), + any::(), prop::collection::vec(any::<(u32, bool)>(), 1..10), ) - .prop_map(|((start_bounded, end_bounded), mut vec)| { - // Ensure the bounds are increasing and non-repeating - vec.sort_by_key(|(value, _)| *value); - vec.dedup_by_key(|(value, _)| *value); - - // Construct an iterator of bounds instead of values - let mut vec: Vec<_> = vec - .into_iter() - .map(|(value, inclusive)| { - if inclusive { - Included(value) - } else { - Excluded(value) + .prop_map(|(start_bounded, deltas)| { + let mut start = if start_bounded { 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; } - }) - .collect(); - - // Add the start bound - if !start_bounded { - vec.insert(0, Unbounded); - } - - // Add end bound - if !end_bounded { - if (vec.len() % 2) == 0 { - // Drop the last element if it would result in an uneven vec - vec.pop(); + }; + + 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 0 and the start bound itself is + // exclusive, we wont get a valid segment. E.g: + // + // Exclusive(x), Exclusive(x) -> Basically nothing, so invalid + // Exclusive(x), Inclusive(x) -> Also invalid + // + // If that's the case, skip this delta + if delta == 0 && matches!(start_bound, Excluded(_)) { + 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 both the last + // end bound and the current bound are inclusive, we skip the delta because + // they basically overlap. + if delta == 0 && last_bound_was_inclusive && inclusive { + continue; + } + start = Some(current_bound); } - vec.push(Unbounded); - } else if (vec.len() % 2) == 1 { - // Drop the last element if it would result in an uneven vec - vec.pop(); } - let mut segments = SmallVec::empty(); - let mut iter = vec.into_iter(); - while let Some(start) = iter.next() { - let end = iter.next().expect("not an even amount of values"); - segments.push((start, end)); + // 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 }; + + return Range { segments }.check_invariants(); }) } From 63277a01874b0cb04a4aa2211977aa185c0b9836 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Fri, 10 Jun 2022 20:25:54 +0200 Subject: [PATCH 09/13] docs(range): added comment about discrete values --- src/range.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/range.rs b/src/range.rs index bfafb956..5c3ccaf0 100644 --- a/src/range.rs +++ b/src/range.rs @@ -15,6 +15,15 @@ //! - [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` +//! +//! Ranges can be created from any type that implements [`Ord`] + [`Clone`]. +//! +//! In general any range will always be represented in the same way unless the the type `T` does not +//! represent a continuous space. If the type `T` does not represent a continuous space there is +//! no way to distinguish an unbounded bound from the minimal or maximum value of the range. For +//! instance the segment `[Unbounded, Exclusive(0u32)]` does not include any value but its +//! representation differs from the empty set. This means that for types that represent a discrete +//! range there is the potential that the same set of versions is represented in multiple ways. use crate::{internal::small_vec::SmallVec, version_set::VersionSet}; use std::ops::RangeBounds; From 83d8725bae3418498e6ce3fd4ed60503f0d6bd6a Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 25 Jun 2022 21:37:34 +0200 Subject: [PATCH 10/13] More restrictive for valid random range generation --- src/range.rs | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/range.rs b/src/range.rs index 5c3ccaf0..7560f806 100644 --- a/src/range.rs +++ b/src/range.rs @@ -420,6 +420,8 @@ pub mod tests { use super::*; + /// Generate version sets from a random vector of deltas between bounds. + /// Each bound is randomly inclusive or exclusive. pub fn strategy() -> impl Strategy> { ( any::(), @@ -449,24 +451,19 @@ pub mod tests { // 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 0 and the start bound itself is - // exclusive, we wont get a valid segment. E.g: - // - // Exclusive(x), Exclusive(x) -> Basically nothing, so invalid - // Exclusive(x), Inclusive(x) -> Also invalid - // - // If that's the case, skip this delta - if delta == 0 && matches!(start_bound, Excluded(_)) { + // 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 both the last - // end bound and the current bound are inclusive, we skip the delta because - // they basically overlap. - if delta == 0 && last_bound_was_inclusive && inclusive { + // 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); From 3bc73f1dbaa60c8211c816edfffbecfece87936f Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 25 Jun 2022 23:04:47 +0200 Subject: [PATCH 11/13] Remove duplicate check in check_invariants --- src/range.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/range.rs b/src/range.rs index 7560f806..66fb20e0 100644 --- a/src/range.rs +++ b/src/range.rs @@ -221,14 +221,6 @@ impl Range { fn check_invariants(self) -> Self { if cfg!(debug_assertions) { - for (i, (s, e)) in self.segments.iter().enumerate() { - if matches!(s, Unbounded) && i != 0 { - panic!() - } - if matches!(e, Unbounded) && i != (self.segments.len() - 1) { - panic!() - } - } 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), From f35c2e975a87bb629d4a400782ae49f08ffe1bbc Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 25 Jun 2022 23:30:06 +0200 Subject: [PATCH 12/13] Add warning about non-unique ranges representations --- src/range.rs | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/src/range.rs b/src/range.rs index 66fb20e0..f8b4479c 100644 --- a/src/range.rs +++ b/src/range.rs @@ -18,12 +18,37 @@ //! //! Ranges can be created from any type that implements [`Ord`] + [`Clone`]. //! -//! In general any range will always be represented in the same way unless the the type `T` does not -//! represent a continuous space. If the type `T` does not represent a continuous space there is -//! no way to distinguish an unbounded bound from the minimal or maximum value of the range. For -//! instance the segment `[Unbounded, Exclusive(0u32)]` does not include any value but its -//! representation differs from the empty set. This means that for types that represent a discrete -//! range there is the potential that the same set of versions is represented in multiple ways. +//! 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; From ec8dfce1e746db2bfc2cdf904bb7dc2133da469f Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 25 Jun 2022 23:36:03 +0200 Subject: [PATCH 13/13] Rename start_bounded into start_unbounded --- src/range.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/range.rs b/src/range.rs index f8b4479c..8d900814 100644 --- a/src/range.rs +++ b/src/range.rs @@ -444,8 +444,12 @@ pub mod tests { any::(), prop::collection::vec(any::<(u32, bool)>(), 1..10), ) - .prop_map(|(start_bounded, deltas)| { - let mut start = if start_bounded { Some(Unbounded) } else { None }; + .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;