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 all commits
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
164 changes: 152 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,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)]
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 +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())
}
}
}
Loading