Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add a simplify for error messages #156

Merged
merged 5 commits into from
Nov 29, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 145 additions & 12 deletions src/range.rs
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,46 @@ 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,
{
self.locate_versions(versions).map(|m| m.is_some())
}

/// Return the segment index in the range for each version in the range, None otherwise
fn locate_versions<'s, I>(&'s self, versions: I) -> impl Iterator<Item = Option<usize>> + 's
where
I: Iterator<Item = &'s V> + 's,
V: 's,
{
versions.scan(0, |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)
})
}

/// 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 +288,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 +318,33 @@ fn valid_segment<T: PartialOrd>(start: &Bound<T>, end: &Bound<T>) -> bool {
}
}

/// group adjacent versions locations
Eh2406 marked this conversation as resolved.
Show resolved Hide resolved
/// [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)]
Eh2406 marked this conversation as resolved.
Show resolved Hide resolved
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 +392,41 @@ 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<'s, I>(&'s self, versions: I) -> Self
where
I: Iterator<Item = &'s V> + 's,
V: 's,
{
let version_locations = self.locate_versions(versions);
let kept_segments = group_adjacent_locations(version_locations);
self.keep_segments(kept_segments)
}

/// simplify range with segments at given location bounds.
Eh2406 marked this conversation as resolved.
Show resolved Hide resolved
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 +706,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())
}
}
}
Loading