From af66cedd709cde5c043f8c10bb46b9224644c5e8 Mon Sep 17 00:00:00 2001
From: konstin <konstin@mailbox.org>
Date: Tue, 5 Mar 2024 13:22:17 +0100
Subject: [PATCH 01/15] Use binary search for `Range::contains`

---
 src/range.rs | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/src/range.rs b/src/range.rs
index a7eb0e82..90e6dc56 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -221,19 +221,32 @@ impl<V: Ord> Range<V> {
         })
     }
 
-    /// Returns true if the this Range contains the specified value.
+    /// Returns true if this Range contains the specified value.
     pub fn contains(&self, v: &V) -> bool {
-        for segment in self.segments.iter() {
-            match within_bounds(v, segment) {
-                Ordering::Less => return false,
-                Ordering::Equal => return true,
-                Ordering::Greater => (),
-            }
-        }
-        false
+        self.segments
+            .binary_search_by(|segment| {
+                let lower_bound_greater = match segment {
+                    (Excluded(start), _) => v <= start,
+                    (Included(start), _) => v < start,
+                    (Unbounded, _) => false,
+                };
+                if lower_bound_greater {
+                    return Ordering::Greater;
+                }
+                let upper_bound_greater = match segment {
+                    (_, Unbounded) => true,
+                    (_, Included(end)) => v <= end,
+                    (_, Excluded(end)) => v < end,
+                };
+                if upper_bound_greater {
+                    return Ordering::Equal;
+                }
+                Ordering::Less
+            })
+            .is_ok()
     }
 
-    /// Returns true if the this Range contains the specified values.
+    /// Returns true if this Range contains the specified values.
     ///
     /// The `versions` iterator must be sorted.
     /// Functionally equivalent to `versions.map(|v| self.contains(v))`.

From d306d9ce04b78e11f9bec85a6a1efeeacdb81537 Mon Sep 17 00:00:00 2001
From: konstin <konstin@mailbox.org>
Date: Tue, 5 Mar 2024 16:35:03 +0100
Subject: [PATCH 02/15] Avoid cloning and dropping the term in prior_cause

---
 src/internal/incompatibility.rs |  7 ++++--
 src/internal/small_map.rs       | 43 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs
index 2dc6fbde..6f6d8010 100644
--- a/src/internal/incompatibility.rs
+++ b/src/internal/incompatibility.rs
@@ -173,8 +173,11 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> {
         incompatibility_store: &Arena<Self>,
     ) -> Self {
         let kind = Kind::DerivedFrom(incompat, satisfier_cause);
-        let mut package_terms = incompatibility_store[incompat].package_terms.clone();
-        let t1 = package_terms.remove(package).unwrap();
+        // Optimization to avoid cloning and dropping t1
+        let (t1, mut package_terms) = incompatibility_store[incompat]
+            .package_terms
+            .split_one(package)
+            .unwrap();
         let satisfier_cause_terms = &incompatibility_store[satisfier_cause].package_terms;
         package_terms.merge(
             satisfier_cause_terms.iter().filter(|(p, _)| p != &package),
diff --git a/src/internal/small_map.rs b/src/internal/small_map.rs
index 8fc16a06..476af91d 100644
--- a/src/internal/small_map.rs
+++ b/src/internal/small_map.rs
@@ -99,6 +99,49 @@ impl<K: PartialEq + Eq + Hash, V> SmallMap<K, V> {
             }
         };
     }
+
+    /// Returns a reference to the value for one key and a copy of the map without the key.
+    ///
+    /// This is an optimization over the following, where we only need a reference to `t1`. It
+    /// avoids cloning and then drop the ranges in each `prior_cause` call.
+    /// ```ignore
+    /// let mut package_terms = package_terms.clone();
+    //  let t1 = package_terms.remove(package).unwrap();
+    /// ```
+    pub fn split_one(&self, key: &K) -> Option<(&V, Self)>
+    where
+        K: Clone,
+        V: Clone,
+    {
+        match self {
+            Self::Empty => None,
+            Self::One([(k, v)]) => {
+                if k == key {
+                    Some((v, Self::Empty))
+                } else {
+                    None
+                }
+            }
+            Self::Two([(k1, v1), (k2, v2)]) => {
+                if k1 == key {
+                    Some((v1, Self::One([(k2.clone(), v2.clone())])))
+                } else if k2 == key {
+                    Some((v2, Self::One([(k1.clone(), v1.clone())])))
+                } else {
+                    None
+                }
+            }
+            Self::Flexible(map) => {
+                if let Some(value) = map.get(key) {
+                    let mut map = map.clone();
+                    map.remove(key).unwrap();
+                    Some((value, Self::Flexible(map)))
+                } else {
+                    None
+                }
+            }
+        }
+    }
 }
 
 impl<K: Clone + PartialEq + Eq + Hash, V: Clone> SmallMap<K, V> {

From 288dd67930163d40abdd9bd97f2ece988b060349 Mon Sep 17 00:00:00 2001
From: konstin <konstin@mailbox.org>
Date: Thu, 7 Mar 2024 11:24:36 +0100
Subject: [PATCH 03/15] Simplify contains

---
 src/range.rs | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/src/range.rs b/src/range.rs
index 90e6dc56..52d426e2 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -222,27 +222,14 @@ impl<V: Ord> Range<V> {
     }
 
     /// Returns true if this Range contains the specified value.
-    pub fn contains(&self, v: &V) -> bool {
+    pub fn contains(&self, version: &V) -> bool {
         self.segments
             .binary_search_by(|segment| {
-                let lower_bound_greater = match segment {
-                    (Excluded(start), _) => v <= start,
-                    (Included(start), _) => v < start,
-                    (Unbounded, _) => false,
-                };
-                if lower_bound_greater {
-                    return Ordering::Greater;
-                }
-                let upper_bound_greater = match segment {
-                    (_, Unbounded) => true,
-                    (_, Included(end)) => v <= end,
-                    (_, Excluded(end)) => v < end,
-                };
-                if upper_bound_greater {
-                    return Ordering::Equal;
-                }
-                Ordering::Less
+                // We have to reverse because we need the segment wrt to the version, while
+                // within bounds tells us the version wrt to the segment.
+                within_bounds(version, segment).reverse()
             })
+            // An equal interval is one that contains the version
             .is_ok()
     }
 
@@ -328,10 +315,16 @@ impl<V: Ord> Range<V> {
     }
 }
 
-fn within_bounds<V: PartialOrd>(v: &V, segment: &Interval<V>) -> Ordering {
+/// The ordering of the version wrt to the interval.
+/// ```text
+///      |-------|
+///   ^      ^      ^
+///   less   equal  greater
+/// ```
+fn within_bounds<V: PartialOrd>(version: &V, segment: &Interval<V>) -> Ordering {
     let below_lower_bound = match segment {
-        (Excluded(start), _) => v <= start,
-        (Included(start), _) => v < start,
+        (Excluded(start), _) => version <= start,
+        (Included(start), _) => version < start,
         (Unbounded, _) => false,
     };
     if below_lower_bound {
@@ -339,8 +332,8 @@ fn within_bounds<V: PartialOrd>(v: &V, segment: &Interval<V>) -> Ordering {
     }
     let below_upper_bound = match segment {
         (_, Unbounded) => true,
-        (_, Included(end)) => v <= end,
-        (_, Excluded(end)) => v < end,
+        (_, Included(end)) => version <= end,
+        (_, Excluded(end)) => version < end,
     };
     if below_upper_bound {
         return Ordering::Equal;

From 775a5d8326b6e7903677b46be8f543bd3879213a Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Mon, 11 Mar 2024 18:20:08 +0000
Subject: [PATCH 04/15] Specialize union code

---
 src/range.rs | 108 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 104 insertions(+), 4 deletions(-)

diff --git a/src/range.rs b/src/range.rs
index 52d426e2..41ddb10c 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -341,8 +341,10 @@ fn within_bounds<V: PartialOrd>(version: &V, segment: &Interval<V>) -> Ordering
     Ordering::Greater
 }
 
+/// A valid segment is one where at least one version fits between start and end
 fn valid_segment<T: PartialOrd>(start: &Bound<T>, end: &Bound<T>) -> bool {
     match (start, end) {
+        // Singleton interval are allowed
         (Included(s), Included(e)) => s <= e,
         (Included(s), Excluded(e)) => s < e,
         (Excluded(s), Included(e)) => s < e,
@@ -351,6 +353,34 @@ fn valid_segment<T: PartialOrd>(start: &Bound<T>, end: &Bound<T>) -> bool {
     }
 }
 
+/// The end of one interval is before the start of the next one, so they can't be concatenated
+/// into a single interval. The `union` method calling with both intervals and then the intervals
+/// switched. If either is true, the intervals are separate in the union and if both are false, they
+/// are merged.
+/// ```text
+/// True for these two:
+///  |----|
+///                |-----|
+///       ^ end    ^ start
+/// False for these two:
+///  |----|
+///     |-----|
+/// Here it depends: If they both exclude the position they share, there is a version in between
+/// them that blocks concatenation
+///  |----|
+///       |-----|
+/// ```
+fn end_before_start_with_gap<V: PartialOrd>(end: &Bound<V>, start: &Bound<V>) -> bool {
+    match (end, start) {
+        (_, Unbounded) => false,
+        (Unbounded, _) => false,
+        (Included(left), Included(right)) => left < right,
+        (Included(left), Excluded(right)) => left < right,
+        (Excluded(left), Included(right)) => left < right,
+        (Excluded(left), Excluded(right)) => left <= right,
+    }
+}
+
 /// Group adjacent versions locations.
 ///
 /// ```text
@@ -384,10 +414,70 @@ fn group_adjacent_locations(
 impl<V: Ord + Clone> Range<V> {
     /// Computes the union of this `Range` and another.
     pub fn union(&self, other: &Self) -> Self {
-        self.complement()
-            .intersection(&other.complement())
-            .complement()
-            .check_invariants()
+        let mut output: SmallVec<Interval<V>> = SmallVec::empty();
+        let mut accumulator: Option<(&Bound<_>, &Bound<_>)> = None;
+        let mut left_iter = self.segments.iter().peekable();
+        let mut right_iter = other.segments.iter().peekable();
+        loop {
+            let smaller_interval = match (left_iter.peek(), right_iter.peek()) {
+                (Some((left_start, left_end)), Some((right_start, right_end))) => {
+                    let left_start_is_smaller = match (left_start, right_start) {
+                        (Unbounded, _) => true,
+                        (_, Unbounded) => false,
+                        (Included(l), Included(r)) => l <= r,
+                        (Excluded(l), Excluded(r)) => l <= r,
+                        (Included(l), Excluded(r)) => l <= r,
+                        (Excluded(l), Included(r)) => l < r,
+                    };
+
+                    if left_start_is_smaller {
+                        left_iter.next();
+                        (left_start, left_end)
+                    } else {
+                        right_iter.next();
+                        (right_start, right_end)
+                    }
+                }
+                (Some((left_start, left_end)), None) => {
+                    left_iter.next();
+                    (left_start, left_end)
+                }
+                (None, Some((right_start, right_end))) => {
+                    right_iter.next();
+                    (right_start, right_end)
+                }
+                (None, None) => break,
+            };
+
+            if let Some(accumulator_) = accumulator {
+                if end_before_start_with_gap(accumulator_.1, smaller_interval.0) {
+                    output.push((accumulator_.0.clone(), accumulator_.1.clone()));
+                    accumulator = Some(smaller_interval);
+                } else {
+                    let accumulator_end = match (accumulator_.1, smaller_interval.1) {
+                        (_, Unbounded) | (Unbounded, _) => &Unbounded,
+                        (Included(l), Excluded(r) | Included(r)) if l == r => accumulator_.1,
+                        (Excluded(l) | Included(l), Included(r)) if l == r => smaller_interval.1,
+                        (Included(l) | Excluded(l), Included(r) | Excluded(r)) => {
+                            if l > r {
+                                accumulator_.1
+                            } else {
+                                smaller_interval.1
+                            }
+                        }
+                    };
+                    accumulator = Some((accumulator_.0, accumulator_end));
+                }
+            } else {
+                accumulator = Some(smaller_interval)
+            }
+        }
+
+        if let Some(accumulator) = accumulator {
+            output.push((accumulator.0.clone(), accumulator.1.clone()));
+        }
+
+        Self { segments: output }.check_invariants()
     }
 
     /// Computes the intersection of two sets of versions.
@@ -767,6 +857,16 @@ pub mod tests {
             assert_eq!(r1.union(&r2).contains(&version), r1.contains(&version) || r2.contains(&version));
         }
 
+        #[test]
+        fn union_through_intersection(r1 in strategy(), r2 in strategy()) {
+            let union_def = r1
+                .complement()
+                .intersection(&r2.complement())
+                .complement()
+                .check_invariants();
+            assert_eq!(r1.union(&r2), union_def);
+        }
+
         // Testing contains --------------------------------
 
         #[test]

From 1c4479de2bb183d3c821d26661f8aef3c84eaf0e Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Mon, 11 Mar 2024 18:22:39 +0000
Subject: [PATCH 05/15] remove a redundant check and reuse helper

---
 src/range.rs | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/src/range.rs b/src/range.rs
index 41ddb10c..69aa9902 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -298,14 +298,7 @@ impl<V: Ord> Range<V> {
     fn check_invariants(self) -> Self {
         if cfg!(debug_assertions) {
             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!(),
-                }
+                assert!(end_before_start_with_gap(&p[0].1, &p[1].0));
             }
             for (s, e) in self.segments.iter() {
                 assert!(valid_segment(s, e));
@@ -457,7 +450,6 @@ impl<V: Ord + Clone> Range<V> {
                     let accumulator_end = match (accumulator_.1, smaller_interval.1) {
                         (_, Unbounded) | (Unbounded, _) => &Unbounded,
                         (Included(l), Excluded(r) | Included(r)) if l == r => accumulator_.1,
-                        (Excluded(l) | Included(l), Included(r)) if l == r => smaller_interval.1,
                         (Included(l) | Excluded(l), Included(r) | Excluded(r)) => {
                             if l > r {
                                 accumulator_.1

From 78f67ebaf69875593917cf5070858a7d07094ff9 Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Mon, 11 Mar 2024 18:27:41 +0000
Subject: [PATCH 06/15] Specialize is_disjoint

---
 src/range.rs | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/src/range.rs b/src/range.rs
index 69aa9902..2ca16471 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -539,6 +539,38 @@ impl<V: Ord + Clone> Range<V> {
         Self { segments: output }.check_invariants()
     }
 
+    /// Return true is there can be no `V` so that `V` is contained in both `self` and `other`.
+    ///
+    /// Note that we don't know that set of all existing `V`s here, so we only check if the segments
+    /// are disjoint, not if no version is contained in both.
+    pub fn is_disjoint(&self, other: &Self) -> bool {
+        let end_before_start = |end, start| match (end, start) {
+            (_, Unbounded) => false,
+            (Unbounded, _) => false,
+            (Included(left), Included(right)) => left < right,
+            (Included(left), Excluded(right)) => left <= right,
+            (Excluded(left), Included(right)) => left <= right,
+            (Excluded(left), Excluded(right)) => left <= right,
+        };
+
+        // The operation is symmetric
+        let mut left_iter = self.segments.iter().peekable();
+        let mut right_iter = other.segments.iter().peekable();
+
+        while let (Some(left), Some(right)) = (left_iter.peek(), right_iter.peek()) {
+            if end_before_start(left.end_bound(), right.start_bound()) {
+                left_iter.next();
+            } else if end_before_start(right.end_bound(), left.start_bound()) {
+                right_iter.next();
+            } else {
+                return false;
+            }
+        }
+
+        // The remaining element(s) can't intersect anymore
+        true
+    }
+
     /// Returns a simpler Range that contains the same versions
     ///
     /// For every one of the Versions provided in versions the existing range and

From 82d906b3af27838f2601ce3a9198ca26133807b2 Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Mon, 11 Mar 2024 18:49:11 +0000
Subject: [PATCH 07/15] simplify and add tests for is_disjoint

---
 src/range.rs | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/src/range.rs b/src/range.rs
index 2ca16471..56bb891b 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -301,7 +301,7 @@ impl<V: Ord> Range<V> {
                 assert!(end_before_start_with_gap(&p[0].1, &p[1].0));
             }
             for (s, e) in self.segments.iter() {
-                assert!(valid_segment(s, e));
+                assert!(valid_segment(&s, &e));
             }
         }
         self
@@ -513,7 +513,7 @@ impl<V: Ord + Clone> Range<V> {
             // But, we already know that the segments in our input are valid.
             // So we do not need to check if the `start`  from the input `end` came from is smaller then `end`.
             // If the `other_start` is larger than end, then the intersection will be invalid.
-            if !valid_segment(other_start, end) {
+            if !valid_segment(&other_start, &end) {
                 // Note: We can call `this_iter.next_if(!valid_segment(other_start, this_end))` in a loop.
                 // But the checks make it slower for the benchmarked inputs.
                 continue;
@@ -539,28 +539,19 @@ impl<V: Ord + Clone> Range<V> {
         Self { segments: output }.check_invariants()
     }
 
-    /// Return true is there can be no `V` so that `V` is contained in both `self` and `other`.
+    /// Return true if there can be no `V` so that `V` is contained in both `self` and `other`.
     ///
     /// Note that we don't know that set of all existing `V`s here, so we only check if the segments
     /// are disjoint, not if no version is contained in both.
     pub fn is_disjoint(&self, other: &Self) -> bool {
-        let end_before_start = |end, start| match (end, start) {
-            (_, Unbounded) => false,
-            (Unbounded, _) => false,
-            (Included(left), Included(right)) => left < right,
-            (Included(left), Excluded(right)) => left <= right,
-            (Excluded(left), Included(right)) => left <= right,
-            (Excluded(left), Excluded(right)) => left <= right,
-        };
-
         // The operation is symmetric
         let mut left_iter = self.segments.iter().peekable();
         let mut right_iter = other.segments.iter().peekable();
 
-        while let (Some(left), Some(right)) = (left_iter.peek(), right_iter.peek()) {
-            if end_before_start(left.end_bound(), right.start_bound()) {
+        while let Some((left, right)) = left_iter.peek().zip(right_iter.peek()) {
+            if !valid_segment(&right.start_bound(), &left.end_bound()) {
                 left_iter.next();
-            } else if end_before_start(right.end_bound(), left.start_bound()) {
+            } else if !valid_segment(&left.start_bound(), &right.end_bound()) {
                 right_iter.next();
             } else {
                 return false;
@@ -881,6 +872,12 @@ pub mod tests {
             assert_eq!(r1.union(&r2).contains(&version), r1.contains(&version) || r2.contains(&version));
         }
 
+        #[test]
+        fn is_disjoint_through_intersection(r1 in strategy(), r2 in strategy()) {
+            let disjoint_def = r1.intersection(&r2) == Range::empty();
+            assert_eq!(r1.is_disjoint(&r2), disjoint_def);
+        }
+
         #[test]
         fn union_through_intersection(r1 in strategy(), r2 in strategy()) {
             let union_def = r1

From f6498f878857acedb52340f05fe211a6d1c07cf1 Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Mon, 11 Mar 2024 18:55:02 +0000
Subject: [PATCH 08/15] Specialize subset_of

---
 src/range.rs | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/src/range.rs b/src/range.rs
index 56bb891b..f36e53f6 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -374,6 +374,19 @@ fn end_before_start_with_gap<V: PartialOrd>(end: &Bound<V>, start: &Bound<V>) ->
     }
 }
 
+/// The end of one interval is before the start of the next one, meaning they don't share any
+/// version.
+fn end_before_start<V: PartialOrd>(end: Bound<&V>, start: Bound<&V>) -> bool {
+    match (end, start) {
+        (_, Unbounded) => false,
+        (Unbounded, _) => false,
+        (Included(left), Included(right)) => left < right,
+        (Included(left), Excluded(right)) => left <= right,
+        (Excluded(left), Included(right)) => left <= right,
+        (Excluded(left), Excluded(right)) => left <= right,
+    }
+}
+
 /// Group adjacent versions locations.
 ///
 /// ```text
@@ -562,6 +575,60 @@ impl<V: Ord + Clone> Range<V> {
         true
     }
 
+    /// Return true if any `V` that is contained in `self` is also contained in `other`.
+    ///
+    /// Note that we don't know that set of all existing `V`s here, so we only check if all
+    /// segments `self` are contained in a segment of `other`.
+    fn subset_of(&self, other: &Self) -> bool {
+        let mut containing_iter = other.segments.iter();
+        let mut subset_iter = self.segments.iter();
+        let Some(mut containing_elem) = containing_iter.next() else {
+            // As long as we have subset elements, we need containing elements
+            return subset_iter.next().is_none();
+        };
+
+        for subset_elem in subset_iter {
+            // Check if the current containing element ends before the subset element.
+            // There needs to be another containing element for our subset element in this case.
+            while end_before_start(containing_elem.end_bound(), subset_elem.start_bound()) {
+                if let Some(containing_elem_) = containing_iter.next() {
+                    containing_elem = containing_elem_;
+                } else {
+                    return false;
+                };
+            }
+
+            let start_contained = match (containing_elem.start_bound(), subset_elem.start_bound()) {
+                (Unbounded, _) => true,
+                (Included(_) | Excluded(_), Unbounded) => false,
+                // This is the only case where the subset is bound is "wider" than containing bound ...
+                (Excluded(left), Included(right)) => left < right,
+                // ... while in the other cases they can share the point
+                (Included(left), Included(right)) => left <= right,
+                (Included(left), Excluded(right)) => left <= right,
+                (Excluded(left), Excluded(right)) => left <= right,
+            };
+
+            let end_contained = match (subset_elem.end_bound(), containing_elem.end_bound()) {
+                (_, Unbounded) => true,
+                (Unbounded, Included(_) | Excluded(_)) => false,
+                // This is the only case where the subset is bound is "wider" than containing bound ...
+                (Included(left), Excluded(right)) => left < right,
+                // ... while in the other cases they can share the point
+                (Included(left), Included(right)) => left <= right,
+                (Excluded(left), Included(right)) => left <= right,
+                (Excluded(left), Excluded(right)) => left <= right,
+            };
+
+            if !(start_contained && end_contained) {
+                // This subset element is not contained
+                return false;
+            }
+        }
+
+        true
+    }
+
     /// Returns a simpler Range that contains the same versions
     ///
     /// For every one of the Versions provided in versions the existing range and

From ea90799afd4e3fd029c08ddedb2e8ebbbdbf5f17 Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Mon, 11 Mar 2024 19:21:05 +0000
Subject: [PATCH 09/15] simplify and add tests for subset_of

---
 src/range.rs | 89 ++++++++++++++++++++++------------------------------
 1 file changed, 38 insertions(+), 51 deletions(-)

diff --git a/src/range.rs b/src/range.rs
index f36e53f6..1410cf29 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -374,16 +374,25 @@ fn end_before_start_with_gap<V: PartialOrd>(end: &Bound<V>, start: &Bound<V>) ->
     }
 }
 
-/// The end of one interval is before the start of the next one, meaning they don't share any
-/// version.
-fn end_before_start<V: PartialOrd>(end: Bound<&V>, start: Bound<&V>) -> bool {
-    match (end, start) {
+fn left_start_is_smaller<V: PartialOrd>(left: Bound<V>, right: Bound<V>) -> bool {
+    match (left, right) {
+        (Unbounded, _) => true,
         (_, Unbounded) => false,
+        (Included(l), Included(r)) => l <= r,
+        (Excluded(l), Excluded(r)) => l <= r,
+        (Included(l), Excluded(r)) => l <= r,
+        (Excluded(l), Included(r)) => l < r,
+    }
+}
+
+fn left_end_is_smaller<V: PartialOrd>(left: Bound<V>, right: Bound<V>) -> bool {
+    match (left, right) {
+        (_, Unbounded) => true,
         (Unbounded, _) => false,
-        (Included(left), Included(right)) => left < right,
-        (Included(left), Excluded(right)) => left <= right,
-        (Excluded(left), Included(right)) => left <= right,
-        (Excluded(left), Excluded(right)) => left <= right,
+        (Included(l), Included(r)) => l <= r,
+        (Excluded(l), Excluded(r)) => l <= r,
+        (Excluded(l), Included(r)) => l <= r,
+        (Included(l), Excluded(r)) => l < r,
     }
 }
 
@@ -427,16 +436,7 @@ impl<V: Ord + Clone> Range<V> {
         loop {
             let smaller_interval = match (left_iter.peek(), right_iter.peek()) {
                 (Some((left_start, left_end)), Some((right_start, right_end))) => {
-                    let left_start_is_smaller = match (left_start, right_start) {
-                        (Unbounded, _) => true,
-                        (_, Unbounded) => false,
-                        (Included(l), Included(r)) => l <= r,
-                        (Excluded(l), Excluded(r)) => l <= r,
-                        (Included(l), Excluded(r)) => l <= r,
-                        (Excluded(l), Included(r)) => l < r,
-                    };
-
-                    if left_start_is_smaller {
+                    if left_start_is_smaller(left_start.as_ref(), right_start.as_ref()) {
                         left_iter.next();
                         (left_start, left_end)
                     } else {
@@ -500,15 +500,7 @@ impl<V: Ord + Clone> Range<V> {
             left_iter.peek().zip(right_iter.peek())
         {
             // The next smallest `end` value is going to come from one of the inputs.
-            let left_end_is_smaller = match (left_end, right_end) {
-                (Included(l), Included(r))
-                | (Excluded(l), Excluded(r))
-                | (Excluded(l), Included(r)) => l <= r,
-
-                (Included(l), Excluded(r)) => l < r,
-                (_, Unbounded) => true,
-                (Unbounded, _) => false,
-            };
+            let left_end_is_smaller = left_end_is_smaller(left_end.as_ref(), right_end.as_ref());
             // Now that we are processing `end` we will never have to process any segment smaller than that.
             // We can ensure that the input that `end` came from is larger than `end` by advancing it one step.
             // `end` is the smaller available input, so we know the other input is already larger than `end`.
@@ -579,7 +571,7 @@ impl<V: Ord + Clone> Range<V> {
     ///
     /// Note that we don't know that set of all existing `V`s here, so we only check if all
     /// segments `self` are contained in a segment of `other`.
-    fn subset_of(&self, other: &Self) -> bool {
+    pub fn subset_of(&self, other: &Self) -> bool {
         let mut containing_iter = other.segments.iter();
         let mut subset_iter = self.segments.iter();
         let Some(mut containing_elem) = containing_iter.next() else {
@@ -590,7 +582,7 @@ impl<V: Ord + Clone> Range<V> {
         for subset_elem in subset_iter {
             // Check if the current containing element ends before the subset element.
             // There needs to be another containing element for our subset element in this case.
-            while end_before_start(containing_elem.end_bound(), subset_elem.start_bound()) {
+            while !valid_segment(&subset_elem.start_bound(), &containing_elem.end_bound()) {
                 if let Some(containing_elem_) = containing_iter.next() {
                     containing_elem = containing_elem_;
                 } else {
@@ -598,30 +590,19 @@ impl<V: Ord + Clone> Range<V> {
                 };
             }
 
-            let start_contained = match (containing_elem.start_bound(), subset_elem.start_bound()) {
-                (Unbounded, _) => true,
-                (Included(_) | Excluded(_), Unbounded) => false,
-                // This is the only case where the subset is bound is "wider" than containing bound ...
-                (Excluded(left), Included(right)) => left < right,
-                // ... while in the other cases they can share the point
-                (Included(left), Included(right)) => left <= right,
-                (Included(left), Excluded(right)) => left <= right,
-                (Excluded(left), Excluded(right)) => left <= right,
-            };
+            let start_contained =
+                left_start_is_smaller(containing_elem.start_bound(), subset_elem.start_bound());
 
-            let end_contained = match (subset_elem.end_bound(), containing_elem.end_bound()) {
-                (_, Unbounded) => true,
-                (Unbounded, Included(_) | Excluded(_)) => false,
-                // This is the only case where the subset is bound is "wider" than containing bound ...
-                (Included(left), Excluded(right)) => left < right,
-                // ... while in the other cases they can share the point
-                (Included(left), Included(right)) => left <= right,
-                (Excluded(left), Included(right)) => left <= right,
-                (Excluded(left), Excluded(right)) => left <= right,
-            };
+            if !start_contained {
+                // The start element is not contained
+                return false;
+            }
 
-            if !(start_contained && end_contained) {
-                // This subset element is not contained
+            let end_contained =
+                left_end_is_smaller(subset_elem.end_bound(), containing_elem.end_bound());
+
+            if !end_contained {
+                // The end element is not contained
                 return false;
             }
         }
@@ -945,6 +926,12 @@ pub mod tests {
             assert_eq!(r1.is_disjoint(&r2), disjoint_def);
         }
 
+        #[test]
+        fn subset_of_through_intersection(r1 in strategy(), r2 in strategy()) {
+            let disjoint_def = r1.intersection(&r2) == r1;
+            assert_eq!(r1.subset_of(&r2), disjoint_def);
+        }
+
         #[test]
         fn union_through_intersection(r1 in strategy(), r2 in strategy()) {
             let union_def = r1

From 776ed8aead697a8eff99373aef6d24e7f189b2bb Mon Sep 17 00:00:00 2001
From: konstin <konstin@mailbox.org>
Date: Mon, 11 Mar 2024 19:21:51 +0000
Subject: [PATCH 10/15] Specialize range operations for better performance

---
 src/internal/partial_solution.rs |  2 +-
 src/range.rs                     |  8 ++++++
 src/term.rs                      | 47 +++++++++++++++++++++++++++++---
 src/version_set.rs               | 10 +++++++
 4 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs
index cf1d24db..9a4c8841 100644
--- a/src/internal/partial_solution.rs
+++ b/src/internal/partial_solution.rs
@@ -501,7 +501,7 @@ impl<P: Package, VS: VersionSet> PackageAssignments<P, VS> {
         let idx = self
             .dated_derivations
             .as_slice()
-            .partition_point(|dd| dd.accumulated_intersection.intersection(start_term) != empty);
+            .partition_point(|dd| !dd.accumulated_intersection.is_disjoint(start_term));
         if let Some(dd) = self.dated_derivations.get(idx) {
             debug_assert_eq!(dd.accumulated_intersection.intersection(start_term), empty);
             return (Some(dd.cause), dd.global_index, dd.decision_level);
diff --git a/src/range.rs b/src/range.rs
index 1410cf29..0e7abd02 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -709,6 +709,14 @@ impl<T: Debug + Display + Clone + Eq + Ord> VersionSet for Range<T> {
     fn union(&self, other: &Self) -> Self {
         Range::union(self, other)
     }
+
+    fn is_disjoint(&self, other: &Self) -> bool {
+        Range::is_disjoint(self, other)
+    }
+
+    fn subset_of(&self, other: &Self) -> bool {
+        Range::subset_of(self, other)
+    }
 }
 
 // REPORT ######################################################################
diff --git a/src/term.rs b/src/term.rs
index 2974da62..67bc9b0f 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -6,7 +6,14 @@
 use crate::version_set::VersionSet;
 use std::fmt::{self, Display};
 
-///  A positive or negative expression regarding a set of versions.
+/// A positive or negative expression regarding a set of versions.
+///
+/// If a version is selected then `Positive(r)` and `Negative(r.complement())` are equivalent, but
+/// they have different semantics when no version is selected. A `Positive` term in the partial
+/// solution requires a version to be selected. But a `Negative` term allows for a solution that
+/// does not have that package selected. Specifically, `Positive(VS::empty())` means that there was
+/// a conflict, we need to select a version for the package but can't pick any, while
+/// `Negative(VS::full())` would mean it is fine as long as we don't select the package.
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub enum Term<VS: VersionSet> {
     /// For example, "1.0.0 <= v < 2.0.0" is a positive expression
@@ -84,7 +91,8 @@ impl<VS: VersionSet> Term<VS> {
 /// Set operations with terms.
 impl<VS: VersionSet> Term<VS> {
     /// Compute the intersection of two terms.
-    /// If at least one term is positive, the intersection is also positive.
+    ///
+    /// The intersection is positive is at least one of the two terms is positive.
     pub(crate) fn intersection(&self, other: &Self) -> Self {
         match (self, other) {
             (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.intersection(r2)),
@@ -98,10 +106,34 @@ impl<VS: VersionSet> Term<VS> {
         }
     }
 
+    /// Check whether two terms are mutually exclusive.
+    ///
+    /// An optimization for the native implementation of checking whether the intersection of two sets is empty.
+    pub(crate) fn is_disjoint(&self, other: &Self) -> bool {
+        match (self, other) {
+            (Self::Positive(r1), Self::Positive(r2)) => r1.is_disjoint(r2),
+            (Self::Negative(r1), Self::Negative(r2)) => r1 == &VS::empty() && r2 == &VS::empty(),
+            // If the positive term is a subset of the negative term, it lies fully in the region that the negative
+            // term excludes.
+            (Self::Positive(r1), Self::Negative(r2)) => r1.subset_of(r2),
+            // Inversely, if there is a region outside the negative, they overlap in this region.
+            (Self::Negative(r1), Self::Positive(r2)) => r2.subset_of(r1),
+        }
+    }
+
     /// Compute the union of two terms.
     /// If at least one term is negative, the union is also negative.
     pub(crate) fn union(&self, other: &Self) -> Self {
-        (self.negate().intersection(&other.negate())).negate()
+        match (self, other) {
+            (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.union(r2)),
+            (Self::Positive(r1), Self::Negative(r2)) => {
+                Self::Negative(r1.union(&r2.complement()).complement())
+            }
+            (Self::Negative(r1), Self::Positive(r2)) => {
+                Self::Negative(r1.complement().union(r2).complement())
+            }
+            (Self::Negative(r1), Self::Negative(r2)) => Self::Negative(r1.intersection(r2)),
+        }
     }
 
     /// Indicate if this term is a subset of another term.
@@ -200,7 +232,6 @@ pub mod tests {
             crate::range::tests::strategy().prop_map(Term::Negative),
         ]
     }
-
     proptest! {
 
         // Testing relation --------------------------------
@@ -217,5 +248,13 @@ pub mod tests {
             }
         }
 
+        /// Ensure that we don't wrongly convert between positive and negative ranges
+        #[test]
+        fn positive_negative(term1 in strategy(), term2 in strategy()) {
+            let intersection_positive = term1.is_positive() || term2.is_positive();
+            let union_positive = term1.is_positive() && term2.is_positive();
+            assert_eq!(term1.intersection(&term2).is_positive(), intersection_positive);
+            assert_eq!(term1.union(&term2).is_positive(), union_positive);
+        }
     }
 }
diff --git a/src/version_set.rs b/src/version_set.rs
index 501ec700..fae22948 100644
--- a/src/version_set.rs
+++ b/src/version_set.rs
@@ -57,4 +57,14 @@ pub trait VersionSet: Debug + Display + Clone + Eq {
             .intersection(&other.complement())
             .complement()
     }
+
+    /// Whether the range have no overlapping segmets
+    fn is_disjoint(&self, other: &Self) -> bool {
+        self.intersection(other) == Self::empty()
+    }
+
+    /// Whether all range of `self` are contained in `other`
+    fn subset_of(&self, other: &Self) -> bool {
+        self == &self.intersection(other)
+    }
 }

From 2c56fee516aff2552d9402691036b49eb36a8fef Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Mon, 11 Mar 2024 20:08:59 +0000
Subject: [PATCH 11/15] add tests and use in more places

---
 src/term.rs | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/src/term.rs b/src/term.rs
index 67bc9b0f..327ccb1e 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -92,7 +92,7 @@ impl<VS: VersionSet> Term<VS> {
 impl<VS: VersionSet> Term<VS> {
     /// Compute the intersection of two terms.
     ///
-    /// The intersection is positive is at least one of the two terms is positive.
+    /// The intersection is positive if at least one of the two terms is positive.
     pub(crate) fn intersection(&self, other: &Self) -> Self {
         match (self, other) {
             (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.intersection(r2)),
@@ -139,9 +139,13 @@ impl<VS: VersionSet> Term<VS> {
     /// Indicate if this term is a subset of another term.
     /// Just like for sets, we say that t1 is a subset of t2
     /// if and only if t1 ∩ t2 = t1.
-    #[cfg(test)]
     pub(crate) fn subset_of(&self, other: &Self) -> bool {
-        self == &self.intersection(other)
+        match (self, other) {
+            (Self::Positive(r1), Self::Positive(r2)) => r1.subset_of(r2),
+            (Self::Positive(r1), Self::Negative(r2)) => r1.subset_of(&r2.complement()),
+            (Self::Negative(_), Self::Positive(_)) => false,
+            (Self::Negative(r1), Self::Negative(r2)) => r2.subset_of(&r1),
+        }
     }
 }
 
@@ -190,10 +194,9 @@ impl<VS: VersionSet> Term<VS> {
     /// Check if a set of terms satisfies or contradicts a given term.
     /// Otherwise the relation is inconclusive.
     pub(crate) fn relation_with(&self, other_terms_intersection: &Self) -> Relation {
-        let full_intersection = self.intersection(other_terms_intersection);
-        if &full_intersection == other_terms_intersection {
+        if other_terms_intersection.subset_of(self) {
             Relation::Satisfied
-        } else if full_intersection == Self::empty() {
+        } else if self.is_disjoint(other_terms_intersection) {
             Relation::Contradicted
         } else {
             Relation::Inconclusive
@@ -256,5 +259,26 @@ pub mod tests {
             assert_eq!(term1.intersection(&term2).is_positive(), intersection_positive);
             assert_eq!(term1.union(&term2).is_positive(), union_positive);
         }
+
+        #[test]
+        fn is_disjoint_through_intersection(r1 in strategy(), r2 in strategy()) {
+            let disjoint_def = r1.intersection(&r2) == Term::empty();
+            assert_eq!(r1.is_disjoint(&r2), disjoint_def);
+        }
+
+        #[test]
+        fn subset_of_through_intersection(r1 in strategy(), r2 in strategy()) {
+            let disjoint_def = r1.intersection(&r2) == r1;
+            assert_eq!(r1.subset_of(&r2), disjoint_def);
+        }
+
+        #[test]
+        fn union_through_intersection(r1 in strategy(), r2 in strategy()) {
+            let union_def = r1
+                .negate()
+                .intersection(&r2.negate())
+                .negate();
+            assert_eq!(r1.union(&r2), union_def);
+        }
     }
 }

From 4fc3a500b1a61fd31b540214af3e2b475ba5a7bb Mon Sep 17 00:00:00 2001
From: konstin <konstin@mailbox.org>
Date: Mon, 11 Mar 2024 21:41:34 +0100
Subject: [PATCH 12/15] Fix clippy lints

---
 src/range.rs | 4 ++--
 src/term.rs  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/range.rs b/src/range.rs
index 0e7abd02..4498e2f9 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -301,7 +301,7 @@ impl<V: Ord> Range<V> {
                 assert!(end_before_start_with_gap(&p[0].1, &p[1].0));
             }
             for (s, e) in self.segments.iter() {
-                assert!(valid_segment(&s, &e));
+                assert!(valid_segment(s, e));
             }
         }
         self
@@ -518,7 +518,7 @@ impl<V: Ord + Clone> Range<V> {
             // But, we already know that the segments in our input are valid.
             // So we do not need to check if the `start`  from the input `end` came from is smaller then `end`.
             // If the `other_start` is larger than end, then the intersection will be invalid.
-            if !valid_segment(&other_start, &end) {
+            if !valid_segment(other_start, end) {
                 // Note: We can call `this_iter.next_if(!valid_segment(other_start, this_end))` in a loop.
                 // But the checks make it slower for the benchmarked inputs.
                 continue;
diff --git a/src/term.rs b/src/term.rs
index 327ccb1e..6d7f7e6f 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -144,7 +144,7 @@ impl<VS: VersionSet> Term<VS> {
             (Self::Positive(r1), Self::Positive(r2)) => r1.subset_of(r2),
             (Self::Positive(r1), Self::Negative(r2)) => r1.subset_of(&r2.complement()),
             (Self::Negative(_), Self::Positive(_)) => false,
-            (Self::Negative(r1), Self::Negative(r2)) => r2.subset_of(&r1),
+            (Self::Negative(r1), Self::Negative(r2)) => r2.subset_of(r1),
         }
     }
 }

From a11f1d7fbae8de326fcb22a182f51941f3e87e31 Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Wed, 13 Mar 2024 15:46:54 +0000
Subject: [PATCH 13/15] replace a complement with is_disjoint

---
 src/term.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/term.rs b/src/term.rs
index 6d7f7e6f..c9fea4f7 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -142,7 +142,7 @@ impl<VS: VersionSet> Term<VS> {
     pub(crate) fn subset_of(&self, other: &Self) -> bool {
         match (self, other) {
             (Self::Positive(r1), Self::Positive(r2)) => r1.subset_of(r2),
-            (Self::Positive(r1), Self::Negative(r2)) => r1.subset_of(&r2.complement()),
+            (Self::Positive(r1), Self::Negative(r2)) => r1.is_disjoint(r2),
             (Self::Negative(_), Self::Positive(_)) => false,
             (Self::Negative(r1), Self::Negative(r2)) => r2.subset_of(r1),
         }

From 72c12b889411708994711a05fcbedeb7f91054ea Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Wed, 13 Mar 2024 16:05:42 +0000
Subject: [PATCH 14/15] one fewer complement using intersection

---
 src/term.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/term.rs b/src/term.rs
index c9fea4f7..643de031 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -127,10 +127,10 @@ impl<VS: VersionSet> Term<VS> {
         match (self, other) {
             (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.union(r2)),
             (Self::Positive(r1), Self::Negative(r2)) => {
-                Self::Negative(r1.union(&r2.complement()).complement())
+                Self::Negative(r1.complement().intersection(r2))
             }
             (Self::Negative(r1), Self::Positive(r2)) => {
-                Self::Negative(r1.complement().union(r2).complement())
+                Self::Negative(r1.intersection(&r2.complement()))
             }
             (Self::Negative(r1), Self::Negative(r2)) => Self::Negative(r1.intersection(r2)),
         }

From dc1fc6e0594e0622ffdd227af3ac90ef9fe967b0 Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Wed, 13 Mar 2024 16:00:56 +0000
Subject: [PATCH 15/15] use the symmetry of set functions

---
 src/term.rs | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/src/term.rs b/src/term.rs
index 643de031..55ac7518 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -96,11 +96,8 @@ impl<VS: VersionSet> Term<VS> {
     pub(crate) fn intersection(&self, other: &Self) -> Self {
         match (self, other) {
             (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.intersection(r2)),
-            (Self::Positive(r1), Self::Negative(r2)) => {
-                Self::Positive(r1.intersection(&r2.complement()))
-            }
-            (Self::Negative(r1), Self::Positive(r2)) => {
-                Self::Positive(r1.complement().intersection(r2))
+            (Self::Positive(p), Self::Negative(n)) | (Self::Negative(n), Self::Positive(p)) => {
+                Self::Positive(n.complement().intersection(p))
             }
             (Self::Negative(r1), Self::Negative(r2)) => Self::Negative(r1.union(r2)),
         }
@@ -115,9 +112,9 @@ impl<VS: VersionSet> Term<VS> {
             (Self::Negative(r1), Self::Negative(r2)) => r1 == &VS::empty() && r2 == &VS::empty(),
             // If the positive term is a subset of the negative term, it lies fully in the region that the negative
             // term excludes.
-            (Self::Positive(r1), Self::Negative(r2)) => r1.subset_of(r2),
-            // Inversely, if there is a region outside the negative, they overlap in this region.
-            (Self::Negative(r1), Self::Positive(r2)) => r2.subset_of(r1),
+            (Self::Positive(p), Self::Negative(n)) | (Self::Negative(n), Self::Positive(p)) => {
+                p.subset_of(n)
+            }
         }
     }
 
@@ -126,11 +123,8 @@ impl<VS: VersionSet> Term<VS> {
     pub(crate) fn union(&self, other: &Self) -> Self {
         match (self, other) {
             (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.union(r2)),
-            (Self::Positive(r1), Self::Negative(r2)) => {
-                Self::Negative(r1.complement().intersection(r2))
-            }
-            (Self::Negative(r1), Self::Positive(r2)) => {
-                Self::Negative(r1.intersection(&r2.complement()))
+            (Self::Positive(p), Self::Negative(n)) | (Self::Negative(n), Self::Positive(p)) => {
+                Self::Negative(p.complement().intersection(n))
             }
             (Self::Negative(r1), Self::Negative(r2)) => Self::Negative(r1.intersection(r2)),
         }