Fix algorithm with new proptest

Author: konstin

version-ranges/src/lib.rs

@@ -826,7 +826,7 @@ impl<V: Ord + Clone> Ranges<V> {
     }
 }
 
-impl<V: Ord + Debug> FromIterator<(Bound<V>, Bound<V>)> for Ranges<V> {
+impl<V: Ord> FromIterator<(Bound<V>, Bound<V>)> for Ranges<V> {
     /// Constructor from arbitrary, unsorted and potentially overlapping ranges.
     ///
     /// This is equivalent, but faster, to computing the [`Ranges::union`] of the
@@ -836,6 +836,10 @@ impl<V: Ord + Debug> FromIterator<(Bound<V>, Bound<V>)> for Ranges<V> {
         // 1. The segments are sorted, from lowest to highest (through `Ord`): By sorting.
         // 2. Each segment contains at least one version (start < end): By `union`.
         // 3. There is at least one version between two segments: By `union`.
+
+        // For this implementation, we choose to only build a single smallvec and insert or remove
+        // in it, instead of e.g. collecting the segments into one smallvec, sorting that and then
+        // construction the second smallvec without shifting.
         let mut segments: SmallVec<[Interval<V>; 1]> = SmallVec::new();
 
         for segment in iter {
@@ -855,7 +859,8 @@ impl<V: Ord + Debug> FromIterator<(Bound<V>, Bound<V>)> for Ranges<V> {
                     &segment.start_bound(),
                 );
 
-            // Is it overlapping with the following segment?
+            // Is it overlapping with the following segment? We'll check if there's more than one
+            // overlap later.
             let next_overlapping = insertion_point < segments.len()
                 && !end_before_start_with_gap(
                     &segment.end_bound(),
@@ -867,29 +872,112 @@ impl<V: Ord + Debug> FromIterator<(Bound<V>, Bound<V>)> for Ranges<V> {
                     // previous:  |------|
                     // segment:       |------|
                     // following:          |------|
+                    // final:     |---------------|
+                    //
+                    // OR
                     //
+                    // previous:  |------|
+                    // segment:       |-----------|
+                    // following:          |----|
                     // final:     |---------------|
+                    //
+                    // OR
+                    //
+                    // previous:  |------|
+                    // segment:       |----------------|
+                    // following:          |----|   |------|
+                    // final:     |------------------------|
                     // We merge all three segments into one, which is effectively removing one of
                     // two previously inserted and changing the bounds on the other.
-                    let following = segments.remove(insertion_point);
-                    segments[insertion_point - 1].1 = following.1;
+
+                    // Remove all elements covered by the final element
+                    let mut following = segments.remove(insertion_point);
+                    while insertion_point < segments.len()
+                        && !end_before_start_with_gap(
+                            &segment.end_bound(),
+                            &segments[insertion_point].start_bound(),
+                        )
+                    {
+                        following = segments.remove(insertion_point);
+                    }
+
+                    // Set end to max(segment.end, <last overlapping segment>.end)
+                    if cmp_bounds_end(segment.end_bound(), following.end_bound())
+                        .unwrap()
+                        .is_lt()
+                    {
+                        segments[insertion_point - 1].1 = following.1;
+                    } else {
+                        segments[insertion_point - 1].1 = segment.1;
+                    }
                 }
                 (true, false) => {
                     // previous:  |------|
                     // segment:       |------|
                     // following:                |------|
                     //
+                    // OR
+                    //
+                    // previous:  |----------|
+                    // segment:       |---|
+                    // following:                |------|
+                    //
                     // final:     |----------|   |------|
                     // We can reuse the existing element by extending it.
-                    segments[insertion_point - 1].1 = segment.1;
+
+                    // Set end to max(segment.end, <previous>.end)
+                    if cmp_bounds_end(
+                        segments[insertion_point - 1].end_bound(),
+                        segment.end_bound(),
+                    )
+                    .unwrap()
+                    .is_lt()
+                    {
+                        segments[insertion_point - 1].1 = segment.1;
+                    }
                 }
                 (false, true) => {
                     // previous:  |------|
                     // segment:             |------|
                     // following:               |------|
+                    // final:    |------|   |----------|
+                    //
+                    // OR
+                    //
+                    // previous:  |------|
+                    // segment:             |----------|
+                    // following:               |---|
+                    // final:    |------|   |----------|
+                    //
+                    // OR
+                    //
+                    // previous:  |------|
+                    // segment:             |------------|
+                    // following:               |---|  |------|
                     //
-                    // final:    |------|   |---------|
+                    // final:    |------|   |-----------------|
                     // We can reuse the existing element by extending it.
+
+                    // Remove all fully covered segments so the next element is the last one that
+                    // overlaps.
+                    while insertion_point + 1 < segments.len()
+                        && !end_before_start_with_gap(
+                            &segment.end_bound(),
+                            &segments[insertion_point + 1].start_bound(),
+                        )
+                    {
+                        // We know that the one after also overlaps, so we can drop the current
+                        // following.
+                        segments.remove(insertion_point);
+                    }
+
+                    // Set end to max(segment.end, <last overlapping segment>.end)
+                    if cmp_bounds_end(segments[insertion_point].end_bound(), segment.end_bound())
+                        .unwrap()
+                        .is_lt()
+                    {
+                        segments[insertion_point].1 = segment.1;
+                    }
                     segments[insertion_point].0 = segment.0;
                 }
                 (false, false) => {
@@ -1214,8 +1302,12 @@ pub mod tests {
 
         #[test]
         fn from_iter_valid(segments in proptest::collection::vec(any::<(Bound<u32>, Bound<u32>)>(), ..30)) {
-            // We check invariants in the method.
-            Ranges::from_iter(segments.clone());
+            let mut expected = Ranges::empty();
+            for segment in &segments {
+                expected = expected.union(&Ranges::from_range_bounds(*segment));
+            }
+            let actual =  Ranges::from_iter(segments.clone());
+            assert_eq!(expected, actual, "{segments:?}");
         }
     }
 
@@ -1253,15 +1345,6 @@ pub mod tests {
         );
     }
 
-    #[test]
-    fn from_iter_regression() {
-        Ranges::from_iter([
-            (Included(0), Included(0)),
-            (Excluded(1u32), Unbounded),
-            (Included(0), Included(0)),
-        ]);
-    }
-
     #[test]
     fn version_ord() {
         let versions: &[Ranges<u32>] = &[