Skip to content

Commit

Permalink
feat: add a simplify for error messages (#156)
Browse files Browse the repository at this point in the history 
* feat: add a `simplify` for error messages

* Fix broken links

Co-authored-by: konsti <[email protected]>

* Inline locate_versions, to simplify lifetimes

While attempting to use this simplification code I got an odd lifetime error with
```
let c = set.complement();
let s = c.simplify(versions);
s.complement()
```
By in lining locate_versions the lifetimes could be simplified so that that code works

* correct capitalization

Co-authored-by: Zanie Blue <[email protected]>

* improve comment

Co-authored-by: Zanie Blue <[email protected]>

---------

Co-authored-by: konsti <[email protected]>
Co-authored-by: Zanie Blue <[email protected]>
  • Loading branch information
@Eh2406 @konstin @zanieb
3 people authored Nov 29, 2023
1 parent acfbe99 commit 2b2d8d4
Showing 1 changed file with 152 additions and 12 deletions.
164 changes: 152 additions & 12 deletions src/range.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
//! 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::cmp::Ordering;
use std::ops::RangeBounds;
use std::{
fmt::{Debug, Display, Formatter},
Expand Down Expand Up @@ -202,23 +203,37 @@ impl<V: Ord> Range<V> {
/// 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;
match within_bounds(v, segment) {
Ordering::Less => return false,
Ordering::Equal => return true,
Ordering::Greater => (),
}
}
false
}

/// Returns true if the this Range contains the specified values.
///
/// The `versions` iterator must be sorted.
/// Functionally equivalent to `versions.map(|v| self.contains(v))`.
/// Except it runs in `O(size_of_range + len_of_versions)` not `O(size_of_range * len_of_versions)`
pub fn contains_many<'s, I>(&'s self, versions: I) -> impl Iterator<Item = bool> + 's
where
I: Iterator<Item = &'s V> + 's,
V: 's,
{
versions.scan(0, move |i, v| {
while let Some(segment) = self.segments.get(*i) {
match within_bounds(v, segment) {
Ordering::Less => return Some(false),
Ordering::Equal => return Some(true),
Ordering::Greater => *i += 1,
}
}
Some(false)
})
}

/// Construct a simple range from anything that impls [RangeBounds] like `v1..v2`.
pub fn from_range_bounds<R, IV>(bounds: R) -> Self
where
Expand Down Expand Up @@ -264,6 +279,26 @@ impl<V: Ord> Range<V> {
}
}

fn within_bounds<V: PartialOrd>(v: &V, segment: &Interval<V>) -> Ordering {
let below_lower_bound = match segment {
(Excluded(start), _) => v <= start,
(Included(start), _) => v < start,
(Unbounded, _) => false,
};
if below_lower_bound {
return Ordering::Less;
}
let below_upper_bound = match segment {
(_, Unbounded) => true,
(_, Included(end)) => v <= end,
(_, Excluded(end)) => v < end,
};
if below_upper_bound {
return Ordering::Equal;
}
Ordering::Greater
}

fn valid_segment<T: PartialOrd>(start: &Bound<T>, end: &Bound<T>) -> bool {
match (start, end) {
(Included(s), Included(e)) => s <= e,
Expand All @@ -274,6 +309,36 @@ fn valid_segment<T: PartialOrd>(start: &Bound<T>, end: &Bound<T>) -> bool {
}
}

/// Group adjacent versions locations.
///
/// ```text
/// [None, 3, 6, 7, None] -> [(3, 7)]
/// [3, 6, 7, None] -> [(None, 7)]
/// [3, 6, 7] -> [(None, None)]
/// [None, 1, 4, 7, None, None, None, 8, None, 9] -> [(1, 7), (8, 8), (9, None)]
/// ```
fn group_adjacent_locations(
mut locations: impl Iterator<Item = Option<usize>>,
) -> impl Iterator<Item = (Option<usize>, Option<usize>)> {
// If the first version matched, then the lower bound of that segment is not needed
let mut seg = locations.next().flatten().map(|ver| (None, Some(ver)));
std::iter::from_fn(move || {
for ver in locations.by_ref() {
if let Some(ver) = ver {
// As long as were still matching versions, we keep merging into the currently matching segment
seg = Some((seg.map_or(Some(ver), |(s, _)| s), Some(ver)));
} else {
// If we have found a version that doesn't match, then right the merge segment and prepare for a new one.
if seg.is_some() {
return seg.take();
}
}
}
// If the last version matched, then write out the merged segment but the upper bound is not needed.
seg.take().map(|(s, _)| (s, None))
})
}

impl<V: Ord + Clone> Range<V> {
/// Computes the union of this `Range` and another.
pub fn union(&self, other: &Self) -> Self {
Expand Down Expand Up @@ -321,6 +386,54 @@ impl<V: Ord + Clone> Range<V> {

Self { segments }.check_invariants()
}

/// Returns a simpler Range that contains the same versions
///
/// For every one of the Versions provided in versions the existing range and
/// the simplified range will agree on whether it is contained.
/// The simplified version may include or exclude versions that are not in versions as the implementation wishes.
/// For example:
/// - If all the versions are contained in the original than the range will be simplified to `full`.
/// - If none of the versions are contained in the original than the range will be simplified to `empty`.
///
/// If versions are not sorted the correctness of this function is not guaranteed.
pub fn simplify<'v, I>(&self, versions: I) -> Self
where
I: Iterator<Item = &'v V> + 'v,
V: 'v,
{
// Return the segment index in the range for each version in the range, None otherwise
let version_locations = versions.scan(0, move |i, v| {
while let Some(segment) = self.segments.get(*i) {
match within_bounds(v, segment) {
Ordering::Less => return Some(None),
Ordering::Equal => return Some(Some(*i)),
Ordering::Greater => *i += 1,
}
}
Some(None)
});
let kept_segments = group_adjacent_locations(version_locations);
self.keep_segments(kept_segments)
}

/// Create a new range with a subset of segments at given location bounds.
///
/// Each new segment is constructed from a pair of segments, taking the
/// start of the first and the end of the second.
fn keep_segments(
&self,
kept_segments: impl Iterator<Item = (Option<usize>, Option<usize>)>,
) -> Range<V> {
let mut segments = SmallVec::Empty;
for (s, e) in kept_segments {
segments.push((
s.map_or(Unbounded, |s| self.segments[s].0.clone()),
e.map_or(Unbounded, |e| self.segments[e].1.clone()),
));
}
Self { segments }.check_invariants()
}
}

impl<T: Debug + Display + Clone + Eq + Ord> VersionSet for Range<T> {
Expand Down Expand Up @@ -600,5 +713,32 @@ pub mod tests {
let rv2: Range<u32> = rv.bounding_range().map(Range::from_range_bounds::<_, u32>).unwrap_or_else(Range::empty);
assert_eq!(rv, rv2);
}

#[test]
fn contains(range in strategy(), versions in proptest::collection::vec(version_strat(), ..30)) {
for v in versions {
assert_eq!(range.contains(&v), range.segments.iter().any(|s| RangeBounds::contains(s, &v)));
}
}

#[test]
fn contains_many(range in strategy(), mut versions in proptest::collection::vec(version_strat(), ..30)) {
versions.sort();
assert_eq!(versions.len(), range.contains_many(versions.iter()).count());
for (a, b) in versions.iter().zip(range.contains_many(versions.iter())) {
assert_eq!(range.contains(a), b);
}
}

#[test]
fn simplify(range in strategy(), mut versions in proptest::collection::vec(version_strat(), ..30)) {
versions.sort();
let simp = range.simplify(versions.iter());

for v in versions {
assert_eq!(range.contains(&v), simp.contains(&v));
}
assert!(simp.segments.len() <= range.segments.len())
}
}
}

0 comments on commit 2b2d8d4

Please sign in to comment.